feat(api): add API key auth and per-key rate quotas#69
Conversation
|
@Salmatcre8 Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
Miracle656
left a comment
There was a problem hiding this comment.
Really solid work — only SHA-256 hashes are stored, auth is fail-secure by default (REQUIRE_API_KEY must be explicitly false to disable), routes opt out cleanly via config.public, revoke is idempotent, and the 17-test suite covers the important paths. The typecheck failure in CI is the two pre-existing webhookDispatcher.ts errors on main — your code adds none. Two things to address before merge, plus a couple of nits.
🟠 The WebSocket endpoint will break under auth
/ws is registered in src/api/websocket.ts as app.get('/ws', { websocket: true }, ...) with no config.public. Since the auth hook is a global onRequest, the WS upgrade request now requires Authorization: Bearer <key> — but browser WebSocket clients can't set custom headers on the handshake. With REQUIRE_API_KEY defaulting to on, the live feed stops working for browser clients.
Pick one:
- Mark
/wsconfig.public = trueand authenticate inside the handler, or - Accept the key as a query param (
/ws?token=...) and validate it in theonRequest/handler, or - Use the
Sec-WebSocket-Protocolsubprotocol header to carry the key.
Please also sanity-check /graphql and the x402 routes — those are fine to gate (clients can send headers), just confirm it's intended.
🟠 ratePerDay is stored and returned but never enforced
The model, key context, mint response, and CLI all carry ratePerDay, and the PR/acceptance criteria say "each key honors its own quota" — but @fastify/rate-limit is only wired for the per-minute window (max: req.apiKey?.ratePerMin, timeWindow: '1 minute'). There's no daily limiter, so the daily quota silently does nothing. Either add a second daily-window limiter keyed on ratePerDay, or drop the field + claim for now and file a follow-up.
🟢 Nits (non-blocking)
isAdminAuthorized:supplied === adminTokenis a non-constant-time comparison of a shared secret. Usecrypto.timingSafeEqual(guard for length mismatch) to avoid the timing side-channel.POST /admin/keysdoesn't validateratePerMin/ratePerDayare positive integers — admin-only, so low risk, but a negative/NaN value would land in the DB and feed the limiter.
Note (not a blocker)
This is a breaking change for any current consumer: deployments must set ADMIN_TOKEN and mint keys (npm run key:issue) before non-public routes are usable. Worth a line in the README / release notes. Closes #54 nicely otherwise.
Happy to merge once /ws is unblocked and the daily quota is either enforced or removed.
Miracle656
left a comment
There was a problem hiding this comment.
Merging — second-account PR. The pre-existing webhookDispatcher.ts CI failure is being deferred until after the Wave. Applying the WebSocket-auth fix + admin timing-safe comparison directly on main as a follow-up.
Summary
Adds API key authentication with per-key rate quotas, plus an admin surface for minting and revoking keys. Lens previously had no auth — every endpoint was open.
Closes #54
What was built
api_keystable (Prisma model + rawsql/schema.sql)id, hash, label, rate_per_min, rate_per_day, revoked_at, created_at.src/api/auth.ts— auth preHandler/onRequest hookAuthorization: Bearer <key>, hashes it, looks it up, and attachesreq.apiKey(id + quotas).onRequesthook before@fastify/rate-limitso the limiter can readreq.apiKey.config.public = true(used by/status,/metrics, and the admin routes).Per-key rate quotas (
src/index.ts)@fastify/rate-limitnow uses a dynamicmax= the key'sratePerMin, keyed by the key id (falls back to a shared IP-based limit for public/anonymous traffic).src/api/admin.ts— key issuance/revocationPOST /admin/keysmints a key and returns the plaintext once;DELETE /admin/keys/:idrevokes.ADMIN_TOKENenv var (X-Admin-Tokenheader).scripts/issue-api-key.ts— CLI for minting the first key:npm run key:issue -- --label "Acme" --per-min 120 --per-day 50000.Env vars documented in
.env.example(ADMIN_TOKEN,REQUIRE_API_KEY).Verification
New test suite
src/__tests__/auth.test.ts(17 tests) plus the existing suite:Covers: SHA-256 hashing, bearer extraction, lookup (valid/unknown/revoked), 401 paths, public-route bypass, per-key quota enforcement (429 after quota, independent buckets per key), and the admin mint/revoke endpoints (incl. asserting only the hash is stored).
npx tsc --noEmit: no new errors (the 2 pre-existingwebhookDispatcher.tserrors exist onmainand are unrelated).Acceptance criteria