fix(commit): bound co_authors trailer parsing - #2257
Open
Keerthana-64 wants to merge 1 commit into
Open
Keerthana-64 wants to merge 1 commit into
Keerthana-64 wants to merge 1 commit into
Conversation
`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.
Contributor
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
What changed in this PR
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.
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.
Repro: read
Commit.co_authorson a commit whose message has aCo-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 isCo-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 existingtest_commit_co_authorscases 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.