From 4b87b9e33b6b08ea71f411d8b10427076daebbb0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sun, 19 Jul 2026 13:45:16 +0200 Subject: [PATCH] test: pin three invariants that were fixed but unguarded MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Each of these was already correct in the tree and had nothing asserting it, so a refactor would have regressed it silently. The decrypted-snapshot LRU keys on userId, but only same-user memoisation and scope-change recomputation were covered — dropping the userId segment would have served one account's health snapshot to another with a green suite. The project has shipped one cross-user cache-leak fix already; this is a repeat class. The GlitchTip forwarder redacts path-segment secrets in the url it sends. The query-string strip was tested; the path-segment pass was not, and that is the one that reaches a durable external incident UI. The Withings credentials DELETE narrows its connection-row delete to P2025 and rethrows everything else, so a real failure can no longer null the client credentials and report a successful disconnect while the encrypted OAuth tokens survive. The route had no test file at all. --- .../credentials/__tests__/route.test.ts | 96 ++++++++++++++ .../glitchtip-path-secret-redaction.test.ts | 118 ++++++++++++++++++ .../snapshot-cache-tenant-isolation.test.ts | 80 ++++++++++++ 3 files changed, 294 insertions(+) create mode 100644 src/app/api/withings/credentials/__tests__/route.test.ts create mode 100644 src/lib/__tests__/glitchtip-path-secret-redaction.test.ts create mode 100644 src/lib/ai/coach/__tests__/snapshot-cache-tenant-isolation.test.ts diff --git a/src/app/api/withings/credentials/__tests__/route.test.ts b/src/app/api/withings/credentials/__tests__/route.test.ts new file mode 100644 index 000000000..c5603b222 --- /dev/null +++ b/src/app/api/withings/credentials/__tests__/route.test.ts @@ -0,0 +1,96 @@ +/** + * `DELETE /api/withings/credentials` — disconnect honesty. + * + * The handler deletes the connection row (holding the encrypted OAuth access + + * refresh tokens) before nulling the credential columns. That delete used to be + * `.catch(() => {})`: on a real failure the handler carried on, nulled the + * client id/secret, and returned `{ deleted: true }` while the token-bearing + * row survived — the user is told they are disconnected and they are not. + * + * It now narrows to P2025 ("already gone", a genuine no-op) and rethrows + * everything else. Withings had no test file at all, which is why this one + * exists; the WHOOP and Fitbit twins carry the same shape. + */ +import { describe, it, expect, vi, beforeEach } from "vitest"; + +vi.mock("@/lib/db", () => ({ + prisma: { + withingsConnection: { delete: vi.fn() }, + user: { update: vi.fn() }, + }, +})); + +vi.mock("@/lib/auth/session", () => ({ getSession: vi.fn() })); +vi.mock("@/lib/auth/audit", () => ({ + auditLog: vi.fn().mockResolvedValue(undefined), +})); +vi.mock("@/lib/logging/transports", () => ({ emitIfSampled: vi.fn() })); +vi.mock("@/lib/db-compat", () => ({ + ensureDbCompatibility: vi.fn().mockResolvedValue(undefined), +})); +vi.mock("next/headers", () => ({ + headers: vi.fn(async () => ({ get: () => null })), + cookies: vi.fn(async () => ({ + get: () => undefined, + set: () => {}, + delete: () => {}, + })), +})); + +import { DELETE } from "../route"; +import { prisma } from "@/lib/db"; +import { getSession } from "@/lib/auth/session"; + +const SESSION_OK = { + session: { id: "sess-1", expiresAt: new Date(Date.now() + 3_600_000) }, + user: { id: "user-1", username: "tester", role: "USER" as const }, +}; + +/** Shape of a Prisma "record to delete does not exist" rejection. */ +function p2025() { + return Object.assign(new Error("Record to delete does not exist."), { + code: "P2025", + name: "PrismaClientKnownRequestError", + }); +} + +beforeEach(() => { + vi.resetAllMocks(); + vi.mocked(getSession).mockResolvedValue(SESSION_OK as never); + vi.mocked(prisma.user.update).mockResolvedValue({} as never); +}); + +describe("DELETE /api/withings/credentials", () => { + it("disconnects and nulls the credential columns on the happy path", async () => { + vi.mocked(prisma.withingsConnection.delete).mockResolvedValue({} as never); + const res = await DELETE(); + expect(res.status).toBe(200); + expect(prisma.user.update).toHaveBeenCalledWith( + expect.objectContaining({ + data: { + withingsClientIdEncrypted: null, + withingsClientSecretEncrypted: null, + }, + }), + ); + }); + + it("treats an already-missing connection row (P2025) as a benign no-op", async () => { + vi.mocked(prisma.withingsConnection.delete).mockRejectedValue(p2025()); + const res = await DELETE(); + expect(res.status).toBe(200); + // Idempotent disconnect still clears the credentials. + expect(prisma.user.update).toHaveBeenCalled(); + }); + + it("does NOT report success when the token-bearing row fails to delete", async () => { + vi.mocked(prisma.withingsConnection.delete).mockRejectedValue( + new Error("connection terminated unexpectedly"), + ); + const res = await DELETE(); + expect(res.status).toBe(500); + // Critically: the credentials must NOT be nulled, because doing so while + // the encrypted OAuth tokens survive is the worst of both states. + expect(prisma.user.update).not.toHaveBeenCalled(); + }); +}); diff --git a/src/lib/__tests__/glitchtip-path-secret-redaction.test.ts b/src/lib/__tests__/glitchtip-path-secret-redaction.test.ts new file mode 100644 index 000000000..0193cb82b --- /dev/null +++ b/src/lib/__tests__/glitchtip-path-secret-redaction.test.ts @@ -0,0 +1,118 @@ +/** + * The GlitchTip forwarder must redact path-segment secrets in `url`. + * + * `reportToGlitchtip` strips the query string, which covers `?secret=…` and + * `?code=…&state=…`. It does NOT cover a secret carried as a path SEGMENT — + * `/api/withings/webhook/`, `/api/whoop/webhook/` — and those + * are exactly the shapes `PATH_SECRET_PATHS` exists for. Every other sink (the + * stdout wide event, the admin app-logs render) already redacts them; GlitchTip + * was the one gap, and it is the worst one, because the value lands durably in + * an external incident UI. + * + * The fix is in place. This pins it: the assertion is on the wiring, not on + * `redactSecrets` itself, because the defect was a missing call, not a broken + * redactor. + */ +import { describe, it, expect, vi, beforeEach } from "vitest"; + +vi.mock("@/lib/db", () => ({ + prisma: { + apiToken: { findUnique: vi.fn(), update: vi.fn() }, + user: { findUnique: vi.fn() }, + }, +})); + +vi.mock("@/lib/auth/session", () => ({ getSession: vi.fn() })); +vi.mock("@/lib/auth/audit", () => ({ + auditLog: vi.fn().mockResolvedValue(undefined), +})); +vi.mock("@/lib/logging/transports", () => ({ emitIfSampled: vi.fn() })); +vi.mock("@/lib/db-compat", () => ({ + ensureDbCompatibility: vi.fn().mockResolvedValue(undefined), +})); + +vi.mock("@/lib/monitoring-settings", () => ({ + getGlitchtipSettings: vi.fn(async () => ({ + glitchtipEnabled: true, + glitchtipDsn: "https://key@glitchtip.example/1", + glitchtipEnvironment: "test", + })), +})); + +const sendGlitchtipEvent = vi.fn(async (_payload: unknown) => {}); +vi.mock("@/lib/monitoring/glitchtip", () => ({ + sendGlitchtipEvent: (payload: unknown) => sendGlitchtipEvent(payload), +})); + +vi.mock("next/headers", () => ({ + headers: vi.fn(async () => ({ get: () => null })), + cookies: vi.fn(async () => ({ + get: () => undefined, + set: () => {}, + delete: () => {}, + })), +})); + +import { NextRequest } from "next/server"; +import { apiHandler } from "../api-handler"; + +const WEBHOOK_SECRET = "s3cr3t-webhook-path-segment-value"; + +beforeEach(() => { + sendGlitchtipEvent.mockClear(); +}); + +function throwingHandler() { + return apiHandler(async (_request: NextRequest) => { + throw new Error("boom"); + }); +} + +async function forwardedPayload(url: string) { + const GET = throwingHandler(); + await GET(new NextRequest(url, { method: "GET" })); + // The forwarder is fire-and-forget inside the handler's catch; let its + // dynamic imports and the send settle before asserting. + for (let i = 0; i < 20; i++) await Promise.resolve(); + await new Promise((resolve) => setTimeout(resolve, 0)); + expect(sendGlitchtipEvent).toHaveBeenCalledTimes(1); + const call = sendGlitchtipEvent.mock.calls[0]?.[0] as { + input: { url: string }; + }; + return call.input; +} + +describe("GlitchTip forwarder — path-segment secret redaction", () => { + it("does not forward a Withings webhook path secret verbatim", async () => { + const input = await forwardedPayload( + `http://localhost/api/withings/webhook/${WEBHOOK_SECRET}`, + ); + expect(input.url).not.toContain(WEBHOOK_SECRET); + }); + + it("does not forward a WHOOP webhook path secret verbatim", async () => { + const input = await forwardedPayload( + `http://localhost/api/whoop/webhook/${WEBHOOK_SECRET}`, + ); + expect(input.url).not.toContain(WEBHOOK_SECRET); + }); + + it("does not forward a clinician share token verbatim", async () => { + const shareToken = "hls_abcdef0123456789abcdef0123456789"; + const input = await forwardedPayload(`http://localhost/c/${shareToken}`); + expect(input.url).not.toContain(shareToken); + }); + + it("still strips the query string", async () => { + const input = await forwardedPayload( + "http://localhost/api/auth/oidc/callback?code=abc123&state=xyz789", + ); + expect(input.url).not.toContain("abc123"); + expect(input.url).not.toContain("xyz789"); + }); + + it("leaves an ordinary path readable", async () => { + const input = await forwardedPayload("http://localhost/api/measurements"); + expect(input.url).toContain("/api/measurements"); + }); +}); diff --git a/src/lib/ai/coach/__tests__/snapshot-cache-tenant-isolation.test.ts b/src/lib/ai/coach/__tests__/snapshot-cache-tenant-isolation.test.ts new file mode 100644 index 000000000..11dc6e7dd --- /dev/null +++ b/src/lib/ai/coach/__tests__/snapshot-cache-tenant-isolation.test.ts @@ -0,0 +1,80 @@ +/** + * Cross-tenant isolation of the decrypted-snapshot LRU. + * + * The cache holds a fully-assembled health snapshot — one account's + * measurements, mood, medication intake — for 60 seconds, keyed by + * `${userId}|${window}|${sourceList}`. `userId` IS in the key today, so there + * is no live defect. The gap this file closes is that nothing PINNED it: the + * existing suite covers same-user memoisation and scope-change recomputation, + * so a refactor dropping the `userId` segment would serve one user's snapshot + * to another with a fully green run. + * + * The project has already shipped one cross-user cache-leak fix (v1.25.1), so + * this is a repeat class, not a hypothetical one. + */ +import { beforeEach, describe, expect, it } from "vitest"; + +import { + __resetCoachSnapshotCacheForTests, + readSnapshotCache, + snapshotCacheKey, + writeSnapshotCache, +} from "../snapshot-cache"; +import type { CoachSnapshotResult } from "../snapshot"; +import type { CoachScope } from "../types"; + +function fakeResult(marker: string): CoachSnapshotResult { + return { snapshotJson: marker } as unknown as CoachSnapshotResult; +} + +beforeEach(() => { + __resetCoachSnapshotCacheForTests(); +}); + +describe("snapshot cache — tenant isolation", () => { + it("never returns user A's snapshot to user B at identical scope", () => { + const scope: CoachScope = { window: "last30days" }; + + const keyA = snapshotCacheKey("user-a", scope); + const keyB = snapshotCacheKey("user-b", scope); + + // The keys must differ in the first place — this is the invariant. + expect(keyA).not.toBe(keyB); + + writeSnapshotCache(keyA, fakeResult("A-PRIVATE")); + + // B, at the very same scope, must miss. + expect(readSnapshotCache(keyB)).toBeNull(); + + // And once B has its own entry, the two never cross. + writeSnapshotCache(keyB, fakeResult("B-PRIVATE")); + expect(readSnapshotCache(keyA)?.snapshotJson).toBe("A-PRIVATE"); + expect(readSnapshotCache(keyB)?.snapshotJson).toBe("B-PRIVATE"); + }); + + it("keeps the userId segment leading, so no scope string can forge another tenant", () => { + // A source list is user-controllable in the request. It must not be able to + // reconstruct a different user's key. + const forged = snapshotCacheKey("user-a", { + window: "last30days", + sources: ["user-b"], + } as unknown as CoachScope); + const victim = snapshotCacheKey("user-b", { window: "last30days" }); + expect(forged).not.toBe(victim); + expect(forged.startsWith("user-a|")).toBe(true); + }); + + it("isolates identical explicit source lists across users", () => { + const scope: CoachScope = { + window: "last7days", + sources: ["MANUAL", "APPLE_HEALTH"], + } as unknown as CoachScope; + + const keyA = snapshotCacheKey("user-a", scope); + const keyB = snapshotCacheKey("user-b", scope); + expect(keyA).not.toBe(keyB); + + writeSnapshotCache(keyA, fakeResult("A-PRIVATE")); + expect(readSnapshotCache(keyB)).toBeNull(); + }); +});