Skip to content

Harden diff path and actor identity parsing - #2215

Merged
Byron merged 2 commits into
mainfrom
various-fixes
Aug 11, 2026
Merged

Harden diff path and actor identity parsing#2215
Byron merged 2 commits into
mainfrom
various-fixes

Conversation

@Byron

@Byron Byron commented Aug 11, 2026

Copy link
Copy Markdown
Member

Tasks

This section is for Byron only. Models continuing this PR must not add, remove, check, uncheck, rename, or reorder checkboxes here.

  • refackiew

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

  • Decode Git C-style quoted diff paths in one pass so escaped literal backslashes cannot be reinterpreted as byte escapes.
  • Parse actor identities with Git-style delimiter scans instead of backtracking regular expressions.
  • Use GitPython’s whole-string fallback when an actor delimiter is missing.

Advisory summary

The implementation and this description are intentionally limited to the input-validation classes and do not reproduce private proof-of-concept details.

References

Behavior was checked against Git cf5497b14c5a24f10c13f7e0ee85cb95 (quote.c::unquote_c_style, ident.c::split_ident_line, and invalid-committer tests) and the local gix-actor signature parser/tests.

Validation

  • Focused quoted-path regression: passed.
  • test/test_diff.py: 24 passed, 1 environment-sensitive existing failure in test_diff_with_staged_file (expected two conflict entries, got zero).
  • test/test_actor.py: 7 passed.
  • TestUtils::test_actor_from_string: passed.
  • Ruff check and format checks: passed.
  • Codex review: clean for both final commit hashes.

@Byron
Byron marked this pull request as ready for review August 11, 2026 12:42
Copilot AI lite review requested due to automatic review settings August 11, 2026 12:42
<!-- agent -->
GHSA-v6xg-m7rh-r365 (closed) reports that quoted patch paths can crash or
silently change when an escaped literal backslash precedes digits.

Add regression coverage distinguishing literal backslashes from real
octal byte escapes, then decode Git's C-style quoting sequentially so
one escape cannot be reinterpreted by a later pass. Match Git baseline
cf5497b14c5a24f10c13f7e0ee85cb95af13ea6a quote.c::unquote_c_style by
accepting octal bytes only when all three digits are valid and the first
is 0 through 3.

Assisted-by: GPT 5.6
Co-authored-by: GPT 5.6 <codex@openai.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens two parsing surfaces in GitPython that can process attacker-controlled input: diff path decoding (C-style quoted paths) and Actor identity parsing. It aligns behavior more closely with Git while preventing pathological parsing cases (e.g., reinterpretation of escaped backslashes as octal byte escapes, and regex backtracking blowups).

Changes:

  • Replaces multi-step quoted diff path decoding with a single-pass C-style unquoter to avoid double-interpretation of escapes.
  • Reworks Actor._from_string to use bounded delimiter scans (no regex backtracking) while preserving existing multiline/malformed parsing behavior.
  • Adds targeted regression tests for both hardened parsers.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
git/diff.py Implements single-pass decoding of Git C-style quoted diff paths via _unquote_path, used by decode_path.
git/util.py Replaces regex-based Actor._from_string parsing with delimiter scanning to avoid backtracking.
test/test_diff.py Adds regression test ensuring escaped backslashes aren’t reinterpreted as octal byte escapes.
test/test_actor.py Adds tests covering large unterminated emails (anti-backtracking) and newline-bounded parsing.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot AI review requested due to automatic review settings August 11, 2026 12:47

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

<!-- agent -->
GHSA-g5vv-9gxw-82hx reports quadratic backtracking when an actor
identity contains a long unterminated email delimiter.

Add a regression that exercises a 20,000-character malformed identity,
then replace both actor regexes with direct delimiter scans following
Git's first-opening, first-closing delimiter behavior. Keep GitPython's
whole-string fallback when either delimiter is absent.

Reference Git baseline cf5497b14c5a24f10c13f7e0ee85cb95
ident.c::split_ident_line and its invalid-committer cases in
t/t9300-fast-import.sh. Also reference gix-actor's signature decoder and
lenient identity tests.

Assisted-by: GPT 5.6
Co-authored-by: GPT 5.6 <codex@openai.com>
Copilot AI review requested due to automatic review settings August 11, 2026 13:07

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

Suppressed comments (3)

test/test_actor.py:35

  • This test asserts on an internal implementation detail (vars(Actor) lacking name_email_regex). That makes the test brittle to refactors that still satisfy the security property (no catastrophic backtracking) and may fail even if behavior remains correct. Prefer asserting only on externally observable behavior.
    def test_from_string_handles_unterminated_email_without_regex_backtracking(self):
        value = "A" * 20_000 + " <unterminated"
        actor = Actor._from_string(value)
        self.assertNotIn("name_email_regex", vars(Actor))
        self.assertEqual(actor, Actor(value, None))

doc/source/changes.rst:11

  • PR description lists fixes for both GHSA-g5vv-9gxw-82hx and GHSA-v6xg-m7rh-r365, but the 3.1.60 changelog entry only references GHSA-g5vv-9gxw-82hx. If both advisories are addressed by this release, add the missing advisory link here (or update the PR description if only one is in scope).
Security fixes for

* https://github.com/gitpython-developers/GitPython/security/advisories/GHSA-g5vv-9gxw-82hx

git/diff.py:126

  • In _unquote_path, the escapes dict is re-created on every call. decode_path() is used while parsing diff headers, so this extra allocation can add overhead for large diffs. Consider hoisting the escape table to a module-level constant and reusing it.
def _unquote_path(path: bytes) -> bytes:
    result = bytearray()
    escapes = {
        ord("a"): 7,
        ord("b"): 8,
        ord("f"): 12,
        ord("n"): 10,
        ord("r"): 13,
        ord("t"): 9,
        ord("v"): 11,
    }

@Byron
Byron merged commit f44c1fb into main Aug 11, 2026
54 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants