Merge pull request #2213 from gitpython-developers/config-sanitizer-follow-up

Byron · web-flow · commit 4b9afe9e174a · 2026-08-11T14:47:26.000+02:00
Preserve Git config escape semantics
diff --git a/git/config.py b/git/config.py
@@ -462,7 +462,8 @@ def string_decode(v: str) -> str:
                 v = v[:-1]
             # END cut trailing escapes to prevent decode error
 
-            return v.encode(defenc).decode("unicode_escape")
+            escapes = {"b": "\b", "n": "\n", "r": "\r", "t": "\t", '"': '"', "\\": "\\"}
+            return re.sub(r"\\(.)", lambda match: escapes.get(match.group(1), match.group(0)), v)
 
         # END string_decode
 
@@ -517,10 +518,12 @@ def string_decode(v: str) -> str:
                         # Opens quoting and does not close: appears to start multi-line quoting.
                         is_multi_line = True
                         optval = string_decode(optval[1:])
-                    elif optval.find("\\", 1, -1) == -1 and optval.find('"', 1, -1) == -1:
-                        # Opens and closes quoting. Single line, and all we need is quote removal.
-                        optval = optval[1:-1]
-                    # TODO: Handle other quoted content, especially well-formed backslash escapes.
+                    elif re.search(r'(?:^|[^\\])(?:\\\\)*"', optval[1:-1]):
+                        # Preserve malformed values containing unescaped quotes.
+                        pass
+                    else:
+                        # Opens and closes quoting.
+                        optval = string_decode(optval[1:-1])
 
                     # Preserves multiple values for duplicate optnames.
                     cursect.add(optname, optval)
@@ -706,7 +709,7 @@ def write_section(name: str, section_dict: _OMD) -> None:
 
                 for v in values:
                     value = self._value_to_string(v)
-                    if any(char in value for char in '\n\t\b\\"'):
+                    if any(char in value for char in '\n\t\b\\"#;') or value[:1].isspace() or value[-1:].isspace():
                         value = value.replace("\\", "\\\\").replace('"', '\\"')
                         value = '"%s\\\n"' % value.replace("\n", "\\n").replace("\t", "\\t").replace("\b", "\\b")
                     fp.write(("\t%s = %s\n" % (key, value)).encode(defenc))
@@ -768,6 +771,20 @@ def write(self) -> None:
             return
         # END stop if we have include files
 
+        sections: List[_OMD] = [self._defaults]
+        section: _OMD
+        stored_section: _OMD
+        values: List[Any]
+        raw_value: Any
+        for _, stored_section in self._sections.items():
+            sections.append(stored_section)
+        for section in sections:
+            for key, values in section.items_all():
+                if key != "__name__":
+                    for raw_value in values:
+                        if "\r" in self._value_to_string(raw_value) or "\x00" in self._value_to_string(raw_value):
+                            raise ValueError("Git config values must not contain CR or NUL")
+
         fp = self._file_or_files
 
         # We have a physical file on disk, so get a lock.
diff --git a/test/test_config.py b/test/test_config.py
@@ -14,6 +14,7 @@
 import pytest
 
 from git import GitConfigParser
+from git.compat import defenc
 from git.config import _OMD, cp
 from git.util import cwd, rmfile
 from test.lib import SkipTest, TestCase, fixture_path, with_rw_directory
@@ -170,7 +171,16 @@ def test_rewriting_multiline_value_does_not_create_option(self, rw_dir):
     @with_rw_directory
     def test_writer_escapes_special_characters_without_newline(self, rw_dir):
         config_path = osp.join(rw_dir, "config")
-        values = {"tab": "\tvalue\t", "backspace": "a\bb", "quote": 'a"b', "backslash": "a\\qb"}
+        values = {
+            "tab": "\tvalue\t",
+            "backspace": "a\bb",
+            "quote": 'a"b',
+            "backslash": "a\\qb",
+            "hash": "value#fragment",
+            "semicolon": "value;fragment",
+            "leading": " value",
+            "trailing": "value ",
+        }
 
         with GitConfigParser(config_path, read_only=False) as git_config:
             for key, value in values.items():
@@ -185,11 +195,72 @@ def test_writer_escapes_special_characters_without_newline(self, rw_dir):
                         stdout=subprocess.PIPE,
                         check=True,
                     ).stdout,
-                    value.encode() + b"\n",
+                    value.encode(defenc) + b"\n",
                 )
         with open(config_path, "rb") as config_file:
             self.assertNotIn(b"\x08", config_file.read())
 
+    @with_rw_directory
+    def test_writer_preserves_escaped_and_non_ascii_values_safely(self, rw_dir):
+        config_path = osp.join(rw_dir, "config")
+        with open(config_path, "wb") as config_file:
+            config_file.write(
+                (
+                    '[section]\nnewline = "first\\nsecond"\nquote = "a\\"b"\nbackslash = "a\\\\b"\n'
+                    'unicode = "café\\\\path"\n'
+                ).encode(defenc)
+            )
+
+        with GitConfigParser(config_path, read_only=False) as config:
+            config.set_value("unrelated", "key", "value")
+
+        expected = {
+            "newline": "first\nsecond",
+            "quote": 'a"b',
+            "backslash": "a\\b",
+            "unicode": "café\\path",
+        }
+        with GitConfigParser(config_path, read_only=True) as config:
+            for key, value in expected.items():
+                self.assertEqual(
+                    config.get_value("section", key),
+                    value,
+                    "GitPython should preserve values when rewriting unrelated entries",
+                )
+                self.assertEqual(
+                    subprocess.run(
+                        ["git", "config", "--file", config_path, "--get", "section.%s" % key],
+                        stdout=subprocess.PIPE,
+                        check=True,
+                    ).stdout,
+                    value.encode(defenc) + b"\n",
+                    "git should read rewritten values with the same semantics",
+                )
+
+        with open(config_path, "rb") as config_file:
+            contents = config_file.read()
+        self.assertNotIn(b"\r", contents, "the writer should never emit carriage returns")
+        self.assertNotIn(b"\x00", contents, "the writer should never emit NUL bytes")
+
+        for name, value in (("return", b"first\\rsecond"), ("nul", b"first\x00second")):
+            unsafe_path = osp.join(rw_dir, "%s-config" % name)
+            unsafe_contents = b'[section]\nvalue = "' + value + b'"\n'
+            with open(unsafe_path, "wb") as config_file:
+                config_file.write(unsafe_contents)
+            with self.assertRaisesRegex(
+                ValueError,
+                "CR or NUL",
+                msg="unsafe existing values should abort rewrites",
+            ):
+                with GitConfigParser(unsafe_path, read_only=False) as config:
+                    config.set_value("unrelated", "key", "value")
+            with open(unsafe_path, "rb") as config_file:
+                self.assertEqual(
+                    config_file.read(),
+                    unsafe_contents,
+                    "rejected rewrites should leave the original file unchanged",
+                )
+
     @with_rw_directory
     def test_set_value_rejects_config_injection(self, rw_dir):
         config_path = osp.join(rw_dir, "config")
@@ -745,15 +816,14 @@ def test_config_with_quotes_with_whitespace_outside_value(self):
         self.assertEqual(cr.get("init", "defaultBranch"), "trunk")
 
     def test_config_with_quotes_containing_escapes(self):
-        """For now just suppress quote removal. But it would be good to interpret most of these."""
+        """Interpret Git's quoted escapes without changing malformed values."""
         cr = GitConfigParser(fixture_path("git_config_with_quotes_escapes"), read_only=True)
 
-        # These can eventually be supported by substituting the represented character.
-        self.assertEqual(cr.get("custom", "hasnewline"), R'"first\nsecond"')
-        self.assertEqual(cr.get("custom", "hasbackslash"), R'"foo\\bar"')
-        self.assertEqual(cr.get("custom", "hasquote"), R'"ab\"cd"')
-        self.assertEqual(cr.get("custom", "hastrailingbackslash"), R'"word\\"')
-        self.assertEqual(cr.get("custom", "hasunrecognized"), R'"p\qrs"')
+        self.assertEqual(cr.get("custom", "hasnewline"), "first\nsecond")
+        self.assertEqual(cr.get("custom", "hasbackslash"), R"foo\bar")
+        self.assertEqual(cr.get("custom", "hasquote"), 'ab"cd')
+        self.assertEqual(cr.get("custom", "hastrailingbackslash"), "word\\")
+        self.assertEqual(cr.get("custom", "hasunrecognized"), R"p\qrs")
 
         # It is less obvious whether and what to eventually do with this.
         self.assertEqual(cr.get("custom", "hasunescapedquotes"), '"ab"cd"e"')
