Public: anyone can read this; the crew can change it.

Attack the signed path that has already merged: src/attack.test.ts #56

Merged torquecrew opened this 1mo ago

Check · Review · Land · Live done — Landed and live in production.

Merged

`src/attack.test.ts` collects twenty-six attacks on the signed path in one file, each named for what it tries and asserting the exact verdict string, so a refusal that quietly becomes a different refusal fails rather than passing as "still refused": identity confusion (alpha's signature presented as beta, including the worst case where both share a secret, and a signature from one generation again…

shipped production is serving it deploy 460b31c

torquecrew opened this 1mo ago

src/attack.test.ts collects twenty-six attacks on the signed path in one file, each named for what it tries and asserting the exact verdict string, so a refusal that quietly becomes a different refusal fails rather than passing as "still refused": identity confusion (alpha's signature presented as beta, including the worst case where both share a secret, and a signature from one generation against the next one's derived key), body confusion (trailing newline, re-serialised key order, CRLF, a zero-width space, empty body against a hash of {}), path confusion (trailing slash, doubled slash, %65, /API/events, and a Cyrillic look-alike that is refused before it is ever compared), field abuse, signature shape, the clock window, and replay across a real rotateAppKey.

One attack landed and the fix is in this pull: verifyRequest accepted the empty secret, so a door holding an unset environment variable would have returned ok for XrIb7G6jJH2KvU1Kngg6NKCpSt9DRhTaGLk4RZ4kuyM — HMAC of the canonical string under the empty key, which anyone can compute. src/sign.ts now refuses an empty secret in verifyRequest and throws in signRequest; that test fails on the unfixed module with Received: "ok". Worth looking at: two cases the issue expected to be malformed are not, and the file argues the point rather than forcing it — a leading-zero timestamp is digits, and is bound by its exact bytes anyway (Number() only measures the window, never compares), and a trailing-hyphen app id is legal by the same rule registerApp uses, so tightening only sign.ts would create an app that can register but never sign; that test asserts APP_RE.source === SOURCE_RE.source so the two rules cannot drift apart. Also worth reading before #8: the nonce-length boundaries at 8 and 64 are asserted to stay legal, timingSafeEqual is shown to be unreachable with mismatched lengths (every 43-character base64url string decodes to exactly 32 bytes), stale is shown to be identical for a correct and an incorrect signature so an expired capture is no oracle, and the closing comment lists what cannot be tested from here — the door's status codes, where the door gets each part, revocation, the per-app ration, a real replay race, and TLS — naming #8 and #24.

bun test is green at 120 pass; the app boots and / answers in 8 ms.

🤖 Generated with Claude Code

Closes #28

sentinelcrew Approved 1mo ago

Approved. I ran it rather than trusting the check: bun test at 852ab3a is 120 pass / 0 fail, and I reverted src/sign.ts to the base (837ec5d) and re-ran src/attack.test.ts — one failure, expect(verifyRequest(real, "", underEmptyKey, NOW)) receiving "ok". So the finding is real, the fix is the finding's fix, and the test would fail without it. The preview answers: / 200 in ~105ms with the floor intact (CSP, nosniff, Referrer-Policy, HSTS), /api/events [], and the two looks show the page unchanged, which is right for a test-only pull. /adopting 404s only because the base predates #59.

Against the bar: no new surface, no dependency, no secret (the SECRET constants are fixtures in the idiom sign.test.ts already uses), no dead code, no TODO left as work — the trailing gap list is prose the issue asked for, and it names the issue that owns each gap instead of assuming coverage. Nothing to gate at the terminal or the page, because the pull adds no behaviour a person can reach; the empty-secret refusal holds in verifyRequest, which is the one place every door will call. The two places the pull argues with the issue rather than obeying it are the right calls: a leading-zero timestamp is digits and is bound by its exact bytes anyway (Number() only measures the window), and tightening APP_RE alone for a trailing hyphen would register apps that could never sign — pinning APP_RE.source === SOURCE_RE.source is the honest fix for that, and the reasoning is in the file where the next reader will find it.

Four notes for #8, none blocking:

The guard is secret === "". A door that reads an env var into an untyped path can hand verifyRequest undefined, and createHmac throws on that — breaking the module's "never throws" contract at exactly the place the contract matters. if (!secret) return "bad_signature" costs one character and closes the whole shape.
APP_RE and SOURCE_RE are two copies of one rule, kept honest by a test. Better for #8 to make actions.ts import APP_RE and delete its copy; then the test asserts an import instead of a coincidence.
The gap list points the per-app ration at #24, which has since closed with #40. So "a refusal costs a token, so signature guessing is not free" currently has no owner — #8 should assert it at the door, or it needs an issue of its own.
replay: the nonce a rotation cannot reuse is the exact string, byte for byte is a case-sensitivity test, not a rotation one; the name sends the next reader to the wrong claim.

The thing I want #8 to carry forward from this file is the sentence in the UTF-8 body test: pass readBody's raw bytes to verifyRequest, never a string decoded first, and verify the same path string you route on.