server: atomic claim on setup-token redemption#281
Closed
erikolsson wants to merge 1 commit into
Closed
Conversation
The previous flow read setupTokens with redeemed=false, then issued an UPDATE without re-asserting the predicate. Two concurrent /v1/auth/exchange calls with the same token both passed the check, both proceeded into the device upsert + apiKey delete/insert path, and could leave two valid api_keys rows for the same device — each caller walking away with a working long-lived API key. Replace with a single UPDATE … SET redeemed=true WHERE token=? AND redeemed=false AND expires_at>now() RETURNING *. Postgres serializes the row lock; one caller's UPDATE matches and returns the row, the loser's matches zero rows and gets a 400. Expiry check moves into the WHERE so an expired token also returns zero rows atomically. Same pattern already used in oauth/mcp.ts (auth-code consumption, refresh rotation) and auth/sessions.ts (refresh rotation via DELETE…RETURNING). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
5 tasks
giuseppecrj
added a commit
that referenced
this pull request
May 5, 2026
## Summary - Combines the security fixes from #280, #281, and #282 on top of `g/consolidate-server-boundaries` so those separate PRs can be closed after review. - Hashes setup tokens at rest while still returning the raw token once for pairing. - Claims setup tokens atomically during `/v1/auth/exchange` so concurrent redemption cannot mint multiple API keys. - Adds a unique index on `api_keys(device_id)` as the schema-level one-active-key-per-device guarantee. - Updates tests for the new invariant and adds an assertion that raw setup tokens are not stored in the database. ## Validation - `bun run gen:db-schema:check` - `bun run typecheck` - `bun test test/auth-resolvers.test.ts` - `bun run test` — 296 pass, 0 fail ## Notes This PR targets `g/consolidate-server-boundaries` intentionally. Once it lands into the consolidation branch, the older security PRs #280, #281, and #282 should be redundant. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Medium Risk** > Changes security-sensitive auth flows and adds a data-cleanup migration with a new uniqueness constraint on `api_keys`, which could affect existing tokens/devices if assumptions are wrong. > > **Overview** > **Hardens the setup-token pairing flow** by storing only a SHA-256 hash of setup tokens and updating `/v1/auth/exchange` to *atomically* claim (redeem) tokens while also checking expiry. > > **Enforces one active API key per device** by adding a unique index on `api_keys.device_id` (with a migration that deletes older duplicate keys per device) and updating tests/docs to match the new hashing and uniqueness invariants. > > <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 6cede0e. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup> <!-- /CURSOR_SUMMARY --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Fixed race condition in setup token redemption that could create duplicate API keys * Enforced one-API-key-per-device constraint * **Security** * Setup tokens are now securely hashed before storage <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Erik Olsson <valross2@gmail.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
closing due to #289 |
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.
Summary
/v1/auth/exchangeread asetupTokensrow withredeemed=false, then UPDATEd toredeemed=truekeyed only byid. There was no re-assertion of the predicate, no transaction, and no row lock — two concurrent calls with the same token both passed the check, both proceeded, and the downstreamdelete apiKeys WHERE deviceId=?+insert apiKeyspath could leave two valid keys for the same device. Each caller walked away with a working long-lived API key.UPDATE … SET redeemed=true WHERE token=? AND redeemed=false AND expires_at > now() RETURNING *. Postgres serializes the row lock; one caller's UPDATE matches and returns the row, the loser matches zero rows and gets a 400. Expiry moves into the WHERE so an expired token also fails atomically (the prior code had a tiny window where it could redeem-then-fail-on-expiry).Why this shape
The codebase already uses the same atomic-claim pattern in:
apps/server/src/oauth/mcp.ts— auth-code consumption + refresh-token rotationapps/server/src/auth/sessions.ts— refresh-token rotation viaDELETE … RETURNINGThis brings setup-token redemption into line with that prior art. No shared helper extracted because each callsite uses different tables/predicates and abstracting prematurely would obscure the per-table invariants.
Stacked on #280
Branched on top of
security/hash-setup-tokens(#280) so reviewers can read the two changes independently. GitHub will auto-retarget tomainwhen #280 merges.Audit context
PR 2 of 3 from the desktop pairing security review. PR 3 will add a unique index on
api_keys.device_idso the corrupted state above is unrepresentable at the schema level even if a future regression reintroduces the race.Test plan
bun run typecheck(apps/server) — cleanbun run test(apps/server) — 290 pass / 0 fail🤖 Generated with Claude Code
Note
Medium Risk
Touches authentication/token-issuance flow and changes database write semantics; mistakes could block legitimate setup-token redemption or allow unintended replays if predicates are wrong.
Overview
Fixes a race in
POST /v1/auth/exchangeby replacing theSELECT-then-UPDATEsetup-token redemption with a single atomicUPDATE … WHERE token AND redeemed=false AND expiresAt>now() RETURNING.This makes setup-token redemption one-time under concurrency and folds expiry validation into the claim step, returning
400when no row is claimed.Reviewed by Cursor Bugbot for commit d7bd262. Bugbot is set up for automated code reviews on this repo. Configure here.