feat(auth): user-facing API key management (create / list / revoke)#3125
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@koala73 — this builds on #3115 (login gate removal). Together they complete the self-serve flow: sign in → create API keys → use the API. Key details:
Ready for review when you are. Metered billing (#3117) can follow separately. |
Greptile SummaryThis PR adds full-stack API key management (create / list / revoke) with a
Confidence Score: 3/5Not safe to merge until the revocation cache-invalidation gap is addressed — revoked keys remain active for up to 5 minutes. One P1 finding breaks the primary security contract of key revocation: a revoked key continues granting premium API access for up to 5 minutes due to the missing Redis cache invalidation. The test plan explicitly tests for immediate revocation (returns 403), which will fail. server/_shared/user-api-key.ts (missing cache invalidation on revocation) and convex/apiKeys.ts (keyPrefix validation and touchKeyLastUsed guard) Important Files Changed
Sequence DiagramsequenceDiagram
actor User
participant Browser
participant ConvexDB
participant Gateway
participant Redis
User->>Browser: Click Create Key
Browser->>Browser: generateKey() wm_xxx
Browser->>Browser: sha256(wm_xxx) digest
Browser->>ConvexDB: mutation createApiKey
ConvexDB-->>Browser: id, name, keyPrefix
Browser-->>User: Show plaintext key once
Note over User,Gateway: Using the key in an API request
User->>Gateway: GET /api/... with wm_xxx
Gateway->>Redis: GET user-api-key:digest
alt Cache hit (TTL 5 min)
Redis-->>Gateway: userId, keyId, name
Gateway-->>User: 200 OK premium granted
else Cache miss
Redis-->>Gateway: null
Gateway->>ConvexDB: POST internal-validate-api-key
ConvexDB-->>Gateway: userId, id, name or null
Gateway->>Redis: SET user-api-key:digest TTL 300s
Gateway-->>User: 200 OK or 403
end
Note over User,Redis: Revocation — Redis entry NOT invalidated
User->>Browser: Click Revoke
Browser->>ConvexDB: mutation revokeApiKey
ConvexDB-->>Browser: ok true
Note over Gateway,Redis: Stale cache entry lives up to 5 more minutes
User->>Gateway: GET /api/... with revoked wm_xxx
Gateway->>Redis: GET user-api-key:digest
Redis-->>Gateway: stale hit
Gateway-->>User: 200 OK — should be 403
Reviews (1): Last reviewed commit: "feat(auth): user-facing API key manageme..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (result) { | ||
| await setCachedJson(cacheKey, result, CACHE_TTL_SECONDS, true); |
There was a problem hiding this comment.
revokeApiKey patches revokedAt in Convex, but the positive Redis cache entry written here (5-min TTL) is never deleted. A gateway request arriving within that window will hit the cached valid result and grant premium access even after the user clicked "Revoke". The test plan expects a revoked key to return 403 immediately, which will not hold until the cache naturally expires.
A proper fix threads the key's hash digest back through the revocation response so the caller can call deleteCachedJson before acknowledging the revocation to the user. The document's stored hash is already available inside the revokeApiKey mutation handler and can be returned alongside { ok: true }.
| if (args.keyPrefix.length < 4 || args.keyPrefix.length > 12) { | ||
| throw new ConvexError("INVALID_PREFIX"); |
There was a problem hiding this comment.
keyPrefix accepts any 4–12 char string
The validation only checks length; it doesn't enforce that the prefix follows the wm_ format or contains only valid hex characters after the wm_ marker. A caller directly invoking the Convex mutation can store an arbitrary display prefix like notakey! while supplying a legitimate hash — misleading users in the key list UI.
| if (args.keyPrefix.length < 4 || args.keyPrefix.length > 12) { | |
| throw new ConvexError("INVALID_PREFIX"); | |
| if (!/^wm_[a-f0-9]{5,9}$/.test(args.keyPrefix)) { | |
| throw new ConvexError("INVALID_PREFIX"); | |
| } |
| export const touchKeyLastUsed = internalMutation({ | ||
| args: { keyId: v.id("userApiKeys") }, | ||
| handler: async (ctx, args) => { | ||
| const key = await ctx.db.get(args.keyId); | ||
| if (key) { | ||
| await ctx.db.patch(args.keyId, { lastUsedAt: Date.now() }); | ||
| } | ||
| }, |
There was a problem hiding this comment.
touchKeyLastUsed updates revoked keys
touchKeyLastUsed only checks if (key) before patching — it doesn't skip revoked documents. Because the gateway fires it fire-and-forget after a positive cache hit (which can occur for revoked keys during the 5-minute stale window), this will write a fresh lastUsedAt timestamp onto a revoked key, muddying the audit trail.
| export const touchKeyLastUsed = internalMutation({ | |
| args: { keyId: v.id("userApiKeys") }, | |
| handler: async (ctx, args) => { | |
| const key = await ctx.db.get(args.keyId); | |
| if (key) { | |
| await ctx.db.patch(args.keyId, { lastUsedAt: Date.now() }); | |
| } | |
| }, | |
| const key = await ctx.db.get(args.keyId); | |
| if (key && !key.revokedAt) { | |
| await ctx.db.patch(args.keyId, { lastUsedAt: Date.now() }); | |
| } |
Review — PR #3125Why this PR? Ships user-facing API keys (create/list/revoke) so customers can programmatically hit premium endpoints using their own Core scaffolding is good: SHA-256-at-rest, one-time plaintext display, per-user cap, ownership guard on revoke, 160-bit entropy, 🛑 P1 — Blocking1. Privilege escalation: any signed-in user can self-mint premium access on 2. New key type is not wired into the gateway auth path — the test plan is broken. 3. Cross-environment cache poisoning via 4. Revocation is stale up to 5 minutes. 5. No tests. 6. Cache stampede — should use
|
|
@koala73 Addressed all three review items:
Ready for another look when you get a chance! |
Consolidated Review - PR #3125Why this PR?Lets signed-in users mint personal API keys (client-side generation, SHA-256 hashed, stored in Convex, validated at the edge via Redis-cached Convex lookup) and use them in P0 - Blocking1. Entitlement bypass: any signed-in user gets PRO via a self-issued key Files:
Net effect:
Fix: gate issuance on P1 - Blocking2. User keys are not wired into the main domain gateway Files:
The PR test plan says "use a created key against a premium endpoint: should authenticate" which is only true for the handful of standalone endpoints that go through Fix: call 3. Revocation is not atomic (up to 5 min window) Files: Positive auth results are cached for 300s. Cache purge is a fire-and-forget browser Fix options, any one of:
P2 - High4. Dead HTTP
5. Cache-invalidate endpoint is cross-tenant
6. No tests on 939 lines of auth-sensitive code Need unit coverage for: SHA-256 matching, cache HIT/MISS/INVALID sentinel, Convex timeout fallback, revoked-key rejection, per-user limit, duplicate-hash guard, ownership on the invalidate endpoint. Plus one e2e: create, use against a gatewayed endpoint, revoke, confirm next call is rejected within the TTL window. 7. Wrong HTTP status code and raw error leakage
8. Env var surface undocumented
P3 - Medium / Low
Good
RecommendationRequest changes. P0 and P1 must ship in this PR. P2 items (especially tests and the dead-code cleanup) should land alongside them. |
Addressed consolidated review (P0–P3) —
|
Re-review — PR #3125 (post-fixes through
|
Correction — missed a P1I cleared this too fast. Verified in The PR gates on 🛑 Added P1 — gate on
|
Follow-up review after Koala's correctionI agree with Koala's latest P1: this PR should gate API-key issuance/usage on Why this is still blocking:
There are also two additional issues still worth fixing in the same pass:
Both still call
Previously, a transient Convex/network failure during user-key validation degraded to Test note
Suggested fix order
After that, the remaining residuals look non-blocking to me. |
Adds full-stack API key management so authenticated users can create, list, and revoke their own API keys from the Settings UI. Backend: - Convex `userApiKeys` table with SHA-256 key hash storage - Mutations: createApiKey, listApiKeys, revokeApiKey - Internal query validateKeyByHash + touchKeyLastUsed for gateway - HTTP endpoints: /api/api-keys (CRUD) + /api/internal-validate-api-key - Gateway middleware validates user-owned keys via Convex + Redis cache Frontend: - New "API Keys" tab in UnifiedSettings (visible when signed in) - Create form with copy-on-creation banner (key shown once) - List with prefix display, timestamps, and revoke action - Client-side key generation + hashing (plaintext never sent to DB) Closes #3116 Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
…ion, revoked-key guard - Invalidate Redis cache on key revocation so gateway rejects revoked keys immediately instead of waiting for 5-min TTL expiry (P1) - Enforce `wm_` prefix format with regex instead of loose length check (P2) - Skip `touchKeyLastUsed` for revoked keys to preserve clean audit trail (P2) Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
P0: gate createApiKey on pro entitlement (tier >= 1); isCallerPremium now verifies key-owner tier instead of treating existence as premium. P1: wire wm_ user keys into the domain gateway auth path with async Convex-backed validation; user keys go through entitlement checks (only admin keys bypass). Lower cache TTL 300s → 60s and await revocation cache-bust instead of fire-and-forget. P2: remove dead HTTP create/list/revoke path from convex/http.ts; switch to cachedFetchJson (stampede protection, env-prefixed keys, standard NEG_SENTINEL); add tenancy check on cache-invalidation endpoint via new /api/internal-get-key-owner route; add 22 Convex tests covering tier gate, per-user limit, duplicate hash, ownership revoke guard, getKeyOwner, and touchKeyLastUsed debounce. P3: tighten keyPrefix regex to exactly 5 hex chars; debounce touchKeyLastUsed (5 min); surface PRO_REQUIRED in UI. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
…ge routes, harden error paths - Gate API key creation/validation on features.apiAccess instead of tier >= 1. Pro (tier 1, apiAccess=false) can no longer mint keys — only API_STARTER+. - Wire wm_ user keys through standalone edge routes (shipping/route-intelligence, shipping/webhooks) that were short-circuiting on validateApiKey before async Convex validation could run. - Restore fail-soft behavior in validateUserApiKey: transient Convex/network errors degrade to unauthorized instead of bubbling a 500. - Fail-closed on cache invalidation endpoint: ownership check errors now return 503 instead of silently proceeding (surfaces Convex outages in logs). - Tests updated: positive paths use api_starter (apiAccess=true), new test locks Pro-without-API-access rejection. 23 tests pass. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
082f330 to
f9b1ffd
Compare
|
@koala73 — Ready for re-review. Latest commit P0 fix:
P1 fix:
Bug fix: fail-soft regression
Residual: fail-closed invalidation endpoint
Tests
Branch rebased on latest main. |
|
I’m Codex. Re-reviewed the latest head ( What now looks fixed:
One residual I still notice: From Codex’s side, no new blocking findings on this head. |
Re-review — PR #3125 (commit
|
| File | Line | Gate |
|---|---|---|
convex/apiKeys.ts |
38-43 | !entitlement.features.apiAccess → API_ACCESS_REQUIRED |
server/_shared/premium-check.ts |
26 | ent.features.apiAccess === true |
server/gateway.ts |
343 | !ent.features.apiAccess |
Pro (tier 1, apiAccess: false) is correctly rejected. API_STARTER+ (tier 2, apiAccess: true) passes. Gate is catalog-driven via features.apiAccess, not tier-as-proxy.
✅ Tests properly updated
- Positive path uses
seedApiEntitlement→getFeaturesForPlan("api_starter")(apiAccess: true, tier 2) - New
rejects pro-tier users without apiAccesstest (line 79-87) locks the exact regression withseedProEntitlement→getFeaturesForPlan("pro_monthly")(apiAccess: false) - Expired entitlement test updated to use
seedApiEntitlementwithvalidUntil: PAST - All 22 tests exercise the correct fixtures
✅ Standalone endpoints covered
api/v2/shipping/route-intelligence.ts:69 and webhooks.ts:158 both call isCallerPremium(req) after accepting the wm_ key, which re-verifies apiAccess === true on the key owner via getEntitlements. Downgraded users are caught.
✅ Invalidation endpoint now fail-closed
api/invalidate-user-api-key-cache.ts: returns 503 when Convex env is missing (line 64-67), on non-200 from Convex (line 79-82), and on fetch failure (line 91-94). ownerData === null (hash not in DB) correctly returns 200 (no-op). Tenancy guard properly blocks cross-user invalidation (line 88-89).
✅ UI updated
Error message changed to "API keys require an API access subscription (API Starter or higher)" (line 722-723).
🛑 New P1 — webhook ownership model breaks with user-owned keys
api/v2/shipping/webhooks.ts:112-119,121-124,219-240,248-255,257-289
The PR wires wm_ user keys into shipping/webhooks, but webhook ownership is keyed to SHA-256(apiKey) via callerFingerprint(), not to the user. With the new multi-key model (up to 5 keys per user), this creates three concrete failures:
-
Cross-key blindness: User creates webhook with key A (
ownerTag = SHA-256(A)), then calls with key B →callerFingerprint = SHA-256(B)→ owner index miss → "no webhooks". Same user, invisible resources. -
Revoke orphans: Revoking key A leaves the webhook's
ownerTagpointing at a dead fingerprint. No key can ever match it again. The webhook becomes permanently unmanageable (can't list, update, deactivate, or rotate secret). -
Owner index fragmentation: The Redis Set at
webhook:owner:<hash>:v1is per-key, not per-user. 5 keys = 5 disjoint owner indexes for the same account.
Root cause: callerFingerprint() was designed for static admin keys (1 key = 1 identity). User-owned keys break that 1:1 assumption.
Recommended fix: Remove the wm_ fallback from webhooks.ts only (keep it on route-intelligence.ts which is read-only, no ownership state). Then migrate webhook ownership from key-hash to userId in a follow-up PR. Trying to fix the ownership model inside this PR risks touching webhook delivery workers and the HMAC signing path.
Residual (non-blocking, unchanged)
listApiKeyssilent[]on Convex unavailablegetAuthState().userstatic at render time- Hardcoded
#22c55e,window.confirm
Verdict
The apiAccess gate is correctly wired everywhere. Ship-ready once the wm_ fallback is removed from webhooks.ts (a 9-line revert). The webhook ownership migration can follow separately.
Webhook ownership is keyed to SHA-256(apiKey) via callerFingerprint(), not to the user. With user-owned keys (up to 5 per user), this causes cross-key blindness (webhooks invisible when calling with a different key) and revoke-orphaning (revoking the creating key makes the webhook permanently unmanageable). User keys remain supported on the read-only route-intelligence endpoint. Webhook ownership migration to userId will follow in a separate PR.
Webhook ownership P1 — fixed in
|
…QUIRED
NS_ERROR_UNEXPECTED (WORLDMONITOR-N6/N7/N8/N9): Firefox 149/Ubuntu XPCOM
Worker init failure. Same family as already-filtered NS_ERROR_ABORT and
NS_ERROR_OUT_OF_MEMORY. 0 repo matches. Worker fallback confirmed working
via breadcrumbs ("keeping flat list").
ConvexError: API_ACCESS_REQUIRED (WORLDMONITOR-NA, 15 events/5 users):
expected business error from PR #3125 (API key management). Free user opens
API Keys tab, server correctly denies, client try/catch at
UnifiedSettings.ts:731 handles gracefully. Convex WS transport leaks the
rejection to Sentry before the client Promise chain catches it.
…QUIRED (#3188) NS_ERROR_UNEXPECTED (WORLDMONITOR-N6/N7/N8/N9): Firefox 149/Ubuntu XPCOM Worker init failure. Same family as already-filtered NS_ERROR_ABORT and NS_ERROR_OUT_OF_MEMORY. 0 repo matches. Worker fallback confirmed working via breadcrumbs ("keeping flat list"). ConvexError: API_ACCESS_REQUIRED (WORLDMONITOR-NA, 15 events/5 users): expected business error from PR #3125 (API key management). Free user opens API Keys tab, server correctly denies, client try/catch at UnifiedSettings.ts:731 handles gracefully. Convex WS transport leaks the rejection to Sentry before the client Promise chain catches it.
2 participants
Summary
wm_prefix formatBackend
userApiKeysConvex table with indexes onuserIdandkeyHashcreateApiKey,listApiKeys,revokeApiKeyvalidateKeyByHash,touchKeyLastUsedfor gateway validation/api/api-keys(authenticated CRUD) +/api/internal-validate-api-key(service-to-service)premium-check.tsupdated to validate user-owned keys before static key listFrontend
Closes #3116
Depends on #3115 (login gate removal)
Test plan
wm_XXXXX********prefixwm_formatX-WorldMonitor-Keyheader against a premium endpoint — should authenticate🤖 Generated with Claude Code