From 0d47fff01dd18ef507fc6ee9a3e7445fdec15f9d Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 14 Jul 2026 19:05:55 +0000 Subject: [PATCH 1/9] Fix audit findings: catalog rate limits, public-doc redaction, ingestion recovery Addresses the code-level findings from the 2026-07-14 repository audit handover. Security / API: - M4/C1: anonymous curated-catalog GETs (medications, registry, differentials) no longer short-circuit before the limiter. Every caller now resolves access and passes the registry limiter; anonymous requests get a tighter registry allowance. Prevents using the public catalog endpoints as an unauthenticated high-volume egress lever. - S1/D1: authenticated callers can read PUBLIC (owner_id IS NULL) documents they do not own via withOwnerReadScope. The document list and detail routes now redact owner-internal storage fields (storage_path, content_hash, source_path, import_batch_id, error_message) on non-owned rows, and the detail route gates the full owner projection (raw metadata, index health, image storage paths) on ownership rather than mere authentication. - S10/D5: the bulk edit route returned raw per-document DB error text (constraint names, column details). It now logs the sanitized detail server-side and returns a generic per-document failure message. - S11/H6: /api/jobs served unauthenticated demo jobs when the server env was partially missing, masking the misconfiguration. It now fails with a real error. Ingestion: - I1/E1: removed unreachable post-throw code in commitDocumentIndexGeneration and the orphaned client-side deleteStaleIndexGenerationRows fallback. The commit RPC upserts index quality and prunes stale/legacy generation rows atomically server-side; the worker only surfaces failures (fail closed). - I2/E2: buildIngestionRecoveryPlan requeued two jobs for one document when a document had both pending and failed jobs, tripping ingestion_jobs_one_open_per_document_uidx and leaving the queue in a self-perpetuating stall. It now requeues only the first recoverable job per document, supersedes the rest, and orders supersedes before retries so an open sibling is always closed before a retry flips a row to pending. Verification: npm run verify:cheap (lint, typecheck, 2417 tests) passes offline. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01U3gX2PtXiEELGtfL7cmCQb --- src/app/api/differentials/route.ts | 12 +-- src/app/api/documents/[id]/route.ts | 19 +++-- src/app/api/documents/bulk/route.ts | 11 ++- src/app/api/documents/route.ts | 9 +- src/app/api/jobs/route.ts | 9 +- src/app/api/medications/route.ts | 12 +-- src/app/api/registry/records/route.ts | 12 +-- src/lib/api-rate-limit.ts | 5 ++ src/lib/ingestion-recovery.ts | 26 +++++- src/lib/public-api-access.ts | 36 ++++++++ tests/document-mutation-routes.test.ts | 35 ++++++++ tests/ingestion-recovery.test.ts | 32 +++++++ tests/medications-route.test.ts | 15 ++++ tests/private-access-routes.test.ts | 110 ++++++++++++++++++++++++- tests/registry-records-route.test.ts | 16 +++- tests/worker-visual-capture.test.ts | 7 +- worker/main.ts | 84 +------------------ 17 files changed, 321 insertions(+), 129 deletions(-) diff --git a/src/app/api/differentials/route.ts b/src/app/api/differentials/route.ts index 28361fec2..256d6a187 100644 --- a/src/app/api/differentials/route.ts +++ b/src/app/api/differentials/route.ts @@ -23,7 +23,7 @@ import { } from "@/lib/differentials"; import { isDemoMode, isLocalNoAuthMode } from "@/lib/env"; import { jsonError } from "@/lib/http"; -import { publicAccessContext, shouldResolvePublicCatalogAccess } from "@/lib/public-api-access"; +import { publicAccessContext } from "@/lib/public-api-access"; import { createAdminClient } from "@/lib/supabase/admin"; import { AuthenticationError, unauthorizedResponse } from "@/lib/supabase/auth"; import { parseRequestQuery, queryInteger } from "@/lib/validation/query"; @@ -88,13 +88,9 @@ export async function GET(request: Request) { }); } - if (!shouldResolvePublicCatalogAccess(request)) { - return differentialResponse({ - ...publicDifferentialPayload(kind, q, limit), - publicAccess: true, - }); - } - + // Anonymous callers still resolve access + rate limit: publicAccessContext skips the + // Supabase auth round-trip for requests with no session cookie/bearer, but every caller + // (authenticated or not) must pass the registry limiter before we serve the full catalog. const supabase = createAdminClient(); const access = await publicAccessContext(request, supabase); diff --git a/src/app/api/documents/[id]/route.ts b/src/app/api/documents/[id]/route.ts index 35bc57b16..0309e91d9 100644 --- a/src/app/api/documents/[id]/route.ts +++ b/src/app/api/documents/[id]/route.ts @@ -10,7 +10,7 @@ import { invalidateRagCachesForDocumentMutation } from "@/lib/rag"; import { committedIndexGeneration, isCommittedGenerationMetadata } from "@/lib/reindex-pipeline"; import { createAdminClient } from "@/lib/supabase/admin"; import { AuthenticationError, requireAuthenticatedUser, unauthorizedResponse } from "@/lib/supabase/auth"; -import { enforceDocumentReadRateLimit, withOwnerReadScope } from "@/lib/public-api-access"; +import { callerOwnsDocumentRow, enforceDocumentReadRateLimit, withOwnerReadScope } from "@/lib/public-api-access"; import { writeAuditLog } from "@/lib/audit"; import { parseJsonBody } from "@/lib/validation/body"; import { parseRouteParams } from "@/lib/validation/params"; @@ -296,6 +296,13 @@ export async function GET(request: Request, { params }: { params: Promise<{ id: if (error) throw new Error(error.message); if (!document) return NextResponse.json({ error: "Document not found." }, { status: 404 }); + // withOwnerReadScope also returns PUBLIC (owner_id IS NULL) documents to an authenticated + // caller. The owner-only projection (raw metadata, storage_path/content_hash, index health, + // image storage paths) must be gated on OWNERSHIP, not merely on being authenticated, so an + // authed non-owner viewing a shared public document gets the same redacted view as an + // anonymous caller (S1/D1). + const isOwner = callerOwnsDocumentRow(document, access.ownerId); + const chunkId = detailQuery.chunk ?? null; const requestedPage = Math.min(detailQuery.page, Math.max(1, document.page_count ?? 1)); const pageLimit = detailQuery.pageLimit; @@ -394,17 +401,15 @@ export async function GET(request: Request, { params }: { params: Promise<{ id: return Object.fromEntries(Object.entries(row).filter(([key]) => !internalKeys.has(key))); }; const publicRows = >(rows: T[]) => - access.authenticated ? rows : rows.map(omitPublicInternalFields); - const responseDocument = access.authenticated - ? document - : omitPublicInternalFields(document as Record); + isOwner ? rows : rows.map(omitPublicInternalFields); + const responseDocument = isOwner ? document : omitPublicInternalFields(document as Record); return NextResponse.json({ document: { ...responseDocument, labels: publicRows((labelsResult.data ?? []) as Record[]), summary: - access.authenticated || !summaryResult.data + isOwner || !summaryResult.data ? (summaryResult.data ?? null) : omitPublicInternalFields(summaryResult.data as Record), }, @@ -430,7 +435,7 @@ export async function GET(request: Request, { params }: { params: Promise<{ id: hasAfter: Boolean(document.chunk_count && chunkRangeEnd + 1 < document.chunk_count), selectedChunkId: selectedChunk?.id ?? null, }, - ...(access.authenticated + ...(isOwner ? { indexHealth: { extractionQuality: safeMetadata(document.metadata).extraction_quality ?? null, diff --git a/src/app/api/documents/bulk/route.ts b/src/app/api/documents/bulk/route.ts index dcece4aa6..0d04d269e 100644 --- a/src/app/api/documents/bulk/route.ts +++ b/src/app/api/documents/bulk/route.ts @@ -3,6 +3,8 @@ import { z } from "zod"; import { normalizeDocumentLabelForStorage } from "@/lib/document-tags"; import { isDemoMode } from "@/lib/env"; import { jsonError, PublicApiError } from "@/lib/http"; +import { logger } from "@/lib/logger"; +import { safeErrorLogDetails } from "@/lib/privacy"; import { invalidateRagCachesForOwner } from "@/lib/rag"; import { createAdminClient } from "@/lib/supabase/admin"; import type { Json, TablesUpdate } from "@/lib/supabase/database.types"; @@ -207,10 +209,17 @@ export async function POST(request: Request) { if (updateError) throw new Error(updateError.message); results.push({ documentId: document.id, updated: true }); } catch (error) { + // Never surface the raw DB error text (constraint names, column details) to the client; + // log the sanitized detail server-side and return a generic per-document failure (S10/D5). + logger.error("Bulk document edit failed for a document", { + ownerId: user.id, + documentId: document.id, + ...safeErrorLogDetails(error), + }); results.push({ documentId: document.id, updated: false, - error: error instanceof Error ? error.message : "Bulk edit failed.", + error: "Bulk edit failed for this document.", }); } } diff --git a/src/app/api/documents/route.ts b/src/app/api/documents/route.ts index 27c38c423..fdbc48a6f 100644 --- a/src/app/api/documents/route.ts +++ b/src/app/api/documents/route.ts @@ -6,7 +6,7 @@ import { isDemoMode } from "@/lib/env"; import { jsonError } from "@/lib/http"; import { createAdminClient } from "@/lib/supabase/admin"; import { AuthenticationError, unauthorizedResponse } from "@/lib/supabase/auth"; -import { enforceDocumentReadRateLimit, withOwnerReadScope } from "@/lib/public-api-access"; +import { enforceDocumentReadRateLimit, redactNonOwnedDocumentFields, withOwnerReadScope } from "@/lib/public-api-access"; import { parseRequestQuery, queryBoolean, queryInteger } from "@/lib/validation/query"; export const runtime = "nodejs"; @@ -181,7 +181,12 @@ export async function GET(request: Request) { const { data, error, count } = await query; if (error) throw new Error(error.message); - const documents = (data ?? []) as unknown as DocumentListRow[]; + // An authenticated caller reads PUBLIC (owner_id IS NULL) documents alongside their own via + // withOwnerReadScope. Redact operator-internal storage fields on the rows they do not own so a + // shared public document never exposes its owner's storage_path/content_hash/etc. (S1/D1). + const documents = ((data ?? []) as unknown as DocumentListRow[]).map((document) => + redactNonOwnedDocumentFields(document, access.ownerId), + ); const documentIds = documents.map((document) => document.id); const indexing = indexingState(documents); diff --git a/src/app/api/jobs/route.ts b/src/app/api/jobs/route.ts index a1f39092f..2601c061d 100644 --- a/src/app/api/jobs/route.ts +++ b/src/app/api/jobs/route.ts @@ -87,12 +87,9 @@ export async function GET(request: Request) { if (error instanceof AuthenticationError) { return unauthorizedResponse(); } - if (error instanceof Error && error.message.includes("Missing server environment")) { - return jobsResponse(demoJobs, { - demoMode: true, - error: "Server environment is not configured; demo jobs are being served.", - }); - } + // A partially-configured production (isDemoMode() false but createAdminClient throws + // "Missing server environment") must surface as a real error rather than silently serving + // unauthenticated demo jobs, which masked the misconfiguration and returned fake data (S11/H6). return jsonError(error); } } diff --git a/src/app/api/medications/route.ts b/src/app/api/medications/route.ts index cb996d8fa..adb6e9743 100644 --- a/src/app/api/medications/route.ts +++ b/src/app/api/medications/route.ts @@ -21,7 +21,7 @@ import { type MedicationRecord, type MedicationSearchMatch, } from "@/lib/medications"; -import { publicAccessContext, shouldResolvePublicCatalogAccess } from "@/lib/public-api-access"; +import { publicAccessContext } from "@/lib/public-api-access"; import { createAdminClient } from "@/lib/supabase/admin"; import { AuthenticationError, unauthorizedResponse } from "@/lib/supabase/auth"; import { parseRequestQuery, queryInteger } from "@/lib/validation/query"; @@ -105,13 +105,9 @@ export async function GET(request: Request) { }); } - if (!shouldResolvePublicCatalogAccess(request)) { - return medicationResponse({ - ...publicMedicationPayload(q, limit, fields), - publicAccess: true, - }); - } - + // Anonymous callers still resolve access + rate limit: publicAccessContext skips the + // Supabase auth round-trip for requests with no session cookie/bearer, but every caller + // (authenticated or not) must pass the registry limiter before we serve the full catalog. const supabase = createAdminClient(); const access = await publicAccessContext(request, supabase); diff --git a/src/app/api/registry/records/route.ts b/src/app/api/registry/records/route.ts index 310c3c055..552d0b42b 100644 --- a/src/app/api/registry/records/route.ts +++ b/src/app/api/registry/records/route.ts @@ -8,7 +8,7 @@ import { } from "@/lib/api-rate-limit"; import { isDemoMode, isLocalNoAuthMode } from "@/lib/env"; import { jsonError } from "@/lib/http"; -import { publicAccessContext, shouldResolvePublicCatalogAccess } from "@/lib/public-api-access"; +import { publicAccessContext } from "@/lib/public-api-access"; import { rankFormRecords, formRecords } from "@/lib/forms"; import { deriveGovernanceColumns, @@ -81,13 +81,9 @@ export async function GET(request: Request) { }); } - if (!shouldResolvePublicCatalogAccess(request)) { - return registryResponse({ - ...publicRegistryPayload(kind, q, limit), - publicAccess: true, - }); - } - + // Anonymous callers still resolve access + rate limit: publicAccessContext skips the + // Supabase auth round-trip for requests with no session cookie/bearer, but every caller + // (authenticated or not) must pass the registry limiter before we serve the full catalog. const supabase = createAdminClient(); const access = await publicAccessContext(request, supabase); diff --git a/src/lib/api-rate-limit.ts b/src/lib/api-rate-limit.ts index 06ecfe7f0..04b7472f2 100644 --- a/src/lib/api-rate-limit.ts +++ b/src/lib/api-rate-limit.ts @@ -68,6 +68,11 @@ const anonymousApiRateLimitDefaults: Partial; diff --git a/src/lib/ingestion-recovery.ts b/src/lib/ingestion-recovery.ts index 13eb99142..180d7ced8 100644 --- a/src/lib/ingestion-recovery.ts +++ b/src/lib/ingestion-recovery.ts @@ -74,15 +74,35 @@ export function buildIngestionRecoveryPlan(args: { } if (isRecoverableStatus) { + // Audit I2/E2: `ingestion_jobs_one_open_per_document_uidx` forbids two OPEN + // (pending/processing) rows for one document, but a document can legitimately arrive here + // with several recoverable jobs at once (e.g. a `failed` row beside a `pending` row). + // Requeue only the FIRST recoverable job per document; supersede every additional one. + // Without this, recovery emitted two `retry` actions and tried to set both rows to + // `pending`, tripping the unique index (23505) and leaving the queue in a self-perpetuating + // stall that re-crashed on every subsequent run. + if (resetDocuments.has(job.document_id)) { + actions.push({ action: "supersede", jobId: job.id, documentId: job.document_id }); + continue; + } resetDocuments.add(job.document_id); actions.push({ action: "retry", jobId: job.id, documentId: job.document_id, resetDocument: true }); } } + // Apply supersedes before retries. A retry flips a row to `pending`; if a redundant sibling for + // the same document is still open when that happens, the two rows collide on the partial unique + // index. Both consumers apply `actions` in array order, so closing the siblings first here keeps + // recovery crash-safe regardless of the order jobs were fetched in. (Audit I2/E2) + const orderedActions = [ + ...actions.filter((action) => action.action === "supersede"), + ...actions.filter((action) => action.action === "retry"), + ]; + return { - actions, + actions: orderedActions, resetDocumentIds: Array.from(resetDocuments), - supersedeCount: actions.filter((action) => action.action === "supersede").length, - retryCount: actions.filter((action) => action.action === "retry").length, + supersedeCount: orderedActions.filter((action) => action.action === "supersede").length, + retryCount: orderedActions.filter((action) => action.action === "retry").length, }; } diff --git a/src/lib/public-api-access.ts b/src/lib/public-api-access.ts index b17007cad..a228404d2 100644 --- a/src/lib/public-api-access.ts +++ b/src/lib/public-api-access.ts @@ -96,6 +96,42 @@ export function withOwnerReadScope>(query: T, owne return query.is("owner_id", null); } +// Owner-internal document columns that must never be exposed on a row the caller does not own. +// `withOwnerReadScope` lets an authenticated caller read PUBLIC documents (owner_id IS NULL) that +// belong to nobody; those rows must not leak the storage location, dedup hash, import provenance, +// or raw stage error of the operator who ingested them. Shared governance `metadata` is preserved +// so public-corpus badges keep rendering — only the operator-internal storage fields are stripped. +const NON_OWNER_INTERNAL_DOCUMENT_FIELDS = [ + "storage_path", + "content_hash", + "source_path", + "import_batch_id", + "error_message", +] as const; + +/** True when `viewerOwnerId` is set and owns the row (i.e. the caller's own document). */ +export function callerOwnsDocumentRow(row: { owner_id?: unknown }, viewerOwnerId: string | undefined): boolean { + return Boolean(viewerOwnerId) && row.owner_id === viewerOwnerId; +} + +/** + * Strip operator-internal storage fields from a document row unless the caller owns it. + * + * No-op for owned rows and for rows already selected without those columns (e.g. the anonymous + * public projection), so it is safe to map over every returned row regardless of caller identity. + */ +export function redactNonOwnedDocumentFields>( + row: T, + viewerOwnerId: string | undefined, +): T { + if (callerOwnsDocumentRow(row, viewerOwnerId)) return row; + const redacted = { ...row }; + for (const field of NON_OWNER_INTERNAL_DOCUMENT_FIELDS) { + delete redacted[field]; + } + return redacted; +} + export async function publicAccessContext(request: Request, supabase: AdminClient) { const user = await getOptionalAuthenticatedUser(request, supabase); if (user) { diff --git a/tests/document-mutation-routes.test.ts b/tests/document-mutation-routes.test.ts index 8cf92b404..e3d1703fd 100644 --- a/tests/document-mutation-routes.test.ts +++ b/tests/document-mutation-routes.test.ts @@ -197,6 +197,41 @@ describe("/api/documents/bulk", () => { ); expect(invalidateRagCachesForOwner).toHaveBeenCalledWith(ownerId); }); + + it("does not leak the raw database error in a per-document bulk failure", async () => { + const rawDbError = 'duplicate key value violates unique constraint "documents_secret_internal_idx"'; + const supabase = createSupabaseMock((call) => { + if (call.table === "documents" && call.operation === "select") { + return { data: [{ id: documentId, title: "Old title", metadata: {} }], error: null }; + } + if (call.table === "documents" && call.operation === "update") { + return { data: null, error: { message: rawDbError } }; + } + return { data: null, error: null }; + }); + mockRouteRuntime(supabase.client); + const { POST } = await import("../src/app/api/documents/bulk/route"); + + const response = await POST( + new Request("http://localhost/api/documents/bulk", { + method: "POST", + headers: { "content-type": "application/json" }, + body: JSON.stringify({ documentIds: [documentId], metadata: { publisher: "WA Health" } }), + }), + ); + const payload = (await response.json()) as { + ok: boolean; + updatedCount: number; + results: Array<{ documentId: string; updated: boolean; error?: string }>; + }; + + expect(response.status).toBe(200); + expect(payload).toMatchObject({ ok: false, updatedCount: 0 }); + expect(payload.results[0]).toMatchObject({ documentId, updated: false, error: "Bulk edit failed for this document." }); + // The raw DB constraint text must never reach the client. + expect(JSON.stringify(payload)).not.toContain("documents_secret_internal_idx"); + expect(JSON.stringify(payload)).not.toContain("unique constraint"); + }); }); describe("/api/documents/[id]/table-facts", () => { diff --git a/tests/ingestion-recovery.test.ts b/tests/ingestion-recovery.test.ts index cd2976ed6..5d7cc8c67 100644 --- a/tests/ingestion-recovery.test.ts +++ b/tests/ingestion-recovery.test.ts @@ -92,6 +92,38 @@ describe("ingestion queue recovery planning", () => { expect(plan.retryCount).toBe(0); }); + it("requeues only one job and supersedes the sibling when a document has both pending and failed jobs (I2)", () => { + const plan = buildIngestionRecoveryPlan({ + now, + staleAfterMinutes: 45, + jobs: [ + // The older `failed` row is iterated first; the still-open `pending` sibling must not be + // flipped to a second `pending` row (which would collide on the partial unique index). + { + id: "failed-first", + document_id: "doc-double", + status: "failed", + documents: { status: "failed", chunk_count: 0 }, + }, + { + id: "pending-second", + document_id: "doc-double", + status: "pending", + documents: { status: "queued", chunk_count: 0 }, + }, + ], + }); + + expect(plan.retryCount).toBe(1); + expect(plan.supersedeCount).toBe(1); + expect(plan.resetDocumentIds).toEqual(["doc-double"]); + // Supersede must be applied before the retry so the open sibling is closed first. + expect(plan.actions[0]).toMatchObject({ action: "supersede", documentId: "doc-double" }); + expect(plan.actions[1]).toMatchObject({ action: "retry", documentId: "doc-double" }); + // Exactly one job is requeued to `pending`; the other is closed. + expect(plan.actions.filter((action) => action.action === "retry")).toHaveLength(1); + }); + it("does not reclaim fresh processing jobs", () => { expect( isStaleProcessingJob( diff --git a/tests/medications-route.test.ts b/tests/medications-route.test.ts index 14ee3b5bd..36179a42f 100644 --- a/tests/medications-route.test.ts +++ b/tests/medications-route.test.ts @@ -227,6 +227,21 @@ describe("medications API", () => { expect(payload.publicAccess).toBe(true); expect(payload.records.some((record) => record.slug === "acamprosate")).toBe(true); expect(payload.matches?.[0]?.medication.slug).toBe("acamprosate"); + // The catalog is served from seed data (no table read) and no auth round-trip is needed, + // but anonymous list requests must still pass the registry limiter (M4/C1). + expect(client.from).not.toHaveBeenCalled(); + expect(client.rpc).toHaveBeenCalled(); + expect(client.auth.getUser).not.toHaveBeenCalled(); + }); + + it("rate-limits anonymous list requests (429) without falling back to unlimited access", async () => { + const client = createSupabaseMock(() => ok([]), { limited: true }); + mockRuntime(client); + const { GET } = await import("../src/app/api/medications/route"); + + const response = await GET(request("/api/medications")); + + expect(response.status).toBe(429); expect(client.from).not.toHaveBeenCalled(); expect(client.auth.getUser).not.toHaveBeenCalled(); }); diff --git a/tests/private-access-routes.test.ts b/tests/private-access-routes.test.ts index f58484873..54b78bae8 100644 --- a/tests/private-access-routes.test.ts +++ b/tests/private-access-routes.test.ts @@ -527,6 +527,53 @@ describe("private document API access", () => { expect(client.calls[0].range).toEqual({ from: 0, to: 99 }); }); + it("redacts owner-internal storage fields on public rows in an authenticated document list", async () => { + const owned = { + id: documentId, + owner_id: userId, + title: "Owned document", + status: "indexed", + storage_path: `${userId}/documents/owned.pdf`, + content_hash: "sha256:owned", + metadata: { extraction_quality: "good" }, + }; + const shared = { + id: otherDocumentId, + owner_id: null, + title: "Public guideline", + status: "indexed", + storage_path: "someone-else/documents/shared.pdf", + content_hash: "sha256:shared", + source_path: "/import/shared.pdf", + import_batch_id: "batch-1", + error_message: "internal stage error", + metadata: { extraction_quality: "partial" }, + }; + const client = createSupabaseMock((call) => (call.table === "documents" ? ok([owned, shared]) : ok([]))); + mockRuntime(client); + const { GET } = await import("../src/app/api/documents/route"); + + const response = await GET(authenticatedRequest("/api/documents?includeMeta=false")); + const body = await payload(response); + const [ownedRow, sharedRow] = body.documents as Array>; + + expect(response.status).toBe(200); + // The caller's own row keeps its storage internals. + expect(ownedRow).toMatchObject({ + id: documentId, + storage_path: `${userId}/documents/owned.pdf`, + content_hash: "sha256:owned", + }); + // The shared public row keeps its shared governance metadata but not the owner's storage internals. + expect(sharedRow).toMatchObject({ id: otherDocumentId, title: "Public guideline" }); + expect(sharedRow.metadata).toEqual({ extraction_quality: "partial" }); + expect(sharedRow).not.toHaveProperty("storage_path"); + expect(sharedRow).not.toHaveProperty("content_hash"); + expect(sharedRow).not.toHaveProperty("source_path"); + expect(sharedRow).not.toHaveProperty("import_batch_id"); + expect(sharedRow).not.toHaveProperty("error_message"); + }); + it("accepts legacy Supabase auth cookies for private document access", async () => { const documents = [{ id: documentId, owner_id: userId, title: "Owned document" }]; const client = createSupabaseMock((call) => (call.table === "documents" ? ok(documents) : ok([]))); @@ -557,7 +604,7 @@ describe("private document API access", () => { expect(client.calls[0].orFilters).toContain(`owner_id.eq.${userId},owner_id.is.null`); }); - it("allows authenticated users to read public document detail", async () => { + it("redacts owner-internal fields when an authenticated user reads a public document they do not own", async () => { const client = createSupabaseMock((call) => { if (call.table === "documents" && matchesOwnerReadScope(call, userId)) { return ok({ @@ -568,7 +615,12 @@ describe("private document API access", () => { file_type: "application/pdf", page_count: 2, chunk_count: 1, - metadata: { index_generation_id: "generation-a" }, + storage_path: "someone-else/documents/guideline.pdf", + content_hash: "sha256:leaky-hash", + source_path: "/import/guideline.pdf", + import_batch_id: "batch-99", + error_message: "internal stage error", + metadata: { index_generation_id: "generation-a", extraction_quality: "good" }, }); } if (call.table === "document_pages") return ok([]); @@ -584,10 +636,62 @@ describe("private document API access", () => { params: Promise.resolve({ id: documentId }), }); const body = await payload(response); + const document = body.document as Record; expect(response.status).toBe(200); - expect(body.document).toMatchObject({ id: documentId, title: "Public guideline", owner_id: null }); + // The caller can still read the shared public document... + expect(document).toMatchObject({ id: documentId, title: "Public guideline" }); expect(client.calls[0].orFilters).toContain(`owner_id.eq.${userId},owner_id.is.null`); + // ...but not the owner's storage location, dedup hash, import provenance, raw error, metadata, + // or index-health diagnostics — an authed non-owner gets the same redacted view as anonymous. + expect(document).not.toHaveProperty("storage_path"); + expect(document).not.toHaveProperty("content_hash"); + expect(document).not.toHaveProperty("source_path"); + expect(document).not.toHaveProperty("import_batch_id"); + expect(document).not.toHaveProperty("error_message"); + expect(document).not.toHaveProperty("owner_id"); + expect(document).not.toHaveProperty("metadata"); + expect(body).not.toHaveProperty("indexHealth"); + }); + + it("returns full owner-internal fields when the authenticated caller owns the document", async () => { + const client = createSupabaseMock((call) => { + if (call.table === "documents" && matchesOwnerReadScope(call, userId)) { + return ok({ + id: documentId, + owner_id: userId, + title: "Owned guideline", + file_name: "guideline.pdf", + file_type: "application/pdf", + page_count: 2, + chunk_count: 1, + storage_path: `${userId}/documents/guideline.pdf`, + content_hash: "sha256:owned-hash", + metadata: { index_generation_id: "generation-a", extraction_quality: "good" }, + }); + } + if (["document_pages", "document_images", "document_chunks", "document_table_facts"].includes(call.table)) { + return ok([]); + } + return ok([]); + }); + mockRuntime(client); + const { GET } = await import("../src/app/api/documents/[id]/route"); + + const response = await GET(authenticatedRequest(`/api/documents/${documentId}`), { + params: Promise.resolve({ id: documentId }), + }); + const body = await payload(response); + const document = body.document as Record; + + expect(response.status).toBe(200); + expect(document).toMatchObject({ + id: documentId, + owner_id: userId, + storage_path: `${userId}/documents/guideline.pdf`, + content_hash: "sha256:owned-hash", + }); + expect(body).toHaveProperty("indexHealth"); }); it("allows anonymous users to read public document detail", async () => { diff --git a/tests/registry-records-route.test.ts b/tests/registry-records-route.test.ts index ab95c3680..22e437e2a 100644 --- a/tests/registry-records-route.test.ts +++ b/tests/registry-records-route.test.ts @@ -213,8 +213,22 @@ describe("registry records API", () => { expect(payload.publicAccess).toBe(true); expect(payload.records.some((record) => record.slug === "13yarn")).toBe(true); expect(payload.matches?.[0]?.record.slug).toBe("13yarn"); + // The full catalog is served from seed data (no table read) and no auth round-trip is + // needed, but anonymous list requests must still pass the registry limiter (M4/C1). + expect(client.from).not.toHaveBeenCalled(); + expect(client.rpc).toHaveBeenCalled(); + expect(client.auth.getUser).not.toHaveBeenCalled(); + }); + + it("rate-limits anonymous list requests (429) without falling back to unlimited access", async () => { + const client = createSupabaseMock(() => ok([]), { limited: true }); + mockRuntime(client); + const { GET } = await import("../src/app/api/registry/records/route"); + + const response = await GET(request("/api/registry/records?kind=service")); + + expect(response.status).toBe(429); expect(client.from).not.toHaveBeenCalled(); - expect(client.rpc).not.toHaveBeenCalled(); expect(client.auth.getUser).not.toHaveBeenCalled(); }); diff --git a/tests/worker-visual-capture.test.ts b/tests/worker-visual-capture.test.ts index 91147f06d..d74793a19 100644 --- a/tests/worker-visual-capture.test.ts +++ b/tests/worker-visual-capture.test.ts @@ -40,8 +40,11 @@ describe("worker visual capture hardening", () => { expect(workerSource).toContain("p_job_id: args.jobId"); expect(workerSource).toContain("p_worker_id: workerId"); expect(workerSource).not.toContain("await replacePageRows(args.documentId, args.pages)"); - expect(workerSource).toContain("await deleteStaleIndexGenerationRows(args.documentId, args.indexGenerationId)"); - expect(workerSource).toContain("async function deleteStaleIndexGenerationRows"); + // The commit RPC prunes stale/legacy generation rows and upserts index quality atomically + // server-side; the worker must surface any RPC failure (fail closed) rather than run a + // client-side fallback that could partially write after an error. + expect(workerSource).toContain('if (error) throw supabaseStageError("commit_document_index_generation", error)'); + expect(workerSource).not.toContain("deleteStaleIndexGenerationRows"); expect(workerSource).toContain("`${imagePrefix}/${indexGenerationId}/image-${index + 1}${ext}`"); expect(workerSource).toContain('indexing_v3_agent_repair_reason: "core_index_committed"'); expect(workerSource).toContain("indexing_v3_agent_repair_reason: null"); diff --git a/worker/main.ts b/worker/main.ts index 837800fa5..f0027f40a 100644 --- a/worker/main.ts +++ b/worker/main.ts @@ -89,23 +89,6 @@ type OptionalIndexWriteIssue = { code?: string | null; }; -type GenerationTableResult = { - data: unknown[] | null; - error: { message?: string; code?: string; details?: string; hint?: string } | null; -}; - -type GenerationTableFilter = PromiseLike & { - eq: (column: string, value: string) => GenerationTableFilter; - neq: (column: string, value: string) => PromiseLike; - is: (column: string, value: null) => PromiseLike; - limit: (count: number) => GenerationTableFilter; -}; - -type GenerationTableQuery = { - select: (columns: string) => GenerationTableFilter; - delete: () => GenerationTableFilter; -}; - function supabaseStageError( stage: string, error: { message?: string; code?: string; details?: string; hint?: string }, @@ -541,10 +524,10 @@ async function commitDocumentIndexGeneration(args: { })), p_quality: sanitizeJsonbRecord(args.quality), }); - if (!error) return; - throw supabaseStageError("commit_document_index_generation", error); - await upsertIndexQuality(args.quality); - await deleteStaleIndexGenerationRows(args.documentId, args.indexGenerationId); + // The RPC upserts index quality and prunes stale/legacy generation rows atomically server-side + // (20260702000000_commit_generation_preserve_legacy_artifacts, lease-fenced by + // 20260713062125_fence_index_generation_commit), so the worker only needs to surface failures. + if (error) throw supabaseStageError("commit_document_index_generation", error); } function buildDocumentPageRows(documentId: string, extracted: ExtractedDocument) { @@ -557,65 +540,6 @@ function buildDocumentPageRows(documentId: string, extracted: ExtractedDocument) })); } -// Audit M13 (mirrors 20260702000000_commit_generation_preserve_legacy_artifacts): -// this client-side fallback (used only when the commit RPC is missing) must -// apply the same preservation rule as the RPC — legacy generationless rows are -// purged only when this generation wrote replacement rows into the same -// table, so a transient artifact-write failure cannot destroy the -// previously-good legacy artifacts. Rows tagged with a DIFFERENT generation -// are always removed. Note: chunk-anchored artifacts (table facts, embedding -// fields, index units) cascade with their legacy chunks regardless — the -// guarantee fully protects images, memory cards, and sections. -async function deleteStaleIndexGenerationRows(documentId: string, indexGenerationId: string) { - // The generated Supabase client only types known table literals. This cleanup - // path selects among a small runtime-known set of tables, so use a minimal - // adapter at the dynamic boundary instead of widening the whole admin client. - const fromGenerationTable = supabase.from.bind(supabase) as unknown as (table: string) => GenerationTableQuery; - const hasReplacementRows = async (table: string, direct: boolean) => { - let query = fromGenerationTable(table).select("id").eq("document_id", documentId).limit(1); - query = direct - ? query.eq("index_generation_id", indexGenerationId) - : query.eq("metadata->>index_generation_id", indexGenerationId); - const { data, error } = await query; - if (error) throw supabaseStageError(`check replacement ${table}`, error); - return (data ?? []).length > 0; - }; - const deleteDirectGenerationRows = async (table: string) => { - const stale = await fromGenerationTable(table) - .delete() - .eq("document_id", documentId) - .neq("index_generation_id", indexGenerationId); - if (stale.error) throw supabaseStageError(`delete stale ${table}`, stale.error); - if (!(await hasReplacementRows(table, true))) return; - const missing = await fromGenerationTable(table) - .delete() - .eq("document_id", documentId) - .is("index_generation_id", null); - if (missing.error) throw supabaseStageError(`delete generationless ${table}`, missing.error); - }; - const deleteMetadataGenerationRows = async (table: string) => { - const stale = await fromGenerationTable(table) - .delete() - .eq("document_id", documentId) - .neq("metadata->>index_generation_id", indexGenerationId); - if (stale.error) throw supabaseStageError(`delete stale ${table}`, stale.error); - if (!(await hasReplacementRows(table, false))) return; - const missing = await fromGenerationTable(table) - .delete() - .eq("document_id", documentId) - .is("metadata->>index_generation_id", null); - if (missing.error) throw supabaseStageError(`delete generationless ${table}`, missing.error); - }; - - await deleteDirectGenerationRows("document_chunks"); - await deleteMetadataGenerationRows("document_images"); - await deleteMetadataGenerationRows("document_table_facts"); - await deleteMetadataGenerationRows("document_embedding_fields"); - await deleteMetadataGenerationRows("document_index_units"); - await deleteMetadataGenerationRows("document_memory_cards"); - await deleteMetadataGenerationRows("document_sections"); -} - async function upsertIndexQuality(quality: ReturnType) { const { error } = await supabase .from("document_index_quality") From e686048d4a3214176843910e809eb0f362069d86 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 14 Jul 2026 19:07:36 +0000 Subject: [PATCH 2/9] style: apply prettier formatting to audit-fix changes Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01U3gX2PtXiEELGtfL7cmCQb --- src/app/api/documents/route.ts | 6 +++++- tests/document-mutation-routes.test.ts | 6 +++++- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/src/app/api/documents/route.ts b/src/app/api/documents/route.ts index fdbc48a6f..6c8c4bf2d 100644 --- a/src/app/api/documents/route.ts +++ b/src/app/api/documents/route.ts @@ -6,7 +6,11 @@ import { isDemoMode } from "@/lib/env"; import { jsonError } from "@/lib/http"; import { createAdminClient } from "@/lib/supabase/admin"; import { AuthenticationError, unauthorizedResponse } from "@/lib/supabase/auth"; -import { enforceDocumentReadRateLimit, redactNonOwnedDocumentFields, withOwnerReadScope } from "@/lib/public-api-access"; +import { + enforceDocumentReadRateLimit, + redactNonOwnedDocumentFields, + withOwnerReadScope, +} from "@/lib/public-api-access"; import { parseRequestQuery, queryBoolean, queryInteger } from "@/lib/validation/query"; export const runtime = "nodejs"; diff --git a/tests/document-mutation-routes.test.ts b/tests/document-mutation-routes.test.ts index e3d1703fd..d3af3afd6 100644 --- a/tests/document-mutation-routes.test.ts +++ b/tests/document-mutation-routes.test.ts @@ -227,7 +227,11 @@ describe("/api/documents/bulk", () => { expect(response.status).toBe(200); expect(payload).toMatchObject({ ok: false, updatedCount: 0 }); - expect(payload.results[0]).toMatchObject({ documentId, updated: false, error: "Bulk edit failed for this document." }); + expect(payload.results[0]).toMatchObject({ + documentId, + updated: false, + error: "Bulk edit failed for this document.", + }); // The raw DB constraint text must never reach the client. expect(JSON.stringify(payload)).not.toContain("documents_secret_internal_idx"); expect(JSON.stringify(payload)).not.toContain("unique constraint"); From aa161b79ec613ea887f528811a68fad95edc852c Mon Sep 17 00:00:00 2001 From: BigSimmo <87357024+BigSimmo@users.noreply.github.com> Date: Wed, 15 Jul 2026 04:13:27 +0800 Subject: [PATCH 3/9] fix: close public metadata and recovery gaps --- docs/branch-review-ledger.md | 1 + scripts/recover-ingestion-queue.ts | 4 +- scripts/reindex.ts | 7 ++- src/app/api/documents/route.ts | 65 +++++++++++++++++++++------ src/lib/ingestion-recovery.ts | 70 ++++++++++++++++------------- tests/ingestion-recovery.test.ts | 47 ++++++++++++++----- tests/private-access-routes.test.ts | 66 +++++++++++++++++++++++++++ 7 files changed, 202 insertions(+), 58 deletions(-) diff --git a/docs/branch-review-ledger.md b/docs/branch-review-ledger.md index 03f8ed843..991ae06cf 100644 --- a/docs/branch-review-ledger.md +++ b/docs/branch-review-ledger.md @@ -491,3 +491,4 @@ Use this ledger to prevent repeated branch and PR reviews when the reviewed HEAD | 2026-07-14 | PR #655 / codex/release-blocker-remediation | a3f3a89676015cd5f018c07e8c3ad9483f91cef6 + reviewed follow-up diff | offline adversarial-latency follow-up | The final blocking offline-quality failure was an adversarial secret-exfiltration query that correctly refused but first spent about 25 seconds in lexical retrieval. Adversarial manipulation now short-circuits at the search boundary before provider-client creation, cache access, classification, aliases, or Supabase work, and is never cached. | Focused Vitest 2/2; scoped ESLint; Prettier; full TypeScript; `git diff --check`; live provider-free adversarial case completed in 100 ms with 0 ms RPC time; `eval:quality:release:offline` passed with zero blocking failures and zero model, request-ID, token, cost, or generation-latency evidence. Flaky local browser/composite suites intentionally not repeated; hosted CI remains authoritative. | | 2026-07-14 | PR #666 / codex/release-blocker-remediation | 9b56eebe4b23ab783207445fb827c317c8d59be8 + reviewed follow-up diff | review-followup | One late P2 retrieval-contract gap was confirmed: the optimized agitation query retained IM/PO but could drop other already-supported amount, route, and frequency aliases. Medication evidence intent is now shared with retrieval selection, and focused agitation queries preserve requested numeric units, SC, SL, PRN, and frequency signals without restoring the broad ten-term expansion. | GitHub review-thread inspection; focused clinical-search/retrieval Vitest 112/112; scoped ESLint; Prettier; `git diff --check`. Hosted final-head TypeScript/build/CI and exact-head staging evidence remain required after push. | | 2026-07-14 | codex/formulation-specifier-separation-current | 25d7f733f + uncommitted fix | Specifiers/Formulation boundary bug hunt and remediation | Confirmed two P1 clinical-navigation/content boundary defects: the completed Formulation change deleted the distinct psychiatric Specifiers workspace and redirected every `/specifiers/*` route into Formulation; a later universal-search contract also labelled Formulation mechanism results as `specifiers`. Restored the independent Specifiers catalogue, detail, builder, compare, and map surfaces; registered both app modes and action sets; kept submitted searches in their owning standalone workspace; and split universal search into real `specifiers` and `formulation` domains with correct records, headings, mode ownership, and destinations. | Offline only: full Vitest 2,404 passed/1 skipped, TypeScript, full ESLint, focused Vitest 56/56, sitemap check, `git diff --check`, production-readiness READY with expected missing-env warnings, identity-checked server at `http://localhost:3384`, and repaired Chromium journeys for Specifiers desktop/mobile, Formulation mobile, and explicit route-family separation all passed. No provider-backed or live API checks run. | +| 2026-07-15 | PR #680 / claude/rag-scalability-review-x0s55l | e686048d4a3214176843910e809eb0f362069d86 + reviewed follow-up diff | privacy, public-catalog throttling, ingestion-recovery, and merge-readiness review | Confirmed and fixed two remaining P1 defects: mixed-owner authenticated document lists fetched and returned owner-internal nested label/summary fields for public documents, and ingestion recovery selected the first recoverable row so an older failure could supersede a legitimate pending or freshly processing sibling. Nested metadata now uses ownership-specific database projections plus response redaction; recovery preserves active work independent of fetch order; both recovery scripts load pending siblings. Catalog detail throttling findings were already fixed on the reviewed head. | GitHub review-thread and exact-head inspection; Next.js route-handler guide; focused Vitest 129/129; TypeScript; Prettier; `git diff --check`; static CI-mode production-readiness READY with provider keys cleared and offline mode. No live Supabase/OpenAI checks run. | diff --git a/scripts/recover-ingestion-queue.ts b/scripts/recover-ingestion-queue.ts index ba5984063..c74817fa4 100644 --- a/scripts/recover-ingestion-queue.ts +++ b/scripts/recover-ingestion-queue.ts @@ -82,7 +82,7 @@ function parseArgs(argv: string[]) { async function main() { const [ { env, requireServerEnv }, - { buildIngestionRecoveryPlan }, + { buildIngestionRecoveryPlan, INGESTION_RECOVERY_JOB_STATUSES }, { createAdminClient }, { assertSupabaseHealthy, probeSupabaseHealth }, ] = await Promise.all([ @@ -105,7 +105,7 @@ async function main() { const { data, error } = await supabase .from("ingestion_jobs") .select("id,document_id,status,locked_at,documents(status,page_count,chunk_count)") - .in("status", ["pending", "processing", "failed"]) + .in("status", [...INGESTION_RECOVERY_JOB_STATUSES]) .order("created_at", { ascending: true }); if (error) throw supabaseStageError("load open ingestion jobs", error); diff --git a/scripts/reindex.ts b/scripts/reindex.ts index 88bf700eb..1ade1b447 100644 --- a/scripts/reindex.ts +++ b/scripts/reindex.ts @@ -73,7 +73,7 @@ async function safeCount(label: string, countPromise: PromiseLike) async function main() { const [ { env, requireServerEnv }, - { buildIngestionRecoveryPlan, isFreshProcessingJob }, + { buildIngestionRecoveryPlan, INGESTION_RECOVERY_JOB_STATUSES, isFreshProcessingJob }, { hasIncompleteDocumentsWithoutOpenJobs, isReindexQueueClear }, { createAdminClient }, { assertSupabaseHealthy, probeSupabaseHealth }, @@ -229,7 +229,7 @@ async function main() { const { data, error } = await supabase .from("ingestion_jobs") .select("id,document_id,status,locked_at,documents(status,page_count,chunk_count)") - .in("status", ["processing", "failed"]) + .in("status", [...INGESTION_RECOVERY_JOB_STATUSES]) .order("created_at", { ascending: true }); if (error) { @@ -255,6 +255,9 @@ async function main() { : (job.documents as RecoveryDocument | undefined), })) .filter((job) => { + if (job.status === "pending") { + return true; + } if (job.status === "failed") { return true; } diff --git a/src/app/api/documents/route.ts b/src/app/api/documents/route.ts index 6c8c4bf2d..9a4e24ccb 100644 --- a/src/app/api/documents/route.ts +++ b/src/app/api/documents/route.ts @@ -7,6 +7,7 @@ import { jsonError } from "@/lib/http"; import { createAdminClient } from "@/lib/supabase/admin"; import { AuthenticationError, unauthorizedResponse } from "@/lib/supabase/auth"; import { + callerOwnsDocumentRow, enforceDocumentReadRateLimit, redactNonOwnedDocumentFields, withOwnerReadScope, @@ -96,10 +97,18 @@ const VALID_STATUSES = new Set(["queued", "processing", "indexed", "failed"]); const ACTIVE_DOCUMENT_STATUSES = new Set(["queued", "processing"]); const ACTIVE_INDEXING_POLL_MS = 5_000; -type DocumentListRow = Record & { id: string; status?: string | null }; +type DocumentListRow = Record & { id: string; owner_id?: unknown; status?: string | null }; type LabelListRow = Record & { document_id: string }; type SummaryListRow = Record & { document_id: string }; +function projectPublicFields>(row: T, columns: string): Partial { + const projected: Record = {}; + for (const field of columns.split(",")) { + if (Object.hasOwn(row, field)) projected[field] = row[field]; + } + return projected as Partial; +} + const documentListQuerySchema = z.object({ limit: queryInteger({ fallback: 100, min: 1, max: 200 }), offset: queryInteger({ fallback: 0, min: 0, max: 1_000_000 }), @@ -188,9 +197,14 @@ export async function GET(request: Request) { // An authenticated caller reads PUBLIC (owner_id IS NULL) documents alongside their own via // withOwnerReadScope. Redact operator-internal storage fields on the rows they do not own so a // shared public document never exposes its owner's storage_path/content_hash/etc. (S1/D1). - const documents = ((data ?? []) as unknown as DocumentListRow[]).map((document) => - redactNonOwnedDocumentFields(document, access.ownerId), + const rawDocuments = (data ?? []) as unknown as DocumentListRow[]; + const ownedDocumentIds = new Set( + rawDocuments.filter((document) => callerOwnsDocumentRow(document, access.ownerId)).map((document) => document.id), ); + const publicDocumentIds = rawDocuments + .filter((document) => !ownedDocumentIds.has(document.id)) + .map((document) => document.id); + const documents = rawDocuments.map((document) => redactNonOwnedDocumentFields(document, access.ownerId)); const documentIds = documents.map((document) => document.id); const indexing = indexingState(documents); @@ -206,24 +220,49 @@ export async function GET(request: Request) { return documentsResponse({ documents, pagination }, indexing); } - const labelColumns = access.authenticated ? LABEL_LIST_COLUMNS : PUBLIC_LABEL_LIST_COLUMNS; - const summaryColumns = access.authenticated ? SUMMARY_LIST_COLUMNS : PUBLIC_SUMMARY_LIST_COLUMNS; - const [labelsResult, summariesResult] = await Promise.all([ - supabase.from("document_labels").select(labelColumns).in("document_id", documentIds), - supabase.from("document_summaries").select(summaryColumns).in("document_id", documentIds), + const ownedIds = [...ownedDocumentIds]; + const emptyResult = () => Promise.resolve({ data: [], error: null }); + const [ownedLabelsResult, publicLabelsResult, ownedSummariesResult, publicSummariesResult] = await Promise.all([ + ownedIds.length + ? supabase.from("document_labels").select(LABEL_LIST_COLUMNS).in("document_id", ownedIds) + : emptyResult(), + publicDocumentIds.length + ? supabase.from("document_labels").select(PUBLIC_LABEL_LIST_COLUMNS).in("document_id", publicDocumentIds) + : emptyResult(), + ownedIds.length + ? supabase.from("document_summaries").select(SUMMARY_LIST_COLUMNS).in("document_id", ownedIds) + : emptyResult(), + publicDocumentIds.length + ? supabase.from("document_summaries").select(PUBLIC_SUMMARY_LIST_COLUMNS).in("document_id", publicDocumentIds) + : emptyResult(), ]); - if (labelsResult.error) throw new Error(labelsResult.error.message); - if (summariesResult.error) throw new Error(summariesResult.error.message); + for (const result of [ownedLabelsResult, publicLabelsResult, ownedSummariesResult, publicSummariesResult]) { + if (result.error) throw new Error(result.error.message); + } const labelsByDocument = new Map(); - for (const label of (labelsResult.data ?? []) as unknown as LabelListRow[]) { + const labelRows = [ + ...(ownedLabelsResult.data ?? []), + ...(publicLabelsResult.data ?? []), + ] as unknown as LabelListRow[]; + for (const label of labelRows) { const existing = labelsByDocument.get(label.document_id) ?? []; - existing.push(label); + existing.push( + ownedDocumentIds.has(label.document_id) ? label : projectPublicFields(label, PUBLIC_LABEL_LIST_COLUMNS), + ); labelsByDocument.set(label.document_id, existing); } const summariesByDocument = new Map( - ((summariesResult.data ?? []) as unknown as SummaryListRow[]).map((summary) => [summary.document_id, summary]), + [...(ownedSummariesResult.data ?? []), ...(publicSummariesResult.data ?? [])].map((value) => { + const summary = value as unknown as SummaryListRow; + return [ + summary.document_id, + ownedDocumentIds.has(summary.document_id) + ? summary + : projectPublicFields(summary, PUBLIC_SUMMARY_LIST_COLUMNS), + ]; + }), ); return documentsResponse( diff --git a/src/lib/ingestion-recovery.ts b/src/lib/ingestion-recovery.ts index 180d7ced8..5969c617e 100644 --- a/src/lib/ingestion-recovery.ts +++ b/src/lib/ingestion-recovery.ts @@ -14,6 +14,8 @@ export type IngestionRecoveryAction = | { action: "supersede"; jobId: string; documentId: string } | { action: "retry"; jobId: string; documentId: string; resetDocument: boolean }; +export const INGESTION_RECOVERY_JOB_STATUSES = ["pending", "processing", "failed"] as const; + function parseLockedAt(value: string | null | undefined) { if (!value) return null; const time = Date.parse(value); @@ -50,44 +52,50 @@ export function buildIngestionRecoveryPlan(args: { const resetDocuments = new Set(); const actions: IngestionRecoveryAction[] = []; + const jobsByDocument = new Map(); for (const job of args.jobs) { - const documentStatus = job.documents?.status ?? null; - const chunkCount = Number(job.documents?.chunk_count ?? 0); - const isIndexedDocument = documentStatus === "indexed" && chunkCount > 0; - const isRecoverableStatus = - job.status === "failed" || - job.status === "pending" || - isRecoverableProcessingJob(job, now, args.staleAfterMinutes); - - if (isIndexedDocument && job.status !== "completed") { - // Audit R22: a `pending` job on an already-indexed document is a - // legitimately-queued reindex, not an abandoned leftover. Superseding it - // silently cancels the reindex ("completed / superseded by successful - // index"); routing it through the retry branch below would reset the live - // index (R19). Leave it untouched for the worker's atomic reindex path, - // which keeps the old generation live until the new commit swaps it. - if (job.status === "pending") { - continue; + const siblings = jobsByDocument.get(job.document_id) ?? []; + siblings.push(job); + jobsByDocument.set(job.document_id, siblings); + } + + for (const [documentId, jobs] of jobsByDocument) { + const document = jobs[0]?.documents; + const isIndexedDocument = document?.status === "indexed" && Number(document.chunk_count ?? 0) > 0; + const activeJob = + jobs.find((job) => job.status === "pending") ?? + jobs.find((job) => isFreshProcessingJob(job, now, args.staleAfterMinutes)); + + if (activeJob) { + // A pending or freshly processing job is already the legitimate queue owner. Keep it intact + // and close only failed/stale siblings; retrying an older sibling would either collide with + // the open-job unique index or silently supersede valid work depending on fetch order. + for (const job of jobs) { + if (job.id === activeJob.id || job.status === "completed") continue; + actions.push({ action: "supersede", jobId: job.id, documentId }); } - actions.push({ action: "supersede", jobId: job.id, documentId: job.document_id }); continue; } - if (isRecoverableStatus) { - // Audit I2/E2: `ingestion_jobs_one_open_per_document_uidx` forbids two OPEN - // (pending/processing) rows for one document, but a document can legitimately arrive here - // with several recoverable jobs at once (e.g. a `failed` row beside a `pending` row). - // Requeue only the FIRST recoverable job per document; supersede every additional one. - // Without this, recovery emitted two `retry` actions and tried to set both rows to - // `pending`, tripping the unique index (23505) and leaving the queue in a self-perpetuating - // stall that re-crashed on every subsequent run. - if (resetDocuments.has(job.document_id)) { - actions.push({ action: "supersede", jobId: job.id, documentId: job.document_id }); - continue; + const recoverableJobs = jobs.filter( + (job) => job.status === "failed" || isRecoverableProcessingJob(job, now, args.staleAfterMinutes), + ); + + if (isIndexedDocument) { + for (const job of recoverableJobs) { + actions.push({ action: "supersede", jobId: job.id, documentId }); } - resetDocuments.add(job.document_id); - actions.push({ action: "retry", jobId: job.id, documentId: job.document_id, resetDocument: true }); + continue; + } + + const retryJob = recoverableJobs.find((job) => job.status === "processing") ?? recoverableJobs[0]; + if (!retryJob) continue; + + for (const job of recoverableJobs) { + if (job.id !== retryJob.id) actions.push({ action: "supersede", jobId: job.id, documentId }); } + resetDocuments.add(documentId); + actions.push({ action: "retry", jobId: retryJob.id, documentId, resetDocument: true }); } // Apply supersedes before retries. A retry flips a row to `pending`; if a redundant sibling for diff --git a/tests/ingestion-recovery.test.ts b/tests/ingestion-recovery.test.ts index 5d7cc8c67..24f7c7b8a 100644 --- a/tests/ingestion-recovery.test.ts +++ b/tests/ingestion-recovery.test.ts @@ -1,6 +1,7 @@ import { describe, expect, it } from "vitest"; import { buildIngestionRecoveryPlan, + INGESTION_RECOVERY_JOB_STATUSES, isFreshProcessingJob, isRecoverableProcessingJob, isStaleProcessingJob, @@ -9,6 +10,10 @@ import { describe("ingestion queue recovery planning", () => { const now = new Date("2026-06-15T00:00:00.000Z"); + it("loads pending jobs alongside failed and processing siblings", () => { + expect(INGESTION_RECOVERY_JOB_STATUSES).toEqual(["pending", "processing", "failed"]); + }); + it("selects stale processing and failed jobs for retry", () => { const plan = buildIngestionRecoveryPlan({ now, @@ -92,13 +97,13 @@ describe("ingestion queue recovery planning", () => { expect(plan.retryCount).toBe(0); }); - it("requeues only one job and supersedes the sibling when a document has both pending and failed jobs (I2)", () => { + it("preserves a pending job and supersedes its failed sibling regardless of fetch order (I2)", () => { const plan = buildIngestionRecoveryPlan({ now, staleAfterMinutes: 45, jobs: [ - // The older `failed` row is iterated first; the still-open `pending` sibling must not be - // flipped to a second `pending` row (which would collide on the partial unique index). + // The older `failed` row is iterated first; recovery must still prefer the legitimate + // open `pending` sibling rather than replacing it based on fetch order. { id: "failed-first", document_id: "doc-double", @@ -114,14 +119,36 @@ describe("ingestion queue recovery planning", () => { ], }); - expect(plan.retryCount).toBe(1); + expect(plan.retryCount).toBe(0); expect(plan.supersedeCount).toBe(1); - expect(plan.resetDocumentIds).toEqual(["doc-double"]); - // Supersede must be applied before the retry so the open sibling is closed first. - expect(plan.actions[0]).toMatchObject({ action: "supersede", documentId: "doc-double" }); - expect(plan.actions[1]).toMatchObject({ action: "retry", documentId: "doc-double" }); - // Exactly one job is requeued to `pending`; the other is closed. - expect(plan.actions.filter((action) => action.action === "retry")).toHaveLength(1); + expect(plan.resetDocumentIds).toEqual([]); + expect(plan.actions).toEqual([{ action: "supersede", jobId: "failed-first", documentId: "doc-double" }]); + }); + + it("preserves a fresh processing job and supersedes its failed sibling", () => { + const plan = buildIngestionRecoveryPlan({ + now, + staleAfterMinutes: 45, + jobs: [ + { + id: "failed-first", + document_id: "doc-active", + status: "failed", + documents: { status: "processing", chunk_count: 0 }, + }, + { + id: "processing-second", + document_id: "doc-active", + status: "processing", + locked_at: "2026-06-14T23:30:00.000Z", + documents: { status: "processing", chunk_count: 0 }, + }, + ], + }); + + expect(plan.retryCount).toBe(0); + expect(plan.resetDocumentIds).toEqual([]); + expect(plan.actions).toEqual([{ action: "supersede", jobId: "failed-first", documentId: "doc-active" }]); }); it("does not reclaim fresh processing jobs", () => { diff --git a/tests/private-access-routes.test.ts b/tests/private-access-routes.test.ts index 54b78bae8..7ff6f635f 100644 --- a/tests/private-access-routes.test.ts +++ b/tests/private-access-routes.test.ts @@ -574,6 +574,72 @@ describe("private document API access", () => { expect(sharedRow).not.toHaveProperty("error_message"); }); + it("redacts nested metadata for non-owned public documents in an authenticated list", async () => { + const owned = { id: documentId, owner_id: userId, title: "Owned document", status: "indexed" }; + const shared = { id: otherDocumentId, owner_id: null, title: "Public guideline", status: "indexed" }; + const client = createSupabaseMock((call) => { + if (call.table === "documents") return ok([owned, shared]); + const requestedIds = call.inFilters.find((filter) => filter.column === "document_id")?.values ?? []; + if (call.table === "document_labels") { + const rows = [ + { id: "owned-label", document_id: documentId, label: "Owned", metadata: { private: true } }, + { id: "shared-label", document_id: otherDocumentId, label: "Shared", metadata: { private: true } }, + ]; + return ok(rows.filter((row) => requestedIds.includes(row.document_id))); + } + if (call.table === "document_summaries") { + const rows = [ + { + id: "owned-summary", + document_id: documentId, + summary: "Owned summary", + source_chunk_ids: ["owned-chunk"], + metadata: { private: true }, + }, + { + id: "shared-summary", + document_id: otherDocumentId, + summary: "Shared summary", + source_chunk_ids: ["shared-chunk"], + metadata: { private: true }, + }, + ]; + return ok(rows.filter((row) => requestedIds.includes(row.document_id))); + } + return ok([]); + }); + mockRuntime(client); + const { GET } = await import("../src/app/api/documents/route"); + + const response = await GET(authenticatedRequest("/api/documents?includeMeta=true")); + const body = await payload(response); + const [ownedRow, sharedRow] = body.documents as Array>; + + expect(response.status).toBe(200); + const labelCalls = client.calls.filter((call) => call.table === "document_labels"); + const summaryCalls = client.calls.filter((call) => call.table === "document_summaries"); + expect(labelCalls).toHaveLength(2); + expect(summaryCalls).toHaveLength(2); + expect(labelCalls.map((call) => call.selected)).toEqual( + expect.arrayContaining([expect.stringContaining("metadata"), expect.not.stringContaining("metadata")]), + ); + expect(summaryCalls.map((call) => call.selected)).toEqual( + expect.arrayContaining([expect.stringContaining("source_chunk_ids"), expect.not.stringContaining("metadata")]), + ); + expect((ownedRow.labels as Array>)[0]).toHaveProperty("metadata"); + expect(ownedRow.summary).toHaveProperty("source_chunk_ids"); + expect((sharedRow.labels as Array>)[0]).toEqual({ + id: "shared-label", + document_id: otherDocumentId, + label: "Shared", + }); + expect(sharedRow.summary).toEqual({ + id: "shared-summary", + document_id: otherDocumentId, + summary: "Shared summary", + }); + }); + it("accepts legacy Supabase auth cookies for private document access", async () => { const documents = [{ id: documentId, owner_id: userId, title: "Owned document" }]; const client = createSupabaseMock((call) => (call.table === "documents" ? ok(documents) : ok([]))); From 1100c6ba4574f810c816e74f85b4e2e78f3439cc Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 14 Jul 2026 20:33:00 +0000 Subject: [PATCH 4/9] Fix reindex fresh-processing recovery gap + rate-limit catalog detail routes Follow-up to the review of aa161b7. Ingestion recovery (P1, confirmed by CodeRabbit + Codex): - scripts/reindex.ts still filtered out fresh `processing` jobs before calling buildIngestionRecoveryPlan, so a document with a failed row plus a fresh processing retry passed only the failed row to the planner. Applying the resulting retry set the failed row to `pending` while the processing sibling was still open, tripping ingestion_jobs_one_open_per_document_uidx again. The filter is removed; every queried job (pending/processing/failed) now flows to the planner, whose active-vs-recoverable classification keeps the in-flight owner and supersedes only failed/stale siblings. Covered by the planner's "preserves a fresh processing job and supersedes its failed sibling" test. Anonymous catalog detail rate limits (finding C): - The medications, registry-record, differential, and presentation [slug] detail routes still short-circuited to the seed detail before publicAccessContext and the registry limiter, so known-slug requests bypassed rate limiting. They now resolve access and pass the limiter like the list routes. Added anonymous-detail 429 coverage for the medications and registry detail routes. - Removed the now-unused shouldResolvePublicCatalogAccess / hasPublicApiAuthSignal / hasBearerAuthAttempt / hasSessionCookieSignal helpers (no remaining callers). Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01U3gX2PtXiEELGtfL7cmCQb --- scripts/reindex.ts | 34 ++++++------------- src/app/api/differentials/[slug]/route.ts | 27 +++------------ .../presentations/[slug]/route.ts | 23 +++---------- src/app/api/medications/[slug]/route.ts | 14 +++----- src/app/api/registry/records/[slug]/route.ts | 14 +++----- src/lib/public-api-access.ts | 20 ----------- tests/medications-route.test.ts | 16 +++++++++ tests/registry-records-route.test.ts | 18 +++++++++- 8 files changed, 60 insertions(+), 106 deletions(-) diff --git a/scripts/reindex.ts b/scripts/reindex.ts index 1ade1b447..cfeb48006 100644 --- a/scripts/reindex.ts +++ b/scripts/reindex.ts @@ -246,29 +246,17 @@ async function main() { locked_at: string | null; documents: RecoveryDocument | RecoveryDocument[] | null; }; - const staleAfterCutoff = new Date(staleCutoff); - const jobs = (data ?? []) - .map((job: RawJobRow) => ({ - ...job, - documents: Array.isArray(job.documents) - ? (job.documents[0] as RecoveryDocument | undefined) - : (job.documents as RecoveryDocument | undefined), - })) - .filter((job) => { - if (job.status === "pending") { - return true; - } - if (job.status === "failed") { - return true; - } - if (job.status !== "processing") { - return false; - } - if (job.locked_at === null) { - return true; - } - return new Date(job.locked_at) <= staleAfterCutoff; - }); + // Pass every queried job (pending/processing/failed) to the planner and let its + // active-vs-recoverable classification decide. Filtering out fresh `processing` jobs here + // hid legitimate in-flight owners from the planner: a document with a failed row plus a + // fresh processing retry would then requeue the failed row and collide with the still-open + // processing sibling on ingestion_jobs_one_open_per_document_uidx (Audit I2/E2 follow-up). + const jobs = (data ?? []).map((job: RawJobRow) => ({ + ...job, + documents: Array.isArray(job.documents) + ? (job.documents[0] as RecoveryDocument | undefined) + : (job.documents as RecoveryDocument | undefined), + })); hasActiveProcessingJobs = (data ?? []) .map((job: RawJobRow) => ({ ...job, diff --git a/src/app/api/differentials/[slug]/route.ts b/src/app/api/differentials/[slug]/route.ts index 16d51546b..e61e7fcf1 100644 --- a/src/app/api/differentials/[slug]/route.ts +++ b/src/app/api/differentials/[slug]/route.ts @@ -19,7 +19,7 @@ import { getDifferentialDetailContext, getDifferentialRecord, getPresentationWor import { isDemoMode, isLocalNoAuthMode } from "@/lib/env"; import { jsonError } from "@/lib/http"; import { safeErrorLogDetails } from "@/lib/privacy"; -import { publicAccessContext, shouldResolvePublicCatalogAccess } from "@/lib/public-api-access"; +import { publicAccessContext } from "@/lib/public-api-access"; import { createAdminClient } from "@/lib/supabase/admin"; import { AuthenticationError, unauthorizedResponse } from "@/lib/supabase/auth"; import { parseRequestQuery } from "@/lib/validation/query"; @@ -69,28 +69,9 @@ export async function GET(request: Request, context: { params: Promise<{ slug: s }); } - if (!shouldResolvePublicCatalogAccess(request)) { - const snapshot = loadDifferentialSnapshot(); - const governance = deriveGovernanceFromSnapshot(snapshot); - if (kind === "presentation") { - const workflow = getPresentationWorkflow(normalizedSlug); - if (!workflow) return notFoundResponse(normalizedSlug); - return differentialResponse({ - workflow, - governance: { sourceStatus: governance.source_status, validationStatus: governance.validation_status }, - publicAccess: true, - }); - } - const record = getDifferentialRecord(normalizedSlug); - if (!record) return notFoundResponse(normalizedSlug); - return differentialResponse({ - record, - detailContext: getDifferentialDetailContext(record), - governance: { sourceStatus: governance.source_status, validationStatus: governance.validation_status }, - publicAccess: true, - }); - } - + // Anonymous callers still resolve access + rate limit before we serve the seed detail: + // publicAccessContext skips the Supabase auth round-trip when there's no session cookie/bearer, + // but every caller must pass the registry limiter (M4/C1 — no anonymous bypass). const supabase = createAdminClient(); const access = await publicAccessContext(request, supabase); diff --git a/src/app/api/differentials/presentations/[slug]/route.ts b/src/app/api/differentials/presentations/[slug]/route.ts index ff5be6a18..afc883800 100644 --- a/src/app/api/differentials/presentations/[slug]/route.ts +++ b/src/app/api/differentials/presentations/[slug]/route.ts @@ -18,7 +18,7 @@ import { getDifferentialRecord, getPresentationWorkflow } from "@/lib/differenti import { isDemoMode, isLocalNoAuthMode } from "@/lib/env"; import { jsonError } from "@/lib/http"; import { safeErrorLogDetails } from "@/lib/privacy"; -import { publicAccessContext, shouldResolvePublicCatalogAccess } from "@/lib/public-api-access"; +import { publicAccessContext } from "@/lib/public-api-access"; import { createAdminClient } from "@/lib/supabase/admin"; import { AuthenticationError, unauthorizedResponse } from "@/lib/supabase/auth"; @@ -58,24 +58,9 @@ export async function GET(request: Request, context: { params: Promise<{ slug: s }); } - if (!shouldResolvePublicCatalogAccess(request)) { - const snapshot = loadDifferentialSnapshot(); - const workflow = getPresentationWorkflow(normalizedSlug); - if (!workflow) return notFoundResponse(normalizedSlug); - const governance = deriveGovernanceFromSnapshot(snapshot); - const candidates = workflow.candidates.flatMap((candidate) => { - const record = getDifferentialRecord(candidate.slug); - if (!record) return []; - return [{ ...candidate, record }]; - }); - return differentialResponse({ - workflow, - candidates, - governance: { sourceStatus: governance.source_status, validationStatus: governance.validation_status }, - publicAccess: true, - }); - } - + // Anonymous callers still resolve access + rate limit before we serve the seed detail: + // publicAccessContext skips the Supabase auth round-trip when there's no session cookie/bearer, + // but every caller must pass the registry limiter (M4/C1 — no anonymous bypass). const supabase = createAdminClient(); const access = await publicAccessContext(request, supabase); diff --git a/src/app/api/medications/[slug]/route.ts b/src/app/api/medications/[slug]/route.ts index 9dc043993..48de02a16 100644 --- a/src/app/api/medications/[slug]/route.ts +++ b/src/app/api/medications/[slug]/route.ts @@ -17,7 +17,7 @@ import { rowToMedicationRecord, type MedicationRecordRow, } from "@/lib/medication-records"; -import { publicAccessContext, shouldResolvePublicCatalogAccess } from "@/lib/public-api-access"; +import { publicAccessContext } from "@/lib/public-api-access"; import { createAdminClient } from "@/lib/supabase/admin"; import { AuthenticationError, unauthorizedResponse } from "@/lib/supabase/auth"; @@ -61,15 +61,9 @@ export async function GET(request: Request, context: { params: Promise<{ slug: s }); } - if (!shouldResolvePublicCatalogAccess(request)) { - const payload = publicMedicationDetailPayload(normalizedSlug); - if (!payload) return notFoundResponse(normalizedSlug); - return medicationResponse({ - ...payload, - publicAccess: true, - }); - } - + // Anonymous callers still resolve access + rate limit before we serve the seed detail: + // publicAccessContext skips the Supabase auth round-trip when there's no session cookie/bearer, + // but every caller must pass the registry limiter (M4/C1 — no anonymous bypass). const supabase = createAdminClient(); const access = await publicAccessContext(request, supabase); diff --git a/src/app/api/registry/records/[slug]/route.ts b/src/app/api/registry/records/[slug]/route.ts index 10978d62c..0734098cd 100644 --- a/src/app/api/registry/records/[slug]/route.ts +++ b/src/app/api/registry/records/[slug]/route.ts @@ -9,7 +9,7 @@ import { import { isDemoMode, isLocalNoAuthMode } from "@/lib/env"; import { jsonError } from "@/lib/http"; import { safeErrorLogDetails } from "@/lib/privacy"; -import { publicAccessContext, shouldResolvePublicCatalogAccess } from "@/lib/public-api-access"; +import { publicAccessContext } from "@/lib/public-api-access"; import { getFormRecord } from "@/lib/forms"; import { deriveGovernanceColumns, @@ -67,15 +67,9 @@ export async function GET(request: Request, context: { params: Promise<{ slug: s }); } - if (!shouldResolvePublicCatalogAccess(request)) { - const payload = publicRegistryDetailPayload(kind, normalizedSlug); - if (!payload) return notFoundResponse(normalizedSlug); - return registryResponse({ - ...payload, - publicAccess: true, - }); - } - + // Anonymous callers still resolve access + rate limit before we serve the seed detail: + // publicAccessContext skips the Supabase auth round-trip when there's no session cookie/bearer, + // but every caller must pass the registry limiter (M4/C1 — no anonymous bypass). const supabase = createAdminClient(); const access = await publicAccessContext(request, supabase); diff --git a/src/lib/public-api-access.ts b/src/lib/public-api-access.ts index a228404d2..943e3b5d0 100644 --- a/src/lib/public-api-access.ts +++ b/src/lib/public-api-access.ts @@ -55,26 +55,6 @@ export function anonymousApiSubjectKey(request: Request) { return `anon:${createHash("sha256").update(source).digest("hex").slice(0, 32)}`; } -export function hasSessionCookieSignal(request: Request) { - const cookieHeader = request.headers.get("cookie") ?? ""; - return cookieHeader.includes("sb-"); -} - -export function hasBearerAuthAttempt(request: Request) { - const authorization = request.headers.get("authorization") ?? ""; - return /^Bearer\s+\S+/i.test(authorization); -} - -/** True when the request may carry a durable Supabase session (cookie), not a bare bearer attempt. */ -export function hasPublicApiAuthSignal(request: Request) { - return hasSessionCookieSignal(request); -} - -/** Anonymous callers with no cookie or bearer skip auth resolution and rate limits on curated public catalogs. */ -export function shouldResolvePublicCatalogAccess(request: Request) { - return hasSessionCookieSignal(request) || hasBearerAuthAttempt(request); -} - type OwnerScopedQuery = { eq(column: string, value: unknown): T; is(column: string, value: null): T; diff --git a/tests/medications-route.test.ts b/tests/medications-route.test.ts index 36179a42f..2b30fb779 100644 --- a/tests/medications-route.test.ts +++ b/tests/medications-route.test.ts @@ -281,6 +281,22 @@ describe("medications API", () => { expect(payload.publicAccess).toBe(true); expect(payload.record.slug).toBe("acamprosate"); expect(payload.record.name).toBe("Acamprosate"); + // The seed detail is served without a table read, but anonymous detail requests must still + // pass the registry limiter (finding C — no anonymous bypass). + expect(client.from).not.toHaveBeenCalled(); + expect(client.rpc).toHaveBeenCalled(); + }); + + it("rate-limits anonymous detail requests (429) instead of skipping the limiter", async () => { + const client = createSupabaseMock(() => ok([]), { limited: true }); + mockRuntime(client); + const { GET } = await import("../src/app/api/medications/[slug]/route"); + + const response = await GET(request("/api/medications/acamprosate"), { + params: Promise.resolve({ slug: "acamprosate" }), + }); + + expect(response.status).toBe(429); expect(client.from).not.toHaveBeenCalled(); }); diff --git a/tests/registry-records-route.test.ts b/tests/registry-records-route.test.ts index 22e437e2a..9373179c4 100644 --- a/tests/registry-records-route.test.ts +++ b/tests/registry-records-route.test.ts @@ -337,8 +337,24 @@ describe("registry records API", () => { expect(payload.publicAccess).toBe(true); expect(payload.record.slug).toBe("13yarn"); expect(payload.linkedDocuments).toEqual([]); + // The seed detail is served without a table read or auth round-trip, but anonymous detail + // requests must still pass the registry limiter (finding C — no anonymous bypass). + expect(client.from).not.toHaveBeenCalled(); + expect(client.rpc).toHaveBeenCalled(); + expect(client.auth.getUser).not.toHaveBeenCalled(); + }); + + it("rate-limits anonymous detail requests (429) instead of skipping the limiter", async () => { + const client = createSupabaseMock(() => ok([]), { limited: true }); + mockRuntime(client); + const { GET } = await import("../src/app/api/registry/records/[slug]/route"); + + const response = await GET(request("/api/registry/records/13YARN?kind=service"), { + params: Promise.resolve({ slug: "13YARN" }), + }); + + expect(response.status).toBe(429); expect(client.from).not.toHaveBeenCalled(); - expect(client.rpc).not.toHaveBeenCalled(); expect(client.auth.getUser).not.toHaveBeenCalled(); }); From 920ac631ae7e46ae38d82bf44d21696b7ed41181 Mon Sep 17 00:00:00 2001 From: BigSimmo <87357024+BigSimmo@users.noreply.github.com> Date: Wed, 15 Jul 2026 04:36:22 +0800 Subject: [PATCH 5/9] docs: preserve audit remediation review record --- docs/branch-review-ledger.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/branch-review-ledger.md b/docs/branch-review-ledger.md index b4a094c44..18596e132 100644 --- a/docs/branch-review-ledger.md +++ b/docs/branch-review-ledger.md @@ -553,3 +553,5 @@ Use this ledger to prevent repeated branch and PR reviews when the reviewed HEAD | 2026-07-14 | cursor/fix-pr654-ci-53b4 | 9f880853ea7d268186d982f4623b71f46e77d3dc | branch-cleanup-deletion-pending | Pending deletion: content already on origin/main or superseded old-lineage snapshot / throwaway CI-retry branch; branch-only-file sweep found no novel work. Remote deletion DENIED (HTTP 403, session lacks ref-delete permission); branch still exists at this HEAD — operator must complete deletion. Scope is deletion-pending so future cleanup passes re-evaluate rather than skip. | Provider-backed GitHub PR inventory via MCP (400 PRs #264-#674, all merged=false); local read-only git: rewritten-main-aware cherry-pick + two-dot tree + branch-only-file novelty sweep vs origin/main e75fad90; adversarial re-check. Remote `git push --delete` attempted (provider-backed write) and DENIED with HTTP 403. No OpenAI/Supabase/live-eval checks. | | 2026-07-14 | fix/accessibility-remaining-findings | 9f880853ea7d268186d982f4623b71f46e77d3dc | branch-cleanup-deletion-pending | Pending deletion: content already on origin/main or superseded old-lineage snapshot / throwaway CI-retry branch; branch-only-file sweep found no novel work. Remote deletion DENIED (HTTP 403, session lacks ref-delete permission); branch still exists at this HEAD — operator must complete deletion. Scope is deletion-pending so future cleanup passes re-evaluate rather than skip. | Provider-backed GitHub PR inventory via MCP (400 PRs #264-#674, all merged=false); local read-only git: rewritten-main-aware cherry-pick + two-dot tree + branch-only-file novelty sweep vs origin/main e75fad90; adversarial re-check. Remote `git push --delete` attempted (provider-backed write) and DENIED with HTTP 403. No OpenAI/Supabase/live-eval checks. | | 2026-07-15 | PR #677 / claude/cleanup-branches-worktrees-mxov4x | a93db73a29ca6a19619a6592e2e30ca9ea2f8218 | active PR review and remediation | All three actionable review findings are fixed: every branch namespace is parsed from the ledger table, only an exact completed cleanup review at the current HEAD suppresses repeat work, pending deletions remain actionable, and GitHub provider provenance is accurate. No additional high-confidence defect remains in the changed scope. | GitHub unresolved-thread inventory (0 remaining); hosted required CI green; exact-head focused Vitest `tests/repo-hygiene.test.ts` (9/9); `node scripts/sweep-branch-ledger.mjs --no-fetch --json`; `git diff --check`. No Supabase, OpenAI, or other live-service checks. | +| 2026-07-15 | PR #656 / claude/specifiers-v2-design-r55baf | 58ce935758c31672a0a051c5b3e6b888a7d8d153 | full DSM/ICD specifier catalogue and clinical-content gate review | No remaining high-confidence code defect after the review sequence corrected source provenance, verified-content wording, search ranking/deduplication, empty-state behavior, and neutral mixed-source labelling. The 494 unverified definitions remain withheld from display and ranking. Residual risk is the PR-declared qualified-clinician and TGA classification review before broader clinical deployment. | GitHub review-thread inventory (0 unresolved); exact-head hosted required CI, build, critical UI, UI regression, coverage, static, and security checks green. No live Supabase/OpenAI or provider-backed clinical workflow run. | +| 2026-07-15 | PR #680 / claude/rag-scalability-review-x0s55l | e686048d4a3214176843910e809eb0f362069d86 + reviewed follow-up diff | privacy, public-catalog throttling, ingestion-recovery, and merge-readiness review | Confirmed and fixed two remaining P1 defects: mixed-owner authenticated document lists fetched and returned owner-internal nested label/summary fields for public documents, and ingestion recovery selected the first recoverable row so an older failure could supersede a legitimate pending or freshly processing sibling. Nested metadata now uses ownership-specific database projections plus response redaction; recovery preserves active work independent of fetch order; both recovery scripts load pending siblings. Catalog detail throttling findings were already fixed on the reviewed head. | GitHub review-thread and exact-head inspection; Next.js route-handler guide; focused Vitest 129/129; TypeScript; Prettier; `git diff --check`; static CI-mode production-readiness READY with provider keys cleared and offline mode. No live Supabase/OpenAI checks run. | From 56b7ad8a9dc7af54971db7757a6bf2c9191a36e5 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 14 Jul 2026 20:41:12 +0000 Subject: [PATCH 6/9] style: use text-3xs named step for source-capsule count badge (merged from main) Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01U3gX2PtXiEELGtfL7cmCQb --- src/components/ui-primitives.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/components/ui-primitives.tsx b/src/components/ui-primitives.tsx index 1fc77e3ba..fa1652d10 100644 --- a/src/components/ui-primitives.tsx +++ b/src/components/ui-primitives.tsx @@ -84,7 +84,7 @@ export const sourceCapsuleHit = export const sourceCapsule = "source-capsule-face inline-flex items-center gap-1.5 rounded-full border bg-[color-mix(in_srgb,var(--clinical-accent-soft)_55%,var(--surface))] px-2.5 py-1 text-2xs font-medium text-[color:var(--clinical-accent)]"; export const sourceCapsuleCountBadge = - "nums inline-flex h-4 min-w-4 shrink-0 items-center justify-center rounded-full bg-[color:var(--surface-raised)] px-1 text-[10px] font-semibold leading-none text-[color:var(--clinical-accent)] shadow-[var(--shadow-inset)]"; + "nums inline-flex h-4 min-w-4 shrink-0 items-center justify-center rounded-full bg-[color:var(--surface-raised)] px-1 text-3xs font-semibold leading-none text-[color:var(--clinical-accent)] shadow-[var(--shadow-inset)]"; export const evidenceRow = "flex min-h-12 w-full items-center justify-between gap-3 rounded-lg border border-[color:var(--border)] bg-[color:var(--surface)] px-3 py-2 text-left shadow-[var(--shadow-inset)] transition hover:border-[color:var(--border-strong)] hover:bg-[color:var(--surface-subtle)] focus-visible:outline focus-visible:outline-2 focus-visible:outline-offset-2 focus-visible:outline-[color:var(--focus)]"; export const clinicalNotesRow = From d842f3fdfae4f101ba5d39bb5f5f6995eb1b9172 Mon Sep 17 00:00:00 2001 From: BigSimmo <87357024+BigSimmo@users.noreply.github.com> Date: Wed, 15 Jul 2026 04:47:39 +0800 Subject: [PATCH 7/9] docs: retain audit remediation review after sync --- docs/branch-review-ledger.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/branch-review-ledger.md b/docs/branch-review-ledger.md index d3898fabc..65c19a23f 100644 --- a/docs/branch-review-ledger.md +++ b/docs/branch-review-ledger.md @@ -555,3 +555,4 @@ Use this ledger to prevent repeated branch and PR reviews when the reviewed HEAD | 2026-07-15 | PR #677 / claude/cleanup-branches-worktrees-mxov4x | a93db73a29ca6a19619a6592e2e30ca9ea2f8218 | active PR review and remediation | All three actionable review findings are fixed: every branch namespace is parsed from the ledger table, only an exact completed cleanup review at the current HEAD suppresses repeat work, pending deletions remain actionable, and GitHub provider provenance is accurate. No additional high-confidence defect remains in the changed scope. | GitHub unresolved-thread inventory (0 remaining); hosted required CI green; exact-head focused Vitest `tests/repo-hygiene.test.ts` (9/9); `node scripts/sweep-branch-ledger.mjs --no-fetch --json`; `git diff --check`. No Supabase, OpenAI, or other live-service checks. | | 2026-07-15 | PR #656 / claude/specifiers-v2-design-r55baf | 58ce935758c31672a0a051c5b3e6b888a7d8d153 | full DSM/ICD specifier catalogue and clinical-content gate review | No remaining high-confidence code defect after the review sequence corrected source provenance, verified-content wording, search ranking/deduplication, empty-state behavior, and neutral mixed-source labelling. The 494 unverified definitions remain withheld from display and ranking. Residual risk is the PR-declared qualified-clinician and TGA classification review before broader clinical deployment. | GitHub review-thread inventory (0 unresolved); exact-head hosted required CI, build, critical UI, UI regression, coverage, static, and security checks green. No live Supabase/OpenAI or provider-backed clinical workflow run. | | 2026-07-15 | PR #679 / claude/therapy-compass-pages-rz0m5l | f5ca25f8b6b14f41e63f708933fe4bb311994795 | Therapy Compass production promotion and review-followup | All five review findings are fixed on the reviewed head: run-enabled shared-composer links stay on the Therapy Compass route, production data loads outside `/mockups`, deep links seed the in-tool search, and same-route query changes remount the provider. No additional high-confidence defect remains in the changed scope. | GitHub review-thread inventory; exact-head hosted build, critical UI smoke, UI regression, unit coverage, static, security, and image checks green; focused Vitest 28/28; TypeScript; `git diff --check`. No live Supabase/OpenAI checks run. | +| 2026-07-15 | PR #680 / claude/rag-scalability-review-x0s55l | e686048d4a3214176843910e809eb0f362069d86 + reviewed follow-up diff | privacy, public-catalog throttling, ingestion-recovery, and merge-readiness review | Confirmed and fixed two remaining P1 defects: mixed-owner authenticated document lists fetched and returned owner-internal nested label/summary fields for public documents, and ingestion recovery selected the first recoverable row so an older failure could supersede a legitimate pending or freshly processing sibling. Nested metadata now uses ownership-specific database projections plus response redaction; recovery preserves active work independent of fetch order; both recovery scripts load pending siblings. Catalog detail throttling findings were already fixed on the reviewed head. | GitHub review-thread and exact-head inspection; Next.js route-handler guide; focused Vitest 129/129; TypeScript; Prettier; `git diff --check`; static CI-mode production-readiness READY with provider keys cleared and offline mode. No live Supabase/OpenAI checks run. | From 72bdff1885a8a181286d59dac167e7469b11cd9f Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 14 Jul 2026 20:55:53 +0000 Subject: [PATCH 8/9] Redact summary provenance for non-owners in document detail route MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the review of aa161b7/920ac631 (Codex P1). The [id] document detail route fetches the summary with select("*") and, for a non-owner viewing a PUBLIC document, redacted it with omitPublicInternalFields — which did not strip source_chunk_ids, source_image_ids, or model, so summary provenance still leaked. Those keys (present only on document_summaries rows) are now added to the omit set, matching the list route's PUBLIC_SUMMARY projection. Extended the authed-non-owner detail test to assert the summary is readable but its provenance/owner fields are redacted. Also corrected the PR #680 ledger row: catalog detail-route throttling is now actually fixed (not "already fixed"), and the recovery description matches the implemented planner behavior. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01U3gX2PtXiEELGtfL7cmCQb --- docs/branch-review-ledger.md | 2 +- src/app/api/documents/[id]/route.ts | 6 ++++++ tests/private-access-routes.test.ts | 23 +++++++++++++++++++++++ 3 files changed, 30 insertions(+), 1 deletion(-) diff --git a/docs/branch-review-ledger.md b/docs/branch-review-ledger.md index 65c19a23f..a4ef58ce3 100644 --- a/docs/branch-review-ledger.md +++ b/docs/branch-review-ledger.md @@ -555,4 +555,4 @@ Use this ledger to prevent repeated branch and PR reviews when the reviewed HEAD | 2026-07-15 | PR #677 / claude/cleanup-branches-worktrees-mxov4x | a93db73a29ca6a19619a6592e2e30ca9ea2f8218 | active PR review and remediation | All three actionable review findings are fixed: every branch namespace is parsed from the ledger table, only an exact completed cleanup review at the current HEAD suppresses repeat work, pending deletions remain actionable, and GitHub provider provenance is accurate. No additional high-confidence defect remains in the changed scope. | GitHub unresolved-thread inventory (0 remaining); hosted required CI green; exact-head focused Vitest `tests/repo-hygiene.test.ts` (9/9); `node scripts/sweep-branch-ledger.mjs --no-fetch --json`; `git diff --check`. No Supabase, OpenAI, or other live-service checks. | | 2026-07-15 | PR #656 / claude/specifiers-v2-design-r55baf | 58ce935758c31672a0a051c5b3e6b888a7d8d153 | full DSM/ICD specifier catalogue and clinical-content gate review | No remaining high-confidence code defect after the review sequence corrected source provenance, verified-content wording, search ranking/deduplication, empty-state behavior, and neutral mixed-source labelling. The 494 unverified definitions remain withheld from display and ranking. Residual risk is the PR-declared qualified-clinician and TGA classification review before broader clinical deployment. | GitHub review-thread inventory (0 unresolved); exact-head hosted required CI, build, critical UI, UI regression, coverage, static, and security checks green. No live Supabase/OpenAI or provider-backed clinical workflow run. | | 2026-07-15 | PR #679 / claude/therapy-compass-pages-rz0m5l | f5ca25f8b6b14f41e63f708933fe4bb311994795 | Therapy Compass production promotion and review-followup | All five review findings are fixed on the reviewed head: run-enabled shared-composer links stay on the Therapy Compass route, production data loads outside `/mockups`, deep links seed the in-tool search, and same-route query changes remount the provider. No additional high-confidence defect remains in the changed scope. | GitHub review-thread inventory; exact-head hosted build, critical UI smoke, UI regression, unit coverage, static, security, and image checks green; focused Vitest 28/28; TypeScript; `git diff --check`. No live Supabase/OpenAI checks run. | -| 2026-07-15 | PR #680 / claude/rag-scalability-review-x0s55l | e686048d4a3214176843910e809eb0f362069d86 + reviewed follow-up diff | privacy, public-catalog throttling, ingestion-recovery, and merge-readiness review | Confirmed and fixed two remaining P1 defects: mixed-owner authenticated document lists fetched and returned owner-internal nested label/summary fields for public documents, and ingestion recovery selected the first recoverable row so an older failure could supersede a legitimate pending or freshly processing sibling. Nested metadata now uses ownership-specific database projections plus response redaction; recovery preserves active work independent of fetch order; both recovery scripts load pending siblings. Catalog detail throttling findings were already fixed on the reviewed head. | GitHub review-thread and exact-head inspection; Next.js route-handler guide; focused Vitest 129/129; TypeScript; Prettier; `git diff --check`; static CI-mode production-readiness READY with provider keys cleared and offline mode. No live Supabase/OpenAI checks run. | +| 2026-07-15 | PR #680 / claude/rag-scalability-review-x0s55l | d842f3fd + reviewed follow-up fixes | privacy, public-catalog throttling, ingestion-recovery, and merge-readiness review | Audit remediation wave 1 plus review follow-ups. Confirmed and fixed: mixed-owner authenticated document lists returned owner-internal nested label/summary fields for public documents (now fetched with ownership-specific projections plus response redaction); the `[id]` detail route likewise now redacts summary provenance (`source_chunk_ids`/`source_image_ids`/`model`) for non-owners; anonymous catalog rate limiting now covers the `[slug]` DETAIL routes as well as the list routes (removed the `shouldResolvePublicCatalogAccess` bypass and its dead helpers); and ingestion recovery keeps an open pending/freshly-processing job while superseding only failed/stale siblings, with both recovery scripts (`recover-ingestion-queue.ts`, `reindex.ts`) passing all queried statuses to the planner so a failed+fresh-processing document cannot collide on `ingestion_jobs_one_open_per_document_uidx`. Residual (low, deferred): base document metadata is still returned on non-owned public list rows. | GitHub review-thread and exact-head inspection; Next.js route-handler guide; `npm run verify:cheap` (lint, TypeScript, full Vitest) green after merging origin/main; focused route + ingestion-recovery Vitest; Prettier; `git diff --check`. No live Supabase/OpenAI/provider checks run. | diff --git a/src/app/api/documents/[id]/route.ts b/src/app/api/documents/[id]/route.ts index 0309e91d9..5246c653c 100644 --- a/src/app/api/documents/[id]/route.ts +++ b/src/app/api/documents/[id]/route.ts @@ -397,6 +397,12 @@ export async function GET(request: Request, { params }: { params: Promise<{ id: "import_batch_id", "error_message", "metadata", + // Summary provenance: only present on document_summaries rows (fetched with select("*")). + // A non-owner viewing a public document's summary must not see the owner's chunk/image + // source IDs or the generation model, matching the list route's PUBLIC_SUMMARY projection. + "source_chunk_ids", + "source_image_ids", + "model", ]); return Object.fromEntries(Object.entries(row).filter(([key]) => !internalKeys.has(key))); }; diff --git a/tests/private-access-routes.test.ts b/tests/private-access-routes.test.ts index 7ff6f635f..a2e08c79b 100644 --- a/tests/private-access-routes.test.ts +++ b/tests/private-access-routes.test.ts @@ -689,6 +689,20 @@ describe("private document API access", () => { metadata: { index_generation_id: "generation-a", extraction_quality: "good" }, }); } + if (call.table === "document_summaries") { + return ok({ + id: "summary-1", + document_id: documentId, + owner_id: null, + summary: "Public summary text.", + clinical_specifics: null, + source_chunk_ids: ["chunk-a", "chunk-b"], + source_image_ids: ["image-a"], + model: "gpt-internal", + metadata: { internal_note: "owner only" }, + generated_at: "2026-01-01T00:00:00.000Z", + }); + } if (call.table === "document_pages") return ok([]); if (call.table === "document_images") return ok([]); if (call.table === "document_chunks") return ok([]); @@ -718,6 +732,15 @@ describe("private document API access", () => { expect(document).not.toHaveProperty("owner_id"); expect(document).not.toHaveProperty("metadata"); expect(body).not.toHaveProperty("indexHealth"); + // The public document's generated summary is readable, but its owner-internal provenance + // (chunk/image source IDs, generation model, owner_id, metadata) must be redacted. + const summary = document.summary as Record | null; + expect(summary).toMatchObject({ summary: "Public summary text." }); + expect(summary).not.toHaveProperty("source_chunk_ids"); + expect(summary).not.toHaveProperty("source_image_ids"); + expect(summary).not.toHaveProperty("model"); + expect(summary).not.toHaveProperty("owner_id"); + expect(summary).not.toHaveProperty("metadata"); }); it("returns full owner-internal fields when the authenticated caller owns the document", async () => { From 32e242ab7fc386ea82b19c7cfc2112aa41f06f9a Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 14 Jul 2026 21:05:14 +0000 Subject: [PATCH 9/9] Redact free-form metadata from non-owned public document list rows Follow-up to the review of 72bdff18 (CodeRabbit Major). redactNonOwnedDocumentFields kept on non-owned public rows in the document list, but metadata is arbitrary and can carry owner-internal provenance (bulk-edit author user id, prior titles, indexing internals). Anonymous list callers never receive metadata (PUBLIC_DOCUMENT_LIST_COLUMNS excludes it) and the [id] detail route already strips it, so this closes the last inconsistency: metadata is now redacted for non-owners in the list too. Owned rows are unchanged. Updated the list redaction test to assert metadata is absent. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01U3gX2PtXiEELGtfL7cmCQb --- src/lib/public-api-access.ts | 7 +++++-- tests/private-access-routes.test.ts | 5 +++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/lib/public-api-access.ts b/src/lib/public-api-access.ts index 943e3b5d0..101faae41 100644 --- a/src/lib/public-api-access.ts +++ b/src/lib/public-api-access.ts @@ -79,14 +79,17 @@ export function withOwnerReadScope>(query: T, owne // Owner-internal document columns that must never be exposed on a row the caller does not own. // `withOwnerReadScope` lets an authenticated caller read PUBLIC documents (owner_id IS NULL) that // belong to nobody; those rows must not leak the storage location, dedup hash, import provenance, -// or raw stage error of the operator who ingested them. Shared governance `metadata` is preserved -// so public-corpus badges keep rendering — only the operator-internal storage fields are stripped. +// raw stage error, or free-form `metadata` of the operator who ingested them. `metadata` is +// arbitrary and can carry owner-internal provenance (e.g. the bulk-edit author's user id, prior +// titles, indexing internals), so — matching the anonymous list projection and the `[id]` detail +// route — it is stripped for non-owners rather than surfaced as governance data. const NON_OWNER_INTERNAL_DOCUMENT_FIELDS = [ "storage_path", "content_hash", "source_path", "import_batch_id", "error_message", + "metadata", ] as const; /** True when `viewerOwnerId` is set and owns the row (i.e. the caller's own document). */ diff --git a/tests/private-access-routes.test.ts b/tests/private-access-routes.test.ts index a2e08c79b..ba2aaf4a0 100644 --- a/tests/private-access-routes.test.ts +++ b/tests/private-access-routes.test.ts @@ -564,9 +564,10 @@ describe("private document API access", () => { storage_path: `${userId}/documents/owned.pdf`, content_hash: "sha256:owned", }); - // The shared public row keeps its shared governance metadata but not the owner's storage internals. + // The shared public row keeps its public fields but not the owner's storage internals or + // free-form metadata (which can carry owner-internal provenance). expect(sharedRow).toMatchObject({ id: otherDocumentId, title: "Public guideline" }); - expect(sharedRow.metadata).toEqual({ extraction_quality: "partial" }); + expect(sharedRow).not.toHaveProperty("metadata"); expect(sharedRow).not.toHaveProperty("storage_path"); expect(sharedRow).not.toHaveProperty("content_hash"); expect(sharedRow).not.toHaveProperty("source_path");