Harden config and Git option validation

git/cmd.py

@@ -1044,13 +1044,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

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

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 <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:
@@ -1414,6 +1444,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 +1476,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 +1495,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:

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")

test/test_git.py

@@ -230,7 +230,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

test/test_index.py

@@ -327,6 +327,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"