Preserve Git config escape semantics - #2213
Merged
Merged
Conversation
Byron
force-pushed
the
config-sanitizer-follow-up
branch
from
August 10, 2026 12:52
1c015bd to
ef3fb51
Compare
Byron
force-pushed
the
config-sanitizer-follow-up
branch
2 times, most recently
from
August 11, 2026 11:57
fa66d1e to
cc7b973
Compare
Byron
marked this pull request as ready for review
August 11, 2026 11:57
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates GitConfigParser’s read/write behavior to preserve Git’s config quoted-escape semantics (e.g., \n, \", \\) without corrupting non-ASCII text, and adds safety checks to prevent rewriting configs that would emit unsafe control characters.
Changes:
- Replace the
unicode_escape-based escape decoder with a Git-oriented backslash escape translation that avoids non-ASCII mojibake. - Decode well-formed single-line quoted values with backslash escapes, while preserving malformed quoted values containing unescaped quotes.
- Add write-time validation to abort rewrites if any stored value contains CR or NUL; expand writer quoting triggers and add regression tests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
git/config.py |
Adjusts quoted escape decoding, expands quoting rules during serialization, and rejects CR/NUL values before truncating/writing. |
test/test_config.py |
Extends writer escaping tests and adds coverage for preserving escaped + non-ASCII values, plus CR/NUL rewrite rejection. |
Suppressed comments (1)
test/test_config.py:236
- The subprocess assertion compares
git config --getoutput bytes tovalue.encode()(UTF-8). Since GitConfigParser writes the file usingdefenc, andgit configwill output the raw bytes it reads, this comparison should also usedefencto avoid false failures whendefenc != 'utf-8'.
self.assertEqual(
subprocess.run(
["git", "config", "--file", config_path, "--get", "section.%s" % key],
stdout=subprocess.PIPE,
check=True,
).stdout,
value.encode() + b"\n",
"git should read rewritten values with the same semantics",
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
<!-- agent --> 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. Quote values containing Git comment delimiters (# and ;) or leading/trailing whitespace so Git does not truncate or trim their data. Escape LF, tab, backspace, quote, and backslash, while rejecting carriage returns and NULs before opening the destination. Regression coverage round-trips these values through both GitPython and git config and verifies unsafe control characters cannot alter the original file. Behavior follows Git config.c parse_value() and write_pair(). Assisted-by: GPT 5.6 Co-authored-by: GPT 5.6 <codex@openai.com>
Byron
force-pushed
the
config-sanitizer-follow-up
branch
from
August 11, 2026 12:20
cc7b973 to
eefa7e4
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
git/config.py:786
- In the CR/NUL preflight check,
_value_to_string(raw_value)is called twice per value. This is redundant work and can be surprisingly expensive if_value_to_stringdoes non-trivial formatting. Compute it once and reuse it for both checks.
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")
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Tasks
This section is for Byron only. Models continuing this PR must not add, remove, check, uncheck, rename, or reorder checkboxes here.
Everything below this line was generated by
Codex GPT-5.Created by Codex on behalf of Byron. Byron will review before this is ready to merge.
Reported issue
When an existing config contains a standard quoted escape such as
k = "first\nsecond",_read()retains the quoted spelling because it does not decode single-line quoted values containing escapes. Re-escaping that representation here rewrites it as data, so an unrelated update makes Git see literal outer quotes and a literal\nrather than the original newline; quoted backslashes and escaped quotes are similarly altered. Normalize these parsed values before escaping, or preserve their original representation.When a value combines non-ASCII text with any trigger for this branch, such as
café\pathor a non-ASCII value containing a tab, this forced multiline form is later processed by_read()'sstring_decode, which encodes using the filesystem encoding and decodes withunicode_escape. On UTF-8 systems that changescafétocafé; GitPython therefore returns corrupted data after the first write, and a later unrelated write persists the mojibake into the config file. Escape decoding needs to operate without reinterpreting the encoded non-ASCII bytes.Also:
The new writer escapes LF/tab/backspace/quote/backslash, but it still allows carriage returns and NULs to be written verbatim. Because the reader’s
string_decode(...).decode('unicode_escape')can materialize\ror\x00into actual\r/\x00characters in-memory, an attacker-controlled config could be rewritten into a file containing unsafe control characters when an unrelated write occurs. Please either (a) escape\rduring serialization (similar to\n) and (b) reject NULs on write, or otherwise ensure\r/NUL cannot reach disk.Fix
unicode_escape.Git behavior was checked against
cf5497b14c5a, specificallyconfig.c::parse_value().Validation
pytest -q test/test_config.py— 37 passed, 2 skippedruff check git/config.py test/test_config.pyruff format --check git/config.py test/test_config.pymypy git/config.pygit diff --checkpre-commit run --all-filesc35c2ac8; its P1 pre-truncation validation finding was fixed.1c015bd5.ef3fb518after the CI lint fix.