mirror of
https://github.com/MBombeck/HealthLog.git
synced 2026-11-14 22:50:51 +00:00
fix(security): Withings webhook secret as URL path segment, not query string
Move the Withings webhook shared secret from `?secret=…` to a path segment (`/api/withings/webhook/<secret>`) so it never lands in the reverse-proxy `query_string` access-log column or GlitchTip URL/ breadcrumb capture surface (W10 finding M-2). Withings has no facility for custom HTTP headers on outgoing notifications and never signs the body — every Notify.subscribe call accepts only action/callbackurl/appli/client_id/nonce/signature, so the callback URL is the only authenticity surface a subscriber controls. Path-segment binding is the largest practical shift away from loggable query strings that Withings supports end-to-end. The legacy `/api/withings/webhook` route stays alive for one release cycle (removal target: v1.4.27) so existing subscriptions keep delivering. The query-string path now emits a deprecation breadcrumb on every hit. Active users re-subscribe organically when they reconnect for the v1.4.25 `user.activity` OAuth-scope upgrade. Tests: 8 new for the path-segment route (valid / invalid / missing / unset-env / prefix-match / rate-limited / HEAD / GET), 3 new for `getWithingsWebhookCallbackUrl()`. Existing legacy-route tests continue to pass over the refactored shared helper.
This commit is contained in:
1 parent
b4df3ae8cd
commit
a1ffa497f1
7 files changed
+689
-87
No files matched your search
@@ -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: <T extends (...args: unknown[]) => 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);
|
||||
});
|
||||
});
|
||||
@@ -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/<secret>`)
|
||||
* 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<boolean> {
|
||||
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 });
|
||||
},
|
||||
);
|
||||
@@ -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<NextResponse | null> {
|
||||
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" });
|
||||
|
||||
Reference in new issue
Block a user