From 59a7aaf5c1815285eda324758226fa159a047bac Mon Sep 17 00:00:00 2001 From: BigSimmo <87357024+BigSimmo@users.noreply.github.com> Date: Sat, 18 Jul 2026 03:26:40 +0800 Subject: [PATCH 1/3] fix: minimize durable audit metadata --- docs/branch-review-ledger.md | 1 + docs/privacy-impact-assessment.md | 12 +++---- src/app/api/documents/[id]/route.ts | 8 +++-- src/app/api/upload/route.ts | 5 ++- src/lib/audit.ts | 30 +++++++++++++++- ...0717170000_minimize_audit_log_metadata.sql | 31 ++++++++++++++++ tests/audit.test.ts | 36 +++++++++++++++++++ 7 files changed, 113 insertions(+), 10 deletions(-) create mode 100644 supabase/migrations/20260717170000_minimize_audit_log_metadata.sql diff --git a/docs/branch-review-ledger.md b/docs/branch-review-ledger.md index 43cd46874..2b5610ed2 100644 --- a/docs/branch-review-ledger.md +++ b/docs/branch-review-ledger.md @@ -578,3 +578,4 @@ Use this ledger to prevent repeated branch and PR reviews when the reviewed HEAD | 2026-07-17 | PR #716 / codex/pr-process-hardening-20260717 | 220de891f10f82df11eb2e8137367514c139a206 + reviewed implementation/follow-up diff | PR metadata, CI triage, review routing, Actions permissions, secret scanning, and branch-hygiene hardening | No remaining high-confidence P0-P2 defect in the reviewed process diff. Added a trusted base-SHA PR metadata/risk policy with draft and merge-queue handling; corrected CI triage to compare only the same workflow's latest completed `main` run; wired self-tests into local/hosted gates; documented the policy; created the missing durable Codex routing labels; removed four unused per-head routing labels; and applied read-only default Actions tokens, immutable Action pinning, no Actions-authored approvals, push protection, and automatic merged-branch deletion. Four policy defects were fixed before merge: API-only paths no longer trigger UI evidence, slash-bearing outcome titles are accepted, all seven governance items are required exactly, and placeholder risk/rollback text is rejected. | Offline `check:pr-policy`, `check:ci-triage`, `check:ci-scope`, action-pin check, scoped Prettier, and `git diff --check` passed. Initial hosted CI passed static, safety, coverage, build, images, Semgrep, Gitleaks, GitGuardian, and the aggregate gate; exact-head CI is required after the review fix. Broader local gates were deferred because other registered worktrees repeatedly held the heavyweight lock. GitHub provider inspection/settings changes were explicitly authorized. No Supabase/OpenAI/product-provider command ran. | | 2026-07-17 | codex/mobile-search-phone-refresh-20260717 (supersedes PR #700) | 42a3e3ce65dc5a0e1dce386e0b91fccd23d13d6c + reviewed follow-up diff | phone universal-search command-panel recovery and merge-readiness review | Recovered the still-useful behavior from PR #700 onto current `main`, including its hydration fix and wide-touch regression coverage. Hosted Production UI then exposed one desktop focus race: capability state intentionally initializes false for hydration safety, but an input could receive focus before the post-hydration effect synchronized the real browser state. The follow-up recomputes the same guarded predicate synchronously on focus; it requires the placement breakpoint plus either a fine pointer or a zero-touch desktop fallback, so wide touch devices remain suppressed while desktop keeps the first command-panel interaction. No remaining high-confidence P0-P2 defect was found in the scoped diff. | Focused Vitest 7/7; `npm run ensure` verified the project at `http://localhost:3751`; hosted static, safety, coverage, build, advisory UI, Semgrep, Gitleaks, and GitGuardian passed; the first hosted Production UI run isolated the nine desktop regressions. The focused browser proof reproduced the desktop race while the wide-touch regression passed, and exact-head hosted Production UI remains required after the focus fix. `format:changed -- --check` and `git diff --check` passed before the final follow-up. No Supabase/OpenAI/product-provider command ran. | | 2026-07-17 | final historical branch/worktree cleanup against `origin/main` | 5d195d7ca8752b2ae4006725c6b145c5662bb687 | branch-cleanup | Merged PRs #716 and #717, closed superseded PR #700, and removed the three clean task worktrees. Deleted exact remote refs for `codex/mobile-search-phone-fix-20260717` (`590f32b73`), `claude/audit-findings-review-phgz92` (`ea3b8f95b8`), `codex/chat-forms-import-6914` (`b05da82f82`), `codex/chat-supabase-migration-preflight-b463` (`1ef0faee95`), and `codex/dsm-diagnosis-mode` (`f6cda83ca6`); the merged #716/#717 branches were deleted automatically. Deleted 14 unregistered local refs only after direct-main ancestry, exact ledger deletion-pending proof, or exact merged-PR commit provenance. Final inventory found zero remote branches without an open PR or registered worktree, zero locally merged or exact deletion-pending orphan refs, and no retired target refs or paths. Thirteen non-ancestor local refs and 25 registered worktrees remain preserved because they are backups, patch-unique/unresolved, open-PR-owned, or ownership could not be safely disproved. | Fresh fetch/prune; exact GitHub PR/head/merge associations; cherry-pick-aware logs; DSM PR #661 exact commit/file provenance; exact leased remote deletes; exact-old-value local `update-ref` deletes; clean-worktree and path/process checks; worktree prune; final zero-orphan inventory. The Codex task registry lookup timed out, so ambiguous registered worktrees were conservatively retained. No Supabase, OpenAI, production-data, or live clinical workflow ran. | +| 2026-07-17 | codex/security-governance-hardening-20260717 | bfa0ed3db0c64a4ca860c83d05024e1c7b29151e | security, privacy, and clinical-governance attack-path review | Confirmed P1 privacy defect: service-role-only `audit_logs` are intentionally retained indefinitely, but upload, rename, and delete paths persisted user-controlled filenames/titles and content hashes in JSON metadata. A patient identifier in a document name therefore survived document deletion in a durable governance log. Fixed the durable write boundary with an action-specific metadata allowlist, removed sensitive call-site fields, added a migration to minimize existing rows when deployed, and added regression coverage. No P0 cross-owner, service-role exposure, signed-URL, or prompt-injection policy bypass was reproduced in the reviewed scope. | Local source/sink inspection; focused test command could not run because the worktree has no Vitest/TypeScript/Next dependencies; scoped Prettier and `git diff --check` passed. Client-bundle scan requires a production `.next/static` build and was not runnable. Provider-backed Supabase/OpenAI/production-readiness checks were not run under the confirmation boundary. | diff --git a/docs/privacy-impact-assessment.md b/docs/privacy-impact-assessment.md index 70c8df18e..1d0419e25 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 `20260717170000` 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 87bdf5f18..532728f79 100644 --- a/src/app/api/documents/[id]/route.ts +++ b/src/app/api/documents/[id]/route.ts @@ -424,7 +424,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) { @@ -487,7 +489,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..97afc5736 100644 --- a/src/lib/audit.ts +++ b/src/lib/audit.ts @@ -15,6 +15,34 @@ 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": + return typeof metadata.storageRemoved === "boolean" ? { storageRemoved: metadata.storageRemoved } : {}; + 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 +57,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/20260717170000_minimize_audit_log_metadata.sql b/supabase/migrations/20260717170000_minimize_audit_log_metadata.sql new file mode 100644 index 000000000..3ffa983b7 --- /dev/null +++ b/supabase/migrations/20260717170000_minimize_audit_log_metadata.sql @@ -0,0 +1,31 @@ +-- 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') = '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..851b3593e 100644 --- a/tests/audit.test.ts +++ b/tests/audit.test.ts @@ -43,6 +43,42 @@ 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("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(); From a96567eceb0193f6d52e82d28f8700e96dcdbb47 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Fri, 17 Jul 2026 19:54:19 +0000 Subject: [PATCH 2/3] fix: rename migration to avoid timestamp collision with main 20260717170000_minimize_audit_log_metadata.sql conflicted with 20260717170000_registry_projection_cleanup.sql already on main. Supabase CLI uses the numeric timestamp prefix as the schema_migrations primary key, so two files sharing the same prefix cause a duplicate-key error during migration replay CI. Rename to 20260717174000 (next safe slot after 20260717173000). --- ...etadata.sql => 20260717174000_minimize_audit_log_metadata.sql} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename supabase/migrations/{20260717170000_minimize_audit_log_metadata.sql => 20260717174000_minimize_audit_log_metadata.sql} (100%) diff --git a/supabase/migrations/20260717170000_minimize_audit_log_metadata.sql b/supabase/migrations/20260717174000_minimize_audit_log_metadata.sql similarity index 100% rename from supabase/migrations/20260717170000_minimize_audit_log_metadata.sql rename to supabase/migrations/20260717174000_minimize_audit_log_metadata.sql From 0c6932b752892cf1c1518f26d659889a68dc481d Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Fri, 17 Jul 2026 19:56:01 +0000 Subject: [PATCH 3/3] docs: record CI fix review in branch-review-ledger --- docs/branch-review-ledger.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/docs/branch-review-ledger.md b/docs/branch-review-ledger.md index 5bf1c2944..cd61c55cb 100644 --- a/docs/branch-review-ledger.md +++ b/docs/branch-review-ledger.md @@ -584,3 +584,15 @@ Use this ledger to prevent repeated branch and PR reviews when the reviewed HEAD | 2026-07-17 | codex/mobile-search-phone-refresh-20260717 (supersedes PR #700) | 42a3e3ce65dc5a0e1dce386e0b91fccd23d13d6c + reviewed follow-up diff | phone universal-search command-panel recovery and merge-readiness review | Recovered the still-useful behavior from PR #700 onto current `main`, including its hydration fix and wide-touch regression coverage. Hosted Production UI then exposed one desktop focus race: capability state intentionally initializes false for hydration safety, but an input could receive focus before the post-hydration effect synchronized the real browser state. The follow-up recomputes the same guarded predicate synchronously on focus; it requires the placement breakpoint plus either a fine pointer or a zero-touch desktop fallback, so wide touch devices remain suppressed while desktop keeps the first command-panel interaction. No remaining high-confidence P0-P2 defect was found in the scoped diff. | Focused Vitest 7/7; `npm run ensure` verified the project at `http://localhost:3751`; hosted static, safety, coverage, build, advisory UI, Semgrep, Gitleaks, and GitGuardian passed; the first hosted Production UI run isolated the nine desktop regressions. The focused browser proof reproduced the desktop race while the wide-touch regression passed, and exact-head hosted Production UI remains required after the focus fix. `format:changed -- --check` and `git diff --check` passed before the final follow-up. No Supabase/OpenAI/product-provider command ran. | | 2026-07-17 | final historical branch/worktree cleanup against `origin/main` | 5d195d7ca8752b2ae4006725c6b145c5662bb687 | branch-cleanup | Merged PRs #716 and #717, closed superseded PR #700, and removed the three clean task worktrees. Deleted exact remote refs for `codex/mobile-search-phone-fix-20260717` (`590f32b73`), `claude/audit-findings-review-phgz92` (`ea3b8f95b8`), `codex/chat-forms-import-6914` (`b05da82f82`), `codex/chat-supabase-migration-preflight-b463` (`1ef0faee95`), and `codex/dsm-diagnosis-mode` (`f6cda83ca6`); the merged #716/#717 branches were deleted automatically. Deleted 14 unregistered local refs only after direct-main ancestry, exact ledger deletion-pending proof, or exact merged-PR commit provenance. Final inventory found zero remote branches without an open PR or registered worktree, zero locally merged or exact deletion-pending orphan refs, and no retired target refs or paths. Thirteen non-ancestor local refs and 25 registered worktrees remain preserved because they are backups, patch-unique/unresolved, open-PR-owned, or ownership could not be safely disproved. | Fresh fetch/prune; exact GitHub PR/head/merge associations; cherry-pick-aware logs; DSM PR #661 exact commit/file provenance; exact leased remote deletes; exact-old-value local `update-ref` deletes; clean-worktree and path/process checks; worktree prune; final zero-orphan inventory. The Codex task registry lookup timed out, so ambiguous registered worktrees were conservatively retained. No Supabase, OpenAI, production-data, or live clinical workflow ran. | | 2026-07-17 | codex/security-governance-hardening-20260717 | bfa0ed3db0c64a4ca860c83d05024e1c7b29151e | security, privacy, and clinical-governance attack-path review | Confirmed P1 privacy defect: service-role-only `audit_logs` are intentionally retained indefinitely, but upload, rename, and delete paths persisted user-controlled filenames/titles and content hashes in JSON metadata. A patient identifier in a document name therefore survived document deletion in a durable governance log. Fixed the durable write boundary with an action-specific metadata allowlist, removed sensitive call-site fields, added a migration to minimize existing rows when deployed, and added regression coverage. No P0 cross-owner, service-role exposure, signed-URL, or prompt-injection policy bypass was reproduced in the reviewed scope. | Local source/sink inspection; focused test command could not run because the worktree has no Vitest/TypeScript/Next dependencies; scoped Prettier and `git diff --check` passed. Client-bundle scan requires a production `.next/static` build and was not runnable. Provider-backed Supabase/OpenAI/production-readiness checks were not run under the confirmation boundary. | + +--- + +## Review: codex/review-and-fix-top-security-defects (CI fix pass) + +- **Date**: 2026-07-17 +- **Branch**: codex/review-and-fix-top-security-defects +- **HEAD**: a96567eceb0193f6d52e82d28f8700e96dcdbb47 +- **Scope**: pr-ci-fix +- **Outcome**: Fixed migration replay CI failure. Renamed migration `20260717170000_minimize_audit_log_metadata.sql` → `20260717174000_minimize_audit_log_metadata.sql` to eliminate timestamp collision with `20260717170000_registry_projection_cleanup.sql` already on main. Committed and pushed. +- **Checks run**: local pr-policy.mjs validation (pass); npm run lint (pass) +- **Remaining**: PR Policy CI still requires PR body update (## Summary, ## Verification, ## Clinical Governance Preflight, ## Risk and rollout sections) — Cloud Agent token lacks PR write permission; user must update PR body manually.