Repository navigation
Preserve Git config escape semantics - #2213
Merged
Byron merged 1 commit intoAug 11, 2026
Merged
Byron merged 1 commit into
Byron merged 1 commit into
Conversation
Copy link
Copy Markdown
Member
Sorry, something went wrong.
Byron
force-pushed
the
config-sanitizer-follow-up
branch
from
1c015bd to
ef3fb51
Compare
August 10, 2026 12:52
Byron
force-pushed
the
config-sanitizer-follow-up
branch
2 times, most recently
from
fa66d1e to
cc7b973
Compare
August 11, 2026 11:57
Byron
marked this pull request as ready for review
August 11, 2026 11:57
Copy link
Copy Markdown
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. |
test/test_config.py:236
- The subprocess assertion compares git config --get output bytes to value.encode() (UTF-8). Since GitConfigParser writes the file using defenc, and git config will output the raw bytes it reads, this comparison should also use defenc to avoid false failures when defenc != '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.
Sorry, something went wrong.
Comment thread
test/test_config.py
<!-- 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
cc7b973 to
eefa7e4
Compare
August 11, 2026 12:20
Copy link
Copy Markdown
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_string does 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")
Sorry, something went wrong.
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 \n rather 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é\path or a non-ASCII value containing a tab, this forced multiline form is later processed by _read()'s string_decode, which encodes using the filesystem encoding and decodes with unicode_escape. On UTF-8 systems that changes café to café; 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 \r or \x00 into actual \r/\x00 characters 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 \r during serialization (similar to \n) and (b) reject NULs on write, or otherwise ensure \r/NUL cannot reach disk.
Fix
Git behavior was checked against cf5497b14c5a, specifically config.c::parse_value().
Validation