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] 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 });