Audit remediation wave 1: catalog rate limits, public-doc redaction, ingestion recovery#680
Conversation
…ion recovery Addresses the code-level findings from the 2026-07-14 repository audit handover. Security / API: - M4/C1: anonymous curated-catalog GETs (medications, registry, differentials) no longer short-circuit before the limiter. Every caller now resolves access and passes the registry limiter; anonymous requests get a tighter registry allowance. Prevents using the public catalog endpoints as an unauthenticated high-volume egress lever. - S1/D1: authenticated callers can read PUBLIC (owner_id IS NULL) documents they do not own via withOwnerReadScope. The document list and detail routes now redact owner-internal storage fields (storage_path, content_hash, source_path, import_batch_id, error_message) on non-owned rows, and the detail route gates the full owner projection (raw metadata, index health, image storage paths) on ownership rather than mere authentication. - S10/D5: the bulk edit route returned raw per-document DB error text (constraint names, column details). It now logs the sanitized detail server-side and returns a generic per-document failure message. - S11/H6: /api/jobs served unauthenticated demo jobs when the server env was partially missing, masking the misconfiguration. It now fails with a real error. Ingestion: - I1/E1: removed unreachable post-throw code in commitDocumentIndexGeneration and the orphaned client-side deleteStaleIndexGenerationRows fallback. The commit RPC upserts index quality and prunes stale/legacy generation rows atomically server-side; the worker only surfaces failures (fail closed). - I2/E2: buildIngestionRecoveryPlan requeued two jobs for one document when a document had both pending and failed jobs, tripping ingestion_jobs_one_open_per_document_uidx and leaving the queue in a self-perpetuating stall. It now requeues only the first recoverable job per document, supersedes the rest, and orders supersedes before retries so an open sibling is always closed before a retry flips a row to pending. Verification: npm run verify:cheap (lint, typecheck, 2417 tests) passes offline. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U3gX2PtXiEELGtfL7cmCQb
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U3gX2PtXiEELGtfL7cmCQb
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAPI routes now enforce anonymous catalog rate limits and redact non-owner document fields. Bulk and jobs errors are sanitized. Ingestion recovery de-duplicates retries, and worker index commits rely on the atomic RPC. ChangesAPI access and document privacy
API error handling
Ingestion recovery
Worker index commit flow
Review records
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant AnonymousCaller
participant CatalogRoute
participant publicAccessContext
participant RegistryRateLimiter
AnonymousCaller->>CatalogRoute: request public catalog
CatalogRoute->>publicAccessContext: resolve access
publicAccessContext->>RegistryRateLimiter: consume registry subject limit
RegistryRateLimiter-->>CatalogRoute: allow or reject
RegistryRateLimiter-->>CatalogRoute: 429 when limited
CatalogRoute-->>AnonymousCaller: catalog response
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@src/app/api/documents/route.ts`:
- Around line 188-193: Extend the mapping around redactNonOwnedDocumentFields so
nested label and summary rows are also redacted for document IDs not owned by
access.ownerId, preserving ownership information before the map. Apply the
existing public projections/redaction to non-owned nested rows while keeping
owned rows unchanged, and add a targeted regression test covering an
authenticated viewer reading a public document.
In `@src/app/api/medications/route.ts`:
- Around line 108-113: Update the medication, registry-record, and differential
detail handlers to remove anonymous early returns before publicAccessContext, so
known-slug requests always pass the registry limiter before serving payloads.
Reuse the existing createAdminClient and publicAccessContext flow, and add
coverage verifying anonymous detail requests receive 429 responses when
rate-limited.
In `@tests/private-access-routes.test.ts`:
- Around line 556-575: Update the list handler’s per-row redaction logic so
document fields, labels, summaries, and metadata projections are gated by that
row’s ownership rather than the request-wide context. Preserve authenticated
projections for non-owned public rows while excluding owner-only storage and
nested data, and add a mixed-owner includeMeta=true regression test alongside
this case.
🪄 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: 0766e523-2138-41a7-bab6-f201e3b464ea
📒 Files selected for processing (17)
src/app/api/differentials/route.tssrc/app/api/documents/[id]/route.tssrc/app/api/documents/bulk/route.tssrc/app/api/documents/route.tssrc/app/api/jobs/route.tssrc/app/api/medications/route.tssrc/app/api/registry/records/route.tssrc/lib/api-rate-limit.tssrc/lib/ingestion-recovery.tssrc/lib/public-api-access.tstests/document-mutation-routes.test.tstests/ingestion-recovery.test.tstests/medications-route.test.tstests/private-access-routes.test.tstests/registry-records-route.test.tstests/worker-visual-capture.test.tsworker/main.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e686048d4a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex resolve actionable Codex review findings for this pull request and current head using the repository instructions. This is the pull request's single automatic repair pass: do not perform a fresh review, create new standalone findings, or request another review. Work only the existing unresolved Codex threads on the current head. Always fix P0 and P1 findings. For P2 and lower findings, fix only clear, scoped, low-risk issues; otherwise disposition them with a concise reason. After fixing or dispositioning a thread, reply in that thread with as the first line, followed by a concise summary; that marker authorizes the workflow to close that exact thread. If human input or new authorization is required, do not use the marker and leave the thread open with the blocker. Finish only after every actionable thread is fixed or dispositioned and closed, or explicitly left open for a human decision. Do not update the branch from main, address unrelated reviews, broaden scope, or create more than one scoped fix commit. Do not use external APIs, paid services, credentials, dependency changes, or broad refactors unless explicitly authorized. Add targeted tests where behavior changes and run the narrowest relevant validation. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e686048d4a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/reindex.ts (1)
257-270: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winKeep fresh processing siblings in the recovery-plan input.
This filter removes fresh processing jobs before
buildIngestionRecoveryPlancan recognize them as active owners. With a failed sibling, reindex will retry the failed row and may collide with or supersede the in-flight job; the laterhasActiveProcessingJobscheck only skips the worker after recovery has already been applied.Pass every queried status to the planner and let its active/recoverable classification handle filtering.
Proposed fix
const jobs = (data ?? []) .map((job: RawJobRow) => ({ ...job, documents: Array.isArray(job.documents) ? (job.documents[0] as RecoveryDocument | undefined) : (job.documents as RecoveryDocument | undefined), - })) - .filter((job) => { - if (job.status === "pending") { - return true; - } - if (job.status === "failed") { - return true; - } - if (job.status !== "processing") { - return false; - } - if (job.locked_at === null) { - return true; - } - return new Date(job.locked_at) <= staleAfterCutoff; - }); + }));Add a targeted reindex test covering a failed job with a fresh processing sibling. As per coding guidelines, “When behavior changes, add or update the smallest relevant targeted test and run the narrowest relevant validation before broader suites.”
🤖 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 `@scripts/reindex.ts` around lines 257 - 270, Update the job filter before buildIngestionRecoveryPlan to pass every queried job status through, including fresh processing jobs, and let the planner’s active/recoverable classification determine handling. Add a focused reindex test covering a failed job with a fresh processing sibling, verifying the active sibling prevents conflicting recovery.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@scripts/reindex.ts`:
- Around line 257-270: Update the job filter before buildIngestionRecoveryPlan
to pass every queried job status through, including fresh processing jobs, and
let the planner’s active/recoverable classification determine handling. Add a
focused reindex test covering a failed job with a fresh processing sibling,
verifying the active sibling prevents conflicting recovery.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: aa932fce-bc0d-4758-98a8-901ef094d4cb
📒 Files selected for processing (7)
docs/branch-review-ledger.mdscripts/recover-ingestion-queue.tsscripts/reindex.tssrc/app/api/documents/route.tssrc/lib/ingestion-recovery.tstests/ingestion-recovery.test.tstests/private-access-routes.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/private-access-routes.test.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aa161b79ec
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… routes Follow-up to the review of aa161b7. Ingestion recovery (P1, confirmed by CodeRabbit + Codex): - scripts/reindex.ts still filtered out fresh `processing` jobs before calling buildIngestionRecoveryPlan, so a document with a failed row plus a fresh processing retry passed only the failed row to the planner. Applying the resulting retry set the failed row to `pending` while the processing sibling was still open, tripping ingestion_jobs_one_open_per_document_uidx again. The filter is removed; every queried job (pending/processing/failed) now flows to the planner, whose active-vs-recoverable classification keeps the in-flight owner and supersedes only failed/stale siblings. Covered by the planner's "preserves a fresh processing job and supersedes its failed sibling" test. Anonymous catalog detail rate limits (finding C): - The medications, registry-record, differential, and presentation [slug] detail routes still short-circuited to the seed detail before publicAccessContext and the registry limiter, so known-slug requests bypassed rate limiting. They now resolve access and pass the limiter like the list routes. Added anonymous-detail 429 coverage for the medications and registry detail routes. - Removed the now-unused shouldResolvePublicCatalogAccess / hasPublicApiAuthSignal / hasBearerAuthAttempt / hasSessionCookieSignal helpers (no remaining callers). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U3gX2PtXiEELGtfL7cmCQb
…y-review-x0s55l # Conflicts: # docs/branch-review-ledger.md
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/branch-review-ledger.md`:
- Line 557: Update the recovery-selection wording in the ledger entry to state
that the planner prioritizes a recoverable processing job and otherwise falls
back to the first recoverable sibling, replacing the inaccurate claim that it
always picks the first recoverable row.
🪄 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: 073d2f6c-b835-4d7c-bee3-e92966e02266
📒 Files selected for processing (1)
docs/branch-review-ledger.md
… from main) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U3gX2PtXiEELGtfL7cmCQb
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 920ac631ae
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…s55l' into claude/rag-scalability-review-x0s55l # Conflicts: # docs/branch-review-ledger.md
Follow-up to the review of aa161b7/920ac631 (Codex P1). The [id] document detail route fetches the summary with select("*") and, for a non-owner viewing a PUBLIC document, redacted it with omitPublicInternalFields — which did not strip source_chunk_ids, source_image_ids, or model, so summary provenance still leaked. Those keys (present only on document_summaries rows) are now added to the omit set, matching the list route's PUBLIC_SUMMARY projection. Extended the authed-non-owner detail test to assert the summary is readable but its provenance/owner fields are redacted. Also corrected the PR #680 ledger row: catalog detail-route throttling is now actually fixed (not "already fixed"), and the recovery description matches the implemented planner behavior. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U3gX2PtXiEELGtfL7cmCQb
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/private-access-routes.test.ts (1)
567-574: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRedact arbitrary metadata from non-owned list rows.
Line 569 codifies exposing raw
metadatato authenticated non-owners, while the non-owner detail contract below correctly removes it. This reintroduces the prior privacy issue: arbitrary metadata can carry owner-internal provenance or import data. Redact it per row (or return an explicit public allowlist) and assert it is absent here.🤖 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 `@tests/private-access-routes.test.ts` around lines 567 - 574, The non-owner list-row contract currently exposes raw metadata through sharedRow.metadata. Update the list-row serialization or access path used by the private access routes to redact metadata for non-owners, or apply the established public metadata allowlist, while preserving permitted public fields such as id and title. Adjust the assertions around sharedRow to verify metadata is absent or limited to the approved public fields.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@tests/private-access-routes.test.ts`:
- Around line 567-574: The non-owner list-row contract currently exposes raw
metadata through sharedRow.metadata. Update the list-row serialization or access
path used by the private access routes to redact metadata for non-owners, or
apply the established public metadata allowlist, while preserving permitted
public fields such as id and title. Adjust the assertions around sharedRow to
verify metadata is absent or limited to the approved public fields.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e857b850-69a0-4b65-87f6-8b922929a971
📒 Files selected for processing (12)
docs/branch-review-ledger.mdscripts/reindex.tssrc/app/api/differentials/[slug]/route.tssrc/app/api/differentials/presentations/[slug]/route.tssrc/app/api/documents/[id]/route.tssrc/app/api/medications/[slug]/route.tssrc/app/api/registry/records/[slug]/route.tssrc/components/ui-primitives.tsxsrc/lib/public-api-access.tstests/medications-route.test.tstests/private-access-routes.test.tstests/registry-records-route.test.ts
💤 Files with no reviewable changes (1)
- src/lib/public-api-access.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/branch-review-ledger.md
- src/app/api/documents/[id]/route.ts
- scripts/reindex.ts
Follow-up to the review of 72bdff1 (CodeRabbit Major). redactNonOwnedDocumentFields kept on non-owned public rows in the document list, but metadata is arbitrary and can carry owner-internal provenance (bulk-edit author user id, prior titles, indexing internals). Anonymous list callers never receive metadata (PUBLIC_DOCUMENT_LIST_COLUMNS excludes it) and the [id] detail route already strips it, so this closes the last inconsistency: metadata is now redacted for non-owners in the list too. Owned rows are unchanged. Updated the list redaction test to assert metadata is absent. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U3gX2PtXiEELGtfL7cmCQb
2 participants
Summary
Fixes the code-level findings from the 2026-07-14 repository audit handover (
docs/rag-scalability-wip-review-handover/audit-remediation-plan-2026-07-14.md). This is a focused code-hardening wave; operator/legal findings (M1–M3, M5, release evals) are out of scope for code and are not addressed here.Security / API
registrylimiter; a tighter anonymousregistryallowance (60/min) is added.publicAccessContextstill skips the Supabase auth round-trip for truly anonymous callers, so the hot path stays cheap — it just can no longer be used as an unauthenticated high-volume egress lever. The single-record[slug]detail routes are intentionally left as-is (bounded payloads, much weaker lever).withOwnerReadScopereturns PUBLIC (owner_id IS NULL) documents to an authenticated caller alongside their own. The document list route now redacts owner-internal storage fields (storage_path,content_hash,source_path,import_batch_id,error_message) on non-owned rows while keeping shared governancemetadata. The document detail route now gates the full owner projection (raw metadata, index health, image storage paths) on ownership rather than mere authentication, so an authed non-owner gets the same redacted view an anonymous caller already gets. Redaction helpers added tosrc/lib/public-api-access.ts.error.message(constraint names, column details) in itsresultsarray. It now logs the sanitized detail server-side (safeErrorLogDetails) and returns a generic per-document failure message./api/jobsserved unauthenticated demo jobs when the server env was partially missing (isDemoMode()false butcreateAdminClientthrows), masking the misconfiguration. It now surfaces a real error.Ingestion
throwstatements incommitDocumentIndexGenerationand the fully-orphaned client-sidedeleteStaleIndexGenerationRowsfallback (plus its now-unused types). The commit RPC upserts index quality and prunes stale/legacy generation rows atomically server-side (20260702000000+ lease-fenced by20260713062125); the worker only surfaces failures (fail closed). Zero runtime-behavior change. Theworker-visual-capturetest's two lexical assertions that pinned the dead function were replaced with an assertion of the actual fail-closed behavior.pendingand afailedjob,buildIngestionRecoveryPlanemitted tworetryactions and tried to set both rows topending, trippingingestion_jobs_one_open_per_document_uidxand leaving the queue in a self-perpetuating stall that re-crashed on every run. It now requeues only the first recoverable job per document, supersedes the rest, and orders supersedes before retries so an open sibling is always closed before a retry flips a row topending. Fixes both consumers (recover-ingestion-queue.ts,reindex.ts) via the shared planner.I3/E3 (reindex eval gate unwired) was verified to be staged-by-design per
docs/reindex-runbook.md/docs/reindex-shadow-harness-design.mdand is intentionally left unchanged.Verification
npm run verify:cheap— runtime, GitHub-Actions pin, sitemap, brand, type-scale, icon-scale checks, lint, typecheck, and test (2417 passed, 3 skipped) all pass offline.--checkclean on all changed files.npm run verify:pr-local— not run in this environment (adds production build / client-bundle scan). No provider-backed gates were run (API-confirmation boundary).New/updated regression tests: anonymous catalog 429 + limiter-consulted (medications, registry); authed-non-owner public-doc redaction for list + detail, plus owner-keeps-fields; bulk raw-error non-leak; recovery pending+failed dedup/ordering; worker fail-closed assertion.
Clinical Governance Preflight
Clinical KB Database(sjrfecxgysukkwxsowpy) — unchanged (no env change)/api/jobsno longer serves demo jobs into a misconfigured production (S11)Notes
metadataon public rows (badges), the detail view redacts it (owner diagnostics) — both close the flaggedstorage_path/content_hashleak.🤖 Generated with Claude Code
Generated by Claude Code
Summary by CodeRabbit