From 8ead442d44d8347ec54cae5149d2989a0fc25ff3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sun, 19 Jul 2026 13:31:29 +0200 Subject: [PATCH] fix(proxy,auth,insights): close three drift gaps the audits left open MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The proxy's seven early exits — worker-only 503, two legacy 301s, the demo-mode 403, and the login / onboarding / MFA redirects — returned before the security header block, so they carried no HSTS, no framing refusal and no nosniff. Factor the transport subset into a helper and apply it on every exit. The per-route CSP stays where it is; it is nonce-bound and only meaningful for a response with a body. The WebAuthn relying-party candidate list always carried http://localhost:3000, including in production. The signed rpIdHash still pinned the real domain, so this was never a reachable bypass, but a configured production origin has no reason to carry it. Gate the fallback on development, keeping it when nothing is configured so getRpId() still resolves. ORPHAN_ENUMS was a hand-maintained literal that drifted every time ALLOWED_CHART_TOKENS grew — eighteen allowlisted metrics had no strip entry, so a model writing 'Your PHQ9_SCORE is elevated' leaked the raw enum into rendered prose. Derive it from the allowlist and pin the coupling with a test. --- .../proxy-early-exit-headers.test.ts | 120 ++++++++++++++++++ src/lib/auth/__tests__/webauthn-rp.test.ts | 88 +++++++++++++ src/lib/auth/webauthn-rp.ts | 22 +++- .../insights/__tests__/chart-tokens.test.ts | 40 ++++++ src/lib/insights/chart-tokens.ts | 102 +++++---------- src/proxy.ts | 86 ++++++++++--- 6 files changed, 365 insertions(+), 93 deletions(-) create mode 100644 src/__tests__/proxy-early-exit-headers.test.ts create mode 100644 src/lib/auth/__tests__/webauthn-rp.test.ts diff --git a/src/__tests__/proxy-early-exit-headers.test.ts b/src/__tests__/proxy-early-exit-headers.test.ts new file mode 100644 index 000000000..fbb6f2ebd --- /dev/null +++ b/src/__tests__/proxy-early-exit-headers.test.ts @@ -0,0 +1,120 @@ +/** + * Security headers on the proxy's EARLY EXITS. + * + * The full header block sits at the bottom of `proxy()`, after the pass-through + * `NextResponse.next()`. Seven exits return before it — the worker-only 503, + * two legacy 301s, the demo-mode 403, and the login / onboarding / MFA + * redirects — and used to carry no HSTS, no framing refusal and no nosniff at + * all. + * + * The exposure was one hop (a redirect has no rendered body, and the follow-up + * navigation re-enters the proxy and does get the full set), but HSTS is worth + * the most on exactly that first hop over a hostile network. These assertions + * exist so a future exit added above the header block cannot silently rejoin + * the unprotected set. + */ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { NextRequest } from "next/server"; + +const shouldRunWeb = vi.fn(() => true); + +vi.mock("@/lib/process-type", () => ({ + shouldRunWeb: () => shouldRunWeb(), +})); + +import { proxy } from "../proxy"; + +function makeRequest( + pathname: string, + init: { cookies?: Record; method?: string } = {}, +): NextRequest { + const cookieHeader = Object.entries(init.cookies ?? {}) + .map(([k, v]) => `${k}=${v}`) + .join("; "); + return new NextRequest(`http://localhost${pathname}`, { + method: init.method ?? "GET", + headers: cookieHeader ? { cookie: cookieHeader } : {}, + }); +} + +const ORIGINAL_NODE_ENV = process.env.NODE_ENV; +const ORIGINAL_DEMO = process.env.DEMO_MODE; + +beforeEach(() => { + shouldRunWeb.mockReturnValue(true); + process.env.NODE_ENV = "production"; + delete process.env.DEMO_MODE; +}); + +afterEach(() => { + process.env.NODE_ENV = ORIGINAL_NODE_ENV; + if (ORIGINAL_DEMO === undefined) delete process.env.DEMO_MODE; + else process.env.DEMO_MODE = ORIGINAL_DEMO; +}); + +/** The transport subset every exit must carry. */ +function expectBaselineHeaders(res: Response) { + expect(res.headers.get("X-Content-Type-Options")).toBe("nosniff"); + expect(res.headers.get("X-Frame-Options")).toBe("DENY"); + expect(res.headers.get("Referrer-Policy")).toBe( + "strict-origin-when-cross-origin", + ); + expect(res.headers.get("Permissions-Policy")).toContain("camera=()"); + expect(res.headers.get("Cross-Origin-Opener-Policy")).toBe("same-origin"); + expect(res.headers.get("X-Permitted-Cross-Domain-Policies")).toBe("none"); + expect(res.headers.get("Strict-Transport-Security")).toContain( + "max-age=31536000", + ); +} + +describe("proxy early exits carry the baseline security headers", () => { + it("worker-only 503", () => { + shouldRunWeb.mockReturnValue(false); + const res = proxy(makeRequest("/dashboard")); + expect(res.status).toBe(503); + expectBaselineHeaders(res); + }); + + it("legacy 301 redirect", () => { + const res = proxy(makeRequest("/stimmung")); + expect(res.status).toBe(301); + expectBaselineHeaders(res); + }); + + it("unauthenticated page redirect to /auth/login", () => { + const res = proxy(makeRequest("/dashboard")); + expect(res.headers.get("location")).toMatch(/\/auth\/login$/); + expectBaselineHeaders(res); + }); + + it("onboarding redirect", () => { + const res = proxy( + makeRequest("/", { + cookies: { healthlog_session: "sess-1", hl_onboarding: "pending" }, + }), + ); + expect(res.headers.get("location")).toMatch(/\/onboarding$/); + expectBaselineHeaders(res); + }); + + it("demo-mode 403 on a blocked mutation", () => { + process.env.DEMO_MODE = "true"; + const res = proxy( + makeRequest("/api/measurements", { + method: "POST", + cookies: { healthlog_session: "sess-1" }, + }), + ); + expect(res.status).toBe(403); + expectBaselineHeaders(res); + }); + + it("omits HSTS in development, like the main header block", () => { + process.env.NODE_ENV = "development"; + const res = proxy(makeRequest("/stimmung")); + expect(res.status).toBe(301); + expect(res.headers.get("Strict-Transport-Security")).toBeNull(); + // The non-transport-dependent headers still attach. + expect(res.headers.get("X-Frame-Options")).toBe("DENY"); + }); +}); diff --git a/src/lib/auth/__tests__/webauthn-rp.test.ts b/src/lib/auth/__tests__/webauthn-rp.test.ts new file mode 100644 index 000000000..f2541a7e4 --- /dev/null +++ b/src/lib/auth/__tests__/webauthn-rp.test.ts @@ -0,0 +1,88 @@ +/** + * Relying-party origin resolution. + * + * The localhost fallback used to sit in the candidate list unconditionally, so + * a production deployment with a real `APP_URL` still accepted an assertion + * whose `clientDataJSON.origin` was `http://localhost:3000`. The signed + * `rpIdHash` pinned the real domain either way, so this never was a reachable + * bypass — but a configured production origin has no reason to carry it, and an + * unpinned second layer is exactly what regresses unnoticed. + */ +import { afterEach, beforeEach, describe, expect, it } from "vitest"; + +import { + getConfiguredOrigins, + getExpectedOrigin, + getRpId, +} from "../webauthn-rp"; + +const ORIGINAL = { + NODE_ENV: process.env.NODE_ENV, + APP_URL: process.env.APP_URL, + NEXT_PUBLIC_APP_URL: process.env.NEXT_PUBLIC_APP_URL, +}; + +function setEnv(env: { + nodeEnv?: string; + appUrl?: string; + publicAppUrl?: string; +}) { + if (env.nodeEnv === undefined) delete process.env.NODE_ENV; + else process.env.NODE_ENV = env.nodeEnv; + if (env.appUrl === undefined) delete process.env.APP_URL; + else process.env.APP_URL = env.appUrl; + if (env.publicAppUrl === undefined) delete process.env.NEXT_PUBLIC_APP_URL; + else process.env.NEXT_PUBLIC_APP_URL = env.publicAppUrl; +} + +beforeEach(() => { + setEnv({}); +}); + +afterEach(() => { + setEnv({ + nodeEnv: ORIGINAL.NODE_ENV, + appUrl: ORIGINAL.APP_URL, + publicAppUrl: ORIGINAL.NEXT_PUBLIC_APP_URL, + }); +}); + +describe("getConfiguredOrigins", () => { + it("drops the localhost fallback in production once an app URL is configured", () => { + setEnv({ nodeEnv: "production", appUrl: "https://health.example" }); + const origins = getConfiguredOrigins(); + expect(origins).toEqual(["https://health.example"]); + expect(origins).not.toContain("http://localhost:3000"); + }); + + it("drops it in production when only the public build-time URL is set", () => { + setEnv({ nodeEnv: "production", publicAppUrl: "https://health.example" }); + expect(getConfiguredOrigins()).not.toContain("http://localhost:3000"); + }); + + it("keeps the localhost fallback outside production", () => { + setEnv({ nodeEnv: "development", appUrl: "https://health.example" }); + expect(getConfiguredOrigins()).toContain("http://localhost:3000"); + }); + + it("keeps it in production when nothing is configured, so getRpId still resolves", () => { + setEnv({ nodeEnv: "production" }); + expect(getConfiguredOrigins()).toEqual(["http://localhost:3000"]); + expect(getRpId()).toBe("localhost"); + }); + + it("still pins the RP id to the configured domain in production", () => { + setEnv({ nodeEnv: "production", appUrl: "https://health.example" }); + expect(getRpId()).toBe("health.example"); + expect(getExpectedOrigin()).toBe("https://health.example"); + }); + + it("de-duplicates when both URL vars carry the same origin", () => { + setEnv({ + nodeEnv: "production", + appUrl: "https://health.example", + publicAppUrl: "https://health.example/", + }); + expect(getConfiguredOrigins()).toEqual(["https://health.example"]); + }); +}); diff --git a/src/lib/auth/webauthn-rp.ts b/src/lib/auth/webauthn-rp.ts index ba61f0192..d4864c15a 100644 --- a/src/lib/auth/webauthn-rp.ts +++ b/src/lib/auth/webauthn-rp.ts @@ -10,20 +10,32 @@ * build-time URL, then a localhost fallback for dev. A multi-origin deployment * (rare for a single-tenant self-host, but supported) passes the whole set to * SimpleWebAuthn's `expectedOrigin`. + * + * The localhost fallback is gated: it is offered in development, and in + * production ONLY when neither app URL is configured — otherwise a production + * deployment accepted an assertion whose `clientDataJSON.origin` was + * `http://localhost:3000`. The signed `rpIdHash` still pins the real domain via + * `getRpId()`, so this was defence-in-depth rather than a live bypass, but a + * configured production origin has no reason to carry localhost at all. The + * unconfigured case keeps the fallback so `getRpId()` still resolves instead of + * throwing on an empty candidate list. */ /** Human-readable relying-party name shown by the authenticator UI. */ export const RP_NAME = "HealthLog"; export function getConfiguredOrigins(): string[] { - const candidates = [ - process.env.APP_URL, - process.env.NEXT_PUBLIC_APP_URL, - "http://localhost:3000", - ] + const configured = [process.env.APP_URL, process.env.NEXT_PUBLIC_APP_URL] .map((value) => value?.trim()) .filter((value): value is string => Boolean(value)); + const allowLocalhost = + process.env.NODE_ENV !== "production" || configured.length === 0; + + const candidates = allowLocalhost + ? [...configured, "http://localhost:3000"] + : configured; + const validOrigins = candidates .map((value) => { try { diff --git a/src/lib/insights/__tests__/chart-tokens.test.ts b/src/lib/insights/__tests__/chart-tokens.test.ts index dc82d4301..13cddcb6b 100644 --- a/src/lib/insights/__tests__/chart-tokens.test.ts +++ b/src/lib/insights/__tests__/chart-tokens.test.ts @@ -252,3 +252,43 @@ describe("ALLOWED_CHART_TOKENS — drift guard", () => { expect(ALLOWED_CHART_TOKENS).not.toContain("metric:COMPLIANCE"); }); }); + +describe("orphan-enum stripping stays coupled to the allowlist", () => { + // `ORPHAN_ENUMS` used to be a hand-maintained literal and drifted every time + // `ALLOWED_CHART_TOKENS` grew — by v1.30 eighteen allowlisted metrics had no + // strip entry, so the raw enum leaked into rendered prose. It is now derived; + // this guard is what keeps it derived. + const PROSE_SAFE = new Set(["WEIGHT", "PULSE", "MOOD", "RESILIENCE"]); + + it.each( + ALLOWED_CHART_TOKENS.map((t) => t.replace(/^metric:/, "")).filter( + (name) => !PROSE_SAFE.has(name), + ), + )("strips the bare enum %s out of prose", (name) => { + const stripped = stripChartTokens(`Your ${name} is elevated.`); + expect(stripped).not.toContain(name); + }); + + it.each([...PROSE_SAFE])( + "leaves the prose-safe single word %s alone", + (word) => { + expect(stripChartTokens(`Your ${word} trend is stable.`)).toContain(word); + }, + ); + + // The tokens the drift actually leaked, named so a regression is legible. + it.each([ + "PHQ9_SCORE", + "GAD7_SCORE", + "WHO5_SCORE", + "SCI_SCORE", + "GRIP_STRENGTH", + "CARDIO_LOAD", + "ANS_CHARGE", + "BODY_FAT", + "BLOOD_GLUCOSE", + "OXYGEN_SATURATION", + ])("strips %s, which the stale literal list had missed", (name) => { + expect(stripChartTokens(`Your ${name} is elevated.`)).not.toContain(name); + }); +}); diff --git a/src/lib/insights/chart-tokens.ts b/src/lib/insights/chart-tokens.ts index b73fc1539..02f6da8fc 100644 --- a/src/lib/insights/chart-tokens.ts +++ b/src/lib/insights/chart-tokens.ts @@ -188,80 +188,44 @@ const METRIC_WORD_REGEX = /\bMetric\s+[A-Za-z][A-Za-z0-9_]*\b/g; // fixed alternation so we never accidentally cleave a real word // (e.g. "MOOD") out of legitimate prose unless the model wrote it // as the raw enum in upper-snake form. -const ORPHAN_ENUMS = [ - "BLOOD_PRESSURE_SYS", - "BLOOD_PRESSURE_DIA", +const EXTRA_ORPHAN_ENUMS = [ "PULSE_BPM", "MOOD_SCORE", "MEDICATION_COMPLIANCE_PCT", - // v1.4.23 Apple Health additions — see ALLOWED_CHART_TOKENS above. - "HEART_RATE_VARIABILITY", - "RESTING_HEART_RATE", - "ACTIVE_ENERGY_BURNED", - "FLIGHTS_CLIMBED", - "WALKING_RUNNING_DISTANCE", - "VO2_MAX", - "BODY_TEMPERATURE", - "SLEEP_DURATION", - // v1.4.25 W5d Withings additions. Multi-word upper-snake enum names - // never appear in legitimate user-facing prose, so stripping them - // unconditionally is safe. - "FAT_FREE_MASS", - "FAT_MASS", - "MUSCLE_MASS", - "SKIN_TEMPERATURE", - "PULSE_WAVE_VELOCITY", - "VASCULAR_AGE", - "VISCERAL_FAT", - // v1.4.25 W8d Apple Health server-prep — multi-word upper-snake - // names that should never surface verbatim in user-facing prose. - "AUDIO_EXPOSURE_ENV", - "AUDIO_EXPOSURE_HEADPHONE", - "TIME_IN_DAYLIGHT", - // v1.5.5 iOS-coord additions — same shape, same posture. - "RESPIRATORY_RATE", - "BODY_MASS_INDEX", - "LEAN_BODY_MASS", - "WALKING_HEART_RATE_AVERAGE", - "WALKING_ASYMMETRY", - "WALKING_DOUBLE_SUPPORT", - // v1.5.5 iOS-coord follow-up — raw-SI gait pair, same shape. - "WALKING_STEP_LENGTH", - "WALKING_SPEED", - // v1.10.0 — additive HealthKit signals (WX-A), same shape. - "CARDIO_RECOVERY", - "WRIST_TEMPERATURE", - "FALL_COUNT", - "SIX_MINUTE_WALK_DISTANCE", - "STAIR_ASCENT_SPEED", - "STAIR_DESCENT_SPEED", - "BREATHING_DISTURBANCES", - // v1.10.0 — computed scores (WX-C), same shape. Strip the bare enum name - // if the model drops it into prose (mirrors the MOOD_SCORE precedent). - "RECOVERY_SCORE", - "STRESS_SCORE", - "STRAIN_SCORE", - // v1.11.0 — WHOOP-native score classes, same shape: strip the bare enum - // name if the model drops it into prose. - "HRV_RMSSD", - "DAY_STRAIN", - "WORKOUT_STRAIN", - "SLEEP_PERFORMANCE", - "SLEEP_EFFICIENCY", - "SLEEP_CONSISTENCY", - "SLEEP_NEED", - "ENERGY_EXPENDITURE_KJ", - // v1.12.8 — WHOOP cycle + sleep coverage completion, same shape: strip the - // bare enum name if the model drops it into prose. - "AVERAGE_HEART_RATE", - "MAX_HEART_RATE", - "SLEEP_DISTURBANCE_COUNT", - // v1.17.1 — Oura coverage completion, same shape: strip the bare enum name - // if the model drops it into prose. - "SLEEP_SCORE", - "BODY_TEMPERATURE_DEVIATION", ] as const; +/** + * Single words shared with the user-facing metric label. "Your WEIGHT trend is + * stable" is legitimate prose, so these stay out of the stripper. Every entry + * is a single word by construction — an upper-snake name is never prose. + */ +const PROSE_SAFE_SINGLE_WORDS = new Set([ + "WEIGHT", + "PULSE", + "MOOD", + "RESILIENCE", +]); + +/** + * Bare enum names cleaved out of prose, DERIVED from `ALLOWED_CHART_TOKENS` + * rather than hand-maintained. + * + * The list used to be a literal array and drifted every time the allowlist + * grew: by v1.30 eighteen allowlisted metrics had no strip entry, so a model + * writing "Your PHQ9_SCORE is elevated" leaked the raw enum straight into + * rendered text — the exact class this list exists to catch. Deriving it makes + * that drift impossible. + */ +const ORPHAN_ENUMS: readonly string[] = [ + ...ALLOWED_CHART_TOKENS.map((token) => token.replace(/^metric:/, "")).filter( + (name) => !PROSE_SAFE_SINGLE_WORDS.has(name), + ), + ...EXTRA_ORPHAN_ENUMS, +] + // Longest first so the alternation never matches a shorter enum that is a + // prefix of a longer one. + .sort((a, b) => b.length - a.length); + // `\b` boundaries keep ordinary English prose untouched — "weight" // (lowercase) or "BMI" (no underscore) never match. The fixed list // also means we never accidentally strip a single capitalised word diff --git a/src/proxy.ts b/src/proxy.ts index f8712927d..4051783d4 100644 --- a/src/proxy.ts +++ b/src/proxy.ts @@ -188,6 +188,40 @@ function generateNonce(): string { return btoa(binary); } +/** + * Transport-level security headers every response carries, including the early + * exits (worker-only 503, legacy 301s, demo-mode 403, and the login / + * onboarding / MFA redirects) that return before the full header block below. + * + * Those exits used to answer with no HSTS, no framing refusal and no nosniff at + * all. The gap was one hop — a redirect has no rendered body and the follow-up + * navigation re-enters this function and does get the full set — but "the next + * request will be protected" is not a reason for this one not to be, and HSTS + * in particular is worth the most on the first hop over a hostile network. + * + * Deliberately the transport subset, not the whole block: the per-route CSP + * below is nonce-bound and carries carve-outs for the document-serve and share + * routes that only make sense for a response with a body to protect. + */ +function applyBaselineSecurityHeaders(response: NextResponse): NextResponse { + response.headers.set("X-Content-Type-Options", "nosniff"); + response.headers.set("X-Frame-Options", "DENY"); + response.headers.set("Referrer-Policy", "strict-origin-when-cross-origin"); + response.headers.set( + "Permissions-Policy", + "camera=(), microphone=(), geolocation=()", + ); + response.headers.set("Cross-Origin-Opener-Policy", "same-origin"); + response.headers.set("X-Permitted-Cross-Domain-Policies", "none"); + if (process.env.NODE_ENV !== "development") { + response.headers.set( + "Strict-Transport-Security", + "max-age=31536000; includeSubDomains; preload", + ); + } + return response; +} + export function proxy(request: NextRequest) { const { pathname } = request.nextUrl; @@ -195,26 +229,32 @@ export function proxy(request: NextRequest) { // serving requests that would either crash (no DB pool ready for writes from // the worker) or duplicate work that the dedicated web container is doing. if (!shouldRunWeb()) { - return NextResponse.json( - { - data: null, - error: - "This container runs the worker only — point HTTP at the web service.", - }, - { status: 503, headers: { "X-HealthLog-Process-Type": "worker" } }, + return applyBaselineSecurityHeaders( + NextResponse.json( + { + data: null, + error: + "This container runs the worker only — point HTTP at the web service.", + }, + { status: 503, headers: { "X-HealthLog-Process-Type": "worker" } }, + ), ); } // 301 redirects for renamed routes const redirect = LEGACY_REDIRECTS[pathname]; if (redirect) { - return NextResponse.redirect(new URL(redirect, request.url), 301); + return applyBaselineSecurityHeaders( + NextResponse.redirect(new URL(redirect, request.url), 301), + ); } // 301 for the v1.5 admin section-anchor → dynamic-route migration. const legacyAdmin = LEGACY_ADMIN_ANCHORS[pathname]; if (legacyAdmin) { - return NextResponse.redirect(new URL(legacyAdmin, request.url), 301); + return applyBaselineSecurityHeaders( + NextResponse.redirect(new URL(legacyAdmin, request.url), 301), + ); } // Demo mode: block all mutations except login @@ -230,13 +270,15 @@ export function proxy(request: NextRequest) { (entry) => pathname === entry.path && method === entry.method, ) ) { - return NextResponse.json( - { - data: null, - error: "Demo mode: modifications are disabled", - meta: { demo: true }, - }, - { status: 403 }, + return applyBaselineSecurityHeaders( + NextResponse.json( + { + data: null, + error: "Demo mode: modifications are disabled", + meta: { demo: true }, + }, + { status: 403 }, + ), ); } } @@ -252,7 +294,9 @@ export function proxy(request: NextRequest) { if (isSessionGatedPage) { const hasSession = request.cookies.has("healthlog_session"); if (!hasSession) { - return NextResponse.redirect(new URL("/auth/login", request.url)); + return applyBaselineSecurityHeaders( + NextResponse.redirect(new URL("/auth/login", request.url)), + ); } // v1.4.22 C4 — server-side onboarding redirect. Previously the @@ -272,7 +316,9 @@ export function proxy(request: NextRequest) { const isOnboardingSurface = pathname === "/onboarding" || pathname.startsWith("/onboarding/"); if (onboardingPending && !isOnboardingSurface) { - return NextResponse.redirect(new URL("/onboarding", request.url)); + return applyBaselineSecurityHeaders( + NextResponse.redirect(new URL("/onboarding", request.url)), + ); } // v1.23 — admin-enforced MFA forced-enrollment gate. When the operator @@ -289,7 +335,9 @@ export function proxy(request: NextRequest) { const isMfaEnrollSurface = pathname === "/enroll-mfa" || pathname.startsWith("/settings/security"); if (mfaEnrollRequired && !isMfaEnrollSurface && !isOnboardingSurface) { - return NextResponse.redirect(new URL("/enroll-mfa", request.url)); + return applyBaselineSecurityHeaders( + NextResponse.redirect(new URL("/enroll-mfa", request.url)), + ); } }