diff --git a/docs/privacy-impact-assessment.md b/docs/privacy-impact-assessment.md index 70c8df18e..a8b5504d5 100644 --- a/docs/privacy-impact-assessment.md +++ b/docs/privacy-impact-assessment.md @@ -193,12 +193,12 @@ of the most important privacy items before real patient use (PIA-1). All three log tables are **owner-stamped** and **RLS-enabled** (owner-read for authenticated users; service-role for writes). Redaction is applied centrally at every write site. -| Table | Raw query stored? | Redaction mechanism | Other sensitive columns | RLS | -| -------------------- | --------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------- | --------------------------------------------------------- | -| `rag_queries` | No (hash placeholder) | `queryTextForStorage` / `normalizedQueryTextForStorage` ([query-privacy.ts:33-39](src/lib/query-privacy.ts)); centralized write in `insertRagQuery` | `answer` is null by default and stored only with explicit `RAG_PERSIST_ANSWER_TEXT=true`; `source_chunk_ids` (own data) | owner-read, [schema.sql:3932](supabase/schema.sql) | -| `rag_query_misses` | No (hash placeholder) | same helpers; writes in [search/route.ts:558-559](src/app/api/search/route.ts), [interaction/route.ts:88-89](src/app/api/search/interaction/route.ts) | `metadata.query_hash` | owner-read, [schema.sql:3935](supabase/schema.sql) | -| `rag_retrieval_logs` | No (hash placeholder) | same helpers; write at [search/route.ts:556-559](src/app/api/search/route.ts) | retrieval telemetry only | owner-read, [schema.sql:3938](supabase/schema.sql) | -| `audit_logs` | N/A (no query text) | action/resource metadata only; error strings pass through `redactLogValue` ([privacy.ts:5-31](src/lib/privacy.ts)) | `owner_id`, `action`, `resource_id` | service-role-only, [schema.sql:3959](supabase/schema.sql) | +| Table | Raw query stored? | Redaction mechanism | Other sensitive columns | RLS | +| -------------------- | --------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------- | --------------------------------------------------------- | +| `rag_queries` | No (hash placeholder) | `queryTextForStorage` / `normalizedQueryTextForStorage` ([query-privacy.ts:33-39](src/lib/query-privacy.ts)); centralized write in `insertRagQuery` | `answer` is null by default and stored only with explicit `RAG_PERSIST_ANSWER_TEXT=true`; `source_chunk_ids` (own data) | owner-read, [schema.sql:3932](supabase/schema.sql) | +| `rag_query_misses` | No (hash placeholder) | same helpers; writes in [search/route.ts:558-559](src/app/api/search/route.ts), [interaction/route.ts:88-89](src/app/api/search/interaction/route.ts) | `metadata.query_hash` | owner-read, [schema.sql:3935](supabase/schema.sql) | +| `rag_retrieval_logs` | No (hash placeholder) | same helpers; write at [search/route.ts:556-559](src/app/api/search/route.ts) | retrieval telemetry only | owner-read, [schema.sql:3938](supabase/schema.sql) | +| `audit_logs` | N/A (no query text) | action/resource metadata only; the write boundary allowlists operational metadata and excludes user-controlled filenames/titles/content hashes ([audit.ts](../src/lib/audit.ts)). Migration `20260717163000` minimizes existing rows on deployment. | `owner_id`, `action`, `resource_id` | service-role-only, [schema.sql:3959](supabase/schema.sql) | ### 5.1 M15 HMAC query-hash fix — verified present, enforced in production diff --git a/src/app/api/documents/[id]/route.ts b/src/app/api/documents/[id]/route.ts index abaa39321..a78a3c7f2 100644 --- a/src/app/api/documents/[id]/route.ts +++ b/src/app/api/documents/[id]/route.ts @@ -178,7 +178,9 @@ export async function PATCH(request: Request, { params }: { params: Promise<{ id action: "document_rename", resourceType: "document", resourceId: id, - metadata: { previousTitle: document.title, newTitle: body.title }, + // Audit the rename event and resource without retaining user-controlled + // titles indefinitely in the service-role audit log. + metadata: {}, }); return NextResponse.json({ document: updated }); } catch (error) { @@ -241,7 +243,9 @@ export async function DELETE(request: Request, { params }: { params: Promise<{ i action: "document_delete", resourceType: "document", resourceId: id, - metadata: { title: result.document_title, storageRemoved: cleanup.storageRemoved }, + // The deleted title can contain patient information. Retain only the + // operational cleanup result alongside the resource id and action. + metadata: { storageRemoved: cleanup.storageRemoved }, }); return NextResponse.json({ deleted: true, diff --git a/src/app/api/upload/route.ts b/src/app/api/upload/route.ts index 8ff02fda8..cbcfc9cd9 100644 --- a/src/app/api/upload/route.ts +++ b/src/app/api/upload/route.ts @@ -307,7 +307,10 @@ export async function POST(request: Request) { action: "document_upload", resourceType: "document", resourceId: documentId, - metadata: { fileName: file.name, fileType: file.type, fileSize: file.size, contentHash }, + // `audit_logs` is retained indefinitely. Keep only operational facts there; + // the user-controlled filename and content hash remain on the scoped document + // record, not in the durable audit trail. + metadata: { fileType: file.type, fileSize: file.size }, }); return NextResponse.json({ document, job }, { status: 201 }); diff --git a/src/lib/audit.ts b/src/lib/audit.ts index 8725ea4f5..9240f1904 100644 --- a/src/lib/audit.ts +++ b/src/lib/audit.ts @@ -15,6 +15,43 @@ export type AuditLogEntry = { metadata?: Record; }; +/** + * Audit rows are retained indefinitely, so their metadata must be an operational + * event summary rather than a second copy of document metadata. In particular, + * upload filenames and rename/delete titles are user-controlled and can contain + * patient identifiers. Keep this allowlist at the durable-write boundary so a + * future caller cannot accidentally reintroduce free text into `audit_logs`. + */ +function minimumAuditMetadata(entry: AuditLogEntry): Record { + const metadata = entry.metadata ?? {}; + + switch (entry.action) { + case "document_upload": { + const fileType = typeof metadata.fileType === "string" ? metadata.fileType : undefined; + const fileSize = + typeof metadata.fileSize === "number" && Number.isFinite(metadata.fileSize) ? metadata.fileSize : undefined; + return { + ...(fileType ? { fileType } : {}), + ...(fileSize !== undefined ? { fileSize } : {}), + }; + } + case "document_delete": { + // DELETE routes pass a removed-object count; older callers may pass a boolean. + const storageRemoved = metadata.storageRemoved; + if (typeof storageRemoved === "number" && Number.isFinite(storageRemoved)) { + return { storageRemoved }; + } + if (typeof storageRemoved === "boolean") { + return { storageRemoved }; + } + return {}; + } + case "document_rename": + case "document_label_change": + return {}; + } +} + // Minimal structural type so we do not couple to the full Supabase client type. type AuditInsertClient = { from: (table: string) => { @@ -29,7 +66,7 @@ export async function writeAuditLog(supabase: AuditInsertClient, entry: AuditLog action: entry.action, resource_type: entry.resourceType ?? null, resource_id: entry.resourceId ?? null, - metadata: entry.metadata ?? {}, + metadata: minimumAuditMetadata(entry), }); if (error) { logger.error("Audit log write failed", { action: entry.action, message: error.message }); diff --git a/supabase/migrations/20260717163000_minimize_audit_log_metadata.sql b/supabase/migrations/20260717163000_minimize_audit_log_metadata.sql new file mode 100644 index 000000000..716708afc --- /dev/null +++ b/supabase/migrations/20260717163000_minimize_audit_log_metadata.sql @@ -0,0 +1,33 @@ +-- Audit logs are retained indefinitely for clinical governance. Historical writes +-- included user-controlled document names/titles and content hashes in metadata, +-- which could preserve patient identifiers after the source document was renamed +-- or deleted. Keep the immutable event/resource record but remove free text and +-- retain only operational facts needed for audit triage. + +update public.audit_logs +set metadata = case action + when 'document_upload' then jsonb_strip_nulls(jsonb_build_object( + 'fileType', case when jsonb_typeof(metadata -> 'fileType') = 'string' then metadata -> 'fileType' end, + 'fileSize', case when jsonb_typeof(metadata -> 'fileSize') = 'number' then metadata -> 'fileSize' end + )) + when 'document_delete' then jsonb_strip_nulls(jsonb_build_object( + 'storageRemoved', case + when jsonb_typeof(metadata -> 'storageRemoved') in ('number', 'boolean') then metadata -> 'storageRemoved' + end + )) + else '{}'::jsonb +end +where metadata <> '{}'::jsonb; + +do $$ +declare + unsafe_metadata_count integer; +begin + select count(*) into unsafe_metadata_count + from public.audit_logs + where metadata ?| array['fileName', 'contentHash', 'previousTitle', 'newTitle', 'title']; + + if unsafe_metadata_count > 0 then + raise exception 'audit-log metadata minimization incomplete: % rows still contain user-controlled document text', unsafe_metadata_count; + end if; +end $$; diff --git a/tests/audit.test.ts b/tests/audit.test.ts index 2362fddb3..365a23be7 100644 --- a/tests/audit.test.ts +++ b/tests/audit.test.ts @@ -43,6 +43,76 @@ describe("writeAuditLog", () => { }); }); + it("drops user-controlled document text from indefinitely retained audit metadata", async () => { + const client = mockClient(async () => ({ error: null })); + + await writeAuditLog(client, { + ownerId: "owner-1", + action: "document_upload", + resourceId: "doc-1", + metadata: { + fileName: "Jane Doe MRN 123456.pdf", + contentHash: "content-derived-identifier", + fileType: "application/pdf", + fileSize: 1234, + nested: { title: "Patient Jane Doe" }, + }, + }); + + expect(client.insertSpy).toHaveBeenCalledWith( + expect.objectContaining({ metadata: { fileType: "application/pdf", fileSize: 1234 } }), + ); + expect(JSON.stringify(client.insertSpy.mock.calls[0]?.[0])).not.toContain("Jane Doe"); + expect(JSON.stringify(client.insertSpy.mock.calls[0]?.[0])).not.toContain("content-derived-identifier"); + }); + + it("does not retain previous or replacement titles for rename events", async () => { + const client = mockClient(async () => ({ error: null })); + + await writeAuditLog(client, { + ownerId: "owner-1", + action: "document_rename", + resourceId: "doc-1", + metadata: { previousTitle: "Jane Doe", newTitle: "Patient Smith" }, + }); + + expect(client.insertSpy).toHaveBeenCalledWith(expect.objectContaining({ metadata: {} })); + }); + + it("retains numeric storageRemoved counts from document deletes", async () => { + const client = mockClient(async () => ({ error: null })); + + await writeAuditLog(client, { + ownerId: "owner-1", + action: "document_delete", + resourceId: "doc-1", + metadata: { + storageRemoved: 2, + title: "Jane Doe MRN 123456.pdf", + }, + }); + + expect(client.insertSpy).toHaveBeenCalledWith( + expect.objectContaining({ metadata: { storageRemoved: 2 } }), + ); + expect(JSON.stringify(client.insertSpy.mock.calls[0]?.[0])).not.toContain("Jane Doe"); + }); + + it("retains boolean storageRemoved flags from legacy delete callers", async () => { + const client = mockClient(async () => ({ error: null })); + + await writeAuditLog(client, { + ownerId: "owner-1", + action: "document_delete", + resourceId: "doc-1", + metadata: { storageRemoved: true }, + }); + + expect(client.insertSpy).toHaveBeenCalledWith( + expect.objectContaining({ metadata: { storageRemoved: true } }), + ); + }); + it("does not throw when the insert returns an error", async () => { const client = mockClient(async () => ({ error: { message: "insert failed" } })); await expect(writeAuditLog(client, { ownerId: "o", action: "document_rename" })).resolves.toBeUndefined();