From 6885703505328cc0bba179bc828e3b280df98bd9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sun, 17 May 2026 00:33:34 +0200 Subject: [PATCH] security: HSTS preload + Withings CSP gating + SW URL trust + manifest hygiene MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit HSTS gains `; preload` so the domain becomes eligible for the Chromium preload list. Withings CSP host moves from global to a path-gated entry (mirrors the existing AI-host gating) — closes a DOM-XSS exfil shape on every non-Withings page. `getClientIp` warns once per process when `TRUST_PROXY_HOPS` and the XFF chain length disagree, and exposes a tagged `getClientIpOrTrustWarning` helper so future callers can route to a tighter universal rate-limit bucket when the chain looks tampered with. Service worker now resolves push-notification payload URLs against `self.location.origin` and rejects off-origin redirects (falls back to `/`). The German-only offline fallback HTML body was replaced with a language-neutral icon + 'Offline' wordmark so EN / FR / ES / IT / PL users don't see German on a cold offline boot. Manifest gained `"scope": "/"`, `"id": "/?source=pwa"`, and `"display_override": ["standalone"]` so future sub-path deploys don't accidentally inherit root scope and install-prompt heuristics anchor on a stable id. +16 new test cases across two new files covering HSTS preload, CSP gating on/off, the once-per-process warning, the tagged helper's 4 shape branches, SW same-origin guard, offline-locale-neutrality, and manifest hygiene. Existing proxy/SW suites stay green. --- public/manifest.json | 3 + public/sw.js | 31 +++++- .../proxy-csp-hsts-hardening.test.ts | 103 ++++++++++++++++++ src/__tests__/sw-security-hardening.test.ts | 97 +++++++++++++++++ src/lib/__tests__/get-client-ip.test.ts | 98 ++++++++++++++++- src/lib/api-response.ts | 75 +++++++++++++ src/proxy.ts | 27 +++-- 7 files changed, 420 insertions(+), 14 deletions(-) create mode 100644 src/__tests__/proxy-csp-hsts-hardening.test.ts create mode 100644 src/__tests__/sw-security-hardening.test.ts diff --git a/public/manifest.json b/public/manifest.json index 47232e84e..8c3882279 100644 --- a/public/manifest.json +++ b/public/manifest.json @@ -2,8 +2,11 @@ "name": "HealthLog", "short_name": "HealthLog", "description": "Personal health tracking — weight, blood pressure, medications", + "id": "/?source=pwa", + "scope": "/", "start_url": "/", "display": "standalone", + "display_override": ["standalone"], "theme_color": "#282a36", "background_color": "#282a36", "icons": [ diff --git a/public/sw.js b/public/sw.js index 5d2dcbbb8..a2d42e2cb 100644 --- a/public/sw.js +++ b/public/sw.js @@ -100,9 +100,15 @@ async function networkFirst(request, cacheName) { const cached = await caches.match(request); if (cached) return cached; - // Offline fallback + // Offline fallback. F-7 (mobile security audit, 2026-05-16): the + // previous fallback rendered hard-coded German body copy, which is + // wrong for any en/* user reaching it offline. The replacement is + // language-neutral — wordmark + the universally understood + // "Offline" token + a generic retry hint expressed as an icon + // (the round-trip arrow), so the page does the right thing in any + // locale without shipping a translation bundle into the worker. return new Response( - 'HealthLog — Offline

Offline

Keine Internetverbindung. Bitte versuche es später erneut.

', + 'HealthLog — Offline

HealthLog

Offline

', { status: 503, headers: { "Content-Type": "text/html; charset=utf-8" }, @@ -155,7 +161,22 @@ self.addEventListener("push", (event) => { self.addEventListener("notificationclick", (event) => { event.notification.close(); - const url = event.notification.data?.url || "/"; + // F-7 (mobile security audit, 2026-05-16): validate the destination + // resolves to the same origin before navigating any client. Push + // payloads are VAPID-signed by our own server, but a server-side bug + // (or compromised admin issuing pushes) could ship an off-origin URL + // into `data.url` and use the focused PWA as a redirect-driven phish. + // Reject anything that doesn't match our origin and fall back to "/". + const rawUrl = event.notification.data?.url || "/"; + let safeUrl = "/"; + try { + const resolved = new URL(rawUrl, self.location.origin); + if (resolved.origin === self.location.origin) { + safeUrl = resolved.pathname + resolved.search + resolved.hash; + } + } catch { + safeUrl = "/"; + } event.waitUntil( self.clients @@ -163,11 +184,11 @@ self.addEventListener("notificationclick", (event) => { .then((clientList) => { for (const client of clientList) { if (client.url.includes(self.location.origin) && "focus" in client) { - client.navigate(url); + client.navigate(safeUrl); return client.focus(); } } - return self.clients.openWindow(url); + return self.clients.openWindow(safeUrl); }), ); }); diff --git a/src/__tests__/proxy-csp-hsts-hardening.test.ts b/src/__tests__/proxy-csp-hsts-hardening.test.ts new file mode 100644 index 000000000..7c6c12df9 --- /dev/null +++ b/src/__tests__/proxy-csp-hsts-hardening.test.ts @@ -0,0 +1,103 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { NextRequest } from "next/server"; + +/** + * F-5 (mobile security audit, 2026-05-16): two security headers + * needed to harden against first-visit MITM and to scope the + * Withings backend host to its surface only. + * + * - HSTS now carries `; preload` so `healthlog.bombeck.io` is + * eligible for the Chromium HSTS preload list. + * - The `wbsapi.withings.net` `connect-src` entry only ships on + * the Withings surfaces (`/settings/integrations/withings/*` + * and `/api/withings/*`) instead of globally — same gating + * pattern the AI hosts already use. + * + * Both checks lock the proxy in place so a future refactor cannot + * silently regress either one. + */ + +vi.mock("@/lib/process-type", () => ({ + shouldRunWeb: () => true, +})); + +import { proxy } from "../proxy"; + +// `NODE_ENV` is a special read-only property on node's `process.env` +// proxy. `vi.stubEnv` knows how to patch it without tripping the +// "only accepts a configurable, writable, and enumerable data +// descriptor" guard that a direct `defineProperty` runs into. +function setNodeEnv(value: "development" | "production") { + vi.stubEnv("NODE_ENV", value); +} + +beforeEach(() => { + // Default to production so the production-only headers run; individual + // tests can flip back to development if they care about the dev shape. + setNodeEnv("production"); +}); + +afterEach(() => { + vi.unstubAllEnvs(); +}); + +function makeRequest( + pathname: string, + cookies: Record = {}, +): NextRequest { + const cookieHeader = Object.entries(cookies) + .map(([k, v]) => `${k}=${v}`) + .join("; "); + return new NextRequest(`http://localhost${pathname}`, { + headers: cookieHeader ? { cookie: cookieHeader } : {}, + }); +} + +describe("proxy.ts HSTS preload (F-5, 2026-05-16)", () => { + it("attaches `preload` to the Strict-Transport-Security header in production", () => { + const res = proxy(makeRequest("/", { healthlog_session: "sess-1" })); + expect(res.headers.get("strict-transport-security")).toBe( + "max-age=31536000; includeSubDomains; preload", + ); + }); + + it("does not ship HSTS in development", () => { + setNodeEnv("development"); + const res = proxy(makeRequest("/", { healthlog_session: "sess-1" })); + expect(res.headers.get("strict-transport-security")).toBeNull(); + }); +}); + +describe("proxy.ts Withings CSP gating (F-5, 2026-05-16)", () => { + it("does NOT allow wbsapi.withings.net on a non-Withings page", () => { + const res = proxy(makeRequest("/", { healthlog_session: "sess-1" })); + const csp = res.headers.get("content-security-policy") ?? ""; + expect(csp).not.toMatch(/wbsapi\.withings\.net/); + }); + + it("allows wbsapi.withings.net under /settings/integrations/withings/*", () => { + const res = proxy( + makeRequest("/settings/integrations/withings", { + healthlog_session: "sess-1", + }), + ); + const csp = res.headers.get("content-security-policy") ?? ""; + expect(csp).toMatch(/connect-src[^;]*https:\/\/wbsapi\.withings\.net/); + }); + + it("allows wbsapi.withings.net under /api/withings/*", () => { + const res = proxy( + makeRequest("/api/withings/callback", { + healthlog_session: "sess-1", + }), + ); + const csp = res.headers.get("content-security-policy") ?? ""; + expect(csp).toMatch(/connect-src[^;]*https:\/\/wbsapi\.withings\.net/); + }); + + it("never ships the Withings host on the auth surface (DOM-XSS exfil channel)", () => { + const res = proxy(makeRequest("/auth/login")); + const csp = res.headers.get("content-security-policy") ?? ""; + expect(csp).not.toMatch(/wbsapi\.withings\.net/); + }); +}); diff --git a/src/__tests__/sw-security-hardening.test.ts b/src/__tests__/sw-security-hardening.test.ts new file mode 100644 index 000000000..c082bb60e --- /dev/null +++ b/src/__tests__/sw-security-hardening.test.ts @@ -0,0 +1,97 @@ +import { describe, expect, it } from "vitest"; +import { readFileSync } from "node:fs"; +import { resolve } from "node:path"; + +/** + * F-7 (mobile security audit, 2026-05-16): three regressions to lock + * in for `public/sw.js`. + * + * - `notificationclick` validates the destination resolves to the + * same origin before calling `client.navigate()` — VAPID auth on + * push payloads only proves authenticity, not that the URL is + * safe to navigate to. A compromised admin issuing pushes could + * otherwise drive a focused PWA at an off-origin URL. + * - The offline fallback HTML is no longer hard-coded German body + * copy. The replacement renders a language-neutral icon + + * "Offline" wordmark. + * - The manifest carries `scope`, `id`, and `display_override` so + * PWA install identity and scope are explicit instead of + * defaulting on the user-agent's behalf. + * + * The service worker depends on `self`, `caches`, `clients`, and other + * worker-globals that aren't available in node, so this suite reads + * `public/sw.js` as text and asserts on the source rather than + * executing the worker. Cheap and stable across worker-runtime drift. + */ + +const SW_PATH = resolve(process.cwd(), "public/sw.js"); +const MANIFEST_PATH = resolve(process.cwd(), "public/manifest.json"); + +function readSw(): string { + return readFileSync(SW_PATH, "utf-8"); +} + +describe("public/sw.js notificationclick same-origin guard (F-7, 2026-05-16)", () => { + it("validates the navigation URL resolves to the worker's own origin", () => { + const sw = readSw(); + // The guard reads `data.url`, parses it with `new URL(rawUrl, + // self.location.origin)`, and only navigates when + // `resolved.origin === self.location.origin`. Each substring is + // independently load-bearing. + expect(sw).toMatch(/new URL\(rawUrl, self\.location\.origin\)/); + expect(sw).toMatch(/resolved\.origin === self\.location\.origin/); + }); + + it("falls back to `/` for off-origin or malformed payloads", () => { + const sw = readSw(); + // Two fallback paths — the URL parse failure (try/catch) and the + // origin mismatch branch. Both must end at `safeUrl = "/"`. + const safeUrlAssignments = sw.match(/safeUrl\s*=\s*"\/"/g) ?? []; + expect(safeUrlAssignments.length).toBeGreaterThanOrEqual(2); + }); + + it("never calls client.navigate with the raw payload URL", () => { + const sw = readSw(); + // The previous code called `client.navigate(url)` directly. The + // hardened version only ever navigates the sanitised `safeUrl`. + expect(sw).not.toMatch(/client\.navigate\(url\)/); + expect(sw).toMatch(/client\.navigate\(safeUrl\)/); + expect(sw).toMatch(/self\.clients\.openWindow\(safeUrl\)/); + }); +}); + +describe("public/sw.js offline fallback locale neutrality (F-7, 2026-05-16)", () => { + it("does not embed hard-coded German body copy in the offline page", () => { + const sw = readSw(); + // Previous fallback shipped a German sentence. The replacement is + // language-neutral — wordmark + "Offline" token + an icon. None + // of the German body copy may reappear. + expect(sw).not.toMatch(/Keine Internetverbindung/); + expect(sw).not.toMatch(/Bitte versuche es später erneut/); + }); + + it("still renders the wordmark and the universal `Offline` token", () => { + const sw = readSw(); + // The minimal language-neutral fallback still names the app and + // surfaces the universally understood status token. + expect(sw).toMatch(/HealthLog/); + expect(sw).toMatch(/

Offline<\/p>/); + }); +}); + +describe("public/manifest.json PWA identity hygiene (F-7, 2026-05-16)", () => { + it("declares an explicit scope, id, and display_override", () => { + const raw = readFileSync(MANIFEST_PATH, "utf-8"); + const manifest = JSON.parse(raw) as Record; + expect(manifest.scope).toBe("/"); + expect(manifest.id).toBe("/?source=pwa"); + expect(manifest.display_override).toEqual(["standalone"]); + }); + + it("keeps the existing display/start_url contract", () => { + const raw = readFileSync(MANIFEST_PATH, "utf-8"); + const manifest = JSON.parse(raw) as Record; + expect(manifest.display).toBe("standalone"); + expect(manifest.start_url).toBe("/"); + }); +}); diff --git a/src/lib/__tests__/get-client-ip.test.ts b/src/lib/__tests__/get-client-ip.test.ts index d68479461..712e6df42 100644 --- a/src/lib/__tests__/get-client-ip.test.ts +++ b/src/lib/__tests__/get-client-ip.test.ts @@ -1,10 +1,15 @@ -import { describe, it, expect, beforeEach, afterEach } from "vitest"; -import { getClientIp } from "../api-response"; +import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; +import { + getClientIp, + getClientIpOrTrustWarning, + _resetTrustViolationWarningForTests, +} from "../api-response"; const ORIGINAL_ENV = process.env.TRUST_PROXY_HOPS; beforeEach(() => { delete process.env.TRUST_PROXY_HOPS; + _resetTrustViolationWarningForTests(); }); afterEach(() => { @@ -97,3 +102,92 @@ describe("getClientIp trusted-proxy semantics (V3 audit)", () => { expect(ip).toBe("5.6.7.8"); }); }); + +/** + * F-6 (mobile security audit, 2026-05-16): when `TRUST_PROXY_HOPS` and + * the actual proxy chain don't agree, getClientIp returns null and every + * caller falls back to a literal `"unknown"` rate-limit bucket. The + * operator needs a single warning per process so the dashboards reflect + * the misconfiguration, and a new `getClientIpOrTrustWarning` helper + * lets future callers route the request to a tighter universal bucket. + */ +describe("getClientIp trust-violation warning (F-6, 2026-05-16)", () => { + it("emits a console.warn once when the chain is shorter than configured hops", () => { + process.env.TRUST_PROXY_HOPS = "2"; + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + try { + getClientIp(makeRequest({ "x-forwarded-for": "5.6.7.8" })); + getClientIp(makeRequest({ "x-forwarded-for": "5.6.7.8" })); + getClientIp(makeRequest({ "x-forwarded-for": "5.6.7.8" })); + expect(warn).toHaveBeenCalledTimes(1); + expect(warn.mock.calls[0][0]).toMatch(/TRUST_PROXY_HOPS=2/); + expect(warn.mock.calls[0][0]).toMatch(/1 entry/); + } finally { + warn.mockRestore(); + } + }); + + it("does not warn when the chain length matches", () => { + process.env.TRUST_PROXY_HOPS = "1"; + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + try { + getClientIp(makeRequest({ "x-forwarded-for": "5.6.7.8" })); + expect(warn).not.toHaveBeenCalled(); + } finally { + warn.mockRestore(); + } + }); +}); + +describe("getClientIpOrTrustWarning shape (F-6, 2026-05-16)", () => { + it("returns trustViolation=false on a well-formed chain", () => { + const result = getClientIpOrTrustWarning( + makeRequest({ "x-forwarded-for": "1.2.3.4, 5.6.7.8" }), + ); + expect(result).toEqual({ ip: "5.6.7.8", trustViolation: false }); + }); + + it("returns trustViolation=true when the chain is shorter than hops", () => { + process.env.TRUST_PROXY_HOPS = "2"; + const result = getClientIpOrTrustWarning( + makeRequest({ "x-forwarded-for": "5.6.7.8" }), + ); + expect(result.trustViolation).toBe(true); + // ip falls back to x-real-ip (absent here) so it's null + expect(result.ip).toBeNull(); + }); + + it("returns trustViolation=true and x-real-ip when chain is short but x-real-ip is set", () => { + process.env.TRUST_PROXY_HOPS = "2"; + const result = getClientIpOrTrustWarning( + makeRequest({ + "x-forwarded-for": "5.6.7.8", + "x-real-ip": "9.9.9.9", + }), + ); + expect(result).toEqual({ ip: "9.9.9.9", trustViolation: true }); + }); + + it("returns trustViolation=false when XFF is absent (no chain to validate)", () => { + const result = getClientIpOrTrustWarning( + makeRequest({ "x-real-ip": "9.9.9.9" }), + ); + expect(result).toEqual({ ip: "9.9.9.9", trustViolation: false }); + }); + + it("emits the same once-per-process warning when triggered via the tagged helper", () => { + process.env.TRUST_PROXY_HOPS = "2"; + const warn = vi.spyOn(console, "warn").mockImplementation(() => {}); + try { + getClientIpOrTrustWarning( + makeRequest({ "x-forwarded-for": "5.6.7.8" }), + ); + getClientIpOrTrustWarning( + makeRequest({ "x-forwarded-for": "5.6.7.8" }), + ); + expect(warn).toHaveBeenCalledTimes(1); + } finally { + warn.mockRestore(); + } + }); +}); diff --git a/src/lib/api-response.ts b/src/lib/api-response.ts index a45a8a5cb..83c09cd0f 100644 --- a/src/lib/api-response.ts +++ b/src/lib/api-response.ts @@ -95,6 +95,34 @@ function parseTrustProxyHops(raw: string | undefined): number { return parseInt(trimmed, 10); } +/** + * Module-scope flag so the trust-violation warning fires at most once per + * process. F-6 (mobile security audit, 2026-05-16): every IP-keyed + * rate-limit caller falls back to a literal `"unknown"` bucket when + * `getClientIp` returns null, which collapses anonymous traffic into a + * single shared bucket. A persistent stderr line tells the operator the + * proxy chain length and the configured `TRUST_PROXY_HOPS` value don't + * match and the deployment is silently degrading rate-limit precision. + */ +let trustViolationWarned = false; + +/** + * Reset the once-per-process warning flag. Test-only; the production + * code never calls this. + */ +export function _resetTrustViolationWarningForTests(): void { + trustViolationWarned = false; +} + +function warnTrustViolationOnce(hops: number, chainLength: number): void { + if (trustViolationWarned) return; + trustViolationWarned = true; + console.warn( + `[getClientIp] TRUST_PROXY_HOPS=${hops} but X-Forwarded-For carried ${chainLength} entr${chainLength === 1 ? "y" : "ies"}; ` + + `refusing to read XFF for this request. Every anonymous caller will now share the same "unknown" rate-limit bucket — fix TRUST_PROXY_HOPS or the proxy chain.`, + ); +} + export function getClientIp(request: Request): string | null { const hops = parseTrustProxyHops(process.env.TRUST_PROXY_HOPS); @@ -113,8 +141,55 @@ export function getClientIp(request: Request): string | null { if (chain.length >= hops) { return chain[chain.length - hops]; } + // F-6 (mobile security audit, 2026-05-16): emit a one-shot + // operator signal when the chain shape doesn't match the + // configured trust. Without this warning the silent degrade was + // invisible until rate-limits visibly misfired in production. + warnTrustViolationOnce(hops, chain.length); } } const realIp = request.headers.get("x-real-ip"); return realIp && looksLikeIp(realIp) ? realIp : null; } + +/** + * Tagged return shape so a caller can apply a tighter universal + * rate-limit when the trust chain is misconfigured. F-6 (mobile security + * audit, 2026-05-16): callers today fall back to a literal `"unknown"` + * string, collapsing every anonymous request into one bucket. New + * callers should branch on `trustViolation === true` and route the + * request to a tighter global rate-limit instead of the per-IP one. + * + * Existing callers using `getClientIp(request) ?? "unknown"` keep + * working unchanged; this helper is additive. + */ +export function getClientIpOrTrustWarning(request: Request): { + ip: string | null; + trustViolation: boolean; +} { + const hops = parseTrustProxyHops(process.env.TRUST_PROXY_HOPS); + + if (hops > 0) { + const forwarded = request.headers.get("x-forwarded-for"); + if (forwarded) { + const chain = forwarded + .split(",") + .map((s) => s.trim()) + .filter(looksLikeIp); + if (chain.length >= hops) { + return { ip: chain[chain.length - hops], trustViolation: false }; + } + warnTrustViolationOnce(hops, chain.length); + const realIp = request.headers.get("x-real-ip"); + return { + ip: realIp && looksLikeIp(realIp) ? realIp : null, + trustViolation: true, + }; + } + } + const realIp = request.headers.get("x-real-ip"); + return { + ip: realIp && looksLikeIp(realIp) ? realIp : null, + trustViolation: false, + }; +} diff --git a/src/proxy.ts b/src/proxy.ts index d85be54d6..ee1d52d12 100644 --- a/src/proxy.ts +++ b/src/proxy.ts @@ -234,22 +234,35 @@ export function proxy(request: NextRequest) { "camera=(), microphone=(), geolocation=()", ); - // CSP — permissive in dev, strict in production. AI provider hosts - // (OpenAI / chatgpt.com) are gated to /settings/ai/** because that is - // the only surface a browser fetch is needed (V3 audit: blanket - // chatgpt.com on /auth/login is a DOM-XSS exfil channel). + // CSP — permissive in dev, strict in production. Third-party hosts in + // `connect-src` are gated to the surfaces that actually need them so a + // DOM-XSS on an unrelated page can't exfiltrate to them (V3 audit: + // blanket chatgpt.com on /auth/login was a DOM-XSS exfil channel). + // + // F-5 (mobile security audit, 2026-05-16): `wbsapi.withings.net` used + // to live in the global `connect-src` and shipped on every page. The + // Withings client lives server-side, so the browser never needs to + // reach it from a non-Withings surface; mirror the AI gating shape. const isDev = process.env.NODE_ENV === "development"; const cspReportEndpoint = "/api/monitoring/csp-report"; const isAiSettingsRoute = pathname.startsWith("/settings/ai"); const aiConnectSrc = isAiSettingsRoute ? " https://api.openai.com https://chatgpt.com" : ""; + const isWithingsRoute = + pathname.startsWith("/settings/integrations/withings") || + pathname.startsWith("/api/withings/"); + const withingsConnectSrc = isWithingsRoute + ? " https://wbsapi.withings.net" + : ""; const csp = isDev ? `default-src 'self'; script-src 'self' 'unsafe-eval' 'unsafe-inline'; style-src 'self' 'unsafe-inline'; img-src 'self' data: https://www.gravatar.com; connect-src 'self'; font-src 'self';` - : `default-src 'self'; script-src 'self' 'nonce-${nonce}'; style-src 'self' 'unsafe-inline'; img-src 'self' data: https://www.gravatar.com; connect-src 'self'${aiConnectSrc} https://wbsapi.withings.net; font-src 'self'; object-src 'none'; base-uri 'self'; form-action 'self'; frame-ancestors 'none'; worker-src 'self'; report-uri ${cspReportEndpoint}; report-to csp-endpoint;`; + : `default-src 'self'; script-src 'self' 'nonce-${nonce}'; style-src 'self' 'unsafe-inline'; img-src 'self' data: https://www.gravatar.com; connect-src 'self'${aiConnectSrc}${withingsConnectSrc}; font-src 'self'; object-src 'none'; base-uri 'self'; form-action 'self'; frame-ancestors 'none'; worker-src 'self'; report-uri ${cspReportEndpoint}; report-to csp-endpoint;`; response.headers.set("Content-Security-Policy", csp); - // Production-only headers + // Production-only headers. HSTS carries `preload` so the domain stays + // eligible for the Chromium preload list — closes the first-visit MITM + // window on hostile networks (F-5, mobile security audit 2026-05-16). if (!isDev) { response.headers.set( "Reporting-Endpoints", @@ -257,7 +270,7 @@ export function proxy(request: NextRequest) { ); response.headers.set( "Strict-Transport-Security", - "max-age=31536000; includeSubDomains", + "max-age=31536000; includeSubDomains; preload", ); }