Repository navigation
fix(seed): compare the seed secret in constant time - #2040
Open
liutingqiu wants to merge 1 commit into
Open
liutingqiu wants to merge 1 commit into
liutingqiu wants to merge 1 commit into
Conversation
|
@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
force-pushed
the
fix/1940-timing-safe-seed-secret
branch
from
October 1, 2026 18:44
30473e7 to
15ebc2a
Compare
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
Follows #1940.
isSeedSecretValiddecided whether the caller supplied the rightSEED_SECRETwith a plain string comparison:===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, soseed.tscould not reach it:What changed
src/lib/backend/timingSafeEqual.ts— the constant-time comparison extracted into its own module with the reasoning documented in place.crypto.timingSafeEqualthrows aRangeErroron unequal buffer lengths, and that throw is itself a length oracle, so the length is checked first and a mismatch returnsfalse(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—isSeedSecretValidnow usessafeEqualToken, and an absent/empty header is rejected explicitly before the comparison so a missing secret can never be treated as a match. The "noSEED_SECRETconfigured → open" behaviour is unchanged.Because
safeEqualTokenwas private, extracting it instead of exporting it fromcsrf.tskeeps 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:Attestationinsrc/types/domain.tsnow declares optionalstatus/verdict/severityplus a requiredobservedAt, and theSAMPLE_DATAinseed.tstype-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 theisSeedAllowedguard matrix andseedMockDatarefusals.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 makestimingSafeEqualthrow).src/lib/backend/csrf.test.ts(17 tests) still passes, covering the refactor of the shared helper.How to verify
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):npx prettier --checkandnpx eslinton all changed files: clean.npx tsc --noEmit: 58 errors, identical to the count on unmodifiedmasterin this environment, none in the changed or added files. The job iscontinue-on-error: truewhile the pre-existing backlog is burned down (seedocs/CI.md).npm run test:coverage: the failing-test set is a strict subset ofmaster's, with no new file failing and none of the added/changed files failing. The run-to-run total moves by one becausesrc/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 themasterbaseline 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