T-0162: triage the CodeQL alerts by measuring them - #23
Merged
Conversation
Ten sat open on main, every one rated `high` by its rule. The card said the work was "saying which", and that was literal: the answer came from a benchmark, not from reading the code. `js/polynomial-redos`, four sites, growth at 16x the input — acceptance.ts 1x, records/index.ts 1x, docs/validation.ts 217x, docs/docs.ts 230x. Two noise, two quadratic, and the quadratic ones worse than reading suggested: 43.6s on a 128KB document body, in the scan the doctor runs for every document. The link target now excludes newlines and caps at 1024 — longer than any POSIX path, and `data:` URIs are skipped by the scheme test anyway — and the trailing slash strip is a slice loop. `js/xss-through-dom` in Markdown.tsx: React 19 does block `javascript:` via sanitizeURL, so nothing was exploitable. The allowlist is ours now. A record body is written by whichever agent held the card, and a defence that lives in a dependency's minor version is one you find out about by losing it. `js/regex-injection` in search.ts is the reverse error and stays open. Three caps — 256-char pattern, imsu flags, 20,000-char body — and none of them bounds backtracking: `/(a+)+$/` takes 57s against a 32-character body. Accepted with the measurement, tracked as T-0190. Criterion 3 — a new alert on merged code reaching somebody without a PR comment — is a daily baseline job comparing open alerts against a committed accepted list, the shape strict-baseline.json and doctor-baseline.json already use. Daily rather than on push because the alerts API serves the previous analysis until the new one finishes processing. Criterion 1 is left unchecked on purpose; the card says why. 328 tests, ratchet held at 494, doctor 0/0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SSP9wGYGj6SU4e5Nqy9McA
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…nd it CodeQL reported `docs/validation.ts` again on the branch that fixed it, against a different input, and it was right. The first pass bounded the link *target* and left `\[[^\]]*\]` — the label — with the identical shape one bracket earlier: 837ms at 32,000 `[` characters, 33.6s at 192,000. Fixing one half of a quadratic leaves a quadratic. The budget test now runs both shapes, repeated to a common *length* rather than a common count: `[` is one character and `[](` is three, so counting repetitions fed the label a body a third the size, and the ceiling that gave 5× detection on one shape gave 1.8× on the other. The `js/xss-through-dom` alert is accepted, with the flow read out of the SARIF instead of guessed. It is BodyEditor `event.target.value` → draft → Inspector `savedBody` → Markdown `source` → `href`. Real, and closed by the scheme allowlist, which CodeQL does not recognise as a sanitizer. My first reading of it — a `document` local shadowing the DOM global in Docs.tsx — was wrong, which is the argument for downloading the SARIF rather than grepping for a plausible source. The baseline job loses its `pull_request` trigger. It failed on its own first run, correctly: the alerts API answers for `main`, so a pull request that fixes three alerts sees them still open and reports the state it exists to change. Daily is the honest cadence for a question only `main` can answer. 328 tests, ratchet held at 494, doctor 0/0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SSP9wGYGj6SU4e5Nqy9McA
…re we do `js/xss-through-dom` on the Markdown `href` is a false positive: the flow is real and the sink is closed. Dismissal is the mechanism for that, and it is what makes the PR check honest rather than permanently red — so it moves out of the accepted list, which is for alerts that are open on purpose. But a reason that lives only in the Security tab is the exact failure T-0162 was opened about, so the flow goes in `safe-url.ts` next to the guard it explains, along with the fact that it will return at a new line number the next time that file is edited around it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SSP9wGYGj6SU4e5Nqy9McA
illodev
added a commit
that referenced
this pull request
Aug 5, 2026
PR #22 and #23 merged green across ubuntu, macos and windows on node 22 and 24, plus smoke, doctor and codeql. That is the evidence these were waiting for, and two of them were waiting for it specifically: T-0166 and T-0179 exist because Windows runners failed, and Windows has now passed twice. T-0178 stays in review, because checking rather than assuming turned up the gap it shipped admitting to. Its reachability test sets PATH to the empty string, which every platform reaches identically — the directory loop does nothing, so PATHEXT is split and never used. The positive lookup had no test. The new one writes `workfile-hooks.cmd` on Windows and `workfile-hooks` elsewhere, puts that directory on PATH, and asserts the runtime resolves; on Windows it also asserts a bare extensionless file does not count, which is exactly what npm does not install. Forcing the win32 branch on Linux fails it, so the extension candidates are load-bearing. It closes when a Windows runner has executed it, not before. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SSP9wGYGj6SU4e5Nqy9McA
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
2 participants
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.
Closes T-0162. Ten CodeQL alerts sat open on
main, every one ratedhighbyits rule, and the
codeqljob passed on every run — it uploads results, itdoes not read them.
The card said the work was "saying which are real". That turned out to be
literal: the answer came from a benchmark, not from reading the code, and it
was wrong in both directions.
js/polynomial-redos— two of four are realGrowth at 16× the input:
cards/acceptance.tsITEMrecords/index.tsQUERY_TOKENdocs/validation.tslink scandocs/docs.tstrailing slashThe two real ones were worse than reading had suggested. A document body of
unclosed Markdown links took 43.6 seconds at 128KB, in the link check the
doctor runs for every document in the workspace.
true of a Markdown link independently of the performance argument. The cost
is that a target past 1024 stops being checked — nothing local can be that
long (POSIX caps a path at 4096), and the only things that reach it are
data:URIs, which the scheme test already skips./\/+$/.js/xss-through-dom— not exploitable, fixed anywayReact 19 replaces a
javascript:URL with a throwing stub, sohref={link[2]}was never live. That is exactly the reason to move the rule: the UI renders
record bodies, a record body is written by whichever agent held the card, and a
defence that lives in a dependency's minor version is one you find out about by
losing it.
ui/src/safe-url.tsrefuses any scheme outsidehttps,httpandmailto,plus protocol-relative targets. A refused link renders as its own text — the
words a human wrote survive, the link does not.
js/regex-injection— the reverse error, and it stays openThis one looked handled: a 256-character pattern cap, an
imsuflagallowlist, a 20,000-character body cap. Three guards, none of which bounds
backtracking.
/(a+)+$/is six characters and passes all three:Two more characters is four times the work; against the 20,000-character cap it
does not finish. On the CLI that is self-inflicted.
/api/v2/search?q=takesthe same query, and
workfile ui --host 0.0.0.0puts it on the network — theconfiguration the
codeqljob's own comment calls "unauthenticated read andwrite access to a repository".
Not dismissed, because it is not false. Not fixed here, because the fix is a
choice between a worker deadline, RE2 and dropping user-supplied regex.
Accepted with the measurement and tracked as T-0190.
The gate — criterion 3
An alert on a PR gets commented inline and looked at within a minute. An alert
on merged code has no messenger, which is how ten of these sat open for a week.
codeql-baseline.ymlcompares open alerts against a committed accepted list —the same ratchet shape
strict-baseline.jsonanddoctor-baseline.jsonalready use, so debt is visible in the diff and only an unaccounted alert
breaks the build. It runs daily rather than on push: the alerts API serves the
previous analysis until the new one finishes processing, so a check that races
the upload reports the state it was called to replace. A day is the honest
latency and it is bounded, which "nothing ever looks" was not.
Keyed by rule and path, not by alert number — a number belongs to an instance
and moves with the line, which is how the same
acceptance.tspattern arrivedas a new alert on #22 after the file was edited around it.
codeql-config.ymlalso excludestest/fixtures/.legacy-v1/is a snapshotof the pre-workfile board this package migrates away from: checked in as
input, never imported, never run. Analysing it reports the bug we keep on
purpose.
Criterion 1 is left unchecked
"Every open alert is either fixed or dismissed with a stated reason." The two
search.tsones are neither — they are accepted in a committed file with acard. The intent is met and the letter is not, and checking the box because the
spirit is met is what T-0174 exists to catch.
Verification
pnpm run checkgreen: 328 tests + 7 search-local, strict ratchet held at 494,doctor 0 errors / 0 warnings.
Each fix was vacuity-checked by restoring the broken form in the built
dist:the link budget fails at 9,663ms against its 2,000ms ceiling, the folder budget
at 2,846ms against 500ms, and the scheme test on
javascript:alert(1).The two budgets are wall-clock, which T-0166 and T-0179 are both open scars
about, so the test carries the margin table rather than a bare number — and
reports the measured time on every run. The ceilings differ because the margins
do: a shared one would have left the folder case 1.4× above what it must
exceed. The "in suite" column exists because the link scan nearly doubles under
the full run, so the headroom a solo measurement would have claimed is not the
headroom there is.
🤖 Generated with Claude Code
https://claude.ai/code/session_01SSP9wGYGj6SU4e5Nqy9McA