diff --git a/src/lib/__tests__/api-handler-429-dressing.test.ts b/src/lib/__tests__/api-handler-429-dressing.test.ts new file mode 100644 index 000000000..dba3e70cd --- /dev/null +++ b/src/lib/__tests__/api-handler-429-dressing.test.ts @@ -0,0 +1,191 @@ +/** + * Which 429s `apiHandler` is allowed to dress with rate-limit headers. + * + * The wrapper attaches `Retry-After` and the `X-RateLimit` triple to any 429 + * that leaves a handler without them, and it takes the numbers from the + * request-scoped limiter capture. That is only honest when a limiter actually + * refused: an AI budget that is spent for the day, an hourly generation quota + * and a provider-side 429 relayed onward are all 429s no bucket produced, and + * describing them with a bucket that still has room tells the client to retry + * into a ceiling that has not moved. + * + * So both directions are pinned here: a refusal from the limiter gets the + * headers, a 429 from anything else gets none. + */ +import { NextRequest, NextResponse } from "next/server"; +import { describe, expect, it, vi } from "vitest"; + +// --- Mocks must be hoisted before importing the module under test. --- + +vi.mock("@/lib/db", () => ({ + prisma: {}, +})); + +vi.mock("@/lib/auth/session", () => ({ + getSession: vi.fn(), +})); + +vi.mock("@/lib/auth/hmac", () => ({ + hashToken: vi.fn(), +})); + +vi.mock("@/lib/auth/audit", () => ({ + auditLog: vi.fn().mockResolvedValue(undefined), +})); + +vi.mock("@/lib/logging/transports", () => ({ + emitIfSampled: vi.fn(), +})); + +vi.mock("next/headers", () => ({ + headers: vi.fn(async () => ({ get: () => null })), + cookies: vi.fn(async () => ({ + get: () => undefined, + set: () => {}, + delete: () => {}, + })), +})); + +import { apiHandler } from "@/lib/api-handler"; +import { captureRateLimitResult } from "@/lib/rate-limit-context"; + +const RATE_LIMIT_HEADERS = [ + "Retry-After", + "X-RateLimit-Limit", + "X-RateLimit-Remaining", + "X-RateLimit-Reset", +] as const; + +function refusal(): NextResponse { + return NextResponse.json({ data: null, error: "Too many" }, { status: 429 }); +} + +function request(): NextRequest { + return new NextRequest("http://localhost/api/test", { method: "POST" }); +} + +describe("apiHandler — 429 header dressing", () => { + it("dresses a 429 the limiter refused", async () => { + const resetAt = Date.now() + 30_000; + const route = apiHandler(async (_request: NextRequest) => { + captureRateLimitResult({ + allowed: false, + limit: 60, + remaining: 0, + resetAt, + }); + return refusal(); + }); + + const res = await route(request()); + + expect(res.status).toBe(429); + expect(res.headers.get("X-RateLimit-Limit")).toBe("60"); + expect(res.headers.get("X-RateLimit-Remaining")).toBe("0"); + expect(res.headers.get("X-RateLimit-Reset")).toBe( + new Date(resetAt).toISOString(), + ); + expect(Number(res.headers.get("Retry-After"))).toBeGreaterThan(0); + }); + + it("leaves a 429 no limiter produced undressed", async () => { + const route = apiHandler(async (_request: NextRequest) => refusal()); + + const res = await route(request()); + + expect(res.status).toBe(429); + for (const name of RATE_LIMIT_HEADERS) { + expect(res.headers.get(name)).toBeNull(); + } + }); + + it("leaves a 429 undressed when every bucket the handler consulted passed", async () => { + // The shape of the AI surfaces: a per-minute limiter lets the request + // through, then a daily budget or an upstream provider refuses it. The + // window the limiter cleared says nothing about when the budget resets. + const route = apiHandler(async (_request: NextRequest) => { + captureRateLimitResult({ + allowed: true, + limit: 10, + remaining: 9, + resetAt: Date.now() + 60_000, + }); + return refusal(); + }); + + const res = await route(request()); + + expect(res.status).toBe(429); + for (const name of RATE_LIMIT_HEADERS) { + expect(res.headers.get(name)).toBeNull(); + } + }); + + it("reports the bucket that refused when the handler consulted two", async () => { + const resetAt = Date.now() + 45_000; + const route = apiHandler(async (_request: NextRequest) => { + captureRateLimitResult({ + allowed: true, + limit: 600, + remaining: 599, + resetAt: Date.now() + 3_600_000, + }); + captureRateLimitResult({ + allowed: false, + limit: 5, + remaining: 0, + resetAt, + }); + return refusal(); + }); + + const res = await route(request()); + + expect(res.headers.get("X-RateLimit-Limit")).toBe("5"); + expect(res.headers.get("X-RateLimit-Reset")).toBe( + new Date(resetAt).toISOString(), + ); + }); + + it("keeps the headers a handler attached itself", async () => { + const route = apiHandler(async (_request: NextRequest) => { + captureRateLimitResult({ + allowed: false, + limit: 60, + remaining: 0, + resetAt: Date.now() + 30_000, + }); + return NextResponse.json( + { data: null, error: "Too many" }, + { + status: 429, + headers: { "Retry-After": "900", "X-RateLimit-Limit": "3" }, + }, + ); + }); + + const res = await route(request()); + + expect(res.headers.get("Retry-After")).toBe("900"); + expect(res.headers.get("X-RateLimit-Limit")).toBe("3"); + }); + + it("leaves a non-429 response alone even after a refusal was captured", async () => { + const route = apiHandler(async (_request: NextRequest) => { + captureRateLimitResult({ + allowed: false, + limit: 60, + remaining: 0, + resetAt: Date.now() + 30_000, + }); + return NextResponse.json({ data: { ok: true }, error: null }); + }); + + const res = await route(request()); + + expect(res.status).toBe(200); + for (const name of RATE_LIMIT_HEADERS) { + expect(res.headers.get(name)).toBeNull(); + } + }); +}); diff --git a/src/lib/__tests__/rate-limit-headers.test.ts b/src/lib/__tests__/rate-limit-headers.test.ts index 6aace7a66..51938a67c 100644 --- a/src/lib/__tests__/rate-limit-headers.test.ts +++ b/src/lib/__tests__/rate-limit-headers.test.ts @@ -7,9 +7,10 @@ * plain integers, and the reset instant in the ISO form it has always carried. * * The capture half is what lets `apiHandler` dress a 429 that a handler built - * without headers. Its one interesting rule is that a refusal outranks a pass: - * a handler that clears one bucket and is refused by the next must report the - * bucket that refused it. + * without headers. It keeps refusals and nothing else: a handler that clears + * one bucket and is refused by the next reports the bucket that refused it, + * and a request every bucket let through reports nothing at all, because a + * 429 it ends up answering came from some other ceiling. */ import { describe, expect, it } from "vitest"; @@ -78,7 +79,7 @@ describe("request-scoped rate-limit capture", () => { expect(capturedRateLimit()).toBeNull(); }); - it("reports the last verdict when every bucket passed", () => { + it("reports nothing when every bucket passed, so a 429 from elsewhere stays undressed", () => { runWithRateLimitCapture(() => { captureRateLimitResult({ allowed: true, @@ -92,12 +93,11 @@ describe("request-scoped rate-limit capture", () => { remaining: 8, resetAt: 2_000, }); - expect(capturedRateLimit()).toEqual({ - allowed: true, - limit: 9, - remaining: 8, - resetAt: 2_000, - }); + // Passing every bucket is exactly the state in which a 429 did not come + // from the limiter. Rendering the passing verdict would put + // `X-RateLimit-Remaining: 8` beside a refusal and a delay measured + // against a window that has nothing to do with it. + expect(capturedRateLimit()).toBeNull(); }); }); diff --git a/src/lib/api-handler.ts b/src/lib/api-handler.ts index 9a916127a..5478c3a88 100644 --- a/src/lib/api-handler.ts +++ b/src/lib/api-handler.ts @@ -380,6 +380,11 @@ export function apiHandler Promise>( * `Retry-After` is the presence test rather than a flag: a handler that built * its own headers already carries it (they come from the same builder), so * this leaves that response exactly as it was written. + * + * The cell holds refusals only, so a 429 no limiter produced — a spent daily + * AI budget, an hourly generation quota, a provider-side 429 relayed onward — + * reads null here and goes out undressed. There is no bucket to describe, and + * one that still has room would name a delay against the wrong window. */ function attachRateLimitHeaders(response: NextResponse): void { if (response.status !== 429) return; diff --git a/src/lib/rate-limit-context.ts b/src/lib/rate-limit-context.ts index 84293d0dc..75df1b2a3 100644 --- a/src/lib/rate-limit-context.ts +++ b/src/lib/rate-limit-context.ts @@ -1,5 +1,5 @@ /** - * Request-scoped capture of the rate-limiter's last verdict, plus the one + * Request-scoped capture of the rate-limiter's refusals, plus the one * builder that turns a verdict into response headers — a LEAF module. * * It lives apart from `rate-limit.ts` on purpose. Nearly two hundred route @@ -29,7 +29,6 @@ export interface RateLimitSnapshot { } interface RateLimitCapture { - last: RateLimitSnapshot | null; denied: RateLimitSnapshot | null; } @@ -41,30 +40,39 @@ const storage = new AsyncLocalStorage(); * {@link capturedRateLimit} reads null. */ export function runWithRateLimitCapture(fn: () => T): T { - return storage.run({ last: null, denied: null }, fn); + return storage.run({ denied: null }, fn); } /** * Record a limiter verdict for the request in flight. * - * A denied verdict wins over an allowed one: a handler that consults two - * buckets and is refused by the second must report the bucket that actually - * refused it, not the one it passed. + * Only refusals are kept. A verdict that let the request through describes a + * bucket that still has room, and there is no 429 it could honestly explain: + * if the response ends up being a 429 anyway, some other ceiling produced it. + * The last refusal wins, since a handler that consults two buckets refuses at + * the first denial it acts on. */ export function captureRateLimitResult( result: RateLimitSnapshot & { allowed: boolean }, ): void { const store = storage.getStore(); if (!store) return; - store.last = result; if (!result.allowed) store.denied = result; } -/** The verdict to render for this request, or null when none was taken. */ +/** + * The refusal to render for this request, or null when no limiter refused it. + * + * Null is the answer for a request every bucket let through, so a 429 raised + * by something else — a daily AI budget, an hourly generation quota, a + * provider-side 429 relayed onward — is left undressed rather than labelled + * with a bucket that has room left and a delay measured against an unrelated + * window. + */ export function capturedRateLimit(): RateLimitSnapshot | null { const store = storage.getStore(); if (!store) return null; - return store.denied ?? store.last; + return store.denied; } /** diff --git a/tests/integration/rate-limit-429-headers.test.ts b/tests/integration/rate-limit-429-headers.test.ts index 8fbb1becc..87800caef 100644 --- a/tests/integration/rate-limit-429-headers.test.ts +++ b/tests/integration/rate-limit-429-headers.test.ts @@ -1,7 +1,7 @@ /** - * A 429 from a route that attaches nothing itself still carries the headers. + * Which 429s carry the rate-limit headers, against a real limiter. * - * `POST /api/measurements/batch` is the one that matters most: a first-run + * `POST /api/measurements/batch` is the case that matters most: a first-run * Apple Health import reliably trips its 60-per-minute ceiling, and until now * the client got a prose sentence and nothing to back off against. The route * builds its refusal with a bare `apiError(...)`, so this proves the headers @@ -9,9 +9,20 @@ * from the handler body — which is what makes the other 128 header-less routes * covered too. * + * Two more shapes ride along, because the dressing is only honest when the + * numbers describe the ceiling that actually refused: + * + * - `POST /api/withings/sync` consults two buckets — a 5/minute baseline and + * a 1/hour bucket on the expensive full walk. A request the baseline lets + * through and the full bucket refuses must report the full bucket. + * - `POST /api/medications/extract` clears its limiter and can then be + * refused by the daily AI token budget. That 429 belongs to no bucket, so + * it must go out with no rate-limit headers at all rather than one naming + * a bucket that still has room. + * * Run against a real Postgres because the limiter's window and remainder come - * out of the atomic upsert; a mocked verdict would prove the plumbing and not - * the numbers. + * out of the atomic upsert, and the budget ledger is a real row too; a mocked + * verdict would prove the plumbing and not the numbers. */ import { NextRequest } from "next/server"; import { beforeEach, describe, expect, it, vi } from "vitest"; @@ -132,4 +143,71 @@ describe("429 rate-limit headers (real Postgres)", () => { expect(res.status).toBe(200); expect(res.headers.get("Retry-After")).toBeNull(); }); + + it("reports the bucket that refused when a route consults two", async () => { + const { POST } = await import("@/app/api/withings/sync/route"); + + // Burn only the full-sync bucket. The route's baseline 5/60s bucket is + // untouched, so it passes on the way in and the refusal comes from the + // 1/hour one — the numbers on the wire have to be that one's. + const { checkRateLimit } = await import("@/lib/rate-limit"); + await checkRateLimit(`withings-sync-full:${TEST_USER_ID}`, 1, 60 * 60_000); + + const res = await POST( + new NextRequest("http://localhost/api/withings/sync", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ fullSync: true }), + }) as never, + ); + + expect(res.status).toBe(429); + expect(res.headers.get("X-RateLimit-Limit")).toBe("1"); + expect(res.headers.get("X-RateLimit-Remaining")).toBe("0"); + // The baseline bucket's window is 60 seconds; the hour-long one cannot be + // mistaken for it. + expect(Number(res.headers.get("Retry-After"))).toBeGreaterThan(60); + }); + + it("leaves a 429 the limiter did not produce undressed", async () => { + // The daily AI budget, not a bucket. The route's own 10-per-5-minutes + // limiter passes on the way in; dressing this refusal with that verdict + // would tell the client nine calls are still free and to retry in five + // minutes, when the budget does not move until UTC midnight. + await getPrismaClient().user.update({ + where: { id: TEST_USER_ID }, + // A self-hosted local endpoint resolves as the chain's primary, which + // keeps the request on the user-plan ceiling and off the consent gate. + data: { aiBaseUrl: "http://127.0.0.1:11434/v1" }, + }); + const { buildDateKey, USER_PLAN_CAP } = + await import("@/lib/ai/coach/budget"); + await getPrismaClient().coachUsage.create({ + data: { + userId: TEST_USER_ID, + dateKey: buildDateKey(), + totalTokens: USER_PLAN_CAP, + messageCount: 1, + }, + }); + + const { POST } = await import("@/app/api/medications/extract/route"); + const res = await POST( + new NextRequest("http://localhost/api/medications/extract", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ text: "Mounjaro 5mg weekly Wednesday" }), + }) as never, + ); + + expect(res.status).toBe(429); + for (const name of [ + "Retry-After", + "X-RateLimit-Limit", + "X-RateLimit-Remaining", + "X-RateLimit-Reset", + ]) { + expect(res.headers.get(name)).toBeNull(); + } + }); });