fix(api): use timing-safe compare for x-probe-secret (#3803)#3820
Conversation
Issue #3803 flagged that api/seed-contract-probe.ts used `secret !== expected` for the x-probe-secret header — a timing oracle on RELAY_SHARED_SECRET, which is shared across Vercel functions, the Railway relay, and probe endpoints. Every other internal-auth path in the codebase uses the `timingSafeEqual` helper from server/_shared/internal-auth.ts. Same pattern as the #3705 and #3756 fixes: bring the outlier site in line with the safer convention the rest of the codebase already uses. The helper exists, the cost is one import + a one-line call change. Changes: 1. Export `timingSafeEqual` from server/_shared/internal-auth.ts (was module-private). Added a JSDoc explaining the use case for callers whose secret comes in via a non-Authorization header (x-probe-secret, x-internal-key, etc.) — they need the primitive, not the Bearer-token-shaped `authenticateInternalRequest`. 2. api/seed-contract-probe.ts: import + replace `!==` with `timingSafeEqual(secret, expected)` (with a `?? ''` guard for missing header). Updated comment to mention the timing-oracle rationale and cross-reference issue #3803. New regression test (tests/no-non-timing-safe-secret-compare.test.mts): - Walks all api/*.{ts,js,mjs,cjs} files (skipping tests and vendored dirs). - Greps for `(secret|token|bearer|sharedSecret|...) (===|!==) (process.env.* | expected*)` — the shape of a timing-leaky secret comparison. - Comment-stripped to avoid false-positives on doc comments. - An allowlist exists for future documented exceptions (currently empty). - A second targeted test asserts seed-contract-probe.ts specifically uses the helper, so this exact regression can't reappear silently. Test plan: - [x] 2/2 new tests pass - [x] `npm run typecheck` clean - [x] Manually verified the source-grep regex catches the pre-fix shape (replacing the new call with the old `!==` makes test #1 fail with the right file in the violations list). Closes #3803
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Greptile SummaryThis PR fixes a timing oracle in
Confidence Score: 3/5The auth fix itself is correct and consistent with the rest of the codebase, but the The server/_shared/internal-auth.ts — the early-return on length mismatch needs to be replaced with a constant-time padded comparison before the export is fully safe for all callers.
|
| Filename | Overview |
|---|---|
| server/_shared/internal-auth.ts | Exports timingSafeEqual as a public primitive; the implementation has a length-leaking early-return (line 36) that lets callers observe whether a guess matches the secret's byte length via timing, undermining the full timing-safety guarantee the export promises. |
| api/seed-contract-probe.ts | Replaces the !== direct string comparison with await timingSafeEqual(secret, expected) and adds a ?? '' null-coalescing guard; the auth logic change is correct and follows the established codebase pattern. |
| tests/no-non-timing-safe-secret-compare.test.mts | Adds a source-grep regression test that walks api/ and asserts no (secretVar) === (env) patterns remain; the regex misses reversed comparisons (process.env.X === secret) which is a minor coverage gap. |
Sequence Diagram
sequenceDiagram
participant Client
participant Handler as probe handler
participant TSE as timingSafeEqual
participant Env as Env vars
Client->>Handler: POST with x-probe-secret header
Handler->>Handler: read header value (null-coalesced to empty string)
Handler->>Env: read shared relay secret
alt env var not configured
Handler-->>Client: 503 not-configured
else env var present
Handler->>TSE: await timingSafeEqual(headerValue, envValue)
TSE->>TSE: encode both strings
Note over TSE: early-return if lengths differ (timing leak)
TSE->>TSE: HMAC sign + constant-time XOR compare
TSE-->>Handler: true or false
alt mismatch
Handler-->>Client: 401 unauthorized
else match
Handler->>Handler: run probe checks
Handler-->>Client: 200 or 503 with results
end
end
Comments Outside Diff (1)
-
server/_shared/internal-auth.ts, line 36-45 (link)The early-return on mismatched buffer lengths still leaks the byte length of the secret via timing. An attacker who controls the
aargument (e.g. thex-probe-secretheader value) can binary-search the length ofRELAY_SHARED_SECRETby observing whether responses arrive in the fast-path (wrong length) vs. the full HMAC path (matching length). Now that this function is exported as the canonical timing-safe primitive, callers will reasonably expect it to resist length enumeration too. Padding both inputs tomaxLenbefore the HMAC call removes the distinguishable fast path.
Reviews (1): Last reviewed commit: "fix(api): use timing-safe compare for x-..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
… review) Greptile P2 review on PR #3820 caught that the source-grep regex only matched the forward operand order: secret === process.env.FOO ← caught process.env.FOO === secret ← MISSED (yoda-style or copy-paste) A maintainer who wrote the reverse form (common when porting code from languages where yoda comparisons are idiomatic, or when auto-completing from `process.env.FOO`) would have silently slipped the timing oracle past the guard. Fix: split the pattern into two named arms (forward + reverse) and combine via alternation. The reverse arm anchors on the env-var-ish LHS and matches the secret-variable RHS, mirroring the forward arm. Also added a meta-test that exercises the regex against 10 representative strings (3 forward, 3 reverse, 4 negative) and asserts the match/no-match contract. This pins the coverage so a future "simplification" of the regex can't silently regress it. 3/3 tests pass; typecheck clean.
1 participant
Summary
Per issue #3803, `api/seed-contract-probe.ts` used `secret !== expected` for the `x-probe-secret` header — a timing oracle on `RELAY_SHARED_SECRET`, which is shared across Vercel functions, the Railway relay, and probe endpoints. Every other internal-auth path in the codebase uses the `timingSafeEqual` helper from `server/_shared/internal-auth.ts`.
Same template as the #3705 and #3756 fixes shipped earlier today: bring the outlier site in line with the safer convention the rest of the codebase already uses. The helper exists, the cost is one import + a one-line call change.
Change
Export `timingSafeEqual` from `server/_shared/internal-auth.ts` (was module-private). JSDoc explains the use case: callers whose secret comes in via a non-`Authorization` header (`x-probe-secret`, etc.) need the primitive, not the Bearer-token-shaped `authenticateInternalRequest`.
`api/seed-contract-probe.ts`: import + replace `!==` with `await timingSafeEqual(secret, expected)` (with a `?? ''` guard for missing header).
Regression test (`tests/no-non-timing-safe-secret-compare.test.mts`):
Test plan
Severity context
The reporter rated this Medium-P2. Practical attack against this specific endpoint is borderline (internal-only ops probes, high network jitter dominates per-byte timing differences for a 32-char secret), but the fix is one line and brings the file in line with the codebase's existing pattern — no good reason not to do it.
Closes #3803