From aaeba691d1d130d9915f444735f3c4421ddb1c19 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Mon, 14 Jun 2021 16:07:30 -0500 Subject: [PATCH] Git: don't expand envvars in Gitpython - Gitpython expands envvars by default We execute gitpython outside the containers. Currently there isn't a way to exploit this vulnerability (looks like gitpython is aware of this and may change the default to false any time https://github.com/gitpython-developers/GitPython/blob/617c09e70bfd54af1c88b4d2c892b8d287747542/git/repo/base.py#L142-L143) - In some places we were executing git commands outside the container (to get the commit), let's always use the current env (docker in production). --- readthedocs/doc_builder/backends/mkdocs.py | 12 +++++++++--- readthedocs/doc_builder/backends/sphinx.py | 10 +++++++++- readthedocs/projects/tasks.py | 2 +- readthedocs/rtd_tests/tests/test_doc_builder.py | 4 ++-- readthedocs/vcs_support/backends/git.py | 17 ++++++++++------- 5 files changed, 31 insertions(+), 14 deletions(-) diff --git a/readthedocs/doc_builder/backends/mkdocs.py b/readthedocs/doc_builder/backends/mkdocs.py index 8569f588b9c..76659812c30 100644 --- a/readthedocs/doc_builder/backends/mkdocs.py +++ b/readthedocs/doc_builder/backends/mkdocs.py @@ -4,7 +4,6 @@ .. _MkDocs: http://www.mkdocs.org/ """ -import json import logging import os @@ -17,7 +16,6 @@ from readthedocs.projects.constants import MKDOCS, MKDOCS_HTML from readthedocs.projects.models import Feature - log = logging.getLogger(__name__) @@ -235,6 +233,14 @@ def generate_rtd_data(self, docs_dir, mkdocs_config): # http://www.mkdocs.org/user-guide/configuration/#google_analytics analytics_code = mkdocs_config['google_analytics'][0] + commit = ( + self.version.project.vcs_repo( + version=self.version.slug, + environment=self.build_env, + ) + .commit, + ) + # Will be available in the JavaScript as READTHEDOCS_DATA. readthedocs_data = { 'project': self.version.project.slug, @@ -248,7 +254,7 @@ def generate_rtd_data(self, docs_dir, mkdocs_config): 'source_suffix': '.md', 'api_host': settings.PUBLIC_API_URL, 'ad_free': not self.project.show_advertising, - 'commit': self.version.project.vcs_repo(self.version.slug).commit, + 'commit': commit, 'global_analytics_code': ( None if self.project.analytics_disabled else settings.GLOBAL_ANALYTICS_CODE ), diff --git a/readthedocs/doc_builder/backends/sphinx.py b/readthedocs/doc_builder/backends/sphinx.py index 800e81359d4..c315420b942 100644 --- a/readthedocs/doc_builder/backends/sphinx.py +++ b/readthedocs/doc_builder/backends/sphinx.py @@ -160,6 +160,14 @@ def get_config_params(self): if self.version.is_external: vcs_url = self.version.vcs_url + commit = ( + self.project.vcs_repo( + version=self.version.slug, + environment=self.build_env, + ) + .commit + ) + data = { 'html_theme': 'sphinx_rtd_theme', 'html_theme_import': 'sphinx_rtd_theme', @@ -169,7 +177,7 @@ def get_config_params(self): 'settings': settings, 'conf_py_path': conf_py_path, 'api_host': settings.PUBLIC_API_URL, - 'commit': self.project.vcs_repo(self.version.slug).commit, + 'commit': commit, 'versions': versions, 'downloads': downloads, 'subproject_urls': subproject_urls, diff --git a/readthedocs/projects/tasks.py b/readthedocs/projects/tasks.py index 6e4953a6ff6..bd4fc03559c 100644 --- a/readthedocs/projects/tasks.py +++ b/readthedocs/projects/tasks.py @@ -927,7 +927,7 @@ def setup_vcs(self, environment): # Re raise the exception to stop the build at this point raise - commit = self.commit or self.project.vcs_repo(self.version.slug).commit + commit = self.commit or self.get_vcs_repo(environment).commit if commit: self.build['commit'] = commit diff --git a/readthedocs/rtd_tests/tests/test_doc_builder.py b/readthedocs/rtd_tests/tests/test_doc_builder.py index b53660a336e..8318a27d508 100644 --- a/readthedocs/rtd_tests/tests/test_doc_builder.py +++ b/readthedocs/rtd_tests/tests/test_doc_builder.py @@ -1,6 +1,5 @@ import os import tempfile -from collections import namedtuple from unittest import mock from unittest.mock import patch @@ -25,6 +24,7 @@ SingleHtmlBuilder, ) from readthedocs.doc_builder.config import load_yaml_config +from readthedocs.doc_builder.environments import LocalBuildEnvironment from readthedocs.doc_builder.exceptions import MkDocsYAMLParseError from readthedocs.doc_builder.python_environments import Virtualenv from readthedocs.projects.constants import PRIVATE, PUBLIC @@ -321,7 +321,7 @@ def setUp(self): self.project = get(Project, documentation_type='mkdocs', name='mkdocs') self.version = get(Version, project=self.project) - self.build_env = namedtuple('project', 'version') + self.build_env = LocalBuildEnvironment(record=False) self.build_env.project = self.project self.build_env.version = self.version diff --git a/readthedocs/vcs_support/backends/git.py b/readthedocs/vcs_support/backends/git.py index f96b7fcbb11..f9cdd13573b 100644 --- a/readthedocs/vcs_support/backends/git.py +++ b/readthedocs/vcs_support/backends/git.py @@ -72,11 +72,16 @@ def update(self): def repo_exists(self): try: - git.Repo(self.working_dir) + self._repo except (InvalidGitRepositoryError, NoSuchPathError): return False return True + @property + def _repo(self): + """Get a `git.Repo` instance from the current `self.working_dir`.""" + return git.Repo(self.working_dir, expand_vars=False) + def are_submodules_available(self, config): """Test whether git submodule checkout step should be performed.""" submodules_in_config = ( @@ -232,7 +237,7 @@ def lsremote(self): @property def tags(self): versions = [] - repo = git.Repo(self.working_dir) + repo = self._repo # Build a cache of tag -> commit # GitPython is not very optimized for reading large numbers of tags @@ -265,7 +270,7 @@ def tags(self): @property def branches(self): - repo = git.Repo(self.working_dir) + repo = self._repo versions = [] branches = [] @@ -291,8 +296,7 @@ def commit(self): @property def submodules(self): - repo = git.Repo(self.working_dir) - return list(repo.submodules) + return list(self._repo.submodules) def checkout(self, identifier=None): """Checkout to identifier or latest.""" @@ -350,8 +354,7 @@ def find_ref(self, ref): def ref_exists(self, ref): try: - r = git.Repo(self.working_dir) - if r.commit(ref): + if self._repo.commit(ref): return True except (BadName, ValueError): return False