Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions docs/privacy-impact-assessment.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
8 changes: 6 additions & 2 deletions src/app/api/documents/[id]/route.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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,
Expand Down
5 changes: 4 additions & 1 deletion src/app/api/upload/route.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 });
Expand Down
39 changes: 38 additions & 1 deletion src/lib/audit.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,43 @@ export type AuditLogEntry = {
metadata?: Record<string, unknown>;
};

/**
* 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<string, boolean | number | string> {
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) => {
Expand All @@ -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 });
Expand Down
Original file line number Diff line number Diff line change
@@ -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
))
Comment thread
coderabbitai[bot] marked this conversation as resolved.
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 $$;
70 changes: 70 additions & 0 deletions tests/audit.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
Loading