diff --git a/.agents/pm/history/pm-github-494f.jsonl b/.agents/pm/history/pm-github-494f.jsonl new file mode 100644 index 0000000..23a3943 --- /dev/null +++ b/.agents/pm/history/pm-github-494f.jsonl @@ -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"}}}} diff --git a/.agents/pm/issues/pm-github-494f.toon b/.agents/pm/issues/pm-github-494f.toon new file mode 100644 index 0000000..1494b7e --- /dev/null +++ b/.agents/pm/issues/pm-github-494f.toon @@ -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: "" diff --git a/CHANGELOG.md b/CHANGELOG.md index 196bd53..3b318b8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # Changelog +## Unreleased + +### Security + +- The Link-header parser backtracks polynomially, so one hostile pagination header stalls every repository sync in the fleet ([pm-github-494f](https://github.com/unbraind/pm-github/blob/main/.agents/pm/issues/pm-github-494f.toon)) + ## 2026.9.1 - 2026-09-01 ### Fixed diff --git a/index.ts b/index.ts index bd47587..8a57f6f 100644 --- a/index.ts +++ b/index.ts @@ -576,7 +576,7 @@ export function fetchJSON(url: string, token?: string): Promise { export function parseNextLink(linkHeader?: string): string | undefined { if (!linkHeader) return undefined; for (const part of linkHeader.split(",")) { - const match = part.match(/<([^>]+)>\s*;\s*rel="next"/); + const match = part.match(/^\s*<([^>]{1,2048})>\s*;\s*rel="next"/); if (match) return match[1]; } return undefined; diff --git a/test/smoke.test.ts b/test/smoke.test.ts index 9399670..cf01176 100644 --- a/test/smoke.test.ts +++ b/test/smoke.test.ts @@ -284,6 +284,56 @@ test("parseNextLink extracts the rel=\"next\" page URL", () => { ); }); +test("parseNextLink rejects adversarial Link-header input without polynomial backtracking", () => { + // CodeQL witness for js/polynomial-redos: a string starting with '<' followed + // by many repetitions of '<=' makes the unbounded [^>]+ quantifier - retried + // at every position by match() - do O(n²) work. Anchoring with ^ and bounding + // the capture to 2048 characters removes both the multi-position scan and the + // unbounded backtracking within a single attempt. + // + // The bound is deliberately generous rather than tight. A single cold + // measurement on a contended CI runner carries JIT warm-up, GC pauses, + // scheduler noise and coverage instrumentation, so a 50ms assertion would + // measure the runner as much as the code and flake on a correct fix. 2000ms + // cannot be reached by the linear implementation on any runner while still + // failing decisively against the original expression, which took 25141ms on + // an idle machine - a 12x margin below the defect and a 40x margin above the + // real cost. A bound that can flake gets raised or deleted the first time it + // does, which is how a regression test stops guarding anything. + // + // The scale-free half of the assertion is the ratio: doubling the witness + // must not quadruple the time. That is the actual claim - linear rather than + // polynomial growth - and it holds regardless of how fast the runner is. + const witness = "<" + "<=".repeat(100_000); + const doubleWitness = "<" + "<=".repeat(200_000); + + parseNextLink("; rel=\"next\""); // warm up the JIT and the regex + + const startSingle = performance.now(); + assert.strictEqual(parseNextLink(witness), undefined, "adversarial input must not match"); + const single = performance.now() - startSingle; + + const startDouble = performance.now(); + assert.strictEqual(parseNextLink(doubleWitness), undefined, "adversarial input must not match"); + const double = performance.now() - startDouble; + + assert.ok( + single < 2000, + `a linear scan of a 200001-character header must not approach the quadratic cost ` + + `(25141ms before the fix); took ${single.toFixed(2)}ms`, + ); + // Guard the ratio against a near-zero denominator: below a millisecond the + // measurement is noise, and dividing by it would manufacture a huge ratio on + // an idle machine. Both timings that small already prove the point. + if (single >= 1) { + assert.ok( + double / single < 3, + `doubling the input must not multiply the time superlinearly: ` + + `${single.toFixed(2)}ms then ${double.toFixed(2)}ms (ratio ${(double / single).toFixed(2)})`, + ); + } +}); + test("parseNextLink returns undefined when there is no next page", () => { assert.strictEqual(parseNextLink(undefined), undefined); assert.strictEqual(