From 336ce5aef32da033e86e7a71661acc233b8e365f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Wed, 1 Jul 2026 23:45:08 +0200 Subject: [PATCH] fix(api): narrow swallowed database errors and redact the incident URL The incident forwarder stripped the query string but not path-segment secrets from the reported URL; route it through the same redaction the message and stack already use. Several delete-and-swallow paths caught every error as the expected unique/absent-row case, masking real failures on security-notification, logout, and integration-disconnect paths and the Telegram reply-consume guard; narrow each to the specific Prisma code and let real failures surface. --- src/app/api/fitbit/credentials/route.ts | 9 ++++++- src/app/api/telegram/webhook/route.ts | 19 ++++++++++---- src/app/api/whoop/credentials/route.ts | 9 ++++++- src/app/api/withings/credentials/route.ts | 10 ++++++-- src/lib/api-handler.ts | 15 ++++++++--- src/lib/auth/__tests__/login-alert.test.ts | 25 +++++++++++++++++-- src/lib/auth/__tests__/session.test.ts | 29 +++++++++++++++++++++- src/lib/auth/login-alert.ts | 12 +++++++-- src/lib/auth/session.ts | 9 ++++++- src/lib/prisma-errors.ts | 21 +++++++++++++--- 10 files changed, 136 insertions(+), 22 deletions(-) diff --git a/src/app/api/fitbit/credentials/route.ts b/src/app/api/fitbit/credentials/route.ts index 9ecb39a6e..bef303a15 100644 --- a/src/app/api/fitbit/credentials/route.ts +++ b/src/app/api/fitbit/credentials/route.ts @@ -1,5 +1,6 @@ import { prisma } from "@/lib/db"; import { apiHandler, requireAuth } from "@/lib/api-handler"; +import { isP2025 } from "@/lib/prisma-errors"; import { auditLog } from "@/lib/auth/audit"; import { apiSuccess, apiError, safeJson } from "@/lib/api-response"; import { annotate } from "@/lib/logging/context"; @@ -71,9 +72,15 @@ export const DELETE = apiHandler(async () => { const { user } = await requireAuth(); annotate({ action: { name: "fitbit.credentials.delete" } }); + // A missing connection row (P2025) is a benign "already disconnected" + // no-op; any other failure must propagate rather than be swallowed — + // otherwise the encrypted OAuth tokens can be left orphaned while the user + // is told the integration was disconnected. await prisma.fitbitConnection .delete({ where: { userId: user.id } }) - .catch(() => {}); + .catch((err) => { + if (!isP2025(err)) throw err; + }); await prisma.user.update({ where: { id: user.id }, diff --git a/src/app/api/telegram/webhook/route.ts b/src/app/api/telegram/webhook/route.ts index 7de67e696..d6e9f1555 100644 --- a/src/app/api/telegram/webhook/route.ts +++ b/src/app/api/telegram/webhook/route.ts @@ -1,5 +1,6 @@ import { timingSafeEqual } from "node:crypto"; import { prisma } from "@/lib/db"; +import { isP2025 } from "@/lib/prisma-errors"; import { decrypt } from "@/lib/crypto"; import { answerTelegramCallbackQuery, @@ -945,11 +946,19 @@ async function handleTextMessage(update: TelegramUpdate) { context.userId === user.id && (context.kind === "mood_note" || context.kind === "measure_value") ) { - // Consume the context row regardless of outcome so a reply can't be - // replayed onto the entry twice. - await prisma.telegramPromptContext - .delete({ where: { id: context.id } }) - .catch(() => {}); + // The delete IS the replay guard: consuming the context row is what + // stops a redelivered reply from being applied to the entry twice. Make + // it authoritative — if the delete fails for any reason OTHER than the + // row already being gone (P2025), do NOT apply the reply; abort and let + // Telegram redeliver against a still-present row. P2025 means the row + // was already consumed, which is the guard succeeding, so proceed. + try { + await prisma.telegramPromptContext.delete({ + where: { id: context.id }, + }); + } catch (err) { + if (!isP2025(err)) return; + } const userMsgId = message?.message_id; if (context.expiresAt.getTime() < Date.now()) { const resp = await sendTelegramMessage( diff --git a/src/app/api/whoop/credentials/route.ts b/src/app/api/whoop/credentials/route.ts index 6675fa11e..ffc7e9183 100644 --- a/src/app/api/whoop/credentials/route.ts +++ b/src/app/api/whoop/credentials/route.ts @@ -1,5 +1,6 @@ import { prisma } from "@/lib/db"; import { apiHandler, requireAuth } from "@/lib/api-handler"; +import { isP2025 } from "@/lib/prisma-errors"; import { auditLog } from "@/lib/auth/audit"; import { apiSuccess, apiError, safeJson } from "@/lib/api-response"; import { annotate } from "@/lib/logging/context"; @@ -69,9 +70,15 @@ export const DELETE = apiHandler(async () => { const { user } = await requireAuth(); annotate({ action: { name: "whoop.credentials.delete" } }); + // A missing connection row (P2025) is a benign "already disconnected" + // no-op; any other failure must propagate rather than be swallowed — + // otherwise the encrypted OAuth tokens can be left orphaned while the user + // is told the integration was disconnected. await prisma.whoopConnection .delete({ where: { userId: user.id } }) - .catch(() => {}); + .catch((err) => { + if (!isP2025(err)) throw err; + }); await prisma.user.update({ where: { id: user.id }, diff --git a/src/app/api/withings/credentials/route.ts b/src/app/api/withings/credentials/route.ts index 0bd669f86..429514f95 100644 --- a/src/app/api/withings/credentials/route.ts +++ b/src/app/api/withings/credentials/route.ts @@ -1,6 +1,7 @@ import { prisma } from "@/lib/db"; import { apiHandler, requireAuth } from "@/lib/api-handler"; import { apiSuccess, apiError, safeJson } from "@/lib/api-response"; +import { isP2025 } from "@/lib/prisma-errors"; import { annotate } from "@/lib/logging/context"; import { encrypt } from "@/lib/crypto"; import { withingsCredentialsSchema } from "@/lib/validations/withings"; @@ -64,10 +65,15 @@ export const DELETE = apiHandler(async () => { const { user } = await requireAuth(); annotate({ action: { name: "withings.credentials.delete" } }); - // Remove connection first + // Remove connection first. A missing connection row (P2025) is a benign + // "already disconnected" no-op; any other failure must propagate rather + // than be swallowed — otherwise the encrypted OAuth tokens can be left + // orphaned while the user is told the integration was disconnected. await prisma.withingsConnection .delete({ where: { userId: user.id } }) - .catch(() => {}); + .catch((err) => { + if (!isP2025(err)) throw err; + }); // Remove credentials await prisma.user.update({ diff --git a/src/lib/api-handler.ts b/src/lib/api-handler.ts index 0250bbe3c..3de3383e9 100644 --- a/src/lib/api-handler.ts +++ b/src/lib/api-handler.ts @@ -340,9 +340,11 @@ export async function requireAuth( * * The validation itself lives in the transport-agnostic `resolveBearerToken` * (`./auth/bearer`) — the single source of truth shared with the MCP wire. This - * wrapper adds the HTTP-edge concerns: the `auth.bearer.*` audit trail for both - * outcomes and the Wide-Event `auth_method: "bearer"` annotation, then maps the - * result onto the `AuthContext` contract (`session.id` carries the token id). + * wrapper adds the HTTP-edge concerns: the `auth.bearer.failure` audit trail + * (only the failure path writes a durable audit row — the success row was + * intentionally dropped for perf) and the Wide-Event `auth_method: "bearer"` + * annotation, then maps the result onto the `AuthContext` contract + * (`session.id` carries the token id). * * Authorisation contract (unchanged): a route that declares no * `requiredPermission` accepts any valid token; one that declares a scope @@ -648,7 +650,12 @@ async function reportToGlitchtip( level: "error", type: err.name || "Error", stack: redactOptional(err.stack), - url: scrubbedUrl, + // Query string is already stripped above, but path-segment secrets + // (e.g. `/api/withings/webhook/`, `/api/whoop/webhook/`) + // survive that strip. Run the same `redactSecrets` pass that guards the + // message/stack so a `PATH_SECRET_PATHS`-registered secret cannot reach + // the external incident UI. + url: redactSecrets(scrubbedUrl), sourceTag: "healthlog-api-handler", requestId: evt.getRequestId(), }, diff --git a/src/lib/auth/__tests__/login-alert.test.ts b/src/lib/auth/__tests__/login-alert.test.ts index a4ab165a9..34bc65e09 100644 --- a/src/lib/auth/__tests__/login-alert.test.ts +++ b/src/lib/auth/__tests__/login-alert.test.ts @@ -117,10 +117,10 @@ describe("recordSignInDevice — new-device dedupe", () => { expect(dispatchNotification).not.toHaveBeenCalled(); }); - it("a unique-index race on create does not double-alert", async () => { + it("a unique-index race (P2002) on create does not double-alert", async () => { vi.mocked(prisma.userKnownDevice.findUnique).mockResolvedValue(null); vi.mocked(prisma.userKnownDevice.create).mockRejectedValue( - new Error("unique violation"), + Object.assign(new Error("unique violation"), { code: "P2002" }), ); const res = await recordSignInDevice({ @@ -133,6 +133,27 @@ describe("recordSignInDevice — new-device dedupe", () => { expect(dispatchNotification).not.toHaveBeenCalled(); }); + it("a non-P2002 create failure is NOT masked as a known device", async () => { + // A real DB fault must not be misclassified as "device already known" — + // that would silently skip the SECURITY_ALERT + audit row. The narrowed + // catch rethrows it to the outer handler, which records the failure and + // returns the safe `{ known: false, alerted: false }` verdict without + // dispatching a bogus alert. + vi.mocked(prisma.userKnownDevice.findUnique).mockResolvedValue(null); + vi.mocked(prisma.userKnownDevice.create).mockRejectedValue( + Object.assign(new Error("connection reset"), { code: "P1001" }), + ); + + const res = await recordSignInDevice({ + userId: "u1", + ip: "203.0.113.9", + userAgent: UA, + }); + + expect(res).toEqual({ known: false, alerted: false }); + expect(dispatchNotification).not.toHaveBeenCalled(); + }); + it("never persists the raw User-Agent — only the salted hash + coarse label", async () => { vi.mocked(prisma.userKnownDevice.findUnique).mockResolvedValue(null); vi.mocked(prisma.userKnownDevice.create).mockResolvedValue({} as never); diff --git a/src/lib/auth/__tests__/session.test.ts b/src/lib/auth/__tests__/session.test.ts index e07eb858d..57a01f035 100644 --- a/src/lib/auth/__tests__/session.test.ts +++ b/src/lib/auth/__tests__/session.test.ts @@ -10,6 +10,7 @@ vi.mock("@/lib/db", () => ({ prisma: { session: { findUnique: vi.fn(), + delete: vi.fn(), deleteMany: vi.fn(), update: vi.fn(), }, @@ -46,7 +47,7 @@ vi.mock("next/headers", () => ({ })); import { prisma } from "@/lib/db"; -import { getSession, destroyAllSessions } from "../session"; +import { getSession, destroyAllSessions, destroySession } from "../session"; beforeEach(() => { vi.resetAllMocks(); @@ -74,6 +75,32 @@ describe("getSession", () => { }); }); +describe("destroySession", () => { + it("treats an already-deleted session row (P2025) as an idempotent logout", async () => { + cookieState.sessionId = "sess-gone"; + vi.mocked(prisma.session.delete).mockRejectedValue( + Object.assign(new Error("record not found"), { code: "P2025" }) as never, + ); + + await expect(destroySession()).resolves.toBeUndefined(); + + expect(prisma.session.delete).toHaveBeenCalledWith({ + where: { id: "sess-gone" }, + }); + expect(cookieState.delete).toHaveBeenCalledWith("healthlog_session"); + expect(cookieState.delete).toHaveBeenCalledWith("hl_onboarding"); + }); + + it("propagates a real delete failure instead of reporting a false logout", async () => { + cookieState.sessionId = "sess-live"; + vi.mocked(prisma.session.delete).mockRejectedValue( + Object.assign(new Error("connection reset"), { code: "P1001" }) as never, + ); + + await expect(destroySession()).rejects.toThrow(/connection reset/); + }); +}); + describe("destroyAllSessions", () => { it("revokes web sessions, API tokens, and refresh tokens for the user", async () => { vi.mocked(prisma.session.deleteMany).mockResolvedValue({ diff --git a/src/lib/auth/login-alert.ts b/src/lib/auth/login-alert.ts index 1cad93740..6a2a8b309 100644 --- a/src/lib/auth/login-alert.ts +++ b/src/lib/auth/login-alert.ts @@ -24,6 +24,7 @@ import { dispatchNotification } from "@/lib/notifications/dispatcher"; import { getServerTranslator } from "@/lib/i18n/server-translator"; import { defaultLocale, type Locale } from "@/lib/i18n/config"; import { getEvent } from "@/lib/logging/context"; +import { isP2002 } from "@/lib/prisma-errors"; function isLocale(value: string | null | undefined): value is Locale { return ( @@ -105,8 +106,15 @@ export async function recordSignInDevice(params: { await prisma.userKnownDevice.create({ data: { userId, deviceHash, label: storedLabel }, }); - } catch { - return { known: true, alerted: false }; + } catch (err) { + // ONLY the unique-index race (a concurrent first-login inserting the + // same (userId, deviceHash)) is a legitimate "already known" outcome — + // swallow it and skip the alert so we never double-alert. Any other + // failure (a real DB fault) must NOT be silently classified as a known + // device, which would suppress the SECURITY_ALERT + audit row: rethrow + // so the outer catch records it via `addWarning`. + if (isP2002(err)) return { known: true, alerted: false }; + throw err; } if (!alertOnNew || isBaseline) return { known: false, alerted: false }; diff --git a/src/lib/auth/session.ts b/src/lib/auth/session.ts index 61511b4f2..7dd461668 100644 --- a/src/lib/auth/session.ts +++ b/src/lib/auth/session.ts @@ -4,6 +4,7 @@ import type { User } from "@/generated/prisma/client"; import { ensureDbCompatibility } from "@/lib/db-compat"; import { getEvent } from "@/lib/logging/context"; import { shouldEmitSecureCookie } from "@/lib/auth/secure-cookie"; +import { isP2025 } from "@/lib/prisma-errors"; const SESSION_COOKIE = "healthlog_session"; const SESSION_MAX_AGE_MS = 30 * 24 * 60 * 60 * 1000; // 30 days @@ -179,7 +180,13 @@ export async function destroySession(): Promise { const cookieStore = await cookies(); const sessionId = cookieStore.get(SESSION_COOKIE)?.value; if (sessionId) { - await prisma.session.delete({ where: { id: sessionId } }).catch(() => {}); + // Logout is idempotent: a session row that no longer exists (P2025) is a + // no-op success — the cookie clear below still runs. Any other failure is + // a real delete error that must NOT be masked as a successful logout, so + // it propagates rather than being swallowed. + await prisma.session.delete({ where: { id: sessionId } }).catch((err) => { + if (!isP2025(err)) throw err; + }); } cookieStore.delete(SESSION_COOKIE); cookieStore.delete(ONBOARDING_COOKIE); diff --git a/src/lib/prisma-errors.ts b/src/lib/prisma-errors.ts index bf6096656..67745ad7a 100644 --- a/src/lib/prisma-errors.ts +++ b/src/lib/prisma-errors.ts @@ -13,12 +13,27 @@ * unique-constraint code. */ -/** Narrow a thrown Prisma error to the P2002 unique-constraint code. */ -export function isP2002(err: unknown): boolean { +/** Narrow a thrown Prisma error to a specific known-request code. */ +function hasPrismaCode(err: unknown, code: string): boolean { return ( typeof err === "object" && err !== null && "code" in err && - (err as { code: unknown }).code === "P2002" + (err as { code: unknown }).code === code ); } + +/** Narrow a thrown Prisma error to the P2002 unique-constraint code. */ +export function isP2002(err: unknown): boolean { + return hasPrismaCode(err, "P2002"); +} + +/** + * Narrow a thrown Prisma error to the P2025 "record not found" code — the + * error `delete` / `update` raise when the target row no longer exists. Lets + * an idempotent delete treat "already gone" as success while still surfacing a + * genuine failure (a real DB fault raises a different code and rethrows). + */ +export function isP2025(err: unknown): boolean { + return hasPrismaCode(err, "P2025"); +}