issues: queue the #Z61JRT public-bucket RLS correction - #2563
Conversation
The archived #Z61JRT close outcome asserts that flipping the clinical-images bucket to public would still not produce a working read path, because the "image storage owner read" RLS policy keys on the first path segment equalling auth.uid(). That is wrong, and wrong in the unsafe direction. The installed client documents getPublicUrl as requiring "buckets table permissions: none" and "objects table permissions: none" (node_modules/@supabase/storage-js/src/packages/StorageFileApi.ts). A public bucket serves its objects with no RLS evaluated and no token, so flipping clinical-images would make every object anonymously readable by URL — the exact hazard the original row existed to warn about. Archived rows are immutable history: updateIssue refuses them and check:ledger-write-discipline rejects direct table-row edits. The correction is therefore filed as a new open P2 row that quotes the erroneous sentence, gives the evidence, and states that the archived outcome must be read with it. No code fix is owed. The bucket was not flipped — a read-only query on 2026-09-02 returned public = false for both buckets, and 20260717139000_create_storage_buckets.sql re-asserts that on every replay — and the shipped route signs uniformly via createSignedUrls, with zero getPublicUrl matches repo-wide. Raised by the Codex reviewer on PR #2559 and verified against the installed source rather than accepted on assertion. This PR only queues the request. Reconciliation happens in a later run, as the write-discipline gate requires. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AcjoJHKAPfBPKYgwTF3wTS
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_6381a61a-d704-4488-8e91-3a7443aeca3d) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: abd3d48c6e
ℹ️ 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".
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_61abe0a3-1653-45a7-a5e5-39092a345f09) |
Summary
Queues one correction request. The archived
#Z61JRTclose outcome carries a security claim that is wrong in the unsafe direction, and this files the correction of record against it.The error. That outcome states: "Flipping the bucket would also not have produced a working read path: the storage RLS policy 'image storage owner read' keys on the first path segment equalling auth.uid(), which a published ownerless document never satisfies, so signed URLs minted with the service-role key are the only mechanism that serves public-corpus figures."
Why it is wrong. The owner-folder policy on
storage.objectsgoverns authenticated reads of a private bucket and has no bearing on a public one. The installed client documentsgetPublicUrlatnode_modules/@supabase/storage-js/src/packages/StorageFileApi.tsas requiringbucketstable permissions: none andobjectstable permissions: none. A public bucket therefore serves its objects to anyone holding the URL, with no RLS evaluated and no token. Ifclinical-imageswere ever flipped to public, every object in it would become anonymously readable — which is precisely the hazard#Z61JRTexisted to record. The archived text inverts that risk and would invite a reader to believe a bucket flip is inert.Why it is filed as a new row rather than edited. Archived rows are immutable history here:
updateIssuerefuses them outright, andcheck:ledger-write-disciplinerejects direct table-row edits and any canonical diff that does not match its recorded reconciliation transaction. Deleting the sentence would also destroy the audit trail the ledger exists to keep. So the correction is a new open P2 row that quotes the erroneous sentence, gives the evidence, and states explicitly that anyone reading the archived outcome must read the correction with it.A second error, caught in review and fixed before reconciliation. The first draft of this row asserted that a manual dashboard flip would be "reverted by the next migration run". That was also false, and false in the same containment-overstating direction as the error the row exists to correct.
scripts/check-migration-history-alignment.tscomputeslocalOnly = [...local].filter((version) => !remote.has(version))and labels exactly that set "pending apply", so a version already recorded in remote history never runs again;20260717139000'son conflict (id) do update set public = falsefires only when that migration itself executes — a fresh replay such as a localdb resetor the CI migration-replay job — not against the live project on an ordinary deploy. Fixed infd6b5aecb, so the ledger never carries it. The row now records both errors and how each was caught.The real control is detection, not prevention. If
clinical-imageswere flipped in the dashboard it would stay public until a person restored it.schema_drift_snapshotcapturesstorage_buckets, sonpm run check:driftand the post-mergelive-driftworkflow would surface the changedpublicvalue as drift — butcheck:driftonly reports and repairs nothing. Remediation is manual: flip it back, or ship a NEW migration re-assertingpublic = false, since only a new version would be pending and therefore actually execute.No code fix is owed, and nothing is exposed. The bucket was not flipped: a user-authorised read-only query on 2026-09-02 returned
storage.buckets.public = falsefor bothclinical-imagesandclinical-documents. The shipped route change is unaffected — it signs uniformly throughcreateSignedUrls, never callsgetPublicUrl, and a repo-wide grep forgetPublicUrlreturns zero matches. What was wrong was the stated reason a bucket flip would be harmless, not the conclusion that the bucket must stay private.Provenance. Both errors were raised as P2 findings by the Codex reviewer — the first on #2559, the second on this PR — and each was verified against the repository's own source (
storage-jsremarks;check-migration-history-alignment.ts) rather than accepted on assertion. The original claim came from a schema review earlier in the same session and was carried into the close outcome unchecked.This PR only queues the request; it does not touch the canonical ledger. Reconciliation happens in a later run, as the write-discipline gate requires (a request must land as pending in one PR and be applied in a later one).
Verification
npm run verify:pr-localDecisive lines:
Ledger write discipline passed for f74715b6fcf6..HEADand[snapshot] in step with data/outstanding-issues-snapshot.json. Re-run after thefd6b5aecbrewrite and still clean. Scope was classified as recognised low-risk, so lint, typecheck, the unit suite and the build were skipped by the router rather than by choice.npm run formatreported every file unchanged.npm run verify:releasenot run: not a release or handoff confidence claim.Risk and rollout
docs/outstanding-issues-inbox/. No source, schema, migration, workflow, or configuration file is touched, and the canonical ledger is not edited.docs/outstanding-issues.md, so no reconciliation needs undoing.selectstatements only; this PR runs nothing against any provider.src/lib/rag/, no retrieval RPC, no ranking configuration, no golden fixture and no ranking test is touched.Clinical Governance Preflight
The diff queues one ledger request and does not touch ingestion, answer generation, search/ranking, source rendering, document access, privacy, production environment, or clinical output. It is completed in full because the subject matter is document-access security, and each statement was checked against the tree rather than assumed.
Clinical KB Database(sjrfecxgysukkwxsowpy). No environment value or Supabase configuration is modified; the readings quoted were taken against that project reference and no other, and the staleqjgitjyhxrwxsrydablrwas not used.Notes
Merge order does not matter. This PR only adds a pending request, and #2559 only applies requests that were already pending on
main, so the two do not conflict and neither blocks the other. Whichever lands second, the correction row is created by the next reconciliation run.A follow-up reply on #2559's resolved thread corrects the same false auto-reversion sentence where I had repeated it there, so neither thread stands as a record of it.
🤖 Generated with Claude Code
https://claude.ai/code/session_01AcjoJHKAPfBPKYgwTF3wTS
Note
Low Risk
Documentation-only inbox queue with no runtime, schema, or ledger reconciliation in this PR.
Overview
Adds a single pending outstanding-issues inbox request (
ac026ec0-6070-4623-aecc-c9a1af2f312a.json) withaction: "add"— it does not editdocs/outstanding-issues.mdor any application code.The queued P2 issue documents a correction of record for archived #Z61JRT: the close outcome wrongly implied that flipping
clinical-imagesto public would be blocked or inert because owner-folder RLS would still apply. The request states the accurate risk (a public bucket serves objects anonymously with no RLS), notes live buckets remain private and signing paths are unchanged, and clarifies that migrationON CONFLICT … public = falseonly applies on fresh migration replay — not as automatic reversion on live — with detection viacheck:driftand manual remediation as the real control.Provenance ties the finding to Codex review on PR #2559 (verified against
storage-jsgetPublicUrldocs) and a draft auto-reversion claim fixed before reconciliation.Reviewed by Cursor Bugbot for commit d4ce626. Configure here.