Skip to content

fix(seed): compare the seed secret in constant time - #2040

Open
liutingqiu wants to merge 1 commit into
Commitlabs-Org:masterfrom
liutingqiu:fix/1940-timing-safe-seed-secret
Open

liutingqiu wants to merge 1 commit into
Commitlabs-Org:masterfrom
liutingqiu:fix/1940-timing-safe-seed-secret

Conversation

@liutingqiu

Copy link
Copy Markdown

Context

Follows #1940. isSeedSecretValid decided whether the caller supplied the right SEED_SECRET with a plain string comparison:

const expected = process.env.SEED_SECRET;
if (!expected) return true;
return suppliedSecret === expected;   // ← short-circuits on first differing byte

=== stops at the first byte that differs, so the time taken to reject a guess grows with the number of correct leading bytes. Anyone who can time the seed route can recover the secret byte by byte.

The repo already has a correct implementation of this comparison, but it was a private helper inside csrf.ts, so seed.ts could not reach it:

// src/lib/backend/csrf.ts (before)
function safeEqualToken(a: string, b: string): boolean { ... }

What changed

  • New src/lib/backend/timingSafeEqual.ts — the constant-time comparison extracted into its own module with the reasoning documented in place. crypto.timingSafeEqual throws a RangeError on unequal buffer lengths, and that throw is itself a length oracle, so the length is checked first and a mismatch returns false (length is not treated as secret).
  • src/lib/backend/csrf.ts — drops its private copy and imports the shared helper. No behaviour change; the CSRF path already used this logic.
  • src/lib/backend/seed.ts — isSeedSecretValid now uses safeEqualToken, and an absent/empty header is rejected explicitly before the comparison so a missing secret can never be treated as a match. The "no SEED_SECRET configured → open" behaviour is unchanged.

Because safeEqualToken was private, extracting it instead of exporting it from csrf.ts keeps the CSRF module's surface unchanged while giving the shared primitive one home.

Scope note on the issue

#1940 has two halves. The second half ("mistyped sample data") is already fixed on master: Attestation in src/types/domain.ts now declares optional status/verdict/severity plus a required observedAt, and the SAMPLE_DATA in seed.ts type-checks against it. I deliberately did not touch anything for that half rather than change code that is not broken.

Tests

  • src/lib/backend/seed.test.ts (new, 11 tests) — had no test file at all before: no-secret-configured accepts anything; exact match accepted; missing header, empty header, shorter guess, longer guess and same-length-differs-in-last-byte all rejected; a length mismatch does not throw; plus the isSeedAllowed guard matrix and seedMockData refusals.
  • src/lib/backend/timingSafeEqual.test.ts (new, 7 tests) — the primitive directly, including the empty-vs-non-empty case and multi-byte characters where character count and byte length differ (the exact input that makes timingSafeEqual throw).
  • src/lib/backend/csrf.test.ts (17 tests) still passes, covering the refactor of the shared helper.

How to verify

npm ci
npx vitest run src/lib/backend/seed.test.ts src/lib/backend/timingSafeEqual.test.ts src/lib/backend/csrf.test.ts
npm run format:check
npx tsc --noEmit

Local results on this branch (Windows, node v24.21.0 — not the pinned Node 20 from .nvmrc, so CI on Node 20 remains the authoritative run):

  • The three suites above: 35/35 pass.
  • npx prettier --check and npx eslint on all changed files: clean.
  • npx tsc --noEmit: 58 errors, identical to the count on unmodified master in this environment, none in the changed or added files. The job is continue-on-error: true while the pre-existing backlog is burned down (see docs/CI.md).
  • Full npm run test:coverage: the failing-test set is a strict subset of master's, with no new file failing and none of the added/changed files failing. The run-to-run total moves by one because src/context/__tests__/CommitmentStatusContext.test.tsx > "uses slower poll interval for settled commitments" is flaky here — I reproduced it passing 2 of 3 local runs in isolation, and the master baseline log contains a different timing test from the same suite, not this one. Treat that single delta as environment flakiness rather than a regression; CI on Node 20 is authoritative.

Closes #1940

@vercel

vercel Bot commented Oct 1, 2026

Copy link
Copy Markdown

@liutingqiu is attempting to deploy a commit to the 1nonly's projects Team on Vercel.

A member of the Team first needs to authorize it.

isSeedSecretValid used a plain string comparison, which short-circuits on the first differing byte and leaks how much of SEED_SECRET a guess got right to anyone who can time the seed route.

Extract the constant-time helper that already existed privately in csrf.ts into its own module and use it from both call sites. A missing or empty header is now rejected before the comparison, so an absent secret can never be treated as a match.
@liutingqiu
liutingqiu force-pushed the fix/1940-timing-safe-seed-secret branch from 30473e7 to 15ebc2a Compare October 1, 2026 18:44

This branch has not been deployed

No deployments
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.

Non-timing-safe secret comparison and mistyped sample data in src/lib/backend/seed.ts

1 participant