diff --git a/docs/branch-review-records/6897c4836aee66827e7eec71bfa16917161e501d15805774b5446fb4c393d383.record.md b/docs/branch-review-records/6897c4836aee66827e7eec71bfa16917161e501d15805774b5446fb4c393d383.record.md new file mode 100644 index 0000000000..fd6bdf63b3 --- /dev/null +++ b/docs/branch-review-records/6897c4836aee66827e7eec71bfa16917161e501d15805774b5446fb4c393d383.record.md @@ -0,0 +1 @@ +| 2026-08-16 | codex/chat-trust-boundaries-212-trust-boundaries-212 | f3b55e8555614bffff2eb335b95d342b40960b6c | PR #2003 unblocking review and base synchronization | No confirmed P0-P2 PR-introduced defects; fail-closed list parsing, generic public errors, ownership redaction, and JSON-shape contracts validated; latest main merged without conflicts; distinct manual adversarial pass completed | Exact-head f3b55e85 CI, PR required, static checks, unit coverage, build, lint, typecheck, safety/config, SAST, and secret scan passed; local npm gates unavailable without a checkout; final base-sync head requires CI rerun | diff --git a/docs/branch-review-records/f51d7e3693e266ab35c224b3c487b3173a90a7780810546df0674fb7b35b7d50.record.md b/docs/branch-review-records/f51d7e3693e266ab35c224b3c487b3173a90a7780810546df0674fb7b35b7d50.record.md new file mode 100644 index 0000000000..1ea3827a26 --- /dev/null +++ b/docs/branch-review-records/f51d7e3693e266ab35c224b3c487b3173a90a7780810546df0674fb7b35b7d50.record.md @@ -0,0 +1 @@ +| 2026-08-16 | codex/chat-trust-boundaries-212-trust-boundaries-212 | 89181a5c1f5a74ac08628a750c51d8e3819512c7 | PR #2003 post-review nullish list payload fix | Confirmed CodeRabbit finding: parseListRows coerced null and undefined dependency payloads to successful empty arrays; changed validation to reject nullish values and added focused null, undefined, and explicit empty-array regression coverage | Exact-head 89181a5c PR required, unit coverage, build, static checks, lint, typecheck, safety/config, ingestion SAST, CI-managed Lighthouse, SAST, and secret scan passed before the fix; final fix head requires CI rerun; local npm gates unavailable without a checkout | diff --git a/src/app/api/documents/route.ts b/src/app/api/documents/route.ts index a1af530449..b610378a9a 100644 --- a/src/app/api/documents/route.ts +++ b/src/app/api/documents/route.ts @@ -1,5 +1,10 @@ import { z } from "zod"; -import { ACTIVE_INDEXING_POLL_MS, indexingListResponse, offsetPagination } from "@/lib/api-list-response"; +import { + ACTIVE_INDEXING_POLL_MS, + indexingListResponse, + offsetPagination, + parseListRows, +} from "@/lib/api-list-response"; import { rateLimitJsonResponse } from "@/lib/api-rate-limit"; import { demoDocuments } from "@/lib/demo-data"; import { isDemoMode } from "@/lib/env"; @@ -96,9 +101,17 @@ const SUMMARY_LIST_COLUMNS = [ const VALID_STATUSES = new Set(["queued", "processing", "indexed", "failed"]); const ACTIVE_DOCUMENT_STATUSES = new Set(["queued", "processing"]); -type DocumentListRow = Record & { id: string; owner_id?: unknown; status?: string | null }; -type LabelListRow = Record & { document_id: string }; -type SummaryListRow = Record & { document_id: string }; +const documentListRowSchema = z + .object({ + id: z.string(), + owner_id: z.unknown().optional(), + status: z.string().nullable().optional(), + }) + .passthrough(); +const labelListRowSchema = z.object({ document_id: z.string() }).passthrough(); +const summaryListRowSchema = z.object({ document_id: z.string() }).passthrough(); + +type DocumentListRow = z.infer; function projectPublicFields>(row: T, columns: string): Partial { const projected: Record = {}; @@ -189,7 +202,7 @@ 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 rawDocuments = (error ? [] : (data ?? [])) as unknown as DocumentListRow[]; + const rawDocuments = parseListRows(error ? [] : data, documentListRowSchema); const ownedDocumentIds = new Set( rawDocuments.filter((document) => callerOwnsDocumentRow(document, access.ownerId)).map((document) => document.id), ); @@ -228,10 +241,10 @@ export async function GET(request: Request) { } const labelsByDocument = new Map(); - const labelRows = [ - ...(ownedLabelsResult.data ?? []), - ...(publicLabelsResult.data ?? []), - ] as unknown as LabelListRow[]; + const labelRows = parseListRows( + [...(ownedLabelsResult.data ?? []), ...(publicLabelsResult.data ?? [])], + labelListRowSchema, + ); for (const label of labelRows) { const existing = labelsByDocument.get(label.document_id) ?? []; existing.push( @@ -239,16 +252,15 @@ export async function GET(request: Request) { ); labelsByDocument.set(label.document_id, existing); } + const summaryRows = parseListRows( + [...(ownedSummariesResult.data ?? []), ...(publicSummariesResult.data ?? [])], + summaryListRowSchema, + ); const summariesByDocument = new Map( - [...(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), - ]; - }), + summaryRows.map((summary) => [ + summary.document_id, + ownedDocumentIds.has(summary.document_id) ? summary : projectPublicFields(summary, PUBLIC_SUMMARY_LIST_COLUMNS), + ]), ); return documentsResponse( diff --git a/src/app/api/ingestion/batches/route.ts b/src/app/api/ingestion/batches/route.ts index 270369613e..036c9f8311 100644 --- a/src/app/api/ingestion/batches/route.ts +++ b/src/app/api/ingestion/batches/route.ts @@ -5,6 +5,7 @@ import { emptyPagination, indexingListResponse, offsetPagination, + parseStatusRows, type StatusRow, } from "@/lib/api-list-response"; import { isDemoMode } from "@/lib/env"; @@ -63,7 +64,7 @@ export async function GET(request: Request) { .range(offset, offset + limit - 1); if (error) throw new Error(error.message); - const batches = (data ?? []) as unknown as BatchRow[]; + const batches = parseStatusRows(data); return batchesResponse(batches, { pagination: offsetPagination({ limit, offset, pageLength: batches.length, count }), }); diff --git a/src/app/api/ingestion/jobs/route.ts b/src/app/api/ingestion/jobs/route.ts index b1ba5c9fa0..8ff6cb9ef8 100644 --- a/src/app/api/ingestion/jobs/route.ts +++ b/src/app/api/ingestion/jobs/route.ts @@ -5,6 +5,7 @@ import { emptyPagination, indexingListResponse, offsetPagination, + parseStatusRows, type StatusRow, } from "@/lib/api-list-response"; import { isDemoMode } from "@/lib/env"; @@ -69,7 +70,7 @@ export async function GET(request: Request) { const { data, error, count } = await query; if (error) throw new Error(error.message); - const jobs = (data ?? []) as unknown as JobRow[]; + const jobs = parseStatusRows(data); return jobsResponse(jobs, { pagination: offsetPagination({ limit, offset, pageLength: jobs.length, count }), }); diff --git a/src/app/api/ingestion/quality/route.ts b/src/app/api/ingestion/quality/route.ts index 12c32ef150..1c2ce14738 100644 --- a/src/app/api/ingestion/quality/route.ts +++ b/src/app/api/ingestion/quality/route.ts @@ -1,5 +1,6 @@ import { NextResponse } from "next/server"; import { z } from "zod"; +import { parseListRows } from "@/lib/api-list-response"; import { consumeApiRateLimit, rateLimitJsonResponse } from "@/lib/api-rate-limit"; import { isDemoMode } from "@/lib/env"; import { jsonError } from "@/lib/http"; @@ -13,65 +14,88 @@ type Severity = "danger" | "warning" | "info"; type ReviewType = "failed_ocr" | "low_extraction_confidence" | "missing_tables" | "image_only_pages" | "failed_job" | "manual_review"; -type DocumentRow = { - id: string; - title: string | null; - file_name: string | null; - status: string | null; - page_count: number | null; - chunk_count: number | null; - image_count: number | null; - error_message: string | null; - metadata: Record | null; - updated_at: string | null; -}; - -type QualityRow = { - document_id: string; - quality_score: number | null; - extraction_quality: string | null; - metrics: Record | null; - issues: string[] | null; - updated_at: string | null; -}; - -type JobRow = { - id: string; - document_id: string; - status: string | null; - stage: string | null; - error_message: string | null; - updated_at: string | null; -}; - -type StageRow = { - id: string; - document_id: string; - job_id: string | null; - stage_name: string | null; - stage_status: string | null; - error_message: string | null; - metadata: Record | null; - artifact_counts: Record | null; - finished_at: string | null; - started_at: string | null; -}; - -type PageRow = { - document_id: string; - page_number: number | null; - text: string | null; - ocr_used: boolean | null; - metadata: Record | null; -}; - -type ImageRow = { - document_id: string; - page_number: number | null; - source_kind: string | null; - searchable: boolean | null; - metadata: Record | null; -}; +const nullableRecordSchema = z.record(z.string(), z.unknown()).nullable(); +const nullableStringSchema = z.string().nullable(); +const nullableNumberSchema = z.number().nullable(); + +const documentRowSchema = z + .object({ + id: z.string(), + title: nullableStringSchema, + file_name: nullableStringSchema, + status: nullableStringSchema, + page_count: nullableNumberSchema, + chunk_count: nullableNumberSchema, + image_count: nullableNumberSchema, + error_message: nullableStringSchema, + metadata: nullableRecordSchema, + updated_at: nullableStringSchema, + }) + .passthrough(); + +const qualityRowSchema = z + .object({ + document_id: z.string(), + quality_score: nullableNumberSchema, + extraction_quality: nullableStringSchema, + metrics: nullableRecordSchema, + issues: z.array(z.string()).nullable(), + updated_at: nullableStringSchema, + }) + .passthrough(); + +const jobRowSchema = z + .object({ + id: z.string(), + document_id: z.string(), + status: nullableStringSchema, + stage: nullableStringSchema, + error_message: nullableStringSchema, + updated_at: nullableStringSchema, + }) + .passthrough(); + +const stageRowSchema = z + .object({ + id: z.string(), + document_id: z.string(), + job_id: nullableStringSchema, + stage_name: nullableStringSchema, + stage_status: nullableStringSchema, + error_message: nullableStringSchema, + metadata: nullableRecordSchema, + artifact_counts: nullableRecordSchema, + finished_at: nullableStringSchema, + started_at: nullableStringSchema, + }) + .passthrough(); + +const pageRowSchema = z + .object({ + document_id: z.string(), + page_number: nullableNumberSchema, + text: nullableStringSchema, + ocr_used: z.boolean().nullable(), + metadata: nullableRecordSchema, + }) + .passthrough(); + +const imageRowSchema = z + .object({ + document_id: z.string(), + page_number: nullableNumberSchema, + source_kind: nullableStringSchema, + searchable: z.boolean().nullable(), + metadata: nullableRecordSchema, + }) + .passthrough(); + +type DocumentRow = z.infer; +type QualityRow = z.infer; +type JobRow = z.infer; +type StageRow = z.infer; +type PageRow = z.infer; +type ImageRow = z.infer; type ReviewItem = { id: string; @@ -343,7 +367,7 @@ export async function GET(request: Request) { .limit(limit); if (documentsError) throw new Error(documentsError.message); - const documents = (documentsData ?? []) as unknown as DocumentRow[]; + const documents = parseListRows(documentsData, documentRowSchema); const documentIds = documents.map((document) => document.id); if (documentIds.length === 0) return NextResponse.json({ items: [] }); @@ -381,11 +405,11 @@ export async function GET(request: Request) { return NextResponse.json({ items: buildReviewItems({ documents, - qualityRows: (qualityResult.data ?? []) as unknown as QualityRow[], - jobs: (jobsResult.data ?? []) as unknown as JobRow[], - stages: (stagesResult.data ?? []) as unknown as StageRow[], - pages: (pagesResult.data ?? []) as unknown as PageRow[], - images: (imagesResult.data ?? []) as unknown as ImageRow[], + qualityRows: parseListRows(qualityResult.data, qualityRowSchema), + jobs: parseListRows(jobsResult.data, jobRowSchema), + stages: parseListRows(stagesResult.data, stageRowSchema), + pages: parseListRows(pagesResult.data, pageRowSchema), + images: parseListRows(imagesResult.data, imageRowSchema), }).slice(0, 80), }); } catch (error) { diff --git a/src/app/api/jobs/route.ts b/src/app/api/jobs/route.ts index 7cf390be17..440a10863a 100644 --- a/src/app/api/jobs/route.ts +++ b/src/app/api/jobs/route.ts @@ -4,6 +4,7 @@ import { countActiveRows, indexingListResponse, offsetPagination, + parseStatusRows, type StatusRow, } from "@/lib/api-list-response"; import { demoJobs } from "@/lib/demo-data"; @@ -61,7 +62,7 @@ export async function GET(request: Request) { .range(offset, offset + limit - 1); if (error) throw new Error(error.message); - const jobs = (data ?? []) as unknown as JobRow[]; + const jobs = parseStatusRows(data); return jobsResponse(jobs, { pagination: offsetPagination({ limit, offset, pageLength: jobs.length, count }), }); diff --git a/src/lib/api-list-response.ts b/src/lib/api-list-response.ts index 0f0b980f06..ea143ff7c1 100644 --- a/src/lib/api-list-response.ts +++ b/src/lib/api-list-response.ts @@ -1,4 +1,5 @@ import { NextResponse } from "next/server"; +import { z } from "zod"; /** * Shared shapes for the admin list endpoints (documents, jobs, ingestion jobs, @@ -11,6 +12,36 @@ export const ACTIVE_INDEXING_POLL_MS = 5_000; export type StatusRow = Record & { status?: string | null }; +const statusRowSchema = z + .object({ + status: z.string().nullable().optional(), + }) + .passthrough(); + +/** + * Validates an untrusted list-query result without exposing schema diagnostics. + * Route schemas decide which selected fields are required and whether unknown + * fields should be preserved. + */ +export function parseListRows(data: unknown, rowSchema: z.ZodType): Row[] { + const parsed = z.array(rowSchema).safeParse(data); + if (!parsed.success) { + // Keep dependency-response details out of both the public response and logs. + throw new Error("Invalid list data."); + } + return parsed.data; +} + +/** + * Validates untrusted list-query output before it is counted or returned. + * Unknown fields are preserved because these endpoints intentionally expose + * the selected database rows, but every row must be an object and `status` + * must retain the text/null shape used by polling clients. + */ +export function parseStatusRows(data: unknown): StatusRow[] { + return parseListRows(data, statusRowSchema); +} + export type OffsetPagination = { limit: number; offset: number; diff --git a/tests/api-list-response.test.ts b/tests/api-list-response.test.ts new file mode 100644 index 0000000000..8c237dd049 --- /dev/null +++ b/tests/api-list-response.test.ts @@ -0,0 +1,18 @@ +import { describe, expect, it } from "vitest"; +import { z } from "zod"; +import { parseListRows } from "../src/lib/api-list-response"; + +const rowSchema = z.object({ id: z.string() }); + +describe("parseListRows", () => { + it.each([ + { label: "null", data: null }, + { label: "undefined", data: undefined }, + ])("rejects $label list payloads instead of coercing them to an empty array", ({ data }) => { + expect(() => parseListRows(data, rowSchema)).toThrow("Invalid list data."); + }); + + it("accepts an explicitly empty list", () => { + expect(parseListRows([], rowSchema)).toEqual([]); + }); +}); diff --git a/tests/api-validation-contract.test.ts b/tests/api-validation-contract.test.ts index 54ae24b100..b1c8032ed2 100644 --- a/tests/api-validation-contract.test.ts +++ b/tests/api-validation-contract.test.ts @@ -332,6 +332,42 @@ describe("API validation contracts", () => { expect(client.calls[0].range).toEqual({ from: 10_000, to: 10_049 }); }); + it.each([ + { + table: "documents", + malformedRow: { id: { sensitive: "patient-secret" }, owner_id: userId, status: "indexed" }, + }, + { + table: "document_labels", + malformedRow: { document_id: { sensitive: "patient-secret" }, label: "Clinical" }, + }, + { + table: "document_summaries", + malformedRow: { document_id: { sensitive: "patient-secret" }, summary: "Private summary" }, + }, + ])("rejects and redacts malformed $table rows from the document list", async ({ table, malformedRow }) => { + const client = createSupabaseMock((call) => { + if (call.table === table) return ok([malformedRow]); + if (call.table === "documents") { + return ok([{ id: documentId, owner_id: userId, status: "indexed", title: "Guideline" }], 1); + } + return ok([]); + }); + mockRuntime(client); + const { GET } = await import("../src/app/api/documents/route"); + + const response = await GET(authenticatedRequest("/api/documents")); + const body = await payload(response); + + expect(response.status).toBe(500); + expect(body).toEqual({ + error: "Request failed.", + message: "Request failed.", + code: "internal_error", + }); + expect(JSON.stringify(body)).not.toContain("patient-secret"); + }); + it("treats empty document-detail chunk as absent and clamps page/chunk windows", async () => { const client = createSupabaseMock((call) => { if (call.table === "documents" && call.maybeSingle) { @@ -402,6 +438,115 @@ describe("API validation contracts", () => { expect(client.calls[0]).toMatchObject({ table: "documents", limitCount: 200 }); }); + it.each([ + { + table: "documents", + malformedRow: { + id: { sensitive: "patient-secret" }, + title: "Guideline", + file_name: "guideline.pdf", + status: "indexed", + page_count: 1, + chunk_count: 1, + image_count: 0, + error_message: null, + metadata: {}, + updated_at: null, + }, + }, + { + table: "document_index_quality", + malformedRow: { + document_id: { sensitive: "patient-secret" }, + quality_score: 0.8, + extraction_quality: "good", + metrics: {}, + issues: [], + updated_at: null, + }, + }, + { + table: "ingestion_jobs", + malformedRow: { + id: "job-1", + document_id: { sensitive: "patient-secret" }, + status: "completed", + stage: null, + error_message: null, + updated_at: null, + }, + }, + { + table: "ingestion_job_stages", + malformedRow: { + id: "stage-1", + document_id: { sensitive: "patient-secret" }, + job_id: null, + stage_name: null, + stage_status: null, + error_message: null, + metadata: {}, + artifact_counts: {}, + finished_at: null, + started_at: null, + }, + }, + { + table: "document_pages", + malformedRow: { + document_id: { sensitive: "patient-secret" }, + page_number: 1, + text: null, + ocr_used: false, + metadata: {}, + }, + }, + { + table: "document_images", + malformedRow: { + document_id: { sensitive: "patient-secret" }, + page_number: 1, + source_kind: null, + searchable: false, + metadata: {}, + }, + }, + ])("rejects and redacts malformed $table rows from ingestion quality", async ({ table, malformedRow }) => { + const client = createSupabaseMock((call) => { + if (call.table === table) return ok([malformedRow]); + if (call.table === "documents") { + return ok([ + { + id: documentId, + title: "Guideline", + file_name: "guideline.pdf", + status: "indexed", + page_count: 1, + chunk_count: 1, + image_count: 0, + error_message: null, + metadata: {}, + updated_at: null, + }, + ]); + } + return ok([]); + }); + mockRuntime(client); + const { GET } = await import("../src/app/api/ingestion/quality/route"); + + const response = await GET(authenticatedRequest("/api/ingestion/quality")); + const body = await payload(response); + + expect(response.status).toBe(500); + expect(body).toEqual({ + error: "Request failed.", + message: "Request failed.", + code: "internal_error", + }); + expect(JSON.stringify(body)).not.toContain("patient-secret"); + }); + it("rejects invalid ingestion jobs batchId without querying jobs", async () => { const client = createSupabaseMock(); mockRuntime(client); @@ -473,6 +618,36 @@ describe("API validation contracts", () => { expect(client.calls[2]).toMatchObject({ table: "import_batches", range: { from: 1, to: 2 } }); }); + it.each([ + { + path: "/api/jobs", + loadRoute: () => import("../src/app/api/jobs/route"), + }, + { + path: "/api/ingestion/jobs", + loadRoute: () => import("../src/app/api/ingestion/jobs/route"), + }, + { + path: "/api/ingestion/batches", + loadRoute: () => import("../src/app/api/ingestion/batches/route"), + }, + ])("rejects and redacts malformed status rows from $path", async ({ path, loadRoute }) => { + const client = createSupabaseMock(() => ok([{ status: { sensitive: "patient-secret" } }])); + mockRuntime(client); + const { GET } = await loadRoute(); + + const response = await GET(authenticatedRequest(path)); + const body = await payload(response); + + expect(response.status).toBe(500); + expect(body).toEqual({ + error: "Request failed.", + message: "Request failed.", + code: "internal_error", + }); + expect(JSON.stringify(body)).not.toContain("patient-secret"); + }); + it("validates UUID route params for retry, summarize, and labels endpoints", async () => { const client = createSupabaseMock(); mockRuntime(client);