6.7 - Share SEP-10 challenge single-use claim across instances via Redis - #175
Open
Otfrugger wants to merge 1 commit into
Open
6.7 - Share SEP-10 challenge single-use claim across instances via Redis#175Otfrugger wants to merge 1 commit into
Otfrugger wants to merge 1 commit into
Conversation
…ia Redis apps/api/src/services/challenge.ts tracked redeemed SEP-10 challenges in an in-process Map. With more than one API instance and no shared store, single-use only held per process: the same signed challenge could be redeemed once on each instance inside its ~15-minute validity window, minting a session per instance from one signature. Added packages/core/src/ports/index.ts's UsedChallengeStore port (claim() atomically marks a challenge hash used, returning false if another caller already claimed it). ChallengeService now delegates to it instead of owning a Map directly: - MemoryUsedChallengeStore (challenge.ts) is the default, same behavior as before, just moved behind the port. - RedisUsedChallengeStore (redis-used-challenge-store.ts) uses SET NX PX so two instances racing the same nonce cannot both win, with a TTL matching the challenge's own validity window. container.ts selects between them via REDIS_URL, the same branch the rate limiter already uses. verify() now calls claim() once, after signature verification succeeds, rather than a separate has()-then-set() — the earlier code's own check-then-set was exactly the race this issue is about, so folding it into one atomic call was necessary, not just a port seam. Added a test modeling two ChallengeService instances sharing one store: the same signed challenge succeeds on the first and is rejected on the second.
|
@Otfrugger Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
|
@Otfrugger is attempting to deploy a commit to the determined's projects Team on Vercel. A member of the Team first needs to authorize it. |
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.
closes #160
Summary
apps/api/src/services/challenge.tstracked redeemed SEP-10 challenges in an in-processMap. With more than one API instance and no shared store, "single-use" only held per process: the same signed challenge could be redeemed once on each instance inside its ~15-minute validity window, minting a session per instance from one signature.1. New port
packages/core/src/ports/index.tsgainsUsedChallengeStore:Atomically marks a challenge hash as used; returns
falseif another caller already claimed it. The port's own doc comment explains the exploit it closes.2. Two implementations
MemoryUsedChallengeStore(still inchallenge.ts) — the default. Same behavior as before the change (check + set inside one synchronous call, noawaitin between, so no interleaving within one process), just moved behind the port instead of being private state onChallengeService.RedisUsedChallengeStore(new:apps/api/src/services/redis-used-challenge-store.ts) —SET key value NX PX <ttl>, so two instances racing the same nonce cannot both win. TTL mirrors the challenge's own validity window (expiresAt), so a claim never outlives the challenge it guards.3. Selection
container.tsbranches onenv.redisUrlwhen constructingChallengeService, the sameREDIS_URLcheck the rate limiter already uses inindex.ts.4. The atomicity fix itself
ChallengeService.verify()previously didif (this.used.has(hash)) throw; ...; this.used.set(hash, ...)— a check-then-set, which is exactly the race this issue is about once the store is shared: two instances could both pass thehas()check, both independently verify the (same, valid) signature, and both thenset(). The fix folds this into a singleclaim()call made once, after signature verification succeeds, so there's no window between checking and claiming. The old earlyhas()pre-check (before doing the Horizon signer fetch) is gone — it was a non-atomic fast-path optimization that would have been misleading once real logic lived in a shared store; correctness now depends only on the one atomicclaim()call.Test
Added to
apps/api/test/challenge.test.ts: twoChallengeServiceinstances (same server keypair, differentChallengeServiceobjects) constructed with one sharedMemoryUsedChallengeStore— models a two-instance deployment. The same signed challenge succeeds on the firstverify()and is rejected with/already been used/on the second.I used
MemoryUsedChallengeStorerather than a live Redis connection for this, since no Redis server is available in this environment (and I found no existing test in this repo that spins one up either — the rate limiter's ownRedisStorehas no test coverage today).MemoryUsedChallengeStore.claim()implements the exact sameUsedChallengeStorecontractRedisUsedChallengeStoredoes, so the test exercises the real race-closing logic inChallengeService, just not the Redis wire protocol itself. If you'd like, I can also add aRedisUsedChallengeStore-specific test gated behind a live Redis (e.g. viaredis-memory-serveror a CI service container) as a follow-up.Also updated
docs/MAINNET.md— the existingREDIS_URLcaveat now also mentions SEP-10 challenge sharing.Verification
challenge.ts,container.ts,rate-limit.ts/redis-store.ts(for the existingREDIS_URLpattern to mirror) and theredispackage'sSET ... NX PXAPI before writing this.pnpm typecheckorpnpm test. Please run those before merging, along with the newchallenge.test.tscase specifically.Done-when checklist
REDIS_URLset, a challenge redeemed on one instance is rejected on another — covered by the port contract + the new test againstMemoryUsedChallengeStore; not yet verified against a live Redis (see caveat above).REDIS_URLunset, behavior is exactly as today —MemoryUsedChallengeStoreis a straight extraction of the previousMaplogic with the same single-process, no-interleaving guarantee.