refactor(security): Stack setup-token security fixes#289
Conversation
WalkthroughThis change introduces two security and consistency improvements to the API key and setup token management flow. First, a unique index is added to the Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 3d428d5. Configure here.
|
fixed a similar thing here: #254, we can go with yours though, heres a summary: Short Version PR #289 is a broader stacked PR against g/consolidate-server-boundaries. It includes the same core setup-token fixes, but adds a database-level guarantee that each device can only have one API key. PR #254 Solves the problem by: Generating setup tokens as opaque random base64url bearer tokens instead of UUIDs. PR #289 Solves the problem by: Storing only hashed setup tokens at rest. Key Difference If the target also includes “a device must never end up with multiple active API keys,” #289 is stronger because it enforces that in Postgres with api_keys_device_unique. The best combined shape is probably #289’s schema invariant plus #254’s transactional exchange structure, with a clean conflict-handling path for the unique-index case. |
Setup tokens were stored plaintext and looked up by raw equality, so a read-only DB leak inside the 10-minute expiry window would expose every pending pairing token. Switch to the same SHA-256-at-rest pattern that apiKeys.keyHash, refresh sessions, and MCP OAuth tokens already use: hash on issuance, hash incoming token on /v1/auth/exchange, compare hashes. Raw token is returned to the caller exactly once and never re-read. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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>
Add unique index on api_keys.device_id. Without this, a regression in the /v1/auth/exchange redemption flow (which does DELETE + INSERT on api_keys keyed by device_id) could leave two rows for the same device — the exact corrupted state a recent audit identified as reachable via concurrent setup-token redemptions. With the index in place, the second INSERT violates the constraint and fails loudly instead of silently producing two long-lived API keys. No defensive cleanup in the migration: if rows violating the constraint exist, that is itself a signal worth investigating, not silently fixing. Auto-deleting "older" rows could just as easily revoke the legitimate user's key as the attacker's, depending on which redemption arrived first. api_keys is small (~one row per active device), so a non-CONCURRENT CREATE UNIQUE INDEX completes in milliseconds. Refresh docs/generated/db-schema.md to reflect the new index. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
giuseppecrj
force-pushed
the
codex/setup-token-security-stack
branch
from
9a522b9 to
12088b7
Compare
giuseppecrj
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/server/src/auth/github.ts (1)
266-315:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftMake the exchange fully transactional around device/key replacement.
The setup-token claim is atomic now, but the later device upsert plus
api_keysdelete/insert still spans multiple statements. With the newapi_keys_device_uniqueindex, two different valid setup tokens exchanged concurrently for the same{userId, deviceName}can either 500 on insert or revoke the first request's freshly issued key after it was already returned. This needs one DB transaction so key replacement is deterministic and the token claim rolls back if issuance fails.One safe shape
- const tokenHash = await hashToken(body.token); - const [st] = await db - .update(setupTokens) - .set({ redeemed: true }) - .where( - and( - eq(setupTokens.token, tokenHash), - eq(setupTokens.redeemed, false), - gt(setupTokens.expiresAt, new Date()), - ), - ) - .returning(); - - if (!st) { - set.status = 400; - return { error: "Invalid or expired setup token" }; - } - - const [device] = await db - .insert(devices) - .values({ - userId: st.userId, - deviceName: body.deviceName, - os: body.os ?? null, - lastSeenAt: new Date(), - }) - .onConflictDoUpdate({ - target: [devices.userId, devices.deviceName], - set: { os: body.os ?? null, lastSeenAt: new Date() }, - }) - .returning(); - - await db.delete(apiKeys).where(eq(apiKeys.deviceId, device.id)); - - const rawKey = generateApiKey(); - const keyHash = await hashToken(rawKey); - await db.insert(apiKeys).values({ - userId: st.userId, - deviceId: device.id, - keyHash, - }); - - return { apiKey: rawKey, deviceId: device.id }; + const tokenHash = await hashToken(body.token); + const exchanged = await db.transaction(async (tx) => { + const [st] = await tx + .update(setupTokens) + .set({ redeemed: true }) + .where( + and( + eq(setupTokens.token, tokenHash), + eq(setupTokens.redeemed, false), + gt(setupTokens.expiresAt, new Date()), + ), + ) + .returning(); + + if (!st) return null; + + const [device] = await tx + .insert(devices) + .values({ + userId: st.userId, + deviceName: body.deviceName, + os: body.os ?? null, + lastSeenAt: new Date(), + }) + .onConflictDoUpdate({ + target: [devices.userId, devices.deviceName], + set: { os: body.os ?? null, lastSeenAt: new Date() }, + }) + .returning(); + + await tx.delete(apiKeys).where(eq(apiKeys.deviceId, device.id)); + + const apiKey = generateApiKey(); + await tx.insert(apiKeys).values({ + userId: st.userId, + deviceId: device.id, + keyHash: await hashToken(apiKey), + }); + + return { apiKey, deviceId: device.id }; + }); + + if (!exchanged) { + set.status = 400; + return { error: "Invalid or expired setup token" }; + } + + return exchanged;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/src/auth/github.ts` around lines 266 - 315, Wrap the setup-token claim, device upsert, api_keys delete and api_keys insert in a single DB transaction so those steps are atomic and will rollback if any part fails: move the token update (hashToken + db.update(setupTokens).set(...).where(...).returning()), the device upsert (db.insert(devices)...onConflictDoUpdate().returning()), the apiKeys delete (db.delete(apiKeys).where(...)) and the apiKeys insert into a single db.transaction(...) callback (using the same transaction client for all calls), return errors consistently (e.g., return { error: "..."} and set.status) when the transaction fails, and ensure you use the transaction client (not the outer db) for hashToken-related DB calls so the token claim is reverted if key issuance fails; target symbols: hashToken, setupTokens, devices, apiKeys, generateApiKey, and db.transaction.
🧹 Nitpick comments (1)
apps/server/test/integration.test.ts (1)
149-155: ⚡ Quick winAssert plaintext absence explicitly.
This proves the hash row exists, but it would still pass if the endpoint accidentally wrote both the hash and the raw token in separate rows. Add a second lookup for the plaintext token and assert that no row is returned.
Suggested test tightening
- const [storedSetupToken] = await db + const hashedSetupToken = await hashToken(setupToken); + const [storedSetupToken] = await db .select({ token: setupTokens.token }) .from(setupTokens) - .where(eq(setupTokens.token, await hashToken(setupToken))) + .where(eq(setupTokens.token, hashedSetupToken)) .limit(1); - expect(storedSetupToken?.token).toBe(await hashToken(setupToken)); + const [rawSetupToken] = await db + .select({ token: setupTokens.token }) + .from(setupTokens) + .where(eq(setupTokens.token, setupToken)) + .limit(1); + expect(storedSetupToken?.token).toBe(hashedSetupToken); expect(storedSetupToken?.token).not.toBe(setupToken); + expect(rawSetupToken).toBeUndefined();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/server/test/integration.test.ts` around lines 149 - 155, The test currently verifies a hashed setup token row exists (via hashToken and storedSetupToken) but doesn't assert the plaintext token isn't stored; add a second query against the same table (setupTokens) using eq(setupTokens.token, setupToken) to look up the raw token value and assert that the result is null/undefined (no row returned). Keep the original hashed-token assertions (hashToken, storedSetupToken) and add this explicit negative lookup to ensure the plaintext token was not written.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/server/drizzle/0016_dazzling_victor_mancha.sql`:
- Line 1: The migration will fail on DBs with duplicate api_keys.device_id
values; add a new corrective migration that first deduplicates api_keys by
keeping a single deterministic row per device (e.g., keep the row with the
smallest id or the newest created_at) and deleting the other rows, then create
the UNIQUE index "api_keys_device_unique" on api_keys(device_id); reference the
api_keys table, device_id column, and the "api_keys_device_unique" index name in
the SQL so the script explicitly removes duplicates (use a DELETE ... WHERE id
NOT IN (SELECT MIN(id) FROM api_keys GROUP BY device_id) or equivalent
deterministic criterion) and then creates the unique index in the same
migration.
---
Outside diff comments:
In `@apps/server/src/auth/github.ts`:
- Around line 266-315: Wrap the setup-token claim, device upsert, api_keys
delete and api_keys insert in a single DB transaction so those steps are atomic
and will rollback if any part fails: move the token update (hashToken +
db.update(setupTokens).set(...).where(...).returning()), the device upsert
(db.insert(devices)...onConflictDoUpdate().returning()), the apiKeys delete
(db.delete(apiKeys).where(...)) and the apiKeys insert into a single
db.transaction(...) callback (using the same transaction client for all calls),
return errors consistently (e.g., return { error: "..."} and set.status) when
the transaction fails, and ensure you use the transaction client (not the outer
db) for hashToken-related DB calls so the token claim is reverted if key
issuance fails; target symbols: hashToken, setupTokens, devices, apiKeys,
generateApiKey, and db.transaction.
---
Nitpick comments:
In `@apps/server/test/integration.test.ts`:
- Around line 149-155: The test currently verifies a hashed setup token row
exists (via hashToken and storedSetupToken) but doesn't assert the plaintext
token isn't stored; add a second query against the same table (setupTokens)
using eq(setupTokens.token, setupToken) to look up the raw token value and
assert that the result is null/undefined (no row returned). Keep the original
hashed-token assertions (hashToken, storedSetupToken) and add this explicit
negative lookup to ensure the plaintext token was not written.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 032b866f-625e-44e1-9be7-d202b86645b0
⛔ Files ignored due to path filters (1)
docs/generated/db-schema.mdis excluded by!**/generated/**
📒 Files selected for processing (9)
apps/server/drizzle/0016_dazzling_victor_mancha.sqlapps/server/drizzle/meta/0016_snapshot.jsonapps/server/drizzle/meta/_journal.jsonapps/server/src/auth/github.tsapps/server/src/db/schema.tsapps/server/src/user/routes.tsapps/server/test/auth-resolvers.test.tsapps/server/test/integration.test.tsapps/server/test/upload.test.ts
3 participants
Summary
g/consolidate-server-boundariesso those separate PRs can be closed after review./v1/auth/exchangeso concurrent redemption cannot mint multiple API keys.api_keys(device_id)as the schema-level one-active-key-per-device guarantee.Validation
bun run gen:db-schema:checkbun run typecheckbun test test/auth-resolvers.test.tsbun run test— 296 pass, 0 failNotes
This PR targets
g/consolidate-server-boundariesintentionally. Once it lands into the consolidation branch, the older security PRs #280, #281, and #282 should be redundant.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/exchangeto 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.Reviewed by Cursor Bugbot for commit 6cede0e. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Bug Fixes
Security