Skip to content

Commit 1c015bd

Browse files
codexByron
authored andcommitted
Preserve Git config escape semantics
Decode Git-supported quoted value escapes directly instead of routing UTF-8 text through Python unicode_escape. This preserves newlines, quotes, backslashes, and non-ASCII text when an unrelated config update rewrites existing values. Reject carriage returns and NULs at the shared writer sink so values parsed from existing files cannot place unsafe control characters into rewritten config files. Git itself does not accept \r as a config escape, so rejection is the compatible safe behavior. Regression coverage rewrites representative quoted values, compares GitPython and git-config results, and verifies CR/NUL rejection. Validated with the complete config test module, Ruff, and mypy. Behavior checked against Git cf5497b14c5a, particularly config.c parse_value(), which supports \n, \t, \b, \\, and \" and rejects unknown escapes.
1 parent 52a6cba commit 1c015bd

2 files changed

Lines changed: 74 additions & 12 deletions

File tree

git/config.py

Lines changed: 22 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -462,7 +462,8 @@ def string_decode(v: str) -> str:
462462
v = v[:-1]
463463
# END cut trailing escapes to prevent decode error
464464

465-
return v.encode(defenc).decode("unicode_escape")
465+
escapes = {"b": "\b", "n": "\n", "r": "\r", "t": "\t", '"': '"', "\\": "\\"}
466+
return re.sub(r"\\(.)", lambda match: escapes.get(match.group(1), match.group(0)), v)
466467

467468
# END string_decode
468469

@@ -517,10 +518,12 @@ def string_decode(v: str) -> str:
517518
# Opens quoting and does not close: appears to start multi-line quoting.
518519
is_multi_line = True
519520
optval = string_decode(optval[1:])
520-
elif optval.find("\\", 1, -1) == -1 and optval.find('"', 1, -1) == -1:
521-
# Opens and closes quoting. Single line, and all we need is quote removal.
522-
optval = optval[1:-1]
523-
# TODO: Handle other quoted content, especially well-formed backslash escapes.
521+
elif re.search(r'(?:^|[^\\])(?:\\\\)*"', optval[1:-1]):
522+
# Preserve malformed values containing unescaped quotes.
523+
pass
524+
else:
525+
# Opens and closes quoting.
526+
optval = string_decode(optval[1:-1])
524527

525528
# Preserves multiple values for duplicate optnames.
526529
cursect.add(optname, optval)
@@ -768,6 +771,20 @@ def write(self) -> None:
768771
return
769772
# END stop if we have include files
770773

774+
sections: List[_OMD] = [self._defaults]
775+
section: _OMD
776+
stored_section: _OMD
777+
values: List[Any]
778+
raw_value: Any
779+
for _, stored_section in self._sections.items():
780+
sections.append(stored_section)
781+
for section in sections:
782+
for key, values in section.items_all():
783+
if key != "__name__":
784+
for raw_value in values:
785+
if "\r" in self._value_to_string(raw_value) or "\x00" in self._value_to_string(raw_value):
786+
raise ValueError("Git config values must not contain CR or NUL")
787+
771788
fp = self._file_or_files
772789

773790
# We have a physical file on disk, so get a lock.

test/test_config.py

Lines changed: 52 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -190,6 +190,52 @@ def test_writer_escapes_special_characters_without_newline(self, rw_dir):
190190
with open(config_path, "rb") as config_file:
191191
self.assertNotIn(b"\x08", config_file.read())
192192

193+
@with_rw_directory
194+
def test_writer_preserves_escaped_and_non_ascii_values_safely(self, rw_dir):
195+
config_path = osp.join(rw_dir, "config")
196+
with open(config_path, "wb") as config_file:
197+
config_file.write(
198+
b'[section]\nnewline = "first\\nsecond"\nquote = "a\\"b"\nbackslash = "a\\\\b"\n'
199+
b'unicode = "caf\xc3\xa9\\\\path"\n'
200+
)
201+
202+
with GitConfigParser(config_path, read_only=False) as config:
203+
config.set_value("unrelated", "key", "value")
204+
205+
expected = {
206+
"newline": "first\nsecond",
207+
"quote": 'a"b',
208+
"backslash": "a\\b",
209+
"unicode": "caf\xe9\\path",
210+
}
211+
with GitConfigParser(config_path, read_only=True) as config:
212+
for key, value in expected.items():
213+
self.assertEqual(config.get_value("section", key), value)
214+
self.assertEqual(
215+
subprocess.run(
216+
["git", "config", "--file", config_path, "--get", "section.%s" % key],
217+
stdout=subprocess.PIPE,
218+
check=True,
219+
).stdout,
220+
value.encode() + b"\n",
221+
)
222+
223+
with open(config_path, "rb") as config_file:
224+
contents = config_file.read()
225+
self.assertNotIn(b"\r", contents)
226+
self.assertNotIn(b"\x00", contents)
227+
228+
for name, value in (("return", b"first\\rsecond"), ("nul", b"first\x00second")):
229+
unsafe_path = osp.join(rw_dir, "%s-config" % name)
230+
unsafe_contents = b'[section]\nvalue = "' + value + b'"\n'
231+
with open(unsafe_path, "wb") as config_file:
232+
config_file.write(unsafe_contents)
233+
with self.assertRaisesRegex(ValueError, "CR or NUL"):
234+
with GitConfigParser(unsafe_path, read_only=False) as config:
235+
config.set_value("unrelated", "key", "value")
236+
with open(unsafe_path, "rb") as config_file:
237+
self.assertEqual(config_file.read(), unsafe_contents)
238+
193239
@with_rw_directory
194240
def test_set_value_rejects_config_injection(self, rw_dir):
195241
config_path = osp.join(rw_dir, "config")
@@ -745,15 +791,14 @@ def test_config_with_quotes_with_whitespace_outside_value(self):
745791
self.assertEqual(cr.get("init", "defaultBranch"), "trunk")
746792

747793
def test_config_with_quotes_containing_escapes(self):
748-
"""For now just suppress quote removal. But it would be good to interpret most of these."""
794+
"""Interpret Git's quoted escapes without changing malformed values."""
749795
cr = GitConfigParser(fixture_path("git_config_with_quotes_escapes"), read_only=True)
750796

751-
# These can eventually be supported by substituting the represented character.
752-
self.assertEqual(cr.get("custom", "hasnewline"), R'"first\nsecond"')
753-
self.assertEqual(cr.get("custom", "hasbackslash"), R'"foo\\bar"')
754-
self.assertEqual(cr.get("custom", "hasquote"), R'"ab\"cd"')
755-
self.assertEqual(cr.get("custom", "hastrailingbackslash"), R'"word\\"')
756-
self.assertEqual(cr.get("custom", "hasunrecognized"), R'"p\qrs"')
797+
self.assertEqual(cr.get("custom", "hasnewline"), "first\nsecond")
798+
self.assertEqual(cr.get("custom", "hasbackslash"), R"foo\bar")
799+
self.assertEqual(cr.get("custom", "hasquote"), 'ab"cd')
800+
self.assertEqual(cr.get("custom", "hastrailingbackslash"), "word\\")
801+
self.assertEqual(cr.get("custom", "hasunrecognized"), R"p\qrs")
757802

758803
# It is less obvious whether and what to eventually do with this.
759804
self.assertEqual(cr.get("custom", "hasunescapedquotes"), '"ab"cd"e"')

0 commit comments

Comments
 (0)