diff --git a/docs/api/openapi.yaml b/docs/api/openapi.yaml index 380c0a6cb..8ac3d0181 100644 --- a/docs/api/openapi.yaml +++ b/docs/api/openapi.yaml @@ -396,9 +396,10 @@ paths: "409": *a7 "422": *a4 "429": &a12 - description: "Rate limit exceeded. This route shares one per-account bucket with the other single-record writes — + description: "Rate limit exceeded. This route shares one per-account bucket with every other authenticated write — `record-write:`, 300 requests per 60 seconds — keyed on the ACTING account, so a delegate burns - their own allowance rather than the record owner's. Nothing was written. `meta.errorCode` is + their own allowance rather than the record owner's. Only the bulk ingest endpoints, which meter themselves + with a dedicated bucket, stay outside it. Nothing was written. `meta.errorCode` is `record_write.rate_limited`, which is what tells this refusal apart from a route's own narrower bucket when both can answer 429. The `Retry-After`, `X-RateLimit-Limit`, `X-RateLimit-Remaining` and `X-RateLimit-Reset` headers describe that bucket; back off on `Retry-After` rather than guessing. Every request counts against diff --git a/src/__tests__/record-write-rate-limit-contract.test.ts b/src/__tests__/record-write-rate-limit-contract.test.ts index 3330c46c4..73a775e93 100644 --- a/src/__tests__/record-write-rate-limit-contract.test.ts +++ b/src/__tests__/record-write-rate-limit-contract.test.ts @@ -4,9 +4,11 @@ * The batch endpoints have been capped at 60 calls a minute since they were * written; the per-record creates a naive or older client loops over were not * capped at all, which is backwards — the batch endpoint is the one a - * well-behaved client uses. Eleven of them share one generous per-account - * bucket now, and the operations publish its name and its cap so a client that - * meets the ceiling can read which one it met. + * well-behaved client uses. Every authenticated write now shares one generous + * per-account bucket, charged where the caller is resolved + * (`src/lib/auth/write-meter.ts`), and the operations listed below publish + * its name and its cap so a client that meets the ceiling can read which one + * it met. * * A published number is only worth reading if it cannot drift from the code, so * this pulls the bucket key and the limit out of `prisma.$queryRaw` — the same @@ -15,9 +17,9 @@ * would prove nothing: the sentence is written by hand, deliberately, and this * is what keeps the hand-written half honest. * - * The route half is structural: every route in the list below calls the shared - * helper. Its limit is that it reads the file rather than the running handler, - * so a call placed after an early return would still count as present. + * The route half is structural: every route in the list below resolves its + * caller through a metering resolver and does not opt out. Its limit is that + * it reads the file rather than the running handler. */ import { readFileSync } from "node:fs"; import { join } from "node:path"; @@ -32,6 +34,7 @@ import { } from "@/lib/rate-limit"; import { buildOpenApiDocument } from "@/lib/openapi/registry"; import { recordWriteRateLimitResponse } from "@/lib/openapi/routes/shared"; +import { RECORD_WRITE_RATE_LIMITED_CODE } from "@/lib/auth/write-meter"; vi.mock("@/lib/db", () => ({ prisma: { $queryRaw: vi.fn() } })); @@ -121,24 +124,27 @@ describe("the shared single-record write bucket", () => { expect(published).toContain("Retry-After"); }); - it("is called by every route that publishes it", () => { - const missing = ON_THE_SHARED_BUCKET.filter(([route]) => { + it("is charged on the path every route that publishes it takes", () => { + // The bucket is charged by the identity resolvers (`meterAccountWrite`), + // not by each route, so a listed route meets it by resolving its caller + // through one of them and not opting out. `write-rate-limit-guard` + // holds every other mutating route to the same rule. + const unmetered = ON_THE_SHARED_BUCKET.filter(([route]) => { const src = readFileSync(join(process.cwd(), route), "utf8"); - return !src.includes("checkRecordWriteRateLimit("); + return ( + !/\brequire(?:Record)?Auth\s*\(/.test(src) || + /\bwriteBucket\s*:\s*["']own["']/.test(src) + ); }).map(([route]) => route); - expect(missing).toEqual([]); + expect(unmetered).toEqual([]); }); - it("names the code it publishes at every site", () => { - // The prose is not the discriminator: two of these routes consult a second - // bucket of their own, and a client that can only read `error` cannot tell - // which one refused it. - const silent = ON_THE_SHARED_BUCKET.filter(([route]) => { - const src = readFileSync(join(process.cwd(), route), "utf8"); - return !src.includes('errorCode: "record_write.rate_limited"'); - }).map(([route]) => route); - expect(silent).toEqual([]); - expect(published).toContain("record_write.rate_limited"); + it("names the code it publishes", () => { + // The prose is not the discriminator: several of these routes consult a + // second bucket of their own, and a client that can only read `error` + // cannot tell which one refused it. + expect(RECORD_WRITE_RATE_LIMITED_CODE).toBe("record_write.rate_limited"); + expect(published).toContain(RECORD_WRITE_RATE_LIMITED_CODE); }); it("is published on every operation that answers it", () => { diff --git a/src/__tests__/write-rate-limit-guard.test.ts b/src/__tests__/write-rate-limit-guard.test.ts new file mode 100644 index 000000000..a579655ee --- /dev/null +++ b/src/__tests__/write-rate-limit-guard.test.ts @@ -0,0 +1,220 @@ +/** + * Structural guard: every mutating route meets a rate limit. + * + * 169 of 364 mutating handlers met no limiter at all: every PATCH and DELETE + * on an `[id]`, and most single-record creates. The shared per-account write + * bucket now sits on the identity resolvers (`meterAccountWrite`, + * `src/lib/auth/write-meter.ts`), so a route that resolves its caller through + * one of them is metered without knowing it. This guard is what keeps that + * true for the routes added next year: + * + * - a route file exporting POST, PUT, PATCH or DELETE must either resolve + * its caller through a metering resolver, call a limiter of its own, or + * stand in {@link UNMETERED} with the reason; + * - a route that opts out of the shared bucket (`writeBucket: "own"`) must + * stand in {@link OWN_BUCKET} with the reason AND call its own limiter, so + * the opt-out can never mean "no limit"; + * - a helper that limits on a route's behalf is accepted only while its own + * source calls a limiter. + * + * The routes are derived from the tree, never listed by hand. The check is + * per file, not per handler: a file whose GET resolves through a metering + * resolver while its POST does not would pass. Comments are stripped first; + * the planted cases at the top prove the matchers fire. + */ +import { readFileSync } from "node:fs"; +import { join } from "node:path"; +import { describe, expect, it } from "vitest"; + +import { walkSourceFiles } from "./helpers/source-files"; + +const SRC = join(process.cwd(), "src"); + +/** The resolvers in `api-handler.ts` that charge the shared write bucket. */ +const METERING_RESOLVERS = [ + "requireAuth", + "requireRecordAuth", + "requireActorAuth", + "requireGuardianAuth", + "requireAdmin", + "requireCookieAuth", + "requireFreshMfa", + "requireFreshMfaIfEnrolled", + "requireFreshMfaOrElevationIfEnrolled", + "requireRecentProof", + "requireBearerAuth", + "requireMfaManagementAuth", +]; + +/** Helpers that call a limiter on the route's behalf, and where they live. */ +const LIMITING_HELPERS: Record = { + claimGate: ["lib/managed-profiles/claim-gate.ts"], + applyWebhookRateLimit: [ + "lib/withings/webhook-handler.ts", + "lib/whoop/webhook-handler.ts", + ], +}; + +/** Mutating routes with no limit, each with the reason it needs none. */ +const UNMETERED: Record = { + "app/api/auth/logout/route.ts": + "Ends the caller's own session or token and nothing else. It is idempotent, writes no record data, and refusing it would keep alive a credential its holder asked to end.", +}; + +/** Routes metered by a dedicated bucket instead of the shared one. */ +const OWN_BUCKET: Record = { + "app/api/measurements/batch/route.ts": + "The HealthKit uploader sends up to 60 batches of 500 a minute, and an outbox replay of parked pages bypasses the client pacer; the route's own 60-a-minute batch bucket meters it.", + "app/api/workouts/batch/route.ts": + "The workout importer sends page after page during a catch-up pass; the route's own batch bucket meters it.", + "app/api/medications/intake/bulk/route.ts": + "The Apple Health dose import and the standalone adoption send 200 to 500 intakes per request in sequence; the route's own bulk bucket meters it.", + "app/api/mood-entries/bulk/route.ts": + "The standalone adoption sends 200 entries per request in sequence; the route's own bulk bucket meters it.", + "app/api/cycle/day-logs/bulk/route.ts": + "The cycle HealthKit sweep sends 500-day slices in sequence; the route's own bulk bucket meters it.", + "app/api/nutrients/batch/route.ts": + "The nutrient sync sends 500-row batches in sequence; the route's own batch bucket meters it.", + "app/api/insights/ecg/route.ts": + "The ECG sweep sends one recording per request with no client-side delay, newest first, over the whole HealthKit history; the route's own 600-a-minute ingest bucket meters it.", +}; + +function stripComments(text: string): string { + return text.replace(/\/\*[\s\S]*?\*\//g, "").replace(/^\s*\/\/.*$/gm, ""); +} + +const WRITES = + /export\s+(?:const|async\s+function)\s+(POST|PUT|PATCH|DELETE)\b/; +const OWN_LIMITER = /\b(?:check\w*RateLimit|checkRateLimit)\s*\(/; +const OPTS_OUT = /\bwriteBucket\s*:\s*["']own["']/; + +function callsAny(code: string, names: readonly string[]): boolean { + return names.some((name) => new RegExp(`\\b${name}\\s*\\(`).test(code)); +} + +function limited(code: string): boolean { + return ( + OWN_LIMITER.test(code) || callsAny(code, Object.keys(LIMITING_HELPERS)) + ); +} + +describe("every mutating route meets a rate limit", () => { + const files = walkSourceFiles(SRC, { floor: 3000 }).filter( + (file) => !file.startsWith("generated/") && !file.includes("__tests__/"), + ); + const read = (file: string) => + stripComments(readFileSync(join(SRC, file), "utf8")); + const mutating = files + .filter((f) => /^app\/.+\/route\.tsx?$/.test(f)) + .filter((f) => WRITES.test(read(f))); + + it("the matchers fire on planted sources", () => { + expect(WRITES.test("export const PATCH = apiHandler(async () => {")).toBe( + true, + ); + expect(WRITES.test("export async function DELETE(req) {")).toBe(true); + expect(WRITES.test("export const GET = apiHandler(async () => {")).toBe( + false, + ); + expect( + callsAny( + 'const { user } = await requireRecordAuth("write", "labs");', + METERING_RESOLVERS, + ), + ).toBe(true); + // A resolver named in a comment or an import does not count. + expect( + callsAny( + stripComments( + "import { requireAuth } from 'x';\n// requireAuth() here", + ), + METERING_RESOLVERS, + ), + ).toBe(false); + expect(OPTS_OUT.test('requireAuth(SCOPE, { writeBucket: "own" })')).toBe( + true, + ); + expect(limited("const rl = await checkRateLimit(`k:${id}`, 5, 60)")).toBe( + true, + ); + expect(limited("const rl = rateLimitHeaders(x)")).toBe(false); + }); + + it("finds the mutating routes", () => { + expect(mutating.length).toBeGreaterThanOrEqual(300); + }); + + it("meters every mutating route, or names why not", () => { + const unlimited = mutating.filter((f) => { + if (f in UNMETERED) return false; + const code = read(f); + const metered = + callsAny(code, METERING_RESOLVERS) && !OPTS_OUT.test(code); + return !metered && !limited(code); + }); + expect(unlimited).toEqual([]); + }); + + it("holds every opt-out to the reviewed list, and each to its own limiter", () => { + const optedOut = mutating.filter((f) => OPTS_OUT.test(read(f))).sort(); + expect(optedOut).toEqual(Object.keys(OWN_BUCKET).sort()); + const withoutOwn = optedOut.filter((f) => !OWN_LIMITER.test(read(f))); + expect(withoutOwn).toEqual([]); + }); + + it("accepts a limiting helper only while it calls a limiter", () => { + for (const [helper, homes] of Object.entries(LIMITING_HELPERS)) { + for (const home of homes) { + const code = read(home); + expect(code, home).toMatch( + new RegExp(`export\\s+async\\s+function\\s+${helper}\\b`), + ); + expect(OWN_LIMITER.test(code), home).toBe(true); + } + } + }); + + it("keeps the exceptions to routes that exist and still need them", () => { + for (const route of Object.keys(UNMETERED)) { + expect(mutating).toContain(route); + const code = read(route); + expect(callsAny(code, METERING_RESOLVERS) || limited(code), route).toBe( + false, + ); + } + }); + + it("charges the bucket in each metering resolver", () => { + const handler = read("lib/api-handler.ts"); + for (const name of [ + "requireAuth", + "requireActorAuth", + "requireRecordAuth", + "requireGuardianAuth", + "requireAdmin", + "requireCookieAuth", + "requireFreshMfa", + "requireBearerAuth", + ]) { + const start = handler.search( + new RegExp(`export\\s+async\\s+function\\s+${name}\\s*\\(`), + ); + expect(start, name).toBeGreaterThanOrEqual(0); + const next = handler.indexOf("\nexport ", start + 1); + const body = handler.slice(start, next === -1 ? undefined : next); + expect(body, name).toMatch(/\bmeterAccountWrite\s*\(/); + } + // The three that only delegate reach the meter through `requireAuth`. + for (const name of [ + "requireFreshMfaIfEnrolled", + "requireFreshMfaOrElevationIfEnrolled", + "requireRecentProof", + ]) { + const start = handler.search( + new RegExp(`export\\s+async\\s+function\\s+${name}\\s*\\(`), + ); + const next = handler.indexOf("\nexport ", start + 1); + expect(handler.slice(start, next), name).toMatch(/\brequireAuth\s*\(/); + } + }); +}); diff --git a/src/app/api/allergies/route.ts b/src/app/api/allergies/route.ts index 5f5436ef8..57a6b0153 100644 --- a/src/app/api/allergies/route.ts +++ b/src/app/api/allergies/route.ts @@ -15,13 +15,11 @@ import { apiHandler, requireRecordAuth } from "@/lib/api-handler"; import { annotate } from "@/lib/logging/context"; import { auditLog } from "@/lib/auth/audit"; import { - apiError, apiSuccess, getClientIp, returnAllZodIssues, safeJson, } from "@/lib/api-response"; -import { checkRecordWriteRateLimit } from "@/lib/rate-limit"; import { withIdempotency } from "@/lib/idempotency"; import { encryptToBytes } from "@/lib/ai/coach/bytes-codec"; import { @@ -92,17 +90,7 @@ async function postAllergy(request: NextRequest): Promise { // A READ or WRITE delegate reaches neither: the destination is not on their // Settings list, and this line refuses them with `sharing.access.denied` // regardless. - const { user, actor } = await requireRecordAuth("manage", "profile"); - - // Shared per-account write ceiling — see `checkRecordWriteRateLimit`. The - // batch siblings have always been capped; the per-record creates a looping - // client hits were not. - const writeRl = await checkRecordWriteRateLimit(actor.id); - if (!writeRl.allowed) { - return apiError("Too many writes, try again later", 429, { - errorCode: "record_write.rate_limited", - }); - } + const { user } = await requireRecordAuth("manage", "profile"); const { data: rawBody, error: jsonError } = await safeJson(request, { maxBytes: 16 * 1024, diff --git a/src/app/api/biomarkers/[id]/merge/route.ts b/src/app/api/biomarkers/[id]/merge/route.ts index ea266a552..3c0e55465 100644 --- a/src/app/api/biomarkers/[id]/merge/route.ts +++ b/src/app/api/biomarkers/[id]/merge/route.ts @@ -31,7 +31,6 @@ import { invalidateUserHealthScore } from "@/lib/cache/invalidate"; import { withIdempotency } from "@/lib/idempotency"; import { mergeBiomarkers as runMerge } from "@/lib/labs/biomarker-merge"; import { annotate } from "@/lib/logging/context"; -import { checkRecordWriteRateLimit } from "@/lib/rate-limit"; import { destroyedDetails } from "@/lib/sharing/audit-details"; import { biomarkerMergeSchema } from "@/lib/validations/biomarker-merge"; @@ -45,16 +44,9 @@ async function mergeBiomarkers( request: NextRequest, { params }: RouteParams, ): Promise { - const { user, actor } = await requireRecordAuth("manage", "labs"); + const { user } = await requireRecordAuth("manage", "labs"); const { id } = await params; - const writeRl = await checkRecordWriteRateLimit(actor.id); - if (!writeRl.allowed) { - return apiError("Too many writes, try again later", 429, { - errorCode: "record_write.rate_limited", - }); - } - const { data: body, error: jsonError } = await safeJson(request, { maxBytes: 4 * 1024, }); diff --git a/src/app/api/biomarkers/route.ts b/src/app/api/biomarkers/route.ts index 2ff94ce4f..a68d39152 100644 --- a/src/app/api/biomarkers/route.ts +++ b/src/app/api/biomarkers/route.ts @@ -9,7 +9,6 @@ import { safeJson, sanitiseZodIssues, } from "@/lib/api-response"; -import { checkRecordWriteRateLimit } from "@/lib/rate-limit"; import { auditLog } from "@/lib/auth/audit"; import { prisma } from "@/lib/db"; import { withIdempotency } from "@/lib/idempotency"; @@ -76,17 +75,7 @@ async function postBiomarker(request: NextRequest) { // duplicate pre-check and the `@@unique([userId, name])` backstop both scope // to the same resolved id, so a delegate adding a marker the owner already // tracks gets the ordinary 409 rather than a second copy. - const { user, actor } = await requireRecordAuth("write", "labs"); - - // Shared per-account write ceiling — see `checkRecordWriteRateLimit`. The - // batch siblings have always been capped; the per-record creates a looping - // client hits were not. - const writeRl = await checkRecordWriteRateLimit(actor.id); - if (!writeRl.allowed) { - return apiError("Too many writes, try again later", 429, { - errorCode: "record_write.rate_limited", - }); - } + const { user } = await requireRecordAuth("write", "labs"); const { data: body, error: jsonError } = await safeJson(request, { maxBytes: 16 * 1024, diff --git a/src/app/api/custom-metrics/[id]/entries/route.ts b/src/app/api/custom-metrics/[id]/entries/route.ts index bb4477ede..2e1832ad5 100644 --- a/src/app/api/custom-metrics/[id]/entries/route.ts +++ b/src/app/api/custom-metrics/[id]/entries/route.ts @@ -9,7 +9,6 @@ import { safeJson, sanitiseZodIssues, } from "@/lib/api-response"; -import { checkRecordWriteRateLimit } from "@/lib/rate-limit"; import { auditLog } from "@/lib/auth/audit"; import { invalidateUserCorrelationPatterns } from "@/lib/cache/invalidate"; import { serialiseCustomMetricEntry } from "@/lib/custom-metrics/custom-metric-store"; @@ -108,15 +107,6 @@ async function postCustomMetricEntry( // was never wrong, and the GET arm above stays delegable meanwhile. const { user } = await requireAuth(); - // Shared per-account write ceiling — see `checkRecordWriteRateLimit`. The - // batch siblings have always been capped; the per-record creates a looping - // client hits were not. - const writeRl = await checkRecordWriteRateLimit(user.id); - if (!writeRl.allowed) { - return apiError("Too many writes, try again later", 429, { - errorCode: "record_write.rate_limited", - }); - } const { id } = await params; const metric = await prisma.customMetric.findFirst({ diff --git a/src/app/api/cycle/day-logs/bulk/route.ts b/src/app/api/cycle/day-logs/bulk/route.ts index 66abea65a..b005cfa7f 100644 --- a/src/app/api/cycle/day-logs/bulk/route.ts +++ b/src/app/api/cycle/day-logs/bulk/route.ts @@ -57,7 +57,10 @@ interface EntryResult { export const POST = apiHandler(withIdempotency<[NextRequest]>(postBulk)); async function postBulk(request: NextRequest): Promise { - const { user } = await requireAuth(); + // Metered by this route's own bulk bucket below, not the shared per-account + // write bucket: the native client syncs through here in bursts (see + // `meterAccountWrite`). + const { user } = await requireAuth(undefined, { writeBucket: "own" }); const gate = await requireCycleEnabled(user.id, user.gender); if (!gate.enabled) return gate.response; diff --git a/src/app/api/cycle/day-logs/route.ts b/src/app/api/cycle/day-logs/route.ts index 158d85c84..3f776a0bc 100644 --- a/src/app/api/cycle/day-logs/route.ts +++ b/src/app/api/cycle/day-logs/route.ts @@ -26,7 +26,6 @@ import { returnAllZodIssues, safeJson, } from "@/lib/api-response"; -import { checkRecordWriteRateLimit } from "@/lib/rate-limit"; import { withIdempotency } from "@/lib/idempotency"; import { requireCycleEnabled } from "@/lib/cycle/gate"; import { @@ -95,21 +94,11 @@ async function postDayLog(request: NextRequest): Promise { // v1.37.0 — MANAGE. The upsert replaces the day, which is what edit means // here; the gate below resolves `user.gender` against the RECORD, so the // module follows the record rather than the caller. - const { user, actor } = await requireRecordAuth("manage", "cycle"); + const { user } = await requireRecordAuth("manage", "cycle"); const gate = await requireCycleEnabled(user.id, user.gender); if (!gate.enabled) return gate.response; - // Shared per-account write ceiling — see `checkRecordWriteRateLimit`. The - // batch siblings have always been capped; the per-record creates a looping - // client hits were not. - const writeRl = await checkRecordWriteRateLimit(actor.id); - if (!writeRl.allowed) { - return apiError("Too many writes, try again later", 429, { - errorCode: "record_write.rate_limited", - }); - } - const { data: rawBody, error: jsonError } = await safeJson(request, { maxBytes: 64 * 1024, }); diff --git a/src/app/api/encounters/route.ts b/src/app/api/encounters/route.ts index f44a047f1..5686620b2 100644 --- a/src/app/api/encounters/route.ts +++ b/src/app/api/encounters/route.ts @@ -25,7 +25,6 @@ import { returnAllZodIssues, safeJson, } from "@/lib/api-response"; -import { checkRecordWriteRateLimit } from "@/lib/rate-limit"; import { apiListSuccess } from "@/lib/api/list-cap"; import { combinedCap, listPage } from "@/lib/http/list-query"; import { withIdempotency } from "@/lib/idempotency"; @@ -167,17 +166,7 @@ export const GET = apiHandler(async (request: NextRequest) => { export const POST = apiHandler(withIdempotency<[NextRequest]>(postEncounter)); async function postEncounter(request: NextRequest): Promise { - const { user, actor, grantId } = await requireRecordAuth("write", "profile"); - - // Shared per-account write ceiling — see `checkRecordWriteRateLimit`. The - // batch siblings have always been capped; the per-record creates a looping - // client hits were not. - const writeRl = await checkRecordWriteRateLimit(actor.id); - if (!writeRl.allowed) { - return apiError("Too many writes, try again later", 429, { - errorCode: "record_write.rate_limited", - }); - } + const { user, grantId } = await requireRecordAuth("write", "profile"); const { data: rawBody, error: jsonError } = await safeJson(request, { maxBytes: 32 * 1024, diff --git a/src/app/api/insights/ecg/route.ts b/src/app/api/insights/ecg/route.ts index 83db67783..4a9cfd219 100644 --- a/src/app/api/insights/ecg/route.ts +++ b/src/app/api/insights/ecg/route.ts @@ -203,7 +203,10 @@ export const GET = apiHandler(async () => { * to the same row by construction rather than by a cached response. */ export const POST = apiHandler(async (request: NextRequest) => { - const { user } = await requireAuth(); + // Metered by this route's own bulk bucket below, not the shared per-account + // write bucket: the native client syncs through here in bursts (see + // `meterAccountWrite`). + const { user } = await requireAuth(undefined, { writeBucket: "own" }); // A device ingest carries no model text: switching AI off, or the AI // analysis opt-out, must never refuse a recording. diff --git a/src/app/api/labs/ocr/commit/__tests__/route.test.ts b/src/app/api/labs/ocr/commit/__tests__/route.test.ts index b855c1fd6..435dd77eb 100644 --- a/src/app/api/labs/ocr/commit/__tests__/route.test.ts +++ b/src/app/api/labs/ocr/commit/__tests__/route.test.ts @@ -33,6 +33,12 @@ vi.mock("@/lib/rate-limit", async (importOriginal) => ({ resetAt: new Date(Date.now() + 60_000), })), })); +// The real meter, so the ceiling is proven on the path it now takes: the route +// resolves its caller, the resolver charges the bucket. The unit setup stubs +// the meter for every other route test. +vi.mock("@/lib/auth/write-meter", async (importOriginal) => + importOriginal(), +); const tzMock = vi.hoisted(() => ({ zone: "UTC" })); vi.mock("@/lib/tz/resolver", async (importOriginal) => ({ ...(await importOriginal()), @@ -216,7 +222,7 @@ describe("POST /api/labs/ocr/commit dedup across the user's day", () => { }); describe("POST /api/labs/ocr/commit write ceiling", () => { - it("charges the shared per-account record-write bucket", async () => { + it("charges the shared per-account record-write bucket where the caller is resolved", async () => { const res = await POST(postReq({ rows: [numericRow(95)] })); expect(res.status).toBe(200); expect(checkRecordWriteRateLimit).toHaveBeenCalledWith("user-1"); diff --git a/src/app/api/labs/ocr/commit/route.ts b/src/app/api/labs/ocr/commit/route.ts index 3dd653fc7..b9bd332f0 100644 --- a/src/app/api/labs/ocr/commit/route.ts +++ b/src/app/api/labs/ocr/commit/route.ts @@ -20,7 +20,6 @@ import { fireAndForget } from "@/lib/logging/fire-and-forget"; import { apiHandler, requireAuth } from "@/lib/api-handler"; import { - apiError, apiSuccess, getClientIp, returnAllZodIssues, @@ -31,7 +30,6 @@ import { auditLog } from "@/lib/auth/audit"; import { prisma } from "@/lib/db"; import { invalidateUserHealthScore } from "@/lib/cache/invalidate"; import { withIdempotency } from "@/lib/idempotency"; -import { checkRecordWriteRateLimit } from "@/lib/rate-limit"; import { enqueueReminderSatisfy } from "@/lib/jobs/reminder-satisfy"; import { emitDataArrival } from "@/lib/arrivals/emit-shared"; import { rememberImportAlias } from "@/lib/labs/biomarker-aliases"; @@ -159,17 +157,6 @@ function inBatchKey( async function commitOcrRows(request: NextRequest) { const { user } = await requireAuth(); - // Shared per-account write ceiling, the same bucket every other lab write - // charges (`POST /api/labs`, `POST /api/biomarkers`). A commit can write a - // whole report's rows and mint markers on the way, so it was the one lab - // write a looping client could repeat without any ceiling. - const writeRl = await checkRecordWriteRateLimit(user.id); - if (!writeRl.allowed) { - return apiError("Too many writes, try again later", 429, { - errorCode: "record_write.rate_limited", - }); - } - const { data: body, error: jsonError } = await safeJson(request, { maxBytes: 256 * 1024, }); diff --git a/src/app/api/labs/route.ts b/src/app/api/labs/route.ts index 60d4dcfbc..31bd837a1 100644 --- a/src/app/api/labs/route.ts +++ b/src/app/api/labs/route.ts @@ -10,7 +10,6 @@ import { safeJson, sanitiseZodIssues, } from "@/lib/api-response"; -import { checkRecordWriteRateLimit } from "@/lib/rate-limit"; import { auditLog } from "@/lib/auth/audit"; import { prisma } from "@/lib/db"; import { invalidateUserHealthScore } from "@/lib/cache/invalidate"; @@ -156,17 +155,7 @@ async function postLabResult(request: NextRequest) { // it mints it under `user.id`: the marker joins the record's catalogue, not // the caller's, which is the only reading of "add a result to your record" // that leaves the owner with a usable catalogue afterwards. - const { user, actor } = await requireRecordAuth("write", "labs"); - - // Shared per-account write ceiling — see `checkRecordWriteRateLimit`. The - // batch siblings have always been capped; the per-record creates a looping - // client hits were not. - const writeRl = await checkRecordWriteRateLimit(actor.id); - if (!writeRl.allowed) { - return apiError("Too many writes, try again later", 429, { - errorCode: "record_write.rate_limited", - }); - } + const { user } = await requireRecordAuth("write", "labs"); const { data: body, error: jsonError } = await safeJson(request, { maxBytes: 16 * 1024, diff --git a/src/app/api/life-events/route.ts b/src/app/api/life-events/route.ts index cdb0d32a0..3d9dd4ee4 100644 --- a/src/app/api/life-events/route.ts +++ b/src/app/api/life-events/route.ts @@ -21,7 +21,6 @@ import { NextRequest } from "next/server"; import { apiHandler, requireAuth } from "@/lib/api-handler"; import { - apiError, apiSuccess, getClientIp, returnAllZodIssues, @@ -36,7 +35,6 @@ import { toLifeEventDTO, } from "@/lib/life-events/service"; import { annotate } from "@/lib/logging/context"; -import { checkRecordWriteRateLimit } from "@/lib/rate-limit"; export const GET = apiHandler(async () => { const { user } = await requireAuth(); @@ -53,13 +51,6 @@ export const POST = apiHandler(withIdempotency<[NextRequest]>(postLifeEvent)); async function postLifeEvent(request: NextRequest): Promise { const { user } = await requireAuth(); - const writeRl = await checkRecordWriteRateLimit(user.id); - if (!writeRl.allowed) { - return apiError("Too many writes, try again later", 429, { - errorCode: "record_write.rate_limited", - }); - } - const { data: rawBody, error: jsonError } = await safeJson(request, { maxBytes: 16 * 1024, }); diff --git a/src/app/api/measurements/batch/route.ts b/src/app/api/measurements/batch/route.ts index f6e04823f..501b03fea 100644 --- a/src/app/api/measurements/batch/route.ts +++ b/src/app/api/measurements/batch/route.ts @@ -293,7 +293,12 @@ async function postBatch(request: NextRequest): Promise { // a native login mints. `requireAuth` refuses any acting-account carrier for // every credential shape, so the scoped token is confined to its owner's own // record here without this route adding an arm of its own. - const auth = await requireAuth(MEASUREMENTS_WRITE_SCOPE); + // Metered by this route's own bulk bucket below, not the shared per-account + // write bucket: the native client syncs through here in bursts (see + // `meterAccountWrite`). + const auth = await requireAuth(MEASUREMENTS_WRITE_SCOPE, { + writeBucket: "own", + }); const { user } = auth; // A scoped credential is a bridge — a scale, a home-automation rule — and not diff --git a/src/app/api/measurements/route.ts b/src/app/api/measurements/route.ts index e707e63d9..655666a85 100644 --- a/src/app/api/measurements/route.ts +++ b/src/app/api/measurements/route.ts @@ -17,7 +17,6 @@ import { safeJson, sanitiseZodIssues, } from "@/lib/api-response"; -import { checkRecordWriteRateLimit } from "@/lib/rate-limit"; import { createMeasurementSchema, createBatchMeasurementSchema, @@ -403,16 +402,6 @@ async function postMeasurement(request: NextRequest) { const { user } = auth; const scoped = isScopedCredential(auth); - // Shared per-account write ceiling — see `checkRecordWriteRateLimit`. The - // batch siblings have always been capped; the per-record creates a looping - // client hits were not. - const writeRl = await checkRecordWriteRateLimit(auth.actor.id); - if (!writeRl.allowed) { - return apiError("Too many writes, try again later", 429, { - errorCode: "record_write.rate_limited", - }); - } - const { data: body, error: jsonError } = await safeJson(request, { maxBytes: 64 * 1024, }); diff --git a/src/app/api/medications/[id]/intake/route.ts b/src/app/api/medications/[id]/intake/route.ts index 0ecd6c9d1..0e05bbecf 100644 --- a/src/app/api/medications/[id]/intake/route.ts +++ b/src/app/api/medications/[id]/intake/route.ts @@ -10,7 +10,6 @@ import { safeJson, sanitiseZodIssues, } from "@/lib/api-response"; -import { checkRecordWriteRateLimit } from "@/lib/rate-limit"; import { intakeSchema, listIntakeEventsSchema, @@ -62,16 +61,6 @@ async function postIntake(request: NextRequest, { params }: RouteParams) { "medications", ); - // Shared per-account write ceiling — see `checkRecordWriteRateLimit`. The - // batch siblings have always been capped; the per-record creates a looping - // client hits were not. - const writeRl = await checkRecordWriteRateLimit(actor.id); - if (!writeRl.allowed) { - return apiError("Too many writes, try again later", 429, { - errorCode: "record_write.rate_limited", - }); - } - // v1.36.1 — the sibling route `POST /api/medications/intake` // refuses a delegate changing a dose the owner already recorded. This route // reaches the same rows through the slot upsert, so without the same rule it diff --git a/src/app/api/medications/intake/bulk/route.ts b/src/app/api/medications/intake/bulk/route.ts index 7b9847940..4b20c810d 100644 --- a/src/app/api/medications/intake/bulk/route.ts +++ b/src/app/api/medications/intake/bulk/route.ts @@ -197,7 +197,10 @@ interface EntryResult { export const POST = apiHandler(withIdempotency<[NextRequest]>(postBulk)); async function postBulk(request: NextRequest): Promise { - const auth = await requireAuth(); + // Metered by this route's own bulk bucket below, not the shared per-account + // write bucket: the native client syncs through here in bursts (see + // `meterAccountWrite`). + const auth = await requireAuth(undefined, { writeBucket: "own" }); const { user } = auth; // A narrow, single-purpose credential (a bridge, an ingest token) is no // Apple Health sync, so it never writes the dose arrival record below. The diff --git a/src/app/api/medications/intake/route.ts b/src/app/api/medications/intake/route.ts index 98d92f114..dbdaf6e1c 100644 --- a/src/app/api/medications/intake/route.ts +++ b/src/app/api/medications/intake/route.ts @@ -21,7 +21,6 @@ import { safeJson, sanitiseZodIssues, } from "@/lib/api-response"; -import { checkRecordWriteRateLimit } from "@/lib/rate-limit"; import { annotate } from "@/lib/logging/context"; import { prisma } from "@/lib/db"; import { auditLog } from "@/lib/auth/audit"; @@ -212,16 +211,6 @@ export const POST = apiHandler(async (request: NextRequest) => { // notification at the end of this handler. const { user, actor } = await requireRecordAuth("write", "medications"); - // Shared per-account write ceiling — see `checkRecordWriteRateLimit`. The - // batch siblings have always been capped; the per-record creates a looping - // client hits were not. - const writeRl = await checkRecordWriteRateLimit(actor.id); - if (!writeRl.allowed) { - return apiError("Too many writes, try again later", 429, { - errorCode: "record_write.rate_limited", - }); - } - const { data: body, error } = await safeJson(request, { maxBytes: 64 * 1024, }); diff --git a/src/app/api/mood-entries/bulk/route.ts b/src/app/api/mood-entries/bulk/route.ts index 8a606cf7d..b0564355e 100644 --- a/src/app/api/mood-entries/bulk/route.ts +++ b/src/app/api/mood-entries/bulk/route.ts @@ -194,7 +194,10 @@ interface EntryResult { export const POST = apiHandler(withIdempotency<[NextRequest]>(postBulk)); async function postBulk(request: NextRequest): Promise { - const { user } = await requireAuth(); + // Metered by this route's own bulk bucket below, not the shared per-account + // write bucket: the native client syncs through here in bursts (see + // `meterAccountWrite`). + const { user } = await requireAuth(undefined, { writeBucket: "own" }); const rl = await checkRateLimit( `mood-entries:bulk:${user.id}`, diff --git a/src/app/api/mood-entries/route.ts b/src/app/api/mood-entries/route.ts index 94a8c4a82..67f38fe4d 100644 --- a/src/app/api/mood-entries/route.ts +++ b/src/app/api/mood-entries/route.ts @@ -8,7 +8,6 @@ import { safeJson, sanitiseZodIssues, } from "@/lib/api-response"; -import { checkRecordWriteRateLimit } from "@/lib/rate-limit"; import { createMoodEntrySchema, listMoodEntriesSchema, @@ -109,16 +108,6 @@ async function postMoodEntry(request: NextRequest) { // the `externalId` refusal below. const { user, actor } = await requireRecordAuth("manage", "mind"); - // Shared per-account write ceiling — see `checkRecordWriteRateLimit`. The - // batch siblings have always been capped; the per-record creates a looping - // client hits were not. - const writeRl = await checkRecordWriteRateLimit(actor.id); - if (!writeRl.allowed) { - return apiError("Too many writes, try again later", 429, { - errorCode: "record_write.rate_limited", - }); - } - const { data: body, error: jsonError } = await safeJson(request, { maxBytes: 64 * 1024, }); diff --git a/src/app/api/nutrients/batch/route.ts b/src/app/api/nutrients/batch/route.ts index 04a3c31a2..ac2d88f2c 100644 --- a/src/app/api/nutrients/batch/route.ts +++ b/src/app/api/nutrients/batch/route.ts @@ -79,7 +79,10 @@ interface EntryResult { export const POST = apiHandler(withIdempotency<[NextRequest]>(postBatch)); async function postBatch(request: NextRequest): Promise { - const { user } = await requireAuth(); + // Metered by this route's own bulk bucket below, not the shared per-account + // write bucket: the native client syncs through here in bursts (see + // `meterAccountWrite`). + const { user } = await requireAuth(undefined, { writeBucket: "own" }); // Module gate first: a disabled module must refuse before any body // work, so iOS can branch on the 403 `module.disabled` errorCode and diff --git a/src/app/api/vaccinations/custom/route.ts b/src/app/api/vaccinations/custom/route.ts index dbb15e488..d114d3a3a 100644 --- a/src/app/api/vaccinations/custom/route.ts +++ b/src/app/api/vaccinations/custom/route.ts @@ -31,7 +31,6 @@ import { returnAllZodIssues, safeJson, } from "@/lib/api-response"; -import { checkRecordWriteRateLimit } from "@/lib/rate-limit"; import { customVaccineCreateSchema } from "@/lib/validations/vaccinations"; import { toCustomVaccineDTO } from "@/lib/vaccinations/dto"; import { @@ -57,16 +56,7 @@ export const GET = apiHandler(async () => { }); export const POST = apiHandler(async (request: NextRequest) => { - const { user, actor } = await requireRecordAuth("write", "profile"); - - // The shared per-account write ceiling, keyed on the ACTOR so a delegate - // spends their own allowance (`checkRecordWriteRateLimit`). - const writeRl = await checkRecordWriteRateLimit(actor.id); - if (!writeRl.allowed) { - return apiError("Too many writes, try again later", 429, { - errorCode: "record_write.rate_limited", - }); - } + const { user } = await requireRecordAuth("write", "profile"); const { data: rawBody, error: jsonError } = await safeJson(request, { maxBytes: 8 * 1024, diff --git a/src/app/api/vaccinations/route.ts b/src/app/api/vaccinations/route.ts index 3a15bc2c8..afb938b15 100644 --- a/src/app/api/vaccinations/route.ts +++ b/src/app/api/vaccinations/route.ts @@ -29,7 +29,6 @@ import { returnAllZodIssues, safeJson, } from "@/lib/api-response"; -import { checkRecordWriteRateLimit } from "@/lib/rate-limit"; import { withIdempotency } from "@/lib/idempotency"; import { apiListSuccess } from "@/lib/api/list-cap"; import { listPage } from "@/lib/http/list-query"; @@ -150,17 +149,7 @@ export const GET = apiHandler(async (request: NextRequest) => { export const POST = apiHandler(withIdempotency<[NextRequest]>(postVaccination)); async function postVaccination(request: NextRequest): Promise { - const { user, actor } = await requireRecordAuth("write", "profile"); - - // Shared per-account write ceiling — see `checkRecordWriteRateLimit`. The - // batch siblings have always been capped; the per-record creates a looping - // client hits were not. - const writeRl = await checkRecordWriteRateLimit(actor.id); - if (!writeRl.allowed) { - return apiError("Too many writes, try again later", 429, { - errorCode: "record_write.rate_limited", - }); - } + const { user } = await requireRecordAuth("write", "profile"); const { data: rawBody, error: jsonError } = await safeJson(request, { maxBytes: 32 * 1024, diff --git a/src/app/api/workouts/batch/route.ts b/src/app/api/workouts/batch/route.ts index c25beccca..37ca6dfaf 100644 --- a/src/app/api/workouts/batch/route.ts +++ b/src/app/api/workouts/batch/route.ts @@ -239,7 +239,12 @@ async function postBatch(request: NextRequest): Promise { // native login mints. The only route that names it. `requireAuth` refuses // any acting-account carrier for every credential shape, so the scoped token // is confined to its owner's own record without an arm of its own here. - const auth = await requireAuth(WORKOUTS_WRITE_SCOPE); + // Metered by this route's own bulk bucket below, not the shared per-account + // write bucket: the native client syncs through here in bursts (see + // `meterAccountWrite`). + const auth = await requireAuth(WORKOUTS_WRITE_SCOPE, { + writeBucket: "own", + }); const { user } = auth; // A scoped credential is a bridge, not the native client: the source of diff --git a/src/lib/api-handler.ts b/src/lib/api-handler.ts index 564bba265..8166b60d5 100644 --- a/src/lib/api-handler.ts +++ b/src/lib/api-handler.ts @@ -55,6 +55,7 @@ import { attachRecordContextEcho, } from "./sharing/record-session-fence"; import { refuseOnKeyMismatch } from "./boot/key-mismatch-refusal"; +import { meterAccountWrite, type WriteMeterOptions } from "./auth/write-meter"; import { acceptIncomingRequestId } from "./http/request-id"; import { withErrorEnvelopeDefaults } from "./api-response"; import { @@ -545,12 +546,16 @@ export function isScopedCredential(auth: AuthContext): boolean { */ export async function requireAuth( requiredPermission?: string, + options?: Pick, ): Promise { const auth = await authenticateCaller(requiredPermission); const carrier = await readActingCarrier(auth); if (carrier.kind !== "none") { throw refuseUndeclaredMode(auth, carrier); } + // The shared write ceiling, for every write this resolver admits — see + // `meterAccountWrite`. + await meterAccountWrite(auth.user.id, options); return auth; } @@ -866,6 +871,7 @@ export async function requireActorAuth(): Promise { auditRefusal(auth, { kind: "header", accountId: header }, "actor_surface"); throw new SharingNotPermittedError(); } + await meterAccountWrite(auth.user.id); return auth; } @@ -913,6 +919,13 @@ export interface RecordAuthOptions { * above `"read"`. */ readonly readOverPost?: true; + /** + * This route meters its writes with a dedicated bucket instead of the shared + * per-account one (`meterAccountWrite`). Frozen per route, with the reason, + * by `write-rate-limit-guard.test.ts`, which also requires the route to call + * its own limiter. + */ + readonly writeBucket?: "own"; } /** @@ -966,6 +979,13 @@ export async function requireRecordAuth( // fence reads, so the invariant is "before the carrier", not "before the // first database await". await assertRecordSessionFence(auth); + // The shared write ceiling, charged to the ACTING account before any grant + // is read, so a refused write counts like an admitted one — see + // `meterAccountWrite`. + await meterAccountWrite(auth.user.id, { + readOverPost: options?.readOverPost, + writeBucket: options?.writeBucket, + }); const carrier = await readActingCarrier(auth); // A narrow, single-purpose Bearer never acts on another record. @@ -1063,6 +1083,7 @@ export async function requireGuardianAuth(): Promise { // a context that has since moved must be refused before the marker is read, // not after. await assertRecordSessionFence(auth); + await meterAccountWrite(auth.user.id); const carrier = await readActingCarrier(auth); if (carrier.kind === "none") { @@ -1207,6 +1228,7 @@ export async function requireAdmin(): Promise { if (sessionData.user.role !== "ADMIN") { throw new HttpError(403, "Admin access required", AUTH_ERROR_CODES.admin); } + await meterAccountWrite(sessionData.user.id); return { ...sessionData, authMethod: "cookie" }; } @@ -1233,6 +1255,7 @@ export async function requireCookieAuth(): Promise { auth_method: "session", }); } + await meterAccountWrite(sessionData.user.id); return { ...sessionData, authMethod: "cookie" }; } @@ -1343,6 +1366,8 @@ export async function requireFreshMfa( }); } + await meterAccountWrite(sessionData.user.id); + // The account must hold a credential that can stamp `mfaVerifiedAt` in the // first place. Without one a step-up-gated action is unreachable by design // rather than merely refused, so the refusal is named differently @@ -1661,6 +1686,7 @@ export async function requireBearerAuth(): Promise { } const raw = authHeader.slice(7); const auth = await authenticateBearer(raw, undefined); + await meterAccountWrite(auth.user.id); return { ...auth, apiTokenId: auth.session.id, diff --git a/src/lib/auth/__tests__/write-meter.test.ts b/src/lib/auth/__tests__/write-meter.test.ts new file mode 100644 index 000000000..ac6069b83 --- /dev/null +++ b/src/lib/auth/__tests__/write-meter.test.ts @@ -0,0 +1,102 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; + +// The unit setup stubs the meter for every route test; this file is where the +// real one is pinned. +vi.mock("@/lib/auth/write-meter", async (importOriginal) => + importOriginal(), +); + +const state = vi.hoisted(() => ({ + event: null as null | { getHttpMethod: () => string | undefined }, + allowed: true, +})); + +vi.mock("@/lib/logging/context", () => ({ + getEvent: () => state.event, + annotate: vi.fn(), +})); + +vi.mock("@/lib/rate-limit", () => ({ + checkRecordWriteRateLimit: vi.fn(async () => ({ + allowed: state.allowed, + limit: 300, + remaining: state.allowed ? 299 : 0, + resetAt: Date.now() + 60_000, + })), +})); + +import { + meterAccountWrite, + RECORD_WRITE_RATE_LIMITED_CODE, +} from "../write-meter"; +import { checkRecordWriteRateLimit } from "@/lib/rate-limit"; +import { HttpError } from "@/lib/api-errors"; + +function request(method: string | undefined) { + state.event = { getHttpMethod: () => method }; +} + +beforeEach(() => { + vi.mocked(checkRecordWriteRateLimit).mockClear(); + state.allowed = true; + state.event = null; +}); + +describe("meterAccountWrite", () => { + it.each(["POST", "PUT", "PATCH", "DELETE", "patch"])( + "charges the acting account for a %s", + async (method) => { + request(method); + await meterAccountWrite("account-1"); + expect(checkRecordWriteRateLimit).toHaveBeenCalledWith("account-1"); + }, + ); + + it.each(["GET", "HEAD", "OPTIONS"])("leaves a %s alone", async (method) => { + request(method); + await meterAccountWrite("account-1"); + expect(checkRecordWriteRateLimit).not.toHaveBeenCalled(); + }); + + it("charges nothing outside a request", async () => { + await meterAccountWrite("account-1"); + expect(checkRecordWriteRateLimit).not.toHaveBeenCalled(); + }); + + it("charges one request once, however many resolvers it passes", async () => { + request("POST"); + await meterAccountWrite("account-1"); + await meterAccountWrite("account-1"); + expect(checkRecordWriteRateLimit).toHaveBeenCalledTimes(1); + + request("POST"); + await meterAccountWrite("account-1"); + expect(checkRecordWriteRateLimit).toHaveBeenCalledTimes(2); + }); + + it("leaves a read-over-POST alone, and only the POST", async () => { + request("POST"); + await meterAccountWrite("account-1", { readOverPost: true }); + expect(checkRecordWriteRateLimit).not.toHaveBeenCalled(); + request("DELETE"); + await meterAccountWrite("account-1", { readOverPost: true }); + expect(checkRecordWriteRateLimit).toHaveBeenCalledTimes(1); + }); + + it("leaves a route with its own bucket alone", async () => { + request("POST"); + await meterAccountWrite("account-1", { writeBucket: "own" }); + expect(checkRecordWriteRateLimit).not.toHaveBeenCalled(); + }); + + it("refuses a spent bucket with 429 and the published code", async () => { + state.allowed = false; + request("PATCH"); + const refusal = await meterAccountWrite("account-1").catch((e) => e); + expect(refusal).toBeInstanceOf(HttpError); + expect((refusal as HttpError).statusCode).toBe(429); + expect((refusal as HttpError).errorCode).toBe( + RECORD_WRITE_RATE_LIMITED_CODE, + ); + }); +}); diff --git a/src/lib/auth/write-meter.ts b/src/lib/auth/write-meter.ts new file mode 100644 index 000000000..25c4a1895 --- /dev/null +++ b/src/lib/auth/write-meter.ts @@ -0,0 +1,88 @@ +/** + * The shared per-account write ceiling, applied where the caller is resolved. + * + * `checkRecordWriteRateLimit` (300 writes a minute per acting account) used to + * be called by hand in a dozen create routes, and the other mutating routes + * had no ceiling at all: of 364 mutating handlers, 169 met no limiter, every + * PATCH and DELETE on an `[id]` among them. A positive list of routes that + * remember to call it can only fall behind the routes that exist, so the + * ceiling now sits on the path every authenticated write already takes: the + * identity resolvers in `src/lib/api-handler.ts` call {@link meterAccountWrite} + * once the caller is known, and it charges the bucket for any request whose + * method writes. + * + * What it does not do: + * + * - charge a read. GET, HEAD and OPTIONS pass untouched, and so does a POST a + * route declared `readOverPost` (a search whose query must not stand in a + * URL). + * - charge one request twice. A route that resolves the caller through two + * helpers (step-up on top of a session) is charged once. + * - meter a route that declared its own bucket. The bulk ingest endpoints + * the native client syncs through have dedicated ceilings sized for that + * volume; charging the shared bucket as well would let a large HealthKit + * backfill lock the person out of logging a single dose by hand. Declaring + * `writeBucket: "own"` is frozen per route by + * `src/__tests__/write-rate-limit-guard.test.ts`, which also requires the + * route to call its own limiter. + * + * The bucket is keyed on the ACTING account, the precedent from the delegable + * medication routes: a delegate spends their own allowance rather than the + * record owner's, and cannot collect a fresh one by switching records. A + * refused request is counted too, since the charge happens before the body is + * read. + */ +import { HttpError } from "@/lib/api-errors"; +import { annotate, getEvent } from "@/lib/logging/context"; +import { checkRecordWriteRateLimit } from "@/lib/rate-limit"; + +/** The code a refusal by the shared write bucket carries. */ +export const RECORD_WRITE_RATE_LIMITED_CODE = "record_write.rate_limited"; + +const WRITE_METHODS = new Set(["POST", "PUT", "PATCH", "DELETE"]); + +/** Requests already charged, so a second resolver in one request is free. */ +const charged = new WeakSet(); + +export interface WriteMeterOptions { + /** The route answers this POST without writing (see `readOverPost`). */ + readOverPost?: true; + /** + * The route meters its writes with a dedicated bucket instead of the shared + * one. Reviewed per route by the write rate-limit guard. + */ + writeBucket?: "own"; +} + +/** + * Charge the acting account's shared write bucket for this request, or refuse + * it with 429 when the bucket is spent. + * + * Outside a request (no wide event, so no method) there is nothing to charge. + */ +export async function meterAccountWrite( + accountId: string, + options: WriteMeterOptions = {}, +): Promise { + const evt = getEvent(); + if (!evt) return; + const method = (evt.getHttpMethod() ?? "").toUpperCase(); + if (!WRITE_METHODS.has(method)) return; + if (options.readOverPost === true && method === "POST") return; + if (options.writeBucket === "own") { + annotate({ meta: { write_bucket: "own" } }); + return; + } + if (charged.has(evt)) return; + charged.add(evt); + + const rl = await checkRecordWriteRateLimit(accountId); + if (!rl.allowed) { + annotate({ meta: { record_write_rate_limited: true } }); + throw new HttpError( + 429, + "Too many writes, try again later", + RECORD_WRITE_RATE_LIMITED_CODE, + ); + } +} diff --git a/src/lib/openapi/routes/shared.ts b/src/lib/openapi/routes/shared.ts index 4c20a843b..afa7f2533 100644 --- a/src/lib/openapi/routes/shared.ts +++ b/src/lib/openapi/routes/shared.ts @@ -356,10 +356,12 @@ export const stdResponses = { * * The batch endpoints have been capped at 60 calls a minute since they were * written and the per-record siblings were not capped at all, which is exactly - * backwards: the batch endpoint is the one a well-behaved client uses. The - * eleven single-record creates now share one generous per-account bucket, and - * this is the response that names it — a client that meets a ceiling should be - * able to read which one it met without a bug report. + * backwards: the batch endpoint is the one a well-behaved client uses. Every + * authenticated write now shares one generous per-account bucket, charged where + * the caller is resolved (`src/lib/auth/write-meter.ts`), and this is the + * response that names it on the single-record creates that publish it — a + * client that meets a ceiling should be able to read which one it met without + * a bug report. * * Spread AFTER `...stdResponses` so it replaces the generic 429 on those * operations. The headers block is the one the standard 429 already declares. @@ -371,7 +373,7 @@ export const stdResponses = { export const recordWriteRateLimitResponse = { "429": { description: - "Rate limit exceeded. This route shares one per-account bucket with the other single-record writes — `record-write:`, 300 requests per 60 seconds — keyed on the ACTING account, so a delegate burns their own allowance rather than the record owner's. Nothing was written. `meta.errorCode` is `record_write.rate_limited`, which is what tells this refusal apart from a route's own narrower bucket when both can answer 429. The `Retry-After`, `X-RateLimit-Limit`, `X-RateLimit-Remaining` and `X-RateLimit-Reset` headers describe that bucket; back off on `Retry-After` rather than guessing. Every request counts against the bucket, refused ones included: the check runs before the body is read, so a client looping on a 422 spends the same allowance as one that writes rows. A client with more than a handful of rows to send should use the batch endpoint for its domain instead of looping this one.", + "Rate limit exceeded. This route shares one per-account bucket with every other authenticated write — `record-write:`, 300 requests per 60 seconds — keyed on the ACTING account, so a delegate burns their own allowance rather than the record owner's. Only the bulk ingest endpoints, which meter themselves with a dedicated bucket, stay outside it. Nothing was written. `meta.errorCode` is `record_write.rate_limited`, which is what tells this refusal apart from a route's own narrower bucket when both can answer 429. The `Retry-After`, `X-RateLimit-Limit`, `X-RateLimit-Remaining` and `X-RateLimit-Reset` headers describe that bucket; back off on `Retry-After` rather than guessing. Every request counts against the bucket, refused ones included: the check runs before the body is read, so a client looping on a 422 spends the same allowance as one that writes rows. A client with more than a handful of rows to send should use the batch endpoint for its domain instead of looping this one.", content: { "application/json": { schema: errorEnvelope } }, headers: stdResponses["429"].headers, }, diff --git a/src/lib/rate-limit.ts b/src/lib/rate-limit.ts index 9c9daa3cd..9951a2bd9 100644 --- a/src/lib/rate-limit.ts +++ b/src/lib/rate-limit.ts @@ -136,25 +136,23 @@ export async function checkEnvironmentBackfillRateLimit( } /** - * Shared per-account ceiling for the single-record writes. + * Shared per-account ceiling for every authenticated write. * * The batch endpoints have been capped at 60 calls a minute since they were * written; the per-record siblings a naive or older client loops over were not * capped at all, which is backwards — the batch endpoint is the one a - * well-behaved client uses. This is the floor under the other direction: - * `POST /api/measurements`, `/api/mood-entries`, both medication-intake writes, - * `/api/labs`, a custom-metric entry, a cycle day-log, a vaccination, an - * allergy, a biomarker and an encounter. + * well-behaved client uses. This was first the floor under a dozen creates, + * called by hand; it is now charged where the caller is resolved + * (`meterAccountWrite`, `src/lib/auth/write-meter.ts`), so every POST, PUT, + * PATCH and DELETE an authenticated caller sends counts against it, except on + * the bulk ingest routes that meter themselves. * * One shared bucket, not one per route, for the reason - * `checkAnalyticsReadRateLimit` gives: a loop that rotates across ten creates + * `checkAnalyticsReadRateLimit` gives: a loop that rotates across ten routes * should still meet a cap. Three hundred a minute is five writes a second * sustained for a full minute — no hand-driven session and no sync burst - * reaches it. Ten of the eleven are wrapped in `withIdempotency`, so an honest - * retry storm is deduplicated before it is counted; `POST - * /api/medications/intake` is not, because it updates a named `intakeId` - * rather than creating a row, so a replay writes the status that is already - * there. It exists to cut off the pathological loop, not to shape traffic. + * reaches it. It exists to cut off the pathological loop, not to shape + * traffic. * * A refused request is counted too: the check runs before the body is read, * so a client looping on a 422 spends its allowance without writing a row. diff --git a/tests/integration/write-meter.test.ts b/tests/integration/write-meter.test.ts new file mode 100644 index 000000000..5b448a8f8 --- /dev/null +++ b/tests/integration/write-meter.test.ts @@ -0,0 +1,185 @@ +/** + * The shared write ceiling, against Postgres, on routes that never call it. + * + * The bucket is charged where the caller is resolved, so the routes below + * carry no limiter of their own; a PATCH and a DELETE on an `[id]` met none + * before. What has to hold: a write is counted, a read is not, a spent bucket + * refuses with the published code and the headers that describe it and + * writes nothing, and a bulk route on its own bucket keeps working while the + * shared one is spent. + */ +import { NextRequest } from "next/server"; +import { beforeEach, describe, expect, it, vi } from "vitest"; + +import { cookieJar, headerJar } from "./mock-next-headers"; +import { getPrismaClient, truncateAllTables } from "./setup"; +import { + RECORD_WRITE_BUCKET_PREFIX, + RECORD_WRITE_LIMIT, +} from "@/lib/rate-limit"; + +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), +})); + +const USER_ID = "user-write-meter"; +const BUCKET = `${RECORD_WRITE_BUCKET_PREFIX}:${USER_ID}`; + +async function bucketCount(): Promise { + const row = await getPrismaClient().rateLimit.findUnique({ + where: { key: BUCKET }, + }); + return row?.count ?? 0; +} + +async function spendBucket(): Promise { + await getPrismaClient().rateLimit.upsert({ + where: { key: BUCKET }, + create: { + key: BUCKET, + count: RECORD_WRITE_LIMIT, + resetAt: new Date(Date.now() + 60_000), + }, + update: { + count: RECORD_WRITE_LIMIT, + resetAt: new Date(Date.now() + 60_000), + }, + }); +} + +let allergyId: string; + +beforeEach(async () => { + const prisma = getPrismaClient(); + await truncateAllTables(prisma); + cookieJar.clear(); + headerJar.clear(); + await prisma.user.create({ + data: { + id: USER_ID, + username: "write-meter", + email: "write-meter@example.test", + timezone: "UTC", + }, + }); + const session = await prisma.session.create({ + data: { userId: USER_ID, expiresAt: new Date(Date.now() + 3_600_000) }, + }); + cookieJar.set("healthlog_session", session.id); + const allergy = await prisma.allergy.create({ + data: { + userId: USER_ID, + substance: "Pollen", + category: "ENVIRONMENT", + type: "ALLERGY", + status: "ACTIVE", + }, + }); + allergyId = allergy.id; +}); + +function patchAllergy(body: unknown) { + return import("@/app/api/allergies/[id]/route").then(({ PATCH }) => + PATCH( + new NextRequest(`http://localhost/api/allergies/${allergyId}`, { + method: "PATCH", + body: JSON.stringify(body), + headers: { "content-type": "application/json" }, + }), + { params: Promise.resolve({ id: allergyId }) }, + ), + ); +} + +describe("the shared write bucket on routes without a limiter of their own", () => { + it("counts a PATCH and a DELETE, and not a GET", async () => { + const { GET, DELETE } = await import("@/app/api/allergies/[id]/route"); + + const read = await GET( + new NextRequest(`http://localhost/api/allergies/${allergyId}`), + { params: Promise.resolve({ id: allergyId }) }, + ); + expect(read.status).toBe(200); + expect(await bucketCount()).toBe(0); + + expect((await patchAllergy({ substance: "Birch pollen" })).status).toBe( + 200, + ); + expect(await bucketCount()).toBe(1); + + const gone = await DELETE( + new NextRequest(`http://localhost/api/allergies/${allergyId}`, { + method: "DELETE", + }), + { params: Promise.resolve({ id: allergyId }) }, + ); + expect(gone.status).toBe(200); + expect(await bucketCount()).toBe(2); + }); + + it("refuses a write once the bucket is spent, says why, and writes nothing", async () => { + await spendBucket(); + + const res = await patchAllergy({ substance: "Birch pollen" }); + expect(res.status).toBe(429); + const body = (await res.json()) as { meta?: { errorCode?: string } }; + expect(body.meta?.errorCode).toBe("record_write.rate_limited"); + expect(Number(res.headers.get("Retry-After"))).toBeGreaterThan(0); + expect(res.headers.get("X-RateLimit-Limit")).toBe( + String(RECORD_WRITE_LIMIT), + ); + + const row = await getPrismaClient().allergy.findUniqueOrThrow({ + where: { id: allergyId }, + }); + expect(row.substance).toBe("Pollen"); + }); + + it("keeps a bulk route on its own bucket working while the shared one is spent", async () => { + await spendBucket(); + const { POST } = await import("@/app/api/measurements/batch/route"); + const res = await POST( + new NextRequest("http://localhost/api/measurements/batch", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ + measurements: [ + { + type: "WEIGHT", + value: 72.4, + unit: "kg", + measuredAt: new Date(Date.now() - 3_600_000).toISOString(), + source: "APPLE_HEALTH", + externalId: "write-meter-weight-1", + }, + ], + }), + }), + ); + expect(res.status).toBeLessThan(300); + expect( + await getPrismaClient().measurement.count({ where: { userId: USER_ID } }), + ).toBe(1); + }); +}); diff --git a/vitest.setup.ts b/vitest.setup.ts index 2b48ae93d..8e1a180de 100644 --- a/vitest.setup.ts +++ b/vitest.setup.ts @@ -18,7 +18,7 @@ * Server-side translation (`getServerTranslator`, `shared-resolve`) holds * every catalog on its own and needs no priming. */ -import { afterEach } from "vitest"; +import { afterEach, vi } from "vitest"; import { settleBackgroundTasks } from "@/lib/logging/background-tasks"; import { primeMessages } from "@/lib/i18n/load-locale"; import enMessages from "./messages/en.json"; @@ -43,3 +43,19 @@ primeMessages("en", enMessages); afterEach(async () => { await settleBackgroundTasks(); }); + +/** + * The shared write ceiling is charged by every identity resolver on a write + * (`src/lib/auth/write-meter.ts`), which is a database round-trip most route + * tests have no fake for: their Prisma mock carries the models the handler + * touches and nothing else. The meter is therefore a no-op in unit tests by + * default. Its own behaviour is pinned by + * `src/lib/auth/__tests__/write-meter.test.ts`, which loads the real module, + * and against Postgres by `tests/integration/write-meter.test.ts`; a route test + * that wants the refusal mocks `meterAccountWrite` to throw. + */ +vi.mock("@/lib/auth/write-meter", async (importOriginal) => { + const actual = + await importOriginal(); + return { ...actual, meterAccountWrite: vi.fn(async () => undefined) }; +});