maximus/vps-health-api

Fork 0

fix(auth): compare HEALTH_TOKEN in constant time via tokenMatches #30

Merged

maximus merged 3 commits from issue-17-timing-safe-health-token into main

2026-09-27 23:27:45 +00:00

maximus commented

2026-09-27 20:06:18 +00:00

Owner

Fixes #17

What

The read path compared the bearer with header === + template literal, which stops at the first differing byte and leaks the HEALTH_TOKEN's length and prefix through response time. checkAuth() now has a single comparison for both token kinds: exact Bearer scheme check, then tokenMatches() (SHA-256 digests, then timingSafeEqual, so lengths are always equal; cf. GHSA-376v-xgjx-7mfr). Only the expected token varies.

Semantics unchanged: exact match, case-sensitive scheme, single space separator. Every pre-existing auth test (#12) passes untouched.

Tests (`tests/auth.test.js`, 29 -> 67 cases)

Behavioural guard: a spy on crypto.timingSafeEqual, set before a fresh require (index.js destructures it at load), requires every request to compare two 32-byte digests: 6 read routes x (shorter wrong token, same-length wrong token), the right token on /defenseurs, and the ingest route. This catches a short-circuiting ===, a per-kind early return, and raw strings behind a length check.
Source pins (comments stripped): no equality against a Bearer template literal or concatenation anywhere in index.js; in checkAuth(), one tokenMatches( call, one return null, and every equality operator allowlisted (both operand orders); in tokenMatches(), both sides hashed, one timingSafeEqual, no .length.
Contract cases: near-miss tokens (prefix, suffix, double space) on all 6 read routes; separator other than a single space (Bearer\t<token>, BearerX<token>); empty token pinned on tokenMatches("", TOKEN) === false (a trailing-space Bearer is trimmed to Bearer by the HTTP parser, already covered).

Near-miss cases describe the contract but would also pass against a naive ===; the timing property is guarded by the spy and the pins.

Mutation check

All restored from a scratchpad copy, diff empty, suite green after.

Mutation	Red
Old two-branch template-literal expression	15
Read branch `"Bearer " + expected`	15
Single branch `header.slice(7) === expected`	15
Read branch `want === header` (review 1)	14
Early `if (!isIngest) return want === header ? ...` (review 1)	14
tokenMatches -> `provided === expected` (review 1)	15
tokenMatches -> length check + raw timingSafeEqual (review 1)	15
`startsWith("Bearer")` without the space (review 2)	2

npm test: 301/301 (263 before this PR)
bash test-curl.sh: 45/45. Port 3099 was held by another project's next-server locally, so it was run with ports rewritten on the fly (3199/3198), script unchanged.

Post-merge (not done by this PR)

Manual Coolify redeploy (no auto-deploy, see CLAUDE.md)
Smoke on the 6 read routes: /health, /defenseurs, /defenseurs/state, /defenseurs/findings?project=..., /reports/scans?date=..., /hosts: 200 with token, 401 without
Admin dashboard card (Vercel) loads

🤖 Generated with Claude Code

Fixes #17 ## What The read path compared the bearer with `header === ` + template literal, which stops at the first differing byte and leaks the HEALTH_TOKEN's length and prefix through response time. `checkAuth()` now has a single comparison for both token kinds: exact `Bearer ` scheme check, then `tokenMatches()` (SHA-256 digests, then `timingSafeEqual`, so lengths are always equal; cf. GHSA-376v-xgjx-7mfr). Only the expected token varies. **Semantics unchanged**: exact match, case-sensitive scheme, single space separator. Every pre-existing auth test (#12) passes untouched. ## Tests (`__tests__/auth.test.js`, 29 -> 67 cases) - **Behavioural guard**: a spy on `crypto.timingSafeEqual`, set before a fresh require (index.js destructures it at load), requires every request to compare two 32-byte digests: 6 read routes x (shorter wrong token, same-length wrong token), the right token on `/defenseurs`, and the ingest route. This catches a short-circuiting `===`, a per-kind early return, and raw strings behind a length check. - **Source pins** (comments stripped): no equality against a `Bearer` template literal or concatenation anywhere in `index.js`; in `checkAuth()`, one `tokenMatches(` call, one `return null`, and every equality operator allowlisted (both operand orders); in `tokenMatches()`, both sides hashed, one `timingSafeEqual`, no `.length`. - **Contract cases**: near-miss tokens (prefix, suffix, double space) on all 6 read routes; separator other than a single space (`Bearer\t<token>`, `BearerX<token>`); empty token pinned on `tokenMatches("", TOKEN) === false` (a trailing-space `Bearer ` is trimmed to `Bearer` by the HTTP parser, already covered). Near-miss cases describe the contract but would also pass against a naive `===`; the timing property is guarded by the spy and the pins. ## Mutation check All restored from a scratchpad copy, diff empty, suite green after. | Mutation | Red | |---|---| | Old two-branch template-literal expression | 15 | | Read branch `"Bearer " + expected` | 15 | | Single branch `header.slice(7) === expected` | 15 | | Read branch `want === header` (review 1) | 14 | | Early `if (!isIngest) return want === header ? ...` (review 1) | 14 | | tokenMatches -> `provided === expected` (review 1) | 15 | | tokenMatches -> length check + raw timingSafeEqual (review 1) | 15 | | `startsWith("Bearer")` without the space (review 2) | 2 | - `npm test`: 301/301 (263 before this PR) - `bash test-curl.sh`: 45/45. Port 3099 was held by another project's `next-server` locally, so it was run with ports rewritten on the fly (3199/3198), script unchanged. ## Post-merge (not done by this PR) - [ ] Manual Coolify redeploy (no auto-deploy, see CLAUDE.md) - [ ] Smoke on the 6 read routes: `/health`, `/defenseurs`, `/defenseurs/state`, `/defenseurs/findings?project=...`, `/reports/scans?date=...`, `/hosts`: 200 with token, 401 without - [ ] Admin dashboard card (Vercel) loads 🤖 Generated with [Claude Code](https://claude.com/claude-code)

maximus added 1 commit 2026-09-27 20:06:19 +00:00

fix(auth): compare HEALTH_TOKEN in constant time via tokenMatches f70a7e928f

The read path still compared the bearer with `header === \`Bearer ${token}\``,
which stops at the first differing byte and leaks the token's length and
prefix through response time. checkAuth() now has a single comparison for
both token kinds: `Bearer ` scheme check, then tokenMatches() (SHA-256
digests, then timingSafeEqual, so lengths are always equal). Only the
expected token varies.

Semantics are unchanged: exact match, case-sensitive scheme. Every existing
auth test passes untouched.

Tests: near-miss tokens (prefix, suffix, double space) on all six read
routes, an empty-token assertion on the helper, and a source pin that
forbids direct equality against a Bearer string and requires exactly one
tokenMatches() call in checkAuth(). A timing test would be flaky, so the pin
is the regression guard: three mutations back to direct comparison each
turn it red.

Fixes #17

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

maximus commented

2026-09-27 20:08:38 +00:00

Author

Owner

Review — APPROVE

Summary: The fix is correct and minimal. checkAuth() now has a single comparison for both token kinds (Bearer scheme check, then tokenMatches() on SHA-256 digests). The semantics are unchanged: exact match and a case-sensitive scheme, and the fail-closed and 404-before-401 order is intact. Suite: 284/284 locally on f70a7e9.

Checks

No secrets in the diff. The issue closing keyword (Fixes #17) is in the commit message. The commit follows conventional format.
header.startsWith("Bearer ") short-circuits on the scheme only, which is public, so nothing leaks. slice(7) then tokenMatches: the hashing makes the lengths equal, so timingSafeEqual never throws.
CLAUDE.md is updated consistently (#17 contract, 263 -> 284 cases).

Reviewer mutations (on a copy outside the repo, tree left untouched)

Mutation	Result
Read branch as `isIngest ? ... : want === header` (with `want = "Bearer " + expected`)	1 red (isIngest ternary) — caught
Early return `if (!isIngest) return want === header ? null : 401` before the shared expression	284 green
`tokenMatches()` body reduced to `return provided === expected`	284 green
`tokenMatches()` without hashing: `provided.length === expected.length && timingSafeEqual(raw)`	284 green

Suggestions (non-blocking)

The helper itself is not pinned. The source pin guards how checkAuth() calls tokenMatches(), not what tokenMatches() does. A regression to ===, or to raw timingSafeEqual with a length short-circuit (the exact case cited in GHSA-376v-xgjx-7mfr), passes the whole suite. Since the helper now protects every route, it is worth a pin on its body too: two createHash("sha256") calls, one timingSafeEqual(, and no ===/.length on provided/expected. This gap predates the PR (the helper was already unpinned on the ingest path), but the new CLAUDE.md text ("la garde est une epingle sur le source") suggests broader coverage than there is.
The pin can be bypassed by indirection plus reversed operands (mutation 2): const want = "Bearer " + expected; want === header. The regex \bheader\s*[!=]==? only catches header on the left. Covering both sides ([!=]==?\s*header\b) and requiring that checkAuth() contain exactly one return null would close this case cheaply. A structural pin will never be complete, so this is a best-effort hardening.
Minor: the edited comment in auth.test.js L120-121 now runs well past the usual width (a reflow is enough).

Post-merge

Nothing to add to the PR checklist (manual Coolify redeploy, smoke on the 6 read routes, admin card).

## Review — APPROVE **Summary**: The fix is correct and minimal. `checkAuth()` now has a single comparison for both token kinds (`Bearer ` scheme check, then `tokenMatches()` on SHA-256 digests). The semantics are unchanged: exact match and a case-sensitive scheme, and the fail-closed and 404-before-401 order is intact. Suite: 284/284 locally on `f70a7e9`. ### Checks - No secrets in the diff. The issue closing keyword (`Fixes #17`) is in the commit message. The commit follows conventional format. - `header.startsWith("Bearer ")` short-circuits on the scheme only, which is public, so nothing leaks. `slice(7)` then `tokenMatches`: the hashing makes the lengths equal, so `timingSafeEqual` never throws. - CLAUDE.md is updated consistently (#17 contract, 263 -> 284 cases). ### Reviewer mutations (on a copy outside the repo, tree left untouched) | Mutation | Result | |---|---| | Read branch as `isIngest ? ... : want === header` (with `want = "Bearer " + expected`) | 1 red (isIngest ternary) — caught | | Early return `if (!isIngest) return want === header ? null : 401` before the shared expression | **284 green** | | `tokenMatches()` body reduced to `return provided === expected` | **284 green** | | `tokenMatches()` without hashing: `provided.length === expected.length && timingSafeEqual(raw)` | **284 green** | ### Suggestions (non-blocking) 1. **The helper itself is not pinned.** The source pin guards how `checkAuth()` calls `tokenMatches()`, not what `tokenMatches()` does. A regression to `===`, or to raw `timingSafeEqual` with a length short-circuit (the exact case cited in GHSA-376v-xgjx-7mfr), passes the whole suite. Since the helper now protects every route, it is worth a pin on its body too: two `createHash("sha256")` calls, one `timingSafeEqual(`, and no `===`/`.length` on `provided`/`expected`. This gap predates the PR (the helper was already unpinned on the ingest path), but the new CLAUDE.md text ("la garde est une epingle sur le source") suggests broader coverage than there is. 2. **The pin can be bypassed by indirection plus reversed operands** (mutation 2): `const want = "Bearer " + expected; want === header`. The regex `\bheader\s*[!=]==?` only catches `header` on the left. Covering both sides (`[!=]==?\s*header\b`) and requiring that `checkAuth()` contain exactly one `return null` would close this case cheaply. A structural pin will never be complete, so this is a best-effort hardening. 3. Minor: the edited comment in `auth.test.js` L120-121 now runs well past the usual width (a reflow is enough). ### Post-merge Nothing to add to the PR checklist (manual Coolify redeploy, smoke on the 6 read routes, admin card).

maximus commented

2026-09-27 20:13:17 +00:00

Author

Owner

Follow-up to review 16536 (commit on top of f70a7e9).

tokenMatches() now guarded. Behavioural: spy on crypto.timingSafeEqual (set before a fresh require, since index.js destructures at load) requires every request to compare two 32-byte digests: 6 read routes x (short wrong token, same-length wrong token), right token on /defenseurs, and the ingest route. Structural: tokenMatches() must hash both sides, call timingSafeEqual once, no .length, no equality beyond the type guards. CLAUDE.md wording fixed.
Pin blind spots closed. checkAuth(): every equality operator is allowlisted (typeof header === "string", route.tokenKind === "ingest"), both operand orders, and exactly one return null.
MALFORMED comment rewrapped.

Mutations (restored from a scratchpad copy, diff empty, suite green after):

Mutation	Red
R1 read branch `want === header`	14
R2 early `if (!isIngest) return want === header ? null : 401`	14
R3 tokenMatches -> `provided === expected`	15
R4 tokenMatches -> length check + raw timingSafeEqual	15
M1 old template-literal branch	15
M2 `"Bearer " + expected` branch	15
M3 `header.slice(7) === expected`	15

The spy alone catches all seven. npm test 299/299, test-curl.sh 45/45 (ports 3199/3198, 3099 held by another project locally).

Follow-up to review 16536 (commit on top of f70a7e9). 1. **tokenMatches() now guarded.** Behavioural: spy on `crypto.timingSafeEqual` (set before a fresh require, since index.js destructures at load) requires every request to compare two 32-byte digests: 6 read routes x (short wrong token, same-length wrong token), right token on /defenseurs, and the ingest route. Structural: tokenMatches() must hash both sides, call timingSafeEqual once, no `.length`, no equality beyond the type guards. CLAUDE.md wording fixed. 2. **Pin blind spots closed.** checkAuth(): every equality operator is allowlisted (`typeof header === "string"`, `route.tokenKind === "ingest"`), both operand orders, and exactly one `return null`. 3. MALFORMED comment rewrapped. Mutations (restored from a scratchpad copy, diff empty, suite green after): | Mutation | Red | |---|---| | R1 read branch `want === header` | 14 | | R2 early `if (!isIngest) return want === header ? null : 401` | 14 | | R3 tokenMatches -> `provided === expected` | 15 | | R4 tokenMatches -> length check + raw timingSafeEqual | 15 | | M1 old template-literal branch | 15 | | M2 `"Bearer " + expected` branch | 15 | | M3 `header.slice(7) === expected` | 15 | The spy alone catches all seven. `npm test` 299/299, `test-curl.sh` 45/45 (ports 3199/3198, 3099 held by another project locally).

maximus referenced this pull request from a commit

2026-09-27 20:13:18 +00:00

test(auth): guard tokenMatches itself and close the pin's blind spots

maximus added 1 commit 2026-09-27 20:13:19 +00:00

test(auth): guard tokenMatches itself and close the pin's blind spots 5074f95d0a

Review of #30 found four mutations that left the suite green: an early
success return for the read kind, a reversed `want === header` comparison,
and tokenMatches() reduced to `===` or to raw strings behind a length check
(the GHSA-376v-xgjx-7mfr shape).

Behavioural guard: a spy on crypto.timingSafeEqual, set before a fresh
require of index.js, requires every request (6 read routes, short and
same-length wrong tokens, the right token, the ingest route) to compare two
32-byte digests. Source pins now also allowlist every equality operator in
checkAuth() (both operand orders), require a single `return null`, and check
that tokenMatches() hashes both sides without a length test.

All seven mutations (the reviewer's four, the original three) now turn
13-15 tests red. CLAUDE.md no longer claims the pin covers the helper.

Refs #17

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

maximus commented

2026-09-27 20:15:02 +00:00

Author

Owner

Review — APPROVE (head `5074f95`)

Summary: checkAuth() has a single comparison path for both token kinds: the Bearer scheme check, then tokenMatches() on SHA-256 digests. The follow-up commit closes the guard gaps from the earlier review with a spy on timingSafeEqual plus source pins. Semantics are unchanged, the fail-closed paths and the routing-before-auth order are intact, and there are no secrets, .only or .skip in the diff. The commits follow conventional format, and Fixes #17 is in the message of f70a7e9.

Verification (my own mutations, on a scratchpad copy of the tree; the repo was not touched)

Suite on the head: 299/299 (263 + 36; the 65 cases in auth.test.js match CLAUDE.md).

Mutation	Result
Old two-branch expression (`header === \`Bearer ${expected}``)	15 red
`if (!expected.startsWith(provided)) return false;` before hashing	13 red (spy never called)
`return a.equals(b)` instead of `timingSafeEqual`	15 red
`Buffer.compare(a,b) === 0 \|\| timingSafeEqual(a,b)`	1 red (pin on the operators)
`timingSafeEqual(a,b) && provided.localeCompare(expected) === 0`	0 red. This is an equivalent mutant: the raw comparison only runs once the digests already match, so nothing leaks. Not a finding.
`startsWith("Bearer")` (space dropped)	0 red (see suggestion 1)

test-curl.sh was not rerun: port 3099 is held locally by another process. The socket-level surface is unchanged by this PR.

Suggestions (non-blocking)

Separator after the scheme is not pinned. If the check becomes startsWith("Bearer"), the suite stays green, and Bearer\t<token> or BearerX<token> would be accepted (slice(7) skips the 7th character, whatever it is). There is no timing leak, but it loosens the "exact match" contract. One ["tab instead of space", Bearer\t${TOKEN}] case in MALFORMED (__tests__/auth.test.js:124) closes the gap.
The PR body is stale after 5074f95: it still says 284/284 and describes only the source pin as the guard. Update it to 299 and mention the timingSafeEqual spy for the merge record.

Post-merge

Items from the PR body: manual Coolify redeploy, then a smoke test on the 6 read routes (200 with the token / 401 without) and on the admin card.

## Review — APPROVE (head `5074f95`) **Summary**: `checkAuth()` has a single comparison path for both token kinds: the `Bearer ` scheme check, then `tokenMatches()` on SHA-256 digests. The follow-up commit closes the guard gaps from the earlier review with a spy on `timingSafeEqual` plus source pins. Semantics are unchanged, the fail-closed paths and the routing-before-auth order are intact, and there are no secrets, `.only` or `.skip` in the diff. The commits follow conventional format, and `Fixes #17` is in the message of `f70a7e9`. ### Verification (my own mutations, on a scratchpad copy of the tree; the repo was not touched) - Suite on the head: **299/299** (263 + 36; the 65 cases in `auth.test.js` match CLAUDE.md). | Mutation | Result | |---|---| | Old two-branch expression (`header === \`Bearer ${expected}\``) | **15 red** | | `if (!expected.startsWith(provided)) return false;` before hashing | **13 red** (spy never called) | | `return a.equals(b)` instead of `timingSafeEqual` | **15 red** | | `Buffer.compare(a,b) === 0 \|\| timingSafeEqual(a,b)` | 1 red (pin on the operators) | | `timingSafeEqual(a,b) && provided.localeCompare(expected) === 0` | 0 red. This is an **equivalent mutant**: the raw comparison only runs once the digests already match, so nothing leaks. Not a finding. | | `startsWith("Bearer")` (space dropped) | **0 red** (see suggestion 1) | - `test-curl.sh` was not rerun: port 3099 is held locally by another process. The socket-level surface is unchanged by this PR. ### Suggestions (non-blocking) 1. **Separator after the scheme is not pinned.** If the check becomes `startsWith("Bearer")`, the suite stays green, and `Bearer\t<token>` or `BearerX<token>` would be accepted (`slice(7)` skips the 7th character, whatever it is). There is no timing leak, but it loosens the "exact match" contract. One `["tab instead of space", `Bearer\t${TOKEN}`]` case in `MALFORMED` (`__tests__/auth.test.js:124`) closes the gap. 2. **The PR body is stale** after `5074f95`: it still says 284/284 and describes only the source pin as the guard. Update it to 299 and mention the `timingSafeEqual` spy for the merge record. ### Post-merge Items from the PR body: manual Coolify redeploy, then a smoke test on the 6 read routes (200 with the token / 401 without) and on the admin card.

maximus referenced this pull request from a commit

2026-09-27 23:23:53 +00:00

test(auth): pin the single space after the Bearer scheme

maximus added 1 commit 2026-09-27 23:23:53 +00:00

test(auth): pin the single space after the Bearer scheme f006a1c05e

Second review of #30: `startsWith("Bearer")` followed by slice(7) would
accept any byte as the separator (`Bearer\t<token>`, `BearerX<token>`) and
the suite stayed green. Two MALFORMED cases now reject both; the mutation
turns them red.

Refs #17

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

maximus commented

2026-09-27 23:25:00 +00:00

Author

Owner

Review — APPROVE

Summary: checkAuth() now funnels both token kinds through a single tokenMatches() call (SHA-256 digests, then timingSafeEqual), which removes the early-exit === on HEALTH_TOKEN. The observable semantics are unchanged: exact Bearer scheme, case-sensitive, single space. The new tests do catch the defect when it is reintroduced.

Verification (reviewer's own mutations, on a copy outside the repo)

Suite on the head commit f006a1c: 301/301 (auth.test.js: 67/67), no .only/.skip.
M1, index.js from main (the old template-literal ===): 15 red.
M2, early if (!isIngest) return header === "Bearer " + expected ? null : …: 15 red.
M3, tokenMatches() on the raw strings with a byteLength guard and then timingSafeEqual (the GHSA-376v shape, written with byteLength so it slips past the .length pin): 15 red. The spy catches it where the source pin alone would not, so the two layers do complement each other.
Copy restored, diff against the working tree is empty.
test-curl.sh was not re-run: port 3099 is still held by a next-server locally (same as the PR body reports).

Checklist

Security: no secrets in the diff. The startsWith("Bearer ") check compares a public constant, not a secret, so it leaks nothing.
Fixes #17 is present in the message of f70a7e9. Commits follow conventional commit format.
CLAUDE.md is updated to match: 301 cases, and the #17 note is inverted.

Suggestions (non-blocking)

__tests__/auth.test.js, functionBody(): the pin's end boundary is the next ^function at the start of a line. If an arrow function (const x = () => …) is inserted between checkAuth and the next declaration, its equalities would count toward checkAuth's allowlist. That would give a false red, not a false green, so it is acceptable.
After the manual Coolify redeploy, also run the smoke check with a wrong token of the same length on at least one read route. It is cheap, and it confirms the image really contains the new index.js (the COPY lists files one by one).

## Review — APPROVE **Summary**: `checkAuth()` now funnels both token kinds through a single `tokenMatches()` call (SHA-256 digests, then `timingSafeEqual`), which removes the early-exit `===` on `HEALTH_TOKEN`. The observable semantics are unchanged: exact `Bearer ` scheme, case-sensitive, single space. The new tests do catch the defect when it is reintroduced. ### Verification (reviewer's own mutations, on a copy outside the repo) - Suite on the head commit `f006a1c`: **301/301** (auth.test.js: 67/67), no `.only`/`.skip`. - M1, `index.js` from `main` (the old template-literal `===`): **15 red**. - M2, early `if (!isIngest) return header === "Bearer " + expected ? null : …`: **15 red**. - M3, `tokenMatches()` on the raw strings with a `byteLength` guard and then `timingSafeEqual` (the GHSA-376v shape, written with `byteLength` so it slips past the `.length` pin): **15 red**. The spy catches it where the source pin alone would not, so the two layers do complement each other. - Copy restored, `diff` against the working tree is empty. - `test-curl.sh` was not re-run: port 3099 is still held by a `next-server` locally (same as the PR body reports). ### Checklist - Security: no secrets in the diff. The `startsWith("Bearer ")` check compares a public constant, not a secret, so it leaks nothing. - `Fixes #17` is present in the message of `f70a7e9`. Commits follow conventional commit format. - CLAUDE.md is updated to match: 301 cases, and the #17 note is inverted. ### Suggestions (non-blocking) 1. `__tests__/auth.test.js`, `functionBody()`: the pin's end boundary is the next `^function` at the start of a line. If an arrow function (`const x = () => …`) is inserted between `checkAuth` and the next declaration, its equalities would count toward `checkAuth`'s allowlist. That would give a false red, not a false green, so it is acceptable. 2. After the manual Coolify redeploy, also run the smoke check with a wrong token of the **same length** on at least one read route. It is cheap, and it confirms the image really contains the new `index.js` (the `COPY` lists files one by one).

maximus merged commit 2b35b78a0b into main

2026-09-27 23:27:45 +00:00

maximus deleted branch issue-17-timing-safe-health-token

2026-09-27 23:27:45 +00:00

maximus referenced this pull request from a commit

2026-09-27 23:27:46 +00:00

Merge pull request 'fix(auth): compare HEALTH_TOKEN in constant time via tokenMatches' (#30) from issue-17-timing-safe-health-token into main

No reviewers