mirror of
https://github.com/MBombeck/HealthLog.git
synced 2026-11-09 04:50:59 +00:00
fix(rate-limit): fail the shared write ceiling open and skip reads over POST
The shared write bucket threw on any SQL error, so a lock on rate_limits answered every authenticated write with 500. It is a loop cutter, not an access check: when the bucket cannot be read the write now goes ahead with a warn signal. The login and other auth-surface limits call the limiter directly and still fail closed. requireAuth now passes readOverPost to the meter, the draft-only document assist declares it, and the three chat routes, which meter their turns with buckets of their own, declare that instead of being counted twice.
This commit is contained in:
1 parent
408ae35b7c
commit
5185edca74
12 files changed
+148
-19
No files matched your search
@@ -398,14 +398,15 @@ paths:
|
||||
"429": &a12
|
||||
description: "Rate limit exceeded. This route shares one per-account bucket with every other authenticated write —
|
||||
`record-write:<accountId>`, 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."
|
||||
their own allowance rather than the record owner's. Only the routes that meter themselves with a dedicated
|
||||
bucket (the bulk ingest endpoints and the model-turn chats) and a POST that writes nothing stay outside it.
|
||||
If the bucket cannot be read, the write goes ahead unmetered rather than failing. 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."
|
||||
headers: *a8
|
||||
content:
|
||||
application/json:
|
||||
|
||||
@@ -2,8 +2,10 @@
|
||||
* `readOverPost` lets a delegable route answer a POST at the grant's read
|
||||
* level. That widens what a read-only grant reaches, so the set of routes
|
||||
* declaring it is frozen here: adding one is a reviewed diff, not an
|
||||
* accident. Today it is the record search alone, whose query is health data
|
||||
* that must stay out of URLs.
|
||||
* accident. Today it is the record search, whose query is health data that
|
||||
* must stay out of URLs, and the document assist, whose POST writes nothing
|
||||
* (under `requireAuth` the declaration only keeps it off the shared write
|
||||
* ceiling).
|
||||
*
|
||||
* The matcher tolerates whitespace and line breaks, and the guard asserts it
|
||||
* matched something, so a rewrite that hides the declaration from the regex
|
||||
@@ -16,7 +18,13 @@ import { join, relative, sep } from "node:path";
|
||||
|
||||
const ROOT = process.cwd();
|
||||
|
||||
const FROZEN = ["src/app/api/search/route.ts"];
|
||||
const FROZEN = [
|
||||
// The draft-only document assist: a POST that writes nothing. Under
|
||||
// `requireAuth` the declaration only keeps it out of the shared write
|
||||
// ceiling; there is no grant for it to widen.
|
||||
"src/app/api/documents/inbound/[id]/suggest/route.ts",
|
||||
"src/app/api/search/route.ts",
|
||||
];
|
||||
|
||||
describe("readOverPost declarations", () => {
|
||||
it("are exactly the frozen set", () => {
|
||||
|
||||
@@ -75,6 +75,12 @@ const OWN_BUCKET: Record<string, string> = {
|
||||
"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/chat/route.ts":
|
||||
"A Coach turn is metered by the Coach's own buckets: 20 turns a minute (`coach-chat`) and one turn slot at a time.",
|
||||
"app/api/insights/chat/fenced/route.ts":
|
||||
"A fenced document chat turn is metered by its own per-account bucket, sized for model turns.",
|
||||
"app/api/documents/inbound/[id]/chat/route.ts":
|
||||
"A document chat turn is metered by its own per-account bucket, sized for model turns.",
|
||||
"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.",
|
||||
};
|
||||
|
||||
@@ -71,7 +71,9 @@ type RouteParams = { params: Promise<{ id: string }> };
|
||||
|
||||
export const POST = apiHandler(
|
||||
async (request: NextRequest, { params }: RouteParams) => {
|
||||
const { user } = await requireAuth();
|
||||
// Metered by the document chat's own bucket below, not the shared
|
||||
// ceiling.
|
||||
const { user } = await requireAuth(undefined, { writeBucket: "own" });
|
||||
const userId = user.id;
|
||||
|
||||
const gate = await requireModuleEnabled(userId, "inboundDocuments");
|
||||
|
||||
@@ -73,7 +73,9 @@ type RouteParams = { params: Promise<{ id: string }> };
|
||||
|
||||
export const POST = apiHandler(
|
||||
async (request: NextRequest, { params }: RouteParams) => {
|
||||
const { user } = await requireAuth();
|
||||
// Writes nothing (the draft goes back to the edit form), so the shared
|
||||
// write ceiling does not count it; the document-AI bucket meters it.
|
||||
const { user } = await requireAuth(undefined, { readOverPost: true });
|
||||
|
||||
const gate = await requireModuleEnabled(user.id, "inboundDocuments");
|
||||
if (!gate.enabled) return gate.response;
|
||||
|
||||
@@ -52,7 +52,8 @@ export const dynamic = "force-dynamic";
|
||||
export const runtime = "nodejs";
|
||||
|
||||
export const POST = apiHandler(async (request: NextRequest) => {
|
||||
const { user } = await requireAuth();
|
||||
// Metered by the fenced chat's own bucket below, not the shared ceiling.
|
||||
const { user } = await requireAuth(undefined, { writeBucket: "own" });
|
||||
const userId = user.id;
|
||||
|
||||
const gate = await requireModuleEnabled(userId, "inboundDocuments");
|
||||
|
||||
@@ -81,7 +81,9 @@ const COACH_CONCURRENT_TURNS = 2;
|
||||
const COACH_TURN_SLOT_MS = TURN_LIMITS.user.wallMs + 30_000;
|
||||
|
||||
async function handleChatRequest(request: NextRequest): Promise<Response> {
|
||||
const auth = await requireAuth();
|
||||
// A turn is metered by the Coach's own buckets (`coach-chat`, the turn
|
||||
// slot), not the shared write ceiling.
|
||||
const auth = await requireAuth(undefined, { writeBucket: "own" });
|
||||
// The `coach` capability, right after auth and before the rate limit and
|
||||
// the body parse. It folds in what the Coach module gate, the operator
|
||||
// switch and the `disableCoach` opt-out used to answer separately, plus
|
||||
|
||||
@@ -546,7 +546,7 @@ export function isScopedCredential(auth: AuthContext): boolean {
|
||||
*/
|
||||
export async function requireAuth(
|
||||
requiredPermission?: string,
|
||||
options?: Pick<WriteMeterOptions, "writeBucket">,
|
||||
options?: WriteMeterOptions,
|
||||
): Promise<AuthContext> {
|
||||
const auth = await authenticateCaller(requiredPermission);
|
||||
const carrier = await readActingCarrier(auth);
|
||||
|
||||
@@ -16,6 +16,12 @@ vi.mock("@/lib/logging/context", () => ({
|
||||
annotate: vi.fn(),
|
||||
}));
|
||||
|
||||
const signals = vi.hoisted(() => ({ emitted: [] as unknown[] }));
|
||||
|
||||
vi.mock("@/lib/logging/signal", () => ({
|
||||
emitSignal: (input: unknown) => signals.emitted.push(input),
|
||||
}));
|
||||
|
||||
vi.mock("@/lib/rate-limit", () => ({
|
||||
checkRecordWriteRateLimit: vi.fn(async () => ({
|
||||
allowed: state.allowed,
|
||||
@@ -38,6 +44,7 @@ function request(method: string | undefined) {
|
||||
|
||||
beforeEach(() => {
|
||||
vi.mocked(checkRecordWriteRateLimit).mockClear();
|
||||
signals.emitted.length = 0;
|
||||
state.allowed = true;
|
||||
state.event = null;
|
||||
});
|
||||
@@ -99,4 +106,27 @@ describe("meterAccountWrite", () => {
|
||||
RECORD_WRITE_RATE_LIMITED_CODE,
|
||||
);
|
||||
});
|
||||
|
||||
it("lets the write through with a warning when the bucket cannot be read", async () => {
|
||||
vi.mocked(checkRecordWriteRateLimit).mockRejectedValueOnce(
|
||||
new Error("canceling statement due to lock timeout"),
|
||||
);
|
||||
request("POST");
|
||||
await expect(meterAccountWrite("account-1")).resolves.toBeUndefined();
|
||||
expect(signals.emitted).toEqual([
|
||||
expect.objectContaining({
|
||||
action: "rate_limit.record_write.unavailable",
|
||||
level: "warn",
|
||||
}),
|
||||
]);
|
||||
});
|
||||
|
||||
it("counter-check: a readable, spent bucket still refuses, with no warning", async () => {
|
||||
state.allowed = false;
|
||||
request("POST");
|
||||
await expect(meterAccountWrite("account-1")).rejects.toBeInstanceOf(
|
||||
HttpError,
|
||||
);
|
||||
expect(signals.emitted).toEqual([]);
|
||||
});
|
||||
});
|
||||
@@ -31,9 +31,18 @@
|
||||
* 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.
|
||||
*
|
||||
* The bucket fails open. It cuts off a runaway loop; it is not what decides
|
||||
* who may write, which the resolver that called it has already settled. So
|
||||
* when the bucket itself cannot be read (the upsert fails: a lock on
|
||||
* `rate_limits`, a pool at its limit), the write goes ahead and a `warn`
|
||||
* signal says the ceiling was not applied, instead of every authenticated
|
||||
* write in the app answering 500. The login, sign-up and other auth-surface
|
||||
* limits call the limiter directly and keep failing closed.
|
||||
*/
|
||||
import { HttpError } from "@/lib/api-errors";
|
||||
import { annotate, getEvent } from "@/lib/logging/context";
|
||||
import { emitSignal } from "@/lib/logging/signal";
|
||||
import { checkRecordWriteRateLimit } from "@/lib/rate-limit";
|
||||
|
||||
/** The code a refusal by the shared write bucket carries. */
|
||||
@@ -76,7 +85,18 @@ export async function meterAccountWrite(
|
||||
if (charged.has(evt)) return;
|
||||
charged.add(evt);
|
||||
|
||||
const rl = await checkRecordWriteRateLimit(accountId);
|
||||
let rl: Awaited<ReturnType<typeof checkRecordWriteRateLimit>>;
|
||||
try {
|
||||
rl = await checkRecordWriteRateLimit(accountId);
|
||||
} catch (error) {
|
||||
annotate({ meta: { record_write_rate_limit_unavailable: true } });
|
||||
emitSignal({
|
||||
action: "rate_limit.record_write.unavailable",
|
||||
level: "warn",
|
||||
error,
|
||||
});
|
||||
return;
|
||||
}
|
||||
if (!rl.allowed) {
|
||||
annotate({ meta: { record_write_rate_limited: true } });
|
||||
throw new HttpError(
|
||||
|
||||
@@ -373,7 +373,7 @@ export const stdResponses = {
|
||||
export const recordWriteRateLimitResponse = {
|
||||
"429": {
|
||||
description:
|
||||
"Rate limit exceeded. This route shares one per-account bucket with every other authenticated write — `record-write:<accountId>`, 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.",
|
||||
"Rate limit exceeded. This route shares one per-account bucket with every other authenticated write — `record-write:<accountId>`, 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 routes that meter themselves with a dedicated bucket (the bulk ingest endpoints and the model-turn chats) and a POST that writes nothing stay outside it. If the bucket cannot be read, the write goes ahead unmetered rather than failing. 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,
|
||||
},
|
||||
|
||||
@@ -6,7 +6,9 @@
|
||||
* 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.
|
||||
* shared one is spent. A POST that reads (`readOverPost`) is not counted on
|
||||
* either resolver, and a bucket that cannot be read lets the write through
|
||||
* while the limiter itself still fails for its other callers.
|
||||
*/
|
||||
import { NextRequest } from "next/server";
|
||||
import { beforeEach, describe, expect, it, vi } from "vitest";
|
||||
@@ -183,3 +185,58 @@ describe("the shared write bucket on routes without a limiter of their own", ()
|
||||
).toBe(1);
|
||||
});
|
||||
});
|
||||
|
||||
describe("what the shared bucket does not count", () => {
|
||||
async function postThrough(options?: { readOverPost: true }) {
|
||||
const { apiHandler, requireAuth } = await import("@/lib/api-handler");
|
||||
const { apiSuccess } = await import("@/lib/api-response");
|
||||
const POST = apiHandler(async (_request: NextRequest) => {
|
||||
const { user } = await requireAuth(undefined, options);
|
||||
return apiSuccess({ id: user.id });
|
||||
});
|
||||
return POST(
|
||||
new NextRequest("http://localhost/api/probe", {
|
||||
method: "POST",
|
||||
body: "{}",
|
||||
headers: { "content-type": "application/json" },
|
||||
}),
|
||||
);
|
||||
}
|
||||
|
||||
it("leaves a read-over-POST through requireAuth uncounted", async () => {
|
||||
expect((await postThrough({ readOverPost: true })).status).toBe(200);
|
||||
expect(await bucketCount()).toBe(0);
|
||||
});
|
||||
|
||||
it("counter-check: the same POST without the declaration is counted", async () => {
|
||||
expect((await postThrough()).status).toBe(200);
|
||||
expect(await bucketCount()).toBe(1);
|
||||
});
|
||||
});
|
||||
|
||||
describe("a bucket that cannot be read", () => {
|
||||
it("lets the write through instead of answering 500, and the limiter still fails for its own callers", async () => {
|
||||
const prisma = getPrismaClient();
|
||||
const { checkRateLimit } = await import("@/lib/rate-limit");
|
||||
await prisma.$executeRawUnsafe(
|
||||
`ALTER TABLE "rate_limits" RENAME TO "rate_limits_away"`,
|
||||
);
|
||||
try {
|
||||
const res = await patchAllergy({ substance: "Birch pollen" });
|
||||
expect(res.status, await res.clone().text()).toBe(200);
|
||||
const row = await prisma.allergy.findUniqueOrThrow({
|
||||
where: { id: allergyId },
|
||||
});
|
||||
expect(row.substance).toBe("Birch pollen");
|
||||
|
||||
// A login or sign-up limit calls the limiter directly: no fail-open.
|
||||
await expect(
|
||||
checkRateLimit("auth:login:probe", 5, 60_000),
|
||||
).rejects.toThrow();
|
||||
} finally {
|
||||
await prisma.$executeRawUnsafe(
|
||||
`ALTER TABLE "rate_limits_away" RENAME TO "rate_limits"`,
|
||||
);
|
||||
}
|
||||
});
|
||||
});
|
||||
Reference in new issue
Block a user