Skip to content

server: one API key per device — schema-level guarantee#282

Closed
erikolsson wants to merge 1 commit into
mainfrom
security/api-keys-device-unique
Closed

server: one API key per device — schema-level guarantee#282
erikolsson wants to merge 1 commit into
mainfrom
security/api-keys-device-unique

Conversation

@erikolsson

@erikolsson erikolsson commented May 4, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Add UNIQUE INDEX api_keys_device_unique on api_keys(device_id).
  • The redemption flow at /v1/auth/exchange does DELETE FROM api_keys WHERE device_id=? → INSERT api_keys (...) to enforce "one active key per device". A recent audit showed that a concurrency bug in setup-token redemption could let two requests interleave that delete/insert pair and leave two valid api_keys rows for the same device — each caller walking away with a working API key.
  • The two parallel PRs in this audit (server: hash setup tokens at rest #280 hashes setup tokens at rest, server: atomic claim on setup-token redemption #281 makes the redemption claim atomic) close the upstream race. This PR adds a schema-level guarantee so a future regression in the redemption flow can't reintroduce the corrupted state silently — the second INSERT now fails with 23505 (unique violation) instead of producing a phantom key.

Migration

  • CREATE UNIQUE INDEX api_keys_device_unique ON api_keys USING btree (device_id);
  • Not CONCURRENTLY: api_keys is ~1 row per active device, so the ACCESS EXCLUSIVE lock completes in milliseconds and CONCURRENTLY would force the index out of Drizzle's transaction.
  • No defensive cleanup of duplicate rows. If the constraint is violated at deploy time, the migration fails loudly. That is the right behavior — auto-deleting "the older row" could just as easily revoke a legitimate user's key as an attacker's, depending on which redemption arrived first. A failure here is a signal worth a human investigating, not silently masking.

Hot path impact

  • Auth middleware looks up by keyHash (separate unique index, unchanged). New index does not affect the hot path.
  • Speeds up the existing DELETE … WHERE device_id=? in both /v1/auth/exchange (auth/github.ts) and DELETE /api/me/devices/:id (user/routes.ts). Net positive.

Audit context

PR 3 of 3 from the desktop pairing security review. Independent of PRs #280 and #281 — based on main, no merge order required, but this lands cleanly only if no production rows currently violate the constraint (expected case).

Test plan

  • bun run db:generate — produced 0016_dazzling_victor_mancha.sql
  • bun run gen:db-schema — refreshed docs/generated/db-schema.md
  • bun run typecheck (apps/server) — clean
  • bun run test (apps/server) — 290 pass / 0 fail (test bootstrap runs the new migration)
  • Pre-deploy: confirm SELECT device_id, COUNT(*) FROM api_keys GROUP BY 1 HAVING COUNT(*) > 1 returns zero rows in production

🤖 Generated with Claude Code


Note

Medium Risk
Adds a new unique DB constraint on api_keys.device_id, which can cause the migration to fail if production already contains duplicates and will turn concurrent redemptions into 23505 errors instead of creating multiple valid keys.

Overview
Adds a schema-level guarantee that each device can have at most one active API key by introducing a new unique index api_keys_device_unique on api_keys(device_id).

Updates the Drizzle schema/migration metadata and regenerates the DB schema docs so the new uniqueness constraint is reflected across code and documentation.

Reviewed by Cursor Bugbot for commit cbc89b3. Bugbot is set up for automated code reviews on this repo. Configure here.

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>
@coderabbitai

coderabbitai Bot commented May 4, 2026

Copy link
Copy Markdown

Warning

Rate limit exceeded

@erikolsson has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 53 minutes and 27 seconds before requesting another review.

To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: f2e2e4ef-e830-45a5-9169-794075f0a4a8

📥 Commits

Reviewing files that changed from the base of the PR and between 719dcbb and cbc89b3.

⛔ Files ignored due to path filters (1)
  • docs/generated/db-schema.md is excluded by !**/generated/**
📒 Files selected for processing (4)
  • apps/server/drizzle/0016_dazzling_victor_mancha.sql
  • apps/server/drizzle/meta/0016_snapshot.json
  • apps/server/drizzle/meta/_journal.json
  • apps/server/src/db/schema.ts
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch security/api-keys-device-unique

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.

❤️ Share
Review rate limit: 0/1 reviews remaining, refill in 53 minutes and 27 seconds.

Comment @coderabbitai help to get the list of available commands and usage tips.

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>
@giuseppecrj

Copy link
Copy Markdown
Contributor

closing due to #289

@giuseppecrj giuseppecrj closed this May 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants