fix: harden config parsing boundaries - #2211
Conversation
<!-- agent --> GitConfigParser decoded valid multiline values into embedded newlines, but _write() serialized those newlines as indented physical lines. Rewriting an otherwise unchanged config could therefore change its meaning to Git. Serialize resident multiline values with Git-compatible escapes inside a quoted continuation, preserving GitPython read compatibility while keeping each option structurally intact. This addresses GHSA-284h-m62q-gf8w. The regression starts with an inert multiline value, performs an unrelated write, and verifies with both GitPython and git config that it remains one value and does not create another option. Git baseline: config.c parse_value() and write_pair() at cf5497b14c5a escape embedded LF as \\n rather than emitting it as a physical config line. Assisted-by: GPT 5.6 Co-authored-by: GPT 5.6 <codex@openai.com>
<!-- agent --> Submodule configuration is read from .gitmodules, whose contents may come from an untrusted repository. Its parser inherited merge_includes=True and could therefore open files named by include directives during ordinary submodule enumeration. Disable include merging at the SubmoduleConfigParser construction site. This matches Repo.config_writer() hardening from 41ecc6a and addresses GHSA-7833-fr7j-v32q without changing include behavior for trusted config parsers. The regression points .gitmodules at a non-config file and verifies the submodule entry remains readable without opening the included path. Assisted-by: GPT 5.6 Co-authored-by: GPT 5.6 <codex@openai.com>
5326ebf to
ef7568e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef7568e3b3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| value = value.replace("\\", "\\\\").replace('"', '\\"') | ||
| value = '"%s\\\n"' % value.replace("\n", "\\n").replace("\t", "\\t").replace("\b", "\\b") |
There was a problem hiding this comment.
Preserve Git semantics for quoted escaped values
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.
Useful? React with 👍 / 👎.
| value = self._value_to_string(v) | ||
| if any(char in value for char in '\n\t\b\\"'): | ||
| value = value.replace("\\", "\\\\").replace('"', '\\"') | ||
| value = '"%s\\\n"' % value.replace("\n", "\\n").replace("\t", "\\t").replace("\b", "\\b") |
There was a problem hiding this comment.
Keep non-ASCII text intact through the escape decoder
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.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Hardens Git config parsing/serialization to prevent unsafe boundary crossings when rewriting config files and when parsing repository-controlled .gitmodules, aligning GitPython behavior more closely with Git’s escaping and include-handling expectations.
Changes:
- Update
GitConfigParserwriter to escape/quote values containing special characters (notably multiline values) instead of emitting physical continuation lines. - Disable
[include]merging when parsing.gitmodulesviaSubmoduleConfigParser. - Add regression tests covering multiline rewrite safety, special-character escaping, and
.gitmodulesinclude isolation.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
git/config.py |
Adjusts config serialization to quote/escape special characters to preserve multiline values safely. |
git/objects/submodule/base.py |
Forces .gitmodules parsing to use merge_includes=False to avoid opening arbitrary local files. |
test/test_config.py |
Adds tests to ensure multiline rewrite doesn’t create unintended options and that special chars round-trip and match git config. |
test/test_submodule.py |
Adds a test ensuring .gitmodules does not merge includes (and thus doesn’t parse unrelated files). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| value = self._value_to_string(v) | ||
| if any(char in value for char in '\n\t\b\\"'): | ||
| 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)) |
There was a problem hiding this comment.
While it's correct, it seems there is no imminent security risk from this, so let's leave it to the advisory-bots to find something related to this.
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.
Summary
.gitmodules, so repository-controlled submodule configuration cannot cause unrelated local files to be opened.Advisories
GHSA-284h-m62q-gf8w)GHSA-7833-fr7j-v32q)Both advisories remain private/under triage, so reproduction and exploit details are intentionally omitted here.
Advisory summary
GHSA-284h-m62q-gf8w
GitPython(pip)<= 3.1.58GHSA-7833-fr7j-v32q
GitPython(pip)<= 3.1.58Baselines
config.catcf5497b14c5aserializes embedded LF with an escape rather than as a physical config line.41ecc6a4disables include merging for another write-sensitive config parser.Validation
pytest -q test/test_config.py(35 passed, 2 skipped)pytest -q(passed after each commit)git diff --check