fix(share-backend): auth-boundary hardening — open-redirect, JWKS rotation, nonce race#368
Merged
Merged
Conversation
…ation, nonce race
Three related fixes along the auth/identity boundary, all surfaced in
the post-launch audit pass on the share-publish stack.
safeNext (open redirect):
/%2Fevil.com decodes to //evil.com but the raw startsWith('//')
guard didn't catch it — modern browsers follow this as a
protocol-relative redirect. Decode once and re-run the same shape
guards on the decoded value. Malformed percent-encoding fails closed
to "/".
JWKS rotation auto-recovery:
cache was a module-level singleton with a 1h TTL. After Google
rotated a JWKS key, every desktop sign-in carrying the freshly-
rotated key id would fail "no matching jwk" for up to 60 minutes —
the cache held the stale key set and never invalidated. verifyIdToken
now force-refreshes the JWKS once on "no matching jwk" and retries.
Other failures (bad signature, bad iss, expired) propagate
immediately — refreshing keys can't help those.
Nonce reservation order on /api/auth/sign-in-with-id-token:
KV write was AFTER verifyNativeIdToken. Two concurrent requests with
the same nonce could both pass the pre-check, both verify, both
mint sessions. Reserve in KV BEFORE verify; on verify failure
release the reservation via waitUntil so a legitimate retry with a
corrected payload isn't locked out for the full 10-minute TTL. KV
doesn't offer CAS so the race isn't fully closed, but the window
shrinks from "verify duration + KV propagation" to "just KV
propagation".
Tests:
- safeNext: percent-encoded //, \, .. ; malformed %; safe encoded
paths round-trip intact
- JWKS: rotation succeeds after one force-refresh; bad signature
does NOT trigger refresh
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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.
What
Three related fixes along the auth/identity boundary, surfaced in the post-launch audit pass on the share-publish stack.
safeNext— open-redirect via percent-encoded bypass/%2Fevil.comdecodes to//evil.com. The rawstartsWith('//')guard didn't catch it. Modern browsers follow this as a protocol-relative redirect.Fix: decode once and re-run the same shape guards on the decoded value. Malformed percent-encoding fails closed to
/. Same-origin paths that happen to contain encoded segments (/me%2Fpublished) round-trip intact.JWKS rotation auto-recovery
cacheis a module-level singleton with a 1h TTL. After Google rotated a JWKS key, every desktop sign-in carrying the freshly-rotatedkidwould failno matching jwkfor up to 60 minutes — the cache held the stale key set and never invalidated.sequenceDiagram autonumber participant D as Desktop client participant B as share-backend participant K as JWKS cache (1h TTL) participant G as Google JWKS Note over G: keys rotated at T0<br/>(new kid: kid-new) D->>B: id_token signed by kid-new B->>K: cached keys K-->>B: [kid-old] (stale) B->>B: verify → no matching jwk B->>K: force-refresh K->>G: GET /oauth2/v3/certs G-->>K: [kid-new] K-->>B: [kid-new] B->>B: verify OK B-->>D: session_tokenverifyIdTokennow force-refreshes once onno matching jwkand retries. Other failures (bad signature, bad iss, expired) propagate immediately — refreshing keys can't help those, and a refresh on every bad-signature attempt would be a DoS amplifier.Nonce reservation order on
/api/auth/sign-in-with-id-tokenKV write was AFTER
verifyNativeIdToken. Two concurrent requests with the same nonce could both pass the pre-check, both verify, both mint sessions.sequenceDiagram autonumber participant A as Request A participant B as Request B participant KV as NONCE KV participant V as verifyNativeIdToken Note over A,B: same nonce N arrives ~simultaneously par before fix A->>KV: GET nonce/N → null B->>KV: GET nonce/N → null A->>V: verify (~50ms) B->>V: verify (~50ms) V-->>A: claims V-->>B: claims A->>KV: PUT nonce/N=1 B->>KV: PUT nonce/N=1 Note over A,B: both mint sessions ✗ end par after fix A->>KV: GET nonce/N → null A->>KV: PUT nonce/N=1 B->>KV: GET nonce/N → 1 B-->>B: 403 nonce replay ✓ A->>V: verify V-->>A: claims (or release on failure) endReserve in KV BEFORE verify; on verify failure release the reservation via
waitUntilso a legitimate retry with a corrected payload isn't locked out for the full 10-minute TTL. KV doesn't offer CAS so the race isn't fully closed, but the window shrinks fromverify duration + KV propagationtojust KV propagation.Verification
safeNext:%2F,%2E%2E,%5C, malformed%, safe encoded pathpnpm --filter @spool/share-backend test→ 180/180 green.Risk
None functional.
safeNextis forward-only stricter (one edge case — malformed/%— now returns/instead of the malformed raw value, which is the safer choice). JWKS rotation only triggers on previously-fatalno matching jwkerrors. Nonce reorder narrows a race that previously allowed duplicate sessions.Submitted by @graydawnc.