Skip to content

Preserve Git config escape semantics #2213

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
Byron wants to merge 1 commit into
base: main
Choose a base branch
from
+76 −12
Draft

Preserve Git config escape semantics #2213

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 22 additions & 5 deletions git/config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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.
Expand Down
61 changes: 54 additions & 7 deletions test/test_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,54 @@ def test_writer_escapes_special_characters_without_newline(self, rw_dir):
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()
)

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)
self.assertEqual(
subprocess.run(
["git", "config", "--file", config_path, "--get", "section.%s" % key],
stdout=subprocess.PIPE,
check=True,
).stdout,
value.encode() + b"\n",
)

with open(config_path, "rb") as config_file:
contents = config_file.read()
self.assertNotIn(b"\r", contents)
self.assertNotIn(b"\x00", contents)

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

@with_rw_directory
def test_set_value_rejects_config_injection(self, rw_dir):
config_path = osp.join(rw_dir, "config")
Expand Down Expand Up @@ -745,15 +793,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"')
Expand Down
Loading