From 08100b6207500ea1f97b9b26a3686e524e5f5d04 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Thu, 10 Sep 2026 03:55:46 +0200 Subject: [PATCH 01/13] fix(api): put Retry-After and the rate-limit triple on every 429 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A refused client had nothing to back off against. `rateLimitHeaders()` emitted `X-RateLimit-Remaining` and an ISO `X-RateLimit-Reset`, no `Retry-After` and no cap, and only about 70 of the 199 route files that consult a limiter attached even those two. The rest answered with a bare prose envelope, including every endpoint an offline replay queue hits: the four batch ingests, `/api/sync/changes` and `/api/auth/refresh`. A first-run Apple Health import trips the 60-per-minute batch ceiling reliably, and the only strategy left to the client was a blind backoff. Threading headers through 129 handlers is 129 edits the next new route forgets, so the verdict is carried instead: `checkRateLimit` records it in a request-scoped cell and `apiHandler` dresses any 429 that leaves without headers. A route that already built its own is left alone — `Retry-After` is the presence test, and it comes from the same builder. `Retry-After` is whole seconds rounding UP (RFC 9110 §10.2.3), floored at zero. `X-RateLimit-Limit` is new and needs the cap, so the limiter result carries it. `X-RateLimit-Reset` keeps its ISO form: it is the wrong convention for the name, but it is on the wire today and changing it is a decision of its own. The cell lives in its own leaf module because 177 test files replace `@/lib/rate-limit` wholesale with a mock factory, and `apiHandler` must not depend on anything those factories stand in for. --- docs/api/openapi.yaml | 5 +- .../api/auth/check-user/route.test.ts | 5 + .../auth/me/diabetes/__tests__/route.test.ts | 2 + .../me/disable-coach/__tests__/route.test.ts | 2 + .../me/glucose-unit/__tests__/route.test.ts | 2 + .../__tests__/route.test.ts | 2 + .../auth/me/modules/__tests__/route.test.ts | 2 + .../__tests__/route.test.ts | 2 + .../c/[token]/unlock/__tests__/route.test.ts | 2 + .../api/consent/ai/__tests__/route.test.ts | 8 +- .../consent/ai/latest/__tests__/route.test.ts | 9 +- .../consent/ai/web/__tests__/route.test.ts | 8 +- .../digest/dismiss/__tests__/route.test.ts | 2 + .../[id]/extract/__tests__/route.test.ts | 2 + .../[id]/index/__tests__/route.test.ts | 3 + .../__tests__/measurement-streaming.test.ts | 1 + .../export/__tests__/per-type-routes.test.ts | 11 + .../__tests__/soft-delete-filter.test.ts | 1 + .../comprehensive/__tests__/route.test.ts | 1 + .../correlations/__tests__/route.test.ts | 2 + .../insights/generate/__tests__/route.test.ts | 5 + .../web-vitals/__tests__/route.test.ts | 2 + src/app/api/mcp/oauth/__tests__/token.test.ts | 4 + src/app/api/mcp/oauth/authorize/route.ts | 12 +- src/app/api/mcp/oauth/register/route.ts | 6 +- src/app/api/mcp/oauth/token/route.ts | 6 +- .../__tests__/aggregated-hr-wire.test.ts | 1 + .../__tests__/hr-bucket-observability.test.ts | 1 + .../insert-returning-arrivals.test.ts | 1 + .../batch/__tests__/pr-detection-hook.test.ts | 1 + .../batch/__tests__/sync-trigger.test.ts | 2 + .../__tests__/timestamp-plausibility.test.ts | 1 + .../__tests__/unstable-external-id.test.ts | 1 + .../[id]/compliance/__tests__/route.test.ts | 3 + .../[id]/dose-history/__tests__/route.test.ts | 3 + .../[id]/glp1/__tests__/route.test.ts | 3 + .../bulk-delete/__tests__/route.test.ts | 3 + .../intake/import/__tests__/route.test.ts | 2 + .../[id]/inventory/__tests__/route.test.ts | 3 + .../[id]/side-effects/__tests__/route.test.ts | 3 + .../compliance/__tests__/route.test.ts | 3 + .../extract/__tests__/route.test.ts | 2 + .../assessments/__tests__/route.test.ts | 1 + .../nutrients/water/__tests__/route.test.ts | 2 + src/app/api/onboarding/__tests__/step.test.ts | 3 + .../api/user/avatar/__tests__/route.test.ts | 3 + .../workouts/__tests__/batch-create.test.ts | 2 + src/app/mcp/__tests__/route.test.ts | 2 + src/app/mcp/route.ts | 9 +- src/lib/__tests__/rate-limit-headers.test.ts | 141 +++++++++ src/lib/api-handler.ts | 277 ++++++++++-------- .../__tests__/ai-route-support-limit.test.ts | 1 + src/lib/openapi/routes/shared.ts | 3 +- src/lib/rate-limit-context.ts | 97 ++++++ src/lib/rate-limit.ts | 26 +- .../rate-limit-429-headers.test.ts | 135 +++++++++ 56 files changed, 681 insertions(+), 161 deletions(-) create mode 100644 src/lib/__tests__/rate-limit-headers.test.ts create mode 100644 src/lib/rate-limit-context.ts create mode 100644 tests/integration/rate-limit-429-headers.test.ts diff --git a/docs/api/openapi.yaml b/docs/api/openapi.yaml index 72915fbc3..580ddaf59 100644 --- a/docs/api/openapi.yaml +++ b/docs/api/openapi.yaml @@ -101,7 +101,10 @@ paths: schema: $ref: "#/components/schemas/ErrorEnvelope" "429": &a5 - description: Rate limit exceeded. + description: Rate limit exceeded. The response carries `Retry-After` (whole seconds, rounded up — wait at least that + long before retrying), `X-RateLimit-Limit` (the bucket's cap), `X-RateLimit-Remaining` and + `X-RateLimit-Reset` (the reset instant, ISO-8601, not epoch seconds). Back off on `Retry-After` rather than + guessing. content: application/json: schema: diff --git a/src/__tests__/api/auth/check-user/route.test.ts b/src/__tests__/api/auth/check-user/route.test.ts index 7cd4e5eae..8ae3cbd40 100644 --- a/src/__tests__/api/auth/check-user/route.test.ts +++ b/src/__tests__/api/auth/check-user/route.test.ts @@ -27,6 +27,7 @@ vi.mock("@/lib/logging/transports", () => ({ emitIfSampled: vi.fn() })); vi.mock("@/lib/rate-limit", () => ({ checkRateLimit: vi.fn().mockResolvedValue({ allowed: true, + limit: 60, remaining: 29, resetAt: Date.now() + 15 * 60 * 1000, }), @@ -34,6 +35,7 @@ vi.mock("@/lib/rate-limit", () => ({ // wrapper. Default to a clean per-IP result. checkAuthSurfaceRateLimit: vi.fn().mockResolvedValue({ allowed: true, + limit: 60, remaining: 29, resetAt: Date.now() + 15 * 60 * 1000, ip: "203.0.113.1", @@ -84,11 +86,13 @@ beforeEach(() => { vi.resetAllMocks(); vi.mocked(checkRateLimit).mockResolvedValue({ allowed: true, + limit: 60, remaining: 29, resetAt: Date.now() + 15 * 60 * 1000, }); vi.mocked(checkAuthSurfaceRateLimit).mockResolvedValue({ allowed: true, + limit: 60, remaining: 29, resetAt: Date.now() + 15 * 60 * 1000, ip: "203.0.113.1", @@ -175,6 +179,7 @@ describe("POST /api/auth/check-user", () => { it("returns 429 when the per-IP rate-limit is exhausted", async () => { vi.mocked(checkAuthSurfaceRateLimit).mockResolvedValueOnce({ allowed: false, + limit: 60, remaining: 0, resetAt: Date.now() + 15 * 60 * 1000, ip: "203.0.113.1", diff --git a/src/app/api/auth/me/diabetes/__tests__/route.test.ts b/src/app/api/auth/me/diabetes/__tests__/route.test.ts index 5919ff386..0cee31013 100644 --- a/src/app/api/auth/me/diabetes/__tests__/route.test.ts +++ b/src/app/api/auth/me/diabetes/__tests__/route.test.ts @@ -32,6 +32,7 @@ vi.mock("@/lib/db-compat", () => ({ vi.mock("@/lib/rate-limit", () => ({ checkRateLimit: vi.fn().mockResolvedValue({ allowed: true, + limit: 60, remaining: 59, resetAt: Date.now() + 60_000, }), @@ -192,6 +193,7 @@ describe("PATCH /api/auth/me/diabetes", () => { vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); vi.mocked(checkRateLimit).mockResolvedValueOnce({ allowed: false, + limit: 60, remaining: 0, resetAt: Date.now() + 30_000, }); diff --git a/src/app/api/auth/me/disable-coach/__tests__/route.test.ts b/src/app/api/auth/me/disable-coach/__tests__/route.test.ts index 8042a65c6..5ed9ad5b2 100644 --- a/src/app/api/auth/me/disable-coach/__tests__/route.test.ts +++ b/src/app/api/auth/me/disable-coach/__tests__/route.test.ts @@ -24,6 +24,7 @@ vi.mock("@/lib/db-compat", () => ({ vi.mock("@/lib/rate-limit", () => ({ checkRateLimit: vi.fn().mockResolvedValue({ allowed: true, + limit: 60, remaining: 59, resetAt: Date.now() + 60_000, }), @@ -247,6 +248,7 @@ describe("PATCH /api/auth/me/disable-coach", () => { vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); vi.mocked(checkRateLimit).mockResolvedValueOnce({ allowed: false, + limit: 60, remaining: 0, resetAt: Date.now() + 30_000, }); diff --git a/src/app/api/auth/me/glucose-unit/__tests__/route.test.ts b/src/app/api/auth/me/glucose-unit/__tests__/route.test.ts index 943f37294..a699059cb 100644 --- a/src/app/api/auth/me/glucose-unit/__tests__/route.test.ts +++ b/src/app/api/auth/me/glucose-unit/__tests__/route.test.ts @@ -36,6 +36,7 @@ vi.mock("@/lib/db-compat", () => ({ vi.mock("@/lib/rate-limit", () => ({ checkRateLimit: vi.fn().mockResolvedValue({ allowed: true, + limit: 60, remaining: 59, resetAt: Date.now() + 60_000, }), @@ -197,6 +198,7 @@ describe("PATCH /api/auth/me/glucose-unit", () => { vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); vi.mocked(checkRateLimit).mockResolvedValueOnce({ allowed: false, + limit: 60, remaining: 0, resetAt: Date.now() + 30_000, }); diff --git a/src/app/api/auth/me/health-score-config/__tests__/route.test.ts b/src/app/api/auth/me/health-score-config/__tests__/route.test.ts index 75d4ff20f..da967879f 100644 --- a/src/app/api/auth/me/health-score-config/__tests__/route.test.ts +++ b/src/app/api/auth/me/health-score-config/__tests__/route.test.ts @@ -35,6 +35,7 @@ vi.mock("@/lib/db-compat", () => ({ vi.mock("@/lib/rate-limit", () => ({ checkRateLimit: vi.fn().mockResolvedValue({ allowed: true, + limit: 60, remaining: 59, resetAt: Date.now() + 60_000, }), @@ -397,6 +398,7 @@ describe("PATCH — envelope and transport", () => { vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); vi.mocked(checkRateLimit).mockResolvedValueOnce({ allowed: false, + limit: 60, remaining: 0, resetAt: Date.now() + 30_000, }); diff --git a/src/app/api/auth/me/modules/__tests__/route.test.ts b/src/app/api/auth/me/modules/__tests__/route.test.ts index 1e23957ac..7a14aef25 100644 --- a/src/app/api/auth/me/modules/__tests__/route.test.ts +++ b/src/app/api/auth/me/modules/__tests__/route.test.ts @@ -35,6 +35,7 @@ vi.mock("@/lib/db-compat", () => ({ vi.mock("@/lib/rate-limit", () => ({ checkRateLimit: vi.fn().mockResolvedValue({ allowed: true, + limit: 60, remaining: 59, resetAt: Date.now() + 60_000, }), @@ -300,6 +301,7 @@ describe("PATCH /api/auth/me/modules", () => { vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); vi.mocked(checkRateLimit).mockResolvedValueOnce({ allowed: false, + limit: 60, remaining: 0, resetAt: Date.now() + 30_000, }); diff --git a/src/app/api/auth/me/notification-prefs/__tests__/route.test.ts b/src/app/api/auth/me/notification-prefs/__tests__/route.test.ts index 088cecd53..c8d503649 100644 --- a/src/app/api/auth/me/notification-prefs/__tests__/route.test.ts +++ b/src/app/api/auth/me/notification-prefs/__tests__/route.test.ts @@ -26,6 +26,7 @@ vi.mock("@/lib/db-compat", () => ({ vi.mock("@/lib/rate-limit", () => ({ checkRateLimit: vi.fn().mockResolvedValue({ allowed: true, + limit: 60, remaining: 59, resetAt: Date.now() + 60_000, }), @@ -587,6 +588,7 @@ describe("PATCH /api/auth/me/notification-prefs", () => { vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); vi.mocked(checkRateLimit).mockResolvedValueOnce({ allowed: false, + limit: 60, remaining: 0, resetAt: Date.now() + 30_000, }); diff --git a/src/app/api/c/[token]/unlock/__tests__/route.test.ts b/src/app/api/c/[token]/unlock/__tests__/route.test.ts index 7c0828da3..e7cf2ef56 100644 --- a/src/app/api/c/[token]/unlock/__tests__/route.test.ts +++ b/src/app/api/c/[token]/unlock/__tests__/route.test.ts @@ -68,6 +68,7 @@ beforeEach(() => { vi.stubEnv("API_TOKEN_HMAC_KEY", TEST_HMAC_KEY); vi.mocked(checkAuthSurfaceRateLimit).mockResolvedValue({ allowed: true, + limit: 60, remaining: 9, resetAt: Date.now() + 60_000, ip: "1.2.3.4", @@ -137,6 +138,7 @@ describe("POST /api/c/[token]/unlock", () => { }); vi.mocked(checkAuthSurfaceRateLimit).mockResolvedValue({ allowed: false, + limit: 60, remaining: 0, resetAt: Date.now() + 60_000, ip: "1.2.3.4", diff --git a/src/app/api/consent/ai/__tests__/route.test.ts b/src/app/api/consent/ai/__tests__/route.test.ts index cfb9c52c3..8f4a55859 100644 --- a/src/app/api/consent/ai/__tests__/route.test.ts +++ b/src/app/api/consent/ai/__tests__/route.test.ts @@ -55,7 +55,12 @@ import { getSession } from "@/lib/auth/session"; import { auditLog } from "@/lib/auth/audit"; import { checkConsentRateLimit } from "@/lib/rate-limit"; -const RL_OK = { allowed: true, remaining: 19, resetAt: Date.now() + 60_000 }; +const RL_OK = { + allowed: true, + limit: 20, + remaining: 19, + resetAt: Date.now() + 60_000, +}; const $transaction = vi.mocked(prisma.$transaction) as unknown as { mockImplementation: (impl: (fn: TxFn) => unknown) => void; @@ -109,6 +114,7 @@ describe("POST /api/consent/ai", () => { vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); vi.mocked(checkConsentRateLimit).mockResolvedValue({ allowed: false, + limit: 60, remaining: 0, resetAt: Date.now() + 60_000, }); diff --git a/src/app/api/consent/ai/latest/__tests__/route.test.ts b/src/app/api/consent/ai/latest/__tests__/route.test.ts index b21a15eeb..db3ed6ddb 100644 --- a/src/app/api/consent/ai/latest/__tests__/route.test.ts +++ b/src/app/api/consent/ai/latest/__tests__/route.test.ts @@ -54,7 +54,12 @@ import { getSession } from "@/lib/auth/session"; import { auditLog } from "@/lib/auth/audit"; import { checkConsentRateLimit } from "@/lib/rate-limit"; -const RL_OK = { allowed: true, remaining: 19, resetAt: Date.now() + 60_000 }; +const RL_OK = { + allowed: true, + limit: 20, + remaining: 19, + resetAt: Date.now() + 60_000, +}; const $transaction = vi.mocked(prisma.$transaction) as unknown as { mockImplementation: (impl: (fn: TxFn) => unknown) => void; @@ -99,6 +104,7 @@ describe("GET /api/consent/ai/latest", () => { vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); vi.mocked(checkConsentRateLimit).mockResolvedValue({ allowed: false, + limit: 60, remaining: 0, resetAt: Date.now() + 60_000, }); @@ -197,6 +203,7 @@ describe("DELETE /api/consent/ai/latest", () => { vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); vi.mocked(checkConsentRateLimit).mockResolvedValue({ allowed: false, + limit: 60, remaining: 0, resetAt: Date.now() + 60_000, }); diff --git a/src/app/api/consent/ai/web/__tests__/route.test.ts b/src/app/api/consent/ai/web/__tests__/route.test.ts index d646dcb1f..6d5651727 100644 --- a/src/app/api/consent/ai/web/__tests__/route.test.ts +++ b/src/app/api/consent/ai/web/__tests__/route.test.ts @@ -43,7 +43,12 @@ const SESSION_OK = { user: { id: "user-1", username: "testuser", role: "USER" as const }, }; -const RL_OK = { allowed: true, remaining: 19, resetAt: Date.now() + 60_000 }; +const RL_OK = { + allowed: true, + limit: 20, + remaining: 19, + resetAt: Date.now() + 60_000, +}; /** The mount heal posts no body and no content-type. */ function mkHeal(): Request { @@ -79,6 +84,7 @@ describe("POST /api/consent/ai/web", () => { vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); vi.mocked(checkConsentRateLimit).mockResolvedValue({ allowed: false, + limit: 60, remaining: 0, resetAt: Date.now() + 60_000, }); diff --git a/src/app/api/daily/digest/dismiss/__tests__/route.test.ts b/src/app/api/daily/digest/dismiss/__tests__/route.test.ts index 8c2ab3e05..795f7ef8c 100644 --- a/src/app/api/daily/digest/dismiss/__tests__/route.test.ts +++ b/src/app/api/daily/digest/dismiss/__tests__/route.test.ts @@ -24,6 +24,7 @@ vi.mock("@/lib/modules/gate", () => ({ vi.mock("@/lib/rate-limit", () => ({ checkRateLimit: vi.fn(async () => ({ allowed: true, + limit: 60, remaining: 39, resetAt: Date.now() + 60_000, })), @@ -159,6 +160,7 @@ describe("POST /api/daily/digest/dismiss", () => { const { checkRateLimit } = await import("@/lib/rate-limit"); vi.mocked(checkRateLimit).mockResolvedValueOnce({ allowed: false, + limit: 60, remaining: 0, resetAt: Date.now() + 60_000, }); diff --git a/src/app/api/documents/inbound/[id]/extract/__tests__/route.test.ts b/src/app/api/documents/inbound/[id]/extract/__tests__/route.test.ts index c5b3ba303..6acb80659 100644 --- a/src/app/api/documents/inbound/[id]/extract/__tests__/route.test.ts +++ b/src/app/api/documents/inbound/[id]/extract/__tests__/route.test.ts @@ -168,6 +168,7 @@ beforeEach(() => { vi.mocked(prisma.extractedFact.count).mockResolvedValue(0 as never); vi.mocked(checkRateLimit).mockResolvedValue({ allowed: true, + limit: 60, remaining: 5, resetAt: Date.now() + 60 * 60 * 1000, }); @@ -304,6 +305,7 @@ describe("POST /api/documents/inbound/[id]/extract — stored mode", () => { it("consumes the shared document-AI bucket honestly", async () => { vi.mocked(checkRateLimit).mockResolvedValue({ allowed: false, + limit: 60, remaining: 0, resetAt: Date.now() + 30 * 60_000, }); diff --git a/src/app/api/documents/inbound/[id]/index/__tests__/route.test.ts b/src/app/api/documents/inbound/[id]/index/__tests__/route.test.ts index 5b6540011..aa5b6c7d5 100644 --- a/src/app/api/documents/inbound/[id]/index/__tests__/route.test.ts +++ b/src/app/api/documents/inbound/[id]/index/__tests__/route.test.ts @@ -128,6 +128,7 @@ beforeEach(() => { vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); vi.mocked(checkRateLimit).mockResolvedValue({ allowed: true, + limit: 60, remaining: 5, resetAt: Date.now() + 60 * 60 * 1000, }); @@ -222,6 +223,7 @@ describe("POST /api/documents/inbound/[id]/index — bucket honesty", () => { const resetAt = Date.now() + 47 * 60_000; vi.mocked(checkRateLimit).mockResolvedValue({ allowed: false, + limit: 60, remaining: 0, resetAt, }); @@ -309,6 +311,7 @@ describe("POST /api/documents/inbound/[id]/index — staging continuation", () = it("does not reach staging when the index itself was refused", async () => { vi.mocked(checkRateLimit).mockResolvedValue({ allowed: false, + limit: 60, remaining: 0, resetAt: Date.now() + 60_000, }); diff --git a/src/app/api/export/__tests__/measurement-streaming.test.ts b/src/app/api/export/__tests__/measurement-streaming.test.ts index 3fedbe375..aa674b460 100644 --- a/src/app/api/export/__tests__/measurement-streaming.test.ts +++ b/src/app/api/export/__tests__/measurement-streaming.test.ts @@ -71,6 +71,7 @@ beforeEach(() => { vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); vi.mocked(checkRateLimit).mockResolvedValue({ allowed: true, + limit: 60, remaining: 9, resetAt: Date.now() + 3_600_000, }); diff --git a/src/app/api/export/__tests__/per-type-routes.test.ts b/src/app/api/export/__tests__/per-type-routes.test.ts index f29671ba1..f3ca60494 100644 --- a/src/app/api/export/__tests__/per-type-routes.test.ts +++ b/src/app/api/export/__tests__/per-type-routes.test.ts @@ -190,6 +190,7 @@ describe("GET /api/export/measurements", () => { vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); vi.mocked(checkRateLimit).mockResolvedValue({ allowed: true, + limit: 60, remaining: 9, resetAt: Date.now() + 3600_000, }); @@ -229,6 +230,7 @@ describe("GET /api/export/measurements", () => { vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); vi.mocked(checkRateLimit).mockResolvedValue({ allowed: true, + limit: 60, remaining: 9, resetAt: Date.now() + 3600_000, }); @@ -258,6 +260,7 @@ describe("GET /api/export/measurements", () => { vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); vi.mocked(checkRateLimit).mockResolvedValue({ allowed: true, + limit: 60, remaining: 9, resetAt: Date.now() + 3600_000, }); @@ -289,6 +292,7 @@ describe("GET /api/export/measurements", () => { vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); vi.mocked(checkRateLimit).mockResolvedValue({ allowed: true, + limit: 60, remaining: 9, resetAt: Date.now() + 3600_000, }); @@ -327,6 +331,7 @@ describe("GET /api/export/medications", () => { vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); vi.mocked(checkRateLimit).mockResolvedValue({ allowed: true, + limit: 60, remaining: 9, resetAt: Date.now() + 3600_000, }); @@ -370,6 +375,7 @@ describe("GET /api/export/medications", () => { vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); vi.mocked(checkRateLimit).mockResolvedValue({ allowed: true, + limit: 60, remaining: 9, resetAt: Date.now() + 3600_000, }); @@ -390,6 +396,7 @@ describe("GET /api/export/medications", () => { vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); vi.mocked(checkRateLimit).mockResolvedValue({ allowed: true, + limit: 60, remaining: 9, resetAt: Date.now() + 3600_000, }); @@ -425,6 +432,7 @@ describe("GET /api/export/medications", () => { vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); vi.mocked(checkRateLimit).mockResolvedValue({ allowed: true, + limit: 60, remaining: 9, resetAt: Date.now() + 3600_000, }); @@ -452,6 +460,7 @@ describe("GET /api/export/mood", () => { vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); vi.mocked(checkRateLimit).mockResolvedValue({ allowed: true, + limit: 60, remaining: 9, resetAt: Date.now() + 3600_000, }); @@ -492,6 +501,7 @@ describe("GET /api/export/full-backup", () => { vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); vi.mocked(checkRateLimit).mockResolvedValue({ allowed: true, + limit: 60, remaining: 9, resetAt: Date.now() + 3600_000, }); @@ -562,6 +572,7 @@ describe("GET /api/export/full-backup", () => { vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); vi.mocked(checkRateLimit).mockResolvedValue({ allowed: true, + limit: 60, remaining: 9, resetAt: Date.now() + 3600_000, }); diff --git a/src/app/api/export/__tests__/soft-delete-filter.test.ts b/src/app/api/export/__tests__/soft-delete-filter.test.ts index 329405629..bd4a5669c 100644 --- a/src/app/api/export/__tests__/soft-delete-filter.test.ts +++ b/src/app/api/export/__tests__/soft-delete-filter.test.ts @@ -130,6 +130,7 @@ beforeEach(() => { vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); vi.mocked(checkRateLimit).mockResolvedValue({ allowed: true, + limit: 60, remaining: 9, resetAt: Date.now() + 3_600_000, }); diff --git a/src/app/api/insights/comprehensive/__tests__/route.test.ts b/src/app/api/insights/comprehensive/__tests__/route.test.ts index 372a313ea..6f776d239 100644 --- a/src/app/api/insights/comprehensive/__tests__/route.test.ts +++ b/src/app/api/insights/comprehensive/__tests__/route.test.ts @@ -117,6 +117,7 @@ beforeEach(() => { // v1.15.20 — default to an allowing analytics-read budget. vi.mocked(checkAnalyticsReadRateLimit).mockResolvedValue({ allowed: true, + limit: 60, remaining: 119, resetAt: Date.now() + 60_000, }); diff --git a/src/app/api/insights/correlations/__tests__/route.test.ts b/src/app/api/insights/correlations/__tests__/route.test.ts index 0a110f9ed..bc481a4f4 100644 --- a/src/app/api/insights/correlations/__tests__/route.test.ts +++ b/src/app/api/insights/correlations/__tests__/route.test.ts @@ -103,6 +103,7 @@ beforeEach(() => { // v1.15.20 — default to an allowing analytics-read budget. vi.mocked(checkAnalyticsReadRateLimit).mockResolvedValue({ allowed: true, + limit: 60, remaining: 119, resetAt: Date.now() + 60_000, }); @@ -229,6 +230,7 @@ describe("GET /api/insights/correlations", () => { vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); vi.mocked(checkAnalyticsReadRateLimit).mockResolvedValue({ allowed: false, + limit: 60, remaining: 0, resetAt: Date.now() + 60_000, }); diff --git a/src/app/api/insights/generate/__tests__/route.test.ts b/src/app/api/insights/generate/__tests__/route.test.ts index 55516a66d..5289fa0d1 100644 --- a/src/app/api/insights/generate/__tests__/route.test.ts +++ b/src/app/api/insights/generate/__tests__/route.test.ts @@ -188,6 +188,7 @@ beforeEach(() => { selfContextText.mockResolvedValue(null); vi.mocked(checkRateLimit).mockResolvedValue({ allowed: true, + limit: 60, remaining: 9, resetAt: Date.now() + 3600_000, }); @@ -366,6 +367,7 @@ describe("POST /api/insights/generate — rate limit (v1.4.16 A7.1)", () => { expect(limit).toBe(10); return { allowed: callCount <= 10, + limit: 10, remaining: Math.max(0, 10 - callCount), resetAt: Date.now() + 3600_000, }; @@ -392,6 +394,7 @@ describe("POST /api/insights/generate — rate limit (v1.4.16 A7.1)", () => { expect(limit).toBe(3); return { allowed: callCount <= 3, + limit: 3, remaining: Math.max(0, 3 - callCount), resetAt: Date.now() + 3600_000, }; @@ -673,6 +676,7 @@ describe("POST /api/insights/generate — briefingless fresh cache (v1.28.30)", mockBriefinglessFreshCache(); vi.mocked(checkRateLimit).mockResolvedValue({ allowed: false, + limit: 60, remaining: 0, resetAt: Date.now() + 3600_000, }); @@ -691,6 +695,7 @@ describe("POST /api/insights/generate — briefingless fresh cache (v1.28.30)", mockBriefinglessFreshCache(); vi.mocked(checkRateLimit).mockResolvedValue({ allowed: false, + limit: 60, remaining: 0, resetAt: Date.now() + 3600_000, }); diff --git a/src/app/api/internal/web-vitals/__tests__/route.test.ts b/src/app/api/internal/web-vitals/__tests__/route.test.ts index 81a792abf..e3e1ac303 100644 --- a/src/app/api/internal/web-vitals/__tests__/route.test.ts +++ b/src/app/api/internal/web-vitals/__tests__/route.test.ts @@ -50,6 +50,7 @@ beforeEach(() => { vi.resetAllMocks(); vi.mocked(checkRateLimit).mockResolvedValue({ allowed: true, + limit: 60, remaining: 59, resetAt: Date.now() + 60_000, }); @@ -114,6 +115,7 @@ describe("POST /api/internal/web-vitals", () => { it("returns 429 when the rate-limit fires", async () => { vi.mocked(checkRateLimit).mockResolvedValue({ allowed: false, + limit: 60, remaining: 0, resetAt: Date.now() + 60_000, }); diff --git a/src/app/api/mcp/oauth/__tests__/token.test.ts b/src/app/api/mcp/oauth/__tests__/token.test.ts index 54bf4e451..df17975e9 100644 --- a/src/app/api/mcp/oauth/__tests__/token.test.ts +++ b/src/app/api/mcp/oauth/__tests__/token.test.ts @@ -37,12 +37,14 @@ vi.mock("@/lib/auth/issue-token", () => ({ vi.mock("@/lib/rate-limit", () => ({ checkAuthSurfaceRateLimit: vi.fn(async () => ({ allowed: true, + limit: 60, remaining: 100, resetAt: Date.now() + 60_000, ip: "1.2.3.4", })), checkRateLimit: vi.fn(async () => ({ allowed: true, + limit: 60, remaining: 0, resetAt: Date.now() + 60_000, })), @@ -103,6 +105,7 @@ beforeEach(() => { vi.clearAllMocks(); vi.mocked(checkRateLimit).mockResolvedValue({ allowed: true, + limit: 60, remaining: 0, resetAt: Date.now() + 60_000, }); @@ -215,6 +218,7 @@ describe("authorization_code grant", () => { it("rejects a replayed code (single-use jti already claimed)", async () => { vi.mocked(checkRateLimit).mockResolvedValue({ allowed: false, + limit: 60, remaining: 0, resetAt: Date.now() + 60_000, }); diff --git a/src/app/api/mcp/oauth/authorize/route.ts b/src/app/api/mcp/oauth/authorize/route.ts index 1abc2c847..04de08df8 100644 --- a/src/app/api/mcp/oauth/authorize/route.ts +++ b/src/app/api/mcp/oauth/authorize/route.ts @@ -302,11 +302,7 @@ export async function GET(request: NextRequest): Promise { { error: "temporarily_unavailable" }, { status: 429, - headers: rateLimitHeaders({ - allowed: false, - remaining: rl.remaining, - resetAt: rl.resetAt, - }), + headers: rateLimitHeaders(rl), }, ); } @@ -407,11 +403,7 @@ export async function POST(request: NextRequest): Promise { { error: "temporarily_unavailable" }, { status: 429, - headers: rateLimitHeaders({ - allowed: false, - remaining: rl.remaining, - resetAt: rl.resetAt, - }), + headers: rateLimitHeaders(rl), }, ); } diff --git a/src/app/api/mcp/oauth/register/route.ts b/src/app/api/mcp/oauth/register/route.ts index 046453158..aa5d76a83 100644 --- a/src/app/api/mcp/oauth/register/route.ts +++ b/src/app/api/mcp/oauth/register/route.ts @@ -80,11 +80,7 @@ export async function POST(request: NextRequest): Promise { { error: "temporarily_unavailable" }, { status: 429, - headers: rateLimitHeaders({ - allowed: false, - remaining: rl.remaining, - resetAt: rl.resetAt, - }), + headers: rateLimitHeaders(rl), }, ); } diff --git a/src/app/api/mcp/oauth/token/route.ts b/src/app/api/mcp/oauth/token/route.ts index e9493db20..a82b732a5 100644 --- a/src/app/api/mcp/oauth/token/route.ts +++ b/src/app/api/mcp/oauth/token/route.ts @@ -222,11 +222,7 @@ export async function POST(request: NextRequest): Promise { { error: "temporarily_unavailable" }, { status: 429, - headers: rateLimitHeaders({ - allowed: false, - remaining: rl.remaining, - resetAt: rl.resetAt, - }), + headers: rateLimitHeaders(rl), }, ); } diff --git a/src/app/api/measurements/batch/__tests__/aggregated-hr-wire.test.ts b/src/app/api/measurements/batch/__tests__/aggregated-hr-wire.test.ts index a46987f29..3f766d9fb 100644 --- a/src/app/api/measurements/batch/__tests__/aggregated-hr-wire.test.ts +++ b/src/app/api/measurements/batch/__tests__/aggregated-hr-wire.test.ts @@ -189,6 +189,7 @@ beforeEach(() => { vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); vi.mocked(checkRateLimit).mockResolvedValue({ allowed: true, + limit: 60, remaining: 60, resetAt: Date.now() + 60_000, }); diff --git a/src/app/api/measurements/batch/__tests__/hr-bucket-observability.test.ts b/src/app/api/measurements/batch/__tests__/hr-bucket-observability.test.ts index b5e36298c..7597199f4 100644 --- a/src/app/api/measurements/batch/__tests__/hr-bucket-observability.test.ts +++ b/src/app/api/measurements/batch/__tests__/hr-bucket-observability.test.ts @@ -186,6 +186,7 @@ beforeEach(() => { vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); vi.mocked(checkRateLimit).mockResolvedValue({ allowed: true, + limit: 60, remaining: 60, resetAt: Date.now() + 60_000, }); diff --git a/src/app/api/measurements/batch/__tests__/insert-returning-arrivals.test.ts b/src/app/api/measurements/batch/__tests__/insert-returning-arrivals.test.ts index 9b551c5ac..e7eb79f74 100644 --- a/src/app/api/measurements/batch/__tests__/insert-returning-arrivals.test.ts +++ b/src/app/api/measurements/batch/__tests__/insert-returning-arrivals.test.ts @@ -159,6 +159,7 @@ beforeEach(() => { vi.mocked(getSession).mockResolvedValue(session as never); vi.mocked(checkRateLimit).mockResolvedValue({ allowed: true, + limit: 60, remaining: 59, resetAt: Date.now() + 60_000, }); diff --git a/src/app/api/measurements/batch/__tests__/pr-detection-hook.test.ts b/src/app/api/measurements/batch/__tests__/pr-detection-hook.test.ts index 01f5cb72d..a36974a0c 100644 --- a/src/app/api/measurements/batch/__tests__/pr-detection-hook.test.ts +++ b/src/app/api/measurements/batch/__tests__/pr-detection-hook.test.ts @@ -114,6 +114,7 @@ beforeEach(() => { vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); vi.mocked(checkRateLimit).mockResolvedValue({ allowed: true, + limit: 60, remaining: 60, resetAt: Date.now() + 60_000, }); diff --git a/src/app/api/measurements/batch/__tests__/sync-trigger.test.ts b/src/app/api/measurements/batch/__tests__/sync-trigger.test.ts index fa834286f..83a7416d5 100644 --- a/src/app/api/measurements/batch/__tests__/sync-trigger.test.ts +++ b/src/app/api/measurements/batch/__tests__/sync-trigger.test.ts @@ -194,6 +194,7 @@ beforeEach(() => { vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); vi.mocked(checkRateLimit).mockResolvedValue({ allowed: true, + limit: 60, remaining: 60, resetAt: Date.now() + 60_000, }); @@ -271,6 +272,7 @@ describe("POST /api/measurements/batch — syncTrigger diagnostic (iOS #66)", () vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); vi.mocked(checkRateLimit).mockResolvedValue({ allowed: true, + limit: 60, remaining: 60, resetAt: Date.now() + 60_000, }); diff --git a/src/app/api/measurements/batch/__tests__/timestamp-plausibility.test.ts b/src/app/api/measurements/batch/__tests__/timestamp-plausibility.test.ts index 5c79d949e..8fdeb46da 100644 --- a/src/app/api/measurements/batch/__tests__/timestamp-plausibility.test.ts +++ b/src/app/api/measurements/batch/__tests__/timestamp-plausibility.test.ts @@ -148,6 +148,7 @@ beforeEach(() => { vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); vi.mocked(checkRateLimit).mockResolvedValue({ allowed: true, + limit: 60, remaining: 60, resetAt: Date.now() + 60_000, }); diff --git a/src/app/api/measurements/batch/__tests__/unstable-external-id.test.ts b/src/app/api/measurements/batch/__tests__/unstable-external-id.test.ts index e7369d833..a68fdd012 100644 --- a/src/app/api/measurements/batch/__tests__/unstable-external-id.test.ts +++ b/src/app/api/measurements/batch/__tests__/unstable-external-id.test.ts @@ -151,6 +151,7 @@ beforeEach(() => { vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); vi.mocked(checkRateLimit).mockResolvedValue({ allowed: true, + limit: 60, remaining: 60, resetAt: Date.now() + 60_000, }); diff --git a/src/app/api/medications/[id]/compliance/__tests__/route.test.ts b/src/app/api/medications/[id]/compliance/__tests__/route.test.ts index 4b6de773f..128bcfe87 100644 --- a/src/app/api/medications/[id]/compliance/__tests__/route.test.ts +++ b/src/app/api/medications/[id]/compliance/__tests__/route.test.ts @@ -46,6 +46,7 @@ vi.mock("@/lib/cache/server-cache", () => ({ vi.mock("@/lib/rate-limit", () => ({ checkRateLimit: vi.fn().mockResolvedValue({ allowed: true, + limit: 60, remaining: 59, resetAt: Date.now() + 60_000, }), @@ -139,6 +140,7 @@ beforeEach(() => { ); vi.mocked(checkRateLimit).mockResolvedValue({ allowed: true, + limit: 60, remaining: 59, resetAt: Date.now() + 60_000, }); @@ -416,6 +418,7 @@ describe("GET /api/medications/[id]/compliance — caching and limits", () => { it("returns 429 when the per-user rate limit is exhausted", async () => { vi.mocked(checkRateLimit).mockResolvedValue({ allowed: false, + limit: 60, remaining: 0, resetAt: Date.now() + 60_000, }); diff --git a/src/app/api/medications/[id]/dose-history/__tests__/route.test.ts b/src/app/api/medications/[id]/dose-history/__tests__/route.test.ts index ccc4f707d..4a5c8fed8 100644 --- a/src/app/api/medications/[id]/dose-history/__tests__/route.test.ts +++ b/src/app/api/medications/[id]/dose-history/__tests__/route.test.ts @@ -30,6 +30,7 @@ vi.mock("@/lib/medications/route-guards", () => ({ vi.mock("@/lib/rate-limit", () => ({ checkRateLimit: vi.fn().mockResolvedValue({ allowed: true, + limit: 60, remaining: 59, resetAt: Date.now() + 60_000, }), @@ -92,6 +93,7 @@ beforeEach(() => { vi.mocked(assertMedicationOwnership).mockResolvedValue(null); vi.mocked(checkRateLimit).mockResolvedValue({ allowed: true, + limit: 60, remaining: 59, resetAt: Date.now() + 60_000, }); @@ -134,6 +136,7 @@ describe("GET /api/medications/[id]/dose-history", () => { it("returns 429 when the per-user cap is exhausted", async () => { vi.mocked(checkRateLimit).mockResolvedValueOnce({ allowed: false, + limit: 60, remaining: 0, resetAt: Date.now() + 60_000, }); diff --git a/src/app/api/medications/[id]/glp1/__tests__/route.test.ts b/src/app/api/medications/[id]/glp1/__tests__/route.test.ts index 5e6bbad29..3d52191ee 100644 --- a/src/app/api/medications/[id]/glp1/__tests__/route.test.ts +++ b/src/app/api/medications/[id]/glp1/__tests__/route.test.ts @@ -38,6 +38,7 @@ vi.mock("@/lib/auth/audit", () => ({ vi.mock("@/lib/rate-limit", () => ({ checkRateLimit: vi.fn().mockResolvedValue({ allowed: true, + limit: 60, remaining: 29, resetAt: Date.now() + 60_000, }), @@ -85,6 +86,7 @@ beforeEach(() => { vi.resetAllMocks(); vi.mocked(checkRateLimit).mockResolvedValue({ allowed: true, + limit: 60, remaining: 29, resetAt: Date.now() + 60_000, }); @@ -251,6 +253,7 @@ describe("POST /api/medications/[id]/glp1", () => { vi.mocked(prisma.medication.findUnique).mockResolvedValue(MED_OK as never); vi.mocked(checkRateLimit).mockResolvedValueOnce({ allowed: false, + limit: 60, remaining: 0, resetAt: Date.now() + 60_000, }); diff --git a/src/app/api/medications/[id]/intake/bulk-delete/__tests__/route.test.ts b/src/app/api/medications/[id]/intake/bulk-delete/__tests__/route.test.ts index 381421305..c087df6f9 100644 --- a/src/app/api/medications/[id]/intake/bulk-delete/__tests__/route.test.ts +++ b/src/app/api/medications/[id]/intake/bulk-delete/__tests__/route.test.ts @@ -39,6 +39,7 @@ vi.mock("@/lib/medications/route-guards", () => ({ vi.mock("@/lib/rate-limit", () => ({ checkRateLimit: vi.fn().mockResolvedValue({ allowed: true, + limit: 60, remaining: 29, resetAt: Date.now() + 60_000, }), @@ -124,6 +125,7 @@ beforeEach(() => { vi.mocked(assertMedicationOwnership).mockResolvedValue(null); vi.mocked(checkRateLimit).mockResolvedValue({ allowed: true, + limit: 60, remaining: 29, resetAt: Date.now() + 60_000, }); @@ -239,6 +241,7 @@ describe("POST /api/medications/[id]/intake/bulk-delete — F-1 H-6 rate limit", it("returns 429 when the per-user cap is exhausted", async () => { vi.mocked(checkRateLimit).mockResolvedValueOnce({ allowed: false, + limit: 60, remaining: 0, resetAt: Date.now() + 60_000, }); diff --git a/src/app/api/medications/[id]/intake/import/__tests__/route.test.ts b/src/app/api/medications/[id]/intake/import/__tests__/route.test.ts index f4c5a0664..a66164c05 100644 --- a/src/app/api/medications/[id]/intake/import/__tests__/route.test.ts +++ b/src/app/api/medications/[id]/intake/import/__tests__/route.test.ts @@ -97,6 +97,7 @@ beforeEach(() => { vi.mocked(auditLog).mockResolvedValue(undefined as never); vi.mocked(checkRateLimit).mockResolvedValue({ allowed: true, + limit: 60, remaining: 60, resetAt: Date.now() + 60_000, }); @@ -407,6 +408,7 @@ describe("POST /api/medications/[id]/intake/import — future bound and limiter" it("refuses with 429 once the import limiter trips", async () => { vi.mocked(checkRateLimit).mockResolvedValue({ allowed: false, + limit: 60, remaining: 0, resetAt: Date.now() + 60_000, }); diff --git a/src/app/api/medications/[id]/inventory/__tests__/route.test.ts b/src/app/api/medications/[id]/inventory/__tests__/route.test.ts index cd9d4251a..5231ae2af 100644 --- a/src/app/api/medications/[id]/inventory/__tests__/route.test.ts +++ b/src/app/api/medications/[id]/inventory/__tests__/route.test.ts @@ -30,6 +30,7 @@ vi.mock("@/lib/auth/audit", () => ({ vi.mock("@/lib/rate-limit", () => ({ checkRateLimit: vi.fn().mockResolvedValue({ allowed: true, + limit: 60, remaining: 29, resetAt: Date.now() + 60_000, }), @@ -87,6 +88,7 @@ beforeEach(() => { ); vi.mocked(checkRateLimit).mockResolvedValue({ allowed: true, + limit: 60, remaining: 29, resetAt: Date.now() + 60_000, }); @@ -253,6 +255,7 @@ describe("POST /api/medications/[id]/inventory", () => { vi.mocked(prisma.medication.findUnique).mockResolvedValue(MED_OK as never); vi.mocked(checkRateLimit).mockResolvedValueOnce({ allowed: false, + limit: 60, remaining: 0, resetAt: Date.now() + 60_000, }); diff --git a/src/app/api/medications/[id]/side-effects/__tests__/route.test.ts b/src/app/api/medications/[id]/side-effects/__tests__/route.test.ts index e30b8372a..ba27f803e 100644 --- a/src/app/api/medications/[id]/side-effects/__tests__/route.test.ts +++ b/src/app/api/medications/[id]/side-effects/__tests__/route.test.ts @@ -31,6 +31,7 @@ vi.mock("@/lib/auth/audit", () => ({ vi.mock("@/lib/rate-limit", () => ({ checkRateLimit: vi.fn().mockResolvedValue({ allowed: true, + limit: 60, remaining: 29, resetAt: Date.now() + 60_000, }), @@ -84,6 +85,7 @@ beforeEach(() => { vi.resetAllMocks(); vi.mocked(checkRateLimit).mockResolvedValue({ allowed: true, + limit: 60, remaining: 29, resetAt: Date.now() + 60_000, }); @@ -298,6 +300,7 @@ describe("POST /api/medications/[id]/side-effects", () => { vi.mocked(prisma.medication.findUnique).mockResolvedValue(MED_OK as never); vi.mocked(checkRateLimit).mockResolvedValueOnce({ allowed: false, + limit: 60, remaining: 0, resetAt: Date.now() + 60_000, }); diff --git a/src/app/api/medications/compliance/__tests__/route.test.ts b/src/app/api/medications/compliance/__tests__/route.test.ts index b947205d6..8490762bb 100644 --- a/src/app/api/medications/compliance/__tests__/route.test.ts +++ b/src/app/api/medications/compliance/__tests__/route.test.ts @@ -30,6 +30,7 @@ vi.mock("@/lib/cache/server-cache", () => ({ vi.mock("@/lib/rate-limit", () => ({ checkRateLimit: vi.fn().mockResolvedValue({ allowed: true, + limit: 60, remaining: 29, resetAt: Date.now() + 60_000, }), @@ -106,6 +107,7 @@ beforeEach(() => { ); vi.mocked(checkRateLimit).mockResolvedValue({ allowed: true, + limit: 60, remaining: 29, resetAt: Date.now() + 60_000, }); @@ -178,6 +180,7 @@ describe("GET /api/medications/compliance", () => { it("returns 429 when the per-user rate limit is exhausted", async () => { vi.mocked(checkRateLimit).mockResolvedValue({ allowed: false, + limit: 60, remaining: 0, resetAt: Date.now() + 60_000, }); diff --git a/src/app/api/medications/extract/__tests__/route.test.ts b/src/app/api/medications/extract/__tests__/route.test.ts index b80464eb3..c01d578ee 100644 --- a/src/app/api/medications/extract/__tests__/route.test.ts +++ b/src/app/api/medications/extract/__tests__/route.test.ts @@ -110,6 +110,7 @@ beforeEach(() => { vi.clearAllMocks(); vi.mocked(checkRateLimit).mockResolvedValue({ allowed: true, + limit: 60, remaining: 9, resetAt: Date.now() + 60_000, }); @@ -152,6 +153,7 @@ describe("POST /api/medications/extract — rate limit", () => { vi.mocked(requireAuth).mockResolvedValue(AUTH_OK as never); vi.mocked(checkRateLimit).mockResolvedValueOnce({ allowed: false, + limit: 60, remaining: 0, resetAt: Date.now() + 60_000, }); diff --git a/src/app/api/mental-health/assessments/__tests__/route.test.ts b/src/app/api/mental-health/assessments/__tests__/route.test.ts index 1d780f051..1d17b58a0 100644 --- a/src/app/api/mental-health/assessments/__tests__/route.test.ts +++ b/src/app/api/mental-health/assessments/__tests__/route.test.ts @@ -96,6 +96,7 @@ beforeEach(() => { vi.mocked(requireModuleEnabled).mockResolvedValue({ enabled: true }); vi.mocked(checkRateLimit).mockResolvedValue({ allowed: true, + limit: 60, remaining: 29, resetAt: Date.now(), }); diff --git a/src/app/api/nutrients/water/__tests__/route.test.ts b/src/app/api/nutrients/water/__tests__/route.test.ts index 051a16d14..354ce7cc2 100644 --- a/src/app/api/nutrients/water/__tests__/route.test.ts +++ b/src/app/api/nutrients/water/__tests__/route.test.ts @@ -101,6 +101,7 @@ beforeEach(() => { vi.mocked(requireModuleEnabled).mockResolvedValue({ enabled: true } as never); vi.mocked(checkRateLimit).mockResolvedValue({ allowed: true, + limit: 60, remaining: 59, resetAt: Date.now() + 60_000, }); @@ -137,6 +138,7 @@ describe("POST /api/nutrients/water", () => { it("returns 429 when the rate limit is exceeded", async () => { vi.mocked(checkRateLimit).mockResolvedValue({ allowed: false, + limit: 60, remaining: 0, resetAt: Date.now() + 60_000, }); diff --git a/src/app/api/onboarding/__tests__/step.test.ts b/src/app/api/onboarding/__tests__/step.test.ts index 1700626a3..5856891f4 100644 --- a/src/app/api/onboarding/__tests__/step.test.ts +++ b/src/app/api/onboarding/__tests__/step.test.ts @@ -36,6 +36,7 @@ vi.mock("@/lib/auth/audit", () => ({ vi.mock("@/lib/rate-limit", () => ({ checkRateLimit: vi.fn().mockResolvedValue({ allowed: true, + limit: 60, remaining: 29, resetAt: Date.now() + 60_000, }), @@ -79,6 +80,7 @@ beforeEach(() => { vi.resetAllMocks(); vi.mocked(checkRateLimit).mockResolvedValue({ allowed: true, + limit: 60, remaining: 29, resetAt: Date.now() + 60_000, }); @@ -243,6 +245,7 @@ describe("POST /api/onboarding/step — rate limit", () => { vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); vi.mocked(checkRateLimit).mockResolvedValueOnce({ allowed: false, + limit: 60, remaining: 0, resetAt: Date.now() + 60_000, }); diff --git a/src/app/api/user/avatar/__tests__/route.test.ts b/src/app/api/user/avatar/__tests__/route.test.ts index 0cfb500ee..72e005dce 100644 --- a/src/app/api/user/avatar/__tests__/route.test.ts +++ b/src/app/api/user/avatar/__tests__/route.test.ts @@ -24,6 +24,7 @@ vi.mock("@/lib/db-compat", () => ({ vi.mock("@/lib/rate-limit", () => ({ checkRateLimit: vi.fn().mockResolvedValue({ allowed: true, + limit: 60, remaining: 9, resetAt: Date.now() + 3_600_000, }), @@ -89,6 +90,7 @@ beforeEach(() => { // Default rate-limit pass-through; per-test overrides drop in below. vi.mocked(checkRateLimit).mockResolvedValue({ allowed: true, + limit: 60, remaining: 9, resetAt: Date.now() + 3_600_000, }); @@ -210,6 +212,7 @@ describe("POST /api/user/avatar", () => { vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); vi.mocked(checkRateLimit).mockResolvedValueOnce({ allowed: false, + limit: 60, remaining: 0, resetAt: Date.now() + 30_000, }); diff --git a/src/app/api/workouts/__tests__/batch-create.test.ts b/src/app/api/workouts/__tests__/batch-create.test.ts index 05a5dba93..37e971e2a 100644 --- a/src/app/api/workouts/__tests__/batch-create.test.ts +++ b/src/app/api/workouts/__tests__/batch-create.test.ts @@ -145,6 +145,7 @@ beforeEach(() => { vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); vi.mocked(checkRateLimit).mockResolvedValue({ allowed: true, + limit: 60, remaining: 60, resetAt: Date.now() + 60_000, }); @@ -218,6 +219,7 @@ describe("POST /api/workouts/batch — auth + size + rate-limit", () => { it("returns 429 when the rate-limit gate blocks the call", async () => { vi.mocked(checkRateLimit).mockResolvedValue({ allowed: false, + limit: 60, remaining: 0, resetAt: Date.now() + 60_000, }); diff --git a/src/app/mcp/__tests__/route.test.ts b/src/app/mcp/__tests__/route.test.ts index 465939dc1..3780faff5 100644 --- a/src/app/mcp/__tests__/route.test.ts +++ b/src/app/mcp/__tests__/route.test.ts @@ -156,6 +156,7 @@ beforeEach(() => { vi.mocked(isApiGloballyEnabled).mockResolvedValue(true); vi.mocked(checkMcpRateLimit).mockResolvedValue({ allowed: true, + limit: 60, remaining: 119, resetAt: Date.now() + 60_000, }); @@ -352,6 +353,7 @@ describe("/mcp — rate limit", () => { validToken(); vi.mocked(checkMcpRateLimit).mockResolvedValue({ allowed: false, + limit: 60, remaining: 0, resetAt: Date.now() + 30_000, }); diff --git a/src/app/mcp/route.ts b/src/app/mcp/route.ts index ae4a7dceb..c71635dc8 100644 --- a/src/app/mcp/route.ts +++ b/src/app/mcp/route.ts @@ -55,6 +55,7 @@ import { annotate } from "@/lib/logging/context"; import { isModuleEnabled } from "@/lib/modules/gate"; import { isApiGloballyEnabled } from "@/lib/app-settings"; import { checkMcpRateLimit, rateLimitHeaders } from "@/lib/rate-limit"; +import type { RateLimitSnapshot } from "@/lib/rate-limit-context"; import { isMcpOriginConfigured, resolveBaseOrigin, @@ -114,11 +115,9 @@ function moduleDisabled(): Response { } /** 429 with the standard rate-limit headers. */ -function tooManyRequests(resetAt: number, remaining: number): Response { +function tooManyRequests(rl: RateLimitSnapshot): Response { const response = Response.json({ error: "rate_limited" }, { status: 429 }); - for (const [k, v] of Object.entries( - rateLimitHeaders({ allowed: false, remaining, resetAt }), - )) { + for (const [k, v] of Object.entries(rateLimitHeaders(rl))) { response.headers.set(k, v); } return response; @@ -179,7 +178,7 @@ async function handleMcp(request: Request): Promise { const rl = await checkMcpRateLimit(ctx.binding); if (!rl.allowed) { annotate({ action: { name: "mcp.rate_limited" } }); - return tooManyRequests(rl.resetAt, rl.remaining); + return tooManyRequests(rl); } // 3. MODULE GATE — off by default; hide the surface entirely when off. diff --git a/src/lib/__tests__/rate-limit-headers.test.ts b/src/lib/__tests__/rate-limit-headers.test.ts new file mode 100644 index 000000000..6aace7a66 --- /dev/null +++ b/src/lib/__tests__/rate-limit-headers.test.ts @@ -0,0 +1,141 @@ +/** + * The rate-limit response headers and the request-scoped verdict capture. + * + * The headers are what a refused client backs off against, so the shape is + * pinned here rather than left to whichever route happens to be exercised: + * `Retry-After` in whole seconds rounding UP, the cap and the remainder as + * 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. + */ +import { describe, expect, it } from "vitest"; + +import { + capturedRateLimit, + captureRateLimitResult, + rateLimitResponseHeaders, + runWithRateLimitCapture, +} from "@/lib/rate-limit-context"; + +describe("rateLimitResponseHeaders", () => { + it("emits Retry-After, the cap, the remainder and the reset instant", () => { + const now = Date.UTC(2026, 0, 1, 12, 0, 0); + const headers = rateLimitResponseHeaders( + { limit: 60, remaining: 0, resetAt: now + 30_000 }, + now, + ); + + expect(headers).toEqual({ + "Retry-After": "30", + "X-RateLimit-Limit": "60", + "X-RateLimit-Remaining": "0", + "X-RateLimit-Reset": new Date(now + 30_000).toISOString(), + }); + }); + + it("rounds Retry-After up so the named delay outlasts the window", () => { + const now = Date.UTC(2026, 0, 1, 12, 0, 0); + const headers = rateLimitResponseHeaders( + { limit: 10, remaining: 0, resetAt: now + 1 }, + now, + ); + // One millisecond left still means "wait a second", never "retry now" — + // a floored zero would send the client straight back into the same window. + expect(headers["Retry-After"]).toBe("1"); + }); + + it("never names a delay in the past", () => { + const now = Date.UTC(2026, 0, 1, 12, 0, 0); + const headers = rateLimitResponseHeaders( + { limit: 10, remaining: 0, resetAt: now - 5_000 }, + now, + ); + expect(headers["Retry-After"]).toBe("0"); + }); + + it("keeps X-RateLimit-Reset in the ISO form already on the wire", () => { + const resetAt = Date.UTC(2026, 4, 17, 8, 30, 0); + const headers = rateLimitResponseHeaders({ + limit: 120, + remaining: 7, + resetAt, + }); + expect(headers["X-RateLimit-Reset"]).toBe("2026-05-17T08:30:00.000Z"); + }); +}); + +describe("request-scoped rate-limit capture", () => { + it("reads null outside a request scope", () => { + captureRateLimitResult({ + allowed: false, + limit: 5, + remaining: 0, + resetAt: Date.now(), + }); + expect(capturedRateLimit()).toBeNull(); + }); + + it("reports the last verdict when every bucket passed", () => { + runWithRateLimitCapture(() => { + captureRateLimitResult({ + allowed: true, + limit: 5, + remaining: 4, + resetAt: 1_000, + }); + captureRateLimitResult({ + allowed: true, + limit: 9, + remaining: 8, + resetAt: 2_000, + }); + expect(capturedRateLimit()).toEqual({ + allowed: true, + limit: 9, + remaining: 8, + resetAt: 2_000, + }); + }); + }); + + it("reports the bucket that refused, not one that passed after it", () => { + runWithRateLimitCapture(() => { + captureRateLimitResult({ + allowed: false, + limit: 60, + remaining: 0, + resetAt: 5_000, + }); + captureRateLimitResult({ + allowed: true, + limit: 120, + remaining: 100, + resetAt: 9_000, + }); + expect(capturedRateLimit()).toMatchObject({ limit: 60, resetAt: 5_000 }); + }); + }); + + it("gives each scope its own cell", async () => { + const first = runWithRateLimitCapture(async () => { + captureRateLimitResult({ + allowed: false, + limit: 1, + remaining: 0, + resetAt: 111, + }); + await Promise.resolve(); + return capturedRateLimit(); + }); + const second = runWithRateLimitCapture(async () => { + await Promise.resolve(); + return capturedRateLimit(); + }); + + expect((await first)?.resetAt).toBe(111); + expect(await second).toBeNull(); + }); +}); diff --git a/src/lib/api-handler.ts b/src/lib/api-handler.ts index 63f6cdf28..3dc8fb7c7 100644 --- a/src/lib/api-handler.ts +++ b/src/lib/api-handler.ts @@ -41,6 +41,11 @@ import { assertRecordSessionFence, attachRecordContextEcho, } from "./sharing/record-session-fence"; +import { + capturedRateLimit, + rateLimitResponseHeaders, + runWithRateLimitCapture, +} from "./rate-limit-context"; /** * HTTP methods a read-only credential may use on the REST surface. A request @@ -221,133 +226,163 @@ export function apiHandler Promise>( undefined, }); - return eventStorage.run(evt, async () => { - let response: Response | undefined; - try { - response = await handler(...args); - } catch (error) { - if (error instanceof AssistantDisabledError) { - // v1.4.31 — operator has disabled the assistant surface. - // The 403 + `errorCode: "assistant.disabled."` - // envelope is locked per - // `.planning/RESPONSE-TO-IOS-TEAM-2026-05-16.md` §3 R5. - // Older iOS clients that don't know the errorCode surface - // this as a generic 403; v1.4.31+ clients can branch on the - // errorCode to render an inline operator-disabled notice. - evt.setError(error); - response = NextResponse.json( - { - data: null, - error: error.message, - meta: { errorCode: error.errorCode }, - }, - { status: 403 }, - ); - } else if (error instanceof ConsentRequiredError) { - // v1.12.1 — server-side consent gate before external-LLM PHI - // egress on the operator's server-managed key. Mirrors the - // AssistantDisabledError envelope (403 + meta.errorCode) so the - // iOS client renders an inline "grant consent" notice instead of - // a generic failure. - evt.setError(error); - response = NextResponse.json( - { - data: null, - error: error.message, - meta: { errorCode: error.errorCode }, - }, - { status: 403 }, - ); - } else if (error instanceof StepUpRequiredError) { - // v1.23 — step-up gate not satisfied. Same 401 + meta.errorCode - // envelope shape as the assistant/consent gates so the client can - // branch on the stable code and launch a re-verification flow. - // Checked before the generic HttpError branch because - // StepUpRequiredError extends it. - evt.setError(error); - response = NextResponse.json( - { - data: null, - error: error.message, - meta: { errorCode: error.errorCode }, - }, - { status: error.statusCode }, - ); - } else if (error instanceof SharingAuthError) { - // v1.36.0 — the acting-account resolver refused. Same - // 403 + meta.errorCode envelope as the gates above, so a client - // distinguishes "you may not act as that account" from "this - // endpoint does not support acting on one" without parsing prose. - // Checked before the generic HttpError branch because it extends it. - evt.setError(error); - response = NextResponse.json( - { - data: null, - error: error.message, - meta: { errorCode: error.errorCode }, - }, - { status: error.statusCode }, - ); - } else if (error instanceof HttpError) { - evt.setError(error); - response = NextResponse.json( - { data: null, error: error.message }, - { status: error.statusCode }, - ); - } else if (error instanceof SyntaxError) { - evt.setError(error); - response = NextResponse.json( - { data: null, error: "Invalid JSON body" }, - { status: 400 }, - ); - } else { - evt.setError(error); - // Report to GlitchTip (fire-and-forget) - reportToGlitchtip(error, request, evt).catch(() => {}); - response = NextResponse.json( - { data: null, error: "Interner Serverfehler" }, - { status: 500 }, - ); - } - } finally { - const status = (response as Response | undefined)?.status ?? 500; - evt.finish(status); + return runWithRateLimitCapture(() => + eventStorage.run(evt, async () => { + let response: Response | undefined; try { - emitIfSampled(evt.toJSON()); - } catch { - /* logging must never crash the handler */ + response = await handler(...args); + } catch (error) { + if (error instanceof AssistantDisabledError) { + // v1.4.31 — operator has disabled the assistant surface. + // The 403 + `errorCode: "assistant.disabled."` + // envelope is locked per + // `.planning/RESPONSE-TO-IOS-TEAM-2026-05-16.md` §3 R5. + // Older iOS clients that don't know the errorCode surface + // this as a generic 403; v1.4.31+ clients can branch on the + // errorCode to render an inline operator-disabled notice. + evt.setError(error); + response = NextResponse.json( + { + data: null, + error: error.message, + meta: { errorCode: error.errorCode }, + }, + { status: 403 }, + ); + } else if (error instanceof ConsentRequiredError) { + // v1.12.1 — server-side consent gate before external-LLM PHI + // egress on the operator's server-managed key. Mirrors the + // AssistantDisabledError envelope (403 + meta.errorCode) so the + // iOS client renders an inline "grant consent" notice instead of + // a generic failure. + evt.setError(error); + response = NextResponse.json( + { + data: null, + error: error.message, + meta: { errorCode: error.errorCode }, + }, + { status: 403 }, + ); + } else if (error instanceof StepUpRequiredError) { + // v1.23 — step-up gate not satisfied. Same 401 + meta.errorCode + // envelope shape as the assistant/consent gates so the client can + // branch on the stable code and launch a re-verification flow. + // Checked before the generic HttpError branch because + // StepUpRequiredError extends it. + evt.setError(error); + response = NextResponse.json( + { + data: null, + error: error.message, + meta: { errorCode: error.errorCode }, + }, + { status: error.statusCode }, + ); + } else if (error instanceof SharingAuthError) { + // v1.36.0 — the acting-account resolver refused. Same + // 403 + meta.errorCode envelope as the gates above, so a client + // distinguishes "you may not act as that account" from "this + // endpoint does not support acting on one" without parsing prose. + // Checked before the generic HttpError branch because it extends it. + evt.setError(error); + response = NextResponse.json( + { + data: null, + error: error.message, + meta: { errorCode: error.errorCode }, + }, + { status: error.statusCode }, + ); + } else if (error instanceof HttpError) { + evt.setError(error); + response = NextResponse.json( + { data: null, error: error.message }, + { status: error.statusCode }, + ); + } else if (error instanceof SyntaxError) { + evt.setError(error); + response = NextResponse.json( + { data: null, error: "Invalid JSON body" }, + { status: 400 }, + ); + } else { + evt.setError(error); + // Report to GlitchTip (fire-and-forget) + reportToGlitchtip(error, request, evt).catch(() => {}); + response = NextResponse.json( + { data: null, error: "Interner Serverfehler" }, + { status: 500 }, + ); + } + } finally { + const status = (response as Response | undefined)?.status ?? 500; + evt.finish(status); + try { + emitIfSampled(evt.toJSON()); + } catch { + /* logging must never crash the handler */ + } } - } - const nr = response as NextResponse; - nr.headers.set("x-request-id", evt.getRequestId()); - // v1.37.0 — echo the record context this response was actually served - // under, when one was decided. The value comes from the wide event and - // from nowhere else: the fence stamps it on every call it makes, so - // exactly the responses that resolved a record scope carry it and - // everything else — public routes, actor surfaces, admin, `/me`, static — - // carries nothing. - // - // Deliberately NOT derived here with a `getSession()`. That would put a - // session read on every public route, and it would make the echo a second - // derivation of the context rather than a report of the one that was - // used, which is the difference between "what this response served" and - // "what a later read thinks is true". - // - // Absence is therefore meaningful and safe: the client discards a - // response whose echo CONTRADICTS its adopted context, and serves one - // that carries no echo normally. A discard-on-absence rule would throw - // away the `/api/version` poll on every cycle. - // - // The two header names stay inside the fence module, which is what keeps - // them to four files overall (declared, read, attached client-side, - // published) — see `src/__tests__/record-session-fence-guard.test.ts`. - attachRecordContextEcho(nr.headers, evt.getRecordContext()); - return nr; - }); + const nr = response as NextResponse; + nr.headers.set("x-request-id", evt.getRequestId()); + // v1.37.0 — echo the record context this response was actually served + // under, when one was decided. The value comes from the wide event and + // from nowhere else: the fence stamps it on every call it makes, so + // exactly the responses that resolved a record scope carry it and + // everything else — public routes, actor surfaces, admin, `/me`, static — + // carries nothing. + // + // Deliberately NOT derived here with a `getSession()`. That would put a + // session read on every public route, and it would make the echo a second + // derivation of the context rather than a report of the one that was + // used, which is the difference between "what this response served" and + // "what a later read thinks is true". + // + // Absence is therefore meaningful and safe: the client discards a + // response whose echo CONTRADICTS its adopted context, and serves one + // that carries no echo normally. A discard-on-absence rule would throw + // away the `/api/version` poll on every cycle. + // + // The two header names stay inside the fence module, which is what keeps + // them to four files overall (declared, read, attached client-side, + // published) — see `src/__tests__/record-session-fence-guard.test.ts`. + attachRecordContextEcho(nr.headers, evt.getRecordContext()); + attachRateLimitHeaders(nr); + return nr; + }), + ); }; return wrapped as T; } +/** + * Put the rate-limit headers on a refusal that left the handler without them. + * + * Only ~70 of the 199 route files that consult a limiter attach + * `rateLimitHeaders(rl)` by hand; the rest answer 429 with a bare prose + * envelope, so an offline replay queue that trips the batch ceiling has + * nothing to back off against but a guess. The limiter records its verdict in + * the request-scoped cell, so the headers can be derived here for every one of + * them at once instead of being threaded through each call site — and a route + * added tomorrow gets them without knowing this exists. + * + * `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. + */ +function attachRateLimitHeaders(response: NextResponse): void { + if (response.status !== 429) return; + if (response.headers.has("Retry-After")) return; + const snapshot = capturedRateLimit(); + if (!snapshot) return; + for (const [name, value] of Object.entries( + rateLimitResponseHeaders(snapshot), + )) { + response.headers.set(name, value); + } +} + /** * Authenticated request context. Returned by both session-cookie and Bearer-token * authentication paths. The `session.id` is the session-record id for cookie auth diff --git a/src/lib/documents/__tests__/ai-route-support-limit.test.ts b/src/lib/documents/__tests__/ai-route-support-limit.test.ts index d2a4b1110..f47184ce6 100644 --- a/src/lib/documents/__tests__/ai-route-support-limit.test.ts +++ b/src/lib/documents/__tests__/ai-route-support-limit.test.ts @@ -77,6 +77,7 @@ describe("documentAiRateLimited", () => { const resetAt = Date.now() + 42 * 60_000; const response = documentAiRateLimited({ allowed: false, + limit: 60, remaining: 0, resetAt, }); diff --git a/src/lib/openapi/routes/shared.ts b/src/lib/openapi/routes/shared.ts index 4ce94c266..dad0f7624 100644 --- a/src/lib/openapi/routes/shared.ts +++ b/src/lib/openapi/routes/shared.ts @@ -181,7 +181,8 @@ export const stdResponses = { content: { "application/json": { schema: errorEnvelope } }, }, "429": { - description: "Rate limit exceeded.", + description: + "Rate limit exceeded. The response carries `Retry-After` (whole seconds, rounded up — wait at least that long before retrying), `X-RateLimit-Limit` (the bucket's cap), `X-RateLimit-Remaining` and `X-RateLimit-Reset` (the reset instant, ISO-8601, not epoch seconds). Back off on `Retry-After` rather than guessing.", content: { "application/json": { schema: errorEnvelope } }, }, }; diff --git a/src/lib/rate-limit-context.ts b/src/lib/rate-limit-context.ts new file mode 100644 index 000000000..84293d0dc --- /dev/null +++ b/src/lib/rate-limit-context.ts @@ -0,0 +1,97 @@ +/** + * Request-scoped capture of the rate-limiter's last verdict, 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 + * tests replace `@/lib/rate-limit` wholesale with a `vi.mock` factory, so + * anything `apiHandler` needs from the limiter has to come from a module those + * factories do not stand in for; otherwise the wrapper calls `undefined()` the + * moment a mocked route is exercised. + * + * Why the capture exists at all: a route that refuses a request answers 429 + * from its own handler body, and most of them build that response with a bare + * `apiError(...)`. Threading the headers through every one of those call sites + * is 129 edits that the next new route immediately forgets. Instead the + * limiter records its verdict here, `apiHandler` runs every handler inside the + * store, and any 429 that leaves without headers gets them on the way out. A + * route that already attaches them keeps exactly the response it built. + */ +import { AsyncLocalStorage } from "node:async_hooks"; + +/** What a rate-limit verdict has to carry to be renderable as headers. */ +export interface RateLimitSnapshot { + /** Requests allowed per window. */ + limit: number; + /** Requests left in the current window; zero once the cap is hit. */ + remaining: number; + /** Epoch milliseconds at which the current window rolls over. */ + resetAt: number; +} + +interface RateLimitCapture { + last: RateLimitSnapshot | null; + denied: RateLimitSnapshot | null; +} + +const storage = new AsyncLocalStorage(); + +/** + * Run `fn` with a fresh capture cell. Called once per request by + * `apiHandler`; outside one, every capture below is a no-op and + * {@link capturedRateLimit} reads null. + */ +export function runWithRateLimitCapture(fn: () => T): T { + return storage.run({ last: null, 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. + */ +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. */ +export function capturedRateLimit(): RateLimitSnapshot | null { + const store = storage.getStore(); + if (!store) return null; + return store.denied ?? store.last; +} + +/** + * The rate-limit response headers. + * + * `Retry-After` is whole seconds and rounds UP, per RFC 9110 §10.2.3: a client + * that waits the value it is given must find the window already rolled over, + * which flooring cannot promise. It never goes below zero — a window that + * expired between the verdict and the response says "retry now" rather than + * naming a time in the past. + * + * `X-RateLimit-Reset` keeps the ISO-8601 instant it has always carried. It is + * the wrong convention for the header name and every other deployment uses + * epoch or delta seconds, but it is on the wire today and changing it is a + * contract decision of its own; `Retry-After` gives an integer-parsing client + * the number it needs without touching the existing field. + */ +export function rateLimitResponseHeaders( + snapshot: RateLimitSnapshot, + now: number = Date.now(), +): Record { + return { + "Retry-After": String( + Math.max(0, Math.ceil((snapshot.resetAt - now) / 1000)), + ), + "X-RateLimit-Limit": String(snapshot.limit), + "X-RateLimit-Remaining": String(snapshot.remaining), + "X-RateLimit-Reset": new Date(snapshot.resetAt).toISOString(), + }; +} diff --git a/src/lib/rate-limit.ts b/src/lib/rate-limit.ts index b8dccc0a5..d76113579 100644 --- a/src/lib/rate-limit.ts +++ b/src/lib/rate-limit.ts @@ -5,11 +5,14 @@ import { prisma } from "@/lib/db"; import { getClientIpOrTrustWarning } from "@/lib/api-response"; +import { + captureRateLimitResult, + rateLimitResponseHeaders, + type RateLimitSnapshot, +} from "@/lib/rate-limit-context"; -export interface RateLimitResult { +export interface RateLimitResult extends RateLimitSnapshot { allowed: boolean; - remaining: number; - resetAt: number; } /** @@ -45,11 +48,16 @@ export async function checkRateLimit( const resetAt = row.reset_at.getTime(); const count = Number(row.count); - return { + const result: RateLimitResult = { allowed: count <= limit, + limit, remaining: Math.max(0, limit - count), resetAt, }; + // Hand the verdict to the request-scoped cell so `apiHandler` can dress a + // bare 429 on the way out. A no-op outside a request. + captureRateLimitResult(result); + return result; } /** @@ -77,15 +85,13 @@ export async function refundRateLimit(key: string): Promise { } /** - * Rate limit response headers. + * Rate limit response headers: `Retry-After` plus the `X-RateLimit-*` triple. + * See `rateLimitResponseHeaders` for what each one promises. */ export function rateLimitHeaders( - result: RateLimitResult, + result: RateLimitSnapshot, ): Record { - return { - "X-RateLimit-Remaining": String(result.remaining), - "X-RateLimit-Reset": new Date(result.resetAt).toISOString(), - }; + return rateLimitResponseHeaders(result); } /** diff --git a/tests/integration/rate-limit-429-headers.test.ts b/tests/integration/rate-limit-429-headers.test.ts new file mode 100644 index 000000000..8fbb1becc --- /dev/null +++ b/tests/integration/rate-limit-429-headers.test.ts @@ -0,0 +1,135 @@ +/** + * A 429 from a route that attaches nothing itself still carries the headers. + * + * `POST /api/measurements/batch` is the one 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 + * come from `apiHandler` and the limiter's request-scoped verdict rather than + * from the handler body — which is what makes the other 128 header-less routes + * covered too. + * + * 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. + */ +import { NextRequest } from "next/server"; +import { beforeEach, describe, expect, it, vi } from "vitest"; + +import { cookieJar } from "./mock-next-headers"; +import { getPrismaClient, truncateAllTables } from "./setup"; + +const TEST_USER_ID = "user-rate-limit-headers"; + +vi.mock("next/headers", async () => { + const { cookieJar, headerJar } = await import("./mock-next-headers"); + return { + headers: vi.fn(async () => ({ + get: (name: string) => headerJar.get(name.toLowerCase()) ?? null, + })), + cookies: vi.fn(async () => ({ + get: (name: string) => { + const value = cookieJar.get(name); + return value ? { name, value } : undefined; + }, + set: (name: string, value: string) => { + cookieJar.set(name, value); + }, + delete: (name: string) => { + cookieJar.delete(name); + }, + })), + }; +}); + +vi.mock("@/lib/db-compat", () => ({ + ensureDbCompatibility: vi.fn().mockResolvedValue(undefined), +})); + +beforeEach(async () => { + await truncateAllTables(getPrismaClient()); + cookieJar.clear(); + await getPrismaClient().user.create({ + data: { + id: TEST_USER_ID, + username: "rate-limit-headers", + email: "rate-limit-headers@example.test", + }, + }); + const session = await getPrismaClient().session.create({ + data: { + userId: TEST_USER_ID, + expiresAt: new Date(Date.now() + 60 * 60 * 1000), + }, + }); + cookieJar.set("healthlog_session", session.id); +}); + +function batchRequest(): NextRequest { + return new NextRequest("http://localhost/api/measurements/batch", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ + entries: [ + { + hkIdentifier: "HKQuantityTypeIdentifierBodyMass", + value: 81.4, + unit: "kg", + startDate: "2026-07-20T06:00:00.000Z", + endDate: "2026-07-20T06:00:00.000Z", + externalId: `rl-${Math.random().toString(36).slice(2)}`, + }, + ], + }), + }); +} + +describe("429 rate-limit headers (real Postgres)", () => { + it("carries Retry-After and the X-RateLimit triple on a route that attaches none", async () => { + const { POST } = await import("@/app/api/measurements/batch/route"); + + // Burn the bucket directly rather than posting sixty batches: the route + // and the pre-charge share one key, so the next call through the handler + // is the one that is refused. + const { checkRateLimit } = await import("@/lib/rate-limit"); + const key = `measurements:batch:${TEST_USER_ID}`; + for (let i = 0; i < 60; i += 1) { + await checkRateLimit(key, 60, 60_000); + } + + const before = Date.now(); + const res = await POST(batchRequest() as never); + expect(res.status).toBe(429); + + const retryAfter = res.headers.get("Retry-After"); + expect(retryAfter).toMatch(/^\d+$/); + const seconds = Number(retryAfter); + expect(seconds).toBeGreaterThan(0); + expect(seconds).toBeLessThanOrEqual(60); + + expect(res.headers.get("X-RateLimit-Limit")).toBe("60"); + expect(res.headers.get("X-RateLimit-Remaining")).toBe("0"); + + const reset = res.headers.get("X-RateLimit-Reset"); + expect(reset).toBeTruthy(); + const resetAt = Date.parse(reset as string); + expect(Number.isNaN(resetAt)).toBe(false); + expect(resetAt).toBeGreaterThan(before); + + // The named delay must actually outlast the window it names. + expect(before + seconds * 1000).toBeGreaterThanOrEqual(resetAt - 1_000); + + // The body is unchanged — the headers are additive. + const body = (await res.json()) as { data: null; error: string }; + expect(body.data).toBeNull(); + expect(typeof body.error).toBe("string"); + }); + + it("leaves a successful response without them", async () => { + const { POST } = await import("@/app/api/measurements/batch/route"); + + const res = await POST(batchRequest() as never); + expect(res.status).toBe(200); + expect(res.headers.get("Retry-After")).toBeNull(); + }); +}); From 1d2744a1092516bd34d0dd18a2a5e32876e089e8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Thu, 10 Sep 2026 04:01:14 +0200 Subject: [PATCH 02/13] fix(api): give every offset-paged list a unique tiebreaker MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `/api/measurements` and `/api/labs` sort on one non-unique column and page with `skip`/`take`. When the sort key ties, Postgres does not promise the same order for the query that fetches page N and the one that fetches page N+1, so a client paging its own history can receive a row twice and never receive another — silently, on both sides. Ties are not hypothetical here. The partial unique index forbids two live measurements of the same type at one instant, but the tie that occurs in production is the cross-type one: a daily `stats:` rollup posts steps, distance, flights and active energy at the identical timestamp, and an unfiltered history list pages straight through the block. A lab panel drawn in one sitting shares `takenAt` across every analyte in it. Appending `{ id: }` to the `orderBy` makes the order total. Same fix on the five siblings that page the same way — medication intake history, illness day-logs, custom-metric entries, reminder history, mental-health assessments — because the defect is the shape, not the two routes it was found on. No wire change; `/api/sync/changes` already ordered this way. The fixtures seed ids in an order unrelated to insertion order, so the assertion on order inside the tied block can only hold if the secondary key is doing the work: with the tiebreaker removed the new file fails on all three cases. --- .../api/custom-metrics/[id]/entries/route.ts | 4 +- .../illness/episodes/[id]/day-logs/route.ts | 3 +- src/app/api/labs/route.ts | 6 +- .../[id]/history/route.ts | 3 +- src/app/api/measurements/route.ts | 10 +- src/app/api/medications/[id]/intake/route.ts | 11 +- .../api/mental-health/assessments/route.ts | 4 +- .../list-pagination-tiebreaker.test.ts | 214 ++++++++++++++++++ 8 files changed, 247 insertions(+), 8 deletions(-) create mode 100644 tests/integration/list-pagination-tiebreaker.test.ts diff --git a/src/app/api/custom-metrics/[id]/entries/route.ts b/src/app/api/custom-metrics/[id]/entries/route.ts index 71a54f7e0..5fdd9ff4b 100644 --- a/src/app/api/custom-metrics/[id]/entries/route.ts +++ b/src/app/api/custom-metrics/[id]/entries/route.ts @@ -62,7 +62,9 @@ export const GET = apiHandler( const [rows, total] = await Promise.all([ prisma.customMetricEntry.findMany({ where, - orderBy: { measuredAt: sortDir }, + // Unique tiebreaker — see the measurements list: offset paging over + // a non-unique sort key can repeat and drop rows between pages. + orderBy: [{ measuredAt: sortDir }, { id: sortDir }], take: limit, skip: offset, }), diff --git a/src/app/api/illness/episodes/[id]/day-logs/route.ts b/src/app/api/illness/episodes/[id]/day-logs/route.ts index 9d01977df..2202f338f 100644 --- a/src/app/api/illness/episodes/[id]/day-logs/route.ts +++ b/src/app/api/illness/episodes/[id]/day-logs/route.ts @@ -88,7 +88,8 @@ export const GET = apiHandler( const [rows, total] = await Promise.all([ prisma.illnessDayLog.findMany({ where, - orderBy: { date: sortDir }, + // Unique tiebreaker — `date` is a calendar day and repeats. + orderBy: [{ date: sortDir }, { id: sortDir }], take: limit, skip: offset, include: dayLogSymptomInclude, diff --git a/src/app/api/labs/route.ts b/src/app/api/labs/route.ts index b420b745e..97fc344ce 100644 --- a/src/app/api/labs/route.ts +++ b/src/app/api/labs/route.ts @@ -105,7 +105,11 @@ export const GET = apiHandler(async (request: NextRequest) => { const [rows, total] = await Promise.all([ prisma.labResult.findMany({ where, - orderBy: { takenAt: sortDir }, + // Unique tiebreaker: `takenAt` is a day-resolution timestamp, so a + // panel drawn in one sitting ties across every analyte in it. Offset + // paging over a non-unique sort key can repeat a row on one page and + // drop another, invisibly to both sides. + orderBy: [{ takenAt: sortDir }, { id: sortDir }], take: limit, skip: offset, include: { diff --git a/src/app/api/measurement-reminders/[id]/history/route.ts b/src/app/api/measurement-reminders/[id]/history/route.ts index 19872c4fc..df16f4a93 100644 --- a/src/app/api/measurement-reminders/[id]/history/route.ts +++ b/src/app/api/measurement-reminders/[id]/history/route.ts @@ -45,7 +45,8 @@ export const GET = apiHandler( const [rows, total] = await Promise.all([ prisma.measurementReminderEvent.findMany({ where, - orderBy: { occurredAt: "desc" }, + // Unique tiebreaker — several reminder events can share one instant. + orderBy: [{ occurredAt: "desc" }, { id: "desc" }], take: limit, skip: offset, }), diff --git a/src/app/api/measurements/route.ts b/src/app/api/measurements/route.ts index d8f76ada0..87b22745d 100644 --- a/src/app/api/measurements/route.ts +++ b/src/app/api/measurements/route.ts @@ -544,7 +544,15 @@ export const GET = apiHandler(async (request: NextRequest) => { const [measurements, total] = await Promise.all([ prisma.measurement.findMany({ where, - orderBy: { [sortBy]: sortDir }, + // `{ id: sortDir }` is the tiebreaker, not decoration. `measuredAt` + // is not unique here and ties are the normal case — an Apple Health + // import stamps long runs with one instant, and the + // `stats::YYYY-MM-DD` daily-rollup convention puts every daily + // total at the same one. Without a unique secondary key Postgres is + // free to order the tied block differently between the query for page + // N and the query for page N+1, so a client paging its own history + // silently gets one row twice and never gets another. + orderBy: [{ [sortBy]: sortDir }, { id: sortDir }], take: limit, skip: offset, }), diff --git a/src/app/api/medications/[id]/intake/route.ts b/src/app/api/medications/[id]/intake/route.ts index 6d684b667..635ee69f1 100644 --- a/src/app/api/medications/[id]/intake/route.ts +++ b/src/app/api/medications/[id]/intake/route.ts @@ -729,10 +729,17 @@ export const GET = apiHandler( // the history view. Pinning them last keeps the descending order // reading today → yesterday → … with real timestamps first. Other // sort columns are non-null so they keep the simple shape. + // The trailing `{ id: sortDir }` is the unique tiebreaker. Scheduled + // slots share an instant by construction — a twice-daily medication puts + // every morning dose on the same `scheduledFor` — so offset paging over + // the primary key alone can repeat a row on one page and drop another. const orderBy = sortBy === "takenAt" - ? { takenAt: { sort: sortDir, nulls: "last" as const } } - : { [sortBy]: sortDir }; + ? [ + { takenAt: { sort: sortDir, nulls: "last" as const } }, + { id: sortDir }, + ] + : [{ [sortBy]: sortDir }, { id: sortDir }]; const [events, total] = await Promise.all([ prisma.medicationIntakeEvent.findMany({ diff --git a/src/app/api/mental-health/assessments/route.ts b/src/app/api/mental-health/assessments/route.ts index a37b223b3..4229ecfd3 100644 --- a/src/app/api/mental-health/assessments/route.ts +++ b/src/app/api/mental-health/assessments/route.ts @@ -123,7 +123,9 @@ export const GET = apiHandler(async (request: NextRequest) => { deletedAt: null, ...(instrument ? { instrument } : {}), }, - orderBy: { takenAt: "desc" }, + // Unique tiebreaker — `takenAt` is day-resolution and two instruments + // taken in one sitting tie. + orderBy: [{ takenAt: "desc" }, { id: "desc" }], take: limit ?? 100, skip: offset ?? 0, select: { diff --git a/tests/integration/list-pagination-tiebreaker.test.ts b/tests/integration/list-pagination-tiebreaker.test.ts new file mode 100644 index 000000000..171aa9aad --- /dev/null +++ b/tests/integration/list-pagination-tiebreaker.test.ts @@ -0,0 +1,214 @@ +/** + * Offset pagination over a tied sort key must not repeat or drop a row. + * + * `/api/measurements` and `/api/labs` page with `skip`/`take` over a single + * non-unique column. Ties are the normal case on both: an Apple Health import + * stamps long runs of samples with one instant and the documented + * `stats::YYYY-MM-DD` rollup convention puts every daily total at + * the same one, while a lab panel drawn in one sitting shares `takenAt` across + * every analyte in it. Without a unique secondary key Postgres may order the + * tied block differently for the query that fetches page N and the one that + * fetches page N+1, so a client mirroring its own history silently receives + * one row twice and never receives another. For a health record that is a + * data-integrity bug, and it is invisible to both sides. + * + * The fixtures are seeded with explicit ids in a deliberately scrambled order, + * so heap order and id order disagree. That is what makes this a real gate: + * asserting only that two pages are disjoint would pass on an unordered scan + * that happened to be stable, whereas the id ordering inside a tied block can + * only hold if the secondary sort key is actually there. + */ +import { NextRequest } from "next/server"; +import { beforeEach, describe, expect, it, vi } from "vitest"; + +import { cookieJar } from "./mock-next-headers"; +import { getPrismaClient, truncateAllTables } from "./setup"; + +const TEST_USER_ID = "user-pagination-tiebreaker"; +const TIED_AT = new Date("2026-03-04T09:00:00.000Z"); + +vi.mock("next/headers", async () => { + const { cookieJar, headerJar } = await import("./mock-next-headers"); + return { + headers: vi.fn(async () => ({ + get: (name: string) => headerJar.get(name.toLowerCase()) ?? null, + })), + cookies: vi.fn(async () => ({ + get: (name: string) => { + const value = cookieJar.get(name); + return value ? { name, value } : undefined; + }, + set: (name: string, value: string) => { + cookieJar.set(name, value); + }, + delete: (name: string) => { + cookieJar.delete(name); + }, + })), + }; +}); + +vi.mock("@/lib/db-compat", () => ({ + ensureDbCompatibility: vi.fn().mockResolvedValue(undefined), +})); + +/** + * Insertion order deliberately unrelated to id order: 04, 09, 01, 06, 00, … + * A scan that ignores the secondary key returns this sequence; one that + * honours it returns 09, 08, 07, … on a descending sort. + */ +const SCRAMBLED = [4, 9, 1, 6, 0, 8, 3, 7, 2, 5]; + +/** + * One type per row. The partial unique index on + * `(user_id, type, measured_at, source, sleep_stage)` forbids two live rows of + * the SAME type at one instant, so the tie that actually occurs in production + * is the cross-type one: a daily `stats:` rollup posts steps, distance, + * flights, active energy and the rest at the identical timestamp, and an + * unfiltered history list pages straight through the block. + */ +const TYPES = [ + "ACTIVITY_STEPS", + "FLIGHTS_CLIMBED", + "WALKING_RUNNING_DISTANCE", + "ACTIVE_ENERGY_BURNED", + "RESTING_HEART_RATE", + "HEART_RATE_VARIABILITY", + "OXYGEN_SATURATION", + "RESPIRATORY_RATE", + "BODY_TEMPERATURE", + "WEIGHT", +] as const; + +beforeEach(async () => { + const prisma = getPrismaClient(); + await truncateAllTables(prisma); + cookieJar.clear(); + await prisma.user.create({ + data: { + id: TEST_USER_ID, + username: "pagination-tiebreaker", + email: "pagination-tiebreaker@example.test", + }, + }); + const session = await prisma.session.create({ + data: { + userId: TEST_USER_ID, + expiresAt: new Date(Date.now() + 60 * 60 * 1000), + }, + }); + cookieJar.set("healthlog_session", session.id); + + // Every row shares one `measuredAt` / `takenAt` — the whole page is one + // tied block, which is the worst case and the realistic one. + for (const n of SCRAMBLED) { + const suffix = String(n).padStart(2, "0"); + await prisma.measurement.create({ + data: { + id: `tie-measurement-${suffix}`, + userId: TEST_USER_ID, + type: TYPES[n], + value: 80 + n, + unit: "count", + measuredAt: TIED_AT, + source: "APPLE_HEALTH", + }, + }); + await prisma.labResult.create({ + data: { + id: `tie-lab-${suffix}`, + userId: TEST_USER_ID, + analyte: `Analyte ${suffix}`, + value: 1 + n, + unit: "mg/dl", + takenAt: TIED_AT, + }, + }); + } +}); + +function get(path: string): NextRequest { + return new NextRequest(`http://localhost${path}`); +} + +async function idsOf( + res: Response, + key: "measurements" | "results", +): Promise { + const body = (await res.json()) as { + data: Record>; + }; + return body.data[key].map((row) => row.id); +} + +describe("offset pagination over a tied sort key (real Postgres)", () => { + it("pages /api/measurements with no duplicate and no gap", async () => { + const { GET } = await import("@/app/api/measurements/route"); + + const first = await idsOf( + await GET(get("/api/measurements?limit=4&offset=0") as never), + "measurements", + ); + const second = await idsOf( + await GET(get("/api/measurements?limit=4&offset=4") as never), + "measurements", + ); + const third = await idsOf( + await GET(get("/api/measurements?limit=4&offset=8") as never), + "measurements", + ); + + const seen = [...first, ...second, ...third]; + expect(seen).toHaveLength(10); + expect(new Set(seen).size).toBe(10); + expect([...seen].sort()).toEqual( + SCRAMBLED.map((n) => `tie-measurement-${String(n).padStart(2, "0")}`) + .slice() + .sort(), + ); + + // The default sort is `measuredAt desc`; with every row tied, the + // tiebreaker is the only thing deciding the order, so it must be + // strictly descending by id across the page boundaries. + expect(seen).toEqual([...seen].sort().reverse()); + }); + + it("pages /api/labs with no duplicate and no gap", async () => { + const { GET } = await import("@/app/api/labs/route"); + + const first = await idsOf( + await GET(get("/api/labs?limit=4&offset=0") as never), + "results", + ); + const second = await idsOf( + await GET(get("/api/labs?limit=4&offset=4") as never), + "results", + ); + const third = await idsOf( + await GET(get("/api/labs?limit=4&offset=8") as never), + "results", + ); + + const seen = [...first, ...second, ...third]; + expect(seen).toHaveLength(10); + expect(new Set(seen).size).toBe(10); + expect(seen).toEqual([...seen].sort().reverse()); + }); + + it("keeps the ascending direction stable too", async () => { + const { GET } = await import("@/app/api/measurements/route"); + + const first = await idsOf( + await GET(get("/api/measurements?limit=5&offset=0&sortDir=asc") as never), + "measurements", + ); + const second = await idsOf( + await GET(get("/api/measurements?limit=5&offset=5&sortDir=asc") as never), + "measurements", + ); + + const seen = [...first, ...second]; + expect(new Set(seen).size).toBe(10); + expect(seen).toEqual([...seen].sort()); + }); +}); From fd8fc0ec0e811e0b1e5d18c0ad94fb11b0881b13 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Thu, 10 Sep 2026 04:03:02 +0200 Subject: [PATCH 03/13] docs(api): publish `details.issues` and open `meta` on the error envelope MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every documented 422, 401, 403, 409, 410 and 429 points at one `ErrorEnvelope`, and that schema declared `data`, `error` and a `meta` closed to `requestId` + `errorCode`. Two things were wrong with it. `returnAllZodIssues` has sent a third top-level key since v1.4.42 — `details.issues`, one entry per rejected field — from 166 route files. It exists so a client can fix three bad fields in one round-trip instead of three, and the doc comment says so, yet no component schema anywhere declared it. A client generated from the contract had no field to read it from, so the feature was invisible to exactly the consumer it was built for. It is now a schema, with the `unrecognized_keys` list and the promise that no rejected value is echoed back. And `meta` was closed while real refusals put more in it: `removedIn` / `replacedBy` on a 410, `module` beside `module.disabled`, the per-integration context on a failed connection test. Closed plus those keys means the response was invalid against its own published schema and a strict decoder is entitled to reject it. It is open now. No wire change; both halves describe what the server already sends. --- docs/api/openapi.yaml | 48 +++++++++++++++++++++++-- src/lib/openapi/routes/shared.ts | 62 ++++++++++++++++++++++++++++++-- 2 files changed, 105 insertions(+), 5 deletions(-) diff --git a/docs/api/openapi.yaml b/docs/api/openapi.yaml index 580ddaf59..71a91a954 100644 --- a/docs/api/openapi.yaml +++ b/docs/api/openapi.yaml @@ -29018,19 +29018,63 @@ components: type: "null" error: type: string + details: + description: "Present on a 422 raised by the multi-issue validator: every field the request got wrong, not only the + first. Absent on every other status." + type: object + properties: + issues: + type: array + items: + $ref: "#/components/schemas/ValidationIssue" + required: + - issues + additionalProperties: false meta: type: object properties: requestId: type: string errorCode: + description: Stable machine code for this refusal. Branch on it rather than on `error`, which is prose and may be + reworded. type: string - additionalProperties: false + additionalProperties: {} required: - data - error additionalProperties: false - description: "Standard error response: data is null, error is human prose." + description: "Standard error response: `data` is null, `error` is human prose. `meta.errorCode` carries the stable + machine code where one exists, and `meta` may carry further per-refusal context. A 422 from the multi-issue + validator additionally carries `details.issues` — every field the request got wrong, so a client can fix them in + one round-trip." + ValidationIssue: + type: object + properties: + path: + type: string + description: Dot-joined path to the rejected field, e.g. `entries.3.measuredAt`. Empty string when the whole body was + rejected. + code: + type: string + description: Zod issue code, e.g. `invalid_type`, `too_small`, `unrecognized_keys`. Treat an unfamiliar code as a + generic rejection of `path`. + message: + type: string + description: English sentence describing the rejection. Safe to log; show the user your own wording keyed on `path`. + keys: + description: "Only on `unrecognized_keys`: the refused key names, bounded to 10 and 64 characters each. Zod reports + every unknown key of one object in a single issue whose `path` is the object, so the names live here and + nowhere in `path`." + type: array + items: + type: string + required: + - path + - code + - message + additionalProperties: false + description: "One rejected field in a multi-issue 422. Sanitised: no rejected VALUE is echoed back." CycleCustomSymptomCreatedEnvelope: type: object properties: diff --git a/src/lib/openapi/routes/shared.ts b/src/lib/openapi/routes/shared.ts index dad0f7624..f75a366a9 100644 --- a/src/lib/openapi/routes/shared.ts +++ b/src/lib/openapi/routes/shared.ts @@ -21,20 +21,76 @@ import { createBatchWorkoutSchema as createBatchWorkoutSchemaBase } from "@/lib/ * `{ data, error, meta? }`. The OpenAPI surface mirrors that contract * so iOS / external-ingest clients can decode uniformly. */ +/** + * One rejected field from a multi-issue 422. + * + * `returnAllZodIssues` has emitted this since v1.4.42 so a client can fix + * three bad fields in one round-trip instead of three, and 166 route files + * send it — but no component schema declared it, so the very consumer it was + * built for could not read it from the contract. `params` never ships: some + * Zod codes embed the offending value in it and that is user content. + */ +const validationIssue = z + .object({ + path: z + .string() + .describe( + "Dot-joined path to the rejected field, e.g. `entries.3.measuredAt`. Empty string when the whole body was rejected.", + ), + code: z + .string() + .describe( + "Zod issue code, e.g. `invalid_type`, `too_small`, `unrecognized_keys`. Treat an unfamiliar code as a generic rejection of `path`.", + ), + message: z + .string() + .describe( + "English sentence describing the rejection. Safe to log; show the user your own wording keyed on `path`.", + ), + keys: z + .array(z.string()) + .optional() + .describe( + "Only on `unrecognized_keys`: the refused key names, bounded to 10 and 64 characters each. Zod reports every unknown key of one object in a single issue whose `path` is the object, so the names live here and nowhere in `path`.", + ), + }) + .meta({ + id: "ValidationIssue", + description: + "One rejected field in a multi-issue 422. Sanitised: no rejected VALUE is echoed back.", + }); + export const errorEnvelope = z .object({ data: z.null(), error: z.string(), + details: z + .object({ issues: z.array(validationIssue) }) + .optional() + .describe( + "Present on a 422 raised by the multi-issue validator: every field the request got wrong, not only the first. Absent on every other status.", + ), + // Loose on purpose. Real refusals put more than the two named keys here — + // `removedIn` / `replacedBy` on a 410, `module` beside `module.disabled`, + // the per-integration context on a failed connection test — and a closed + // object made the body invalid against its own published schema, which any + // strict generated decoder is entitled to reject. meta: z - .object({ + .looseObject({ requestId: z.string().optional(), - errorCode: z.string().optional(), + errorCode: z + .string() + .optional() + .describe( + "Stable machine code for this refusal. Branch on it rather than on `error`, which is prose and may be reworded.", + ), }) .optional(), }) .meta({ id: "ErrorEnvelope", - description: "Standard error response: data is null, error is human prose.", + description: + "Standard error response: `data` is null, `error` is human prose. `meta.errorCode` carries the stable machine code where one exists, and `meta` may carry further per-refusal context. A 422 from the multi-issue validator additionally carries `details.issues` — every field the request got wrong, so a client can fix them in one round-trip.", }); export function dataEnvelope(payload: T, id: string) { From 0cd996e7de64a25618bfd1ac5d08b051d94510a7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Thu, 10 Sep 2026 04:05:04 +0200 Subject: [PATCH 04/13] docs(api): declare the two sync domains that were already on the wire MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `GET /api/sync/changes` returns five domains — measurements, mood, intakes, cycleDays, cycles — and has since the cycle module landed. The registered schema declared three, closed the object and required only those three, so `cycleDays` and `cycles` did not exist in the contract: full day-log and cycle payloads, both upserts and tombstones, computed and serialised on every single pull, invisible to any client generated from the spec. The body was also invalid against its own schema, since the object was closed against the two keys it always sends. The day-log and cycle DTOs were already published for the cycle routes, so the two upsert arrays reuse them and only the tombstone shapes are new. The module's own description and the operation summary were stale in the same direction and now name all five. This is the two-ended change the contributor notes describe: the writer shipped, the contract half did not, and no gate noticed because every gate proves the other end. No wire change. --- docs/api/openapi.yaml | 106 +++++++++++++++++++++++++++++--- src/lib/openapi/routes/cycle.ts | 4 +- src/lib/openapi/routes/sync.ts | 53 +++++++++++++++- 3 files changed, 151 insertions(+), 12 deletions(-) diff --git a/docs/api/openapi.yaml b/docs/api/openapi.yaml index 71a91a954..fe7ca0be4 100644 --- a/docs/api/openapi.yaml +++ b/docs/api/openapi.yaml @@ -3331,10 +3331,12 @@ paths: get: tags: - Sync - summary: Measurements delta feed - description: "Incremental catch-up after the first-pair backfill (never a replacement for it). Pages over an opaque - keyset cursor; each page carries `tombstones` (soft-deleted rows, keyed on `externalId`) and `upserts` (live - rows). Apply tombstones before upserts within a page. `cursorExpired: true` forces a clean re-init." + summary: Multi-domain delta feed + description: "Incremental catch-up after the first-pair backfill (never a replacement for it). Pages over one opaque + keyset cursor spanning five domains — measurements, mood, medication intakes, cycle day logs and cycles. Each + domain carries `tombstones` (soft-deleted rows) and `upserts` (live rows); apply tombstones before upserts + within a domain. Tombstone identity is `externalId` for measurements and the server `id` everywhere else. + `cursorExpired: true` forces a clean re-init and the page is empty in every domain." parameters: - in: query name: cursor @@ -37327,10 +37329,45 @@ components: - upserts - tombstones additionalProperties: false + cycleDays: + type: object + properties: + upserts: + type: array + items: + $ref: "#/components/schemas/CycleDayLogDTO" + tombstones: + type: array + items: + $ref: "#/components/schemas/SyncCycleDayTombstone" + required: + - upserts + - tombstones + additionalProperties: false + description: Cycle day logs. Present on every page, empty arrays when the account has the cycle module off. + cycles: + type: object + properties: + upserts: + type: array + items: + $ref: "#/components/schemas/MenstrualCycleDTO" + tombstones: + type: array + items: + $ref: "#/components/schemas/SyncCycleTombstone" + required: + - upserts + - tombstones + additionalProperties: false + description: Menstrual cycles, observed and forward-predicted. Present on every page, empty arrays when the account has + the cycle module off. required: - measurements - mood - intakes + - cycleDays + - cycles additionalProperties: false required: - serverNow @@ -37339,9 +37376,10 @@ components: - cursorExpired - changes additionalProperties: false - description: "Multi-domain delta page (v1.7.0): measurements + mood + intakes. One opaque multi-domain keyset cursor; - tombstones apply before upserts within each domain. Tombstone identity: measurements key on externalId, mood + - intakes on server id. The iOS consumer is measurements-only this cycle; mood + intakes are forward-prep." + description: "Multi-domain delta page: measurements, mood, intakes, cycle day logs and cycles. One opaque multi-domain + keyset cursor; tombstones apply before upserts within each domain. Tombstone identity: measurements key on + externalId, every other domain on server id. All five keys are present on every response, including the + `cursorExpired` short-circuit — a domain with nothing to report sends empty arrays rather than being omitted." SyncMeasurementUpsert: type: object properties: @@ -37683,6 +37721,60 @@ components: - updatedAt additionalProperties: false description: A soft-deleted medication intake, keyed on server `id`. Apply before upserts within the domain page. + SyncCycleDayTombstone: + type: object + properties: + id: + type: string + externalId: + anyOf: + - type: string + - type: "null" + description: The client's own key for the day log, when it set one. + syncVersion: + type: integer + minimum: 0 + maximum: 9007199254740991 + deletedAt: + type: string + format: date-time + pattern: ^(?:(?:\d\d[2468][048]|\d\d[13579][26]|\d\d0[48]|[02468][048]00|[13579][26]00)-02-29|\d{4}-(?:(?:0[13578]|1[02])-(?:0[1-9]|[12]\d|3[01])|(?:0[469]|11)-(?:0[1-9]|[12]\d|30)|(?:02)-(?:0[1-9]|1\d|2[0-8])))T(?:(?:[01]\d|2[0-3]):[0-5]\d(?::[0-5]\d(?:\.\d+)?)?(?:Z|([+-](?:[01]\d|2[0-3]):[0-5]\d)))$ + updatedAt: + type: string + format: date-time + pattern: ^(?:(?:\d\d[2468][048]|\d\d[13579][26]|\d\d0[48]|[02468][048]00|[13579][26]00)-02-29|\d{4}-(?:(?:0[13578]|1[02])-(?:0[1-9]|[12]\d|3[01])|(?:0[469]|11)-(?:0[1-9]|[12]\d|30)|(?:02)-(?:0[1-9]|1\d|2[0-8])))T(?:(?:[01]\d|2[0-3]):[0-5]\d(?::[0-5]\d(?:\.\d+)?)?(?:Z|([+-](?:[01]\d|2[0-3]):[0-5]\d)))$ + required: + - id + - externalId + - syncVersion + - deletedAt + - updatedAt + additionalProperties: false + description: A soft-deleted cycle day log, keyed on server `id`. Apply before upserts within the domain page. + SyncCycleTombstone: + type: object + properties: + id: + type: string + syncVersion: + type: integer + minimum: 0 + maximum: 9007199254740991 + deletedAt: + type: string + format: date-time + pattern: ^(?:(?:\d\d[2468][048]|\d\d[13579][26]|\d\d0[48]|[02468][048]00|[13579][26]00)-02-29|\d{4}-(?:(?:0[13578]|1[02])-(?:0[1-9]|[12]\d|3[01])|(?:0[469]|11)-(?:0[1-9]|[12]\d|30)|(?:02)-(?:0[1-9]|1\d|2[0-8])))T(?:(?:[01]\d|2[0-3]):[0-5]\d(?::[0-5]\d(?:\.\d+)?)?(?:Z|([+-](?:[01]\d|2[0-3]):[0-5]\d)))$ + updatedAt: + type: string + format: date-time + pattern: ^(?:(?:\d\d[2468][048]|\d\d[13579][26]|\d\d0[48]|[02468][048]00|[13579][26]00)-02-29|\d{4}-(?:(?:0[13578]|1[02])-(?:0[1-9]|[12]\d|3[01])|(?:0[469]|11)-(?:0[1-9]|[12]\d|30)|(?:02)-(?:0[1-9]|1\d|2[0-8])))T(?:(?:[01]\d|2[0-3]):[0-5]\d(?::[0-5]\d(?:\.\d+)?)?(?:Z|([+-](?:[01]\d|2[0-3]):[0-5]\d)))$ + required: + - id + - syncVersion + - deletedAt + - updatedAt + additionalProperties: false + description: A soft-deleted menstrual cycle, keyed on server `id`. Apply before upserts within the domain page. GetMeasurementsSeriesBatchResponse: type: object properties: diff --git a/src/lib/openapi/routes/cycle.ts b/src/lib/openapi/routes/cycle.ts index 903a8cd93..e32173f30 100644 --- a/src/lib/openapi/routes/cycle.ts +++ b/src/lib/openapi/routes/cycle.ts @@ -112,7 +112,7 @@ const cycleSymptomDto = z.object({ severity: z.number().int().min(1).max(4).nullable(), }); -const cycleDayLogDto = z +export const cycleDayLogDto = z .object({ id: z.string(), date: z.string(), @@ -145,7 +145,7 @@ const cycleDayLogDto = z "The canonical day-log row iOS mirrors. `note` is decrypted on read. Soft-deleted rows ride `/api/sync/changes` as tombstones.", }); -const menstrualCycleDto = z +export const menstrualCycleDto = z .object({ id: z.string(), startDate: z.string(), diff --git a/src/lib/openapi/routes/sync.ts b/src/lib/openapi/routes/sync.ts index 2f767327b..33071b202 100644 --- a/src/lib/openapi/routes/sync.ts +++ b/src/lib/openapi/routes/sync.ts @@ -9,6 +9,7 @@ import { z } from "zod/v4"; import type { ZodOpenApiObject } from "zod-openapi"; import { measurementResource } from "./measurements"; import { moodContextWire } from "./mood"; +import { cycleDayLogDto, menstrualCycleDto } from "./cycle"; import { dataEnvelope, stdResponses } from "./shared"; // ── Sync (v1.7.0 offline / server-optional) ───────────────────────── @@ -185,6 +186,36 @@ const syncIntakeTombstone = z "A soft-deleted medication intake, keyed on server `id`. Apply before upserts within the domain page.", }); +const syncCycleDayTombstone = z + .object({ + id: z.string(), + externalId: z + .string() + .nullable() + .describe("The client's own key for the day log, when it set one."), + syncVersion: z.number().int().nonnegative(), + deletedAt: z.iso.datetime({ offset: true }), + updatedAt: z.iso.datetime({ offset: true }), + }) + .meta({ + id: "SyncCycleDayTombstone", + description: + "A soft-deleted cycle day log, keyed on server `id`. Apply before upserts within the domain page.", + }); + +const syncCycleTombstone = z + .object({ + id: z.string(), + syncVersion: z.number().int().nonnegative(), + deletedAt: z.iso.datetime({ offset: true }), + updatedAt: z.iso.datetime({ offset: true }), + }) + .meta({ + id: "SyncCycleTombstone", + description: + "A soft-deleted menstrual cycle, keyed on server `id`. Apply before upserts within the domain page.", + }); + const syncChangesQuery = z .object({ cursor: z @@ -233,12 +264,28 @@ const syncChangesResponse = z upserts: z.array(syncIntakeUpsert), tombstones: z.array(syncIntakeTombstone), }), + cycleDays: z + .object({ + upserts: z.array(cycleDayLogDto), + tombstones: z.array(syncCycleDayTombstone), + }) + .describe( + "Cycle day logs. Present on every page, empty arrays when the account has the cycle module off.", + ), + cycles: z + .object({ + upserts: z.array(menstrualCycleDto), + tombstones: z.array(syncCycleTombstone), + }) + .describe( + "Menstrual cycles, observed and forward-predicted. Present on every page, empty arrays when the account has the cycle module off.", + ), }), }) .meta({ id: "SyncChangesResponse", description: - "Multi-domain delta page (v1.7.0): measurements + mood + intakes. One opaque multi-domain keyset cursor; tombstones apply before upserts within each domain. Tombstone identity: measurements key on externalId, mood + intakes on server id. The iOS consumer is measurements-only this cycle; mood + intakes are forward-prep.", + "Multi-domain delta page: measurements, mood, intakes, cycle day logs and cycles. One opaque multi-domain keyset cursor; tombstones apply before upserts within each domain. Tombstone identity: measurements key on externalId, every other domain on server id. All five keys are present on every response, including the `cursorExpired` short-circuit — a domain with nothing to report sends empty arrays rather than being omitted.", }); export const syncPaths: NonNullable = { @@ -264,9 +311,9 @@ export const syncPaths: NonNullable = { "/api/sync/changes": { get: { tags: ["Sync"], - summary: "Measurements delta feed", + summary: "Multi-domain delta feed", description: - "Incremental catch-up after the first-pair backfill (never a replacement for it). Pages over an opaque keyset cursor; each page carries `tombstones` (soft-deleted rows, keyed on `externalId`) and `upserts` (live rows). Apply tombstones before upserts within a page. `cursorExpired: true` forces a clean re-init.", + "Incremental catch-up after the first-pair backfill (never a replacement for it). Pages over one opaque keyset cursor spanning five domains — measurements, mood, medication intakes, cycle day logs and cycles. Each domain carries `tombstones` (soft-deleted rows) and `upserts` (live rows); apply tombstones before upserts within a domain. Tombstone identity is `externalId` for measurements and the server `id` everywhere else. `cursorExpired: true` forces a clean re-init and the page is empty in every domain.", requestParams: { query: syncChangesQuery, }, From 4993352cc925ce67ebfbc65a8b1cf74a3d410abb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Thu, 10 Sep 2026 04:08:52 +0200 Subject: [PATCH 05/13] test(api): parse real responses against the schemas that publish them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Response drift was structurally undetectable. `openapi:check` proves the YAML matches the registry and the route-coverage guard proves the verb is listed; both pass when the registered schema disagrees with what `apiSuccess()` sends, and the coverage guard says so in its own doc comment. That is how `/api/sync/changes` shipped five domains against a three-domain contract and how the error envelope went years without declaring the field the multi-issue 422 was built for. Two bodies, the two that were broken: a full `/api/sync/changes` page with live and soft-deleted rows in all five domains, its `cursorExpired` short-circuit (a second shape under the same schema), and a 422 carrying more than one issue. `safeParse` alone would not have caught either finding, because Zod strips undeclared keys instead of refusing them — a schema short of the wire parses cleanly. So the parsed value is compared back to the raw body; a strip means the contract is missing something the server sends. Removing `cycleDays` / `cycles` from the sync schema, or `details` from the envelope, fails all three cases. --- src/lib/openapi/routes/sync.ts | 10 +- .../openapi-response-shape.test.ts | 298 ++++++++++++++++++ 2 files changed, 307 insertions(+), 1 deletion(-) create mode 100644 tests/integration/openapi-response-shape.test.ts diff --git a/src/lib/openapi/routes/sync.ts b/src/lib/openapi/routes/sync.ts index 33071b202..007fd424c 100644 --- a/src/lib/openapi/routes/sync.ts +++ b/src/lib/openapi/routes/sync.ts @@ -236,7 +236,15 @@ const syncChangesQuery = z }) .meta({ id: "SyncChangesQuery" }); -const syncChangesResponse = z +/** + * Exported so a test can `safeParse` a real `/api/sync/changes` body against + * it. Nothing else checks a response SHAPE against its published schema — + * `openapi:check` proves the YAML matches the registry, and the route-coverage + * guard proves the path is listed, so a schema that disagrees with what + * `apiSuccess()` sends passes both. That is how this endpoint went five + * domains wide against a three-domain contract. + */ +export const syncChangesResponse = z .object({ serverNow: z.iso.datetime({ offset: true }), cursor: z diff --git a/tests/integration/openapi-response-shape.test.ts b/tests/integration/openapi-response-shape.test.ts new file mode 100644 index 000000000..89ee4f550 --- /dev/null +++ b/tests/integration/openapi-response-shape.test.ts @@ -0,0 +1,298 @@ +/** + * A real response, parsed against the schema that publishes it. + * + * Nothing in the gate did this before. `pnpm openapi:check` proves the YAML + * matches the registry; `openapi-route-coverage-guard` proves the verb is + * listed. Both pass when the registered schema disagrees with what the handler + * actually sends, and that gap is how `/api/sync/changes` ended up shipping + * five domains against a three-domain contract while the body was invalid + * against its own published schema. + * + * Two responses are covered — the ones the audit found broken: + * + * - `GET /api/sync/changes`, the multi-domain delta the native client mirrors + * its whole history from. + * - a multi-issue 422, whose `details.issues` array is the reason the + * envelope exists and was the field the envelope did not declare. + * + * The check is stricter than `safeParse` alone. Zod strips undeclared keys + * rather than refusing them, so a pass would say nothing about a field the + * server sends and the contract omits — exactly the direction both High + * findings drifted in. Comparing the parsed value back to the raw body catches + * the strip: if anything was dropped, the schema is short of the wire. + */ +import { NextRequest } from "next/server"; +import { beforeEach, describe, expect, it, vi } from "vitest"; + +import { cookieJar } from "./mock-next-headers"; +import { getPrismaClient, truncateAllTables } from "./setup"; +import { syncChangesResponse } from "@/lib/openapi/routes/sync"; +import { errorEnvelope } from "@/lib/openapi/routes/shared"; + +const TEST_USER_ID = "user-openapi-response-shape"; + +vi.mock("next/headers", async () => { + const { cookieJar, headerJar } = await import("./mock-next-headers"); + return { + headers: vi.fn(async () => ({ + get: (name: string) => headerJar.get(name.toLowerCase()) ?? null, + })), + cookies: vi.fn(async () => ({ + get: (name: string) => { + const value = cookieJar.get(name); + return value ? { name, value } : undefined; + }, + set: (name: string, value: string) => { + cookieJar.set(name, value); + }, + delete: (name: string) => { + cookieJar.delete(name); + }, + })), + }; +}); + +vi.mock("@/lib/db-compat", () => ({ + ensureDbCompatibility: vi.fn().mockResolvedValue(undefined), +})); + +beforeEach(async () => { + const prisma = getPrismaClient(); + await truncateAllTables(prisma); + cookieJar.clear(); + + // FEMALE so the cycle module is on by default and the two domains the + // contract used to omit actually carry rows. + await prisma.user.create({ + data: { + id: TEST_USER_ID, + username: "openapi-response-shape", + email: "openapi-response-shape@example.test", + gender: "FEMALE", + timezone: "UTC", + }, + }); + const session = await prisma.session.create({ + data: { + userId: TEST_USER_ID, + expiresAt: new Date(Date.now() + 60 * 60 * 1000), + }, + }); + cookieJar.set("healthlog_session", session.id); +}); + +/** + * Seed one live row and one soft-deleted row per domain, so every `upserts` + * and every `tombstones` array in the page is non-empty. An all-empty page + * would parse against almost any schema. + */ +async function seedEveryDomain(): Promise { + const prisma = getPrismaClient(); + const at = new Date("2026-02-02T08:00:00.000Z"); + const gone = new Date("2026-02-02T09:00:00.000Z"); + + await prisma.measurement.createMany({ + data: [ + { + id: "shape-measurement-live", + userId: TEST_USER_ID, + type: "WEIGHT", + value: 79.2, + unit: "kg", + measuredAt: at, + source: "MANUAL", + externalId: "shape-live", + }, + { + id: "shape-measurement-gone", + userId: TEST_USER_ID, + type: "PULSE", + value: 61, + unit: "bpm", + measuredAt: at, + source: "MANUAL", + externalId: "shape-gone", + deletedAt: gone, + }, + ], + }); + + await prisma.moodEntry.createMany({ + data: [ + { + id: "shape-mood-live", + userId: TEST_USER_ID, + date: "2026-02-02", + mood: "GOOD", + score: 4, + moodLoggedAt: at, + source: "MANUAL", + }, + { + id: "shape-mood-gone", + userId: TEST_USER_ID, + date: "2026-02-01", + mood: "OKAY", + score: 3, + moodLoggedAt: at, + source: "MANUAL", + deletedAt: gone, + }, + ], + }); + + const medication = await prisma.medication.create({ + data: { + id: "shape-medication", + userId: TEST_USER_ID, + name: "Test compound", + dose: "1 tablet", + }, + }); + await prisma.medicationIntakeEvent.createMany({ + data: [ + { + id: "shape-intake-live", + userId: TEST_USER_ID, + medicationId: medication.id, + scheduledFor: at, + takenAt: at, + skipped: false, + source: "WEB", + }, + { + id: "shape-intake-gone", + userId: TEST_USER_ID, + medicationId: medication.id, + scheduledFor: gone, + takenAt: null, + skipped: true, + source: "WEB", + deletedAt: gone, + }, + ], + }); + + await prisma.menstrualCycle.createMany({ + data: [ + { + id: "shape-cycle-live", + userId: TEST_USER_ID, + startDate: "2026-01-05", + }, + { + id: "shape-cycle-gone", + userId: TEST_USER_ID, + startDate: "2025-12-05", + deletedAt: gone, + }, + ], + }); + await prisma.cycleDayLog.createMany({ + data: [ + { + id: "shape-day-live", + userId: TEST_USER_ID, + date: "2026-01-05", + cycleId: "shape-cycle-live", + flow: "MEDIUM", + source: "MANUAL", + }, + { + id: "shape-day-gone", + userId: TEST_USER_ID, + date: "2026-01-04", + source: "MANUAL", + deletedAt: gone, + }, + ], + }); +} + +describe("published response schemas against real bodies", () => { + it("parses a full /api/sync/changes page and drops nothing", async () => { + await seedEveryDomain(); + const { GET } = await import("@/app/api/sync/changes/route"); + + const res = await GET( + new NextRequest("http://localhost/api/sync/changes") as never, + ); + expect(res.status).toBe(200); + const body = (await res.json()) as { data: unknown }; + + const parsed = syncChangesResponse.safeParse(body.data); + expect(parsed.error?.issues ?? []).toEqual([]); + expect(parsed.success).toBe(true); + + // Every domain actually carried rows, so the parse above was asked a real + // question rather than being handed five empty pairs. + const changes = ( + body.data as { + changes: Record; + } + ).changes; + for (const domain of [ + "measurements", + "mood", + "intakes", + "cycleDays", + "cycles", + ]) { + expect(changes[domain].upserts.length).toBeGreaterThan(0); + expect(changes[domain].tombstones.length).toBeGreaterThan(0); + } + + // Zod strips what it does not declare, so an equal round-trip is the part + // that proves the contract is not short of the wire. + expect(parsed.data).toEqual(body.data); + }); + + it("parses the cursorExpired short-circuit page too", async () => { + const { GET } = await import("@/app/api/sync/changes/route"); + + // A cursor whose watermark predates tombstone retention forces the + // early return, which builds its own literal page rather than the one + // the domain walks assemble — a second shape under the same schema. + const { encodeCursor } = await import("@/lib/sync/cursor"); + const stale = encodeCursor({ + measurements: { + updatedAtMs: Date.now() - 400 * 24 * 60 * 60 * 1000, + id: "long-gone", + }, + }); + + const res = await GET( + new NextRequest( + `http://localhost/api/sync/changes?cursor=${stale}`, + ) as never, + ); + expect(res.status).toBe(200); + const body = (await res.json()) as { data: { cursorExpired: boolean } }; + + const parsed = syncChangesResponse.safeParse(body.data); + expect(parsed.error?.issues ?? []).toEqual([]); + expect(parsed.data).toEqual(body.data); + }); + + it("parses a multi-issue 422 against the published error envelope", async () => { + const { GET } = await import("@/app/api/measurements/route"); + + const res = await GET( + new NextRequest( + "http://localhost/api/measurements?limit=0&offset=-3&sortDir=sideways", + ) as never, + ); + expect(res.status).toBe(422); + const body = (await res.json()) as { + details?: { issues: unknown[] }; + }; + + // The envelope exists to report every bad field at once; if only one + // arrives, the fixture stopped exercising the multi-issue path. + expect(body.details?.issues.length ?? 0).toBeGreaterThan(1); + + const parsed = errorEnvelope.safeParse(body); + expect(parsed.error?.issues ?? []).toEqual([]); + expect(parsed.data).toEqual(body); + }); +}); From a7bdf14a4e267a132b1155ed067615fc7cf71f78 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Thu, 10 Sep 2026 04:17:45 +0200 Subject: [PATCH 06/13] test(api): pin the pagination tiebreaker in the two route tests Both asserted the single-column `orderBy` the offset lists used to build, so they held the defect in place: a test that pins the shape a fix has to change is not evidence, it is a lock. Turned round to assert the total order, with the reason at the call site. --- .../illness/episodes/[id]/day-logs/__tests__/route.test.ts | 7 +++++-- .../[id]/__tests__/skip-snooze-history.test.ts | 5 ++++- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/src/app/api/illness/episodes/[id]/day-logs/__tests__/route.test.ts b/src/app/api/illness/episodes/[id]/day-logs/__tests__/route.test.ts index 659d66159..6c2ae4d52 100644 --- a/src/app/api/illness/episodes/[id]/day-logs/__tests__/route.test.ts +++ b/src/app/api/illness/episodes/[id]/day-logs/__tests__/route.test.ts @@ -105,7 +105,10 @@ describe("GET /api/illness/episodes/{id}/day-logs — date-less list", () => { const findArgs = vi.mocked(prisma.illnessDayLog.findMany).mock.calls[0][0]; expect(findArgs).toMatchObject({ where: { episodeId: "ep-1", deletedAt: null }, - orderBy: { date: "desc" }, + // `date` is a calendar day and repeats within an episode, so the + // unique tiebreaker is what keeps `skip`-based paging from repeating + // one row and dropping another between pages. + orderBy: [{ date: "desc" }, { id: "desc" }], take: 60, skip: 0, }); @@ -126,7 +129,7 @@ describe("GET /api/illness/episodes/{id}/day-logs — date-less list", () => { const findArgs = vi.mocked(prisma.illnessDayLog.findMany).mock.calls[0][0]; expect(findArgs).toMatchObject({ - orderBy: { date: "asc" }, + orderBy: [{ date: "asc" }, { id: "asc" }], take: 10, skip: 20, }); diff --git a/src/app/api/measurement-reminders/[id]/__tests__/skip-snooze-history.test.ts b/src/app/api/measurement-reminders/[id]/__tests__/skip-snooze-history.test.ts index 8483ec1f6..ecafa0b85 100644 --- a/src/app/api/measurement-reminders/[id]/__tests__/skip-snooze-history.test.ts +++ b/src/app/api/measurement-reminders/[id]/__tests__/skip-snooze-history.test.ts @@ -303,7 +303,10 @@ describe("GET /api/measurement-reminders/[id]/history", () => { }); expect(body.data.meta).toEqual({ total: 2, limit: 10, offset: 0 }); const query = eventFindManyMock.mock.calls[0][0]; - expect(query.orderBy).toEqual({ occurredAt: "desc" }); + // The trailing `id` is the unique tiebreaker: several reminder events + // can share one `occurredAt`, and offset paging over a tied key repeats + // and drops rows between pages. + expect(query.orderBy).toEqual([{ occurredAt: "desc" }, { id: "desc" }]); expect(query.where).toEqual({ reminderId: "r1", userId: "u1" }); }); From 21dd7c7e480998204d2fbe6f8f687836cee97a73 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Thu, 10 Sep 2026 04:24:34 +0200 Subject: [PATCH 07/13] fix(api): give the generic auth refusals a stable errorCode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `requireAuth`, `requireAdmin`, `requireCookieAuth`, `requireFreshMfa` and `requireBearerAuth` all refused with a bare prose string and no `meta`. So the single most important branch a native client makes — refresh the token and retry, drop the session and show the login screen, or stop retrying because this credential will never work — could only be made by string-matching the English sentence "Token expired". Any rewording of that string, or a localisation pass, silently turns a recoverable refresh into a forced logout for every already-shipped build. The mechanism was already here and these gates were the ones skipped: the assistant, consent, step-up, module and sharing refusals all carry `meta.errorCode`, and `/api/auth/refresh` emits `auth.refresh.*`. So `HttpError` takes an optional code and the wrapper puts it in the same field. Five values, dot.case like the majority: `auth.missing`, `auth.token.expired`, `auth.token.invalid`, `auth.scope.insufficient`, `auth.admin.required`. A revoked token reads invalid, not expired — refreshing a revoked token loops forever. Additive: an HttpError thrown without a code serialises byte-identically to before, which is most of them. The published 401 description now enumerates the codes and says what a client should do with each. --- docs/api/openapi.yaml | 8 +- src/lib/__tests__/auth-error-codes.test.ts | 249 +++++++++++++++++++++ src/lib/api-errors.ts | 32 +++ src/lib/api-handler.ts | 45 ++-- src/lib/openapi/routes/shared.ts | 3 +- 5 files changed, 322 insertions(+), 15 deletions(-) create mode 100644 src/lib/__tests__/auth-error-codes.test.ts diff --git a/docs/api/openapi.yaml b/docs/api/openapi.yaml index fe7ca0be4..06a43b581 100644 --- a/docs/api/openapi.yaml +++ b/docs/api/openapi.yaml @@ -70,7 +70,13 @@ paths: schema: $ref: "#/components/schemas/CycleCustomSymptomListEnvelope" "401": &a1 - description: Authentication required or invalid credentials. + description: "Authentication required or invalid credentials. `meta.errorCode` says which, and it is the field to branch + on — the English sentence beside it may be reworded or localised at any time. `auth.missing`: no session + cookie and no Bearer header; send the user to sign in. `auth.token.expired`: the Bearer token's lifetime has + passed; refresh and retry the request. `auth.token.invalid`: unknown, revoked, or the owning account is + gone; discard the credential and sign in again — retrying will not help. The related 403 refusals carry + `auth.scope.insufficient` (the credential is valid but its scope does not reach this route, including any + Bearer token against an admin surface) and `auth.admin.required` (a cookie session that is not an admin's)." content: application/json: schema: diff --git a/src/lib/__tests__/auth-error-codes.test.ts b/src/lib/__tests__/auth-error-codes.test.ts new file mode 100644 index 000000000..eb608b7e9 --- /dev/null +++ b/src/lib/__tests__/auth-error-codes.test.ts @@ -0,0 +1,249 @@ +/** + * The generic auth gates refuse with a stable machine code. + * + * This decides the single most important branch a native client makes: + * refresh the token and retry, drop the session and show the login screen, or + * stop retrying because the credential will never work. Before these codes the + * only way to tell those apart was to string-match the English sentence "Token + * expired", so a rewording — or a localisation pass — would have turned a + * recoverable refresh into a forced logout for every already-shipped build. + * + * So the assertions here are on the CODE, and deliberately not on the prose. + * The one place the sentence is pinned is the pair of cases proving the + * envelope is otherwise unchanged: `error` still carries a sentence, and an + * HttpError thrown without a code still serialises without a `meta`. + */ +import { describe, it, expect, vi, beforeEach } from "vitest"; + +vi.mock("@/lib/db", () => ({ + prisma: { + apiToken: { findUnique: vi.fn(), update: vi.fn() }, + user: { findUnique: vi.fn() }, + session: { findUnique: vi.fn() }, + webauthnMfaCredential: { count: vi.fn() }, + passkey: { count: vi.fn() }, + }, +})); + +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() })); + +const headersGet = vi.fn<(name: string) => string | null>(); +vi.mock("next/headers", () => ({ + headers: vi.fn(async () => ({ get: headersGet })), + cookies: vi.fn(async () => ({ + get: () => undefined, + set: () => {}, + delete: () => {}, + })), +})); + +import { + apiHandler, + AUTH_ERROR_CODES, + HttpError, + MFA_STEP_UP_MAX_AGE_SECONDS, + requireAdmin, + requireAuth, + requireFreshMfa, +} from "../api-handler"; +import { prisma } from "@/lib/db"; +import { getSession } from "@/lib/auth/session"; +import { hashToken } from "@/lib/auth/hmac"; +import { auditLog } from "@/lib/auth/audit"; + +const FAKE_HASH = "deadbeefcafef00d"; +const RAW_TOKEN = "hlk_" + "a".repeat(64); +const FAKE_USER = { + id: "user-1", + role: "USER" as const, + username: "testuser", + email: "user@example.com", +}; + +function setBearerHeader(value: string | null): void { + headersGet.mockReset(); + headersGet.mockImplementation((name: string) => + name.toLowerCase() === "authorization" ? value : null, + ); +} + +function mockSession(user: unknown) { + vi.mocked(getSession).mockResolvedValue({ + session: { id: "sess-1", expiresAt: new Date(Date.now() + 1e6) }, + user, + } as never); +} + +beforeEach(() => { + vi.resetAllMocks(); + vi.mocked(hashToken).mockReturnValue(FAKE_HASH); + vi.mocked(prisma.apiToken.update).mockResolvedValue({} as never); + vi.mocked(auditLog).mockResolvedValue(undefined as never); +}); + +/** Run a gate through `apiHandler` and read the envelope it produced. */ +async function envelopeFor(gate: () => Promise): Promise<{ + status: number; + body: { data: null; error: string; meta?: { errorCode?: string } }; +}> { + const handler = apiHandler(async (_request: Request) => { + await gate(); + return new Response(null, { status: 204 }); + }); + const res = (await handler( + new Request("http://localhost/api/anything"), + )) as Response; + return { + status: res.status, + body: (await res.json()) as { + data: null; + error: string; + meta?: { errorCode?: string }; + }, + }; +} + +describe("auth failures carry a machine-readable errorCode", () => { + it("names auth.missing when no credential was presented at all", async () => { + vi.mocked(getSession).mockResolvedValue(null); + setBearerHeader(null); + + const { status, body } = await envelopeFor(() => requireAuth()); + + expect(status).toBe(401); + expect(body.meta?.errorCode).toBe(AUTH_ERROR_CODES.missing); + // The envelope is otherwise what it always was. + expect(body.data).toBeNull(); + expect(body.error).toBe("Not authenticated"); + }); + + it("names auth.token.expired so the client refreshes instead of logging out", async () => { + vi.mocked(getSession).mockResolvedValue(null); + setBearerHeader(`Bearer ${RAW_TOKEN}`); + vi.mocked(prisma.apiToken.findUnique).mockResolvedValue({ + id: "token-expired", + userId: "user-1", + permissions: ["*"], + revoked: false, + expiresAt: new Date(Date.now() - 60_000), + } as never); + + const { status, body } = await envelopeFor(() => requireAuth()); + + expect(status).toBe(401); + expect(body.meta?.errorCode).toBe(AUTH_ERROR_CODES.expired); + }); + + it("names auth.token.invalid for an unknown token", async () => { + vi.mocked(getSession).mockResolvedValue(null); + setBearerHeader(`Bearer ${RAW_TOKEN}`); + vi.mocked(prisma.apiToken.findUnique).mockResolvedValue(null); + + const { status, body } = await envelopeFor(() => requireAuth()); + + expect(status).toBe(401); + expect(body.meta?.errorCode).toBe(AUTH_ERROR_CODES.invalid); + }); + + it("names auth.token.invalid for a revoked token — not the expired code", async () => { + vi.mocked(getSession).mockResolvedValue(null); + setBearerHeader(`Bearer ${RAW_TOKEN}`); + vi.mocked(prisma.apiToken.findUnique).mockResolvedValue({ + id: "token-revoked", + userId: "user-1", + permissions: ["*"], + revoked: true, + expiresAt: null, + } as never); + + const { body } = await envelopeFor(() => requireAuth()); + + // Refreshing a revoked token loops forever; the two must not share a code. + expect(body.meta?.errorCode).toBe(AUTH_ERROR_CODES.invalid); + expect(body.meta?.errorCode).not.toBe(AUTH_ERROR_CODES.expired); + }); + + it("names auth.scope.insufficient when the token's scope does not reach the route", async () => { + vi.mocked(getSession).mockResolvedValue(null); + setBearerHeader(`Bearer ${RAW_TOKEN}`); + vi.mocked(prisma.apiToken.findUnique).mockResolvedValue({ + id: "token-narrow", + userId: "user-1", + permissions: ["something:else"], + revoked: false, + expiresAt: null, + } as never); + + const { status, body } = await envelopeFor(() => + requireAuth("medication:ingest"), + ); + + expect(status).toBe(403); + expect(body.meta?.errorCode).toBe(AUTH_ERROR_CODES.scope); + }); + + it("names auth.scope.insufficient when a narrow token hits a route that declared none", async () => { + vi.mocked(getSession).mockResolvedValue(null); + setBearerHeader(`Bearer ${RAW_TOKEN}`); + vi.mocked(prisma.apiToken.findUnique).mockResolvedValue({ + id: "token-undeclared", + userId: "user-1", + permissions: ["health:read"], + revoked: false, + expiresAt: null, + } as never); + + const { status, body } = await envelopeFor(() => requireAuth()); + + expect(status).toBe(403); + expect(body.meta?.errorCode).toBe(AUTH_ERROR_CODES.scope); + }); + + it("names auth.missing and auth.admin.required on the admin gate", async () => { + vi.mocked(getSession).mockResolvedValue(null); + const anonymous = await envelopeFor(() => requireAdmin()); + expect(anonymous.status).toBe(401); + expect(anonymous.body.meta?.errorCode).toBe(AUTH_ERROR_CODES.missing); + + mockSession(FAKE_USER); + const nonAdmin = await envelopeFor(() => requireAdmin()); + expect(nonAdmin.status).toBe(403); + expect(nonAdmin.body.meta?.errorCode).toBe(AUTH_ERROR_CODES.admin); + }); + + it("names auth.missing on the step-up gate with no session", async () => { + vi.mocked(getSession).mockResolvedValue(null); + + const { status, body } = await envelopeFor(() => + requireFreshMfa(MFA_STEP_UP_MAX_AGE_SECONDS), + ); + + expect(status).toBe(401); + expect(body.meta?.errorCode).toBe(AUTH_ERROR_CODES.missing); + }); + + it("leaves an HttpError thrown without a code exactly as it was", async () => { + const handler = apiHandler(async (_request: Request) => { + throw new HttpError(404, "Not found"); + }); + const res = (await handler( + new Request("http://localhost/api/anything"), + )) as Response; + + expect(res.status).toBe(404); + expect(await res.json()).toEqual({ data: null, error: "Not found" }); + }); + + it("keeps every code distinct and dot.case", () => { + const codes = Object.values(AUTH_ERROR_CODES); + expect(new Set(codes).size).toBe(codes.length); + for (const code of codes) { + expect(code).toMatch(/^auth(\.[a-z_]+)+$/); + } + }); +}); diff --git a/src/lib/api-errors.ts b/src/lib/api-errors.ts index 58597c667..05b49dec6 100644 --- a/src/lib/api-errors.ts +++ b/src/lib/api-errors.ts @@ -14,17 +14,49 @@ import { RECORD_FENCE_ERROR_CODE } from "./sharing/record-session-fence-contract /** * Custom error class for HTTP errors with status codes. * Throw inside apiHandler-wrapped routes to return a JSON error response. + * + * `errorCode` is optional and lands in `meta.errorCode` when set — the same + * field the step-up, consent, module and sharing gates already use. An + * HttpError without one serialises exactly as before. */ export class HttpError extends Error { constructor( public statusCode: number, message: string, + public errorCode?: string, ) { super(message); this.name = "HttpError"; } } +/** + * The stable codes the generic auth gates refuse with. + * + * These decide the single most important branch a native client makes: + * refresh the token and retry, drop the session and show the login screen, or + * stop retrying because this credential will never work. Until they existed + * the only way to tell those apart was to string-match the English sentence + * "Token expired", so a rewording — or a localisation pass — would have turned + * a recoverable refresh into a forced logout for every shipped build. + * + * Dot.case, matching the majority convention and the `auth.refresh.*` / + * `auth.stepup.*` families already on the wire. Add to this list rather than + * inventing a second field. + */ +export const AUTH_ERROR_CODES = { + /** No credential at all: no session cookie and no Bearer header. */ + missing: "auth.missing", + /** The Bearer token was valid once and its expiry has passed. Refresh. */ + expired: "auth.token.expired", + /** Unknown, revoked, or belonging to a user that no longer exists. */ + invalid: "auth.token.invalid", + /** Authenticated, but the credential's scope does not reach this route. */ + scope: "auth.scope.insufficient", + /** Authenticated, but not an admin. Cookie-only surface; never a token. */ + admin: "auth.admin.required", +} as const; + /** * v1.36.0 — the two refusals the acting-account resolver can raise. * diff --git a/src/lib/api-handler.ts b/src/lib/api-handler.ts index 3dc8fb7c7..9a916127a 100644 --- a/src/lib/api-handler.ts +++ b/src/lib/api-handler.ts @@ -81,6 +81,7 @@ export function isMcpAudienceToken(permissions: readonly string[]): boolean { // the api-handler ↔ record-session-fence import cycle) and are re-exported // here so the hundreds of existing importers read exactly as before. import { + AUTH_ERROR_CODES, HttpError, SharingAuthError, SharingAccessDeniedError, @@ -90,6 +91,7 @@ import { } from "./api-errors"; export { + AUTH_ERROR_CODES, HttpError, SharingAuthError, SharingAccessDeniedError, @@ -295,9 +297,17 @@ export function apiHandler Promise>( { status: error.statusCode }, ); } else if (error instanceof HttpError) { + // `meta.errorCode` only when the throw site named one, so every + // HttpError without a code serialises byte-identically to before. evt.setError(error); response = NextResponse.json( - { data: null, error: error.message }, + { + data: null, + error: error.message, + ...(error.errorCode + ? { meta: { errorCode: error.errorCode } } + : {}), + }, { status: error.statusCode }, ); } else if (error instanceof SyntaxError) { @@ -540,7 +550,7 @@ async function authenticateCaller( } // 3. No credentials. - throw new HttpError(401, "Not authenticated"); + throw new HttpError(401, "Not authenticated", AUTH_ERROR_CODES.missing); } /** @@ -584,13 +594,15 @@ async function authenticateBearer( : {}), }, }).catch(() => {}); - const message = + // Sentence and code are decided together, so a future rewording of the + // prose cannot silently change what a client does with the refusal. + const [message, errorCode] = err.statusCode === 403 - ? "Insufficient permissions" + ? (["Insufficient permissions", AUTH_ERROR_CODES.scope] as const) : err.reason === "expired" - ? "Token expired" - : "Invalid token"; - throw new HttpError(err.statusCode, message); + ? (["Token expired", AUTH_ERROR_CODES.expired] as const) + : (["Invalid token", AUTH_ERROR_CODES.invalid] as const); + throw new HttpError(err.statusCode, message, errorCode); } throw err; } @@ -626,7 +638,11 @@ async function authenticateBearer( method: method || "unknown", }, }).catch(() => {}); - throw new HttpError(403, "Insufficient permissions"); + throw new HttpError( + 403, + "Insufficient permissions", + AUTH_ERROR_CODES.scope, + ); } } @@ -1114,7 +1130,8 @@ async function resolveSwitchedRecord( */ export async function requireAdmin(): Promise { const sessionData = await getSession(); - if (!sessionData) throw new HttpError(401, "Not authenticated"); + if (!sessionData) + throw new HttpError(401, "Not authenticated", AUTH_ERROR_CODES.missing); const evt = getEvent(); if (evt) { @@ -1126,7 +1143,7 @@ export async function requireAdmin(): Promise { } if (sessionData.user.role !== "ADMIN") { - throw new HttpError(403, "Admin access required"); + throw new HttpError(403, "Admin access required", AUTH_ERROR_CODES.admin); } return { ...sessionData, authMethod: "cookie" }; } @@ -1143,7 +1160,8 @@ export async function requireAdmin(): Promise { */ export async function requireCookieAuth(): Promise { const sessionData = await getSession(); - if (!sessionData) throw new HttpError(401, "Not authenticated"); + if (!sessionData) + throw new HttpError(401, "Not authenticated", AUTH_ERROR_CODES.missing); const evt = getEvent(); if (evt) { @@ -1217,7 +1235,8 @@ export async function requireFreshMfa( proofSource: FreshFactorProofSource = "second-factor", ): Promise { const sessionData = await getSession(); - if (!sessionData) throw new HttpError(401, "Not authenticated"); + if (!sessionData) + throw new HttpError(401, "Not authenticated", AUTH_ERROR_CODES.missing); const evt = getEvent(); if (evt) { @@ -1448,7 +1467,7 @@ export async function requireBearerAuth(): Promise { authHeader = null; } if (!authHeader?.startsWith("Bearer ")) { - throw new HttpError(401, "Not authenticated"); + throw new HttpError(401, "Not authenticated", AUTH_ERROR_CODES.missing); } const raw = authHeader.slice(7); const auth = await authenticateBearer(raw, undefined); diff --git a/src/lib/openapi/routes/shared.ts b/src/lib/openapi/routes/shared.ts index f75a366a9..66f0baf49 100644 --- a/src/lib/openapi/routes/shared.ts +++ b/src/lib/openapi/routes/shared.ts @@ -229,7 +229,8 @@ export const invalidBaseTokenResponse = { export const stdResponses = { "401": { - description: "Authentication required or invalid credentials.", + description: + "Authentication required or invalid credentials. `meta.errorCode` says which, and it is the field to branch on — the English sentence beside it may be reworded or localised at any time. `auth.missing`: no session cookie and no Bearer header; send the user to sign in. `auth.token.expired`: the Bearer token's lifetime has passed; refresh and retry the request. `auth.token.invalid`: unknown, revoked, or the owning account is gone; discard the credential and sign in again — retrying will not help. The related 403 refusals carry `auth.scope.insufficient` (the credential is valid but its scope does not reach this route, including any Bearer token against an admin surface) and `auth.admin.required` (a cookie session that is not an admin's).", content: { "application/json": { schema: errorEnvelope } }, }, "422": { From dd3844a7a26e9c4d7cefcf9e1f4dd9f028382b3e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Thu, 10 Sep 2026 04:55:19 +0200 Subject: [PATCH 08/13] fix(api): dress only the 429s a limiter actually refused MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The request-scoped capture kept the last verdict of any kind and fell back to it when nothing had refused. That fallback fires in exactly one state — no limiter refused this request — which is the definition of "this 429 came from somewhere else". So a daily AI budget that is spent until midnight, an hourly generation quota, and a provider-side 429 relayed onward all went out carrying our own bucket's numbers: a remainder that says there is room left and a Retry-After measured against an unrelated window. On the AI paths the captured window can also expire while the provider call runs, so the dressed delay could land at zero and tell the client to retry immediately into the ceiling that had just refused it. The cell now keeps refusals only. A request every bucket let through reports nothing, and a 429 from another cause leaves undressed, which is correct: there is no bucket to describe. The two-limiter rule is unchanged — the last refusal wins, and a refusal still outranks a pass. The unit case that pinned the fallback is turned round, and both directions now run through the real wrapper: a limiter refusal gets the headers, a 429 raised without one gets none. The integration file gains the two shapes it was missing — a route that consults two buckets must report the one that refused, and a route whose 429 comes from the daily budget must carry no rate-limit headers at all. --- .../api-handler-429-dressing.test.ts | 191 ++++++++++++++++++ src/lib/__tests__/rate-limit-headers.test.ts | 20 +- src/lib/api-handler.ts | 5 + src/lib/rate-limit-context.ts | 26 ++- .../rate-limit-429-headers.test.ts | 86 +++++++- 5 files changed, 305 insertions(+), 23 deletions(-) create mode 100644 src/lib/__tests__/api-handler-429-dressing.test.ts 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(); + } + }); }); From dd8a724039c1470dafa2b0594e4ed5849fed8b84 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Thu, 10 Sep 2026 04:55:47 +0200 Subject: [PATCH 09/13] fix(api): floor Retry-After at one second on a refusal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `resetAt` is written by Postgres as `NOW() + interval` and subtracted from the app process's clock, so a row that still refuses can read as already expired here. The old floor let that render as `Retry-After: 0`, which tells a client to retry immediately into the ceiling that just turned it away — a contradiction of the status it rides on, and a hot loop for anything that honours the header. One second is the honest minimum on a 429. The rounding-up rule and the reason for it are unchanged; only the floor moves. --- src/lib/__tests__/rate-limit-headers.test.ts | 7 +++++-- src/lib/rate-limit-context.ts | 10 ++++++---- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/src/lib/__tests__/rate-limit-headers.test.ts b/src/lib/__tests__/rate-limit-headers.test.ts index 51938a67c..248cd2aad 100644 --- a/src/lib/__tests__/rate-limit-headers.test.ts +++ b/src/lib/__tests__/rate-limit-headers.test.ts @@ -48,13 +48,16 @@ describe("rateLimitResponseHeaders", () => { expect(headers["Retry-After"]).toBe("1"); }); - it("never names a delay in the past", () => { + it("names at least a second even when the window already reads as expired", () => { const now = Date.UTC(2026, 0, 1, 12, 0, 0); const headers = rateLimitResponseHeaders( { limit: 10, remaining: 0, resetAt: now - 5_000 }, now, ); - expect(headers["Retry-After"]).toBe("0"); + // `resetAt` is the database clock and `now` is this process's, so a row + // that still refuses can read as expired here. Zero would say "retry now" + // beside a refusal and turn the client into a hot loop. + expect(headers["Retry-After"]).toBe("1"); }); it("keeps X-RateLimit-Reset in the ISO form already on the wire", () => { diff --git a/src/lib/rate-limit-context.ts b/src/lib/rate-limit-context.ts index 75df1b2a3..fd1181233 100644 --- a/src/lib/rate-limit-context.ts +++ b/src/lib/rate-limit-context.ts @@ -80,9 +80,11 @@ export function capturedRateLimit(): RateLimitSnapshot | null { * * `Retry-After` is whole seconds and rounds UP, per RFC 9110 §10.2.3: a client * that waits the value it is given must find the window already rolled over, - * which flooring cannot promise. It never goes below zero — a window that - * expired between the verdict and the response says "retry now" rather than - * naming a time in the past. + * which flooring cannot promise. It never goes below one second: `resetAt` + * comes from the database clock and the subtraction happens on the app + * process's, so a window can read as already expired here while the row still + * refuses; and zero on a refusal means "retry now", which contradicts the + * status it rides on and invites a hot loop. * * `X-RateLimit-Reset` keeps the ISO-8601 instant it has always carried. It is * the wrong convention for the header name and every other deployment uses @@ -96,7 +98,7 @@ export function rateLimitResponseHeaders( ): Record { return { "Retry-After": String( - Math.max(0, Math.ceil((snapshot.resetAt - now) / 1000)), + Math.max(1, Math.ceil((snapshot.resetAt - now) / 1000)), ), "X-RateLimit-Limit": String(snapshot.limit), "X-RateLimit-Remaining": String(snapshot.remaining), From 2c63b0c4a7e2f4f2c3558789ca53797db98ada7a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Thu, 10 Sep 2026 04:59:40 +0200 Subject: [PATCH 10/13] fix(api): give the remaining 401 throws a code, and say what the code covers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three refusals still answered 401 with prose alone: the Bearer re-read on `/api/auth/me/sessions`, and the wrong-second-factor arm on both MFA step-up routes. The shared 401 description told clients the code is the field to branch on, so a client that took that advice landed in its default arm on all three — and on the MFA pair that is the difference between re-prompting for a digit and signing the user out over a typo. The sessions re-read is the same missing-credential fact the gate already names, so it reuses `auth.missing`. The MFA pair gets a code of its own, `auth.mfa.code_invalid`: none of the existing ones fit, because every other 401 code asks the client to discard something, and this one asks it to keep the session and try another code. The description narrows to match what is actually true. The generic gates name a reason; a route that checks a credential of its own — sign-in, the MFA challenge exchange, a password confirmation — still answers 401 with prose, and the text now says so instead of promising a field those responses do not carry. --- docs/api/openapi.yaml | 15 ++++++++++----- .../auth/me/mfa/disable/__tests__/route.test.ts | 4 ++++ src/app/api/auth/me/mfa/disable/route.ts | 3 ++- src/app/api/auth/me/mfa/totp/confirm/route.ts | 3 ++- src/app/api/auth/me/sessions/route.ts | 9 +++++++-- src/lib/api-errors.ts | 7 +++++++ src/lib/openapi/routes/shared.ts | 2 +- 7 files changed, 33 insertions(+), 10 deletions(-) diff --git a/docs/api/openapi.yaml b/docs/api/openapi.yaml index 06a43b581..ad23edaa4 100644 --- a/docs/api/openapi.yaml +++ b/docs/api/openapi.yaml @@ -70,13 +70,18 @@ paths: schema: $ref: "#/components/schemas/CycleCustomSymptomListEnvelope" "401": &a1 - description: "Authentication required or invalid credentials. `meta.errorCode` says which, and it is the field to branch - on — the English sentence beside it may be reworded or localised at any time. `auth.missing`: no session + description: "Authentication required or invalid credentials. The generic auth gates — the ones every route passes + through before its own body runs — name the reason in `meta.errorCode`, and that is the field to branch on + there; the English sentence beside it may be reworded or localised at any time. `auth.missing`: no session cookie and no Bearer header; send the user to sign in. `auth.token.expired`: the Bearer token's lifetime has passed; refresh and retry the request. `auth.token.invalid`: unknown, revoked, or the owning account is - gone; discard the credential and sign in again — retrying will not help. The related 403 refusals carry - `auth.scope.insufficient` (the credential is valid but its scope does not reach this route, including any - Bearer token against an admin surface) and `auth.admin.required` (a cookie session that is not an admin's)." + gone; discard the credential and sign in again — retrying will not help. `auth.mfa.code_invalid`: a second + factor presented on a step-up flow did not verify; re-prompt for a code and keep the session. The related + 403 refusals carry `auth.scope.insufficient` (the credential is valid but its scope does not reach this + route, including any Bearer token against an admin surface) and `auth.admin.required` (a cookie session that + is not an admin's). A route that checks a credential of its own — sign-in, the MFA challenge exchange, a + password confirmation — may still answer 401 with prose and no code, so a missing `meta.errorCode` means the + route refused the credential it was handed, not that the response is malformed." content: application/json: schema: diff --git a/src/app/api/auth/me/mfa/disable/__tests__/route.test.ts b/src/app/api/auth/me/mfa/disable/__tests__/route.test.ts index 7884d3f52..6639267e6 100644 --- a/src/app/api/auth/me/mfa/disable/__tests__/route.test.ts +++ b/src/app/api/auth/me/mfa/disable/__tests__/route.test.ts @@ -147,6 +147,10 @@ describe("POST /api/auth/me/mfa/disable", () => { } as never); const res = await POST(req()); expect(res.status).toBe(401); + // The code is what separates "re-prompt for a digit" from "this session is + // gone"; a client that branches on prose here signs the user out on a typo. + const body = (await res.json()) as { meta?: { errorCode?: string } }; + expect(body.meta?.errorCode).toBe("auth.mfa.code_invalid"); expect(destroyOtherSessions).not.toHaveBeenCalled(); }); }); diff --git a/src/app/api/auth/me/mfa/disable/route.ts b/src/app/api/auth/me/mfa/disable/route.ts index 50d22d12a..8e9718220 100644 --- a/src/app/api/auth/me/mfa/disable/route.ts +++ b/src/app/api/auth/me/mfa/disable/route.ts @@ -13,6 +13,7 @@ */ import { apiHandler, + AUTH_ERROR_CODES, requireMfaManagementAuth, HttpError, } from "@/lib/api-handler"; @@ -81,7 +82,7 @@ export const POST = apiHandler(async (req: Request) => { }, }); annotate({ action: { name: "auth.mfa.disable.invalid_factor" } }); - throw new HttpError(401, "Invalid code"); + throw new HttpError(401, "Invalid code", AUTH_ERROR_CODES.mfaCode); } // Every cheap check has passed and the teardown is next, so spend the diff --git a/src/app/api/auth/me/mfa/totp/confirm/route.ts b/src/app/api/auth/me/mfa/totp/confirm/route.ts index 8e701bf04..1251829d0 100644 --- a/src/app/api/auth/me/mfa/totp/confirm/route.ts +++ b/src/app/api/auth/me/mfa/totp/confirm/route.ts @@ -18,6 +18,7 @@ */ import { apiHandler, + AUTH_ERROR_CODES, requireMfaManagementAuth, HttpError, } from "@/lib/api-handler"; @@ -99,7 +100,7 @@ export const POST = apiHandler(async (req: Request) => { action: { name: "auth.mfa.totp.confirm.invalid" }, meta: { replay: result.replay }, }); - throw new HttpError(401, "Invalid code"); + throw new HttpError(401, "Invalid code", AUTH_ERROR_CODES.mfaCode); } const recoveryCodes = generateRecoveryCodes(); diff --git a/src/app/api/auth/me/sessions/route.ts b/src/app/api/auth/me/sessions/route.ts index 3d447e3d9..ee4ade61c 100644 --- a/src/app/api/auth/me/sessions/route.ts +++ b/src/app/api/auth/me/sessions/route.ts @@ -13,7 +13,12 @@ */ import { NextRequest } from "next/server"; import { headers } from "next/headers"; -import { apiHandler, HttpError, requireAuth } from "@/lib/api-handler"; +import { + apiHandler, + AUTH_ERROR_CODES, + HttpError, + requireAuth, +} from "@/lib/api-handler"; import { apiSuccess } from "@/lib/api-response"; import { annotate } from "@/lib/logging/context"; import { auditLog } from "@/lib/auth/audit"; @@ -139,7 +144,7 @@ export const DELETE = apiHandler(async (request: NextRequest) => { async function presentedBearerToken(): Promise { const authHeader = (await headers()).get("authorization"); if (!authHeader?.startsWith("Bearer ")) { - throw new HttpError(401, "Not authenticated"); + throw new HttpError(401, "Not authenticated", AUTH_ERROR_CODES.missing); } return authHeader.slice(7); } diff --git a/src/lib/api-errors.ts b/src/lib/api-errors.ts index 05b49dec6..29aaf9a22 100644 --- a/src/lib/api-errors.ts +++ b/src/lib/api-errors.ts @@ -55,6 +55,13 @@ export const AUTH_ERROR_CODES = { scope: "auth.scope.insufficient", /** Authenticated, but not an admin. Cookie-only surface; never a token. */ admin: "auth.admin.required", + /** + * The second factor presented on a step-up flow did not verify — a wrong + * TOTP digit, a replayed one, or a recovery code that is spent. The session + * itself is untouched: the client re-prompts for a code rather than signing + * the user out, which is the opposite of what the other 401 codes ask for. + */ + mfaCode: "auth.mfa.code_invalid", } as const; /** diff --git a/src/lib/openapi/routes/shared.ts b/src/lib/openapi/routes/shared.ts index 66f0baf49..4018f4ab3 100644 --- a/src/lib/openapi/routes/shared.ts +++ b/src/lib/openapi/routes/shared.ts @@ -230,7 +230,7 @@ export const invalidBaseTokenResponse = { export const stdResponses = { "401": { description: - "Authentication required or invalid credentials. `meta.errorCode` says which, and it is the field to branch on — the English sentence beside it may be reworded or localised at any time. `auth.missing`: no session cookie and no Bearer header; send the user to sign in. `auth.token.expired`: the Bearer token's lifetime has passed; refresh and retry the request. `auth.token.invalid`: unknown, revoked, or the owning account is gone; discard the credential and sign in again — retrying will not help. The related 403 refusals carry `auth.scope.insufficient` (the credential is valid but its scope does not reach this route, including any Bearer token against an admin surface) and `auth.admin.required` (a cookie session that is not an admin's).", + "Authentication required or invalid credentials. The generic auth gates — the ones every route passes through before its own body runs — name the reason in `meta.errorCode`, and that is the field to branch on there; the English sentence beside it may be reworded or localised at any time. `auth.missing`: no session cookie and no Bearer header; send the user to sign in. `auth.token.expired`: the Bearer token's lifetime has passed; refresh and retry the request. `auth.token.invalid`: unknown, revoked, or the owning account is gone; discard the credential and sign in again — retrying will not help. `auth.mfa.code_invalid`: a second factor presented on a step-up flow did not verify; re-prompt for a code and keep the session. The related 403 refusals carry `auth.scope.insufficient` (the credential is valid but its scope does not reach this route, including any Bearer token against an admin surface) and `auth.admin.required` (a cookie session that is not an admin's). A route that checks a credential of its own — sign-in, the MFA challenge exchange, a password confirmation — may still answer 401 with prose and no code, so a missing `meta.errorCode` means the route refused the credential it was handed, not that the response is malformed.", content: { "application/json": { schema: errorEnvelope } }, }, "422": { From 42100d94a71bc5a2f56c8dd55367544293fc0b70 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Thu, 10 Sep 2026 05:07:02 +0200 Subject: [PATCH 11/13] fix(api): finish the pagination tiebreaker, and state the tie premise correctly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The comment on the measurements list justified the tiebreaker with an Apple Health import stamping long runs with one instant, and said the unique index forbids two live measurements of the same type at that instant. The index is `(userId, type, measuredAt, source, sleepStage)`, so the named example is the one case it does block — while ties survive across `type` (the `stats:` daily rollups), across `source` (two writers landing together) and across `sleepStage`. Same conclusion, honest premise. The per-day drill-down on the same route still sorted on `measuredAt` alone. It slices in memory rather than with `skip`, so nothing drifts between pages, but two identical requests were free to render a tied block in different orders, and a list that reshuffles on refresh reads as data changing. Three of the tiebreakered routes had nothing pinning the order: custom-metric entries, the per-medication intake history (both sort arms, so the NULLS LAST pin cannot be lost to the tiebreaker) and the mental-health assessment history. Each now asserts the `orderBy` the way the illness day-log test does. --- .../[id]/entries/__tests__/route.test.ts | 10 ++++++- src/app/api/measurements/route.ts | 17 +++++++---- .../[id]/intake/__tests__/route.test.ts | 21 +++++++++++++ .../assessments/__tests__/route.test.ts | 30 +++++++++++++++++-- 4 files changed, 70 insertions(+), 8 deletions(-) diff --git a/src/app/api/custom-metrics/[id]/entries/__tests__/route.test.ts b/src/app/api/custom-metrics/[id]/entries/__tests__/route.test.ts index d02cd4a08..c13967317 100644 --- a/src/app/api/custom-metrics/[id]/entries/__tests__/route.test.ts +++ b/src/app/api/custom-metrics/[id]/entries/__tests__/route.test.ts @@ -90,10 +90,18 @@ describe("GET /api/custom-metrics/[id]/entries", () => { expect(body.data.meta.total).toBe(5); expect(body.data.meta.limit).toBe(1); const findCall = vi.mocked(prisma.customMetricEntry.findMany).mock - .calls[0][0] as { where: Record; take: number }; + .calls[0][0] as { + where: Record; + take: number; + orderBy: unknown; + }; expect(findCall.where.userId).toBe("user-1"); expect(findCall.where.customMetricId).toBe("cm-1"); expect(findCall.take).toBe(1); + // `measuredAt` is not unique on a custom metric — a batch of readings can + // share one instant — so offset paging over it alone can repeat a row on + // one page and drop another. + expect(findCall.orderBy).toEqual([{ measuredAt: "desc" }, { id: "desc" }]); }); it("404s a cross-user / unknown metric", async () => { diff --git a/src/app/api/measurements/route.ts b/src/app/api/measurements/route.ts index 87b22745d..6dcfe5456 100644 --- a/src/app/api/measurements/route.ts +++ b/src/app/api/measurements/route.ts @@ -190,7 +190,12 @@ export const GET = apiHandler(async (request: NextRequest) => { measuredAt: { gte: dayStart, lt: dayEnd }, deletedAt: null, }, - orderBy: { measuredAt: sortDir }, + // Same tiebreaker as the paged list. This one slices in memory rather + // than with `skip`, so there is no page drift to prevent — but two + // identical requests would still be free to render a tied block in + // different orders, and a drill-down that reshuffles on refresh reads + // as data changing. + orderBy: [{ measuredAt: sortDir }, { id: sortDir }], // v1.4.38 — the 1000-row cap now lives on the validator (refine // on `(limit, dayKey)` returning 422 when a caller asks for // more). The route reads `limit` straight through because the @@ -545,10 +550,12 @@ export const GET = apiHandler(async (request: NextRequest) => { prisma.measurement.findMany({ where, // `{ id: sortDir }` is the tiebreaker, not decoration. `measuredAt` - // is not unique here and ties are the normal case — an Apple Health - // import stamps long runs with one instant, and the - // `stats::YYYY-MM-DD` daily-rollup convention puts every daily - // total at the same one. Without a unique secondary key Postgres is + // is not unique here and ties are the normal case. Only the exact + // tuple `(userId, type, measuredAt, source, sleepStage)` is unique, + // so a tie survives across `type` (the `stats::YYYY-MM-DD` + // daily-rollup convention puts every daily total at one instant), + // across `source` (two writers landing on the same instant) and + // across `sleepStage`. Without a unique secondary key Postgres is // free to order the tied block differently between the query for page // N and the query for page N+1, so a client paging its own history // silently gets one row twice and never gets another. diff --git a/src/app/api/medications/[id]/intake/__tests__/route.test.ts b/src/app/api/medications/[id]/intake/__tests__/route.test.ts index 2555f8d7b..60569dcb6 100644 --- a/src/app/api/medications/[id]/intake/__tests__/route.test.ts +++ b/src/app/api/medications/[id]/intake/__tests__/route.test.ts @@ -208,6 +208,27 @@ describe("GET /api/medications/[id]/intake — status filter", () => { }); }); + it("orders on a unique tiebreaker, keeping NULLS LAST on the takenAt arm", async () => { + // Scheduled slots share an instant by construction — a twice-daily + // medication puts every morning dose on the same `scheduledFor` — so + // offset paging over the sort key alone can repeat a row on one page and + // drop another. The `takenAt` arm keeps its NULLS LAST pin as well: the + // tiebreaker must not push skipped rows back to the top. + await GET(makeRequest("sortBy=takenAt&sortDir=desc"), ROUTE_PARAMS); + expect( + vi.mocked(prisma.medicationIntakeEvent.findMany).mock.calls[0][0] + ?.orderBy, + ).toEqual([{ takenAt: { sort: "desc", nulls: "last" } }, { id: "desc" }]); + + vi.mocked(prisma.medicationIntakeEvent.findMany).mockClear(); + + await GET(makeRequest("sortBy=scheduledFor&sortDir=asc"), ROUTE_PARAMS); + expect( + vi.mocked(prisma.medicationIntakeEvent.findMany).mock.calls[0][0] + ?.orderBy, + ).toEqual([{ scheduledFor: "asc" }, { id: "asc" }]); + }); + it("rejects unknown status values with a 422", async () => { const res = await GET(makeRequest("status=junk"), ROUTE_PARAMS); expect(res.status).toBe(422); diff --git a/src/app/api/mental-health/assessments/__tests__/route.test.ts b/src/app/api/mental-health/assessments/__tests__/route.test.ts index 1d17b58a0..b6311acfc 100644 --- a/src/app/api/mental-health/assessments/__tests__/route.test.ts +++ b/src/app/api/mental-health/assessments/__tests__/route.test.ts @@ -15,7 +15,11 @@ import { NextRequest } from "next/server"; vi.mock("@/lib/db", () => ({ prisma: { appSettings: { findUnique: vi.fn().mockResolvedValue(null) }, - mentalHealthAssessment: { create: vi.fn(), findFirst: vi.fn() }, + mentalHealthAssessment: { + create: vi.fn(), + findFirst: vi.fn(), + findMany: vi.fn(), + }, measurement: { create: vi.fn().mockResolvedValue({}) }, }, })); @@ -60,7 +64,7 @@ vi.mock("next/headers", () => ({ })), })); -import { POST } from "../route"; +import { GET, POST } from "../route"; import { enqueueReminderSatisfy } from "@/lib/jobs/reminder-satisfy"; import { getSession } from "@/lib/auth/session"; import { requireModuleEnabled } from "@/lib/modules/gate"; @@ -449,3 +453,25 @@ describe("POST /api/mental-health/assessments — external-id stability floor", expect(prisma.mentalHealthAssessment.create).toHaveBeenCalledTimes(1); }); }); + +describe("GET /api/mental-health/assessments", () => { + it("orders the history on a unique tiebreaker", async () => { + vi.mocked(prisma.mentalHealthAssessment.findMany).mockResolvedValue( + [] as never, + ); + + const res = await (GET as unknown as (r: NextRequest) => Promise)( + new NextRequest( + new URL("http://localhost/api/mental-health/assessments"), + ), + ); + expect(res.status).toBe(200); + + // `takenAt` is day-resolution, so two instruments answered in one sitting + // tie; without the unique secondary key offset paging can repeat a row on + // one page and drop another. + const findArgs = vi.mocked(prisma.mentalHealthAssessment.findMany).mock + .calls[0][0] as { orderBy: unknown }; + expect(findArgs.orderBy).toEqual([{ takenAt: "desc" }, { id: "desc" }]); + }); +}); From cf9e8b8790aac2c775d4cb16f0057ba43ad4e6c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Thu, 10 Sep 2026 05:07:08 +0200 Subject: [PATCH 12/13] docs(api): declare the 429 headers instead of only describing them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The shared 429 named `Retry-After` and the `X-RateLimit` triple in prose, so a generated client got no typed accessor for the field the same sentence tells it to back off on. They are a `headers` block now: `Retry-After` and the cap and remainder as integers, the reset instant as a date-time. The text also says what a 429 from a ceiling that is not one of these buckets carries — nothing, because there is no bucket to describe. Two route descriptions still listed the pre-change header set: the ingest 429 named only `X-RateLimit-Remaining` / `X-RateLimit-Reset`, and the document suggest 429 named only the reset instant beside `meta.retryAt`. Both are short of the wire in exactly the direction this work exists to close. --- docs/api/openapi.yaml | 41 ++++++++++++++++++++++------- src/lib/openapi/routes/documents.ts | 2 +- src/lib/openapi/routes/ingest.ts | 2 +- src/lib/openapi/routes/shared.ts | 24 ++++++++++++++++- 4 files changed, 57 insertions(+), 12 deletions(-) diff --git a/docs/api/openapi.yaml b/docs/api/openapi.yaml index ad23edaa4..5c9cfe846 100644 --- a/docs/api/openapi.yaml +++ b/docs/api/openapi.yaml @@ -112,10 +112,32 @@ paths: schema: $ref: "#/components/schemas/ErrorEnvelope" "429": &a5 - description: Rate limit exceeded. The response carries `Retry-After` (whole seconds, rounded up — wait at least that - long before retrying), `X-RateLimit-Limit` (the bucket's cap), `X-RateLimit-Remaining` and + description: Rate limit exceeded. The response carries `Retry-After` (whole seconds, rounded up and never below 1 — wait + at least that long before retrying), `X-RateLimit-Limit` (the bucket's cap), `X-RateLimit-Remaining` and `X-RateLimit-Reset` (the reset instant, ISO-8601, not epoch seconds). Back off on `Retry-After` rather than - guessing. + guessing. A 429 raised by a ceiling that is not one of these buckets — a daily AI budget, an upstream + provider's own refusal relayed onward — carries none of them, because there is no bucket to describe. + headers: + Retry-After: + description: Whole seconds to wait before retrying. Rounded up, never below 1. + schema: + type: integer + minimum: 1 + X-RateLimit-Limit: + description: Requests the bucket allows per window. + schema: + type: integer + minimum: 1 + X-RateLimit-Remaining: + description: Requests left in the current window; 0 on a refusal. + schema: + type: integer + minimum: 0 + X-RateLimit-Reset: + description: Instant the current window rolls over, ISO-8601 — not epoch seconds. + schema: + type: string + format: date-time content: application/json: schema: @@ -15432,9 +15454,10 @@ paths: returns a `{ title, kind, documentDate }` DRAFT for the edit form. AI-consent / rate / budget gated (shares the per-user document-AI bucket with extract/summary/index — default 6/hour, operator-tunable via `DOCUMENT_AI_LIMIT_PER_HOUR`; a slot is only consumed when the request reaches the provider, and the 429 carries - the reset instant in `X-RateLimit-Reset` and `meta.retryAt`). WRITES NOTHING — never stages facts, never flips - status; the user reviews and saves. 422 (`documents.inbound.providerUnsupported`) with no provider configured. - Never interprets or diagnoses; the title is a neutral filing label." + `Retry-After`, the `X-RateLimit-Limit` / `-Remaining` / `-Reset` triple, and `meta.retryAt`). WRITES NOTHING — + never stages facts, never flips status; the user reviews and saves. 422 + (`documents.inbound.providerUnsupported`) with no provider configured. Never interprets or diagnoses; the title + is a neutral filing label." parameters: - name: id in: path @@ -19768,9 +19791,9 @@ paths: schema: $ref: "#/components/schemas/ErrorEnvelope" "429": - description: More than 60 requests in a minute for this token — or, when no valid token was presented, from this client - IP. `meta.errorCode` = `ingest.rate_limited`, plus the standard `X-RateLimit-Remaining` / - `X-RateLimit-Reset` headers. + description: "More than 60 requests in a minute for this token — or, when no valid token was presented, from this client + IP. `meta.errorCode` = `ingest.rate_limited`, plus the standard rate-limit headers: `Retry-After` (whole + seconds, at least 1), `X-RateLimit-Limit`, `X-RateLimit-Remaining` and `X-RateLimit-Reset`." content: application/json: schema: diff --git a/src/lib/openapi/routes/documents.ts b/src/lib/openapi/routes/documents.ts index 81159f2b7..95d5d20d8 100644 --- a/src/lib/openapi/routes/documents.ts +++ b/src/lib/openapi/routes/documents.ts @@ -793,7 +793,7 @@ export const inboundDocumentPaths: NonNullable = { tags: ["Documents"], summary: "Suggest filing metadata (drafts only)", description: - 'Optional AI assist on an already-stored document. Runs ONE provider call over the stored original (VISION, empty body) or browser-OCR\'d text (TEXT, `application/json` `{ mode: "text", text }`, opt-in local OCR) and returns a `{ title, kind, documentDate }` DRAFT for the edit form. AI-consent / rate / budget gated (shares the per-user document-AI bucket with extract/summary/index — default 6/hour, operator-tunable via `DOCUMENT_AI_LIMIT_PER_HOUR`; a slot is only consumed when the request reaches the provider, and the 429 carries the reset instant in `X-RateLimit-Reset` and `meta.retryAt`). WRITES NOTHING — never stages facts, never flips status; the user reviews and saves. 422 (`documents.inbound.providerUnsupported`) with no provider configured. Never interprets or diagnoses; the title is a neutral filing label.', + 'Optional AI assist on an already-stored document. Runs ONE provider call over the stored original (VISION, empty body) or browser-OCR\'d text (TEXT, `application/json` `{ mode: "text", text }`, opt-in local OCR) and returns a `{ title, kind, documentDate }` DRAFT for the edit form. AI-consent / rate / budget gated (shares the per-user document-AI bucket with extract/summary/index — default 6/hour, operator-tunable via `DOCUMENT_AI_LIMIT_PER_HOUR`; a slot is only consumed when the request reaches the provider, and the 429 carries `Retry-After`, the `X-RateLimit-Limit` / `-Remaining` / `-Reset` triple, and `meta.retryAt`). WRITES NOTHING — never stages facts, never flips status; the user reviews and saves. 422 (`documents.inbound.providerUnsupported`) with no provider configured. Never interprets or diagnoses; the title is a neutral filing label.', parameters: [ { name: "id", in: "path", required: true, schema: { type: "string" } }, ], diff --git a/src/lib/openapi/routes/ingest.ts b/src/lib/openapi/routes/ingest.ts index fb2cfb643..216619427 100644 --- a/src/lib/openapi/routes/ingest.ts +++ b/src/lib/openapi/routes/ingest.ts @@ -92,7 +92,7 @@ export const ingestPaths: NonNullable = { }, "429": { description: - "More than 60 requests in a minute for this token — or, when no valid token was presented, from this client IP. `meta.errorCode` = `ingest.rate_limited`, plus the standard `X-RateLimit-Remaining` / `X-RateLimit-Reset` headers.", + "More than 60 requests in a minute for this token — or, when no valid token was presented, from this client IP. `meta.errorCode` = `ingest.rate_limited`, plus the standard rate-limit headers: `Retry-After` (whole seconds, at least 1), `X-RateLimit-Limit`, `X-RateLimit-Remaining` and `X-RateLimit-Reset`.", content: { "application/json": { schema: errorEnvelope } }, }, }, diff --git a/src/lib/openapi/routes/shared.ts b/src/lib/openapi/routes/shared.ts index 4018f4ab3..43b625a9e 100644 --- a/src/lib/openapi/routes/shared.ts +++ b/src/lib/openapi/routes/shared.ts @@ -239,8 +239,30 @@ export const stdResponses = { }, "429": { description: - "Rate limit exceeded. The response carries `Retry-After` (whole seconds, rounded up — wait at least that long before retrying), `X-RateLimit-Limit` (the bucket's cap), `X-RateLimit-Remaining` and `X-RateLimit-Reset` (the reset instant, ISO-8601, not epoch seconds). Back off on `Retry-After` rather than guessing.", + "Rate limit exceeded. The response carries `Retry-After` (whole seconds, rounded up and never below 1 — wait at least that long before retrying), `X-RateLimit-Limit` (the bucket's cap), `X-RateLimit-Remaining` and `X-RateLimit-Reset` (the reset instant, ISO-8601, not epoch seconds). Back off on `Retry-After` rather than guessing. A 429 raised by a ceiling that is not one of these buckets — a daily AI budget, an upstream provider's own refusal relayed onward — carries none of them, because there is no bucket to describe.", content: { "application/json": { schema: errorEnvelope } }, + // Declared rather than only described: a generated client gets a typed + // accessor for the field the description tells it to back off on. + headers: { + "Retry-After": { + description: + "Whole seconds to wait before retrying. Rounded up, never below 1.", + schema: { type: "integer" as const, minimum: 1 }, + }, + "X-RateLimit-Limit": { + description: "Requests the bucket allows per window.", + schema: { type: "integer" as const, minimum: 1 }, + }, + "X-RateLimit-Remaining": { + description: "Requests left in the current window; 0 on a refusal.", + schema: { type: "integer" as const, minimum: 0 }, + }, + "X-RateLimit-Reset": { + description: + "Instant the current window rolls over, ISO-8601 — not epoch seconds.", + schema: { type: "string" as const, format: "date-time" }, + }, + }, }, }; From 6f718121c7d59798bfde5cc8b0a16543e7e26e54 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Thu, 10 Sep 2026 05:07:15 +0200 Subject: [PATCH 13/13] test(api): use the real caps in the limiter mocks, and drop a stale pointer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three consent-surface tests mocked the shared AI-consent bucket at 60 where it is 20, and the analytics-read and MCP mocks named 60 against real caps of 120. Nothing asserts the number today, so nothing was wrong on the wire — but the mocked verdict now carries `limit` all the way to `X-RateLimit-Limit`, and the first test that reads that header off a mock would pin the fiction. The response-shape file's header pointed at a document that is not in the repo; it says what it means directly instead. It also now records the one thing the gate cannot see: `meta` is a loose object on purpose, so undeclared keys inside it survive the parse and the strip comparison does not reach them. --- src/app/api/consent/ai/__tests__/route.test.ts | 2 +- .../api/consent/ai/latest/__tests__/route.test.ts | 4 ++-- src/app/api/consent/ai/web/__tests__/route.test.ts | 2 +- .../insights/comprehensive/__tests__/route.test.ts | 2 +- .../insights/correlations/__tests__/route.test.ts | 4 ++-- src/app/mcp/__tests__/route.test.ts | 4 ++-- tests/integration/openapi-response-shape.test.ts | 13 ++++++++++--- 7 files changed, 19 insertions(+), 12 deletions(-) diff --git a/src/app/api/consent/ai/__tests__/route.test.ts b/src/app/api/consent/ai/__tests__/route.test.ts index 8f4a55859..4a9cb42f3 100644 --- a/src/app/api/consent/ai/__tests__/route.test.ts +++ b/src/app/api/consent/ai/__tests__/route.test.ts @@ -114,7 +114,7 @@ describe("POST /api/consent/ai", () => { vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); vi.mocked(checkConsentRateLimit).mockResolvedValue({ allowed: false, - limit: 60, + limit: 20, remaining: 0, resetAt: Date.now() + 60_000, }); diff --git a/src/app/api/consent/ai/latest/__tests__/route.test.ts b/src/app/api/consent/ai/latest/__tests__/route.test.ts index db3ed6ddb..b486eb6b7 100644 --- a/src/app/api/consent/ai/latest/__tests__/route.test.ts +++ b/src/app/api/consent/ai/latest/__tests__/route.test.ts @@ -104,7 +104,7 @@ describe("GET /api/consent/ai/latest", () => { vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); vi.mocked(checkConsentRateLimit).mockResolvedValue({ allowed: false, - limit: 60, + limit: 20, remaining: 0, resetAt: Date.now() + 60_000, }); @@ -203,7 +203,7 @@ describe("DELETE /api/consent/ai/latest", () => { vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); vi.mocked(checkConsentRateLimit).mockResolvedValue({ allowed: false, - limit: 60, + limit: 20, remaining: 0, resetAt: Date.now() + 60_000, }); diff --git a/src/app/api/consent/ai/web/__tests__/route.test.ts b/src/app/api/consent/ai/web/__tests__/route.test.ts index 6d5651727..baecb7a59 100644 --- a/src/app/api/consent/ai/web/__tests__/route.test.ts +++ b/src/app/api/consent/ai/web/__tests__/route.test.ts @@ -84,7 +84,7 @@ describe("POST /api/consent/ai/web", () => { vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); vi.mocked(checkConsentRateLimit).mockResolvedValue({ allowed: false, - limit: 60, + limit: 20, remaining: 0, resetAt: Date.now() + 60_000, }); diff --git a/src/app/api/insights/comprehensive/__tests__/route.test.ts b/src/app/api/insights/comprehensive/__tests__/route.test.ts index 6f776d239..e6dde951a 100644 --- a/src/app/api/insights/comprehensive/__tests__/route.test.ts +++ b/src/app/api/insights/comprehensive/__tests__/route.test.ts @@ -117,7 +117,7 @@ beforeEach(() => { // v1.15.20 — default to an allowing analytics-read budget. vi.mocked(checkAnalyticsReadRateLimit).mockResolvedValue({ allowed: true, - limit: 60, + limit: 120, remaining: 119, resetAt: Date.now() + 60_000, }); diff --git a/src/app/api/insights/correlations/__tests__/route.test.ts b/src/app/api/insights/correlations/__tests__/route.test.ts index bc481a4f4..aa9ddb6d4 100644 --- a/src/app/api/insights/correlations/__tests__/route.test.ts +++ b/src/app/api/insights/correlations/__tests__/route.test.ts @@ -103,7 +103,7 @@ beforeEach(() => { // v1.15.20 — default to an allowing analytics-read budget. vi.mocked(checkAnalyticsReadRateLimit).mockResolvedValue({ allowed: true, - limit: 60, + limit: 120, remaining: 119, resetAt: Date.now() + 60_000, }); @@ -230,7 +230,7 @@ describe("GET /api/insights/correlations", () => { vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); vi.mocked(checkAnalyticsReadRateLimit).mockResolvedValue({ allowed: false, - limit: 60, + limit: 120, remaining: 0, resetAt: Date.now() + 60_000, }); diff --git a/src/app/mcp/__tests__/route.test.ts b/src/app/mcp/__tests__/route.test.ts index 3780faff5..d132bd115 100644 --- a/src/app/mcp/__tests__/route.test.ts +++ b/src/app/mcp/__tests__/route.test.ts @@ -156,7 +156,7 @@ beforeEach(() => { vi.mocked(isApiGloballyEnabled).mockResolvedValue(true); vi.mocked(checkMcpRateLimit).mockResolvedValue({ allowed: true, - limit: 60, + limit: 120, remaining: 119, resetAt: Date.now() + 60_000, }); @@ -353,7 +353,7 @@ describe("/mcp — rate limit", () => { validToken(); vi.mocked(checkMcpRateLimit).mockResolvedValue({ allowed: false, - limit: 60, + limit: 120, remaining: 0, resetAt: Date.now() + 30_000, }); diff --git a/tests/integration/openapi-response-shape.test.ts b/tests/integration/openapi-response-shape.test.ts index 89ee4f550..59c640c99 100644 --- a/tests/integration/openapi-response-shape.test.ts +++ b/tests/integration/openapi-response-shape.test.ts @@ -8,7 +8,7 @@ * five domains against a three-domain contract while the body was invalid * against its own published schema. * - * Two responses are covered — the ones the audit found broken: + * Two responses are covered — the two that were broken: * * - `GET /api/sync/changes`, the multi-domain delta the native client mirrors * its whole history from. @@ -17,9 +17,16 @@ * * The check is stricter than `safeParse` alone. Zod strips undeclared keys * rather than refusing them, so a pass would say nothing about a field the - * server sends and the contract omits — exactly the direction both High - * findings drifted in. Comparing the parsed value back to the raw body catches + * server sends and the contract omits — exactly the direction both of those + * drifted in. Comparing the parsed value back to the raw body catches * the strip: if anything was dropped, the schema is short of the wire. + * + * One limit worth knowing before leaning on that half: `meta` is declared as a + * loose object, deliberately, because real refusals put `removedIn`, + * `replacedBy` and per-integration context there. Loose keys survive the parse, + * so the strip comparison does not reach inside `meta`. A `meta` field that + * ever becomes load-bearing for a client needs its own declaration and its own + * assertion. */ import { NextRequest } from "next/server"; import { beforeEach, describe, expect, it, vi } from "vitest";