Skip to content

fix: CodeQL's three open src alerts (bearer-header regex, req.files type) - #498

Merged
bbertucc merged 2 commits into
mainfrom
codeql-src-alerts
Oct 1, 2026
Merged

bbertucc merged 2 commits into
mainfrom
codeql-src-alerts

Conversation

@bbertucc

@bbertucc bbertucc commented Oct 1, 2026 •

Copy link
Copy Markdown
Member

Iris Maintainer Agent here.

Fixes CodeQL's three open alerts in src/. The ten in test/ are dismissed as test code.

  • src/auth/middleware.ts:88, src/routes/quality.ts:32 (polynomial ReDoS): /^Bearer\s+(.+)$/i becomes /^Bearer\s+(\S.*)$/i. \s+ and \S can't overlap, so there's no backtracking. The slow input needs a CR or LF, which Node rejects in a header, so this wasn't reachable over HTTP. An all-whitespace token was refused before and still is.
  • src/routes/sessions.ts:284 (type confusion): req.files is checked with Array.isArray instead of cast.

npm test: 1749 pass, 0 skipped. ./test/e2e.sh passes.

🤖 Generated with Claude Code

…ype)

The bearer regexes in middleware.ts and quality.ts become /^Bearer\s+(\S.*)$/i, which is
linear. An all-whitespace token was already refused and still is. sessions.ts reads req.files
with Array.isArray instead of a cast.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All six checks in the context pass. Verified each of the three changes is behaviour-preserving.

/^Bearer\s+(.+)$/i → /^Bearer\s+(\S.*)$/i (src/auth/middleware.ts:88, src/routes/quality.ts:32). The two regexes differ on exactly one input class: a header whose post-Bearer remainder is non-empty but all whitespace. The old form matched there (\s+ gives back one character, (.+) captures it) and match[1].trim() was ""; the new form does not match. Both end in the same 401, because apiToken (src/config.ts:664) and cfg.quality_token?.trim() (src/routes/quality.ts:109) each collapse a whitespace-only configured value to undefined/falsy, so neither comparison can ever have an empty right-hand side. "An all-whitespace token was refused before and still is" is accurate. The padded-token path the \S anchor could have broken is already pinned — test/one-identity.test.ts:386 sends Bearer s3cret and asserts 200.

The backtracking claim holds too: \s+ and \S are disjoint, so that boundary is forced, and the only remaining give-back is .* against $ — O(n) steps at O(1) each, linear rather than quadratic.

Array.isArray(req.files) (src/routes/sessions.ts:284) is strictly safer than the cast. With upload.array("images") req.files is always an array on success, so the old cast was sound in practice; had it ever been the .fields()-shaped record, the old code fell through files.length === 0 (undefined === 0 is false) into a for...of over a non-iterable and threw, where this answers 400.

Non-blocking notes

  • src/routes/quality.ts:32 — nothing pins that a whitespace-padded quality token still opens the endpoint. test/quality-route.test.ts:114-117 covers the rejections and line 88 the exact token, but there is no Bearer ${TOKEN} case, so the \S anchor's live edge is unverified on this route while the identical edge on the gate is pinned (test/one-identity.test.ts:386). Latent: behaviour is unchanged today, and the cost is only that a later edit to this regex could start rejecting a padded token with no test failing.

Accessibility impact: none — no pipeline stage, agent prompt, lint path or output header is touched.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@bbertucc

bbertucc commented Oct 1, 2026

Copy link
Copy Markdown
Member Author

Iris Maintainer Agent here.

Round 1: added test/quality-route.test.ts "a whitespace-padded token opens the endpoint". It fails if \s+ becomes \s. fetch strips trailing header whitespace, so it pins the leading padding, which is the edge the \S anchor decides.

🤖 Generated with Claude Code

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All six checks in the summary pass. The only change since 9370701 is 12 lines in test/quality-route.test.ts; the src/ changes are unchanged, and the package.json/package-lock.json hunks in the context diff are main's own bumps (#495, #496) arriving through the merge base, not this PR's work.

My earlier non-blocking note is fixed. test/quality-route.test.ts:132-141 now pins the live edge of the \S anchor on this route:

const res = await srv.get("", { headers: { authorization: `Bearer   ${TOKEN}  ` } });
assert.equal(res.status, 200);

The leading padding is the half that reaches the regex — /^Bearer\s+(\S.*)$/i has to let \s+ consume all three spaces and still find a non-space at the capture's first character — and that is the half a future edit could break. The comment is honest that undici strips the trailing padding before the header is sent, so the test does not claim to cover it. /v1/quality's padded-token path is now pinned the same way the gate's is at test/one-identity.test.ts:386.

Nothing else changed, so nothing else to raise.

Accessibility impact: none — the change is a single test; no pipeline stage, agent prompt, lint path or output header is touched.

@bbertucc
bbertucc merged commit 3a7430a into main Oct 1, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant