Skip to content

Add API key rotation with a bounded overlap window - #10

Merged
GunsNR merged 3 commits into
mainfrom
claude/dns-rebinding-socket-pinning-ar8ilh
Aug 30, 2026
Merged

GunsNR merged 3 commits into
mainfrom
claude/dns-rebinding-socket-pinning-ar8ilh

Conversation

@GunsNR

@GunsNR GunsNR commented Aug 28, 2026 •

Copy link
Copy Markdown
Owner

Closes the rotation half of the phase-2 criterion "API keys carry scopes, quotas and a rotation flow" — scopes and quotas already shipped; rotation did not exist.

Three commits: the rotation flow, a Cache-Control: no-store fix found in review, and the replacement of a quota-group sentinel with a real database invariant. Both follow-ups are described in the review comments below.

Why an overlap, not just a new key

Revocation on its own is a trap. It breaks the customer's integration at the exact moment they discover a leak, so in practice nobody revokes and the leaked key stays live. Rotation makes the safe action the convenient one: the replacement works immediately, the old key keeps working for 24 hours while the integration is updated, and it then stops whether or not anyone comes back.

The overlap is the part that needed care, because a second live key is a second way to be wrong.

How each risk is closed

Risk Answer
Old key lives forever without a sweeper overlapExpiresAt is evaluated where the key is presented. The predecessor dies on its own — nothing sweeps it, and there is no worker to be down when it matters
Rotation doubles the daily budget for 24h Both keys carry the same quotaGroupId; authenticateApiKey sums that group's usage for the UTC day, constrained by tenant. Two keys, one budget
A quota group could be misread quotaGroupId is required and CHECK <> '' — there is no falsy value for application code to interpret. Successors inherit the predecessor's group verbatim
One tenant reaching another's budget ApiKey carries a denormalised orgId and every group lookup constrains on it. A test forces two tenants onto a literally identical group string and proves their budgets stay apart
Rotation escalates permissions The successor inherits project, scopes, quota and expiry verbatim. Rotating an hour before expiry does not buy a further day; a key that could not publish before cannot publish after
Concurrent rotation forks the key Settled twice: a conditional claim filtered on rotatedAt: null inside a transaction (losers roll back having created nothing) and a unique index on rotatedFromId, so the storage layer refuses a second successor even if application logic were bypassed
Cross-tenant rotation Refused, and reported as not_found rather than forbidden — "exists but is not yours" is an existence oracle for another tenant's key ids
A cached secret Every response in the route carries Cache-Control: no-store. force-dynamic does not imply it — the route was observed emitting no cache header at all
Audit trail leaks key material New append-only ApiKeyEvent table holds identifiers, actor and overlap expiry. It has no column that could hold a plaintext key or a digest, and a structural test keeps it that way

What changed

  • prisma/schema.prisma + one reviewed migration — five ApiKey columns (rotatedAt, overlapExpiresAt, rotatedFromId unique, quotaGroupId, orgId), the ApiKeyEvent table, and @@index([orgId, quotaGroupId]). The migration is hand-written, not generated: two columns need a backfill, and a generated diff would have left existing rows to be interpreted by application code, which is the thing it exists to prevent.
  • src/lib/apikey.ts — rotateApiKey(), newQuotaGroupId(), ROTATION_OVERLAP_MS, overlap enforcement and tenant-scoped group quota in authenticateApiKey, new overlap_expired rejection reason.
  • src/app/api/app/api-keys/route.ts — PATCH handler behind the same apikey:manage permission as create and revoke; all responses through one json() helper that sets no-store.
  • ApiKeyManager.tsx — rotate control with a confirmation explaining the overlap, one-time display and copy of the replacement, the exact instant the old key stops working, and Revoke old key now.
  • capabilities.ts + release-truth-audit.md — corrections, below.

Registry and truth-audit corrections

capabilities.ts described public_api as having "no scopes, no per-key quota and no rotation flow" and cited only tests/crypto.test.ts — long after scopes and quotas shipped. Two-thirds of that was false. Corrected, with the drift recorded in the audit rather than quietly erased.

Both documents now also state plainly that quota admission is not atomic: the counter is read and then written, so simultaneous requests can each be admitted against the same count. The limit holds for sequential traffic and is not a hard guarantee under concurrency. This predates rotation and is not repaired here. Quotas are not described as closed or externally validated, and the audit records that Phase 2 criterion 7 is partial for that reason. The next focused task is an atomic per-group, per-day counter.

public_api stays beta. No capability became sellable.

Tests — 39 in the rotation suite, 714 total

Overlap behaviour, inheritance without escalation, shared budget, atomic single-successor under three concurrent rotations, every refusal path, secret handling, the no-store header on both success paths, the database invariant, the migration's backfill, and registry/audit synchronization. Over-blocking guards throughout.

Verification

Against PostgreSQL 16, matching CI:

  • Migration applied to a populated two-tenant database with live usage — no null or empty groups, distinct groups per key, usage and quotas unchanged, orgId matching Project on every row, empty group refused by the check constraint
  • prisma migrate deploy on a fresh database + drift check — no drift
  • npm run typecheck / npm run lint — clean
  • npm test — 714 passed / 714
  • npm run build — compiled; npm run db:rehearse — passed

Scope

Nothing deployed, Railway unmodified, no provider integration, no capability activated or made sellable, Phase 2 stays in-progress. roadmap.ts untouched.

claude added 2 commits August 28, 2026 22:48
Revocation on its own is a trap. It breaks the customer's integration at
the exact moment they discover a leak, so in practice nobody revokes and
the leaked key stays live. Rotation makes the safe action the convenient
one: the replacement works immediately, the old key keeps working for 24
hours while the integration is updated, and it then stops whether or not
anyone comes back.

The overlap is the part that needed care, because a second live key is a
second way to be wrong.

Expiry is evaluated where the key is presented, so the predecessor dies on
its own — nothing sweeps it, and there is no worker to be down when it
matters. The pair shares one daily budget through a quota group: rotating
must not hand out a second allowance for 24 hours, which would be a quota
bypass anyone could trigger at will. The successor inherits project,
scopes, quota and expiry verbatim and gains nothing, so rotating an hour
before expiry does not buy a further day, and a key that could not publish
before cannot publish after.

Concurrency is settled twice over. The predecessor is claimed with a
conditional update filtered on `rotatedAt: null` inside a transaction, so
simultaneous rotations resolve to one winner and the losers roll back
having created nothing; a unique index on `rotatedFromId` says the same
thing at the storage layer, in case application logic is ever bypassed.
Revoked, expired, already-rotated and cross-tenant keys are refused, and a
key in another tenant is reported as missing rather than forbidden,
because "exists but is not yours" is an existence oracle.

Rotations are recorded in a new append-only ApiKeyEvent table holding
identifiers, actor and overlap expiry. It has no column that could hold a
plaintext key or a digest, and a structural test keeps it that way.

The settings screen gains a rotate control with a confirmation that
explains the overlap, one-time display and copy of the replacement, the
exact instant the old key stops working, and a "Revoke old key now" action
for once the integration is updated.

capabilities.ts described public_api as having "no scopes, no per-key
quota and no rotation flow" and cited only tests/crypto.test.ts, long
after scopes and quotas shipped. Two-thirds of that was false and the
evidence pointer was incomplete; both are corrected here and in the
release truth audit. public_api stays beta: a rotation flow proven in
tests is not one a third-party integrator has used.

One reviewed migration adds four ApiKey columns and the event table.
Phase 2 stays in progress and no capability becomes sellable.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PQ2eShwv2iVM3CQYpjEkxK
The create and rotate responses each carry a plaintext key — the only copy
that will ever exist — and the route sent no cache directive at all. The
`dynamic = 'force-dynamic'` export does not imply one: a running server was
observed returning no Cache-Control header from this route while
/api/health, which sets it explicitly, returned no-store. A shared cache or
a back/forward navigation could therefore retain a secret the server
intended to hand over exactly once.

Every response in the route now goes through one helper that sets the
header, rather than the two secret-bearing returns setting it themselves,
so a handler added later cannot forget. A test asserts exactly one bare
NextResponse.json remains — the helper — which is what keeps that true.

Also adds regression tests for the backfill path, which was implemented but
unproven: keys that predate rotation carry an empty quota group, and if that
were ever read as a real group id every key in the database would share one
budget across every tenant. The tests hold two tenants' backfilled keys
apart, and check that rotating one produces a group scoped to that key
rather than to the empty string.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PQ2eShwv2iVM3CQYpjEkxK

GunsNR commented Aug 28, 2026

Copy link
Copy Markdown
Owner Author

Final review — one blocker found and fixed

Twelve criteria verified against a running server and a populated pre-migration database. One genuine blocker, corrected in 0a2900e; everything else confirmed. Returning for review rather than merging.

🔴 Blocker (fixed): the plaintext response was cacheable

The create and rotate responses each carry the only copy of a plaintext key, and the route sent no cache directive at all. dynamic = 'force-dynamic' does not imply one — observed on a running production build:

PATCH /api/app/api-keys   HTTP/1.1 401     (no cache-control header)
POST  /api/app/api-keys   HTTP/1.1 401     (no cache-control header)
GET   /api/health         HTTP/1.1 200     cache-control: no-store

/api/health and /api/app/export already set it explicitly, so the convention existed and this route was the omission — on the one route where it matters most. A shared cache or a back/forward navigation could retain a secret meant to be handed over once.

Fix: every response in the route goes through one helper that sets the header, rather than tagging the two secret-bearing returns. A test asserts exactly one bare NextResponse.json remains — the helper itself — so a handler added later cannot forget. Re-verified on a rebuilt server:

PATCH  HTTP/1.1 401   cache-control: no-store
POST   HTTP/1.1 401   cache-control: no-store
DELETE HTTP/1.1 401   cache-control: no-store

Design decisions — confirmed intentional

Replacement inherits the original expiration. Deliberate. Lifetime is a permission: rotating an hour before expiry must not buy another 30 days. cannot extend a key past the expiry its issuer chose locks it in. If you'd prefer rotation to reset the clock, that is a one-line change and one test to flip.

Cross-tenant rotation returns not_found. Deliberate. "That key exists but is not yours" is an existence oracle for another tenant's key ids. The test asserts the reason is not_found specifically, not merely that it failed.

The twelve checks

# Check Result
1 Migration on a populated pre-migration database ✅ Built a database at the two prior migrations, seeded 2 tenants / 2 projects / 4 keys with live usage counts and differing quotas, applied the migration: no error, all 4 rows intact, usageCount and dailyQuota preserved, none marked rotated. The unique index on rotatedFromId created cleanly over 4 existing NULL rows
2 Backfilled keys get their own group; no cross-tenant sharing ✅ Backfill sets quotaGroupId = '', which is a sentinel meaning "own group" — the sibling query is guarded by if (match.quotaGroupId), so an empty group never joins anything. Now tested: two tenants' backfilled keys, one exhausted, the other unaffected
3 Successors inherit the group through repeated rotations ✅ quotaGroupId || id — a three-generation chain resolves to one group id, asserted as a Set of size 1
4 Concurrent old+new cannot exceed the original quota ⚠️ Sequentially yes (2 old + 1 new against a budget of 3, fourth refused on either key). Concurrently, see the separate note below
5 Index for the auth-path group query ✅ @@index([quotaGroupId]) serves the filter. (quotaGroupId, usageDay) would be marginally tighter, but a rotation group holds at most a handful of rows, so the residual filter is free. Adequate
6 Old key valid only until the earliest of expiry / revocation / overlap ✅ All three checked independently in authenticateApiKey, in that order, before any quota write
7 New key cannot outlive the original ✅ expiresAt inherited verbatim; asserted with a rotation one hour before expiry
8 Claim + unique constraint under rollback and 3 simultaneous requests ✅ Three concurrent rotations → one ok, two already_rotated, exactly one row with that rotatedFromId. Refused rotations leave no key row and no audit row
9 Authorization is owner/admin, not API-key scope; CSRF ✅ Gated on the session's apikey:manage role permission — an API key scope cannot reach this route at all. CSRF rests on the existing session cookie: httpOnly, sameSite: 'lax', secure in production, same as create and revoke. Structural test asserts all three handlers carry both guards
10 no-store, never in URL / log / audit / storage; gone after navigation ✅ after the fix. No console.*, no localStorage/sessionStorage, no analytics call; only the key id is ever a query parameter; the plaintext lives in React state and dies on unmount
11 Revoking old does not revoke new ✅ Tested. Tenant-wide emergency revocation does not exist for API keys — only revokeAllSessions for user sessions — so the conditional half of this criterion has nothing to check
12 No activation of public_api, Phase 2 not complete ✅ status: 'beta' unchanged and asserted by test; roadmap.ts untouched; phase-2 still in-progress

Reported separately: a pre-existing quota concurrency limitation

Criterion 4 holds sequentially but not under true concurrency, and this predates the PR. On main today:

const nextCount = sameDay ? match.usageCount + 1 : 1;
if (match.dailyQuota > 0 && nextCount > match.dailyQuota) return { reason: 'quota' };
await db.apiKey.update({ ... usageCount: nextCount });

That is a read-then-write with no atomic increment and no row lock, so simultaneous requests can each read the same count and both be admitted. Rotation reuses the same shape across a group rather than introducing it — the overlap does not widen the race beyond what a single key already has. Closing it properly means an atomic UPDATE … SET usageCount = usageCount + 1 … RETURNING with the limit enforced in the statement, or a per-group counter row. That is a separate change to the quota mechanism itself and does not belong in a rotation PR.

Verification after the fix

  • Migration on a populated multi-tenant database — clean; on a fresh database, migrate deploy + drift check — no drift
  • npm run typecheck / npm run lint — clean
  • npm test — 706 passed / 706 (was 701)
  • npm run build — compiled; npm run db:rehearse — passed
  • Header confirmed on a running production build, before and after

Generated by Claude Code

The empty string meant "this key is its own group", which put the rule in
application code where a misreading would be catastrophic: if that value
were ever treated as a real group id, every key in the database — across
every tenant — would pool into one budget, and one tenant could exhaust
another's.

quotaGroupId is now required and constrained non-empty, so the falsy value
no longer exists to be misread. The migration backfills each pre-existing
key to its own id: unique because it is the primary key, stable because ids
never change, and impossible to collide with another tenant's. Standalone
keys are created with an explicit generated group, since a row id does not
exist until the row is written and a key must never be inserted without a
group. Successors inherit the predecessor's group verbatim rather than
recomputing it, so a chain of rotations keeps one allowance however long it
runs. Every `|| id` fallback and every `if (match.quotaGroupId)` guard is
gone.

ApiKey also gains a denormalised tenant column, and the group lookup
constrains on it as well as on the group. Group ids are unique by
construction, but that is an application promise; a tenant column makes
one tenant's usage arithmetically unable to reach another's whatever a
group id turns out to hold. A test forces two tenants onto a literally
identical group string and proves their budgets stay apart.
`@@index([orgId, quotaGroupId])` serves that query, replacing the
single-column index.

The migration was rewritten rather than superseded: it has never merged, so
amending it keeps one reviewed migration for one change. It was applied to a
populated two-tenant database carrying live usage counts — no nulls or empty
groups remain, all four keys received distinct groups equal to their own ids,
usage and quotas were preserved unchanged, orgId matched Project on every
row, and an attempt to write an empty group was refused by the check
constraint.

Quota admission remains non-atomic: the counter is read and then written, so
simultaneous requests can each be admitted against the same count. That
predates rotation and is not repaired here. The registry and truth audit now
say so plainly rather than describing quotas as closed, and record that
Phase 2 criterion 7 is partial for that reason.

Cache-Control: no-store is unchanged, with assertions added for the two
successful responses that actually carry a key.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PQ2eShwv2iVM3CQYpjEkxK

GunsNR commented Aug 28, 2026

Copy link
Copy Markdown
Owner Author

Sentinel replaced with a database invariant (29c6a7b)

The empty string meant "this key is its own group", which put the rule in application code where a misreading is catastrophic: if that value were ever read as a real group id, every key in the database — across every tenant — would pool into one budget. The falsy value no longer exists.

# Requirement Implementation
1 Non-null, non-empty group on every key quotaGroupId String (required) plus CHECK ("quotaGroupId" <> ''). Prisma's drift check accepts the constraint — verified
2 Existing keys each get their own unique group Migration backfills quotaGroupId = id: unique because it is the primary key, stable because ids never change, never another tenant's
3 New standalone keys start with an explicit group newQuotaGroupId() → grp_<16 random bytes>. Generated rather than derived from the row id, because the id does not exist until the row is written and a key must never be inserted without a group
4 Successors inherit the exact group const quotaGroupId = existing.quotaGroupId — no recomputation, no fallback
5 No falsy-group special case remains Both || id and if (match.quotaGroupId) are gone; a test asserts neither pattern can return
6 Every group lookup constrains by tenant ApiKey gains a denormalised orgId; the sibling query filters on it as well as the group
7 Composite authentication-path index @@index([orgId, quotaGroupId]), replacing the single-column index

Why the tenant column rather than relying on unique group ids. Group ids are unique by construction — but that is an application promise. A tenant column makes one tenant's usage arithmetically unable to reach another's whatever a group id turns out to hold. A test forces two tenants onto a literally identical group string (grp_collision, which the application would never produce) and proves their budgets stay apart.

The migration was rewritten, not superseded. It has never merged, so amending keeps one reviewed migration for one change rather than shipping a corrective migration against a state that never existed anywhere.

Requirement 8 — populated pre-migration upgrade, re-run

Two tenants, two projects, four keys with live usage counts and differing quotas, at the two prior migrations, then upgraded:

1. null-or-empty groups (expect 0):        0
2. distinct groups / keys (expect 4/4):    4/4
2b. group equals own id (expect 4):        4
4. orgId mismatches vs Project (expect 0): 0
4b. null orgId (expect 0):                 0
   composite index present:                1

 id   | orgId | quota | used | usageDay      ← usage and quotas unchanged
 k_a1 | org_a |    10 |    4 | 2026-08-28
 k_a2 | org_a |    10 |    7 | 2026-08-28
 k_b1 | org_b |     5 |    2 | 2026-08-28
 k_b2 | org_b |     0 |   99 | 2026-08-28

Writing an empty group afterwards: ERROR: violates check constraint "ApiKey_quotaGroupId_not_empty".

Cross-tenant collision and group retention through rotation are covered as runtime tests rather than SQL, since both are properties of the query rather than the schema.

Quota concurrency — recorded, not repaired

Not touched in this PR, as instructed. The registry and truth audit now say it plainly:

Quota admission is also not yet atomic: the counter is read and then written, so simultaneous requests can each be admitted against the same count. The limit holds for sequential traffic and is not a hard guarantee under concurrency.

Quotas are not described as closed or externally validated, and the audit records that Phase 2 criterion 7 is partial for this reason — scopes, quotas and a rotation flow all exist, but the quota is not enforced atomically. The next focused task is an atomic per-group, per-day counter.

Cache-Control: no-store — kept, with success-path assertions added

The previous fix stands. Two assertions added for the responses that actually carry a key, since a 401 with no-store protects nothing:

  • return json({ ok: true, key: plaintext, prefix }) — create
  • the rotate success return, asserted to contain key: result.plaintext

Verification

  • Migration on a populated two-tenant database — clean, invariants asserted above
  • migrate deploy on a fresh database + drift check — no drift
  • npm run typecheck / npm run lint — clean
  • npm test — 714 passed / 714 (was 706)
  • npm run build — compiled; npm run db:rehearse — passed

Railway untouched, public_api still beta, roadmap.ts unmodified, Phase 2 still in-progress. Returned unmerged.


Generated by Claude Code

@GunsNR
GunsNR marked this pull request as ready for review August 30, 2026 02:00
@GunsNR
GunsNR merged commit 2bf6700 into main Aug 30, 2026
2 checks passed

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

setRotated((current) => (current ? { ...current, previousKeyId: '' } : current));

P1 Badge Preserve revoke failures before reporting success

When the DELETE request fails due to a network error, expired session, or server response, revoke() catches the error and resolves normally, so this state update still clears previousKeyId and tells the user that the previous key was revoked. The old credential actually remains usable for the rest of its overlap window, which is particularly dangerous when rotation was prompted by a leak; have revoke() propagate or return failure and only update this state after confirmed success.


await revoke(rotated.previousKeyId);

P1 Badge Keep predecessor usage after early revocation

When a quota-limited predecessor has already consumed requests today, this call reaches the existing DELETE handler, which hard-deletes that row and therefore removes its usageCount from the quota-group sum in authenticateApiKey. The successor immediately regains that spent portion of the daily budget, defeating the shared-budget guarantee whenever the user follows the new “Revoke old key now” flow; early revocation should retain the row and mark revokedAt (as revokeApiKey already does), or otherwise preserve the group's accumulated usage.


The previous key keeps working until{' '}
<strong className="font-semibold text-ink">
{new Date(rotated.overlapExpiresAt).toLocaleString('en-US', {

P2 Badge Show the effective expiry instead of the overlap deadline

When the original key expires less than 24 hours after rotation, both it and the successor inherit that earlier expiresAt, and authenticateApiKey rejects them at that time before considering the overlap deadline. This notice nevertheless promises that the previous key works until overlapExpiresAt, so an operator can plan a migration around a window that does not exist and suffer an outage; return and display the earlier of the original expiry and the overlap deadline, or explicitly warn about the separate expiry.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

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