diff --git a/.env.example b/.env.example index bd71dc4b..53e69aff 100644 --- a/.env.example +++ b/.env.example @@ -31,6 +31,9 @@ REDIS_URL=redis://localhost:6379 PRICE_CACHE_TTL=10 # How long to cache GET /snapshot responses per network in seconds (default: 15) SNAPSHOT_CACHE_TTL=15 +# How long an API-key lookup is cached in-process, in seconds (default: 30). +# Worst-case window for a key revoked on another instance to stop working here. +API_KEY_CACHE_TTL=30 # Default timeout for outbound HTTP requests in milliseconds (default: 10000) HTTP_TIMEOUT_MS=10000 diff --git a/src/__tests__/auth.test.ts b/src/__tests__/auth.test.ts index 77915164..248ad0b2 100644 --- a/src/__tests__/auth.test.ts +++ b/src/__tests__/auth.test.ts @@ -20,9 +20,16 @@ import { hashApiKey, extractBearer, lookupApiKey, + invalidateApiKey, + clearApiKeyCache, } from '../api/auth' import { registerAdminRoutes, generateApiKey } from '../api/admin' -import { register, api_key_auth_failures_total, admin_auth_failures_total } from '../metrics' +import { + register, + api_key_auth_failures_total, + admin_auth_failures_total, + api_key_cache_lookups_total, +} from '../metrics' const mockFindUnique = prisma.apiKey.findUnique as unknown as ReturnType const mockCreate = prisma.apiKey.create as unknown as ReturnType @@ -58,6 +65,9 @@ beforeEach(() => { mockFindUnique.mockReset() mockCreate.mockReset() mockUpdate.mockReset() + // The lookup cache is module-global; a stale entry from a previous test + // would otherwise mask a different mock for the same key. + clearApiKeyCache() }) describe('hashApiKey / extractBearer', () => { @@ -249,6 +259,96 @@ describe('per-key rate quotas', () => { }) }) +describe('API key lookup cache (Issue #495)', () => { + const validRecord = { + id: 'key-1', label: 'acme', ratePerMin: 120, ratePerDay: 50000, revokedAt: null, + } + + it('serves a repeated request for the same key from cache (no Postgres per request)', async () => { + mockFindUnique.mockResolvedValue({ ...validRecord }) + const app = await buildAuthedApp() + const headers = { authorization: 'Bearer good' } + const r1 = await app.inject({ method: 'GET', url: '/price/test', headers }) + const r2 = await app.inject({ method: 'GET', url: '/price/test', headers }) + const r3 = await app.inject({ method: 'GET', url: '/price/test', headers }) + expect([r1.statusCode, r2.statusCode, r3.statusCode]).toEqual([200, 200, 200]) + // Three requests, one database lookup. + expect(mockFindUnique).toHaveBeenCalledTimes(1) + }) + + it('caches negative lookups so invalid keys do not hit Postgres per attempt', async () => { + mockFindUnique.mockResolvedValue(null) + expect(await lookupApiKey('bad')).toBeNull() + expect(await lookupApiKey('bad')).toBeNull() + expect(await lookupApiKey('bad')).toBeNull() + expect(mockFindUnique).toHaveBeenCalledTimes(1) + }) + + it('re-checks expiry on a cache hit, so the TTL never outlives the key', async () => { + vi.useFakeTimers() + try { + mockFindUnique.mockResolvedValue({ + ...validRecord, expiresAt: new Date(Date.now() + 5_000), + }) + expect(await lookupApiKey('soon')).not.toBeNull() + // Move past the key's own expiry but stay well within the cache TTL. + vi.advanceTimersByTime(6_000) + expect(await lookupApiKey('soon')).toBeNull() + // Still served from cache — expiry was re-evaluated, not re-queried. + expect(mockFindUnique).toHaveBeenCalledTimes(1) + } finally { + vi.useRealTimers() + } + }) + + it('stops a revoked key immediately on the instance that handles the revoke', async () => { + const token = 'to-be-revoked' + mockFindUnique.mockResolvedValue({ ...validRecord }) + const app = await buildAuthedApp() + const headers = { authorization: `Bearer ${token}` } + expect((await app.inject({ method: 'GET', url: '/price/test', headers })).statusCode).toBe(200) + + // Revoke: Postgres now reports it revoked, and the revoke path drops the + // cache entry (as admin.ts does after the UPDATE). + mockFindUnique.mockResolvedValue({ ...validRecord, revokedAt: new Date() }) + invalidateApiKey(hashApiKey(token)) + + const res = await app.inject({ method: 'GET', url: '/price/test', headers }) + expect(res.statusCode).toBe(401) + expect(res.json().code).toBe('revoked') + }) + + it('stops a revoked key within the cache TTL even without explicit invalidation', async () => { + vi.useFakeTimers() + try { + mockFindUnique.mockResolvedValue({ ...validRecord }) + expect(await lookupApiKey('cross-instance')).not.toBeNull() + + // Revoked on another instance: our cache is not invalidated here. + mockFindUnique.mockResolvedValue({ ...validRecord, revokedAt: new Date() }) + vi.advanceTimersByTime(29_000) // within the 30s TTL — still trusted + expect(await lookupApiKey('cross-instance')).not.toBeNull() + vi.advanceTimersByTime(2_000) // past the TTL — re-read, now rejected + expect(await lookupApiKey('cross-instance')).toBeNull() + } finally { + vi.useRealTimers() + } + }) + + it('exports cache lookups as a hit/miss metric', async () => { + mockFindUnique.mockResolvedValue({ ...validRecord }) + await lookupApiKey('metric-key') // miss → Postgres + await lookupApiKey('metric-key') // hit → cache + const output = await register.metrics() + expect(output).toContain('api_key_cache_lookups_total') + const values = (await api_key_cache_lookups_total.get()).values + const hit = values.find((v) => v.labels.result === 'hit')?.value ?? 0 + const miss = values.find((v) => v.labels.result === 'miss')?.value ?? 0 + expect(hit).toBeGreaterThanOrEqual(1) + expect(miss).toBeGreaterThanOrEqual(1) + }) +}) + describe('admin endpoints', () => { const ORIGINAL = process.env.ADMIN_TOKEN beforeEach(() => { process.env.ADMIN_TOKEN = 'admin-secret' }) diff --git a/src/__tests__/soroswapIngester.test.ts b/src/__tests__/soroswapIngester.test.ts index 9ae7358b..3369db5d 100644 --- a/src/__tests__/soroswapIngester.test.ts +++ b/src/__tests__/soroswapIngester.test.ts @@ -101,6 +101,12 @@ describe('ingestPool', () => { beforeEach(() => { vi.clearAllMocks() + // clearAllMocks resets call history but not implementations, so restore the + // default resolved behavior each test starts from. Without this, a prior + // test's mockRejectedValue could survive into the next one under shuffled + // ordering. + ;(upsertPricePoints as ReturnType).mockResolvedValue(undefined) + ;(dispatchPriceUpdate as ReturnType).mockResolvedValue(undefined) }) it('stores a price point when reserves are non-zero', async () => { @@ -144,7 +150,12 @@ describe('ingestPool', () => { it('does not throw when upsertPricePoints rejects', async () => { const mockFetch = vi.fn().mockResolvedValue([1000n, 200n]) - ;(upsertPricePoints as ReturnType).mockRejectedValue(new Error('DB error')) + // `...Once`: this rejection must not outlive this test. `beforeEach` only + // calls `vi.clearAllMocks()`, which resets call history but NOT mock + // implementations, so a persistent `mockRejectedValue` here would leak into + // whichever test runs next — and test order is shuffled in CI, so the + // victim (e.g. "stores a price point") would fail intermittently. + ;(upsertPricePoints as ReturnType).mockRejectedValueOnce(new Error('DB error')) await expect(ingestPool(mockPoolEntry, mockPair as any, mockFetch)).resolves.toBeUndefined() }) diff --git a/src/api/admin.ts b/src/api/admin.ts index 6cd01f68..31899d42 100644 --- a/src/api/admin.ts +++ b/src/api/admin.ts @@ -2,7 +2,7 @@ import type { FastifyInstance, FastifyRequest, FastifyReply } from 'fastify' import { randomBytes } from 'crypto' import { prisma } from '../db' import { admin_auth_failures_total } from '../metrics' -import { hashApiKey, isAdminAuthorized } from './auth' +import { hashApiKey, isAdminAuthorized, invalidateApiKey } from './auth' /** Generates a new opaque API key with a recognizable `lens_` prefix. */ export function generateApiKey(): string { @@ -111,6 +111,10 @@ export async function registerAdminRoutes(app: FastifyInstance) { data: { revokedAt: new Date() }, }) + // Drop the cached lookup so the revoke takes effect immediately on this + // instance (Issue #495). Other instances pick it up within the cache TTL. + invalidateApiKey(existing.hash) + return reply.status(200).send({ id, revoked: true }) }, ) diff --git a/src/api/auth.ts b/src/api/auth.ts index d0eec6dd..3355c8a8 100644 --- a/src/api/auth.ts +++ b/src/api/auth.ts @@ -2,7 +2,8 @@ import type { FastifyInstance, FastifyRequest, FastifyReply } from 'fastify' import { createHash, timingSafeEqual } from 'crypto' import fp from 'fastify-plugin' import { prisma } from '../db' -import { api_key_auth_failures_total } from '../metrics' +import { config } from '../config' +import { api_key_auth_failures_total, api_key_cache_lookups_total } from '../metrics' type ApiKeyAuthFailureReason = 'missing' | 'malformed' | 'unknown' | 'revoked' | 'expired' @@ -39,6 +40,80 @@ export function extractBearer(header: string | undefined): string | null { return match ? match[1].trim() : null } +// ── In-process API-key lookup cache (Issue #495) ──────────────────────────── +// +// Authentication runs in an `onRequest` hook, so without a cache every request +// — including ones served entirely from the Redis price cache — pays a Postgres +// round-trip before any work begins, and the API cannot even accept traffic +// when the shared connection pool is slow or exhausted. We cache the lookup, +// keyed by the key hash, for a short TTL (`config.cache.apiKeyTtl` seconds). +// +// Correctness of the sharp edges: +// • Revocation. The revoke route (admin.ts) calls `invalidateApiKey(hash)`, +// so a key revoked on THIS instance stops working immediately. A key +// revoked on another instance keeps working here until its cache entry +// expires — a worst case of exactly `config.cache.apiKeyTtl` seconds. The +// test "a revoked key stops working" pins this contract. +// • Expiry. We store the key's own `expiresAt` and re-check it on every hit +// (see `evaluate`), so the TTL never extends a key past its expiry. +// • Negative lookups are cached too. Otherwise an invalid key is a free way +// to make the API hit Postgres as fast as a client can send requests. +// • The cache is size-bounded (`MAX_CACHE_ENTRIES`) so a flood of distinct +// invalid keys cannot grow the negative cache without limit; the oldest +// entry is evicted once the cap is reached. + +/** What we remember about a hash, enough to re-derive the auth result. */ +type CachedKey = + | { found: false } + | { found: true; revoked: boolean; expiresAt: Date | null; context: ApiKeyContext } + +interface CacheEntry { + /** Epoch ms after which this entry is stale and must be re-read from Postgres. */ + staleAt: number + value: CachedKey +} + +const MAX_CACHE_ENTRIES = 10_000 +const apiKeyCache = new Map() + +/** Turns a remembered lookup into an auth result, re-checking expiry at `now`. */ +function evaluate( + value: CachedKey, + now: number, +): { context: ApiKeyContext | null; failureReason?: ApiKeyAuthFailureReason } { + if (!value.found) return { context: null, failureReason: 'unknown' } + if (value.revoked) return { context: null, failureReason: 'revoked' } + if (value.expiresAt && value.expiresAt.getTime() <= now) { + return { context: null, failureReason: 'expired' } + } + return { context: value.context } +} + +function cacheSet(hash: string, value: CachedKey, now: number): void { + // Re-insert so the Map's insertion order tracks recency, then evict the + // oldest entries while we are over the cap. + apiKeyCache.delete(hash) + apiKeyCache.set(hash, { staleAt: now + config.cache.apiKeyTtl * 1000, value }) + while (apiKeyCache.size > MAX_CACHE_ENTRIES) { + const oldest = apiKeyCache.keys().next().value + if (oldest === undefined) break + apiKeyCache.delete(oldest) + } +} + +/** + * Drops a key's cached lookup so the next request re-reads it from Postgres. + * Called on revoke to make revocation effectively immediate on this instance. + */ +export function invalidateApiKey(hash: string): void { + apiKeyCache.delete(hash) +} + +/** Clears the entire lookup cache. Intended for tests. */ +export function clearApiKeyCache(): void { + apiKeyCache.clear() +} + /** * Looks up an API key by its plaintext value, returning the key context only * when the key exists and has not been revoked or expired. @@ -51,20 +126,31 @@ async function lookupApiKeyResult( plaintext: string, ): Promise<{ context: ApiKeyContext | null; failureReason?: ApiKeyAuthFailureReason }> { const hash = hashApiKey(plaintext) - const record = await prisma.apiKey.findUnique({ where: { hash } }) - if (!record) return { context: null, failureReason: 'unknown' } - if (record.revokedAt) return { context: null, failureReason: 'revoked' } - if (record.expiresAt && record.expiresAt <= new Date()) { - return { context: null, failureReason: 'expired' } - } - return { - context: { - id: record.id, - label: record.label, - ratePerMin: record.ratePerMin, - ratePerDay: record.ratePerDay, - }, + const now = Date.now() + + const cached = apiKeyCache.get(hash) + if (cached && cached.staleAt > now) { + api_key_cache_lookups_total.inc({ result: 'hit' }) + return evaluate(cached.value, now) } + + api_key_cache_lookups_total.inc({ result: 'miss' }) + const record = await prisma.apiKey.findUnique({ where: { hash } }) + const value: CachedKey = record + ? { + found: true, + revoked: Boolean(record.revokedAt), + expiresAt: record.expiresAt ?? null, + context: { + id: record.id, + label: record.label, + ratePerMin: record.ratePerMin, + ratePerDay: record.ratePerDay, + }, + } + : { found: false } + cacheSet(hash, value, now) + return evaluate(value, now) } /** diff --git a/src/config.ts b/src/config.ts index 35c54518..693a30dd 100644 --- a/src/config.ts +++ b/src/config.ts @@ -274,6 +274,13 @@ const globalConfig = { // cheap, pollable summary, so a slightly stale copy is preferable to a // per-request scan, but callers must never be shown a minutes-old market. snapshotTtl: parseInt(process.env.SNAPSHOT_CACHE_TTL ?? '15', 10), + // Issue #495: how long an API-key lookup (positive OR negative) is held in + // the in-process auth cache before it must be re-read from Postgres. This + // is the worst-case window in which a key revoked on ANOTHER instance can + // still authenticate here — on the instance that handles the revoke, + // invalidation is immediate. Expiry is re-checked on every cache hit, so + // this TTL never extends a key past its own `expiresAt`. Keep it short. + apiKeyTtl: parseInt(process.env.API_KEY_CACHE_TTL ?? '30', 10), }, http: { timeoutMs: parseInt(process.env.HTTP_TIMEOUT_MS ?? '10000', 10), diff --git a/src/metrics.ts b/src/metrics.ts index 2d10a735..b88410f0 100644 --- a/src/metrics.ts +++ b/src/metrics.ts @@ -69,6 +69,16 @@ export const admin_auth_failures_total = new Counter({ registers: [register] }) +export const api_key_cache_lookups_total = new Counter({ + name: 'api_key_cache_lookups_total', + help: 'API key auth cache lookups by result. Hit rate = hit / (hit + miss).', + // `result` is bounded to hit|miss, so cardinality is fixed at two series. + // A hit is served entirely from the in-process cache with no Postgres query; + // a miss falls through to `prisma.apiKey.findUnique`. + labelNames: ['result'], + registers: [register] +}) + export const rate_limit_rejections_total = new Counter({ name: 'rate_limit_rejections_total', help: 'Total number of requests rejected by the per-key or fallback IP rate limiter',