-
Notifications
You must be signed in to change notification settings - Fork 0
Anchor and bound the Link-header parser so one hostile header cannot stall every sync #69
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| {"ts":"2026-09-03T17:45:52.494Z","author":"pi-agent","author_source":"asserted","agent_harness":"claude-code","agent_instance":"eed31228ffb75f6dba2788c5","agent_provenance":{"model":null,"effort":{"value":"xhigh","source":"environment"},"role":{"value":"implementer","source":"argv"},"topic":null},"op":"create","patch":[{"op":"add","path":"/metadata/id","value":"pm-github-494f"},{"op":"add","path":"/metadata/title","value":"The Link-header parser backtracks polynomially, so one hostile pagination header stalls every repository sync in the fleet"},{"op":"add","path":"/metadata/description","value":"CodeQL alert 1 reports js/polynomial-redos at index.ts line 579, in parseNextLink. The expression was part.match(/<([^>]+)>\\s*;\\s*rel=\"next\"/), and it is quadratic for two compounding reasons: match without an anchor retries the pattern at every position in the string, and the unbounded [^>]+ quantifier backtracks within each attempt. A header consisting of a single < followed by a long run of <= drives both behaviours at once.\n\nThe reachability is what makes this worth more than its severity label. parseNextLink consumes the Link response header from the GitHub API, which is remote input on every paginated request, and pm-github is the package the entire fleet uses to sync pm items with GitHub issues. A stall here is not confined to one repository: it is a stall in the sync path that every other package depends on.\n\nMeasured on a 200001-character witness: the original expression takes 25141 ms and does not return in any practical sense. Anchoring the match with ^\\s* removes the multi-position retry, and bounding the capture to {1,2048} removes the unbounded backtracking within a single attempt. The same witness returns in under 50 ms afterwards. The bound is safe for the input's real domain, because a Link header URI is a GitHub API URL and 2048 characters is well beyond any it produces, and anchoring is correct because RFC 8288 places the URI-reference first in every link-value."},{"op":"add","path":"/metadata/type","value":"Issue"},{"op":"add","path":"/metadata/status","value":"open"},{"op":"add","path":"/metadata/priority","value":1},{"op":"add","path":"/metadata/tags","value":["security"]},{"op":"add","path":"/metadata/created_at","value":"2026-09-03T17:45:52.494Z"},{"op":"add","path":"/metadata/updated_at","value":"2026-09-03T17:45:52.494Z"},{"op":"add","path":"/metadata/author","value":"pi-agent"},{"op":"add","path":"/metadata/acceptance_criteria","value":"The adversarial witness returns within a hard time bound rather than merely faster; the regression test fails when the original expression is restored, verified by reverting rather than asserted; and CodeQL confirms the alert closed on a scan of main rather than the alert being dismissed"},{"op":"add","path":"/metadata/risk","value":"high"}],"before_hash":"3cc22dff72be7b14824654a7a64ea62b04799939b2fee54c1b5f52ca60bf6df0","after_hash":"5995f337d03963417abdf744ca3a7907d2a6cf04d09d49cc256c69f9e586dcb2","item_hash_version":2,"message":"","context":{"agent_provenance_outcomes":{"model":{"status":"failed","reason":"resolver_failed","resolver":"claude_session_file","rule_version":"v1"},"version":{"status":"failed","reason":"resolver_failed","resolver":"claude_session_file","rule_version":"v1"}}}} | ||
| {"ts":"2026-09-03T17:46:05.903Z","author":"codex","author_source":"configured","agent_harness":"claude-code","agent_instance":"eed31228ffb75f6dba2788c5","agent_provenance":{"model":null,"effort":{"value":"xhigh","source":"environment"},"role":null,"topic":null},"op":"comment_add","patch":[{"op":"replace","path":"/metadata/updated_at","value":"2026-09-03T17:46:05.903Z"},{"op":"add","path":"/metadata/comments","value":[{"created_at":"2026-09-03T17:46:05.903Z","author":"codex","text":"Verified the regression test is non-vacuous rather than assuming it. Restored the original unanchored expression with the new test in place: the test fails at 25141.04ms against the 50ms bound. Restored the fix: it passes. A test that passes against both implementations would have proved nothing.\n\nTwo pre-existing test failures in this repository are unrelated to this change and are a local-clone artifact rather than a defect. The privacy gate reports three host-path violations naming blob hashes; git cat-file confirms they are blobs, git rev-list --all does not reach them, and they are absent from origin/main. They are unreachable objects left in this clone's object database by an earlier history rewrite, and a fresh CI checkout does not have them, which is why CI is green on main. The gate scanning the whole object database rather than only reachable history is the stricter and better behaviour; it just makes a developer clone report violations that were never published."}]}],"before_hash":"5995f337d03963417abdf744ca3a7907d2a6cf04d09d49cc256c69f9e586dcb2","after_hash":"62df1669af6993872bd887bd03759315ae5150a92d30cd34d5e516b276ddff35","item_hash_version":2,"context":{"agent_provenance_outcomes":{"model":{"status":"failed","reason":"resolver_failed","resolver":"claude_session_file","rule_version":"v1"},"version":{"status":"failed","reason":"resolver_failed","resolver":"claude_session_file","rule_version":"v1"}}}} | ||
| {"ts":"2026-09-03T17:46:06.424Z","author":"codex","author_source":"configured","agent_harness":"claude-code","agent_instance":"eed31228ffb75f6dba2788c5","agent_provenance":{"model":null,"effort":{"value":"xhigh","source":"environment"},"role":null,"topic":null},"op":"docs_add","patch":[{"op":"replace","path":"/metadata/updated_at","value":"2026-09-03T17:46:06.424Z"},{"op":"add","path":"/metadata/docs","value":[{"path":"https://github.com/unbraind/pm-github/security/code-scanning/1","scope":"project","note":"CodeQL alert 1 js/polynomial-redos index.ts:579"}]}],"before_hash":"62df1669af6993872bd887bd03759315ae5150a92d30cd34d5e516b276ddff35","after_hash":"336457b19da2e0855951f215acc9654ad0f7bc01b2f0453173870203d4d3d4b5","item_hash_version":2,"context":{"agent_provenance_outcomes":{"model":{"status":"failed","reason":"resolver_failed","resolver":"claude_session_file","rule_version":"v1"},"version":{"status":"failed","reason":"resolver_failed","resolver":"claude_session_file","rule_version":"v1"}}}} | ||
| {"ts":"2026-09-03T17:46:07.296Z","author":"codex","author_source":"configured","agent_harness":"claude-code","agent_instance":"eed31228ffb75f6dba2788c5","agent_provenance":{"model":null,"effort":{"value":"xhigh","source":"environment"},"role":{"value":"implementer","source":"argv"},"topic":null},"op":"close","patch":[{"op":"replace","path":"/metadata/updated_at","value":"2026-09-03T17:46:07.296Z"},{"op":"replace","path":"/metadata/status","value":"closed"},{"op":"add","path":"/metadata/closed_at","value":"2026-09-03T17:46:07.284Z"},{"op":"add","path":"/metadata/completed_at","value":"2026-09-03T17:46:07.284Z"},{"op":"add","path":"/metadata/close_reason","value":"Anchored the match and bounded the capture; the 200001-character witness now returns in under 50ms where it previously took 25141ms, and the regression test was proved to fail against the original expression."}],"before_hash":"336457b19da2e0855951f215acc9654ad0f7bc01b2f0453173870203d4d3d4b5","after_hash":"3c9d319820b13a76e07b0ceaa48e1df6755a183b36345b17182c807fc557ae8b","item_hash_version":2,"context":{"agent_provenance_outcomes":{"model":{"status":"failed","reason":"resolver_failed","resolver":"claude_session_file","rule_version":"v1"},"version":{"status":"failed","reason":"resolver_failed","resolver":"claude_session_file","rule_version":"v1"}}}} | ||
| {"ts":"2026-09-03T17:46:33.556Z","author":"codex","author_source":"configured","agent_harness":"claude-code","agent_instance":"eed31228ffb75f6dba2788c5","agent_provenance":{"model":null,"effort":{"value":"xhigh","source":"environment"},"role":null,"topic":null},"op":"tests_add","patch":[{"op":"replace","path":"/metadata/updated_at","value":"2026-09-03T17:46:33.556Z"},{"op":"add","path":"/metadata/tests","value":[{"command":"npx tsx --test --test-name-pattern \"parseNextLink rejects adversarial Link-header input without polynomial backtracking\" test/smoke.test.ts","path":"test/smoke.test.ts","scope":"project","timeout_seconds":120,"provenance":{"author":"codex","created_at":"2026-09-03T17:46:33.523Z","source_kind":"local_mutation","source_ref":"main"},"note":"ReDoS regression: proved to fail (25141ms) against the original unanchored expression"}]}],"before_hash":"3c9d319820b13a76e07b0ceaa48e1df6755a183b36345b17182c807fc557ae8b","after_hash":"96417587db8207b52e73ded415892f8e5f38a258e9d42fdf13477be93dff9ee2","item_hash_version":2,"context":{"agent_provenance_outcomes":{"model":{"status":"failed","reason":"resolver_failed","resolver":"claude_session_file","rule_version":"v1"},"version":{"status":"failed","reason":"resolver_failed","resolver":"claude_session_file","rule_version":"v1"}}}} | ||
| {"ts":"2026-09-03T18:38:09.038Z","author":"codex","author_source":"configured","agent_harness":"claude-code","agent_instance":"eed31228ffb75f6dba2788c5","agent_provenance":{"model":null,"effort":{"value":"xhigh","source":"environment"},"role":{"value":"implementer","source":"argv"},"topic":null},"op":"update","patch":[{"op":"replace","path":"/metadata/updated_at","value":"2026-09-03T18:38:09.038Z"},{"op":"add","path":"/metadata/resolution","value":"Replaced the unanchored, unbounded Link-header expression with an anchored one bounding the capture to 2048 characters. The escaping semantics are unchanged; only the search shape is."},{"op":"add","path":"/metadata/expected_result","value":"A 200001-character adversarial Link header returns within a hard time bound, and CodeQL alert 1 closes on a scan of main."},{"op":"add","path":"/metadata/actual_result","value":"The witness returns in under 50ms where it previously took 25141ms. The regression test was proved to fail against the original expression by restoring it."}],"before_hash":"96417587db8207b52e73ded415892f8e5f38a258e9d42fdf13477be93dff9ee2","after_hash":"42e72722f93698da267a6200de22d49dd7be408b6901fe0b79438bc077eb4ba0","item_hash_version":2,"context":{"agent_provenance_outcomes":{"model":{"status":"failed","reason":"resolver_failed","resolver":"claude_session_file","rule_version":"v1"},"version":{"status":"failed","reason":"resolver_failed","resolver":"claude_session_file","rule_version":"v1"}}}} | ||
| {"ts":"2026-09-03T18:42:18.415Z","author":"codex","author_source":"configured","agent_harness":"claude-code","agent_instance":"eed31228ffb75f6dba2788c5","agent_provenance":{"model":null,"effort":{"value":"xhigh","source":"environment"},"role":null,"topic":null},"op":"comment_add","patch":[{"op":"add","path":"/metadata/comments/1","value":{"created_at":"2026-09-03T18:42:18.415Z","author":"codex","text":"Both review findings this round were valid and are fixed.\n\nThe wall-clock assertion was replaced rather than merely loosened. A single cold 50ms measurement on a contended runner measures the runner as much as the code — JIT warm-up, GC pauses, scheduler noise and coverage instrumentation all land inside it — so it could fail on a correct fix, and a bound that flakes gets raised or deleted the first time it does, which is how a regression test stops guarding anything. The test now warms the JIT first, then asserts two things: an absolute bound of 2000ms, which the linear implementation cannot approach on any runner while still failing decisively against the original expression, and a scale-free ratio, that doubling the witness must not multiply the time superlinearly. The ratio is the actual claim being made — linear rather than polynomial growth — and it holds regardless of how fast the machine is. The ratio check is skipped when the single-run measurement is under a millisecond, because dividing by noise would manufacture a huge ratio on an idle machine, and two sub-millisecond timings already prove the point.\n\nRe-verified non-vacuously against the relaxed bound rather than assuming the old proof carried over: restoring the original unanchored expression fails the test at 24091ms against the 2000ms bound, a 12x margin below the defect and a 40x margin above the real cost.\n\nThe closure metadata gap was also real. The close event carried close_reason without the structured resolution, expected-result and actual-result fields, which is the completeness signal pm-cli 2026.9.1 added. All three are now recorded through pm update rather than by editing the append-only history."}},{"op":"replace","path":"/metadata/updated_at","value":"2026-09-03T18:42:18.415Z"}],"before_hash":"42e72722f93698da267a6200de22d49dd7be408b6901fe0b79438bc077eb4ba0","after_hash":"dc5863d85551f9aab6fdfacd3cd4963752d0d91de359d4a8e9280fffeceddee0","item_hash_version":2,"context":{"agent_provenance_outcomes":{"model":{"status":"failed","reason":"resolver_failed","resolver":"claude_session_file","rule_version":"v1"},"version":{"status":"failed","reason":"resolver_failed","resolver":"claude_session_file","rule_version":"v1"}}}} |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| id: pm-github-494f | ||
| title: "The Link-header parser backtracks polynomially, so one hostile pagination header stalls every repository sync in the fleet" | ||
| description: "CodeQL alert 1 reports js/polynomial-redos at index.ts line 579, in parseNextLink. The expression was part.match(/<([^>]+)>\\s*;\\s*rel=\"next\"/), and it is quadratic for two compounding reasons: match without an anchor retries the pattern at every position in the string, and the unbounded [^>]+ quantifier backtracks within each attempt. A header consisting of a single < followed by a long run of <= drives both behaviours at once.\n\nThe reachability is what makes this worth more than its severity label. parseNextLink consumes the Link response header from the GitHub API, which is remote input on every paginated request, and pm-github is the package the entire fleet uses to sync pm items with GitHub issues. A stall here is not confined to one repository: it is a stall in the sync path that every other package depends on.\n\nMeasured on a 200001-character witness: the original expression takes 25141 ms and does not return in any practical sense. Anchoring the match with ^\\s* removes the multi-position retry, and bounding the capture to {1,2048} removes the unbounded backtracking within a single attempt. The same witness returns in under 50 ms afterwards. The bound is safe for the input's real domain, because a Link header URI is a GitHub API URL and 2048 characters is well beyond any it produces, and anchoring is correct because RFC 8288 places the URI-reference first in every link-value." | ||
| type: Issue | ||
| status: closed | ||
| priority: 1 | ||
| tags[1]: security | ||
| created_at: "2026-09-03T17:45:52.494Z" | ||
| updated_at: "2026-09-03T18:42:18.415Z" | ||
| closed_at: "2026-09-03T17:46:07.284Z" | ||
| completed_at: "2026-09-03T17:46:07.284Z" | ||
| author: pi-agent | ||
| acceptance_criteria: "The adversarial witness returns within a hard time bound rather than merely faster; the regression test fails when the original expression is restored, verified by reverting rather than asserted; and CodeQL confirms the alert closed on a scan of main rather than the alert being dismissed" | ||
| risk: high | ||
| resolution: "Replaced the unanchored, unbounded Link-header expression with an anchored one bounding the capture to 2048 characters. The escaping semantics are unchanged; only the search shape is." | ||
| expected_result: "A 200001-character adversarial Link header returns within a hard time bound, and CodeQL alert 1 closes on a scan of main." | ||
| actual_result: The witness returns in under 50ms where it previously took 25141ms. The regression test was proved to fail against the original expression by restoring it. | ||
| comments[2]{created_at,author,text}: | ||
| "2026-09-03T17:46:05.903Z",codex,"Verified the regression test is non-vacuous rather than assuming it. Restored the original unanchored expression with the new test in place: the test fails at 25141.04ms against the 50ms bound. Restored the fix: it passes. A test that passes against both implementations would have proved nothing.\n\nTwo pre-existing test failures in this repository are unrelated to this change and are a local-clone artifact rather than a defect. The privacy gate reports three host-path violations naming blob hashes; git cat-file confirms they are blobs, git rev-list --all does not reach them, and they are absent from origin/main. They are unreachable objects left in this clone's object database by an earlier history rewrite, and a fresh CI checkout does not have them, which is why CI is green on main. The gate scanning the whole object database rather than only reachable history is the stricter and better behaviour; it just makes a developer clone report violations that were never published." | ||
| "2026-09-03T18:42:18.415Z",codex,"Both review findings this round were valid and are fixed.\n\nThe wall-clock assertion was replaced rather than merely loosened. A single cold 50ms measurement on a contended runner measures the runner as much as the code — JIT warm-up, GC pauses, scheduler noise and coverage instrumentation all land inside it — so it could fail on a correct fix, and a bound that flakes gets raised or deleted the first time it does, which is how a regression test stops guarding anything. The test now warms the JIT first, then asserts two things: an absolute bound of 2000ms, which the linear implementation cannot approach on any runner while still failing decisively against the original expression, and a scale-free ratio, that doubling the witness must not multiply the time superlinearly. The ratio is the actual claim being made — linear rather than polynomial growth — and it holds regardless of how fast the machine is. The ratio check is skipped when the single-run measurement is under a millisecond, because dividing by noise would manufacture a huge ratio on an idle machine, and two sub-millisecond timings already prove the point.\n\nRe-verified non-vacuously against the relaxed bound rather than assuming the old proof carried over: restoring the original unanchored expression fails the test at 24091ms against the 2000ms bound, a 12x margin below the defect and a 40x margin above the real cost.\n\nThe closure metadata gap was also real. The close event carried close_reason without the structured resolution, expected-result and actual-result fields, which is the completeness signal pm-cli 2026.9.1 added. All three are now recorded through pm update rather than by editing the append-only history." | ||
| tests[1]{command,path,scope,timeout_seconds,provenance{author,created_at,source_kind,source_ref},note}: | ||
| "npx tsx --test --test-name-pattern \"parseNextLink rejects adversarial Link-header input without polynomial backtracking\" test/smoke.test.ts",test/smoke.test.ts,project,120,codex,"2026-09-03T17:46:33.523Z",local_mutation,main,"ReDoS regression: proved to fail (25141ms) against the original unanchored expression" | ||
| docs[1]{path,scope,note}: | ||
| "https://github.com/unbraind/pm-github/security/code-scanning/1",project,"CodeQL alert 1 js/polynomial-redos index.ts:579" | ||
| close_reason: "Anchored the match and bounded the capture; the 200001-character witness now returns in under 50ms where it previously took 25141ms, and the regression test was proved to fail against the original expression." | ||
| body: "" | ||
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
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
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.