diff --git a/CHANGELOG.md b/CHANGELOG.md index 6298166f5..864fdd0ad 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,38 @@ and the second from 6.9 ms to 1.6 ms, and both stop growing with the size of the archive. Nothing about the tiles changes: same numbers, same order, same metrics. +- **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 + 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 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); 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 });