diff --git a/.gitleaksignore b/.gitleaksignore new file mode 100644 index 000000000..37ed30dec --- /dev/null +++ b/.gitleaksignore @@ -0,0 +1,6 @@ +# Fingerprints of known false positives, one per line: +# ::: +# Fake Supabase-style secret fixture in a test that verifies secret *redaction*. +# The current tip no longer matches the rule, but this historical commit is +# still scanned on every PR run. +92cd8ac656800a0db504bdf9559bd9b30b52212c:tests/privacy.test.ts:generic-api-key:32 diff --git a/docs/superpowers/specs/2026-07-02-triage-security-reliability-design.md b/docs/superpowers/specs/2026-07-02-triage-security-reliability-design.md new file mode 100644 index 000000000..e586e7e85 --- /dev/null +++ b/docs/superpowers/specs/2026-07-02-triage-security-reliability-design.md @@ -0,0 +1,103 @@ +# Triage Security & Reliability Fixes Design + +Date: 2026-07-02 +Scope: Issue #53 (security), #55 (reliability gaps only), #56 (no change unless concrete gap appears) + +## 1. Goals and non-goals + +### Goals +- Remove authentication bypass behavior so protected APIs fail closed. +- Stop client-side reliance on persisted localStorage auth token hints. +- Redact high-risk identifiers from generated image captions before persistence. +- Ensure Supabase-related error logging uses redacted/safe detail formatting. +- Close currently open reliability gaps from #55 with minimal, targeted changes (especially embedding-dimension drift safeguards). + +### Non-goals +- No front-end refactor work from #51. +- No performance-oriented search/ingestion optimization from #52. +- No broad operational redesign for #56; only patch ops docs/scripts if implementation reveals a concrete missing step. + +## 2. Recommended approach + +Use a surgical patch strategy that edits only existing touch points and preserves current architecture: + +1. Enforce strict auth in `src/lib/supabase/auth.ts` by removing local fallback owner resolution for protected request flows. +2. Update client auth usage in `src/lib/supabase/client.tsx` and dependent dashboard checks in `src/components/ClinicalDashboard.tsx` so privileged behavior is derived from Supabase session state rather than localStorage token presence hints. +3. Add caption identifier sanitization before writing generated captions to `document_images` and `image_caption_cache`. +4. Normalize Supabase-related logging callsites to use existing safe redaction utilities. +5. Strengthen embedding dimension guardrails using a single expected-dimension source and explicit mismatch handling where ingestion asserts are performed. + +This is preferred over broader rewrites because it directly addresses active risk while minimizing regression surface. + +## 3. Architecture and component changes + +### 3.1 Authentication boundary +- Primary file: `src/lib/supabase/auth.ts`. +- Change: remove environment-based no-auth fallback paths for protected API authorization. +- Result: auth gate requires valid Supabase-authenticated user identity; invalid/missing identity returns explicit unauthorized response paths. + +### 3.2 Client-side session handling +- Primary files: `src/lib/supabase/client.tsx`, `src/components/ClinicalDashboard.tsx`. +- Change: stop relying on localStorage token presence scans and persisted auth-email hints for deciding private API/session capability. +- Result: UI behavior follows actual Supabase session/auth state only. + +### 3.3 Caption redaction before persistence +- Primary file: `worker/main.ts` (with helper placement in shared privacy utility area where appropriate). +- Change: sanitize generated caption text before database/cache writes. +- Baseline redaction targets: + - Email-like identifiers + - Phone-like strings + - MRN/NHS-style identifier patterns +- Result: ingestion remains functional, but persisted captions are safer by default. + +### 3.4 Safe Supabase logging +- Primary files: worker/server/script callsites identified during implementation. +- Change: route Supabase-related error detail formatting through existing safe redaction utilities instead of direct raw detail logging. +- Result: operational logs remain actionable without leaking secret/token/identifier content. + +### 3.5 Embedding-dimension drift safeguards +- Primary file: `src/lib/embedding-dimensions.ts` and nearby ingestion assertions/tests. +- Change: align expected dimension checks to a single configuration source used by ingestion-time assertions. +- Result: mismatches fail fast and predictably, avoiding silent search-quality corruption. + +## 4. Data flow and behavior + +1. Protected API request arrives. +2. Auth utility validates bearer token via Supabase. +3. If invalid/missing identity: return unauthorized; no fallback identity resolution. +4. Ingestion pipeline generates captions for images. +5. Caption text is sanitized for high-risk identifiers before persistence. +6. Sanitized caption is written to caption tables/cache. +7. Embedding generation/check asserts configured dimension; mismatch triggers deterministic failure with redacted diagnostics. +8. Supabase-related errors along these paths are logged via safe redaction formatting. + +## 5. Error handling model + +- Preserve existing control flow shape; do not add broad catch-and-ignore blocks. +- Fail closed on auth. +- Continue ingestion with sanitized captions when sanitization succeeds. +- Fail ingestion unit explicitly on embedding-dimension mismatch. +- Keep log messages actionable while redacting sensitive fields. + +## 6. Verification strategy + +1. Run targeted tests for modified auth and ingestion dimension/caption paths. +2. Run focused checks for fail-closed auth and redacted logging behavior in touched areas. +3. Run `npm run verify:cheap` after changes. +4. Only add #56 docs/script updates if a concrete ops gap is discovered during implementation. + +## 7. Risks and mitigations + +- Risk: strict auth may reveal latent callers depending on fallback behavior. + Mitigation: adjust only protected-path behavior and keep unauthorized responses explicit/consistent. + +- Risk: caption redaction could over-redact useful text. + Mitigation: target clear identifier patterns first and keep clinical context text unchanged. + +- Risk: logging redaction could remove needed diagnostics. + Mitigation: preserve non-sensitive context (operation, status, IDs safe for logs) while masking secrets/identifiers. + +## 8. Implementation boundaries + +- In scope now: #53 + #55 open gaps, with minimal #56 follow-up only if necessary. +- Out of scope now: #51 refactor, #52 performance program. diff --git a/src/app/api/search/route.ts b/src/app/api/search/route.ts index 3b61c6969..0023f1967 100644 --- a/src/app/api/search/route.ts +++ b/src/app/api/search/route.ts @@ -23,6 +23,7 @@ import { queryPrivacyMetadata, queryTextForStorage, } from "@/lib/query-privacy"; +import { safeErrorLogDetails } from "@/lib/privacy"; import type { ChunkImage, ClinicalSourceMetadata, SearchResult } from "@/lib/types"; export const runtime = "nodejs"; @@ -544,8 +545,9 @@ function logRetrievalDiagnostics(args: { } catch (error) { retrievalLogWriteMetrics.failures += 1; retrievalLogWriteMetrics.lastFailureAt = new Date().toISOString(); + const safe = safeErrorLogDetails(error); retrievalLogWriteMetrics.lastFailureMessage = - error instanceof Error ? `${error.name}: ${error.message}` : "Unknown retrieval logging error"; + typeof safe.message === "string" ? safe.message : "Unknown retrieval logging error"; if (retrievalLogWriteMetrics.failures <= 3 || retrievalLogWriteMetrics.failures % 25 === 0) { console.warn("rag_retrieval_logs insert failed", { ...retrievalLogWriteMetrics, diff --git a/src/components/ClinicalDashboard.tsx b/src/components/ClinicalDashboard.tsx index b87bee2de..8aadffcf2 100644 --- a/src/components/ClinicalDashboard.tsx +++ b/src/components/ClinicalDashboard.tsx @@ -5775,16 +5775,10 @@ export function ClinicalDashboard({ const clientDemoMode = explicitDemoMode || browserAuthUnavailableDemoFallback || localNoAuthMode; const uploadReadOnlyMode = demoMode || process.env.NEXT_PUBLIC_DEMO_MODE === "true" || browserAuthUnavailableDemoFallback; - const storedSessionExists = - typeof window !== "undefined" && - Object.keys(localStorage).some((k) => k.startsWith("sb-") && k.endsWith("-auth-token")); const localDevCanAttemptPrivateApis = process.env.NODE_ENV !== "production" && hasReadyPublicSearchSetup(setupChecks); const canUsePrivateApis = localProjectReady && - (localNoAuthMode || - localDevCanAttemptPrivateApis || - authStatus === "authenticated" || - (supabaseEnvStatus === "ready" && storedSessionExists)); + (localNoAuthMode || localDevCanAttemptPrivateApis || authStatus === "authenticated"); const canRunSearch = explicitDemoMode || (hasReadyPublicSearchSetup(setupChecks) && canUsePrivateApis); const closeDashboardTransientSurfaces = useCallback( (except?: "guide" | "settings" | "mobileSidebar" | "documents" | "upload") => { diff --git a/src/lib/embedding-dimensions.ts b/src/lib/embedding-dimensions.ts index b2be98689..2e8239cac 100644 --- a/src/lib/embedding-dimensions.ts +++ b/src/lib/embedding-dimensions.ts @@ -1,4 +1,21 @@ -export const EXPECTED_EMBED_DIM = 1536; +import * as envModule from "./env"; + +function expectedEmbeddingDimensions() { + const configured = + Object.prototype.hasOwnProperty.call(envModule, "env") && + typeof (envModule as { env?: unknown }).env === "object" && + (envModule as { env?: { EMBEDDING_DIMENSIONS?: unknown } }).env !== null + ? (envModule as { env?: { EMBEDDING_DIMENSIONS?: unknown } }).env?.EMBEDDING_DIMENSIONS + : undefined; + if (typeof configured === "number" && Number.isInteger(configured) && configured > 0) { + return configured; + } + + const fallback = Number(process.env.EMBEDDING_DIMENSIONS ?? 1536); + return Number.isInteger(fallback) && fallback > 0 ? fallback : 1536; +} + +export const EXPECTED_EMBED_DIM = expectedEmbeddingDimensions(); export function assertEmbeddingDim(vec: unknown, context: string): number[] { if (!Array.isArray(vec)) { diff --git a/src/lib/privacy.ts b/src/lib/privacy.ts index b6790ae7c..17f4595da 100644 --- a/src/lib/privacy.ts +++ b/src/lib/privacy.ts @@ -19,10 +19,11 @@ function redactLogValue(value: unknown): unknown { const htmlTitle = value.match(/\s*([^<]+?)\s*<\/title>/i)?.[1]?.trim(); const normalizedValue = htmlTitle ? `HTML response: ${htmlTitle}` : value; return normalizedValue - .replace(/\b[A-Za-z]:\\[^\s'")]+/g, "[path]") - .replace(/\/(?:[^\s'")]+\/)+[^\s'")]+/g, "[path]") - .replace(/https?:\/\/[^\s'")]+/g, "[url]") - .replace(/\b(?:sk|pk|sbp|eyJ)[A-Za-z0-9._-]{12,}\b/g, "[secret]") + .replace(/\b[A-Za-z]:\\[^\s'\")]+/g, "[path]") + .replace(/\/(?:[^\s'\")]+\/)+[^\s'\")]+/g, "[path]") + .replace(/https?:\/\/[^\s'\")]+/g, "[url]") + // Redact common secret/token formats, including modern Supabase keys like sb_secret_ and sb_publishable_ + .replace(/\b(?:sk|pk|sbp|sb_secret_|sb_publishable_|eyJ)[A-Za-z0-9._-]{8,}\b/g, "[secret]") .replace(/\b[A-Za-z0-9._%+-]+@[A-Za-z0-9.-]+\.[A-Za-z]{2,}\b/g, "[email]") .slice(0, 500); } @@ -52,3 +53,20 @@ export function safeErrorLogDetails(error: unknown) { ...(firstStackLine ? { stack: redactLogValue(firstStackLine) } : {}), }; } + +export function redactCaptionIdentifiers(value: string): string { + const clinicalRangePattern = /^\d+(?:\.\d+)?\s*-\s*\d+(?:\.\d+)?(?:\s*[A-Za-zµ/%][\w/%.-]*)?$/i; + return value + .replace(/\b[A-Za-z0-9._%+-]+@[A-Za-z0-9.-]+\.[A-Za-z]{2,}\b/g, "[email]") + .replace(/\b(?:mrn|nhs)\s*[:#-]?\s*([0-9]+(?:[ \-][0-9]+)+|[A-Za-z0-9-]{4,})\b/gi, (match, idPart: string) => { + const trimmed = idPart.replace(/\s+/g, " ").trim(); + // Count digits only; require at least 4 digits to consider it an identifier (avoids short numeric ranges). + const digitCount = trimmed.replace(/\D/g, "").length; + if (digitCount > 0 && digitCount < 4) return match; + return "[id]"; + }) + .replace(/\b(?:\+?\d[\d\s().-]{6,}\d)\b/g, (match) => { + const digits = match.replace(/\D/g, ""); + return digits.length >= 8 && !clinicalRangePattern.test(match.trim()) ? "[phone]" : match; + }); +} diff --git a/src/lib/supabase/auth.ts b/src/lib/supabase/auth.ts index b567a26e6..721c4a7dd 100644 --- a/src/lib/supabase/auth.ts +++ b/src/lib/supabase/auth.ts @@ -1,7 +1,5 @@ import { NextResponse } from "next/server"; import { createAdminClient } from "@/lib/supabase/admin"; -import { env, isLocalNoAuthMode } from "@/lib/env"; -import { isSafeLocalProjectRequest } from "@/lib/local-project-guard"; type AdminClient = ReturnType<typeof createAdminClient>; @@ -9,31 +7,6 @@ export type AuthenticatedUser = { id: string; }; -const LOCAL_OWNER_CACHE_TTL_MS = 5 * 60_000; - -type LocalOwnerResolutionState = { - cache: { - cacheKey: string; - expiresAt: number; - user: AuthenticatedUser; - } | null; - inFlight: { - cacheKey: string; - promise: Promise<AuthenticatedUser>; - } | null; -}; - -type GlobalWithLocalOwnerResolutionState = typeof globalThis & { - __clinicalKbLocalOwnerResolutionState?: LocalOwnerResolutionState; -}; - -const localOwnerResolutionState = (( - globalThis as GlobalWithLocalOwnerResolutionState -).__clinicalKbLocalOwnerResolutionState ??= { - cache: null, - inFlight: null, -}); - function readCookies(cookieHeader: string | null): Map<string, string> { if (!cookieHeader) return new Map<string, string>(); @@ -95,22 +68,13 @@ export function unauthorizedResponse(error?: AuthenticationError) { } export async function requireAuthenticatedUser(request: Request, supabase: AdminClient): Promise<AuthenticatedUser> { - if (isLocalNoAuthMode()) { - if (!isSafeLocalProjectRequest(request)) { - throw new AuthenticationError("Use the ensured Clinical KB local URL before calling private APIs."); - } - return resolveLocalNoAuthUser(supabase); - } - const token = extractSessionAccessToken(request); - if (!token) { throw new AuthenticationError(); } const { data, error } = await supabase.auth.getUser(token); const userId = data.user?.id; - if (error || !userId) { throw new AuthenticationError(); } @@ -118,107 +82,3 @@ export async function requireAuthenticatedUser(request: Request, supabase: Admin return { id: userId }; } -async function resolveLocalNoAuthUser(supabase: AdminClient): Promise<AuthenticatedUser> { - const configuredOwnerId = env.LOCAL_NO_AUTH_OWNER_ID?.trim(); - if (configuredOwnerId) { - if (!isUuid(configuredOwnerId)) { - throw new Error("LOCAL_NO_AUTH_OWNER_ID must be a valid UUID."); - } - return { id: configuredOwnerId }; - } - - const configuredOwnerEmail = env.LOCAL_NO_AUTH_OWNER_EMAIL?.trim(); - const cacheKey = `email:${configuredOwnerEmail?.toLowerCase() ?? ""}:documents-fallback`; - const now = Date.now(); - - if (localOwnerResolutionState.cache?.cacheKey === cacheKey && localOwnerResolutionState.cache.expiresAt > now) { - return localOwnerResolutionState.cache.user; - } - - if (localOwnerResolutionState.inFlight?.cacheKey === cacheKey) { - return localOwnerResolutionState.inFlight.promise; - } - - const promise = resolveLocalNoAuthOwnerId(supabase, configuredOwnerEmail).then((ownerId) => { - const user = { id: ownerId }; - localOwnerResolutionState.cache = { - cacheKey, - expiresAt: Date.now() + LOCAL_OWNER_CACHE_TTL_MS, - user, - }; - return user; - }); - - localOwnerResolutionState.inFlight = { cacheKey, promise }; - - try { - return await promise; - } finally { - if (localOwnerResolutionState.inFlight?.promise === promise) { - localOwnerResolutionState.inFlight = null; - } - } -} - -async function resolveLocalNoAuthOwnerId(supabase: AdminClient, configuredOwnerEmail?: string) { - const ownerIdFromEmail = await resolveOwnerByEmail(supabase, configuredOwnerEmail); - if (ownerIdFromEmail) return ownerIdFromEmail; - - const fallbackOwnerId = await resolveOwnerFromDocuments(supabase); - if (fallbackOwnerId) return fallbackOwnerId; - - throw new Error( - "Local no-auth mode is enabled, but no owner could be resolved. Set LOCAL_NO_AUTH_OWNER_ID or " + - "LOCAL_NO_AUTH_OWNER_EMAIL, or ensure the documents table has at least one row with owner_id.", - ); -} - -function isUuid(value: string) { - return /^[0-9a-f]{8}-[0-9a-f]{4}-[1-5][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i.test(value); -} - -async function resolveOwnerByEmail(supabase: AdminClient, ownerEmail?: string) { - if (!ownerEmail) return null; - - const normalizedEmail = ownerEmail.trim().toLowerCase(); - if (!normalizedEmail) return null; - - let page = 1; - const seenPages = new Set<number>(); - - while (page > 0) { - if (seenPages.has(page)) { - throw new Error("Failed to resolve owner by email because the admin user listing returned a pagination loop."); - } - seenPages.add(page); - - const { data, error } = await supabase.auth.admin.listUsers({ page, perPage: 100 }); - if (error) { - throw new Error(`Failed to resolve local owner from email: ${error.message}`); - } - if (!data?.users?.length) break; - - const found = data.users.find((user) => user.email?.toLowerCase() === normalizedEmail); - if (found?.id) return found.id; - - page = typeof data.nextPage === "number" ? data.nextPage : 0; - } - - return null; -} - -async function resolveOwnerFromDocuments(supabase: AdminClient) { - const { data, error } = await supabase - .from("documents") - .select("owner_id") - .not("owner_id", "is", null) - .order("created_at", { ascending: false }) - .limit(1) - .maybeSingle(); - - if (error) { - throw new Error(`Failed to resolve local owner fallback from documents: ${error.message}`); - } - - return data?.owner_id ?? null; -} diff --git a/src/lib/supabase/client.tsx b/src/lib/supabase/client.tsx index 57ad334bf..905638996 100644 --- a/src/lib/supabase/client.tsx +++ b/src/lib/supabase/client.tsx @@ -57,7 +57,7 @@ function createBrowserSupabaseClient() { browserSupabaseClientConfig = configKey; browserSupabaseClient = createClient(url, publishableKey, { auth: { - persistSession: true, + persistSession: false, autoRefreshToken: true, detectSessionInUrl: true, }, @@ -66,9 +66,8 @@ function createBrowserSupabaseClient() { } export function authorizationHeadersForAccessToken(accessToken: string | null | undefined): Record<string, string> { - const headers: Record<string, string> = {}; - if (accessToken) headers.authorization = `Bearer ${accessToken}`; - return headers; + if (!accessToken) return {}; + return { authorization: `Bearer ${accessToken}` }; } function clearLocationHash() { @@ -213,12 +212,6 @@ export function AuthProvider({ children }: { children: ReactNode }) { return; } - try { - window.localStorage.setItem(AUTH_EMAIL_STORAGE_KEY, email); - } catch { - // localStorage may be unavailable in restrictive browser modes. - } - setStatus("loading"); setError(null); const { error: signInError } = await client.auth.signInWithOtp({ @@ -279,3 +272,4 @@ export function useAuthSession() { } return context; } + diff --git a/tests/embedding-dimensions.test.ts b/tests/embedding-dimensions.test.ts index f4526e08a..6641fe588 100644 --- a/tests/embedding-dimensions.test.ts +++ b/tests/embedding-dimensions.test.ts @@ -1,18 +1,30 @@ -import { describe, expect, it } from "vitest"; -import { EXPECTED_EMBED_DIM, assertEmbeddingDim } from "../src/lib/embedding-dimensions"; +import { describe, expect, it, vi } from "vitest"; describe("strict embedding dimension guard", () => { - it("accepts only finite 1536-dimensional vectors", () => { + it("derives EXPECTED_EMBED_DIM from EMBEDDING_DIMENSIONS at module load time", async () => { + vi.resetModules(); + vi.stubEnv("EMBEDDING_DIMENSIONS", "3"); + const mod = await import("../src/lib/embedding-dimensions"); + const vector = [0.1, 0.2]; + + expect(mod.EXPECTED_EMBED_DIM).toBe(3); + expect(() => mod.assertEmbeddingDim(vector, "test_vector")).toThrow(/2 dimensions; expected 3/); + }); + + it("accepts only finite vectors of the configured dimension", async () => { + const { EXPECTED_EMBED_DIM, assertEmbeddingDim } = await import("../src/lib/embedding-dimensions"); const vector = Array.from({ length: EXPECTED_EMBED_DIM }, () => 0.01); expect(assertEmbeddingDim(vector, "test_vector")).toBe(vector); }); - it("rejects non-arrays, wrong dimensions, and non-finite values", () => { + it("rejects non-arrays, wrong dimensions, and non-finite values", async () => { + const { EXPECTED_EMBED_DIM, assertEmbeddingDim } = await import("../src/lib/embedding-dimensions"); + expect(() => assertEmbeddingDim("not-a-vector", "test_vector")).toThrow(/must be an array/); - expect(() => assertEmbeddingDim([0.1, 0.2], "test_vector")).toThrow(/2 dimensions; expected 1536/); + expect(() => assertEmbeddingDim([0.1, 0.2], "test_vector")).toThrow(new RegExp(`2 dimensions; expected ${EXPECTED_EMBED_DIM}`)); expect(() => assertEmbeddingDim([...Array.from({ length: EXPECTED_EMBED_DIM - 1 }, () => 0), Infinity], "test_vector"), - ).toThrow(/non-finite value at index 1535/); + ).toThrow(new RegExp(`non-finite value at index ${EXPECTED_EMBED_DIM - 1}`)); }); }); diff --git a/tests/privacy.test.ts b/tests/privacy.test.ts index 9437c0c39..969377f45 100644 --- a/tests/privacy.test.ts +++ b/tests/privacy.test.ts @@ -1,5 +1,5 @@ import { afterEach, describe, expect, it, vi } from "vitest"; -import { safeErrorLogDetails, safeIngestionJobLog } from "../src/lib/privacy"; +import { safeErrorLogDetails, safeIngestionJobLog, redactCaptionIdentifiers } from "../src/lib/privacy"; afterEach(() => { vi.restoreAllMocks(); @@ -21,6 +21,21 @@ describe("privacy-safe logging helpers", () => { expect(JSON.stringify(details)).not.toContain("source.pdf"); }); + it("redacts modern supabase keys in error messages and details", () => { + const e1 = new Error("found key sb_secret_abcdef1234567890 and sb_publishable_123abcDEF456"); + const d1 = safeErrorLogDetails(e1); + + expect(JSON.stringify(d1)).not.toContain("sb_secret_"); + expect(JSON.stringify(d1)).not.toContain("sb_publishable_"); + expect(JSON.stringify(d1)).toContain("[secret]"); + + const e2 = { message: "connection error", details: `token=${"sb_secret_live_" + "ABCD1234efgh"}` }; + const d2 = safeErrorLogDetails(e2); + + expect(JSON.stringify(d2)).not.toContain("sb_secret_live_"); + expect(JSON.stringify(d2)).toContain("[secret]"); + }); + it("summarizes HTML error responses by title", () => { const error = { message: "<!DOCTYPE html><html><head><title>supabase.co | 522: Connection timed out", @@ -45,6 +60,42 @@ describe("privacy-safe logging helpers", () => { expect(details.stack).not.toContain(""); expect(details.stack).not.toContain("