tom/HealthLog
1
0 Fork 0
mirror of https://github.com/MBombeck/HealthLog.git synced 2026-11-08 20:42:43 +00:00
Code Issues Packages Projects Releases Wiki Activity

Merge fix/module-gate-and-webhook

Browse Source
This commit is contained in:
Marc-André Bombeck committed 2026-09-03 03:23:37 +02:00
2 parents f6a45ca5c5 98324b0eff
commit a675daf0c1
11 files changed
+787 -85
Download Patch File Download Diff File Expand all files Collapse all files

No files matched your search

CHANGELOG.md
+32
View File
@@ -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
src/app/api/__tests__/module-route-gate-inventory.test.ts
+219 -63
View File
@@ -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<ModuleKey, …>`, 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<ModuleKey, …>` 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<string> = [
"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<ModuleKey, readonly string[]>
> = {
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<string> = [
...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<string> = [
// 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<Record<string, ModuleKey>> = {
"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);
src/app/api/__tests__/webhook-verb-rate-limit-guard.test.ts
+285
View File
@@ -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<string> = [
// 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<string>): 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<string>,
): Set<string> {
const reaching = new Set<string>();
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([]);
});
});
src/app/api/internal/deploy-webhook/__tests__/route.test.ts
+25
View File
@@ -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)", () => {
src/app/api/internal/deploy-webhook/route.ts
+31 -12
View File
@@ -202,24 +202,39 @@ async function notifyAdminsOfFailure(event: NormalizedEvent): Promise<void> {
}
}
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<NextResponse | null> {
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 });
}
src/app/api/telegram/webhook/__tests__/route.test.ts
+26
View File
@@ -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:/,
);
});
});
src/app/api/telegram/webhook/route.ts
+23 -8
View File
@@ -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<NextResponse | null> {
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 });
}
src/app/api/withings/webhook/[token]/__tests__/route.test.ts
+61
View File
@@ -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:/);
});
});
src/app/api/withings/webhook/[token]/route.ts
+19 -2
View File
@@ -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 });
src/app/api/withings/webhook/__tests__/route.test.ts
+50
View File
@@ -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:/);
});
});
src/app/api/withings/webhook/route.ts
+16
View File
@@ -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 });