fix: harden setup token exchange#254
Conversation
WalkthroughThis pull request refactors setup token handling across the authentication and user management layers. The changes introduce opaque token generation for setup tokens, implement token hashing at storage time, and make token redemption atomic via database transactions. The Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/server/test/upload.test.ts (1)
96-99:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win**Test is broken:
generateApiKeyno longer returns UUID format.**The pipeline failure shows thatgenerateApiKey()now returns a base64url-encoded string (e.g.,"t7y0E1goaejjudnpdAEjv4O7_QvbTfZG6VyhQqjNnVs") instead of a UUID. 32 bytes produces ~43 characters in base64url encoding.The test assertion at line 98 expects UUID format but the implementation has changed. This test must be updated to match the new token format.
🐛 Proposed fix: Update test to validate base64url format
describe("token utilities", () => { - it("generateApiKey returns UUID format", () => { + it("generateApiKey returns base64url format", () => { const key = generateApiKey(); - expect(key).toMatch(/^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i); + // 32 bytes base64url-encoded = 43 characters (A-Za-z0-9_-) + expect(key).toMatch(/^[A-Za-z0-9_-]{43}$/); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/server/test/upload.test.ts` around lines 96 - 99, The test still asserts UUID format but generateApiKey() now returns a base64url-encoded token; update the test for generateApiKey to assert the new format by checking the value from generateApiKey() matches a base64url character set (e.g., /^[A-Za-z0-9\-_]+$/) and the expected length for a 32-byte token encoded in base64url (~43 characters), replacing the UUID regex assertion in the it("generateApiKey returns UUID format", ...) block.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/server/test/upload.test.ts`:
- Around line 96-99: The test still asserts UUID format but generateApiKey() now
returns a base64url-encoded token; update the test for generateApiKey to assert
the new format by checking the value from generateApiKey() matches a base64url
character set (e.g., /^[A-Za-z0-9\-_]+$/) and the expected length for a 32-byte
token encoded in base64url (~43 characters), replacing the UUID regex assertion
in the it("generateApiKey returns UUID format", ...) block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c904c601-f545-4589-a69a-f10cb3bfdb22
📒 Files selected for processing (4)
apps/server/src/auth/github.tsapps/server/src/auth/tokens.tsapps/server/src/user/routes.tsapps/server/test/upload.test.ts
![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 b519baa. Configure here.
| /** Generate a random API key. */ | ||
| export function generateApiKey(): string { | ||
| return crypto.randomUUID(); | ||
| return generateOpaqueToken(); |
There was a problem hiding this comment.
Changed generateApiKey output breaks existing UUID format test
Medium Severity
generateApiKey now returns a base64url-encoded random string via generateOpaqueToken(), but an existing test at line 96–99 of upload.test.ts asserts it matches a UUID v4 regex. That test will always fail because base64url output doesn't conform to the xxxxxxxx-xxxx-4xxx-… pattern. The test wasn't updated alongside the implementation change.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit b519baa. Configure here.
texuf
force-pushed
the
codex/audit-setup-tokens
branch
from
b519baa to
dec59fd
Compare
|
looks like this is handled in #289 |
1 participant
Summary:
Validation:
Note
Medium Risk
Touches credential issuance paths (setup-token generation, redemption, and API key format), so mistakes could block device sign-in or weaken token security. Changes are contained to token handling and covered by new tests for hashing and concurrent redemption.
Overview
Hardens the setup-token flow by storing setup tokens as SHA-256 hashes (returning only the raw token to the caller) and by redeeming tokens transactionally during
/v1/auth/exchangeto prevent race/replay (conditionalUPDATE ... WHERE redeemed=false AND expiresAt>now).API key generation is switched from UUIDs to opaque base64url bearer tokens via
generateOpaqueToken, and the exchange path now consistently hashes the presented setup token before lookup; tests add coverage for hashed-at-rest storage and for allowing only one concurrent exchange to succeed.Reviewed by Cursor Bugbot for commit dec59fd. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Security
Bug Fixes