Skip to content

Commit 8873022

Browse files
thymikeeclaude
andcommitted
fix: remove polynomial-redos ambiguity from the target-v1 annotation line regex
CodeQL js/polynomial-redos flagged TARGET_ANNOTATION_LINE_RE (packages/ad-script/src/internal/target-annotation-serde.ts): the payload group's `\s+(.*)` let `\s+` and the unconstrained `.*` both match whitespace, so a run of separator whitespace that ultimately fails to complete the match has many `\s+`/`.*` splits to backtrack through before concluding failure. Anchor the payload group on `\S` (the exact complement of `\s`), so the mandatory `\s+` separator and the payload's first character can never overlap — the split point becomes unique and no backtracking is possible. Behavior-preserving: the only caller (parseTargetAnnotationCommentLine) always matches against an already-.trim()-ed line, whose last character (whenever the tag matches at all) is never whitespace — so a payload section `\S.*` would reject (content that is entirely whitespace) can never reach this regex through the real call path. Verified against the frozen replay-compat corpus and the full serde/parser test suites, unmodified. Added a regression test with the exact adversarial shape CodeQL/the reviewer cited (many tab pairs after the version digits), asserting sub-second parse. Refs #1478 Co-Authored-By: Claude <noreply@anthropic.com>
1 parent cbdd1ba commit 8873022

2 files changed

Lines changed: 40 additions & 1 deletion

File tree

packages/ad-script/src/internal/__tests__/target-annotation-serde.test.ts

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -312,3 +312,31 @@ test('parser accepts an explicit empty-string role (writer-legal for typeless no
312312
assert.equal(parsed.role, '');
313313
assert.deepEqual(parsed.ancestry, [{ role: '' }]);
314314
});
315+
316+
// ---------------------------------------------------------------------------
317+
// CodeQL js/polynomial-redos (#1536 review): the target-v1 comment-line regex
318+
// used to backtrack polynomially on a run of separator whitespace that never
319+
// resolves to a full match — `\s+` and a bare `.*` both accept whitespace, so
320+
// there were exponentially many ways to split the run between them before
321+
// concluding failure.
322+
// ---------------------------------------------------------------------------
323+
324+
test('parseTargetAnnotationCommentLine resolves an adversarial whitespace-run line in well under a second', () => {
325+
// A trailing non-whitespace character is deliberate: `parseTargetAnnotationCommentLine`
326+
// trims the line before matching, so a purely-trailing whitespace run (the
327+
// literal CodeQL-cited shape) would already be stripped before it ever
328+
// reaches the regex. Appending one non-whitespace byte keeps the long tab
329+
// run internal, so this test genuinely exercises the flagged pattern
330+
// through the public entry point rather than being neutralized by trim().
331+
const adversarial = `#agent-device:target-v0\t${'\t\t'.repeat(20_000)}x`;
332+
const startedAt = Date.now();
333+
const result = parseTargetAnnotationCommentLine(adversarial);
334+
const elapsedMs = Date.now() - startedAt;
335+
assert.ok(
336+
elapsedMs < 1000,
337+
`expected the adversarial line to parse quickly, took ${elapsedMs}ms`,
338+
);
339+
// v0 is a future-version comment to this (v1) reader — verifies the regex still
340+
// matches the tag/version correctly, not just that it fails fast.
341+
assert.deepEqual(result, { kind: 'future-version' });
342+
});

packages/ad-script/src/internal/target-annotation-serde.ts

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,18 @@ import { AppError } from '@agent-device/kernel/errors';
1717
const TARGET_ANNOTATION_TAG = 'agent-device:target-v1';
1818
// Captures the rest of the line verbatim: a line claiming the tag with a
1919
// garbage payload is a malformed v1 annotation, never an ordinary comment.
20-
const TARGET_ANNOTATION_LINE_RE = /^#\s*agent-device:target-v(\d+)(?:\s+(.*))?$/;
20+
//
21+
// The payload group is anchored on `\S` (CodeQL js/polynomial-redos,
22+
// #1536 review): `\s+` and a bare `.*` both accept whitespace, so a run of
23+
// separator characters that ultimately fails to match `$` (e.g. many tabs
24+
// with no trailing content) has exponentially many `\s+`/`.*` splits to
25+
// backtrack through. `\S` is `\s`'s complement, so it can never overlap with
26+
// the mandatory `\s+` before it — the split point is unique, no backtracking
27+
// possible. Behavior-preserving: the only caller (below) always matches
28+
// against an already-`.trim()`-ed line, whose last character (when the tag
29+
// matches at all) is never whitespace, so a payload section that `\S.*`
30+
// would reject (all-whitespace) can never reach this regex in practice.
31+
const TARGET_ANNOTATION_LINE_RE = /^#\s*agent-device:target-v(\d+)(?:\s+(\S.*))?$/;
2132

2233
export const TARGET_ANNOTATION_MAX_FIELD_BYTES = 256;
2334
export const TARGET_ANNOTATION_MAX_PAYLOAD_BYTES = 4096;

0 commit comments

Comments
 (0)