Repository navigation
fix(commit): bound co_authors trailer parsing - #2257
Byron merged 1 commit into
Conversation
Sorry, something went wrong.
`Commit.co_authors` matched `^Co-authored-by: (.*) <(.*?)>$` against the whole commit message. On a single trailer line that repeats `` <`` without ever closing a `>`, the greedy `(.*)` backtracks over every `` <`` position and, for each, the lazy `(.*?)` rescans to the end of the line looking for a `>` that never arrives. That is O(n^2) in the length of the line. The commit message is fully attacker-controlled: it comes straight from the object bytes decoded in `_deserialize`, so a repository can ship a commit whose message is a few hundred KB of `Co-authored-by: a <a <a <...`. Any caller that reads `commit.co_authors` (a public property) then stalls. A 60 KB line already costs several seconds of CPU; a few hundred KB reaches minutes. Parse each `Co-authored-by:` line with string operations instead of a backtracking regex: a trailer is `Co-authored-by: <name> <email>` with the email in the final angle brackets, so the name ends at the last `` <`` and the line ends at `>`. This reproduces the previous results exactly (verified against the old pattern over ~1.8M fuzzed inputs, including the existing `test_commit_co_authors` cases) while running in linear time: the same crafted input drops from seconds to microseconds. Add a CPU-time regression test that fails on the old code and passes here, and confirm a well-formed trailer on a later line is still parsed.
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The implementation preserves parsing behavior while eliminating quadratic backtracking with focused regression coverage.
Review effort: Balanced
Findings: None
Replaces vulnerable quadratic regex parsing with linear string parsing for co-author trailers.
Changes:
- Parses trailers using prefix, suffix, and final-separator checks.
- Adds a CPU-time regression test for malformed input.
| File | Description |
|---|---|
| git/objects/commit.py | Implements bounded co-author parsing. |
| test/test_commit.py | Tests malformed and subsequent valid trailers. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Sorry, something went wrong.
Byron
left a comment
There was a problem hiding this comment.
Thanks a lot!
I ran additional reviews against it and it came up empty.
I myself gave it an armchair review, and feel reminded of the various other regex-related performance issues that were fixed recently.
Sorry, something went wrong.
Repro: read Commit.co_authors on a commit whose message has a Co-authored-by: line that repeats < without ever closing a > (a few hundred KB on that one line is enough). A 60 KB line already burns several seconds of CPU; larger reaches minutes.
Cause: ^Co-authored-by: (.*) <(.*?)>$ lets the greedy (.*) backtrack over every < position, and for each one the lazy (.*?) rescans to end of line looking for a > that never arrives, so the match is O(n^2) in the line length. The message is decoded straight from the object bytes in _deserialize, so it is fully attacker-controlled by any repository whose commits you inspect.
Fix: parse each Co-authored-by: line with string operations instead of a backtracking regex. A trailer is Co-authored-by: <name> <email> with the email in the final angle brackets, so the name ends at the last < and the line ends at >. This is linear and reproduces the old results exactly: I compared it against the previous pattern over ~1.8M fuzzed inputs (names/emails with spaces, extra </>, trailing text, CR, unicode, multi-line) with zero differences, and the existing test_commit_co_authors cases are unchanged. The crafted input drops from seconds to microseconds.
Added test_commit_co_authors_bounds_malformed_trailer, a CPU-time regression that fails on the old code and passes here, and it also checks a well-formed trailer on a later line is still parsed.
I'm an AI agent contributing through this account; this change was prepared with AI assistance.