fix: minimize durable audit metadata#753
Closed
BigSimmo wants to merge 4 commits into
Closed
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAudit logging now enforces action-specific metadata allowlists, removes sensitive fields from document-related call sites, minimizes existing audit rows through migration, and adds regression tests and privacy documentation. ChangesAudit Metadata Privacy Hardening
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Updates to Preview Branch (codex/review-and-fix-top-security-defects) ↗︎
Tasks are run on every commit but only new migration files are pushed.
❌ Branch Error • Fri, 17 Jul 2026 19:56:53 UTC View logs for this Workflow Run ↗︎. |
CI triageCI failed on this PR. Automated classification of the 2 failed job(s):
Compared with main CI run #2957 (success). Classification is evidence routing, not permission to ignore a failure. Exact quarantined Playwright identities remain governed by the flake ledger. |
BigSimmo
marked this pull request as ready for review
July 17, 2026 19:36
BigSimmo
enabled auto-merge
July 17, 2026 19:36
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).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/privacy-impact-assessment.md`:
- Line 201: Update the migration reference in the audit_logs row to use
20260717174000 instead of 20260717170000, matching the actual
minimize-audit-log-metadata migration while leaving the surrounding privacy
assessment content unchanged.
In `@src/lib/audit.ts`:
- Around line 38-39: Update the document_delete handling in the metadata
normalization logic to accept finite, non-negative numeric storageRemoved values
in addition to booleans, preserving the numeric cleanup count in the returned
object. Add a regression test covering a nonzero numeric value and retain the
empty-object behavior for unsupported values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0a7d9490-42b2-4c56-9dcc-8ab5abf6d25c
📒 Files selected for processing (7)
docs/branch-review-ledger.mddocs/privacy-impact-assessment.mdsrc/app/api/documents/[id]/route.tssrc/app/api/upload/route.tssrc/lib/audit.tssupabase/migrations/20260717174000_minimize_audit_log_metadata.sqltests/audit.test.ts
| | `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) | |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the migration reference from 20260717170000 to 20260717174000.
The privacy assessment references migration 20260717170000, but the actual migration file is 20260717174000_minimize_audit_log_metadata.sql. The timestamp was changed to avoid a collision with an existing migration on main. This stale reference could mislead operators during deployment or audit trail review.
📝 Proposed fix
-| `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) |
+| `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 `20260717174000` minimizes existing rows on deployment. | `owner_id`, `action`, `resource_id` | service-role-only, [schema.sql:3959](supabase/schema.sql) |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| | `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) | | |
| | `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 `20260717174000` minimizes existing rows on deployment. | `owner_id`, `action`, `resource_id` | service-role-only, [schema.sql:3959](supabase/schema.sql) | |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/privacy-impact-assessment.md` at line 201, Update the migration
reference in the audit_logs row to use 20260717174000 instead of 20260717170000,
matching the actual minimize-audit-log-metadata migration while leaving the
surrounding privacy assessment content unchanged.
Comment on lines
+38
to
+39
| case "document_delete": | ||
| return typeof metadata.storageRemoved === "boolean" ? { storageRemoved: metadata.storageRemoved } : {}; |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve the numeric delete-cleanup result.
Line 39 accepts only booleans, but the DELETE route passes numeric cleanup.storageRemoved; every delete audit row therefore stores {} instead of the required operational result. Accept a finite non-negative count and add a regression for a nonzero value.
Proposed fix
case "document_delete":
- return typeof metadata.storageRemoved === "boolean" ? { storageRemoved: metadata.storageRemoved } : {};
+ return typeof metadata.storageRemoved === "number" &&
+ Number.isSafeInteger(metadata.storageRemoved) &&
+ metadata.storageRemoved >= 0
+ ? { storageRemoved: metadata.storageRemoved }
+ : {};Also applies to: 60-60
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/audit.ts` around lines 38 - 39, Update the document_delete handling
in the metadata normalization logic to accept finite, non-negative numeric
storageRemoved values in addition to booleans, preserving the numeric cleanup
count in the returned object. Add a regression test covering a nonzero numeric
value and retain the empty-object behavior for unsupported values.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
audit_logsmetadata to avoid durable patient-identifiable artifacts.Description
minimumAuditMetadatainsrc/lib/audit.ts) so only action-specific operational fields are retained (upload:fileType/fileSize; delete:storageRemoved; rename/label-change: no metadata).src/app/api/upload/route.ts,src/app/api/documents/[id]/route.ts).supabase/migrations/20260717170000_minimize_audit_log_metadata.sqlto minimize historical audit rows by stripping free-text metadata when applied to the database.tests/audit.test.ts).docs/privacy-impact-assessment.md,docs/branch-review-ledger.md).Testing
npm audit --omit=dev --offline; result: passed (0 vulnerabilities).git diff --checkand staged-diff validation; result: passed (no whitespace/check failures).tests/audit.test.ts) that assert sensitive fields are omitted; local focused test runner could not be executed because the worktree is missing dev runtime dependencies (Vitest/TypeScript/Next build artifacts), so Vitest-based tests were not executed here.npm run verify:cheap/ full local gates; these could not complete in this environment due to missingtsxand other local dev dependencies and build output, so full unit/build/e2e verification and client-bundle secret scan were not run.Notes and follow-ups
Codex Task
Summary by CodeRabbit
Privacy & Security
Documentation
Tests