diff --git a/.planning/phase-W17a-v1425-withings-webhook-secret-report.md b/.planning/phase-W17a-v1425-withings-webhook-secret-report.md new file mode 100644 index 000000000..98fa02061 --- /dev/null +++ b/.planning/phase-W17a-v1425-withings-webhook-secret-report.md @@ -0,0 +1,146 @@ +# Phase W17a — Withings webhook secret hardening + +**Status:** complete +**Branch:** develop +**Commit:** TBD on completion +**W10 finding addressed:** Security M-2 — Withings webhook secret travels via URL query parameter + +--- + +## Investigation + +### What Withings actually supports + +The Withings `Notify.subscribe` API accepts exactly six parameters: +`action`, `callbackurl`, `appli`, `client_id`, `nonce`, `signature` +(plus optional `comment`). There is **no facility for custom HTTP +headers** and **Withings does not sign outgoing webhook bodies**. + +Inbound webhook deliveries are `POST application/x-www-form-urlencoded` +with payload `userid` / `appli` / `startdate` / `enddate`, and no +Withings-supplied authenticity header (no `X-Withings-Signature`, +nothing equivalent). The subscriber's only authenticity surface is the +`callbackurl` itself. + +The W10 security review's suggestion to "move the secret to the +`X-Withings-Webhook-Secret` header at subscribe time" was based on an +inaccurate claim that Withings supports a `headers` object on +subscribe — confirmed false against the public docs +(`developer.withings.com/developer-guide/v3/data-api/notifications/notification-subscribe/`, +`developer.withings.com/developer-guide/v3/data-api/notifications/notification-content/`). + +### Chosen pattern + +**Path-segment binding.** +`?secret=` → `/` in the URL path. + +This is the largest practical shift away from query-string logging +that Withings supports end-to-end: + +- Most reverse proxies (nginx, Caddy, Coolify default config) log + `request_uri` AND a separate `query_string` column; moving the + secret out of the query string keeps it off the field every default + access-log template captures. +- GlitchTip URL/breadcrumb capture is already scrubbed of `?…` in + `reportToGlitchtip` (`src/lib/api-handler.ts:394-403`), but that + scrubber operates on `searchParams`; path segments would survive it, + which means we need to apply matching path-segment redaction at the + proxy in production. The redaction rule is a single + regex (`/api/withings/webhook/ → /api/withings/webhook/_`), + much narrower than the multiplicity of query-string secrets we'd + otherwise need to spell out. +- The path-segment value is constant-time-compared against + `WITHINGS_WEBHOOK_SECRET` server-side. Rotating the secret rotates + every subscription's URL on the next `setupWebhook` call, which + fires whenever a user re-OAuths (organic during v1.4.25 thanks to + W5d's `user.activity` scope upgrade). + +Header-based auth was rejected because Withings will not propagate any +header we configure — they ignore unknown subscribe parameters +silently, so a `headers` shim would only authorise manual replay tests +(which the legacy route already covers via +`X-Withings-Webhook-Secret`). + +HMAC signature verification was rejected because Withings does not +sign the payload — there's no shared signature for us to verify. + +## Implementation + +### New files + +- `src/app/api/withings/webhook/[token]/route.ts` — + path-segment Withings webhook route. POST / HEAD / GET, each + rate-limited and gated on a `timingSafeEqual` comparison between the + dynamic `[token]` segment and `process.env.WITHINGS_WEBHOOK_SECRET`. +- `src/lib/withings/webhook-handler.ts` — extracted shared body + processing (rate limit, form/JSON body decode, user lookup, + fire-and-forget sync). Used by both the new route and the legacy + one so the migration window doesn't drift between the two surfaces. +- `src/app/api/withings/webhook/[token]/__tests__/route.test.ts` — + 8 tests: valid token / wrong token / empty token / unset env / prefix- + match-rejection / rate-limited / HEAD verify / GET verify. +- `src/lib/withings/__tests__/sync.test.ts` — 3 tests confirming + `getWithingsWebhookCallbackUrl()` returns the path form, encodes + unsafe characters, and falls back gracefully without a secret. + +### Modified files + +- `src/lib/withings/sync.ts` — `getWithingsWebhookCallbackUrl()` now + emits `…/api/withings/webhook/` instead of + `…/api/withings/webhook?secret=`. New `setupWebhook` calls + hand the path-segment URL to Withings; the legacy form is no longer + generated anywhere in the codebase. +- `src/app/api/withings/webhook/route.ts` — refactored over the + shared helper. Legacy semantics preserved: query-string secret still + authorises (with a `getEvent().addWarning(…)` deprecation breadcrumb), + header secret still authorises (manual-replay path). Documented + removal target: **v1.4.27**, by which time every active Withings + subscription should have rotated through `setupWebhook` thanks to + the v1.4.25 OAuth-scope reconnect banner. + +### Backward compatibility + +Two-stage migration: + +1. **v1.4.25 (this release):** new subscriptions go via the path- + segment URL. Existing Withings subscriptions still target the + legacy `/api/withings/webhook?secret=…` URL — they keep working, + each delivery now emits a "migrate to /api/withings/webhook/[token]" + warning to the Wide Event stream. +2. **v1.4.26 — v1.4.27:** every active user reconnects at least once + thanks to W5d's scope upgrade. On reconnect, `setupWebhook` + re-subscribes against the path-segment URL. +3. **v1.4.27:** delete the legacy route. The deprecation-breadcrumb + count in monitoring is the gating signal. + +`getWithingsWebhookCallbackUrl()` is the single point of truth for the +URL handed to both `subscribeWebhook` (on connect) and +`unsubscribeWebhook` (on disconnect). The disconnect path uses the +**current** URL form, which is correct: Withings keys subscription +records by the exact callback URL, so a stale subscription created +under the legacy URL stays attached to the legacy URL — that +mismatch is harmless because the disconnect call wraps each appli +unsubscribe in `try { … } catch {}` already. + +## Quality gates + +| Gate | Result | +|-----------------|-----------------------------------| +| `pnpm typecheck`| pass | +| `pnpm lint` | pass | +| `pnpm test` | 4 files / 56 tests withings scope; 2665 / 1 skipped full suite | + +## Follow-ups + +- **Coolify/Caddy redaction rule (operational, not code):** add a + log-format rewrite that masks the final segment of + `/api/withings/webhook/` before access lines leave the + edge. Without this, the path-segment secret still appears in the + request line — the change above stops it from landing in the + separate `query_string` field, which was the bulk of the W10 + finding, but full defence-in-depth requires the proxy rule. Tracked + as a v1.4.25 W17a-op follow-up. +- **v1.4.27 cleanup:** delete the legacy `/api/withings/webhook` + route + its tests + the deprecation breadcrumb. Confirm + monitoring shows zero `auth_method=webhook_secret` events on the + legacy route over the prior 7 days before removal. diff --git a/src/app/api/withings/webhook/[token]/__tests__/route.test.ts b/src/app/api/withings/webhook/[token]/__tests__/route.test.ts new file mode 100644 index 000000000..5e8251c39 --- /dev/null +++ b/src/app/api/withings/webhook/[token]/__tests__/route.test.ts @@ -0,0 +1,201 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import { NextRequest } from "next/server"; + +// --- Module-boundary mocks must come before importing the route. --- + +vi.mock("@/lib/api-handler", () => ({ + apiHandler: unknown>(fn: T) => fn, +})); + +vi.mock("@/lib/db", () => ({ + prisma: { + withingsConnection: { findFirst: vi.fn() }, + }, +})); + +vi.mock("@/lib/withings/sync", () => ({ + syncUserMeasurements: vi.fn(), +})); + +vi.mock("@/lib/rate-limit", () => ({ + checkRateLimit: vi.fn(), + rateLimitHeaders: vi.fn(() => ({})), +})); + +vi.mock("@/lib/logging/context", () => ({ + annotate: vi.fn(), + getEvent: vi.fn(() => ({ + setAuth: vi.fn(), + addWarning: vi.fn(), + })), +})); + +import { POST, GET, HEAD } from "../route"; +import { prisma } from "@/lib/db"; +import { syncUserMeasurements } from "@/lib/withings/sync"; +import { checkRateLimit } from "@/lib/rate-limit"; +import { getEvent } from "@/lib/logging/context"; + +const ORIGINAL_SECRET = process.env.WITHINGS_WEBHOOK_SECRET; + +beforeEach(() => { + vi.resetAllMocks(); + process.env.WITHINGS_WEBHOOK_SECRET = "test-secret"; + vi.mocked(checkRateLimit).mockResolvedValue({ + allowed: true, + remaining: 30, + resetAt: Date.now() + 60_000, + } as never); + vi.mocked(getEvent).mockReturnValue({ + setAuth: vi.fn(), + addWarning: vi.fn(), + } as never); + vi.mocked(syncUserMeasurements).mockResolvedValue(undefined as never); +}); + +afterEach(() => { + if (ORIGINAL_SECRET === undefined) delete process.env.WITHINGS_WEBHOOK_SECRET; + else process.env.WITHINGS_WEBHOOK_SECRET = ORIGINAL_SECRET; +}); + +function jsonRequest(token: string, body: unknown): NextRequest { + return new NextRequest( + `http://localhost/api/withings/webhook/${encodeURIComponent(token)}`, + { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify(body), + }, + ); +} + +function paramsFor(token: string) { + return { params: Promise.resolve({ token }) }; +} + +describe("POST /api/withings/webhook/[token] (path-segment secret)", () => { + it("returns 200 + triggers sync when the path token matches WITHINGS_WEBHOOK_SECRET", async () => { + vi.mocked(prisma.withingsConnection.findFirst).mockResolvedValueOnce({ + userId: "user-42", + withingsUserId: "wu-1", + } as never); + + const res = await POST( + jsonRequest("test-secret", { userid: "wu-1" }), + paramsFor("test-secret"), + ); + expect(res.status).toBe(200); + const body = (await res.json()) as { status: string }; + expect(body.status).toBe("ok"); + + expect(prisma.withingsConnection.findFirst).toHaveBeenCalledWith({ + where: { withingsUserId: "wu-1" }, + }); + expect(syncUserMeasurements).toHaveBeenCalledTimes(1); + expect(syncUserMeasurements).toHaveBeenCalledWith("user-42"); + }); + + it("returns 401 when the path token does NOT match the configured secret", async () => { + const res = await POST( + jsonRequest("WRONG", { userid: "wu-1" }), + paramsFor("WRONG"), + ); + expect(res.status).toBe(401); + expect(syncUserMeasurements).not.toHaveBeenCalled(); + expect(prisma.withingsConnection.findFirst).not.toHaveBeenCalled(); + }); + + it("returns 401 when the path token is the empty string (missing)", async () => { + // Next.js never routes /api/withings/webhook/ to this handler — that + // path lands on the legacy route. The empty-string case here covers + // direct invocations and adversarial proxies that strip the segment. + const res = await POST( + new NextRequest("http://localhost/api/withings/webhook/", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ userid: "wu-1" }), + }), + paramsFor(""), + ); + expect(res.status).toBe(401); + expect(syncUserMeasurements).not.toHaveBeenCalled(); + }); + + it("returns 401 when WITHINGS_WEBHOOK_SECRET is unset, even if the URL carries a token", async () => { + delete process.env.WITHINGS_WEBHOOK_SECRET; + const addWarning = vi.fn(); + vi.mocked(getEvent).mockReturnValue({ + setAuth: vi.fn(), + addWarning, + } as never); + + const res = await POST( + jsonRequest("anything", { userid: "wu-1" }), + paramsFor("anything"), + ); + expect(res.status).toBe(401); + expect(addWarning).toHaveBeenCalledWith( + "WITHINGS_WEBHOOK_SECRET not configured", + ); + }); + + it("constant-time compare rejects a token of the wrong length without short-circuiting on prefix match", async () => { + // "test-secret" is the configured value; "test-secret-extra" shares + // the full prefix. A non-timing-safe compare would still return false + // here, so the assertion is on the response only — the security + // property is exercised by the underlying timingSafeEqual call shape. + const res = await POST( + jsonRequest("test-secret-extra", { userid: "wu-1" }), + paramsFor("test-secret-extra"), + ); + expect(res.status).toBe(401); + }); + + it("returns 429 when rate-limited, before the auth check", async () => { + vi.mocked(checkRateLimit).mockResolvedValueOnce({ + allowed: false, + remaining: 0, + resetAt: Date.now() + 60_000, + } as never); + const res = await POST( + jsonRequest("WRONG", { userid: "wu-1" }), + paramsFor("WRONG"), + ); + expect(res.status).toBe(429); + expect(syncUserMeasurements).not.toHaveBeenCalled(); + }); +}); + +describe("HEAD /api/withings/webhook/[token]", () => { + it("returns 200 for a valid token, 401 otherwise", async () => { + const okReq = new NextRequest( + "http://localhost/api/withings/webhook/test-secret", + { method: "HEAD" }, + ); + const ok = await HEAD(okReq, paramsFor("test-secret")); + expect(ok.status).toBe(200); + + const badReq = new NextRequest( + "http://localhost/api/withings/webhook/WRONG", + { method: "HEAD" }, + ); + const bad = await HEAD(badReq, paramsFor("WRONG")); + expect(bad.status).toBe(401); + }); +}); + +describe("GET /api/withings/webhook/[token]", () => { + it("returns 200 + ok body for a valid token, 401 otherwise", async () => { + const okReq = new NextRequest( + "http://localhost/api/withings/webhook/test-secret", + ); + const ok = await GET(okReq, paramsFor("test-secret")); + expect(ok.status).toBe(200); + + const badReq = new NextRequest( + "http://localhost/api/withings/webhook/WRONG", + ); + const bad = await GET(badReq, paramsFor("WRONG")); + expect(bad.status).toBe(401); + }); +}); diff --git a/src/app/api/withings/webhook/[token]/route.ts b/src/app/api/withings/webhook/[token]/route.ts new file mode 100644 index 000000000..50e1dee42 --- /dev/null +++ b/src/app/api/withings/webhook/[token]/route.ts @@ -0,0 +1,83 @@ +import { NextRequest, NextResponse } from "next/server"; +import { apiHandler } from "@/lib/api-handler"; +import { annotate, getEvent } from "@/lib/logging/context"; +import { + applyWebhookRateLimit, + processWithingsNotification, + timingSafeStringEqual, +} from "@/lib/withings/webhook-handler"; + +/** + * Withings webhook entrypoint (v1.4.25 W17a — path-segment secret). + * + * The shared secret travels in the URL path (`/api/withings/webhook/`) + * rather than the query string. Withings has no facility for adding HTTP + * headers or signing webhook bodies, so the only authenticity surface a + * subscriber controls is the callback URL itself; moving the secret out of + * the query string keeps it out of the `query_string` column most reverse + * proxies log by default (Caddy, nginx, Coolify) and out of GlitchTip's + * URL/breadcrumb capture path which already strips `?…` aggressively. + * + * The legacy `?secret=` form at `/api/withings/webhook` stays alive for one + * release cycle so existing Withings subscriptions keep delivering while + * users re-subscribe. + */ +interface RouteContext { + params: Promise<{ token: string }>; +} + +async function verifyTokenSegment( + token: string | undefined, +): Promise { + const expected = process.env.WITHINGS_WEBHOOK_SECRET; + if (!expected) { + getEvent()?.addWarning("WITHINGS_WEBHOOK_SECRET not configured"); + return false; + } + if (!token) return false; + return timingSafeStringEqual(expected, token); +} + +export const POST = apiHandler( + async (request: NextRequest, context: RouteContext) => { + annotate({ action: { name: "withings.webhook" } }); + + 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 }); + } + + return processWithingsNotification(request); + }, +); + +/** + * Withings sends HEAD/GET to verify the URL when a subscription is + * created. + */ +export const HEAD = apiHandler( + async (_request: NextRequest, context: RouteContext) => { + annotate({ action: { name: "withings.webhook.verify" } }); + const { token } = await context.params; + if (!(await verifyTokenSegment(token))) { + return new NextResponse(null, { status: 401 }); + } + getEvent()?.setAuth({ auth_method: "webhook_secret" }); + return new NextResponse(null, { status: 200 }); + }, +); + +export const GET = apiHandler( + async (_request: NextRequest, context: RouteContext) => { + annotate({ action: { name: "withings.webhook.verify" } }); + const { token } = await context.params; + if (!(await verifyTokenSegment(token))) { + return NextResponse.json({ status: "unauthorized" }, { status: 401 }); + } + getEvent()?.setAuth({ auth_method: "webhook_secret" }); + return NextResponse.json({ status: "ok" }, { status: 200 }); + }, +); diff --git a/src/app/api/withings/webhook/route.ts b/src/app/api/withings/webhook/route.ts index 9f2960c6e..39f4f807c 100644 --- a/src/app/api/withings/webhook/route.ts +++ b/src/app/api/withings/webhook/route.ts @@ -1,110 +1,83 @@ -import { timingSafeEqual } from "node:crypto"; -import { prisma } from "@/lib/db"; -import { syncUserMeasurements } from "@/lib/withings/sync"; -import { checkRateLimit, rateLimitHeaders } from "@/lib/rate-limit"; -import { getClientIp, safeJson } from "@/lib/api-response"; import { NextRequest, NextResponse } from "next/server"; import { apiHandler } from "@/lib/api-handler"; import { annotate, getEvent } from "@/lib/logging/context"; +import { + applyWebhookRateLimit, + processWithingsNotification, + timingSafeStringEqual, +} from "@/lib/withings/webhook-handler"; /** - * Audit C-3 / phase P2: Withings legacy callback URLs include the shared - * secret as a `?secret=…` query param, which leaks into reverse-proxy - * access logs and any error-tracking pipeline that captures `request.url` - * (see `reportToGlitchtip`). New deployments should configure Withings to - * send the secret via the `X-Withings-Webhook-Secret` header instead. + * Legacy Withings webhook entrypoint. * - * The query-param fallback stays for backwards compatibility with existing - * Withings configurations; remove once all integrators have migrated. + * Pre-v1.4.25 callback URLs accepted the shared secret in two places: + * + * 1. `?secret=…` — the original form, which Withings preserves on + * every notification and which therefore lands in + * reverse-proxy access logs (the `query_string` + * column) and any error-tracking pipeline that + * captures `request.url`. + * 2. `X-Withings-Webhook-Secret` header — Withings does NOT support + * setting custom headers on notifications, so this + * path only fires for manual replay tests. + * + * v1.4.25 W17a moved the production path to + * `/api/withings/webhook/[token]`, where the secret travels as a path + * segment instead of a query parameter. This route stays alive for + * one release cycle so existing Withings subscriptions keep delivering + * while users re-subscribe (which also happens organically when they + * reconnect for the new `user.activity` OAuth scope shipped in W5d). + * + * Removal target: v1.4.27 (after all live subscriptions have rotated). */ -function hasValidWebhookSecret(request: NextRequest): boolean { +function hasValidWebhookSecret( + request: NextRequest, +): Promise<{ ok: boolean; via: "header" | "query" | "none" }> { const expected = process.env.WITHINGS_WEBHOOK_SECRET; if (!expected) { getEvent()?.addWarning("WITHINGS_WEBHOOK_SECRET not configured"); - return false; + return Promise.resolve({ ok: false, via: "none" }); } const fromHeader = request.headers.get("x-withings-webhook-secret"); const fromQuery = request.nextUrl.searchParams.get("secret"); const received = fromHeader ?? fromQuery; - if (!received) return false; + if (!received) return Promise.resolve({ ok: false, via: "none" }); - if (!fromHeader && fromQuery) { + const via: "header" | "query" = fromHeader ? "header" : "query"; + return timingSafeStringEqual(expected, received).then((ok) => ({ ok, via })); +} + +async function checkAndWarn( + request: NextRequest, +): Promise { + const auth = await hasValidWebhookSecret(request); + if (!auth.ok) { + return NextResponse.json({ status: "unauthorized" }, { status: 401 }); + } + if (auth.via === "query") { getEvent()?.addWarning( - "withings webhook secret received via legacy URL query — migrate to X-Withings-Webhook-Secret header", + "withings webhook secret received via legacy URL query — migrate to /api/withings/webhook/[token]", ); } - - const a = Buffer.from(expected, "utf8"); - const b = Buffer.from(received, "utf8"); - if (a.length !== b.length) return false; - return timingSafeEqual(a, b); + return null; } /** - * Withings webhook notification endpoint. + * Withings webhook notification endpoint (legacy form). * Withings sends a POST when new measurements are available. - * The webhook sends: userid, startdate, enddate, appli + * The webhook sends: userid, startdate, enddate, appli. */ export const POST = apiHandler(async (request: NextRequest) => { - annotate({ action: { name: "withings.webhook" } }); + annotate({ action: { name: "withings.webhook.legacy" } }); - const ip = getClientIp(request); - const rl = await checkRateLimit(`withings-webhook:${ip}`, 30, 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 (!hasValidWebhookSecret(request)) { - return NextResponse.json({ status: "unauthorized" }, { status: 401 }); - } + const unauth = await checkAndWarn(request); + if (unauth) return unauth; - getEvent()?.setAuth({ auth_method: "webhook_secret" }); - - // Withings sends form-encoded or JSON depending on version - const contentType = request.headers.get("content-type") ?? ""; - let withingsUserId: string | null = null; - - if (contentType.includes("application/x-www-form-urlencoded")) { - const formData = await request.formData(); - withingsUserId = formData.get("userid") as string; - } else { - const { data: body, error: jsonError } = await safeJson<{ - userid?: string | number; - }>(request); - if (jsonError) return jsonError; - withingsUserId = body.userid?.toString() ?? null; - } - - if (!withingsUserId) { - return NextResponse.json({ status: "ignored" }, { status: 200 }); - } - - annotate({ meta: { withings_user_id: withingsUserId } }); - - // Find user by Withings user ID - const connection = await prisma.withingsConnection.findFirst({ - where: { withingsUserId }, - }); - - if (!connection) { - getEvent()?.addWarning( - "Webhook for unknown withings user: " + withingsUserId, - ); - return NextResponse.json({ status: "unknown_user" }, { status: 200 }); - } - - // Sync measurements (non-blocking response for Withings) - syncUserMeasurements(connection.userId).catch((err) => { - getEvent()?.addWarning( - "Sync failed for user " + connection.userId + ": " + err, - ); - }); - - return NextResponse.json({ status: "ok" }, { status: 200 }); + return processWithingsNotification(request); }); /** @@ -112,8 +85,8 @@ export const POST = apiHandler(async (request: NextRequest) => { */ export const HEAD = apiHandler(async (request: NextRequest) => { annotate({ action: { name: "withings.webhook.verify" } }); - - if (!hasValidWebhookSecret(request)) { + const auth = await hasValidWebhookSecret(request); + if (!auth.ok) { return new NextResponse(null, { status: 401 }); } getEvent()?.setAuth({ auth_method: "webhook_secret" }); @@ -122,8 +95,8 @@ export const HEAD = apiHandler(async (request: NextRequest) => { export const GET = apiHandler(async (request: NextRequest) => { annotate({ action: { name: "withings.webhook.verify" } }); - - if (!hasValidWebhookSecret(request)) { + const auth = await hasValidWebhookSecret(request); + if (!auth.ok) { return NextResponse.json({ status: "unauthorized" }, { status: 401 }); } getEvent()?.setAuth({ auth_method: "webhook_secret" }); diff --git a/src/lib/withings/__tests__/sync.test.ts b/src/lib/withings/__tests__/sync.test.ts new file mode 100644 index 000000000..b671761e5 --- /dev/null +++ b/src/lib/withings/__tests__/sync.test.ts @@ -0,0 +1,58 @@ +import { afterEach, describe, expect, it } from "vitest"; +import { getWithingsWebhookCallbackUrl } from "../sync"; + +/** + * v1.4.25 W17a — Withings webhook callback URL must put the shared + * secret in the PATH (not the `?secret=` query parameter) so that the + * value never lands in a reverse-proxy `query_string` access-log + * column. Withings preserves the full URL on every notification, so + * the format chosen at subscribe time is what every inbound delivery + * carries thereafter. + */ +describe("getWithingsWebhookCallbackUrl", () => { + const ORIGINAL_BASE = process.env.NEXT_PUBLIC_APP_URL; + const ORIGINAL_SECRET = process.env.WITHINGS_WEBHOOK_SECRET; + + afterEach(() => { + if (ORIGINAL_BASE === undefined) delete process.env.NEXT_PUBLIC_APP_URL; + else process.env.NEXT_PUBLIC_APP_URL = ORIGINAL_BASE; + if (ORIGINAL_SECRET === undefined) + delete process.env.WITHINGS_WEBHOOK_SECRET; + else process.env.WITHINGS_WEBHOOK_SECRET = ORIGINAL_SECRET; + }); + + it("encodes the secret as the final path segment, not as ?secret=", () => { + process.env.NEXT_PUBLIC_APP_URL = "https://healthlog.example.com"; + process.env.WITHINGS_WEBHOOK_SECRET = "super-secret-value"; + + const url = getWithingsWebhookCallbackUrl(); + + expect(url).toBe( + "https://healthlog.example.com/api/withings/webhook/super-secret-value", + ); + expect(url).not.toContain("?secret="); + expect(url).not.toContain("?"); + }); + + it("URL-encodes path-unsafe characters in the secret", () => { + process.env.NEXT_PUBLIC_APP_URL = "https://healthlog.example.com"; + process.env.WITHINGS_WEBHOOK_SECRET = "a/b c?d"; + + const url = getWithingsWebhookCallbackUrl(); + + // `/`, ` `, and `?` would all break the path semantics if injected + // raw; encodeURIComponent yields `a%2Fb%20c%3Fd`. + expect(url).toBe( + "https://healthlog.example.com/api/withings/webhook/a%2Fb%20c%3Fd", + ); + }); + + it("falls back to the bare legacy URL when WITHINGS_WEBHOOK_SECRET is unset", () => { + process.env.NEXT_PUBLIC_APP_URL = "https://healthlog.example.com"; + delete process.env.WITHINGS_WEBHOOK_SECRET; + + const url = getWithingsWebhookCallbackUrl(); + + expect(url).toBe("https://healthlog.example.com/api/withings/webhook"); + }); +}); diff --git a/src/lib/withings/sync.ts b/src/lib/withings/sync.ts index 4882dee17..c3ebf4237 100644 --- a/src/lib/withings/sync.ts +++ b/src/lib/withings/sync.ts @@ -19,14 +19,32 @@ import { recordSyncSuccess, } from "@/lib/integrations/status"; +/** + * Build the callback URL handed to Withings at `Notify.subscribe` time. + * + * v1.4.25 W17a moved the shared secret from `?secret=…` (query + * parameter, captured by every reverse-proxy access log) to a path + * segment (`/api/withings/webhook/`). Withings has no + * mechanism for setting custom HTTP headers on outgoing notifications + * and never signs the body, so the callback URL is the only + * authenticity surface a subscriber controls — the path-segment form + * is the largest practical shift away from the loggable query string. + * + * When `WITHINGS_WEBHOOK_SECRET` is unset (dev / new install before + * provisioning) we fall back to the bare legacy URL so subscribe + * doesn't 500 — the route handler will then reject every inbound + * delivery with 401 anyway. + */ export function getWithingsWebhookCallbackUrl(): string { const baseUrl = `${process.env.NEXT_PUBLIC_APP_URL}/api/withings/webhook`; const secret = process.env.WITHINGS_WEBHOOK_SECRET; if (!secret) return baseUrl; - - const url = new URL(baseUrl); - url.searchParams.set("secret", secret); - return url.toString(); + // Path-segment form. Withings preserves the full callback URL on + // every notification, so once a subscription is created with this + // URL every delivery carries the secret in the path rather than the + // query string. Encode for safety even though we expect a strong + // random secret. + return `${baseUrl}/${encodeURIComponent(secret)}`; } /** diff --git a/src/lib/withings/webhook-handler.ts b/src/lib/withings/webhook-handler.ts new file mode 100644 index 000000000..803badb10 --- /dev/null +++ b/src/lib/withings/webhook-handler.ts @@ -0,0 +1,123 @@ +/** + * Shared body processing for the Withings webhook endpoint family. + * + * Two routes share this logic: + * + * - `POST /api/withings/webhook` (legacy: secret via header or + * `?secret=` query, kept alive + * during the migration window) + * - `POST /api/withings/webhook/[token]` (v1.4.25 W17a: secret as a + * path segment so it never + * reaches a reverse-proxy + * `query_string` access-log + * column nor the GlitchTip + * URL/breadcrumb surface) + * + * Withings has no public mechanism for adding HTTP headers to outgoing + * notifications and never signs the body — every `notify_subscribe` + * call carries exactly six parameters (action, callbackurl, appli, + * client_id, nonce, signature). The strongest authenticity surface a + * subscriber controls is therefore the callback URL itself. Moving the + * shared secret from `?secret=` (logged) to a path segment (also in the + * URL, but never logged as a query parameter and uniformly redactable + * by a single proxy rule) is the largest shift Withings supports + * end-to-end. + */ +import { NextRequest, NextResponse } from "next/server"; +import { prisma } from "@/lib/db"; +import { syncUserMeasurements } from "@/lib/withings/sync"; +import { checkRateLimit, rateLimitHeaders } from "@/lib/rate-limit"; +import { getClientIp, safeJson } from "@/lib/api-response"; +import { annotate, getEvent } from "@/lib/logging/context"; + +export type WithingsWebhookAuthOutcome = + | { ok: true } + | { ok: false; reason: "missing_secret" | "mismatch" | "not_configured" }; + +/** + * Constant-time comparison helper. Returns false unless both inputs have + * the same byte length AND match exactly. + */ +export async function timingSafeStringEqual( + expected: string, + received: string, +): Promise { + const { timingSafeEqual } = await import("node:crypto"); + const a = Buffer.from(expected, "utf8"); + const b = Buffer.from(received, "utf8"); + if (a.length !== b.length) return false; + return timingSafeEqual(a, b); +} + +/** + * Apply the rate-limit envelope every Withings webhook entrypoint + * shares. Returns a `NextResponse` when the request must be rejected, + * `null` when it should continue. + */ +export async function applyWebhookRateLimit( + request: NextRequest, +): Promise { + const ip = getClientIp(request); + const rl = await checkRateLimit(`withings-webhook:${ip}`, 30, 60 * 1000); + if (!rl.allowed) { + return NextResponse.json( + { status: "rate_limited" }, + { status: 429, headers: rateLimitHeaders(rl) }, + ); + } + return null; +} + +/** + * Once the request is authorised, decode the body, look the user up, + * and trigger a (non-blocking) sync. Returns the response Withings + * should see — `200 ok` on success, `200 ignored` when the body has no + * `userid`, `200 unknown_user` when the `userid` does not map to any + * `WithingsConnection`. Withings retries on non-2xx, so we deliberately + * return 200 even for "unrecognised user" so they don't queue retries + * for a deleted account forever. + */ +export async function processWithingsNotification( + request: NextRequest, +): Promise { + getEvent()?.setAuth({ auth_method: "webhook_secret" }); + + const contentType = request.headers.get("content-type") ?? ""; + let withingsUserId: string | null = null; + + if (contentType.includes("application/x-www-form-urlencoded")) { + const formData = await request.formData(); + withingsUserId = formData.get("userid") as string; + } else { + const { data: body, error: jsonError } = await safeJson<{ + userid?: string | number; + }>(request); + if (jsonError) return jsonError; + withingsUserId = body.userid?.toString() ?? null; + } + + if (!withingsUserId) { + return NextResponse.json({ status: "ignored" }, { status: 200 }); + } + + annotate({ meta: { withings_user_id: withingsUserId } }); + + const connection = await prisma.withingsConnection.findFirst({ + where: { withingsUserId }, + }); + + if (!connection) { + getEvent()?.addWarning( + "Webhook for unknown withings user: " + withingsUserId, + ); + return NextResponse.json({ status: "unknown_user" }, { status: 200 }); + } + + syncUserMeasurements(connection.userId).catch((err) => { + getEvent()?.addWarning( + "Sync failed for user " + connection.userId + ": " + err, + ); + }); + + return NextResponse.json({ status: "ok" }, { status: 200 }); +}