diff --git a/git/cmd.py b/git/cmd.py index 4f7c3b443..03ecd13f5 100644 --- a/git/cmd.py +++ b/git/cmd.py @@ -654,6 +654,12 @@ class Git(metaclass=_GitMeta): "--upload-pack", ] + unsafe_git_pathspec_from_file_options = [ + # Reads pathspecs from a caller-controlled file. Some commands include an + # unmatched pathspec in their error output, which can disclose the file. + "--pathspec-from-file", + ] + def __getstate__(self) -> Dict[str, Any]: return slots_to_dict(self, exclude=self._excluded_) @@ -1044,13 +1050,21 @@ def _option_candidates(cls, args: Sequence[Any] = (), kwargs: Optional[Mapping[s values = value if isinstance(value, (list, tuple)) else (value,) if any(value is True or (value is not False and value is not None) for value in values): key = str(key) - options.append(f"-{key}" if len(key) == 1 else f"--{dashify(key)}") - if len(key) == 1 and split_single_char_options: + if len(key) != 1: + options.append(f"--{dashify(key)}") + elif split_single_char_options: + options.append(f"-{key}") options.extend( str(value) for value in values if value is not True and value not in (False, None) and str(value).startswith("-") ) + else: + options.extend( + f"-{key}" if value is True else f"-{key}{value}" + for value in values + if value is True or (value is not False and value is not None) + ) return options AutoInterrupt: TypeAlias = _AutoInterrupt diff --git a/git/config.py b/git/config.py index 821710ea3..3218ebeb9 100644 --- a/git/config.py +++ b/git/config.py @@ -75,6 +75,9 @@ UNSAFE_CONFIG_CHARS_RE = re.compile(r"[\r\n\x00]") """Characters that cannot be safely written in config names or values.""" +VALID_CONFIG_OPTION_NAME_RE = re.compile(r"^[A-Za-z0-9_.-]+$") +"""Pattern for option names that can be written without changing config syntax.""" + class MetaParserBuilder(abc.ABCMeta): # noqa: B024 """Utility class wrapping base-class methods into decorators that assure read-only @@ -897,6 +900,8 @@ def _value_to_string_safe(self, value: Union[str, bytes, int, float, bool]) -> s def _assure_config_name_safe(self, name: "cp._SectionName", label: str) -> None: if isinstance(name, str) and UNSAFE_CONFIG_CHARS_RE.search(name): raise ValueError("Git config %s names must not contain CR, LF, or NUL" % label) + if label == "option" and isinstance(name, str) and not VALID_CONFIG_OPTION_NAME_RE.fullmatch(name): + raise ValueError("Git config option names may contain only letters, digits, '-', '_', or '.'") if label == "section" and isinstance(name, str): in_quotes = False escaped = False diff --git a/git/index/base.py b/git/index/base.py index e5b1e72f8..55d9273dd 100644 --- a/git/index/base.py +++ b/git/index/base.py @@ -131,6 +131,7 @@ class IndexFile(LazyMixin, git_diff.Diffable, Serializable): """ unsafe_git_checkout_index_options = ["--prefix"] + unsafe_git_read_tree_options = ["--index-output"] __slots__ = ("repo", "version", "entries", "_extension_data", "_file_path") @@ -256,7 +257,12 @@ def write( @post_clear_cache @default_index - def merge_tree(self, rhs: Treeish, base: Union[None, Treeish] = None) -> "IndexFile": + def merge_tree( + self, + rhs: Treeish, + base: Union[None, Treeish] = None, + allow_unsafe_options: bool = False, + ) -> "IndexFile": """Merge the given `rhs` treeish into the current index, possibly taking a common base treeish into account. @@ -270,6 +276,9 @@ def merge_tree(self, rhs: Treeish, base: Union[None, Treeish] = None) -> "IndexF Optional treeish reference pointing to the common base of `rhs` and this index which equals lhs. + :param allow_unsafe_options: + Allow options that may write to arbitrary paths. + :return: self (containing the merge and possibly unmerged entries in case of conflicts) @@ -280,6 +289,12 @@ def merge_tree(self, rhs: Treeish, base: Union[None, Treeish] = None) -> "IndexF yourself, you have to commit the changed index (or make a valid tree from it) and retry with a three-way :meth:`index.from_tree ` call. """ + if not allow_unsafe_options: + Git.check_unsafe_options( + options=Git._option_candidates([base, rhs]), + unsafe_options=self.unsafe_git_read_tree_options, + ) + # -i : ignore working tree status # --aggressive : handle more merge cases # -m : do an actual merge @@ -324,7 +339,13 @@ def new(cls, repo: "Repo", *tree_sha: Union[str, Tree]) -> "IndexFile": return inst @classmethod - def from_tree(cls, repo: "Repo", *treeish: Treeish, **kwargs: Any) -> "IndexFile": + def from_tree( + cls, + repo: "Repo", + *treeish: Treeish, + allow_unsafe_options: bool = False, + **kwargs: Any, + ) -> "IndexFile": R"""Merge the given treeish revisions into a new index which is returned. The original index will remain unaltered. @@ -348,6 +369,9 @@ def from_tree(cls, repo: "Repo", *treeish: Treeish, **kwargs: Any) -> "IndexFile :param kwargs: Additional arguments passed to :manpage:`git-read-tree(1)`. + :param allow_unsafe_options: + Allow options that may write to arbitrary paths. + :return: New :class:`IndexFile` instance. It will point to a temporary index location which does not exist anymore. If you intend to write such a merged Index, @@ -365,6 +389,12 @@ def from_tree(cls, repo: "Repo", *treeish: Treeish, **kwargs: Any) -> "IndexFile if len(treeish) == 0 or len(treeish) > 3: raise ValueError("Please specify between 1 and 3 treeish, got %i" % len(treeish)) + if not allow_unsafe_options: + Git.check_unsafe_options( + options=Git._option_candidates(treeish, kwargs), + unsafe_options=cls.unsafe_git_read_tree_options, + ) + arg_list: List[Union[Treeish, str]] = [] # Ignore that the working tree and index possibly are out of date. if len(treeish) > 1: @@ -992,6 +1022,7 @@ def remove( self, items: Union[PathLike, Sequence[Union[PathLike, Blob, BaseIndexEntry, "Submodule"]]], working_tree: bool = False, + allow_unsafe_options: bool = False, **kwargs: Any, ) -> List[str]: R"""Remove the given items from the index and optionally from the working tree @@ -1022,6 +1053,10 @@ def remove( physically removing the respective file. This may fail if there are uncommitted changes in it. + :param allow_unsafe_options: + Allow unsafe options such as ``--pathspec-from-file`` to be passed to + :manpage:`git-rm(1)`. + :param kwargs: Additional keyword arguments to be passed to :manpage:`git-rm(1)`, such as ``r`` to allow recursive removal. @@ -1033,6 +1068,11 @@ def remove( This is interesting to know in case you have provided a directory or globs. Paths are relative to the repository. """ + if not allow_unsafe_options: + Git.check_unsafe_options( + options=Git._option_candidates([], kwargs), + unsafe_options=Git.unsafe_git_pathspec_from_file_options, + ) args = [] if not working_tree: args.append("--cached") @@ -1414,6 +1454,7 @@ def reset( working_tree: bool = False, paths: Union[None, Iterable[PathLike]] = None, head: bool = False, + allow_unsafe_options: bool = False, **kwargs: Any, ) -> "IndexFile": """Reset the index to reflect the tree at the given commit. This will not adjust @@ -1445,6 +1486,9 @@ def reset( The paths need to exist at the commit, otherwise an exception will be raised. + :param allow_unsafe_options: + Allow options that may write to arbitrary paths. + :param kwargs: Additional keyword arguments passed to :manpage:`git-reset(1)`. @@ -1461,7 +1505,7 @@ def reset( """ # What we actually want to do is to merge the tree into our existing index, # which is what git-read-tree does. - new_inst = type(self).from_tree(self.repo, commit) + new_inst = type(self).from_tree(self.repo, commit, allow_unsafe_options=allow_unsafe_options) if not paths: self.entries = new_inst.entries else: diff --git a/git/refs/head.py b/git/refs/head.py index 3c43993e7..7f563374e 100644 --- a/git/refs/head.py +++ b/git/refs/head.py @@ -19,6 +19,7 @@ from typing import Any, Sequence, TYPE_CHECKING, Union +from git.cmd import Git from git.types import Commit_ish, PathLike if TYPE_CHECKING: @@ -62,6 +63,7 @@ def reset( index: bool = True, working_tree: bool = False, paths: Union[PathLike, Sequence[PathLike], None] = None, + allow_unsafe_options: bool = False, **kwargs: Any, ) -> "HEAD": """Reset our HEAD to the given commit optionally synchronizing the index and @@ -84,12 +86,21 @@ def reset( Single path or list of paths relative to the git root directory that are to be reset. This allows to partially reset individual files. + :param allow_unsafe_options: + Allow unsafe options such as ``--pathspec-from-file`` to be passed to + :manpage:`git-reset(1)`. + :param kwargs: Additional arguments passed to :manpage:`git-reset(1)`. :return: self """ + if not allow_unsafe_options: + Git.check_unsafe_options( + options=Git._option_candidates([commit], kwargs), + unsafe_options=Git.unsafe_git_pathspec_from_file_options, + ) mode: Union[str, None] mode = "--soft" if index: @@ -234,7 +245,12 @@ def rename(self, new_path: PathLike, force: bool = False) -> "Head": self.path = "%s/%s" % (self._common_path_default, new_path) return self - def checkout(self, force: bool = False, **kwargs: Any) -> Union["HEAD", "Head"]: + def checkout( + self, + force: bool = False, + allow_unsafe_options: bool = False, + **kwargs: Any, + ) -> Union["HEAD", "Head"]: """Check out this head by setting the HEAD to this reference, by updating the index to reflect the tree we point to and by updating the working tree to reflect the latest index. @@ -246,6 +262,10 @@ def checkout(self, force: bool = False, **kwargs: Any) -> Union["HEAD", "Head"]: If ``False``, :exc:`~git.exc.GitCommandError` will be raised in that situation. + :param allow_unsafe_options: + Allow unsafe options such as ``--pathspec-from-file`` to be passed to + :manpage:`git-checkout(1)`. + :param kwargs: Additional keyword arguments to be passed to git checkout, e.g. ``b="new_branch"`` to create a new branch at the given spot. @@ -261,6 +281,11 @@ def checkout(self, force: bool = False, **kwargs: Any) -> Union["HEAD", "Head"]: the HEAD detached which is allowed and possible, but remains a special state that some tools might not be able to handle. """ + if not allow_unsafe_options: + Git.check_unsafe_options( + options=Git._option_candidates([], kwargs), + unsafe_options=Git.unsafe_git_pathspec_from_file_options, + ) kwargs["f"] = force if kwargs["f"] is False: kwargs.pop("f") diff --git a/git/repo/base.py b/git/repo/base.py index 6594101f3..4bda12255 100644 --- a/git/repo/base.py +++ b/git/repo/base.py @@ -142,6 +142,14 @@ class Repo: re_author_committer_start = re.compile(r"^(author|committer)") re_tab_full_line = re.compile(r"^\t(.*)$") + unsafe_git_init_options = [ + # Can install hooks that execute during later Git commands: + "--template", + # Redirects the repository metadata to a caller-controlled path: + "--separate-git-dir", + ] + """Options to :manpage:`git-init(1)` that permit unsafe code execution or I/O.""" + unsafe_git_clone_options = [ # Executes arbitrary commands: "--upload-pack", @@ -1394,6 +1402,7 @@ def init( mkdir: bool = True, odbt: Type[GitCmdObjectDB] = GitCmdObjectDB, expand_vars: bool = True, + allow_unsafe_options: bool = False, **kwargs: Any, ) -> "Repo": """Initialize a git repository at the given path if specified. @@ -1418,6 +1427,10 @@ def init( information disclosure, allowing attackers to access the contents of environment variables. + :param allow_unsafe_options: + Allow unsafe options to be used, such as ``--template`` and + ``--separate-git-dir``. + :param kwargs: Keyword arguments serving as additional options to the :manpage:`git-init(1)` command. @@ -1425,6 +1438,11 @@ def init( :return: :class:`Repo` (the newly created repo) """ + if not allow_unsafe_options: + Git.check_unsafe_options( + options=Git._option_candidates([], kwargs), + unsafe_options=cls.unsafe_git_init_options, + ) if path: path = expand_path(path, expand_vars) if mkdir and path and not osp.exists(path): diff --git a/test/test_config.py b/test/test_config.py index 361a51fa9..274037624 100644 --- a/test/test_config.py +++ b/test/test_config.py @@ -194,6 +194,43 @@ def test_set_value_rejects_unsafe_section_and_option_names(self, rw_dir): self.assertEqual(git_config.get_value("user", "name"), "safe") self.assertFalse(git_config.has_section("core")) + @with_rw_directory + def test_writer_rejects_invalid_option_names(self, rw_dir): + config_path = osp.join(rw_dir, "config") + bad_options = ( + "name=value", + "name#comment", + "name;comment", + "name with space", + "name\twith-tab", + "name[section", + "name]section", + "name:colon", + 'name"quote', + "name\\escape", + ) + + with GitConfigParser(config_path, read_only=False) as git_config: + git_config.add_section("user") + for bad_option in bad_options: + with pytest.raises(ValueError, match="option name"): + git_config.set("user", bad_option, "unsafe") + with pytest.raises(ValueError, match="option name"): + git_config.set_value("user", bad_option, "unsafe") + with pytest.raises(ValueError, match="option name"): + git_config.add_value("user", bad_option, "unsafe") + + git_config.set_value("user", "safe-option1", "safe") + git_config.set_value("user", "safe_option2", "safe") + git_config.set_value("user", "3safe_option", "safe") + git_config.set_value("user", "safe.option3", "safe") + + with GitConfigParser(config_path, read_only=True) as git_config: + self.assertEqual(git_config.get_value("user", "safe-option1"), "safe") + self.assertEqual(git_config.get_value("user", "safe_option2"), "safe") + self.assertEqual(git_config.get_value("user", "3safe_option"), "safe") + self.assertEqual(git_config.get_value("user", "safe.option3"), "safe") + @with_rw_directory def test_writer_rejects_unquoted_section_terminators(self, rw_dir): config_path = osp.join(rw_dir, "config") diff --git a/test/test_git.py b/test/test_git.py index 96df3c8f0..a88d980fb 100644 --- a/test/test_git.py +++ b/test/test_git.py @@ -215,6 +215,18 @@ def test_option_candidates_ignore_untransformed_kwargs(self): self.assertEqual(options, ["--max-count"]) + def test_option_candidates_include_falsey_non_boolean_values(self): + kwargs = {"pathspec_from_file": 0} + candidates = Git._option_candidates(kwargs=kwargs) + + self.assertEqual(candidates, ["--pathspec-from-file"]) + self.assertEqual(self.git.transform_kwargs(**kwargs), ["--pathspec-from-file=0"]) + with self.assertRaises(UnsafeOptionError): + Git.check_unsafe_options( + options=candidates, + unsafe_options=Git.unsafe_git_pathspec_from_file_options, + ) + def test_option_candidates_include_split_single_char_option_values(self): cases = [ ({"n": "--upload-pack=helper"}, ["-n", "--upload-pack=helper"], ["--upload-pack"]), @@ -230,7 +242,15 @@ def test_option_candidates_include_split_single_char_option_values(self): unsplit_kwargs = {"n": "--upload-pack=helper", "split_single_char_options": False} self.assertEqual(self.git.transform_kwargs(**unsplit_kwargs), ["-n--upload-pack=helper"]) - self.assertEqual(Git._option_candidates(kwargs=unsplit_kwargs), ["-n"]) + self.assertEqual(Git._option_candidates(kwargs=unsplit_kwargs), ["-n--upload-pack=helper"]) + + def test_option_candidates_include_joined_single_char_option_values(self): + kwargs = {"n": "uhelper", "split_single_char_options": False} + candidates = Git._option_candidates(kwargs=kwargs) + + self.assertEqual(candidates, ["-nuhelper"]) + with self.assertRaises(UnsafeOptionError): + Git.check_unsafe_options(options=candidates, unsafe_options=["-u"]) _shell_cases = ( # value_in_call, value_from_class, expected_popen_arg diff --git a/test/test_index.py b/test/test_index.py index 3ad5a457f..799039175 100644 --- a/test/test_index.py +++ b/test/test_index.py @@ -212,6 +212,32 @@ def test_checkout_rejects_unsafe_prefix(self, rw_repo): rw_repo.index.checkout(prefix=f"{target}/", allow_unsafe_options=True) self.assertTrue(osp.isfile(osp.join(target, "CHANGES"))) + @with_rw_repo("HEAD") + def test_remove_rejects_pathspec_from_file(self, rw_repo): + with tempfile.TemporaryDirectory() as tdir: + pathspecs = Path(tdir) / "pathspecs" + pathspecs.write_bytes(b"unmatched-path-one\nunmatched-path-two") + for option_name in ("pathspec_from_file", "pathspec_from"): + with self.assertRaises(UnsafeOptionError): + rw_repo.index.remove( + [], + pathspec_file_nul=True, + **{option_name: str(pathspecs)}, + ) + + @with_rw_repo("HEAD") + def test_remove_allows_explicit_pathspec_from_file(self, rw_repo): + with tempfile.TemporaryDirectory() as tdir: + pathspecs = Path(tdir) / "pathspecs" + pathspecs.write_bytes(b"CHANGES\0") + removed = rw_repo.index.remove( + [], + pathspec_from_file=str(pathspecs), + pathspec_file_nul=True, + allow_unsafe_options=True, + ) + assert "CHANGES" in removed + def __init__(self, *args): super().__init__(*args) self._reset_progress() @@ -327,6 +353,22 @@ def add_bad_blob(): except Exception as ex: assert "index.lock' could not be obtained" not in str(ex) + @with_rw_repo("0.1.6") + def test_read_tree_methods_reject_index_output(self, rw_repo): + output_path = (Path(rw_repo.working_tree_dir) / "alternate-index").as_posix() + unsafe_option = f"--index-output={output_path}" + + with pytest.raises(UnsafeOptionError): + IndexFile.from_tree(rw_repo, unsafe_option) + with pytest.raises(UnsafeOptionError): + IndexFile.from_tree(rw_repo, "HEAD", index_output=output_path) + with pytest.raises(UnsafeOptionError): + rw_repo.index.reset(unsafe_option) + with pytest.raises(UnsafeOptionError): + rw_repo.index.merge_tree(unsafe_option) + with pytest.raises(UnsafeOptionError): + rw_repo.index.merge_tree("HEAD", base=unsafe_option) + @with_rw_repo("0.1.6") def test_index_file_from_tree(self, rw_repo): common_ancestor_sha = "5117c9c8a4d3af19a9958677e45cda9269de1541" diff --git a/test/test_refs.py b/test/test_refs.py index 6481b54a8..f6e40071f 100644 --- a/test/test_refs.py +++ b/test/test_refs.py @@ -262,6 +262,51 @@ def test_head_checkout_detached_head(self, rw_repo): assert isinstance(res, SymbolicReference) assert res.name == "HEAD" + @with_rw_repo("HEAD") + def test_head_checkout_rejects_pathspec_from_file(self, rw_repo): + with tempfile.TemporaryDirectory() as tdir: + pathspecs = Path(tdir) / "pathspecs" + pathspecs.write_bytes(b"unmatched-path-one\nunmatched-path-two") + for option_name in ("pathspec_from_file", "pathspec_from"): + with self.assertRaises(UnsafeOptionError): + rw_repo.active_branch.checkout( + pathspec_file_nul=True, + **{option_name: str(pathspecs)}, + ) + + @with_rw_repo("HEAD") + def test_head_reset_rejects_pathspec_from_file(self, rw_repo): + with tempfile.TemporaryDirectory() as tdir: + pathspecs = Path(tdir) / "pathspecs" + pathspecs.write_bytes(b"unmatched-path-one\nunmatched-path-two") + for option_name in ("pathspec_from_file", "pathspec_from"): + with self.assertRaises(UnsafeOptionError): + rw_repo.head.reset( + pathspec_file_nul=True, + **{option_name: str(pathspecs)}, + ) + for option_name in ("--pathspec-from-file", "--pathspec-from"): + with self.assertRaises(UnsafeOptionError): + rw_repo.head.reset( + f"{option_name}={pathspecs}", + pathspec_file_nul=True, + ) + + @with_rw_repo("HEAD") + def test_head_commands_allow_explicit_pathspec_from_file(self, rw_repo): + with tempfile.TemporaryDirectory() as tdir: + pathspecs = Path(tdir) / "pathspecs" + pathspecs.write_bytes(b"CHANGES\0") + options = { + "pathspec_from_file": str(pathspecs), + "pathspec_file_nul": True, + "allow_unsafe_options": True, + } + head = rw_repo.head + branch = rw_repo.active_branch + assert head.reset(**options) is head + assert branch.checkout(**options) == branch + @with_rw_repo("0.1.6") def test_head_reset(self, rw_repo): cur_head = rw_repo.head diff --git a/test/test_repo.py b/test/test_repo.py index 7c7f1dd34..8be5c27c0 100644 --- a/test/test_repo.py +++ b/test/test_repo.py @@ -82,6 +82,33 @@ def test_new_should_raise_on_invalid_repo_location(self): with tempfile.TemporaryDirectory() as tdir: self.assertRaises(InvalidGitRepositoryError, Repo, tdir) + def test_init_rejects_unsafe_options(self): + with tempfile.TemporaryDirectory() as tdir: + template_dir = osp.join(tdir, "template") + os.mkdir(template_dir) + unsafe_options = [ + {"template": template_dir}, + {"templa": template_dir}, + {"separate_git_dir": osp.join(tdir, "git-dir")}, + {"separate_git_di": osp.join(tdir, "git-dir")}, + ] + for index, kwargs in enumerate(unsafe_options): + repo_dir = osp.join(tdir, f"repo-{index}") + with self.assertRaises(UnsafeOptionError): + Repo.init(repo_dir, **kwargs) + assert not osp.exists(repo_dir) + + def test_init_allows_explicitly_unsafe_options(self): + with tempfile.TemporaryDirectory() as tdir: + template_dir = osp.join(tdir, "template") + os.mkdir(template_dir) + repo = Repo.init( + osp.join(tdir, "repo"), + template=template_dir, + allow_unsafe_options=True, + ) + assert repo.git_dir + @with_rw_directory def test_new_should_raise_on_invalid_repo_location_within_repo(self, rw_dir): repo_dir = osp.join(rw_dir, "repo")