From 20812b54f57feaa97f49dcf0e3bb03c424d75476 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Thu, 3 Sep 2026 01:42:56 +0200 Subject: [PATCH 1/2] fix(webhooks): rate-limit every verb that checks the shared secret The Withings entrypoints, the Telegram bot webhook and the Coolify deploy hook limited POST and left the verification verbs unlimited. GET and HEAD compare the same secret and answer 200 against 401, which is an oracle for guessing it and a free load channel besides; timingSafeEqual protects the timing channel, not the status code. The project's rule is that the per-source limit runs before secret verification, and it was being applied per handler rather than per file. Each verification verb now runs the same limit its POST sibling used, on the same bucket. Telegram and the deploy hook grow the small local helper the Withings routes already share. HEAD re-shapes the limiter's JSON envelope into a bodyless 429 and keeps the rate-limit headers. The new guard walks every webhook route from the filesystem, resolves which local helpers reach a comparison and which reach the limiter, and fails when an exported verb reaches a comparison without the limiter in front of it. It asserts a non-zero match count so an empty match set cannot read as green. WHOOP already passed: it exports POST only. --- CHANGELOG.md | 20 ++ .../webhook-verb-rate-limit-guard.test.ts | 285 ++++++++++++++++++ .../deploy-webhook/__tests__/route.test.ts | 25 ++ src/app/api/internal/deploy-webhook/route.ts | 43 ++- .../telegram/webhook/__tests__/route.test.ts | 26 ++ src/app/api/telegram/webhook/route.ts | 31 +- .../webhook/[token]/__tests__/route.test.ts | 61 ++++ src/app/api/withings/webhook/[token]/route.ts | 21 +- .../withings/webhook/__tests__/route.test.ts | 50 +++ src/app/api/withings/webhook/route.ts | 16 + 10 files changed, 556 insertions(+), 22 deletions(-) create mode 100644 src/app/api/__tests__/webhook-verb-rate-limit-guard.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 48b6ec02c..b89c9f1e7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,25 @@ # Changelog +## [Unreleased] + +### Security + +- **Every webhook verb that checks the shared secret is rate-limited now, not + just the one that carries data.** The Withings entrypoints, the Telegram bot + webhook and the Coolify deploy hook all limited their POST and left the + reachability verbs — the GET and HEAD a webhook UI sends to confirm the URL + before saving it — comparing the same secret with nothing in front of them. + Those verbs answer 200 for the right secret and 401 for a wrong one, so + unlimited they were a free guessing machine against the callback token of an + instance whose URL is known, and a free load channel besides. The comparison + was already constant-time, which hides how long the check took but not what + it answered. Each verb now runs the per-source limit its POST sibling used, + on the same bucket, before it looks at the secret. Nothing changes for a + legitimate subscription: Withings, Telegram and Coolify send one or two + probes, far below the limit. A new guard walks every webhook route and fails + the build if a verb reaches a secret comparison without the limiter in front + of it, so the next verb added to one of these files cannot repeat this. + ## [1.38.4] — 2026-09-02 The shared AI key reaches the provider its address names, and the image diff --git a/src/app/api/__tests__/webhook-verb-rate-limit-guard.test.ts b/src/app/api/__tests__/webhook-verb-rate-limit-guard.test.ts new file mode 100644 index 000000000..c30da425b --- /dev/null +++ b/src/app/api/__tests__/webhook-verb-rate-limit-guard.test.ts @@ -0,0 +1,285 @@ +import { readdirSync, readFileSync, statSync } from "node:fs"; +import { join, relative, resolve } from "node:path"; + +import { describe, expect, it } from "vitest"; + +/** + * Webhook verb rate-limit guard. + * + * The project rule for an inbound webhook is: per-source rate limit BEFORE + * secret verification. The reason is not throughput, it is the oracle. A + * secret comparison that answers 200 against 401 tells the caller whether it + * guessed right; constant-time comparison hides the timing channel but not the + * status code, so the only thing standing between a wrong guess and the next + * one is the limiter. An unlimited verb is a free guessing machine and a free + * load channel. + * + * The rule was applied per HANDLER rather than per FILE, which is how it drifted: + * both Withings entrypoints, the Telegram bot webhook and the Coolify deploy + * hook limited POST and left the verification verbs (GET / HEAD, the URL + * reachability probes every webhook UI sends) comparing the same secret with + * nothing in front of them. Reviewing the POST proved nothing about the sibling + * export three lines below it. + * + * So this guard reasons about EVERY exported verb, not about files: + * + * 1. Discover the webhook routes from the filesystem — every `route.ts` + * under `src/app/api` whose path names a webhook, plus the explicitly + * listed unauthenticated inbound endpoints below. + * 2. Resolve which local helpers reach a constant-time secret comparison and + * which reach the rate limiter, transitively, so a verb that delegates + * (`verifyTokenSegment`, `hasValidSecret`, `checkAndWarn`) is judged on + * what the helper does rather than on the call's spelling. + * 3. For every exported verb that reaches a comparison, require that it also + * reaches the limiter and reaches it FIRST. + * + * A verb that compares no secret is not the subject here and is skipped — the + * CSP report endpoint is the example: nothing to guess, and it rate-limits + * anyway. A verb that never gets exported cannot be reached, which is why + * WHOOP passes trivially: it exports POST only. + * + * LIMIT, stated so nobody reads more into a green run than it earns: this is a + * source-text guard. It proves the limiter call precedes the comparison call in + * the handler body; it does not prove the limiter and the POST share a bucket, + * and a comparison moved into an imported module (rather than a local helper) + * would fall outside the transitive resolution below. The per-route tests are + * what prove the refusal. + */ + +const repoRoot = resolve(__dirname, "..", "..", "..", ".."); +const API_ROOT = "src/app/api"; + +/** + * Inbound endpoints that are webhooks by behaviour but not by path. Listed + * explicitly so the discovery below is not silently narrower than the class it + * claims to cover. + */ +const EXTRA_WEBHOOK_ROUTES: ReadonlyArray = [ + // Browser-posted CSP violation reports. Unauthenticated by design (the + // browser cannot carry a secret), rate-limited, no comparison to shield. + "src/app/api/monitoring/csp-report/route.ts", +]; + +/** The HTTP verbs Next.js recognises as route handlers. */ +const HTTP_VERBS = [ + "GET", + "HEAD", + "POST", + "PUT", + "PATCH", + "DELETE", + "OPTIONS", +] as const; + +/** A constant-time secret comparison, however it is spelled. */ +const COMPARISON_NEEDLES = [ + "timingSafeEqual(", + "timingSafeStringEqual(", +] as const; + +/** The per-source rate limiter, direct or through the shared webhook helper. */ +const LIMITER_NEEDLES = [ + "checkRateLimit(", + "checkAuthSurfaceRateLimit(", + "applyWebhookRateLimit(", +] as const; + +/** Drop whole-line comments so a docstring mentioning a helper is not a call. */ +function stripCommentLines(text: string): string { + return text + .split("\n") + .map((line) => { + const trimmed = line.trim(); + if ( + trimmed.startsWith("//") || + trimmed.startsWith("*") || + trimmed.startsWith("/*") + ) { + return ""; + } + return line; + }) + .join("\n"); +} + +/** Index of the earliest needle in `text`, or -1 when none appears. */ +function firstIndexOfAny(text: string, needles: ReadonlyArray): number { + let best = -1; + for (const needle of needles) { + const at = text.indexOf(needle); + if (at === -1) continue; + if (best === -1 || at < best) best = at; + } + return best; +} + +/** Every `route.ts` under `src/app/api`, as repo-relative POSIX paths. */ +function findRouteFiles(): string[] { + const hits: string[] = []; + + function walk(dir: string): void { + for (const entry of readdirSync(dir)) { + const full = join(dir, entry); + if (statSync(full).isDirectory()) { + if (entry === "__tests__" || entry === "node_modules") continue; + walk(full); + continue; + } + if (entry !== "route.ts") continue; + hits.push(relative(repoRoot, full).split(/[\\/]/).join("/")); + } + } + + walk(resolve(repoRoot, API_ROOT)); + return hits.sort(); +} + +function findWebhookRoutes(): string[] { + const byPath = findRouteFiles().filter((p) => + p.toLowerCase().includes("webhook"), + ); + return [...new Set([...byPath, ...EXTRA_WEBHOOK_ROUTES])].sort(); +} + +/** + * Split a module into its top-level declarations. Crude on purpose: a + * declaration runs from its own `const`/`function`/`export` keyword at column + * zero to the next one. Good enough to attribute a call to the handler or the + * helper that contains it. + */ +function topLevelBlocks(text: string): Array<{ name: string; body: string }> { + const starts: Array<{ name: string; at: number }> = []; + const re = + /^(?:export\s+)?(?:async\s+)?(?:function|const|let|class)\s+([A-Za-z0-9_$]+)/gm; + let m: RegExpExecArray | null; + while ((m = re.exec(text)) !== null) { + starts.push({ name: m[1], at: m.index }); + } + return starts.map((s, i) => ({ + name: s.name, + body: text.slice( + s.at, + i + 1 < starts.length ? starts[i + 1].at : undefined, + ), + })); +} + +/** + * The local helper names that transitively reach one of `needles`. Two passes + * over the declaration list resolve one level of delegation + * (`checkAndWarn` → `hasValidWebhookSecret` → `timingSafeStringEqual`), which + * is the depth the webhook routes actually use; a third pass changes nothing + * today and would be the place to grow if a route nests deeper. + */ +function helpersReaching( + blocks: ReadonlyArray<{ name: string; body: string }>, + needles: ReadonlyArray, +): Set { + const reaching = new Set(); + for (let pass = 0; pass < 4; pass += 1) { + let grew = false; + for (const block of blocks) { + if (reaching.has(block.name)) continue; + const direct = firstIndexOfAny(block.body, needles) !== -1; + const viaHelper = [...reaching].some((h) => block.body.includes(`${h}(`)); + if (direct || viaHelper) { + reaching.add(block.name); + grew = true; + } + } + if (!grew) break; + } + return reaching; +} + +interface VerbVerdict { + path: string; + verb: string; + comparisonAt: number; + limiterAt: number; +} + +function analyseRoute(path: string): VerbVerdict[] { + const text = stripCommentLines(readFileSync(resolve(repoRoot, path), "utf8")); + const blocks = topLevelBlocks(text); + + const comparisonHelpers = helpersReaching(blocks, COMPARISON_NEEDLES); + const limiterHelpers = helpersReaching(blocks, LIMITER_NEEDLES); + + const verdicts: VerbVerdict[] = []; + for (const block of blocks) { + if (!HTTP_VERBS.includes(block.name as (typeof HTTP_VERBS)[number])) { + continue; + } + if (!new RegExp(`^export\\s+const\\s+${block.name}\\b`).test(block.body)) { + continue; + } + + const comparisonAt = firstIndexOfAny(block.body, [ + ...COMPARISON_NEEDLES, + ...[...comparisonHelpers] + .filter((h) => h !== block.name) + .map((h) => `${h}(`), + ]); + if (comparisonAt === -1) continue; + + const limiterAt = firstIndexOfAny(block.body, [ + ...LIMITER_NEEDLES, + ...[...limiterHelpers] + .filter((h) => h !== block.name) + .map((h) => `${h}(`), + ]); + + verdicts.push({ path, verb: block.name, comparisonAt, limiterAt }); + } + return verdicts; +} + +describe("webhook verb rate-limit guard", () => { + const routes = findWebhookRoutes(); + const verdicts = routes.flatMap(analyseRoute); + + it("finds the webhook routes it claims to judge", () => { + // An empty match set is the failure mode this project has already been + // bitten by: a guard that matches nothing is green because it looked at + // nothing. Both counts are floors, not inventories. + expect(routes.length).toBeGreaterThanOrEqual(5); + expect( + routes, + "the Withings path-segment webhook must be in the scanned set", + ).toContain("src/app/api/withings/webhook/[token]/route.ts"); + expect(routes).toContain("src/app/api/telegram/webhook/route.ts"); + expect(routes).toContain("src/app/api/internal/deploy-webhook/route.ts"); + }); + + it("recognises the verbs that compare a secret", () => { + expect( + verdicts.length, + "no secret-comparing webhook verb was recognised — the matchers have " + + "drifted away from the code they are supposed to judge", + ).toBeGreaterThanOrEqual(8); + }); + + it("every secret-comparing verb rate-limits before it compares", () => { + const offenders = verdicts + .filter((v) => v.limiterAt === -1 || v.limiterAt > v.comparisonAt) + .map((v) => + v.limiterAt === -1 + ? ` - ${v.path} → ${v.verb}: compares the secret with no rate limit at all` + : ` - ${v.path} → ${v.verb}: rate limit runs AFTER the comparison`, + ); + + expect( + offenders, + [ + "Webhook verbs that compare a shared secret without a rate limit in front:", + ...offenders, + "", + "A 200-against-401 answer is an oracle. Constant-time comparison hides", + "the timing channel, not the status code — the limiter is what makes", + "guessing expensive. Apply the same per-source limit the POST uses,", + "on the same bucket, before the comparison.", + ].join("\n"), + ).toEqual([]); + }); +}); diff --git a/src/app/api/internal/deploy-webhook/__tests__/route.test.ts b/src/app/api/internal/deploy-webhook/__tests__/route.test.ts index 621248027..322eea66b 100644 --- a/src/app/api/internal/deploy-webhook/__tests__/route.test.ts +++ b/src/app/api/internal/deploy-webhook/__tests__/route.test.ts @@ -329,6 +329,31 @@ describe("GET /api/internal/deploy-webhook (reachability check)", () => { const res = await GET(req); expect(res.status).toBe(401); }); + + // The reachability GET compares the same shared secret the POST does and + // answers 200 against 401. Unlimited, that is a free oracle for guessing it. + // A VALID secret over the limit must still be refused — that proves the + // limiter runs before the comparison, not merely beside it. + it("returns 429 for a valid secret when over the limit, on the POST bucket", async () => { + vi.mocked(checkRateLimit).mockResolvedValue({ + allowed: false, + remaining: 0, + resetAt: Date.now() + 60_000, + } as never); + + const res = await GET( + new NextRequest("http://localhost/api/internal/deploy-webhook", { + method: "GET", + headers: { "x-deploy-webhook-secret": "test-secret" }, + }), + ); + + expect(res.status).toBe(429); + expect(checkRateLimit).toHaveBeenCalledTimes(1); + expect(String(vi.mocked(checkRateLimit).mock.calls[0][0])).toMatch( + /^deploy-webhook:/, + ); + }); }); describe("POST /api/internal/deploy-webhook (timestamp replay protection)", () => { diff --git a/src/app/api/internal/deploy-webhook/route.ts b/src/app/api/internal/deploy-webhook/route.ts index ab60010e5..a7a191a86 100644 --- a/src/app/api/internal/deploy-webhook/route.ts +++ b/src/app/api/internal/deploy-webhook/route.ts @@ -202,24 +202,39 @@ async function notifyAdminsOfFailure(event: NormalizedEvent): Promise { } } -export const POST = apiHandler(async (request: NextRequest) => { - annotate({ action: { name: "deploy.webhook" } }); - - // Rate-limit by client IP. 60 requests / minute is comfortably above - // Coolify's per-event burst (typically 1-2 per deploy) but well below - // a hostile actor flooding to fish for a working secret. +/** + * Rate-limit by client IP. 60 requests / minute is comfortably above + * Coolify's per-event burst (typically 1-2 per deploy) but well below a + * hostile actor flooding to fish for a working secret. + * + * Both verbs share the bucket, and both run it before the comparison: the + * reachability GET checks the same secret and answers 200 against 401, so + * leaving it unlimited would hand back the free guessing channel the POST + * limit exists to close. + */ +async function applyWebhookRateLimit( + request: NextRequest, +): Promise { const ip = getClientIp(request); const rl = await checkRateLimit( `deploy-webhook:${ip ?? "unknown"}`, 60, 60_000, ); - if (!rl.allowed) { - return NextResponse.json( - { status: "rate_limited" }, - { status: 429, headers: rateLimitHeaders(rl) }, - ); - } + if (rl.allowed) return null; + return NextResponse.json( + { status: "rate_limited" }, + { status: 429, headers: rateLimitHeaders(rl) }, + ); +} + +export const POST = apiHandler(async (request: NextRequest) => { + annotate({ action: { name: "deploy.webhook" } }); + + const limited = await applyWebhookRateLimit(request); + if (limited) return limited; + + const ip = getClientIp(request); if (!hasValidSecret(request)) { return NextResponse.json({ status: "unauthorized" }, { status: 401 }); @@ -293,6 +308,10 @@ export const POST = apiHandler(async (request: NextRequest) => { */ export const GET = apiHandler(async (request: NextRequest) => { annotate({ action: { name: "deploy.webhook.verify" } }); + + const limited = await applyWebhookRateLimit(request); + if (limited) return limited; + if (!hasValidSecret(request)) { return NextResponse.json({ status: "unauthorized" }, { status: 401 }); } diff --git a/src/app/api/telegram/webhook/__tests__/route.test.ts b/src/app/api/telegram/webhook/__tests__/route.test.ts index ecb653d87..d9a1a364b 100644 --- a/src/app/api/telegram/webhook/__tests__/route.test.ts +++ b/src/app/api/telegram/webhook/__tests__/route.test.ts @@ -1155,3 +1155,29 @@ describe("Telegram webhook — interactive measurement reminder (v1.19.0)", () = expect(deleteMessage).toHaveBeenCalled(); }); }); + +describe("Telegram webhook — GET verification is rate-limited first", () => { + // The reachability GET compares the same bot secret the POST does and + // answers 200 against 401. Without the limit in front it is an unthrottled + // oracle for guessing that secret, so a VALID secret over the limit must + // still be refused — that ordering is the whole point. + it("returns 429 for a valid secret when over the limit", async () => { + vi.mocked(checkRateLimit).mockResolvedValue({ + allowed: false, + remaining: 0, + resetAt: Date.now() + 60_000, + } as never); + + const res = await GET( + new NextRequest("http://localhost/api/telegram/webhook", { + headers: { "x-telegram-bot-api-secret-token": "test-secret" }, + }), + ); + + expect(res.status).toBe(429); + expect(checkRateLimit).toHaveBeenCalledTimes(1); + expect(String(vi.mocked(checkRateLimit).mock.calls[0][0])).toMatch( + /^telegram-webhook:/, + ); + }); +}); diff --git a/src/app/api/telegram/webhook/route.ts b/src/app/api/telegram/webhook/route.ts index fd9a5b3a8..64d00aac6 100644 --- a/src/app/api/telegram/webhook/route.ts +++ b/src/app/api/telegram/webhook/route.ts @@ -32,17 +32,29 @@ function hasValidSecret(request: NextRequest): boolean { return timingSafeEqual(a, b); } +/** + * The per-source limit both verbs share, on one bucket. It runs before the + * secret comparison on every verb that performs one: the comparison answers + * 200 against 401, and a constant-time compare hides the timing channel but + * not the status code, so the limiter is what makes guessing expensive. + */ +async function applyWebhookRateLimit( + request: NextRequest, +): Promise { + const ip = getClientIp(request); + const rl = await checkRateLimit(`telegram-webhook:${ip}`, 120, 60 * 1000); + if (rl.allowed) return null; + return NextResponse.json( + { status: "rate_limited" }, + { status: 429, headers: rateLimitHeaders(rl) }, + ); +} + export const POST = apiHandler(async (request: NextRequest) => { annotate({ action: { name: "telegram.webhook" } }); - const ip = getClientIp(request); - const rl = await checkRateLimit(`telegram-webhook:${ip}`, 120, 60 * 1000); - if (!rl.allowed) { - return NextResponse.json( - { status: "rate_limited" }, - { status: 429, headers: rateLimitHeaders(rl) }, - ); - } + const limited = await applyWebhookRateLimit(request); + if (limited) return limited; if (!hasValidSecret(request)) { return NextResponse.json({ status: "unauthorized" }, { status: 401 }); @@ -84,6 +96,9 @@ export const POST = apiHandler(async (request: NextRequest) => { export const GET = apiHandler(async (request: NextRequest) => { annotate({ action: { name: "telegram.webhook.verify" } }); + const limited = await applyWebhookRateLimit(request); + if (limited) return limited; + if (!hasValidSecret(request)) { return NextResponse.json({ status: "unauthorized" }, { status: 401 }); } diff --git a/src/app/api/withings/webhook/[token]/__tests__/route.test.ts b/src/app/api/withings/webhook/[token]/__tests__/route.test.ts index 96e2402a8..727081148 100644 --- a/src/app/api/withings/webhook/[token]/__tests__/route.test.ts +++ b/src/app/api/withings/webhook/[token]/__tests__/route.test.ts @@ -216,3 +216,64 @@ describe("GET /api/withings/webhook/[token]", () => { expect(bad.status).toBe(401); }); }); + +describe("verification verbs are rate-limited before the secret comparison", () => { + // A verification verb answers 200 for the right token and 401 for a wrong + // one. Without a limit in front that is a free oracle for guessing the + // secret — constant-time comparison hides the timing channel, not the + // status code. Handing each verb a VALID token while over the limit is the + // sharp proof of ordering: the comparison would have said 200. + function overTheLimit(): void { + vi.mocked(checkRateLimit).mockResolvedValue({ + allowed: false, + remaining: 0, + resetAt: Date.now() + 60_000, + } as never); + } + + it("HEAD returns 429 for a valid token when over the limit", async () => { + overTheLimit(); + const res = await HEAD( + new NextRequest("http://localhost/api/withings/webhook/test-secret", { + method: "HEAD", + }), + paramsFor("test-secret"), + ); + expect(res.status).toBe(429); + expect(checkRateLimit).toHaveBeenCalledTimes(1); + }); + + it("GET returns 429 for a valid token when over the limit", async () => { + overTheLimit(); + const res = await GET( + new NextRequest("http://localhost/api/withings/webhook/test-secret"), + paramsFor("test-secret"), + ); + expect(res.status).toBe(429); + expect(checkRateLimit).toHaveBeenCalledTimes(1); + }); + + it("HEAD and GET share the POST bucket", async () => { + await HEAD( + new NextRequest("http://localhost/api/withings/webhook/test-secret", { + method: "HEAD", + }), + paramsFor("test-secret"), + ); + await GET( + new NextRequest("http://localhost/api/withings/webhook/test-secret"), + paramsFor("test-secret"), + ); + await POST( + jsonRequest("test-secret", { userid: "wu-1" }), + paramsFor("test-secret"), + ); + + const buckets = vi + .mocked(checkRateLimit) + .mock.calls.map((call) => String(call[0])); + expect(buckets).toHaveLength(3); + expect(new Set(buckets).size).toBe(1); + expect(buckets[0]).toMatch(/^withings-webhook:/); + }); +}); diff --git a/src/app/api/withings/webhook/[token]/route.ts b/src/app/api/withings/webhook/[token]/route.ts index 523c51e9e..5925915e9 100644 --- a/src/app/api/withings/webhook/[token]/route.ts +++ b/src/app/api/withings/webhook/[token]/route.ts @@ -55,10 +55,23 @@ export const POST = apiHandler( /** * Withings sends HEAD/GET to verify the URL when a subscription is * created. + * + * The verification verbs carry the same rate limit as the notification POST, + * on the same bucket. They compare the same shared secret and answer 200 + * against 401, so without it they are a free oracle for guessing the token — + * a constant-time comparison hides the timing channel, not the status code. */ export const HEAD = apiHandler( - async (_request: NextRequest, context: RouteContext) => { + async (request: NextRequest, context: RouteContext) => { annotate({ action: { name: "withings.webhook.verify" } }); + + // HEAD carries no body, so the limiter's JSON envelope is re-shaped into + // a bodyless 429 that keeps the rate-limit headers. + const limited = await applyWebhookRateLimit(request); + if (limited) { + return new NextResponse(null, { status: 429, headers: limited.headers }); + } + const { token } = await context.params; if (!(await verifyTokenSegment(token))) { return new NextResponse(null, { status: 401 }); @@ -69,8 +82,12 @@ export const HEAD = apiHandler( ); export const GET = apiHandler( - async (_request: NextRequest, context: RouteContext) => { + async (request: NextRequest, context: RouteContext) => { annotate({ action: { name: "withings.webhook.verify" } }); + + const limited = await applyWebhookRateLimit(request); + if (limited) return limited; + const { token } = await context.params; if (!(await verifyTokenSegment(token))) { return NextResponse.json({ status: "unauthorized" }, { status: 401 }); diff --git a/src/app/api/withings/webhook/__tests__/route.test.ts b/src/app/api/withings/webhook/__tests__/route.test.ts index 9c9efb37b..569544557 100644 --- a/src/app/api/withings/webhook/__tests__/route.test.ts +++ b/src/app/api/withings/webhook/__tests__/route.test.ts @@ -473,3 +473,53 @@ describe("GET /api/withings/webhook", () => { expect(fail.status).toBe(401); }); }); + +describe("legacy verification verbs are rate-limited before the secret comparison", () => { + // Same reasoning as the path-segment sibling: a verb that answers 200 for + // the right secret and 401 for a wrong one is a guessing oracle unless the + // limiter runs first. A VALID secret over the limit must still be refused. + function overTheLimit(): void { + vi.mocked(checkRateLimit).mockResolvedValue({ + allowed: false, + remaining: 0, + resetAt: Date.now() + 60_000, + } as never); + } + + it("HEAD returns 429 for a valid secret when over the limit", async () => { + overTheLimit(); + const res = await HEAD( + new NextRequest("http://localhost/api/withings/webhook", { + method: "HEAD", + headers: { "x-withings-webhook-secret": "test-secret" }, + }), + ); + expect(res.status).toBe(429); + expect(checkRateLimit).toHaveBeenCalledTimes(1); + }); + + it("GET returns 429 for a valid secret when over the limit", async () => { + overTheLimit(); + const res = await GET( + new NextRequest("http://localhost/api/withings/webhook", { + headers: { "x-withings-webhook-secret": "test-secret" }, + }), + ); + expect(res.status).toBe(429); + expect(checkRateLimit).toHaveBeenCalledTimes(1); + }); + + it("the legacy ?secret= form is limited on the same bucket as the POST", async () => { + overTheLimit(); + const res = await GET( + new NextRequest("http://localhost/api/withings/webhook?secret=WRONG"), + ); + expect(res.status).toBe(429); + + const buckets = vi + .mocked(checkRateLimit) + .mock.calls.map((call) => String(call[0])); + expect(buckets).toHaveLength(1); + expect(buckets[0]).toMatch(/^withings-webhook:/); + }); +}); diff --git a/src/app/api/withings/webhook/route.ts b/src/app/api/withings/webhook/route.ts index da695f6e8..a5b2ea7ac 100644 --- a/src/app/api/withings/webhook/route.ts +++ b/src/app/api/withings/webhook/route.ts @@ -91,9 +91,21 @@ export const POST = apiHandler(async (request: NextRequest) => { /** * Withings sends a HEAD request to verify the webhook URL. + * + * Rate-limited on the same bucket as the notification POST, and before the + * comparison: HEAD and GET check the same shared secret and answer 200 against + * 401, which is an oracle for guessing it unless the limiter runs first. */ export const HEAD = apiHandler(async (request: NextRequest) => { annotate({ action: { name: "withings.webhook.verify" } }); + + // HEAD carries no body, so the limiter's JSON envelope is re-shaped into a + // bodyless 429 that keeps the rate-limit headers. + const limited = await applyWebhookRateLimit(request); + if (limited) { + return new NextResponse(null, { status: 429, headers: limited.headers }); + } + const auth = await hasValidWebhookSecret(request); if (!auth.ok) { return new NextResponse(null, { status: 401 }); @@ -104,6 +116,10 @@ export const HEAD = apiHandler(async (request: NextRequest) => { export const GET = apiHandler(async (request: NextRequest) => { annotate({ action: { name: "withings.webhook.verify" } }); + + const limited = await applyWebhookRateLimit(request); + if (limited) return limited; + const auth = await hasValidWebhookSecret(request); if (!auth.ok) { return NextResponse.json({ status: "unauthorized" }, { status: 401 }); From 3989d9e9f9fcd09db21c05edbecb1484524114c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Thu, 3 Sep 2026 01:46:01 +0200 Subject: [PATCH 2/2] test(modules): derive the route-gate inventory from the module registry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The inventory walked a flat, hand-written array of route trees, so its coverage grew only when somebody remembered to grow it. It had stopped growing: the coach, environment, mental-health, inbound-document and MCP trees were all outside it, and so was the doctor-report export route the header already named as a doctorReport surface. Every route in the first four turned out to be gated, so nothing leaked — but that was the authors' care rather than the inventory's, and a green run claimed more than it covered. The trees are declared per module key as an exhaustive Record: a new registry key does not compile without an entry, one test asserts every key is declared, another asserts every declared tree exists, and a third discovers an API directory named after a module key on disk and fails when it is not declared. An empty array is now a claim with a reason beside it (glucose and recovery ride the measurement engine), not a silence. Walking the newly covered trees classifies every route: coach, environment, mental-health, the document vault and /api/export/health-record all gate directly. The seven /api/mcp routes are the credential surface behind the remote endpoint and go on the exempt list with the reason written out — the endpoint itself lives outside /api and carries the gate, so a credential minted while the module is off is inert. That refusal is added to the behavioural list so deleting its proof fails here. /api/export/health-record joins /api/fhir as a key-pinned tree: both serve the whole-record aggregate, so doctorReport is the only correct key. --- CHANGELOG.md | 17 ++ .../module-route-gate-inventory.test.ts | 282 ++++++++++++++---- 2 files changed, 236 insertions(+), 63 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b89c9f1e7..ced85c850 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,23 @@ ## [Unreleased] +### Fixed + +- **The check that watches for ungated module routes now watches every + module.** It walked a hand-written list of route trees, and the list had + stopped keeping up: the Coach tree, the environmental-context tree, the + mental-health screeners, the inbound-document vault and the MCP credential + surface all shipped an API tree the check never looked at. Nothing was + leaking — every route in those trees already refuses when the module is off, + and the doctor-report export was gated too — but a green run was saying + something narrower than it appeared to say, which is how the FHIR routes once + sat in a green bucket while serving the whole record. The trees are declared + per module now, so a new module cannot be added without saying where its + routes live, and a directory named after a module is found on disk rather + than remembered. For a self-hoster this changes no behaviour today; it is the + difference between a module switch that is enforced and one that is only + believed to be. + ### Security - **Every webhook verb that checks the shared secret is rate-limited now, not diff --git a/src/app/api/__tests__/module-route-gate-inventory.test.ts b/src/app/api/__tests__/module-route-gate-inventory.test.ts index f7e72fbdf..da860c1da 100644 --- a/src/app/api/__tests__/module-route-gate-inventory.test.ts +++ b/src/app/api/__tests__/module-route-gate-inventory.test.ts @@ -74,80 +74,125 @@ import { MODULE_KEYS, type ModuleKey } from "@/lib/modules/registry"; * `BEHAVIOURAL_GATE_TESTS`. This test asserts those files still exist, so * deleting the behavioural coverage fails here with a pointer to what was * lost rather than silently leaving only a text-match behind. + * + * WHICH TREES GET WALKED, AND WHY THAT IS NOT A HAND-WRITTEN LIST ANY MORE + * ----------------------------------------------------------------------- + * A discovery test is only worth what its discovery covers, and this one used + * to enumerate its trees in a flat array that grew when somebody remembered to + * grow it. It had stopped: `coach`, `environment`, `mentalHealth`, + * `inboundDocuments` and `mcp` each shipped an API tree that this file never + * looked at. No leak came of it — every route in the first four gates — but + * that was the authors' care and not this test's, and a green run was saying + * something narrower than it appeared to say. + * + * The trees are now declared per module key as an exhaustive + * `Record`, so the REGISTRY drives coverage: a new key does not + * compile without an entry, and an API directory named after a key is + * discovered from the filesystem and must be declared for it. A tree at a + * non-obvious address (`achievements` → `/api/gamification`) still has to be + * named by hand — that is the residual gap, and it is the reason the + * declaration carries a comment per key rather than a bare path. */ /** - * The per-domain route trees that serve a toggleable module. Each entry - * is a directory under `src/app/api`; every `route.ts` beneath it is - * enumerated and classified. A domain that is owned entirely by one - * builder (doctor-report / FHIR) or one delegated gate (cycle) is still - * walked so a NEW route in the tree must justify itself. + * The per-domain API route trees each toggleable module owns, keyed by the + * registry key. + * + * Declared as an exhaustive `Record` so the REGISTRY drives + * coverage rather than anybody's memory: adding a key to `MODULE_KEYS` without + * deciding which API tree it owns does not compile, and the two tests below + * catch the same omission at runtime — one asserting every registry key is + * declared, one asserting that an API directory NAMED after a module key is + * declared for that key. + * + * The hand-written array this replaced could only grow when a contributor + * remembered to grow it, and it had stopped growing: `coach`, `environment`, + * `mentalHealth`, `inboundDocuments` and `mcp` all shipped an API tree that + * this inventory never walked. Every route in the first four turned out to be + * gated, so nothing leaked — but that was the authors' care, not this file's + * doing, and the next tree would have been just as invisible. + * + * An empty array is a claim, not a gap: the module owns no API tree of its + * own, and the entry says where its data actually lives. + * + * A domain owned entirely by one builder (doctor-report / FHIR) or one + * delegated gate (cycle) is still walked, so a NEW route in the tree must + * justify itself. */ -const MODULE_ROUTE_TREES: ReadonlyArray = [ - "src/app/api/mood", - "src/app/api/mood-entries", - "src/app/api/sleep", - "src/app/api/workouts", - "src/app/api/labs", +const MODULE_ROUTE_TREES_BY_KEY: Readonly< + Record +> = { + cycle: ["src/app/api/cycle"], + mood: ["src/app/api/mood", "src/app/api/mood-entries"], + sleep: ["src/app/api/sleep"], + // Glucose has no tree of its own: readings are Measurement rows on the core + // measurement engine, and the glucose panel is an insights surface. The + // module gates the surfaces, which live under trees already walked here. + glucose: [], + workouts: ["src/app/api/workouts"], + // Recovery, like glucose, rides the measurement engine — no dedicated tree. + recovery: [], // v1.18.1 — the user-scoped Biomarker catalog backs the Labs feature. - "src/app/api/biomarkers", - "src/app/api/cycle", + labs: ["src/app/api/labs", "src/app/api/biomarkers"], // v1.18.1 (W-B) — the illness/condition journal. Every `/api/illness/*` // route gates on the `illness` module via the thin // `requireIllnessEnabled(...)` wrapper (which re-stamps the // illness-specific errorCode over `requireModuleEnabled("illness")`), - // recognised below as a delegated gate. Walking the tree means a NEW - // ungated illness route fails this test BY NAME rather than leaking the - // surface over a Bearer token when the account turned the module off. - "src/app/api/illness", - "src/app/api/gamification", + // recognised below as a delegated gate. + illness: ["src/app/api/illness"], + achievements: ["src/app/api/gamification"], + // The Coach tree. Every route calls `requireModuleEnabled(userId, "coach")` + // directly, which resolves the delegated two-layer state (assistant master + // flag AND the per-user opt-out) through the registry — so the routes are + // counted as directly gated here, and the sibling + // `coach-route-gate-inventory.test.ts` covers the surface in depth. + coach: ["src/app/api/coach"], + // v1.18.0 (B2) — the AI-narrative insights tree, plus v1.28's unified + // daily-digest read (`GET /api/daily/digest`), which is the AI-narrative + // daily layer and gates on `insights` directly. + insights: ["src/app/api/insights", "src/app/api/daily"], // v1.18.1 (D3) — medications graduated from CORE to a toggleable module. - // It is SURFACE-gated (nav entry, dashboard widget, the dedicated - // Medikamente settings entry), not data-layer-gated: every `/api/medications/*` - // route is raw medication CRUD / intake / inventory / compliance over the - // user's own rows, so they are EXEMPT below under the same data-layer - // reasoning as mood/labs — disabling the module hides the surface, it does - // not wedge an importer / sync / the user's ability to clean up, and - // re-enabling finds the schedule + intake history intact. Walking the tree - // means a NEW medication route must justify itself (gate or document-exempt) - // rather than silently appearing. - "src/app/api/medications", - // v1.18.0 B3 — the legacy `/api/doctor-report` tree (JSON + server-PDF + - // availability probe) was orphaned dead code (no production caller) and - // removed. The doctor-report surface is `/api/export/health-record` plus - // the FHIR REST face below; both gate on `doctorReport` directly. The FHIR - // routes previously relied on the removed builder-mention bucket and were - // in practice ungated — see the header note. - "src/app/api/fhir", - // v1.18.0 (B2) — the AI-narrative insights tree. Every status / cards / - // correlations / derived / narrative / pregenerate / rhythm-events route - // gates on the `insights` module; the Coach sub-tree delegates to the - // coach assistant surface; the AI-settings / tile-layout / therapy-timeline - // infra-and-config routes are EXEMPT below. Walking the tree means a NEW - // ungated insights AI route fails this test BY NAME rather than leaking the - // surface over a Bearer token when the account turned insights off. - "src/app/api/insights", - // v1.28 — the unified daily-digest read (`GET /api/daily/digest`). The digest - // is the AI-narrative daily layer, so it gates on the `insights` module via - // `requireModuleEnabled(user.id, "insights")` directly. Walking the tree means - // a NEW ungated daily route fails BY NAME rather than leaking over a Bearer - // token when the account turned insights off. - "src/app/api/daily", - // v1.28 — the nutrient-intake sync (opt-in `nutrients` module). Unlike the + // SURFACE-gated (nav entry, dashboard widget, the dedicated Medikamente + // settings entry), not data-layer-gated: every `/api/medications/*` route is + // raw CRUD / intake / inventory / compliance over the user's own rows, so + // they are EXEMPT below under the same data-layer reasoning as mood/labs. + medications: ["src/app/api/medications"], + // v1.18.0 B3 — the legacy `/api/doctor-report` tree was orphaned dead code + // and removed. The doctor-report surface is `/api/export/health-record` plus + // the FHIR REST face; both gate on `doctorReport` directly, and both are + // key-pinned below so a future route cannot satisfy the inventory by gating + // on some other, more permissive module. + doctorReport: ["src/app/api/fhir", "src/app/api/export/health-record"], + // v1.25.0 (W-ENV) — the environmental-context tree. Every route calls + // `requireModuleEnabled(user.id, "environment")`, including the geocode + // lookup and the backfill, because each one either reads the module's data + // or spends the outbound weather/geocode budget on the user's behalf. + environment: ["src/app/api/environment"], + // v1.22.0 — the remote MCP endpoint is `src/app/mcp/route.ts`, OUTSIDE the + // `/api` tree, and it gates on the module itself. What lives under + // `/api/mcp` is the credential-management surface behind it, exempt below. + mcp: ["src/app/api/mcp"], + // v1.25.0 (W-DOCS-IN) — the inbound clinical-document vault. Every route + // gates on `inboundDocuments`; the AI READ of a document is a separate + // per-user opt-in on top. + inboundDocuments: ["src/app/api/documents/inbound"], + // v1.25.0 — the mental-health screeners. Both routes gate on `mentalHealth`. + mentalHealth: ["src/app/api/mental-health"], + // v1.28 — the nutrient-intake sync (opt-in module). Unlike the // data-layer-exempt siblings this domain is REFUSE-INGEST-WHEN-OFF (the - // mental-health posture): both the batch ingest and the window-summary - // read call `requireModuleEnabled(user.id, "nutrients")` directly, so a - // phone whose user never opted in cannot land rows server-side. Walking - // the tree means a NEW ungated nutrients route fails BY NAME. - "src/app/api/nutrients", - // v1.38.0 — the immunization log (`vaccinations` module). SURFACE-gated - // like medications, not data-layer-gated: every `/api/vaccinations/*` route - // is raw CRUD over the person's own doses, so all four are EXEMPT below - // under the same reasoning. Walking the tree means a NEW vaccination route - // must justify itself — gate or documented exemption — rather than - // silently appearing. - "src/app/api/vaccinations", -]; + // mental-health posture): both the batch ingest and the window-summary read + // gate directly, so a phone whose user never opted in cannot land rows. + nutrients: ["src/app/api/nutrients"], + // v1.38.0 — the immunization log. SURFACE-gated like medications, not + // data-layer-gated: every `/api/vaccinations/*` route is raw CRUD over the + // person's own doses, so all six are EXEMPT below under the same reasoning. + vaccinations: ["src/app/api/vaccinations"], +}; + +/** Every declared tree, de-duplicated — the set this inventory walks. */ +const MODULE_ROUTE_TREES: ReadonlyArray = [ + ...new Set(Object.values(MODULE_ROUTE_TREES_BY_KEY).flat()), +].sort(); /** * EXEMPT — routes that serve a toggleable domain but are deliberately @@ -309,6 +354,33 @@ const EXEMPT_ROUTES: ReadonlyArray = [ // while the nav entry, the picker and the report leaf hide. "src/app/api/vaccinations/[id]/booster/route.ts", "src/app/api/vaccinations/suggest/route.ts", + // ── INFRA / CREDENTIALS (mcp) ───────────────────────────────────── + // The remote MCP endpoint itself is `src/app/mcp/route.ts`, outside `/api`, + // and it carries the gate: an account with the module off gets a 404 there + // regardless of the credential presented (proven by the behavioural test + // listed below). What lives under `/api/mcp` is the credential surface + // BEHIND that endpoint, and it is deliberately not gated: + // + // - the token list / mint / revoke and the connection rows are the + // cookie-session settings card. Revoking must keep working while the + // module is off — that is precisely when somebody wants to revoke — and + // minting happens in the same breath as switching the module on. + // - the OAuth bridge mints and exchanges the same `health:read` + // credential for a cloud connector. It runs on `withBackgroundEvent` + // with its protections applied by hand (rate limit, body cap, kill + // switch, mandatory PKCE-S256, redirect-URI allowlist) per the + // documented exception, and the credential it issues is inert while the + // module is off because `/mcp` refuses it. + // + // Gating these would not close a data path; it would only wedge the surface + // that turns the module on and off. + "src/app/api/mcp/tokens/route.ts", + "src/app/api/mcp/tokens/[id]/route.ts", + "src/app/api/mcp/connections/route.ts", + "src/app/api/mcp/connections/[id]/route.ts", + "src/app/api/mcp/oauth/authorize/route.ts", + "src/app/api/mcp/oauth/token/route.ts", + "src/app/api/mcp/oauth/register/route.ts", ]; const MODULE_GATE_NEEDLE = "requireModuleEnabled("; @@ -333,6 +405,7 @@ const ILLNESS_GATE_NEEDLE = "requireIllnessEnabled("; */ const REQUIRED_TREE_MODULE: Readonly> = { "src/app/api/fhir": "doctorReport", + "src/app/api/export/health-record": "doctorReport", }; /** @@ -368,6 +441,14 @@ const BEHAVIOURAL_GATE_TESTS: ReadonlyArray<{ path: string; proves: string }> = proves: "the dashboard summary drops disabled-module metric cards while core vitals still ship", }, + { + // The `mcp` module's enforcement point is outside `/api` entirely, so + // nothing in the tree walk above can see it. Listing the proof here is + // what keeps it from disappearing unnoticed. + path: "src/app/mcp/__tests__/route.test.ts", + proves: + "the remote MCP endpoint 404s when the mcp module is off, which is what makes the ungated /api/mcp credential surface safe", + }, ]; /** @@ -456,6 +537,12 @@ describe("module API route gate inventory", () => { "doctorReport", "insights", "medications", + "environment", + "mentalHealth", + "inboundDocuments", + "nutrients", + "mcp", + "vaccinations", ] as const) { expect(known.has(key), `unknown module key in inventory: ${key}`).toBe( true, @@ -463,6 +550,75 @@ describe("module API route gate inventory", () => { } }); + it("every module key declares which API trees it owns", () => { + // The type already makes an omission a compile error. This is the runtime + // half: it fails by NAME, so the message says which key was added without + // anybody deciding where its routes live. + const undeclared = MODULE_KEYS.filter( + (key) => !(key in MODULE_ROUTE_TREES_BY_KEY), + ); + + expect( + undeclared, + [ + "Module keys with no entry in MODULE_ROUTE_TREES_BY_KEY:", + ...undeclared.map((k) => ` - ${k}`), + "Name the module's API tree(s), or declare [] and say in the comment", + "where its data actually lives.", + ].join("\n"), + ).toEqual([]); + }); + + it("every declared route tree exists on disk", () => { + const missing = Object.entries(MODULE_ROUTE_TREES_BY_KEY).flatMap( + ([key, trees]) => + trees + .filter((tree) => !existsSync(resolve(repoRoot, tree))) + .map((tree) => ` - ${key}: ${tree}`), + ); + + expect( + missing, + [ + "MODULE_ROUTE_TREES_BY_KEY names directories that do not exist —", + "a renamed or deleted tree silently stops being walked:", + ...missing, + ].join("\n"), + ).toEqual([]); + }); + + it("an API directory named after a module key is declared for that key", () => { + // This is the check the hand-written array could not perform. A module + // whose routes live at the obvious address (`/api/environment` for + // `environment`, `/api/mental-health` for `mentalHealth`) is discovered + // from the filesystem, so a tree cannot sit outside the inventory just + // because nobody added a line here. Trees at a NON-obvious address + // (`achievements` → `/api/gamification`, `inboundDocuments` → + // `/api/documents/inbound`) still have to be declared by hand; the + // declaration is what this test compares against. + const apiRoot = resolve(repoRoot, "src/app/api"); + const undeclared: string[] = []; + + for (const key of MODULE_KEYS) { + const dirName = key.replace(/[A-Z]/g, (c) => `-${c.toLowerCase()}`); + const tree = `src/app/api/${dirName}`; + if (!existsSync(join(apiRoot, dirName))) continue; + if (MODULE_ROUTE_TREES_BY_KEY[key].includes(tree)) continue; + undeclared.push(` - ${key}: ${tree} exists but is not declared`); + } + + expect( + undeclared, + [ + "API trees named after a module key that this inventory does not walk:", + ...undeclared, + "Declare the tree under its key in MODULE_ROUTE_TREES_BY_KEY. Every", + "route beneath it then has to be gated, delegated, or exempt with a", + "documented reason.", + ].join("\n"), + ).toEqual([]); + }); + it("every toggleable-module route is gated, delegated, or explicitly exempt", () => { const routes = findAllModuleRouteFiles(); expect(routes.length).toBeGreaterThan(0);