DRAFT (#231): RLS lockdown SQL + storage & realtime(a) remediation plans — do not merge/apply - #232

Closed
Jose-Gael-Cruz-Lopez wants to merge 2 commits into
mainfrom
security/231-rls-lockdown-drafts
Closed

DRAFT (#231): RLS lockdown SQL + storage & realtime(a) remediation plans — do not merge/apply#232
Jose-Gael-Cruz-Lopez wants to merge 2 commits into
mainfrom
security/231-rls-lockdown-drafts

Conversation

@Jose-Gael-Cruz-Lopez

@Jose-Gael-Cruz-LopezJose-Gael-Cruz-Lopez commented Jun 13, 2026

Copy link
Copy Markdown
Member

DRAFT for review — do NOT merge or apply. Remediation artifacts for #231 (public anon key has full DML on 38/40 RLS-disabled tables → potential full-DB read/write via PostgREST, bypassing the backend). No code or prod changes here.

URGENT — RLS lockdown (one-time DDL, not a #197 migration)

  • backend/db/security/rls_lockdown.sql: ENABLE ROW LEVEL SECURITY on the 38 tables that lack it + REVOKE all anon DML across the public schema, plus ALTER DEFAULT PRIVILEGES so future tables don't silently re-grant anon.
  • Verified safe for the backend:service_role has rolbypassrls=true (live-checked), so every backend query is unaffected; only anon/authenticated (bypass=false) are constrained.
  • rls_lockdown_rollback.sql: emergency revert (restores the insecure state).
  • docs/security/rls-lockdown-plan.md: apply/verify/rollback checklist (incl. the anon-blocked REST check and the service_role-bypass proof) + Supabase-branching note (branching was unreachable via MCP).
  • Accepted breakage: anon realtime on room_messages pauses until option (a) lands; chat still loads/refreshes via the service-role REST path (fix(social): re-fetch realtime messages via decrypting REST + drop dead reactions sub (#124, #231) #230).

Storage hardening plan

docs/security/storage-hardening-plan.md: issues-media-files + application_resumesprivate + signed URLs + backend-side uploads; avatars stays public-read with the blanket public INSERT revoked; drop the global storage.objects public-INSERT policy. (Live finding: chat-images/cosmetic-assets buckets don't exist — dead code.)

Realtime option (a) design

docs/security/realtime-jwt-bridge-design.md: mint a Supabase JWT at login + realtime.setAuth, membership-scoped RLS SELECT policy on room_messages; postgres_changes + RLS (minimal) vs private channels. JWT refresh flagged (30d session vs ~1h JWT). Builds after the lockdown.

How to use

Review the SQL, then you apply rls_lockdown.sql (branch-first if available). I did not apply anything. Storage SQL + realtime are plans to turn into their own PRs once you approve the approach.

Summary by CodeRabbit

  • Documentation

    • Added security planning guides documenting database access control implementation, real-time authentication architecture, and storage hardening strategies with verification procedures.
  • Chores

    • Added database security configuration scripts and corresponding rollback procedures to support infrastructure security improvements.

…drafts (#231)
DRAFTS ONLY — do not apply; for review. Addresses the #231 finding that the
public anon key has full DML on 38/40 RLS-disabled public tables (potential
full-DB read/write via PostgREST, bypassing the backend).
- backend/db/security/rls_lockdown.sql — enable RLS on the 38 tables + revoke
anon DML across the public schema (+ default privileges). Verified safe:
service_role has rolbypassrls=true, so the backend is unaffected.
- backend/db/security/rls_lockdown_rollback.sql — emergency revert (restores
the insecure state).
- docs/security/rls-lockdown-plan.md — apply/verify/rollback checklist; branch
note (branching unreachable via MCP).
- docs/security/storage-hardening-plan.md — issues-media-files + application_
resumes -> private + signed URLs + backend uploads; avatars public-read,
revoke blanket public INSERT; kill the global storage public-INSERT policy.
- docs/security/realtime-jwt-bridge-design.md — option (a): mint Supabase JWT
at login + setAuth, room_messages membership RLS policy; flags JWT refresh
(30d session vs ~1h JWT). Lands after the lockdown to restore realtime.
No code/prod changes in this PR.
@coderabbitai

coderabbitaiBot commented Jun 13, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@Jose-Gael-Cruz-Lopez, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 58 minutes and 13 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 0ed9b2de-7de8-4558-8028-28bd9d97574d

📥 Commits

Reviewing files that changed from the base of the PR and between 5b1aabb and 07fca30.

📒 Files selected for processing (5)
  • backend/db/security/rls_lockdown.sql
  • backend/db/security/rls_lockdown_rollback.sql
  • docs/security/realtime-jwt-bridge-design.md
  • docs/security/rls-lockdown-plan.md
  • docs/security/storage-hardening-plan.md
📝 Walkthrough

Walkthrough

This PR establishes a project-wide security hardening initiative by introducing RLS lockdown DDL scripts, verification procedures, and design plans for restoring Realtime functionality and hardening storage access—all without requiring backend code changes since the service role bypasses RLS.

Changes

Security Hardening Initiative

Layer / File(s)Summary
RLS Lockdown DDL Apply and Rollback Scripts
backend/db/security/rls_lockdown.sql, backend/db/security/rls_lockdown_rollback.sql
Transactional SQL scripts enable RLS on 38 public tables and revoke all anon DML permissions (SELECT/INSERT/UPDATE/DELETE) plus future default privileges; rollback re-grants anon access and disables RLS while preserving achievement table state.
RLS Lockdown Verification and Deployment Plan
docs/security/rls-lockdown-plan.md
Documents lockdown scope, backend safety via service_role bypass, expected anon realtime breakage on room_messages, test-first and prod deployment procedures, pre-apply snapshots, post-apply verification checklist (RLS status, anon grants, REST auth, backend pytest, realtime behavior), rollback steps, and follow-up notes on authenticated grants and storage hardening.
Realtime JWT Bridge Design for RLS-Protected Room Messages
docs/security/realtime-jwt-bridge-design.md
Proposes backend JWT-minting endpoint with SUPABASE_JWT_SECRET, membership-scoped SELECT policy on room_messages using auth.uid matched to room_members, frontend token fetch post-login with realtime.setAuth, JWT refresh loop with reconnect/sleep handling and REST fallback, plus optional room_reactions follow-up.
Storage Bucket Hardening Plan
docs/security/storage-hardening-plan.md
Draft plan to restrict public-read buckets (issues-media-files, application_resumes) via SQL, drop public INSERT policies, route writes through backend service_role, provide signed URLs for private reads, leave avatars public, with corresponding backend upload/signing endpoints and frontend wiring plus verification checklist.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related issues

  • #231: The RLS lockdown DDL scripts and realtime-JWT bridge design address the same tables, policies, and realtime-auth gap (room_messages/room_reactions membership RLS and JWT-based Realtime authentication) described in that issue.

Poem

A rabbit stands guard at the gate today,
Locking down rows in the SQL way—
Anon must ask now, through tokens bright,
While service_role dances through the night. 🐰🔐

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check nameStatusExplanation
Title check✅ PassedThe title accurately references the RLS lockdown (#231) and lists the main remediation artifacts (SQL, storage & realtime plans), marking it as DRAFT — clearly indicating this is a review-stage document.
Description check✅ PassedThe description covers objectives (RLS lockdown, storage hardening, realtime design), key files/changes, verification notes, and usage instructions, but deviates from the template structure with narrative format instead of structured sections.
Docstring Coverage✅ PassedNo functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check✅ PassedCheck skipped because no linked issues were found for this pull request.
Out of Scope Changes check✅ PassedCheck skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch security/231-rls-lockdown-drafts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@cloudflare-workers-and-pages

cloudflare-workers-and-pagesBot commented Jun 13, 2026

Copy link
Copy Markdown

Deploying with Cloudflare Workers Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

StatusNameLatest CommitPreview URLUpdated (UTC)
✅ Deployment successful!
View logs
frontend07fca30Commit Preview URL

Branch Preview URL
Jun 17 2026, 01:24 AM

@coderabbitaicoderabbitaiBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🧹 Nitpick comments (1)
docs/security/storage-hardening-plan.md (1)

48-50: ⚡ Quick win

Provide concrete SQL queries for the verification checklist.

The verification steps are described narratively (e.g., "storage.buckets: issues-media-files and application_resumes show public=false") but would be more reliable if accompanied by concrete SQL queries that can be automated in tests or runbooks. For example:

  • Query to check storage.buckets.public flag
  • Query to list storage.objects policies and confirm the two public ones are gone
  • Query to test anon-role upload attempt (expect denial)

Add explicit SQL commands so verifiers can run them post-apply.

🤖 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/security/storage-hardening-plan.md` around lines 48 - 50, Add explicit,
runnable SQL and verification commands to the checklist: (1) a SELECT against
storage.buckets filtering name IN
('issues-media-files','application_resumes','avatars') returning the public
column and asserting issues-media-files and application_resumes have public =
false while avatars = true; (2) a SELECT against the PostgreSQL policy metadata
for the storage schema (pg_policies or the pg_catalog view) filtering on
table_name = 'objects' (and/or table_name = 'buckets' if used) to confirm the
two former {public} policies are absent; (3) a SELECT against storage.objects
(or listing via the storage API) to confirm object visibility matches bucket
public flags; (4) a concrete anon-role upload test (curl or psql INSERT attempt
authenticated as anon) that expects a 4xx denial; and (5) a signed-URL GET test
(curl) that expects 200; for each step include the exact command to run and the
expected result/output assertion so the checks can be automated in
runbooks/tests (reference symbols: storage.buckets, storage.objects,
pg_policies, anon role, signed URL).
🤖 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 `@backend/db/security/rls_lockdown.sql`:
- Around line 79-83: Update the ALTER DEFAULT PRIVILEGES statements to target
the table-creator role(s) explicitly: in backend/db/security/rls_lockdown.sql
(lines 79-83) change the generic "ALTER DEFAULT PRIVILEGES IN SCHEMA public
REVOKE ... FROM anon" to "ALTER DEFAULT PRIVILEGES FOR ROLE <creator_role> IN
SCHEMA public REVOKE SELECT, INSERT, UPDATE, DELETE ON TABLES FROM anon;" for
each relevant creator role, and in backend/db/security/rls_lockdown_rollback.sql
(lines 19-21) mirror that change so the rollback uses "ALTER DEFAULT PRIVILEGES
FOR ROLE <creator_role> IN SCHEMA public GRANT SELECT, INSERT, UPDATE, DELETE ON
TABLES TO anon;" (replace <creator_role> with the actual DB role(s) that create
tables); after applying, verify pg_default_acl contains no default table
privileges granting anon in the public schema.
In `@docs/security/realtime-jwt-bridge-design.md`:
- Around line 21-23: The code fence showing the JWT claims (claims: { sub:
<user_id>, role: "authenticated", aud: "authenticated", exp: now+1h, iat: now })
lacks a language tag; update the fenced block to use a JSON language tag (e.g.,
```json) so the claims snippet is properly highlighted and markdown lint passes.
In `@docs/security/rls-lockdown-plan.md`:
- Around line 64-68: Replace the hard-coded project URL in the curl check with a
parameterized environment variable and mark the fenced code block as bash:
change the request URL string to use
"${SUPABASE_URL}/rest/v1/users?select=id&limit=1" instead of the fixed
"https://jxqcmjqtjlpuxfrxmrdv.supabase.co/..." and add the fence language
identifier "bash" to the triple-backtick that opens the snippet so the block
reads as a bash example; keep the header flags (-H "apikey: <ANON_KEY>" -H
"Authorization: Bearer <ANON_KEY>") unchanged.
In `@docs/security/storage-hardening-plan.md`:
- Line 40: Add explicit validation rules to the design for the POST
/api/issue-reports/screenshot endpoint: state that
request_limits.read_within_limit will enforce a max file size of 5MB (consistent
with other UI assets) and that the content-type allowlist will accept image/png,
image/jpeg, image/webp (list these MIME types explicitly); describe that the
endpoint is auth-gated via get_session_user_id and that validation failures
return a 400 JSON response with { error: "validation_failed", details:
"<reason>" } while auth failures return 401 and storage/upload errors return 500
with { error: "upload_failed" }; note that uploads are stored to
issues-media-files using the same service-key flow as careers._upload_resume and
that a per-user rate limit of 10 uploads per hour should be enforced (or call
out that an equivalent existing rate-limiter must be applied), so implementers
know the exact size, types, error format, storage target, auth hook, and
rate-limiting expectation.
- Around line 23-24: The change to make the `issues-media-files` and
`application_resumes` buckets private breaks existing values in
`issue_reports.screenshot_urls` (and any admin code expecting public URLs);
implement a migration and compatibility plan: write a DB migration script that
backfills existing public URLs in `issue_reports.screenshot_urls` to signed URLs
(using the same signing logic you will deploy), and add a fallback in the admin
handlers (the code paths that read `issue_reports.screenshot_urls` and the admin
signed-URL view, plus `careers.py` for resumes) to accept both legacy public-URL
strings and new signed-URL strings during a transition window; publish a clear
timeline and contract change in the SQL deployment notes, add a signed-URL cache
or short TTL mapping if you need to avoid re-signing on every read, and ensure
tests cover backfill, fallback parsing, and expiration behavior before flipping
the bucket privacy flag.
- Line 41: The Signed-URL helper for private buckets lacks concrete TTL, refresh
and error-handling requirements; update the design to specify a short TTL (e.g.,
5 minutes), a client refresh strategy (frontend should request a new signed URL
on-demand when a URL is older than 4 minutes or when a 401/403 occurs, and
proactively refresh visible items when an admin opens the list), failure
handling (backend should return a clear 5xx/4xx error with retryable flag when
service_role key is unavailable; frontend should show a retry UI and backoff
retries, and fall back to a placeholder image or “unavailable” state), caching
policy (do not cache signed URLs in long-lived storage; allow in-memory per-view
caching for the TTL only and invalidate on refresh), and browser cache headers
(backend must set Cache-Control: private, max-age=0, no-store or max-age equal
to TTL if safe and include no-transform to avoid intermediaries caching stale
URLs). Implement these concrete values and flows in the Signed-URL helper and
the admin-view code paths before proceeding to implementation.
---
Nitpick comments:
In `@docs/security/storage-hardening-plan.md`:
- Around line 48-50: Add explicit, runnable SQL and verification commands to the
checklist: (1) a SELECT against storage.buckets filtering name IN
('issues-media-files','application_resumes','avatars') returning the public
column and asserting issues-media-files and application_resumes have public =
false while avatars = true; (2) a SELECT against the PostgreSQL policy metadata
for the storage schema (pg_policies or the pg_catalog view) filtering on
table_name = 'objects' (and/or table_name = 'buckets' if used) to confirm the
two former {public} policies are absent; (3) a SELECT against storage.objects
(or listing via the storage API) to confirm object visibility matches bucket
public flags; (4) a concrete anon-role upload test (curl or psql INSERT attempt
authenticated as anon) that expects a 4xx denial; and (5) a signed-URL GET test
(curl) that expects 200; for each step include the exact command to run and the
expected result/output assertion so the checks can be automated in
runbooks/tests (reference symbols: storage.buckets, storage.objects,
pg_policies, anon role, signed URL).
🪄 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: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: fc9f6d25-9286-4b5f-9117-d8df225ed12c

📥 Commits

Reviewing files that changed from the base of the PR and between a27b688 and 5b1aabb.

📒 Files selected for processing (5)
  • backend/db/security/rls_lockdown.sql
  • backend/db/security/rls_lockdown_rollback.sql
  • docs/security/realtime-jwt-bridge-design.md
  • docs/security/rls-lockdown-plan.md
  • docs/security/storage-hardening-plan.md

Comment threadbackend/db/security/rls_lockdown.sql
Comment on lines +21 to +23
```
claims: { sub: <user_id>, role: "authenticated", aud: "authenticated", exp: now+1h, iat: now }
```

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Add a language tag to the claims code fence.

This keeps markdown lint clean and improves rendering/tool support.

Suggested doc patch
-```+```json
claims: { sub: <user_id>, role: "authenticated", aud: "authenticated", exp: now+1h, iat: now }
</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **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.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)

[warning] 21-21: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🤖 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/security/realtime-jwt-bridge-design.md` around lines 21 - 23, The code
fence showing the JWT claims (claims: { sub: <user_id>, role: "authenticated",
aud: "authenticated", exp: now+1h, iat: now }) lacks a language tag; update the
fenced block to use a JSON language tag (e.g., ```json) so the claims snippet is
properly highlighted and markdown lint passes.

Source: Linters/SAST tools

Comment on lines +64 to +68
```
curl -s -o /dev/null -w "%{http_code}\n" \
"https://jxqcmjqtjlpuxfrxmrdv.supabase.co/rest/v1/users?select=id&limit=1" \
-H "apikey: <ANON_KEY>" -H "Authorization: Bearer <ANON_KEY>"
```

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Parameterize the REST verification URL and annotate the fence language.

The check currently hard-codes one project URL, which makes branch/prod verification error-prone. Also add the missing code-block language for lint/tooling consistency.

Suggested doc patch
- ```+ ```bash
curl -s -o /dev/null -w "%{http_code}\n" \
- "https://jxqcmjqtjlpuxfrxmrdv.supabase.co/rest/v1/users?select=id&limit=1" \+ "${SUPABASE_URL}/rest/v1/users?select=id&limit=1" \
-H "apikey: <ANON_KEY>" -H "Authorization: Bearer <ANON_KEY>"
```
📝 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
```
curl -s -o /dev/null -w "%{http_code}\n" \
"https://jxqcmjqtjlpuxfrxmrdv.supabase.co/rest/v1/users?select=id&limit=1" \
-H "apikey: <ANON_KEY>" -H "Authorization: Bearer <ANON_KEY>"
```
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)

[warning] 64-64: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🤖 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/security/rls-lockdown-plan.md` around lines 64 - 68, Replace the
hard-coded project URL in the curl check with a parameterized environment
variable and mark the fenced code block as bash: change the request URL string
to use "${SUPABASE_URL}/rest/v1/users?select=id&limit=1" instead of the fixed
"https://jxqcmjqtjlpuxfrxmrdv.supabase.co/..." and add the fence language
identifier "bash" to the triple-backtick that opens the snippet so the block
reads as a bash example; keep the header flags (-H "apikey: <ANON_KEY>" -H
"Authorization: Bearer <ANON_KEY>") unchanged.

Source: Linters/SAST tools

Comment on lines +23 to +24
| `issues-media-files` | **false** | new backend endpoint (multipart → service-key upload), reusing `request_limits.read_within_limit` + content-type allowlist (the #220/#229 pattern) | backend signed URL (admin view) |
| `application_resumes` | **false** | already backend (`careers.py`) | backend signed URL (admin view) |

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Address the breaking change: existing issue reports with public screenshot URLs will fail once buckets go private.

The plan correctly proposes making issues-media-files and application_resumes private, but existing rows in issue_reports.screenshot_urls (and any admin code reading them) currently store/expect public URLs. After the buckets are private, those URLs will return 400/403.

Define a migration strategy:

  • Should existing public URLs be migrated to signed URLs (backfill)?
  • Should the admin view handle both old public-URL format and new signed-URL format during a transition window?
  • Should old URLs be invalidated or mapped to a signed-URL cache?

Clarify the timeline and contract change before applying the SQL.

Also applies to: 40-41

🤖 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/security/storage-hardening-plan.md` around lines 23 - 24, The change to
make the `issues-media-files` and `application_resumes` buckets private breaks
existing values in `issue_reports.screenshot_urls` (and any admin code expecting
public URLs); implement a migration and compatibility plan: write a DB migration
script that backfills existing public URLs in `issue_reports.screenshot_urls` to
signed URLs (using the same signing logic you will deploy), and add a fallback
in the admin handlers (the code paths that read `issue_reports.screenshot_urls`
and the admin signed-URL view, plus `careers.py` for resumes) to accept both
legacy public-URL strings and new signed-URL strings during a transition window;
publish a clear timeline and contract change in the SQL deployment notes, add a
signed-URL cache or short TTL mapping if you need to avoid re-signing on every
read, and ensure tests cover backfill, fallback parsing, and expiration behavior
before flipping the bucket privacy flag.

No new storage.objects policies are needed: backend uploads/reads use `service_role` (bypasses storage RLS). `avatars` stays `public=true` so its objects remain readable without a policy.

### Backend
- New `POST /api/issue-reports/screenshot` (auth-gated via `get_session_user_id`): accepts the file, validates type+size with the shared `request_limits` helpers, uploads to `issues-media-files` with the service key (mirror `careers._upload_resume`), returns the storage path (not a public URL).

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Clarify validation logic for the new screenshot upload endpoint.

The plan references "reusing request_limits.read_within_limit + content-type allowlist (the #220/#229 pattern)" but does not detail:

  • What content types are allowed for issue-report screenshots?
  • What is the file-size limit?
  • How are validation errors returned to the frontend?
  • Is there rate-limiting on uploads per user/session?

Document these constraints explicitly so the implementation is not ambiguous.

🤖 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/security/storage-hardening-plan.md` at line 40, Add explicit validation
rules to the design for the POST /api/issue-reports/screenshot endpoint: state
that request_limits.read_within_limit will enforce a max file size of 5MB
(consistent with other UI assets) and that the content-type allowlist will
accept image/png, image/jpeg, image/webp (list these MIME types explicitly);
describe that the endpoint is auth-gated via get_session_user_id and that
validation failures return a 400 JSON response with { error:
"validation_failed", details: "<reason>" } while auth failures return 401 and
storage/upload errors return 500 with { error: "upload_failed" }; note that
uploads are stored to issues-media-files using the same service-key flow as
careers._upload_resume and that a per-user rate limit of 10 uploads per hour
should be enforced (or call out that an equivalent existing rate-limiter must be
applied), so implementers know the exact size, types, error format, storage
target, auth hook, and rate-limiting expectation.


### Backend
- New `POST /api/issue-reports/screenshot` (auth-gated via `get_session_user_id`): accepts the file, validates type+size with the shared `request_limits` helpers, uploads to `issues-media-files` with the service key (mirror `careers._upload_resume`), returns the storage path (not a public URL).
- Signed-URL helper for private buckets (admin views of screenshots/résumés): backend issues a short-TTL signed URL via the storage REST API with the service key.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Specify signed-URL TTL, refresh strategy, and error handling.

The plan mentions "short-TTL signed URL" for private-bucket reads but does not specify:

  • Exact TTL value (seconds/minutes)?
  • How the frontend admin view handles token expiry if a user is viewing a list for >TTL?
  • What happens if signed-URL generation fails (e.g., service_role key temporarily unavailable)?
  • Should signed URLs be cached/pre-fetched or generated on-demand?
  • Interaction with browser cache headers (Cache-Control) to avoid stale signed URLs.

Add concrete values and error paths before implementation.

🤖 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/security/storage-hardening-plan.md` at line 41, The Signed-URL helper
for private buckets lacks concrete TTL, refresh and error-handling requirements;
update the design to specify a short TTL (e.g., 5 minutes), a client refresh
strategy (frontend should request a new signed URL on-demand when a URL is older
than 4 minutes or when a 401/403 occurs, and proactively refresh visible items
when an admin opens the list), failure handling (backend should return a clear
5xx/4xx error with retryable flag when service_role key is unavailable; frontend
should show a retry UI and backoff retries, and fall back to a placeholder image
or “unavailable” state), caching policy (do not cache signed URLs in long-lived
storage; allow in-memory per-view caching for the TTL only and invalidate on
refresh), and browser cache headers (backend must set Cache-Control: private,
max-age=0, no-store or max-age equal to TTL if safe and include no-transform to
avoid intermediaries caching stale URLs). Implement these concrete values and
flows in the Signed-URL helper and the admin-view code paths before proceeding
to implementation.

@Jose-Gael-Cruz-Lopez

Copy link
Copy Markdown
MemberAuthor

Superseded for the docs: the three planning docs are landing via #233 (docs-only). This PR is kept as the applied-to-prod record of the RLS lockdown SQL (rls_lockdown.sql / rls_lockdown_rollback.sql) — already applied directly to production on 2026-06-13 (anon confirmed locked out). Do not merge (the SQL ran straight on prod; nothing re-runs it from the repo). Keep open as the record, or close at your discretion.

Jose-Gael-Cruz-Lopez added a commit that referenced this pull request Jun 14, 2026
…n plans (#231)
Carved from the #232 draft so the planning docs live in main (the SQL stays in
#232 as the applied-to-prod record, not merged). The RLS lockdown is already
applied to production (anon locked out, confirmed). #230 references the
realtime-jwt-bridge design doc landed here.
Jose-Gael-Cruz-Lopez added a commit that referenced this pull request Jun 17, 2026
…us (#231)
Resolve the APPLIED-vs-draft contradiction: Phase 1 (application_resumes →
private) was genuinely applied to prod on 2026-06-15 (commit d14b14f, MCP,
verified public=false, with the CDN-cache caveat learned during verification),
so it is now framed as an after-the-fact RECORD across the SQL file and plan
doc. Phase 2 (issues-media-files) stays clearly draft/not-applied. Removed the
stale top-line "Nothing applied to prod" / "apply now" wording that conflicted
with the recorded Phase 1.
Also:
- Wrap Phase 1 UPDATE in BEGIN/COMMIT for copy-paste safety + Phase 2 parity.
- Phase 2b drops use DROP POLICY IF EXISTS (name drift won't fail the txn);
rollback CREATEs guarded with DROP IF EXISTS to stay idempotent.
- Document the cross-PR ordering constraint: #238 supersedes the old copy of
storage-hardening-plan.md in #232, so #238 must merge AFTER #232 (or #232
must drop the file) to avoid a silent revert.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- rls_lockdown.sql: make ALTER DEFAULT PRIVILEGES REVOKE robust across
table-creating roles (per-role scoping) with a documented discovery query
and FOR ROLE placeholders so a future migration run as another role can't
silently re-grant anon. Mirror the per-role GRANT in the rollback.
- rls-lockdown-plan.md: mark the "anon has zero table DML" query as a
recurring BLOCKING check to wire into CI/cron; clarify verification step 3
so a permission-denied error OR an empty 200 [] both count as PASS (RLS vs
REVOKE), not a false failure.
- realtime-jwt-bridge-design.md: add a presence/typing channel section
(presence:${roomId} in Social.tsx) noting it is NOT broken by the anon DML
REVOKE and that its authorization must be re-verified under setAuth(jwt)/
private channels; promote the asymmetric-vs-HS256 signing-key-type check to
a BLOCKING precondition (wrong key type = silent 401 on setAuth).
- storage-hardening-plan.md: add SUPERSEDED-by-#238 banner (and cross-link in
the lockdown plan) since #238 rewrites the storage plan and corrects two
factual errors in this copy.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Jose-Gael-Cruz-Lopez added a commit that referenced this pull request Jun 17, 2026
…231)
Reconcile the inconsistent résumé/object count (table said 13, prose/SQL
said 12) to a consistent ~12-13 with a snapshot caveat (observed 2026-06-15,
re-verify at apply), since prod cannot be queried here.
Correct the cross-PR ordering notes: the file is add/add between #232 and
#238, so the second merge raises an explicit CONFLICT (add/add) — not a
"silent overwrite/revert". Resolve in #238's favor; recommendation (merge
#232 first or drop the file from #232) unchanged.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Jose-Gael-Cruz-Lopez added a commit that referenced this pull request Jun 24, 2026
…ount
Three correctness fixes to the plan doc (still DO NOT MERGE/APPLY):
- Major: Phase 2a (route issue-report screenshots through the backend) has
already landed on main (feedback.py POST /api/issue-reports/screenshot,
ReportIssueFlow.tsx off supabase.storage, test_issue_screenshot_auth.py).
Update the status block and Phase 2 section so the only remaining storage
step is Phase 2b (flip issues-media-files private).
- Minor: the doc already exists on main (it is a modification, not an add),
so there is no add/add conflict with #232. Replace the cross-PR ordering
section with an accurate note.
- Minor: drop the loose/stale ~12-13 object count; the bucket holds résumé
PII regardless of count, so no number is pinned.
@Jose-Gael-Cruz-Lopez

Copy link
Copy Markdown
MemberAuthor

Re-verified #231 storage against prod (read-only) on 2026-07-01: the storage portion of this draft is largely done already — application_resumes is private, screenshots upload via the auth-gated POST /api/issue-reports/screenshot (service role, feedback.py), and the anon "Allow uploads" policy is scoped to issues-media-files. The only residual (making issues-media-files private + dropping the 2 anon storage.objects policies) is now a focused migration in #304 (0029_storage_lockdown_231.sql).

Suggest this draft's storage scope be closed as superseded by #304; its realtime-RLS content (a separate concern) is untouched and can proceed on its own.

@Jose-Gael-Cruz-Lopez

Copy link
Copy Markdown
MemberAuthor

Closing without merge — this draft served its purpose and everything in it is either live or superseded:

One follow-up remains: backend/db/security/rls_lockdown.sql + rls_lockdown_rollback.sql exist only on this branch and are the sole in-repo copy of DDL that is live on prod. The branch security/231-rls-lockdown-drafts is intentionally not deleted until that SQL is extracted into a numbered migration (the 0029 precedent). The realtime JWT-bridge design (option a) also remains unbuilt — design doc is on main at docs/security/realtime-jwt-bridge-design.md.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@Jose-Gael-Cruz-Lopez
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

DRAFT (#231): RLS lockdown SQL + storage & realtime(a) remediation plans — do not merge/apply - #232

Closed
Jose-Gael-Cruz-Lopez wants to merge 2 commits into
mainfrom
security/231-rls-lockdown-drafts
Closed

DRAFT (#231): RLS lockdown SQL + storage & realtime(a) remediation plans — do not merge/apply#232
Jose-Gael-Cruz-Lopez wants to merge 2 commits into
mainfrom
security/231-rls-lockdown-drafts

Conversation

@Jose-Gael-Cruz-Lopez

@Jose-Gael-Cruz-LopezJose-Gael-Cruz-Lopez commented Jun 13, 2026

Copy link
Copy Markdown
Member

DRAFT for review — do NOT merge or apply. Remediation artifacts for #231 (public anon key has full DML on 38/40 RLS-disabled tables → potential full-DB read/write via PostgREST, bypassing the backend). No code or prod changes here.

URGENT — RLS lockdown (one-time DDL, not a #197 migration)

  • backend/db/security/rls_lockdown.sql: ENABLE ROW LEVEL SECURITY on the 38 tables that lack it + REVOKE all anon DML across the public schema, plus ALTER DEFAULT PRIVILEGES so future tables don't silently re-grant anon.
  • Verified safe for the backend:service_role has rolbypassrls=true (live-checked), so every backend query is unaffected; only anon/authenticated (bypass=false) are constrained.
  • rls_lockdown_rollback.sql: emergency revert (restores the insecure state).
  • docs/security/rls-lockdown-plan.md: apply/verify/rollback checklist (incl. the anon-blocked REST check and the service_role-bypass proof) + Supabase-branching note (branching was unreachable via MCP).
  • Accepted breakage: anon realtime on room_messages pauses until option (a) lands; chat still loads/refreshes via the service-role REST path (fix(social): re-fetch realtime messages via decrypting REST + drop dead reactions sub (#124, #231) #230).

Storage hardening plan

docs/security/storage-hardening-plan.md: issues-media-files + application_resumesprivate + signed URLs + backend-side uploads; avatars stays public-read with the blanket public INSERT revoked; drop the global storage.objects public-INSERT policy. (Live finding: chat-images/cosmetic-assets buckets don't exist — dead code.)

Realtime option (a) design

docs/security/realtime-jwt-bridge-design.md: mint a Supabase JWT at login + realtime.setAuth, membership-scoped RLS SELECT policy on room_messages; postgres_changes + RLS (minimal) vs private channels. JWT refresh flagged (30d session vs ~1h JWT). Builds after the lockdown.

How to use

Review the SQL, then you apply rls_lockdown.sql (branch-first if available). I did not apply anything. Storage SQL + realtime are plans to turn into their own PRs once you approve the approach.

Summary by CodeRabbit

  • Documentation

    • Added security planning guides documenting database access control implementation, real-time authentication architecture, and storage hardening strategies with verification procedures.
  • Chores

    • Added database security configuration scripts and corresponding rollback procedures to support infrastructure security improvements.

…drafts (#231)
DRAFTS ONLY — do not apply; for review. Addresses the #231 finding that the
public anon key has full DML on 38/40 RLS-disabled public tables (potential
full-DB read/write via PostgREST, bypassing the backend).
- backend/db/security/rls_lockdown.sql — enable RLS on the 38 tables + revoke
anon DML across the public schema (+ default privileges). Verified safe:
service_role has rolbypassrls=true, so the backend is unaffected.
- backend/db/security/rls_lockdown_rollback.sql — emergency revert (restores
the insecure state).
- docs/security/rls-lockdown-plan.md — apply/verify/rollback checklist; branch
note (branching unreachable via MCP).
- docs/security/storage-hardening-plan.md — issues-media-files + application_
resumes -> private + signed URLs + backend uploads; avatars public-read,
revoke blanket public INSERT; kill the global storage public-INSERT policy.
- docs/security/realtime-jwt-bridge-design.md — option (a): mint Supabase JWT
at login + setAuth, room_messages membership RLS policy; flags JWT refresh
(30d session vs ~1h JWT). Lands after the lockdown to restore realtime.
No code/prod changes in this PR.
@coderabbitai

coderabbitaiBot commented Jun 13, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@Jose-Gael-Cruz-Lopez, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 58 minutes and 13 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 0ed9b2de-7de8-4558-8028-28bd9d97574d

📥 Commits

Reviewing files that changed from the base of the PR and between 5b1aabb and 07fca30.

📒 Files selected for processing (5)
  • backend/db/security/rls_lockdown.sql
  • backend/db/security/rls_lockdown_rollback.sql
  • docs/security/realtime-jwt-bridge-design.md
  • docs/security/rls-lockdown-plan.md
  • docs/security/storage-hardening-plan.md
📝 Walkthrough

Walkthrough

This PR establishes a project-wide security hardening initiative by introducing RLS lockdown DDL scripts, verification procedures, and design plans for restoring Realtime functionality and hardening storage access—all without requiring backend code changes since the service role bypasses RLS.

Changes

Security Hardening Initiative

Layer / File(s)Summary
RLS Lockdown DDL Apply and Rollback Scripts
backend/db/security/rls_lockdown.sql, backend/db/security/rls_lockdown_rollback.sql
Transactional SQL scripts enable RLS on 38 public tables and revoke all anon DML permissions (SELECT/INSERT/UPDATE/DELETE) plus future default privileges; rollback re-grants anon access and disables RLS while preserving achievement table state.
RLS Lockdown Verification and Deployment Plan
docs/security/rls-lockdown-plan.md
Documents lockdown scope, backend safety via service_role bypass, expected anon realtime breakage on room_messages, test-first and prod deployment procedures, pre-apply snapshots, post-apply verification checklist (RLS status, anon grants, REST auth, backend pytest, realtime behavior), rollback steps, and follow-up notes on authenticated grants and storage hardening.
Realtime JWT Bridge Design for RLS-Protected Room Messages
docs/security/realtime-jwt-bridge-design.md
Proposes backend JWT-minting endpoint with SUPABASE_JWT_SECRET, membership-scoped SELECT policy on room_messages using auth.uid matched to room_members, frontend token fetch post-login with realtime.setAuth, JWT refresh loop with reconnect/sleep handling and REST fallback, plus optional room_reactions follow-up.
Storage Bucket Hardening Plan
docs/security/storage-hardening-plan.md
Draft plan to restrict public-read buckets (issues-media-files, application_resumes) via SQL, drop public INSERT policies, route writes through backend service_role, provide signed URLs for private reads, leave avatars public, with corresponding backend upload/signing endpoints and frontend wiring plus verification checklist.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related issues

  • #231: The RLS lockdown DDL scripts and realtime-JWT bridge design address the same tables, policies, and realtime-auth gap (room_messages/room_reactions membership RLS and JWT-based Realtime authentication) described in that issue.

Poem

A rabbit stands guard at the gate today,
Locking down rows in the SQL way—
Anon must ask now, through tokens bright,
While service_role dances through the night. 🐰🔐

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check nameStatusExplanation
Title check✅ PassedThe title accurately references the RLS lockdown (#231) and lists the main remediation artifacts (SQL, storage & realtime plans), marking it as DRAFT — clearly indicating this is a review-stage document.
Description check✅ PassedThe description covers objectives (RLS lockdown, storage hardening, realtime design), key files/changes, verification notes, and usage instructions, but deviates from the template structure with narrative format instead of structured sections.
Docstring Coverage✅ PassedNo functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check✅ PassedCheck skipped because no linked issues were found for this pull request.
Out of Scope Changes check✅ PassedCheck skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch security/231-rls-lockdown-drafts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@cloudflare-workers-and-pages

cloudflare-workers-and-pagesBot commented Jun 13, 2026

Copy link
Copy Markdown

Deploying with Cloudflare Workers Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

StatusNameLatest CommitPreview URLUpdated (UTC)
✅ Deployment successful!
View logs
frontend07fca30Commit Preview URL

Branch Preview URL
Jun 17 2026, 01:24 AM

@coderabbitaicoderabbitaiBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🧹 Nitpick comments (1)
docs/security/storage-hardening-plan.md (1)

48-50: ⚡ Quick win

Provide concrete SQL queries for the verification checklist.

The verification steps are described narratively (e.g., "storage.buckets: issues-media-files and application_resumes show public=false") but would be more reliable if accompanied by concrete SQL queries that can be automated in tests or runbooks. For example:

  • Query to check storage.buckets.public flag
  • Query to list storage.objects policies and confirm the two public ones are gone
  • Query to test anon-role upload attempt (expect denial)

Add explicit SQL commands so verifiers can run them post-apply.

🤖 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/security/storage-hardening-plan.md` around lines 48 - 50, Add explicit,
runnable SQL and verification commands to the checklist: (1) a SELECT against
storage.buckets filtering name IN
('issues-media-files','application_resumes','avatars') returning the public
column and asserting issues-media-files and application_resumes have public =
false while avatars = true; (2) a SELECT against the PostgreSQL policy metadata
for the storage schema (pg_policies or the pg_catalog view) filtering on
table_name = 'objects' (and/or table_name = 'buckets' if used) to confirm the
two former {public} policies are absent; (3) a SELECT against storage.objects
(or listing via the storage API) to confirm object visibility matches bucket
public flags; (4) a concrete anon-role upload test (curl or psql INSERT attempt
authenticated as anon) that expects a 4xx denial; and (5) a signed-URL GET test
(curl) that expects 200; for each step include the exact command to run and the
expected result/output assertion so the checks can be automated in
runbooks/tests (reference symbols: storage.buckets, storage.objects,
pg_policies, anon role, signed URL).
🤖 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 `@backend/db/security/rls_lockdown.sql`:
- Around line 79-83: Update the ALTER DEFAULT PRIVILEGES statements to target
the table-creator role(s) explicitly: in backend/db/security/rls_lockdown.sql
(lines 79-83) change the generic "ALTER DEFAULT PRIVILEGES IN SCHEMA public
REVOKE ... FROM anon" to "ALTER DEFAULT PRIVILEGES FOR ROLE <creator_role> IN
SCHEMA public REVOKE SELECT, INSERT, UPDATE, DELETE ON TABLES FROM anon;" for
each relevant creator role, and in backend/db/security/rls_lockdown_rollback.sql
(lines 19-21) mirror that change so the rollback uses "ALTER DEFAULT PRIVILEGES
FOR ROLE <creator_role> IN SCHEMA public GRANT SELECT, INSERT, UPDATE, DELETE ON
TABLES TO anon;" (replace <creator_role> with the actual DB role(s) that create
tables); after applying, verify pg_default_acl contains no default table
privileges granting anon in the public schema.
In `@docs/security/realtime-jwt-bridge-design.md`:
- Around line 21-23: The code fence showing the JWT claims (claims: { sub:
<user_id>, role: "authenticated", aud: "authenticated", exp: now+1h, iat: now })
lacks a language tag; update the fenced block to use a JSON language tag (e.g.,
```json) so the claims snippet is properly highlighted and markdown lint passes.
In `@docs/security/rls-lockdown-plan.md`:
- Around line 64-68: Replace the hard-coded project URL in the curl check with a
parameterized environment variable and mark the fenced code block as bash:
change the request URL string to use
"${SUPABASE_URL}/rest/v1/users?select=id&limit=1" instead of the fixed
"https://jxqcmjqtjlpuxfrxmrdv.supabase.co/..." and add the fence language
identifier "bash" to the triple-backtick that opens the snippet so the block
reads as a bash example; keep the header flags (-H "apikey: <ANON_KEY>" -H
"Authorization: Bearer <ANON_KEY>") unchanged.
In `@docs/security/storage-hardening-plan.md`:
- Line 40: Add explicit validation rules to the design for the POST
/api/issue-reports/screenshot endpoint: state that
request_limits.read_within_limit will enforce a max file size of 5MB (consistent
with other UI assets) and that the content-type allowlist will accept image/png,
image/jpeg, image/webp (list these MIME types explicitly); describe that the
endpoint is auth-gated via get_session_user_id and that validation failures
return a 400 JSON response with { error: "validation_failed", details:
"<reason>" } while auth failures return 401 and storage/upload errors return 500
with { error: "upload_failed" }; note that uploads are stored to
issues-media-files using the same service-key flow as careers._upload_resume and
that a per-user rate limit of 10 uploads per hour should be enforced (or call
out that an equivalent existing rate-limiter must be applied), so implementers
know the exact size, types, error format, storage target, auth hook, and
rate-limiting expectation.
- Around line 23-24: The change to make the `issues-media-files` and
`application_resumes` buckets private breaks existing values in
`issue_reports.screenshot_urls` (and any admin code expecting public URLs);
implement a migration and compatibility plan: write a DB migration script that
backfills existing public URLs in `issue_reports.screenshot_urls` to signed URLs
(using the same signing logic you will deploy), and add a fallback in the admin
handlers (the code paths that read `issue_reports.screenshot_urls` and the admin
signed-URL view, plus `careers.py` for resumes) to accept both legacy public-URL
strings and new signed-URL strings during a transition window; publish a clear
timeline and contract change in the SQL deployment notes, add a signed-URL cache
or short TTL mapping if you need to avoid re-signing on every read, and ensure
tests cover backfill, fallback parsing, and expiration behavior before flipping
the bucket privacy flag.
- Line 41: The Signed-URL helper for private buckets lacks concrete TTL, refresh
and error-handling requirements; update the design to specify a short TTL (e.g.,
5 minutes), a client refresh strategy (frontend should request a new signed URL
on-demand when a URL is older than 4 minutes or when a 401/403 occurs, and
proactively refresh visible items when an admin opens the list), failure
handling (backend should return a clear 5xx/4xx error with retryable flag when
service_role key is unavailable; frontend should show a retry UI and backoff
retries, and fall back to a placeholder image or “unavailable” state), caching
policy (do not cache signed URLs in long-lived storage; allow in-memory per-view
caching for the TTL only and invalidate on refresh), and browser cache headers
(backend must set Cache-Control: private, max-age=0, no-store or max-age equal
to TTL if safe and include no-transform to avoid intermediaries caching stale
URLs). Implement these concrete values and flows in the Signed-URL helper and
the admin-view code paths before proceeding to implementation.
---
Nitpick comments:
In `@docs/security/storage-hardening-plan.md`:
- Around line 48-50: Add explicit, runnable SQL and verification commands to the
checklist: (1) a SELECT against storage.buckets filtering name IN
('issues-media-files','application_resumes','avatars') returning the public
column and asserting issues-media-files and application_resumes have public =
false while avatars = true; (2) a SELECT against the PostgreSQL policy metadata
for the storage schema (pg_policies or the pg_catalog view) filtering on
table_name = 'objects' (and/or table_name = 'buckets' if used) to confirm the
two former {public} policies are absent; (3) a SELECT against storage.objects
(or listing via the storage API) to confirm object visibility matches bucket
public flags; (4) a concrete anon-role upload test (curl or psql INSERT attempt
authenticated as anon) that expects a 4xx denial; and (5) a signed-URL GET test
(curl) that expects 200; for each step include the exact command to run and the
expected result/output assertion so the checks can be automated in
runbooks/tests (reference symbols: storage.buckets, storage.objects,
pg_policies, anon role, signed URL).
🪄 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: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: fc9f6d25-9286-4b5f-9117-d8df225ed12c

📥 Commits

Reviewing files that changed from the base of the PR and between a27b688 and 5b1aabb.

📒 Files selected for processing (5)
  • backend/db/security/rls_lockdown.sql
  • backend/db/security/rls_lockdown_rollback.sql
  • docs/security/realtime-jwt-bridge-design.md
  • docs/security/rls-lockdown-plan.md
  • docs/security/storage-hardening-plan.md

Comment threadbackend/db/security/rls_lockdown.sql
Comment on lines +21 to +23
```
claims: { sub: <user_id>, role: "authenticated", aud: "authenticated", exp: now+1h, iat: now }
```

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Add a language tag to the claims code fence.

This keeps markdown lint clean and improves rendering/tool support.

Suggested doc patch
-```+```json
claims: { sub: <user_id>, role: "authenticated", aud: "authenticated", exp: now+1h, iat: now }
</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **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.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)

[warning] 21-21: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🤖 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/security/realtime-jwt-bridge-design.md` around lines 21 - 23, The code
fence showing the JWT claims (claims: { sub: <user_id>, role: "authenticated",
aud: "authenticated", exp: now+1h, iat: now }) lacks a language tag; update the
fenced block to use a JSON language tag (e.g., ```json) so the claims snippet is
properly highlighted and markdown lint passes.

Source: Linters/SAST tools

Comment on lines +64 to +68
```
curl -s -o /dev/null -w "%{http_code}\n" \
"https://jxqcmjqtjlpuxfrxmrdv.supabase.co/rest/v1/users?select=id&limit=1" \
-H "apikey: <ANON_KEY>" -H "Authorization: Bearer <ANON_KEY>"
```

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Parameterize the REST verification URL and annotate the fence language.

The check currently hard-codes one project URL, which makes branch/prod verification error-prone. Also add the missing code-block language for lint/tooling consistency.

Suggested doc patch
- ```+ ```bash
curl -s -o /dev/null -w "%{http_code}\n" \
- "https://jxqcmjqtjlpuxfrxmrdv.supabase.co/rest/v1/users?select=id&limit=1" \+ "${SUPABASE_URL}/rest/v1/users?select=id&limit=1" \
-H "apikey: <ANON_KEY>" -H "Authorization: Bearer <ANON_KEY>"
```
📝 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
```
curl -s -o /dev/null -w "%{http_code}\n" \
"https://jxqcmjqtjlpuxfrxmrdv.supabase.co/rest/v1/users?select=id&limit=1" \
-H "apikey: <ANON_KEY>" -H "Authorization: Bearer <ANON_KEY>"
```
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)

[warning] 64-64: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🤖 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/security/rls-lockdown-plan.md` around lines 64 - 68, Replace the
hard-coded project URL in the curl check with a parameterized environment
variable and mark the fenced code block as bash: change the request URL string
to use "${SUPABASE_URL}/rest/v1/users?select=id&limit=1" instead of the fixed
"https://jxqcmjqtjlpuxfrxmrdv.supabase.co/..." and add the fence language
identifier "bash" to the triple-backtick that opens the snippet so the block
reads as a bash example; keep the header flags (-H "apikey: <ANON_KEY>" -H
"Authorization: Bearer <ANON_KEY>") unchanged.

Source: Linters/SAST tools

Comment on lines +23 to +24
| `issues-media-files` | **false** | new backend endpoint (multipart → service-key upload), reusing `request_limits.read_within_limit` + content-type allowlist (the #220/#229 pattern) | backend signed URL (admin view) |
| `application_resumes` | **false** | already backend (`careers.py`) | backend signed URL (admin view) |

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Address the breaking change: existing issue reports with public screenshot URLs will fail once buckets go private.

The plan correctly proposes making issues-media-files and application_resumes private, but existing rows in issue_reports.screenshot_urls (and any admin code reading them) currently store/expect public URLs. After the buckets are private, those URLs will return 400/403.

Define a migration strategy:

  • Should existing public URLs be migrated to signed URLs (backfill)?
  • Should the admin view handle both old public-URL format and new signed-URL format during a transition window?
  • Should old URLs be invalidated or mapped to a signed-URL cache?

Clarify the timeline and contract change before applying the SQL.

Also applies to: 40-41

🤖 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/security/storage-hardening-plan.md` around lines 23 - 24, The change to
make the `issues-media-files` and `application_resumes` buckets private breaks
existing values in `issue_reports.screenshot_urls` (and any admin code expecting
public URLs); implement a migration and compatibility plan: write a DB migration
script that backfills existing public URLs in `issue_reports.screenshot_urls` to
signed URLs (using the same signing logic you will deploy), and add a fallback
in the admin handlers (the code paths that read `issue_reports.screenshot_urls`
and the admin signed-URL view, plus `careers.py` for resumes) to accept both
legacy public-URL strings and new signed-URL strings during a transition window;
publish a clear timeline and contract change in the SQL deployment notes, add a
signed-URL cache or short TTL mapping if you need to avoid re-signing on every
read, and ensure tests cover backfill, fallback parsing, and expiration behavior
before flipping the bucket privacy flag.

No new storage.objects policies are needed: backend uploads/reads use `service_role` (bypasses storage RLS). `avatars` stays `public=true` so its objects remain readable without a policy.

### Backend
- New `POST /api/issue-reports/screenshot` (auth-gated via `get_session_user_id`): accepts the file, validates type+size with the shared `request_limits` helpers, uploads to `issues-media-files` with the service key (mirror `careers._upload_resume`), returns the storage path (not a public URL).

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Clarify validation logic for the new screenshot upload endpoint.

The plan references "reusing request_limits.read_within_limit + content-type allowlist (the #220/#229 pattern)" but does not detail:

  • What content types are allowed for issue-report screenshots?
  • What is the file-size limit?
  • How are validation errors returned to the frontend?
  • Is there rate-limiting on uploads per user/session?

Document these constraints explicitly so the implementation is not ambiguous.

🤖 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/security/storage-hardening-plan.md` at line 40, Add explicit validation
rules to the design for the POST /api/issue-reports/screenshot endpoint: state
that request_limits.read_within_limit will enforce a max file size of 5MB
(consistent with other UI assets) and that the content-type allowlist will
accept image/png, image/jpeg, image/webp (list these MIME types explicitly);
describe that the endpoint is auth-gated via get_session_user_id and that
validation failures return a 400 JSON response with { error:
"validation_failed", details: "<reason>" } while auth failures return 401 and
storage/upload errors return 500 with { error: "upload_failed" }; note that
uploads are stored to issues-media-files using the same service-key flow as
careers._upload_resume and that a per-user rate limit of 10 uploads per hour
should be enforced (or call out that an equivalent existing rate-limiter must be
applied), so implementers know the exact size, types, error format, storage
target, auth hook, and rate-limiting expectation.


### Backend
- New `POST /api/issue-reports/screenshot` (auth-gated via `get_session_user_id`): accepts the file, validates type+size with the shared `request_limits` helpers, uploads to `issues-media-files` with the service key (mirror `careers._upload_resume`), returns the storage path (not a public URL).
- Signed-URL helper for private buckets (admin views of screenshots/résumés): backend issues a short-TTL signed URL via the storage REST API with the service key.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Specify signed-URL TTL, refresh strategy, and error handling.

The plan mentions "short-TTL signed URL" for private-bucket reads but does not specify:

  • Exact TTL value (seconds/minutes)?
  • How the frontend admin view handles token expiry if a user is viewing a list for >TTL?
  • What happens if signed-URL generation fails (e.g., service_role key temporarily unavailable)?
  • Should signed URLs be cached/pre-fetched or generated on-demand?
  • Interaction with browser cache headers (Cache-Control) to avoid stale signed URLs.

Add concrete values and error paths before implementation.

🤖 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/security/storage-hardening-plan.md` at line 41, The Signed-URL helper
for private buckets lacks concrete TTL, refresh and error-handling requirements;
update the design to specify a short TTL (e.g., 5 minutes), a client refresh
strategy (frontend should request a new signed URL on-demand when a URL is older
than 4 minutes or when a 401/403 occurs, and proactively refresh visible items
when an admin opens the list), failure handling (backend should return a clear
5xx/4xx error with retryable flag when service_role key is unavailable; frontend
should show a retry UI and backoff retries, and fall back to a placeholder image
or “unavailable” state), caching policy (do not cache signed URLs in long-lived
storage; allow in-memory per-view caching for the TTL only and invalidate on
refresh), and browser cache headers (backend must set Cache-Control: private,
max-age=0, no-store or max-age equal to TTL if safe and include no-transform to
avoid intermediaries caching stale URLs). Implement these concrete values and
flows in the Signed-URL helper and the admin-view code paths before proceeding
to implementation.

@Jose-Gael-Cruz-Lopez

Copy link
Copy Markdown
MemberAuthor

Superseded for the docs: the three planning docs are landing via #233 (docs-only). This PR is kept as the applied-to-prod record of the RLS lockdown SQL (rls_lockdown.sql / rls_lockdown_rollback.sql) — already applied directly to production on 2026-06-13 (anon confirmed locked out). Do not merge (the SQL ran straight on prod; nothing re-runs it from the repo). Keep open as the record, or close at your discretion.

Jose-Gael-Cruz-Lopez added a commit that referenced this pull request Jun 14, 2026
…n plans (#231)
Carved from the #232 draft so the planning docs live in main (the SQL stays in
#232 as the applied-to-prod record, not merged). The RLS lockdown is already
applied to production (anon locked out, confirmed). #230 references the
realtime-jwt-bridge design doc landed here.
Jose-Gael-Cruz-Lopez added a commit that referenced this pull request Jun 17, 2026
…us (#231)
Resolve the APPLIED-vs-draft contradiction: Phase 1 (application_resumes →
private) was genuinely applied to prod on 2026-06-15 (commit d14b14f, MCP,
verified public=false, with the CDN-cache caveat learned during verification),
so it is now framed as an after-the-fact RECORD across the SQL file and plan
doc. Phase 2 (issues-media-files) stays clearly draft/not-applied. Removed the
stale top-line "Nothing applied to prod" / "apply now" wording that conflicted
with the recorded Phase 1.
Also:
- Wrap Phase 1 UPDATE in BEGIN/COMMIT for copy-paste safety + Phase 2 parity.
- Phase 2b drops use DROP POLICY IF EXISTS (name drift won't fail the txn);
rollback CREATEs guarded with DROP IF EXISTS to stay idempotent.
- Document the cross-PR ordering constraint: #238 supersedes the old copy of
storage-hardening-plan.md in #232, so #238 must merge AFTER #232 (or #232
must drop the file) to avoid a silent revert.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- rls_lockdown.sql: make ALTER DEFAULT PRIVILEGES REVOKE robust across
table-creating roles (per-role scoping) with a documented discovery query
and FOR ROLE placeholders so a future migration run as another role can't
silently re-grant anon. Mirror the per-role GRANT in the rollback.
- rls-lockdown-plan.md: mark the "anon has zero table DML" query as a
recurring BLOCKING check to wire into CI/cron; clarify verification step 3
so a permission-denied error OR an empty 200 [] both count as PASS (RLS vs
REVOKE), not a false failure.
- realtime-jwt-bridge-design.md: add a presence/typing channel section
(presence:${roomId} in Social.tsx) noting it is NOT broken by the anon DML
REVOKE and that its authorization must be re-verified under setAuth(jwt)/
private channels; promote the asymmetric-vs-HS256 signing-key-type check to
a BLOCKING precondition (wrong key type = silent 401 on setAuth).
- storage-hardening-plan.md: add SUPERSEDED-by-#238 banner (and cross-link in
the lockdown plan) since #238 rewrites the storage plan and corrects two
factual errors in this copy.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Jose-Gael-Cruz-Lopez added a commit that referenced this pull request Jun 17, 2026
…231)
Reconcile the inconsistent résumé/object count (table said 13, prose/SQL
said 12) to a consistent ~12-13 with a snapshot caveat (observed 2026-06-15,
re-verify at apply), since prod cannot be queried here.
Correct the cross-PR ordering notes: the file is add/add between #232 and
#238, so the second merge raises an explicit CONFLICT (add/add) — not a
"silent overwrite/revert". Resolve in #238's favor; recommendation (merge
#232 first or drop the file from #232) unchanged.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Jose-Gael-Cruz-Lopez added a commit that referenced this pull request Jun 24, 2026
…ount
Three correctness fixes to the plan doc (still DO NOT MERGE/APPLY):
- Major: Phase 2a (route issue-report screenshots through the backend) has
already landed on main (feedback.py POST /api/issue-reports/screenshot,
ReportIssueFlow.tsx off supabase.storage, test_issue_screenshot_auth.py).
Update the status block and Phase 2 section so the only remaining storage
step is Phase 2b (flip issues-media-files private).
- Minor: the doc already exists on main (it is a modification, not an add),
so there is no add/add conflict with #232. Replace the cross-PR ordering
section with an accurate note.
- Minor: drop the loose/stale ~12-13 object count; the bucket holds résumé
PII regardless of count, so no number is pinned.
@Jose-Gael-Cruz-Lopez

Copy link
Copy Markdown
MemberAuthor

Re-verified #231 storage against prod (read-only) on 2026-07-01: the storage portion of this draft is largely done already — application_resumes is private, screenshots upload via the auth-gated POST /api/issue-reports/screenshot (service role, feedback.py), and the anon "Allow uploads" policy is scoped to issues-media-files. The only residual (making issues-media-files private + dropping the 2 anon storage.objects policies) is now a focused migration in #304 (0029_storage_lockdown_231.sql).

Suggest this draft's storage scope be closed as superseded by #304; its realtime-RLS content (a separate concern) is untouched and can proceed on its own.

@Jose-Gael-Cruz-Lopez

Copy link
Copy Markdown
MemberAuthor

Closing without merge — this draft served its purpose and everything in it is either live or superseded:

One follow-up remains: backend/db/security/rls_lockdown.sql + rls_lockdown_rollback.sql exist only on this branch and are the sole in-repo copy of DDL that is live on prod. The branch security/231-rls-lockdown-drafts is intentionally not deleted until that SQL is extracted into a numbered migration (the 0029 precedent). The realtime JWT-bridge design (option a) also remains unbuilt — design doc is on main at docs/security/realtime-jwt-bridge-design.md.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@Jose-Gael-Cruz-Lopez
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

DRAFT (#231): RLS lockdown SQL + storage & realtime(a) remediation plans — do not merge/apply - #232

Closed
Jose-Gael-Cruz-Lopez wants to merge 2 commits into
mainfrom
security/231-rls-lockdown-drafts
Closed

DRAFT (#231): RLS lockdown SQL + storage & realtime(a) remediation plans — do not merge/apply#232
Jose-Gael-Cruz-Lopez wants to merge 2 commits into
mainfrom
security/231-rls-lockdown-drafts

Conversation

@Jose-Gael-Cruz-Lopez

@Jose-Gael-Cruz-LopezJose-Gael-Cruz-Lopez commented Jun 13, 2026

Copy link
Copy Markdown
Member

DRAFT for review — do NOT merge or apply. Remediation artifacts for #231 (public anon key has full DML on 38/40 RLS-disabled tables → potential full-DB read/write via PostgREST, bypassing the backend). No code or prod changes here.

URGENT — RLS lockdown (one-time DDL, not a #197 migration)

  • backend/db/security/rls_lockdown.sql: ENABLE ROW LEVEL SECURITY on the 38 tables that lack it + REVOKE all anon DML across the public schema, plus ALTER DEFAULT PRIVILEGES so future tables don't silently re-grant anon.
  • Verified safe for the backend:service_role has rolbypassrls=true (live-checked), so every backend query is unaffected; only anon/authenticated (bypass=false) are constrained.
  • rls_lockdown_rollback.sql: emergency revert (restores the insecure state).
  • docs/security/rls-lockdown-plan.md: apply/verify/rollback checklist (incl. the anon-blocked REST check and the service_role-bypass proof) + Supabase-branching note (branching was unreachable via MCP).
  • Accepted breakage: anon realtime on room_messages pauses until option (a) lands; chat still loads/refreshes via the service-role REST path (fix(social): re-fetch realtime messages via decrypting REST + drop dead reactions sub (#124, #231) #230).

Storage hardening plan

docs/security/storage-hardening-plan.md: issues-media-files + application_resumesprivate + signed URLs + backend-side uploads; avatars stays public-read with the blanket public INSERT revoked; drop the global storage.objects public-INSERT policy. (Live finding: chat-images/cosmetic-assets buckets don't exist — dead code.)

Realtime option (a) design

docs/security/realtime-jwt-bridge-design.md: mint a Supabase JWT at login + realtime.setAuth, membership-scoped RLS SELECT policy on room_messages; postgres_changes + RLS (minimal) vs private channels. JWT refresh flagged (30d session vs ~1h JWT). Builds after the lockdown.

How to use

Review the SQL, then you apply rls_lockdown.sql (branch-first if available). I did not apply anything. Storage SQL + realtime are plans to turn into their own PRs once you approve the approach.

Summary by CodeRabbit

  • Documentation

    • Added security planning guides documenting database access control implementation, real-time authentication architecture, and storage hardening strategies with verification procedures.
  • Chores

    • Added database security configuration scripts and corresponding rollback procedures to support infrastructure security improvements.

…drafts (#231)
DRAFTS ONLY — do not apply; for review. Addresses the #231 finding that the
public anon key has full DML on 38/40 RLS-disabled public tables (potential
full-DB read/write via PostgREST, bypassing the backend).
- backend/db/security/rls_lockdown.sql — enable RLS on the 38 tables + revoke
anon DML across the public schema (+ default privileges). Verified safe:
service_role has rolbypassrls=true, so the backend is unaffected.
- backend/db/security/rls_lockdown_rollback.sql — emergency revert (restores
the insecure state).
- docs/security/rls-lockdown-plan.md — apply/verify/rollback checklist; branch
note (branching unreachable via MCP).
- docs/security/storage-hardening-plan.md — issues-media-files + application_
resumes -> private + signed URLs + backend uploads; avatars public-read,
revoke blanket public INSERT; kill the global storage public-INSERT policy.
- docs/security/realtime-jwt-bridge-design.md — option (a): mint Supabase JWT
at login + setAuth, room_messages membership RLS policy; flags JWT refresh
(30d session vs ~1h JWT). Lands after the lockdown to restore realtime.
No code/prod changes in this PR.
@coderabbitai

coderabbitaiBot commented Jun 13, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@Jose-Gael-Cruz-Lopez, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 58 minutes and 13 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 0ed9b2de-7de8-4558-8028-28bd9d97574d

📥 Commits

Reviewing files that changed from the base of the PR and between 5b1aabb and 07fca30.

📒 Files selected for processing (5)
  • backend/db/security/rls_lockdown.sql
  • backend/db/security/rls_lockdown_rollback.sql
  • docs/security/realtime-jwt-bridge-design.md
  • docs/security/rls-lockdown-plan.md
  • docs/security/storage-hardening-plan.md
📝 Walkthrough

Walkthrough

This PR establishes a project-wide security hardening initiative by introducing RLS lockdown DDL scripts, verification procedures, and design plans for restoring Realtime functionality and hardening storage access—all without requiring backend code changes since the service role bypasses RLS.

Changes

Security Hardening Initiative

Layer / File(s)Summary
RLS Lockdown DDL Apply and Rollback Scripts
backend/db/security/rls_lockdown.sql, backend/db/security/rls_lockdown_rollback.sql
Transactional SQL scripts enable RLS on 38 public tables and revoke all anon DML permissions (SELECT/INSERT/UPDATE/DELETE) plus future default privileges; rollback re-grants anon access and disables RLS while preserving achievement table state.
RLS Lockdown Verification and Deployment Plan
docs/security/rls-lockdown-plan.md
Documents lockdown scope, backend safety via service_role bypass, expected anon realtime breakage on room_messages, test-first and prod deployment procedures, pre-apply snapshots, post-apply verification checklist (RLS status, anon grants, REST auth, backend pytest, realtime behavior), rollback steps, and follow-up notes on authenticated grants and storage hardening.
Realtime JWT Bridge Design for RLS-Protected Room Messages
docs/security/realtime-jwt-bridge-design.md
Proposes backend JWT-minting endpoint with SUPABASE_JWT_SECRET, membership-scoped SELECT policy on room_messages using auth.uid matched to room_members, frontend token fetch post-login with realtime.setAuth, JWT refresh loop with reconnect/sleep handling and REST fallback, plus optional room_reactions follow-up.
Storage Bucket Hardening Plan
docs/security/storage-hardening-plan.md
Draft plan to restrict public-read buckets (issues-media-files, application_resumes) via SQL, drop public INSERT policies, route writes through backend service_role, provide signed URLs for private reads, leave avatars public, with corresponding backend upload/signing endpoints and frontend wiring plus verification checklist.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related issues

  • #231: The RLS lockdown DDL scripts and realtime-JWT bridge design address the same tables, policies, and realtime-auth gap (room_messages/room_reactions membership RLS and JWT-based Realtime authentication) described in that issue.

Poem

A rabbit stands guard at the gate today,
Locking down rows in the SQL way—
Anon must ask now, through tokens bright,
While service_role dances through the night. 🐰🔐

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check nameStatusExplanation
Title check✅ PassedThe title accurately references the RLS lockdown (#231) and lists the main remediation artifacts (SQL, storage & realtime plans), marking it as DRAFT — clearly indicating this is a review-stage document.
Description check✅ PassedThe description covers objectives (RLS lockdown, storage hardening, realtime design), key files/changes, verification notes, and usage instructions, but deviates from the template structure with narrative format instead of structured sections.
Docstring Coverage✅ PassedNo functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check✅ PassedCheck skipped because no linked issues were found for this pull request.
Out of Scope Changes check✅ PassedCheck skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch security/231-rls-lockdown-drafts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@cloudflare-workers-and-pages

cloudflare-workers-and-pagesBot commented Jun 13, 2026

Copy link
Copy Markdown

Deploying with Cloudflare Workers Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

StatusNameLatest CommitPreview URLUpdated (UTC)
✅ Deployment successful!
View logs
frontend07fca30Commit Preview URL

Branch Preview URL
Jun 17 2026, 01:24 AM

@coderabbitaicoderabbitaiBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🧹 Nitpick comments (1)
docs/security/storage-hardening-plan.md (1)

48-50: ⚡ Quick win

Provide concrete SQL queries for the verification checklist.

The verification steps are described narratively (e.g., "storage.buckets: issues-media-files and application_resumes show public=false") but would be more reliable if accompanied by concrete SQL queries that can be automated in tests or runbooks. For example:

  • Query to check storage.buckets.public flag
  • Query to list storage.objects policies and confirm the two public ones are gone
  • Query to test anon-role upload attempt (expect denial)

Add explicit SQL commands so verifiers can run them post-apply.

🤖 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/security/storage-hardening-plan.md` around lines 48 - 50, Add explicit,
runnable SQL and verification commands to the checklist: (1) a SELECT against
storage.buckets filtering name IN
('issues-media-files','application_resumes','avatars') returning the public
column and asserting issues-media-files and application_resumes have public =
false while avatars = true; (2) a SELECT against the PostgreSQL policy metadata
for the storage schema (pg_policies or the pg_catalog view) filtering on
table_name = 'objects' (and/or table_name = 'buckets' if used) to confirm the
two former {public} policies are absent; (3) a SELECT against storage.objects
(or listing via the storage API) to confirm object visibility matches bucket
public flags; (4) a concrete anon-role upload test (curl or psql INSERT attempt
authenticated as anon) that expects a 4xx denial; and (5) a signed-URL GET test
(curl) that expects 200; for each step include the exact command to run and the
expected result/output assertion so the checks can be automated in
runbooks/tests (reference symbols: storage.buckets, storage.objects,
pg_policies, anon role, signed URL).
🤖 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 `@backend/db/security/rls_lockdown.sql`:
- Around line 79-83: Update the ALTER DEFAULT PRIVILEGES statements to target
the table-creator role(s) explicitly: in backend/db/security/rls_lockdown.sql
(lines 79-83) change the generic "ALTER DEFAULT PRIVILEGES IN SCHEMA public
REVOKE ... FROM anon" to "ALTER DEFAULT PRIVILEGES FOR ROLE <creator_role> IN
SCHEMA public REVOKE SELECT, INSERT, UPDATE, DELETE ON TABLES FROM anon;" for
each relevant creator role, and in backend/db/security/rls_lockdown_rollback.sql
(lines 19-21) mirror that change so the rollback uses "ALTER DEFAULT PRIVILEGES
FOR ROLE <creator_role> IN SCHEMA public GRANT SELECT, INSERT, UPDATE, DELETE ON
TABLES TO anon;" (replace <creator_role> with the actual DB role(s) that create
tables); after applying, verify pg_default_acl contains no default table
privileges granting anon in the public schema.
In `@docs/security/realtime-jwt-bridge-design.md`:
- Around line 21-23: The code fence showing the JWT claims (claims: { sub:
<user_id>, role: "authenticated", aud: "authenticated", exp: now+1h, iat: now })
lacks a language tag; update the fenced block to use a JSON language tag (e.g.,
```json) so the claims snippet is properly highlighted and markdown lint passes.
In `@docs/security/rls-lockdown-plan.md`:
- Around line 64-68: Replace the hard-coded project URL in the curl check with a
parameterized environment variable and mark the fenced code block as bash:
change the request URL string to use
"${SUPABASE_URL}/rest/v1/users?select=id&limit=1" instead of the fixed
"https://jxqcmjqtjlpuxfrxmrdv.supabase.co/..." and add the fence language
identifier "bash" to the triple-backtick that opens the snippet so the block
reads as a bash example; keep the header flags (-H "apikey: <ANON_KEY>" -H
"Authorization: Bearer <ANON_KEY>") unchanged.
In `@docs/security/storage-hardening-plan.md`:
- Line 40: Add explicit validation rules to the design for the POST
/api/issue-reports/screenshot endpoint: state that
request_limits.read_within_limit will enforce a max file size of 5MB (consistent
with other UI assets) and that the content-type allowlist will accept image/png,
image/jpeg, image/webp (list these MIME types explicitly); describe that the
endpoint is auth-gated via get_session_user_id and that validation failures
return a 400 JSON response with { error: "validation_failed", details:
"<reason>" } while auth failures return 401 and storage/upload errors return 500
with { error: "upload_failed" }; note that uploads are stored to
issues-media-files using the same service-key flow as careers._upload_resume and
that a per-user rate limit of 10 uploads per hour should be enforced (or call
out that an equivalent existing rate-limiter must be applied), so implementers
know the exact size, types, error format, storage target, auth hook, and
rate-limiting expectation.
- Around line 23-24: The change to make the `issues-media-files` and
`application_resumes` buckets private breaks existing values in
`issue_reports.screenshot_urls` (and any admin code expecting public URLs);
implement a migration and compatibility plan: write a DB migration script that
backfills existing public URLs in `issue_reports.screenshot_urls` to signed URLs
(using the same signing logic you will deploy), and add a fallback in the admin
handlers (the code paths that read `issue_reports.screenshot_urls` and the admin
signed-URL view, plus `careers.py` for resumes) to accept both legacy public-URL
strings and new signed-URL strings during a transition window; publish a clear
timeline and contract change in the SQL deployment notes, add a signed-URL cache
or short TTL mapping if you need to avoid re-signing on every read, and ensure
tests cover backfill, fallback parsing, and expiration behavior before flipping
the bucket privacy flag.
- Line 41: The Signed-URL helper for private buckets lacks concrete TTL, refresh
and error-handling requirements; update the design to specify a short TTL (e.g.,
5 minutes), a client refresh strategy (frontend should request a new signed URL
on-demand when a URL is older than 4 minutes or when a 401/403 occurs, and
proactively refresh visible items when an admin opens the list), failure
handling (backend should return a clear 5xx/4xx error with retryable flag when
service_role key is unavailable; frontend should show a retry UI and backoff
retries, and fall back to a placeholder image or “unavailable” state), caching
policy (do not cache signed URLs in long-lived storage; allow in-memory per-view
caching for the TTL only and invalidate on refresh), and browser cache headers
(backend must set Cache-Control: private, max-age=0, no-store or max-age equal
to TTL if safe and include no-transform to avoid intermediaries caching stale
URLs). Implement these concrete values and flows in the Signed-URL helper and
the admin-view code paths before proceeding to implementation.
---
Nitpick comments:
In `@docs/security/storage-hardening-plan.md`:
- Around line 48-50: Add explicit, runnable SQL and verification commands to the
checklist: (1) a SELECT against storage.buckets filtering name IN
('issues-media-files','application_resumes','avatars') returning the public
column and asserting issues-media-files and application_resumes have public =
false while avatars = true; (2) a SELECT against the PostgreSQL policy metadata
for the storage schema (pg_policies or the pg_catalog view) filtering on
table_name = 'objects' (and/or table_name = 'buckets' if used) to confirm the
two former {public} policies are absent; (3) a SELECT against storage.objects
(or listing via the storage API) to confirm object visibility matches bucket
public flags; (4) a concrete anon-role upload test (curl or psql INSERT attempt
authenticated as anon) that expects a 4xx denial; and (5) a signed-URL GET test
(curl) that expects 200; for each step include the exact command to run and the
expected result/output assertion so the checks can be automated in
runbooks/tests (reference symbols: storage.buckets, storage.objects,
pg_policies, anon role, signed URL).
🪄 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: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: fc9f6d25-9286-4b5f-9117-d8df225ed12c

📥 Commits

Reviewing files that changed from the base of the PR and between a27b688 and 5b1aabb.

📒 Files selected for processing (5)
  • backend/db/security/rls_lockdown.sql
  • backend/db/security/rls_lockdown_rollback.sql
  • docs/security/realtime-jwt-bridge-design.md
  • docs/security/rls-lockdown-plan.md
  • docs/security/storage-hardening-plan.md

Comment threadbackend/db/security/rls_lockdown.sql
Comment on lines +21 to +23
```
claims: { sub: <user_id>, role: "authenticated", aud: "authenticated", exp: now+1h, iat: now }
```

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Add a language tag to the claims code fence.

This keeps markdown lint clean and improves rendering/tool support.

Suggested doc patch
-```+```json
claims: { sub: <user_id>, role: "authenticated", aud: "authenticated", exp: now+1h, iat: now }
</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **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.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)

[warning] 21-21: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🤖 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/security/realtime-jwt-bridge-design.md` around lines 21 - 23, The code
fence showing the JWT claims (claims: { sub: <user_id>, role: "authenticated",
aud: "authenticated", exp: now+1h, iat: now }) lacks a language tag; update the
fenced block to use a JSON language tag (e.g., ```json) so the claims snippet is
properly highlighted and markdown lint passes.

Source: Linters/SAST tools

Comment on lines +64 to +68
```
curl -s -o /dev/null -w "%{http_code}\n" \
"https://jxqcmjqtjlpuxfrxmrdv.supabase.co/rest/v1/users?select=id&limit=1" \
-H "apikey: <ANON_KEY>" -H "Authorization: Bearer <ANON_KEY>"
```

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Parameterize the REST verification URL and annotate the fence language.

The check currently hard-codes one project URL, which makes branch/prod verification error-prone. Also add the missing code-block language for lint/tooling consistency.

Suggested doc patch
- ```+ ```bash
curl -s -o /dev/null -w "%{http_code}\n" \
- "https://jxqcmjqtjlpuxfrxmrdv.supabase.co/rest/v1/users?select=id&limit=1" \+ "${SUPABASE_URL}/rest/v1/users?select=id&limit=1" \
-H "apikey: <ANON_KEY>" -H "Authorization: Bearer <ANON_KEY>"
```
📝 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
```
curl -s -o /dev/null -w "%{http_code}\n" \
"https://jxqcmjqtjlpuxfrxmrdv.supabase.co/rest/v1/users?select=id&limit=1" \
-H "apikey: <ANON_KEY>" -H "Authorization: Bearer <ANON_KEY>"
```
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)

[warning] 64-64: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🤖 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/security/rls-lockdown-plan.md` around lines 64 - 68, Replace the
hard-coded project URL in the curl check with a parameterized environment
variable and mark the fenced code block as bash: change the request URL string
to use "${SUPABASE_URL}/rest/v1/users?select=id&limit=1" instead of the fixed
"https://jxqcmjqtjlpuxfrxmrdv.supabase.co/..." and add the fence language
identifier "bash" to the triple-backtick that opens the snippet so the block
reads as a bash example; keep the header flags (-H "apikey: <ANON_KEY>" -H
"Authorization: Bearer <ANON_KEY>") unchanged.

Source: Linters/SAST tools

Comment on lines +23 to +24
| `issues-media-files` | **false** | new backend endpoint (multipart → service-key upload), reusing `request_limits.read_within_limit` + content-type allowlist (the #220/#229 pattern) | backend signed URL (admin view) |
| `application_resumes` | **false** | already backend (`careers.py`) | backend signed URL (admin view) |

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Address the breaking change: existing issue reports with public screenshot URLs will fail once buckets go private.

The plan correctly proposes making issues-media-files and application_resumes private, but existing rows in issue_reports.screenshot_urls (and any admin code reading them) currently store/expect public URLs. After the buckets are private, those URLs will return 400/403.

Define a migration strategy:

  • Should existing public URLs be migrated to signed URLs (backfill)?
  • Should the admin view handle both old public-URL format and new signed-URL format during a transition window?
  • Should old URLs be invalidated or mapped to a signed-URL cache?

Clarify the timeline and contract change before applying the SQL.

Also applies to: 40-41

🤖 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/security/storage-hardening-plan.md` around lines 23 - 24, The change to
make the `issues-media-files` and `application_resumes` buckets private breaks
existing values in `issue_reports.screenshot_urls` (and any admin code expecting
public URLs); implement a migration and compatibility plan: write a DB migration
script that backfills existing public URLs in `issue_reports.screenshot_urls` to
signed URLs (using the same signing logic you will deploy), and add a fallback
in the admin handlers (the code paths that read `issue_reports.screenshot_urls`
and the admin signed-URL view, plus `careers.py` for resumes) to accept both
legacy public-URL strings and new signed-URL strings during a transition window;
publish a clear timeline and contract change in the SQL deployment notes, add a
signed-URL cache or short TTL mapping if you need to avoid re-signing on every
read, and ensure tests cover backfill, fallback parsing, and expiration behavior
before flipping the bucket privacy flag.

No new storage.objects policies are needed: backend uploads/reads use `service_role` (bypasses storage RLS). `avatars` stays `public=true` so its objects remain readable without a policy.

### Backend
- New `POST /api/issue-reports/screenshot` (auth-gated via `get_session_user_id`): accepts the file, validates type+size with the shared `request_limits` helpers, uploads to `issues-media-files` with the service key (mirror `careers._upload_resume`), returns the storage path (not a public URL).

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Clarify validation logic for the new screenshot upload endpoint.

The plan references "reusing request_limits.read_within_limit + content-type allowlist (the #220/#229 pattern)" but does not detail:

  • What content types are allowed for issue-report screenshots?
  • What is the file-size limit?
  • How are validation errors returned to the frontend?
  • Is there rate-limiting on uploads per user/session?

Document these constraints explicitly so the implementation is not ambiguous.

🤖 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/security/storage-hardening-plan.md` at line 40, Add explicit validation
rules to the design for the POST /api/issue-reports/screenshot endpoint: state
that request_limits.read_within_limit will enforce a max file size of 5MB
(consistent with other UI assets) and that the content-type allowlist will
accept image/png, image/jpeg, image/webp (list these MIME types explicitly);
describe that the endpoint is auth-gated via get_session_user_id and that
validation failures return a 400 JSON response with { error:
"validation_failed", details: "<reason>" } while auth failures return 401 and
storage/upload errors return 500 with { error: "upload_failed" }; note that
uploads are stored to issues-media-files using the same service-key flow as
careers._upload_resume and that a per-user rate limit of 10 uploads per hour
should be enforced (or call out that an equivalent existing rate-limiter must be
applied), so implementers know the exact size, types, error format, storage
target, auth hook, and rate-limiting expectation.


### Backend
- New `POST /api/issue-reports/screenshot` (auth-gated via `get_session_user_id`): accepts the file, validates type+size with the shared `request_limits` helpers, uploads to `issues-media-files` with the service key (mirror `careers._upload_resume`), returns the storage path (not a public URL).
- Signed-URL helper for private buckets (admin views of screenshots/résumés): backend issues a short-TTL signed URL via the storage REST API with the service key.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Specify signed-URL TTL, refresh strategy, and error handling.

The plan mentions "short-TTL signed URL" for private-bucket reads but does not specify:

  • Exact TTL value (seconds/minutes)?
  • How the frontend admin view handles token expiry if a user is viewing a list for >TTL?
  • What happens if signed-URL generation fails (e.g., service_role key temporarily unavailable)?
  • Should signed URLs be cached/pre-fetched or generated on-demand?
  • Interaction with browser cache headers (Cache-Control) to avoid stale signed URLs.

Add concrete values and error paths before implementation.

🤖 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/security/storage-hardening-plan.md` at line 41, The Signed-URL helper
for private buckets lacks concrete TTL, refresh and error-handling requirements;
update the design to specify a short TTL (e.g., 5 minutes), a client refresh
strategy (frontend should request a new signed URL on-demand when a URL is older
than 4 minutes or when a 401/403 occurs, and proactively refresh visible items
when an admin opens the list), failure handling (backend should return a clear
5xx/4xx error with retryable flag when service_role key is unavailable; frontend
should show a retry UI and backoff retries, and fall back to a placeholder image
or “unavailable” state), caching policy (do not cache signed URLs in long-lived
storage; allow in-memory per-view caching for the TTL only and invalidate on
refresh), and browser cache headers (backend must set Cache-Control: private,
max-age=0, no-store or max-age equal to TTL if safe and include no-transform to
avoid intermediaries caching stale URLs). Implement these concrete values and
flows in the Signed-URL helper and the admin-view code paths before proceeding
to implementation.

@Jose-Gael-Cruz-Lopez

Copy link
Copy Markdown
MemberAuthor

Superseded for the docs: the three planning docs are landing via #233 (docs-only). This PR is kept as the applied-to-prod record of the RLS lockdown SQL (rls_lockdown.sql / rls_lockdown_rollback.sql) — already applied directly to production on 2026-06-13 (anon confirmed locked out). Do not merge (the SQL ran straight on prod; nothing re-runs it from the repo). Keep open as the record, or close at your discretion.

Jose-Gael-Cruz-Lopez added a commit that referenced this pull request Jun 14, 2026
…n plans (#231)
Carved from the #232 draft so the planning docs live in main (the SQL stays in
#232 as the applied-to-prod record, not merged). The RLS lockdown is already
applied to production (anon locked out, confirmed). #230 references the
realtime-jwt-bridge design doc landed here.
Jose-Gael-Cruz-Lopez added a commit that referenced this pull request Jun 17, 2026
…us (#231)
Resolve the APPLIED-vs-draft contradiction: Phase 1 (application_resumes →
private) was genuinely applied to prod on 2026-06-15 (commit d14b14f, MCP,
verified public=false, with the CDN-cache caveat learned during verification),
so it is now framed as an after-the-fact RECORD across the SQL file and plan
doc. Phase 2 (issues-media-files) stays clearly draft/not-applied. Removed the
stale top-line "Nothing applied to prod" / "apply now" wording that conflicted
with the recorded Phase 1.
Also:
- Wrap Phase 1 UPDATE in BEGIN/COMMIT for copy-paste safety + Phase 2 parity.
- Phase 2b drops use DROP POLICY IF EXISTS (name drift won't fail the txn);
rollback CREATEs guarded with DROP IF EXISTS to stay idempotent.
- Document the cross-PR ordering constraint: #238 supersedes the old copy of
storage-hardening-plan.md in #232, so #238 must merge AFTER #232 (or #232
must drop the file) to avoid a silent revert.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- rls_lockdown.sql: make ALTER DEFAULT PRIVILEGES REVOKE robust across
table-creating roles (per-role scoping) with a documented discovery query
and FOR ROLE placeholders so a future migration run as another role can't
silently re-grant anon. Mirror the per-role GRANT in the rollback.
- rls-lockdown-plan.md: mark the "anon has zero table DML" query as a
recurring BLOCKING check to wire into CI/cron; clarify verification step 3
so a permission-denied error OR an empty 200 [] both count as PASS (RLS vs
REVOKE), not a false failure.
- realtime-jwt-bridge-design.md: add a presence/typing channel section
(presence:${roomId} in Social.tsx) noting it is NOT broken by the anon DML
REVOKE and that its authorization must be re-verified under setAuth(jwt)/
private channels; promote the asymmetric-vs-HS256 signing-key-type check to
a BLOCKING precondition (wrong key type = silent 401 on setAuth).
- storage-hardening-plan.md: add SUPERSEDED-by-#238 banner (and cross-link in
the lockdown plan) since #238 rewrites the storage plan and corrects two
factual errors in this copy.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Jose-Gael-Cruz-Lopez added a commit that referenced this pull request Jun 17, 2026
…231)
Reconcile the inconsistent résumé/object count (table said 13, prose/SQL
said 12) to a consistent ~12-13 with a snapshot caveat (observed 2026-06-15,
re-verify at apply), since prod cannot be queried here.
Correct the cross-PR ordering notes: the file is add/add between #232 and
#238, so the second merge raises an explicit CONFLICT (add/add) — not a
"silent overwrite/revert". Resolve in #238's favor; recommendation (merge
#232 first or drop the file from #232) unchanged.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Jose-Gael-Cruz-Lopez added a commit that referenced this pull request Jun 24, 2026
…ount
Three correctness fixes to the plan doc (still DO NOT MERGE/APPLY):
- Major: Phase 2a (route issue-report screenshots through the backend) has
already landed on main (feedback.py POST /api/issue-reports/screenshot,
ReportIssueFlow.tsx off supabase.storage, test_issue_screenshot_auth.py).
Update the status block and Phase 2 section so the only remaining storage
step is Phase 2b (flip issues-media-files private).
- Minor: the doc already exists on main (it is a modification, not an add),
so there is no add/add conflict with #232. Replace the cross-PR ordering
section with an accurate note.
- Minor: drop the loose/stale ~12-13 object count; the bucket holds résumé
PII regardless of count, so no number is pinned.
@Jose-Gael-Cruz-Lopez

Copy link
Copy Markdown
MemberAuthor

Re-verified #231 storage against prod (read-only) on 2026-07-01: the storage portion of this draft is largely done already — application_resumes is private, screenshots upload via the auth-gated POST /api/issue-reports/screenshot (service role, feedback.py), and the anon "Allow uploads" policy is scoped to issues-media-files. The only residual (making issues-media-files private + dropping the 2 anon storage.objects policies) is now a focused migration in #304 (0029_storage_lockdown_231.sql).

Suggest this draft's storage scope be closed as superseded by #304; its realtime-RLS content (a separate concern) is untouched and can proceed on its own.

@Jose-Gael-Cruz-Lopez

Copy link
Copy Markdown
MemberAuthor

Closing without merge — this draft served its purpose and everything in it is either live or superseded:

One follow-up remains: backend/db/security/rls_lockdown.sql + rls_lockdown_rollback.sql exist only on this branch and are the sole in-repo copy of DDL that is live on prod. The branch security/231-rls-lockdown-drafts is intentionally not deleted until that SQL is extracted into a numbered migration (the 0029 precedent). The realtime JWT-bridge design (option a) also remains unbuilt — design doc is on main at docs/security/realtime-jwt-bridge-design.md.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@Jose-Gael-Cruz-Lopez
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

DRAFT (#231): RLS lockdown SQL + storage & realtime(a) remediation plans — do not merge/apply - #232

Closed
Jose-Gael-Cruz-Lopez wants to merge 2 commits into
mainfrom
security/231-rls-lockdown-drafts
Closed

DRAFT (#231): RLS lockdown SQL + storage & realtime(a) remediation plans — do not merge/apply#232
Jose-Gael-Cruz-Lopez wants to merge 2 commits into
mainfrom
security/231-rls-lockdown-drafts

Conversation

@Jose-Gael-Cruz-Lopez

@Jose-Gael-Cruz-LopezJose-Gael-Cruz-Lopez commented Jun 13, 2026

Copy link
Copy Markdown
Member

DRAFT for review — do NOT merge or apply. Remediation artifacts for #231 (public anon key has full DML on 38/40 RLS-disabled tables → potential full-DB read/write via PostgREST, bypassing the backend). No code or prod changes here.

URGENT — RLS lockdown (one-time DDL, not a #197 migration)

  • backend/db/security/rls_lockdown.sql: ENABLE ROW LEVEL SECURITY on the 38 tables that lack it + REVOKE all anon DML across the public schema, plus ALTER DEFAULT PRIVILEGES so future tables don't silently re-grant anon.
  • Verified safe for the backend:service_role has rolbypassrls=true (live-checked), so every backend query is unaffected; only anon/authenticated (bypass=false) are constrained.
  • rls_lockdown_rollback.sql: emergency revert (restores the insecure state).
  • docs/security/rls-lockdown-plan.md: apply/verify/rollback checklist (incl. the anon-blocked REST check and the service_role-bypass proof) + Supabase-branching note (branching was unreachable via MCP).
  • Accepted breakage: anon realtime on room_messages pauses until option (a) lands; chat still loads/refreshes via the service-role REST path (fix(social): re-fetch realtime messages via decrypting REST + drop dead reactions sub (#124, #231) #230).

Storage hardening plan

docs/security/storage-hardening-plan.md: issues-media-files + application_resumesprivate + signed URLs + backend-side uploads; avatars stays public-read with the blanket public INSERT revoked; drop the global storage.objects public-INSERT policy. (Live finding: chat-images/cosmetic-assets buckets don't exist — dead code.)

Realtime option (a) design

docs/security/realtime-jwt-bridge-design.md: mint a Supabase JWT at login + realtime.setAuth, membership-scoped RLS SELECT policy on room_messages; postgres_changes + RLS (minimal) vs private channels. JWT refresh flagged (30d session vs ~1h JWT). Builds after the lockdown.

How to use

Review the SQL, then you apply rls_lockdown.sql (branch-first if available). I did not apply anything. Storage SQL + realtime are plans to turn into their own PRs once you approve the approach.

Summary by CodeRabbit

  • Documentation

    • Added security planning guides documenting database access control implementation, real-time authentication architecture, and storage hardening strategies with verification procedures.
  • Chores

    • Added database security configuration scripts and corresponding rollback procedures to support infrastructure security improvements.

…drafts (#231)
DRAFTS ONLY — do not apply; for review. Addresses the #231 finding that the
public anon key has full DML on 38/40 RLS-disabled public tables (potential
full-DB read/write via PostgREST, bypassing the backend).
- backend/db/security/rls_lockdown.sql — enable RLS on the 38 tables + revoke
anon DML across the public schema (+ default privileges). Verified safe:
service_role has rolbypassrls=true, so the backend is unaffected.
- backend/db/security/rls_lockdown_rollback.sql — emergency revert (restores
the insecure state).
- docs/security/rls-lockdown-plan.md — apply/verify/rollback checklist; branch
note (branching unreachable via MCP).
- docs/security/storage-hardening-plan.md — issues-media-files + application_
resumes -> private + signed URLs + backend uploads; avatars public-read,
revoke blanket public INSERT; kill the global storage public-INSERT policy.
- docs/security/realtime-jwt-bridge-design.md — option (a): mint Supabase JWT
at login + setAuth, room_messages membership RLS policy; flags JWT refresh
(30d session vs ~1h JWT). Lands after the lockdown to restore realtime.
No code/prod changes in this PR.
@coderabbitai

coderabbitaiBot commented Jun 13, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@Jose-Gael-Cruz-Lopez, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 58 minutes and 13 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 0ed9b2de-7de8-4558-8028-28bd9d97574d

📥 Commits

Reviewing files that changed from the base of the PR and between 5b1aabb and 07fca30.

📒 Files selected for processing (5)
  • backend/db/security/rls_lockdown.sql
  • backend/db/security/rls_lockdown_rollback.sql
  • docs/security/realtime-jwt-bridge-design.md
  • docs/security/rls-lockdown-plan.md
  • docs/security/storage-hardening-plan.md
📝 Walkthrough

Walkthrough

This PR establishes a project-wide security hardening initiative by introducing RLS lockdown DDL scripts, verification procedures, and design plans for restoring Realtime functionality and hardening storage access—all without requiring backend code changes since the service role bypasses RLS.

Changes

Security Hardening Initiative

Layer / File(s)Summary
RLS Lockdown DDL Apply and Rollback Scripts
backend/db/security/rls_lockdown.sql, backend/db/security/rls_lockdown_rollback.sql
Transactional SQL scripts enable RLS on 38 public tables and revoke all anon DML permissions (SELECT/INSERT/UPDATE/DELETE) plus future default privileges; rollback re-grants anon access and disables RLS while preserving achievement table state.
RLS Lockdown Verification and Deployment Plan
docs/security/rls-lockdown-plan.md
Documents lockdown scope, backend safety via service_role bypass, expected anon realtime breakage on room_messages, test-first and prod deployment procedures, pre-apply snapshots, post-apply verification checklist (RLS status, anon grants, REST auth, backend pytest, realtime behavior), rollback steps, and follow-up notes on authenticated grants and storage hardening.
Realtime JWT Bridge Design for RLS-Protected Room Messages
docs/security/realtime-jwt-bridge-design.md
Proposes backend JWT-minting endpoint with SUPABASE_JWT_SECRET, membership-scoped SELECT policy on room_messages using auth.uid matched to room_members, frontend token fetch post-login with realtime.setAuth, JWT refresh loop with reconnect/sleep handling and REST fallback, plus optional room_reactions follow-up.
Storage Bucket Hardening Plan
docs/security/storage-hardening-plan.md
Draft plan to restrict public-read buckets (issues-media-files, application_resumes) via SQL, drop public INSERT policies, route writes through backend service_role, provide signed URLs for private reads, leave avatars public, with corresponding backend upload/signing endpoints and frontend wiring plus verification checklist.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related issues

  • #231: The RLS lockdown DDL scripts and realtime-JWT bridge design address the same tables, policies, and realtime-auth gap (room_messages/room_reactions membership RLS and JWT-based Realtime authentication) described in that issue.

Poem

A rabbit stands guard at the gate today,
Locking down rows in the SQL way—
Anon must ask now, through tokens bright,
While service_role dances through the night. 🐰🔐

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check nameStatusExplanation
Title check✅ PassedThe title accurately references the RLS lockdown (#231) and lists the main remediation artifacts (SQL, storage & realtime plans), marking it as DRAFT — clearly indicating this is a review-stage document.
Description check✅ PassedThe description covers objectives (RLS lockdown, storage hardening, realtime design), key files/changes, verification notes, and usage instructions, but deviates from the template structure with narrative format instead of structured sections.
Docstring Coverage✅ PassedNo functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check✅ PassedCheck skipped because no linked issues were found for this pull request.
Out of Scope Changes check✅ PassedCheck skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch security/231-rls-lockdown-drafts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@cloudflare-workers-and-pages

cloudflare-workers-and-pagesBot commented Jun 13, 2026

Copy link
Copy Markdown

Deploying with Cloudflare Workers Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

StatusNameLatest CommitPreview URLUpdated (UTC)
✅ Deployment successful!
View logs
frontend07fca30Commit Preview URL

Branch Preview URL
Jun 17 2026, 01:24 AM

@coderabbitaicoderabbitaiBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🧹 Nitpick comments (1)
docs/security/storage-hardening-plan.md (1)

48-50: ⚡ Quick win

Provide concrete SQL queries for the verification checklist.

The verification steps are described narratively (e.g., "storage.buckets: issues-media-files and application_resumes show public=false") but would be more reliable if accompanied by concrete SQL queries that can be automated in tests or runbooks. For example:

  • Query to check storage.buckets.public flag
  • Query to list storage.objects policies and confirm the two public ones are gone
  • Query to test anon-role upload attempt (expect denial)

Add explicit SQL commands so verifiers can run them post-apply.

🤖 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/security/storage-hardening-plan.md` around lines 48 - 50, Add explicit,
runnable SQL and verification commands to the checklist: (1) a SELECT against
storage.buckets filtering name IN
('issues-media-files','application_resumes','avatars') returning the public
column and asserting issues-media-files and application_resumes have public =
false while avatars = true; (2) a SELECT against the PostgreSQL policy metadata
for the storage schema (pg_policies or the pg_catalog view) filtering on
table_name = 'objects' (and/or table_name = 'buckets' if used) to confirm the
two former {public} policies are absent; (3) a SELECT against storage.objects
(or listing via the storage API) to confirm object visibility matches bucket
public flags; (4) a concrete anon-role upload test (curl or psql INSERT attempt
authenticated as anon) that expects a 4xx denial; and (5) a signed-URL GET test
(curl) that expects 200; for each step include the exact command to run and the
expected result/output assertion so the checks can be automated in
runbooks/tests (reference symbols: storage.buckets, storage.objects,
pg_policies, anon role, signed URL).
🤖 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 `@backend/db/security/rls_lockdown.sql`:
- Around line 79-83: Update the ALTER DEFAULT PRIVILEGES statements to target
the table-creator role(s) explicitly: in backend/db/security/rls_lockdown.sql
(lines 79-83) change the generic "ALTER DEFAULT PRIVILEGES IN SCHEMA public
REVOKE ... FROM anon" to "ALTER DEFAULT PRIVILEGES FOR ROLE <creator_role> IN
SCHEMA public REVOKE SELECT, INSERT, UPDATE, DELETE ON TABLES FROM anon;" for
each relevant creator role, and in backend/db/security/rls_lockdown_rollback.sql
(lines 19-21) mirror that change so the rollback uses "ALTER DEFAULT PRIVILEGES
FOR ROLE <creator_role> IN SCHEMA public GRANT SELECT, INSERT, UPDATE, DELETE ON
TABLES TO anon;" (replace <creator_role> with the actual DB role(s) that create
tables); after applying, verify pg_default_acl contains no default table
privileges granting anon in the public schema.
In `@docs/security/realtime-jwt-bridge-design.md`:
- Around line 21-23: The code fence showing the JWT claims (claims: { sub:
<user_id>, role: "authenticated", aud: "authenticated", exp: now+1h, iat: now })
lacks a language tag; update the fenced block to use a JSON language tag (e.g.,
```json) so the claims snippet is properly highlighted and markdown lint passes.
In `@docs/security/rls-lockdown-plan.md`:
- Around line 64-68: Replace the hard-coded project URL in the curl check with a
parameterized environment variable and mark the fenced code block as bash:
change the request URL string to use
"${SUPABASE_URL}/rest/v1/users?select=id&limit=1" instead of the fixed
"https://jxqcmjqtjlpuxfrxmrdv.supabase.co/..." and add the fence language
identifier "bash" to the triple-backtick that opens the snippet so the block
reads as a bash example; keep the header flags (-H "apikey: <ANON_KEY>" -H
"Authorization: Bearer <ANON_KEY>") unchanged.
In `@docs/security/storage-hardening-plan.md`:
- Line 40: Add explicit validation rules to the design for the POST
/api/issue-reports/screenshot endpoint: state that
request_limits.read_within_limit will enforce a max file size of 5MB (consistent
with other UI assets) and that the content-type allowlist will accept image/png,
image/jpeg, image/webp (list these MIME types explicitly); describe that the
endpoint is auth-gated via get_session_user_id and that validation failures
return a 400 JSON response with { error: "validation_failed", details:
"<reason>" } while auth failures return 401 and storage/upload errors return 500
with { error: "upload_failed" }; note that uploads are stored to
issues-media-files using the same service-key flow as careers._upload_resume and
that a per-user rate limit of 10 uploads per hour should be enforced (or call
out that an equivalent existing rate-limiter must be applied), so implementers
know the exact size, types, error format, storage target, auth hook, and
rate-limiting expectation.
- Around line 23-24: The change to make the `issues-media-files` and
`application_resumes` buckets private breaks existing values in
`issue_reports.screenshot_urls` (and any admin code expecting public URLs);
implement a migration and compatibility plan: write a DB migration script that
backfills existing public URLs in `issue_reports.screenshot_urls` to signed URLs
(using the same signing logic you will deploy), and add a fallback in the admin
handlers (the code paths that read `issue_reports.screenshot_urls` and the admin
signed-URL view, plus `careers.py` for resumes) to accept both legacy public-URL
strings and new signed-URL strings during a transition window; publish a clear
timeline and contract change in the SQL deployment notes, add a signed-URL cache
or short TTL mapping if you need to avoid re-signing on every read, and ensure
tests cover backfill, fallback parsing, and expiration behavior before flipping
the bucket privacy flag.
- Line 41: The Signed-URL helper for private buckets lacks concrete TTL, refresh
and error-handling requirements; update the design to specify a short TTL (e.g.,
5 minutes), a client refresh strategy (frontend should request a new signed URL
on-demand when a URL is older than 4 minutes or when a 401/403 occurs, and
proactively refresh visible items when an admin opens the list), failure
handling (backend should return a clear 5xx/4xx error with retryable flag when
service_role key is unavailable; frontend should show a retry UI and backoff
retries, and fall back to a placeholder image or “unavailable” state), caching
policy (do not cache signed URLs in long-lived storage; allow in-memory per-view
caching for the TTL only and invalidate on refresh), and browser cache headers
(backend must set Cache-Control: private, max-age=0, no-store or max-age equal
to TTL if safe and include no-transform to avoid intermediaries caching stale
URLs). Implement these concrete values and flows in the Signed-URL helper and
the admin-view code paths before proceeding to implementation.
---
Nitpick comments:
In `@docs/security/storage-hardening-plan.md`:
- Around line 48-50: Add explicit, runnable SQL and verification commands to the
checklist: (1) a SELECT against storage.buckets filtering name IN
('issues-media-files','application_resumes','avatars') returning the public
column and asserting issues-media-files and application_resumes have public =
false while avatars = true; (2) a SELECT against the PostgreSQL policy metadata
for the storage schema (pg_policies or the pg_catalog view) filtering on
table_name = 'objects' (and/or table_name = 'buckets' if used) to confirm the
two former {public} policies are absent; (3) a SELECT against storage.objects
(or listing via the storage API) to confirm object visibility matches bucket
public flags; (4) a concrete anon-role upload test (curl or psql INSERT attempt
authenticated as anon) that expects a 4xx denial; and (5) a signed-URL GET test
(curl) that expects 200; for each step include the exact command to run and the
expected result/output assertion so the checks can be automated in
runbooks/tests (reference symbols: storage.buckets, storage.objects,
pg_policies, anon role, signed URL).
🪄 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: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: fc9f6d25-9286-4b5f-9117-d8df225ed12c

📥 Commits

Reviewing files that changed from the base of the PR and between a27b688 and 5b1aabb.

📒 Files selected for processing (5)
  • backend/db/security/rls_lockdown.sql
  • backend/db/security/rls_lockdown_rollback.sql
  • docs/security/realtime-jwt-bridge-design.md
  • docs/security/rls-lockdown-plan.md
  • docs/security/storage-hardening-plan.md

Comment threadbackend/db/security/rls_lockdown.sql
Comment on lines +21 to +23
```
claims: { sub: <user_id>, role: "authenticated", aud: "authenticated", exp: now+1h, iat: now }
```

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Add a language tag to the claims code fence.

This keeps markdown lint clean and improves rendering/tool support.

Suggested doc patch
-```+```json
claims: { sub: <user_id>, role: "authenticated", aud: "authenticated", exp: now+1h, iat: now }
</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **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.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)

[warning] 21-21: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🤖 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/security/realtime-jwt-bridge-design.md` around lines 21 - 23, The code
fence showing the JWT claims (claims: { sub: <user_id>, role: "authenticated",
aud: "authenticated", exp: now+1h, iat: now }) lacks a language tag; update the
fenced block to use a JSON language tag (e.g., ```json) so the claims snippet is
properly highlighted and markdown lint passes.

Source: Linters/SAST tools

Comment on lines +64 to +68
```
curl -s -o /dev/null -w "%{http_code}\n" \
"https://jxqcmjqtjlpuxfrxmrdv.supabase.co/rest/v1/users?select=id&limit=1" \
-H "apikey: <ANON_KEY>" -H "Authorization: Bearer <ANON_KEY>"
```

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Parameterize the REST verification URL and annotate the fence language.

The check currently hard-codes one project URL, which makes branch/prod verification error-prone. Also add the missing code-block language for lint/tooling consistency.

Suggested doc patch
- ```+ ```bash
curl -s -o /dev/null -w "%{http_code}\n" \
- "https://jxqcmjqtjlpuxfrxmrdv.supabase.co/rest/v1/users?select=id&limit=1" \+ "${SUPABASE_URL}/rest/v1/users?select=id&limit=1" \
-H "apikey: <ANON_KEY>" -H "Authorization: Bearer <ANON_KEY>"
```
📝 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
```
curl -s -o /dev/null -w "%{http_code}\n" \
"https://jxqcmjqtjlpuxfrxmrdv.supabase.co/rest/v1/users?select=id&limit=1" \
-H "apikey: <ANON_KEY>" -H "Authorization: Bearer <ANON_KEY>"
```
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)

[warning] 64-64: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🤖 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/security/rls-lockdown-plan.md` around lines 64 - 68, Replace the
hard-coded project URL in the curl check with a parameterized environment
variable and mark the fenced code block as bash: change the request URL string
to use "${SUPABASE_URL}/rest/v1/users?select=id&limit=1" instead of the fixed
"https://jxqcmjqtjlpuxfrxmrdv.supabase.co/..." and add the fence language
identifier "bash" to the triple-backtick that opens the snippet so the block
reads as a bash example; keep the header flags (-H "apikey: <ANON_KEY>" -H
"Authorization: Bearer <ANON_KEY>") unchanged.

Source: Linters/SAST tools

Comment on lines +23 to +24
| `issues-media-files` | **false** | new backend endpoint (multipart → service-key upload), reusing `request_limits.read_within_limit` + content-type allowlist (the #220/#229 pattern) | backend signed URL (admin view) |
| `application_resumes` | **false** | already backend (`careers.py`) | backend signed URL (admin view) |

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Address the breaking change: existing issue reports with public screenshot URLs will fail once buckets go private.

The plan correctly proposes making issues-media-files and application_resumes private, but existing rows in issue_reports.screenshot_urls (and any admin code reading them) currently store/expect public URLs. After the buckets are private, those URLs will return 400/403.

Define a migration strategy:

  • Should existing public URLs be migrated to signed URLs (backfill)?
  • Should the admin view handle both old public-URL format and new signed-URL format during a transition window?
  • Should old URLs be invalidated or mapped to a signed-URL cache?

Clarify the timeline and contract change before applying the SQL.

Also applies to: 40-41

🤖 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/security/storage-hardening-plan.md` around lines 23 - 24, The change to
make the `issues-media-files` and `application_resumes` buckets private breaks
existing values in `issue_reports.screenshot_urls` (and any admin code expecting
public URLs); implement a migration and compatibility plan: write a DB migration
script that backfills existing public URLs in `issue_reports.screenshot_urls` to
signed URLs (using the same signing logic you will deploy), and add a fallback
in the admin handlers (the code paths that read `issue_reports.screenshot_urls`
and the admin signed-URL view, plus `careers.py` for resumes) to accept both
legacy public-URL strings and new signed-URL strings during a transition window;
publish a clear timeline and contract change in the SQL deployment notes, add a
signed-URL cache or short TTL mapping if you need to avoid re-signing on every
read, and ensure tests cover backfill, fallback parsing, and expiration behavior
before flipping the bucket privacy flag.

No new storage.objects policies are needed: backend uploads/reads use `service_role` (bypasses storage RLS). `avatars` stays `public=true` so its objects remain readable without a policy.

### Backend
- New `POST /api/issue-reports/screenshot` (auth-gated via `get_session_user_id`): accepts the file, validates type+size with the shared `request_limits` helpers, uploads to `issues-media-files` with the service key (mirror `careers._upload_resume`), returns the storage path (not a public URL).

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Clarify validation logic for the new screenshot upload endpoint.

The plan references "reusing request_limits.read_within_limit + content-type allowlist (the #220/#229 pattern)" but does not detail:

  • What content types are allowed for issue-report screenshots?
  • What is the file-size limit?
  • How are validation errors returned to the frontend?
  • Is there rate-limiting on uploads per user/session?

Document these constraints explicitly so the implementation is not ambiguous.

🤖 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/security/storage-hardening-plan.md` at line 40, Add explicit validation
rules to the design for the POST /api/issue-reports/screenshot endpoint: state
that request_limits.read_within_limit will enforce a max file size of 5MB
(consistent with other UI assets) and that the content-type allowlist will
accept image/png, image/jpeg, image/webp (list these MIME types explicitly);
describe that the endpoint is auth-gated via get_session_user_id and that
validation failures return a 400 JSON response with { error:
"validation_failed", details: "<reason>" } while auth failures return 401 and
storage/upload errors return 500 with { error: "upload_failed" }; note that
uploads are stored to issues-media-files using the same service-key flow as
careers._upload_resume and that a per-user rate limit of 10 uploads per hour
should be enforced (or call out that an equivalent existing rate-limiter must be
applied), so implementers know the exact size, types, error format, storage
target, auth hook, and rate-limiting expectation.


### Backend
- New `POST /api/issue-reports/screenshot` (auth-gated via `get_session_user_id`): accepts the file, validates type+size with the shared `request_limits` helpers, uploads to `issues-media-files` with the service key (mirror `careers._upload_resume`), returns the storage path (not a public URL).
- Signed-URL helper for private buckets (admin views of screenshots/résumés): backend issues a short-TTL signed URL via the storage REST API with the service key.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Specify signed-URL TTL, refresh strategy, and error handling.

The plan mentions "short-TTL signed URL" for private-bucket reads but does not specify:

  • Exact TTL value (seconds/minutes)?
  • How the frontend admin view handles token expiry if a user is viewing a list for >TTL?
  • What happens if signed-URL generation fails (e.g., service_role key temporarily unavailable)?
  • Should signed URLs be cached/pre-fetched or generated on-demand?
  • Interaction with browser cache headers (Cache-Control) to avoid stale signed URLs.

Add concrete values and error paths before implementation.

🤖 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/security/storage-hardening-plan.md` at line 41, The Signed-URL helper
for private buckets lacks concrete TTL, refresh and error-handling requirements;
update the design to specify a short TTL (e.g., 5 minutes), a client refresh
strategy (frontend should request a new signed URL on-demand when a URL is older
than 4 minutes or when a 401/403 occurs, and proactively refresh visible items
when an admin opens the list), failure handling (backend should return a clear
5xx/4xx error with retryable flag when service_role key is unavailable; frontend
should show a retry UI and backoff retries, and fall back to a placeholder image
or “unavailable” state), caching policy (do not cache signed URLs in long-lived
storage; allow in-memory per-view caching for the TTL only and invalidate on
refresh), and browser cache headers (backend must set Cache-Control: private,
max-age=0, no-store or max-age equal to TTL if safe and include no-transform to
avoid intermediaries caching stale URLs). Implement these concrete values and
flows in the Signed-URL helper and the admin-view code paths before proceeding
to implementation.

@Jose-Gael-Cruz-Lopez

Copy link
Copy Markdown
MemberAuthor

Superseded for the docs: the three planning docs are landing via #233 (docs-only). This PR is kept as the applied-to-prod record of the RLS lockdown SQL (rls_lockdown.sql / rls_lockdown_rollback.sql) — already applied directly to production on 2026-06-13 (anon confirmed locked out). Do not merge (the SQL ran straight on prod; nothing re-runs it from the repo). Keep open as the record, or close at your discretion.

Jose-Gael-Cruz-Lopez added a commit that referenced this pull request Jun 14, 2026
…n plans (#231)
Carved from the #232 draft so the planning docs live in main (the SQL stays in
#232 as the applied-to-prod record, not merged). The RLS lockdown is already
applied to production (anon locked out, confirmed). #230 references the
realtime-jwt-bridge design doc landed here.
Jose-Gael-Cruz-Lopez added a commit that referenced this pull request Jun 17, 2026
…us (#231)
Resolve the APPLIED-vs-draft contradiction: Phase 1 (application_resumes →
private) was genuinely applied to prod on 2026-06-15 (commit d14b14f, MCP,
verified public=false, with the CDN-cache caveat learned during verification),
so it is now framed as an after-the-fact RECORD across the SQL file and plan
doc. Phase 2 (issues-media-files) stays clearly draft/not-applied. Removed the
stale top-line "Nothing applied to prod" / "apply now" wording that conflicted
with the recorded Phase 1.
Also:
- Wrap Phase 1 UPDATE in BEGIN/COMMIT for copy-paste safety + Phase 2 parity.
- Phase 2b drops use DROP POLICY IF EXISTS (name drift won't fail the txn);
rollback CREATEs guarded with DROP IF EXISTS to stay idempotent.
- Document the cross-PR ordering constraint: #238 supersedes the old copy of
storage-hardening-plan.md in #232, so #238 must merge AFTER #232 (or #232
must drop the file) to avoid a silent revert.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- rls_lockdown.sql: make ALTER DEFAULT PRIVILEGES REVOKE robust across
table-creating roles (per-role scoping) with a documented discovery query
and FOR ROLE placeholders so a future migration run as another role can't
silently re-grant anon. Mirror the per-role GRANT in the rollback.
- rls-lockdown-plan.md: mark the "anon has zero table DML" query as a
recurring BLOCKING check to wire into CI/cron; clarify verification step 3
so a permission-denied error OR an empty 200 [] both count as PASS (RLS vs
REVOKE), not a false failure.
- realtime-jwt-bridge-design.md: add a presence/typing channel section
(presence:${roomId} in Social.tsx) noting it is NOT broken by the anon DML
REVOKE and that its authorization must be re-verified under setAuth(jwt)/
private channels; promote the asymmetric-vs-HS256 signing-key-type check to
a BLOCKING precondition (wrong key type = silent 401 on setAuth).
- storage-hardening-plan.md: add SUPERSEDED-by-#238 banner (and cross-link in
the lockdown plan) since #238 rewrites the storage plan and corrects two
factual errors in this copy.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Jose-Gael-Cruz-Lopez added a commit that referenced this pull request Jun 17, 2026
…231)
Reconcile the inconsistent résumé/object count (table said 13, prose/SQL
said 12) to a consistent ~12-13 with a snapshot caveat (observed 2026-06-15,
re-verify at apply), since prod cannot be queried here.
Correct the cross-PR ordering notes: the file is add/add between #232 and
#238, so the second merge raises an explicit CONFLICT (add/add) — not a
"silent overwrite/revert". Resolve in #238's favor; recommendation (merge
#232 first or drop the file from #232) unchanged.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Jose-Gael-Cruz-Lopez added a commit that referenced this pull request Jun 24, 2026
…ount
Three correctness fixes to the plan doc (still DO NOT MERGE/APPLY):
- Major: Phase 2a (route issue-report screenshots through the backend) has
already landed on main (feedback.py POST /api/issue-reports/screenshot,
ReportIssueFlow.tsx off supabase.storage, test_issue_screenshot_auth.py).
Update the status block and Phase 2 section so the only remaining storage
step is Phase 2b (flip issues-media-files private).
- Minor: the doc already exists on main (it is a modification, not an add),
so there is no add/add conflict with #232. Replace the cross-PR ordering
section with an accurate note.
- Minor: drop the loose/stale ~12-13 object count; the bucket holds résumé
PII regardless of count, so no number is pinned.
@Jose-Gael-Cruz-Lopez

Copy link
Copy Markdown
MemberAuthor

Re-verified #231 storage against prod (read-only) on 2026-07-01: the storage portion of this draft is largely done already — application_resumes is private, screenshots upload via the auth-gated POST /api/issue-reports/screenshot (service role, feedback.py), and the anon "Allow uploads" policy is scoped to issues-media-files. The only residual (making issues-media-files private + dropping the 2 anon storage.objects policies) is now a focused migration in #304 (0029_storage_lockdown_231.sql).

Suggest this draft's storage scope be closed as superseded by #304; its realtime-RLS content (a separate concern) is untouched and can proceed on its own.

@Jose-Gael-Cruz-Lopez

Copy link
Copy Markdown
MemberAuthor

Closing without merge — this draft served its purpose and everything in it is either live or superseded:

One follow-up remains: backend/db/security/rls_lockdown.sql + rls_lockdown_rollback.sql exist only on this branch and are the sole in-repo copy of DDL that is live on prod. The branch security/231-rls-lockdown-drafts is intentionally not deleted until that SQL is extracted into a numbered migration (the 0029 precedent). The realtime JWT-bridge design (option a) also remains unbuilt — design doc is on main at docs/security/realtime-jwt-bridge-design.md.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@Jose-Gael-Cruz-Lopez
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

DRAFT (#231): RLS lockdown SQL + storage & realtime(a) remediation plans — do not merge/apply - #232

Closed
Jose-Gael-Cruz-Lopez wants to merge 2 commits into
mainfrom
security/231-rls-lockdown-drafts
Closed

DRAFT (#231): RLS lockdown SQL + storage & realtime(a) remediation plans — do not merge/apply#232
Jose-Gael-Cruz-Lopez wants to merge 2 commits into
mainfrom
security/231-rls-lockdown-drafts

Conversation

@Jose-Gael-Cruz-Lopez

@Jose-Gael-Cruz-LopezJose-Gael-Cruz-Lopez commented Jun 13, 2026

Copy link
Copy Markdown
Member

DRAFT for review — do NOT merge or apply. Remediation artifacts for #231 (public anon key has full DML on 38/40 RLS-disabled tables → potential full-DB read/write via PostgREST, bypassing the backend). No code or prod changes here.

URGENT — RLS lockdown (one-time DDL, not a #197 migration)

  • backend/db/security/rls_lockdown.sql: ENABLE ROW LEVEL SECURITY on the 38 tables that lack it + REVOKE all anon DML across the public schema, plus ALTER DEFAULT PRIVILEGES so future tables don't silently re-grant anon.
  • Verified safe for the backend:service_role has rolbypassrls=true (live-checked), so every backend query is unaffected; only anon/authenticated (bypass=false) are constrained.
  • rls_lockdown_rollback.sql: emergency revert (restores the insecure state).
  • docs/security/rls-lockdown-plan.md: apply/verify/rollback checklist (incl. the anon-blocked REST check and the service_role-bypass proof) + Supabase-branching note (branching was unreachable via MCP).
  • Accepted breakage: anon realtime on room_messages pauses until option (a) lands; chat still loads/refreshes via the service-role REST path (fix(social): re-fetch realtime messages via decrypting REST + drop dead reactions sub (#124, #231) #230).

Storage hardening plan

docs/security/storage-hardening-plan.md: issues-media-files + application_resumesprivate + signed URLs + backend-side uploads; avatars stays public-read with the blanket public INSERT revoked; drop the global storage.objects public-INSERT policy. (Live finding: chat-images/cosmetic-assets buckets don't exist — dead code.)

Realtime option (a) design

docs/security/realtime-jwt-bridge-design.md: mint a Supabase JWT at login + realtime.setAuth, membership-scoped RLS SELECT policy on room_messages; postgres_changes + RLS (minimal) vs private channels. JWT refresh flagged (30d session vs ~1h JWT). Builds after the lockdown.

How to use

Review the SQL, then you apply rls_lockdown.sql (branch-first if available). I did not apply anything. Storage SQL + realtime are plans to turn into their own PRs once you approve the approach.

Summary by CodeRabbit

  • Documentation

    • Added security planning guides documenting database access control implementation, real-time authentication architecture, and storage hardening strategies with verification procedures.
  • Chores

    • Added database security configuration scripts and corresponding rollback procedures to support infrastructure security improvements.

…drafts (#231)
DRAFTS ONLY — do not apply; for review. Addresses the #231 finding that the
public anon key has full DML on 38/40 RLS-disabled public tables (potential
full-DB read/write via PostgREST, bypassing the backend).
- backend/db/security/rls_lockdown.sql — enable RLS on the 38 tables + revoke
anon DML across the public schema (+ default privileges). Verified safe:
service_role has rolbypassrls=true, so the backend is unaffected.
- backend/db/security/rls_lockdown_rollback.sql — emergency revert (restores
the insecure state).
- docs/security/rls-lockdown-plan.md — apply/verify/rollback checklist; branch
note (branching unreachable via MCP).
- docs/security/storage-hardening-plan.md — issues-media-files + application_
resumes -> private + signed URLs + backend uploads; avatars public-read,
revoke blanket public INSERT; kill the global storage public-INSERT policy.
- docs/security/realtime-jwt-bridge-design.md — option (a): mint Supabase JWT
at login + setAuth, room_messages membership RLS policy; flags JWT refresh
(30d session vs ~1h JWT). Lands after the lockdown to restore realtime.
No code/prod changes in this PR.
@coderabbitai

coderabbitaiBot commented Jun 13, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@Jose-Gael-Cruz-Lopez, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 58 minutes and 13 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 0ed9b2de-7de8-4558-8028-28bd9d97574d

📥 Commits

Reviewing files that changed from the base of the PR and between 5b1aabb and 07fca30.

📒 Files selected for processing (5)
  • backend/db/security/rls_lockdown.sql
  • backend/db/security/rls_lockdown_rollback.sql
  • docs/security/realtime-jwt-bridge-design.md
  • docs/security/rls-lockdown-plan.md
  • docs/security/storage-hardening-plan.md
📝 Walkthrough

Walkthrough

This PR establishes a project-wide security hardening initiative by introducing RLS lockdown DDL scripts, verification procedures, and design plans for restoring Realtime functionality and hardening storage access—all without requiring backend code changes since the service role bypasses RLS.

Changes

Security Hardening Initiative

Layer / File(s)Summary
RLS Lockdown DDL Apply and Rollback Scripts
backend/db/security/rls_lockdown.sql, backend/db/security/rls_lockdown_rollback.sql
Transactional SQL scripts enable RLS on 38 public tables and revoke all anon DML permissions (SELECT/INSERT/UPDATE/DELETE) plus future default privileges; rollback re-grants anon access and disables RLS while preserving achievement table state.
RLS Lockdown Verification and Deployment Plan
docs/security/rls-lockdown-plan.md
Documents lockdown scope, backend safety via service_role bypass, expected anon realtime breakage on room_messages, test-first and prod deployment procedures, pre-apply snapshots, post-apply verification checklist (RLS status, anon grants, REST auth, backend pytest, realtime behavior), rollback steps, and follow-up notes on authenticated grants and storage hardening.
Realtime JWT Bridge Design for RLS-Protected Room Messages
docs/security/realtime-jwt-bridge-design.md
Proposes backend JWT-minting endpoint with SUPABASE_JWT_SECRET, membership-scoped SELECT policy on room_messages using auth.uid matched to room_members, frontend token fetch post-login with realtime.setAuth, JWT refresh loop with reconnect/sleep handling and REST fallback, plus optional room_reactions follow-up.
Storage Bucket Hardening Plan
docs/security/storage-hardening-plan.md
Draft plan to restrict public-read buckets (issues-media-files, application_resumes) via SQL, drop public INSERT policies, route writes through backend service_role, provide signed URLs for private reads, leave avatars public, with corresponding backend upload/signing endpoints and frontend wiring plus verification checklist.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related issues

  • #231: The RLS lockdown DDL scripts and realtime-JWT bridge design address the same tables, policies, and realtime-auth gap (room_messages/room_reactions membership RLS and JWT-based Realtime authentication) described in that issue.

Poem

A rabbit stands guard at the gate today,
Locking down rows in the SQL way—
Anon must ask now, through tokens bright,
While service_role dances through the night. 🐰🔐

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check nameStatusExplanation
Title check✅ PassedThe title accurately references the RLS lockdown (#231) and lists the main remediation artifacts (SQL, storage & realtime plans), marking it as DRAFT — clearly indicating this is a review-stage document.
Description check✅ PassedThe description covers objectives (RLS lockdown, storage hardening, realtime design), key files/changes, verification notes, and usage instructions, but deviates from the template structure with narrative format instead of structured sections.
Docstring Coverage✅ PassedNo functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check✅ PassedCheck skipped because no linked issues were found for this pull request.
Out of Scope Changes check✅ PassedCheck skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch security/231-rls-lockdown-drafts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@cloudflare-workers-and-pages

cloudflare-workers-and-pagesBot commented Jun 13, 2026

Copy link
Copy Markdown

Deploying with Cloudflare Workers Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

StatusNameLatest CommitPreview URLUpdated (UTC)
✅ Deployment successful!
View logs
frontend07fca30Commit Preview URL

Branch Preview URL
Jun 17 2026, 01:24 AM

@coderabbitaicoderabbitaiBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🧹 Nitpick comments (1)
docs/security/storage-hardening-plan.md (1)

48-50: ⚡ Quick win

Provide concrete SQL queries for the verification checklist.

The verification steps are described narratively (e.g., "storage.buckets: issues-media-files and application_resumes show public=false") but would be more reliable if accompanied by concrete SQL queries that can be automated in tests or runbooks. For example:

  • Query to check storage.buckets.public flag
  • Query to list storage.objects policies and confirm the two public ones are gone
  • Query to test anon-role upload attempt (expect denial)

Add explicit SQL commands so verifiers can run them post-apply.

🤖 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/security/storage-hardening-plan.md` around lines 48 - 50, Add explicit,
runnable SQL and verification commands to the checklist: (1) a SELECT against
storage.buckets filtering name IN
('issues-media-files','application_resumes','avatars') returning the public
column and asserting issues-media-files and application_resumes have public =
false while avatars = true; (2) a SELECT against the PostgreSQL policy metadata
for the storage schema (pg_policies or the pg_catalog view) filtering on
table_name = 'objects' (and/or table_name = 'buckets' if used) to confirm the
two former {public} policies are absent; (3) a SELECT against storage.objects
(or listing via the storage API) to confirm object visibility matches bucket
public flags; (4) a concrete anon-role upload test (curl or psql INSERT attempt
authenticated as anon) that expects a 4xx denial; and (5) a signed-URL GET test
(curl) that expects 200; for each step include the exact command to run and the
expected result/output assertion so the checks can be automated in
runbooks/tests (reference symbols: storage.buckets, storage.objects,
pg_policies, anon role, signed URL).
🤖 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 `@backend/db/security/rls_lockdown.sql`:
- Around line 79-83: Update the ALTER DEFAULT PRIVILEGES statements to target
the table-creator role(s) explicitly: in backend/db/security/rls_lockdown.sql
(lines 79-83) change the generic "ALTER DEFAULT PRIVILEGES IN SCHEMA public
REVOKE ... FROM anon" to "ALTER DEFAULT PRIVILEGES FOR ROLE <creator_role> IN
SCHEMA public REVOKE SELECT, INSERT, UPDATE, DELETE ON TABLES FROM anon;" for
each relevant creator role, and in backend/db/security/rls_lockdown_rollback.sql
(lines 19-21) mirror that change so the rollback uses "ALTER DEFAULT PRIVILEGES
FOR ROLE <creator_role> IN SCHEMA public GRANT SELECT, INSERT, UPDATE, DELETE ON
TABLES TO anon;" (replace <creator_role> with the actual DB role(s) that create
tables); after applying, verify pg_default_acl contains no default table
privileges granting anon in the public schema.
In `@docs/security/realtime-jwt-bridge-design.md`:
- Around line 21-23: The code fence showing the JWT claims (claims: { sub:
<user_id>, role: "authenticated", aud: "authenticated", exp: now+1h, iat: now })
lacks a language tag; update the fenced block to use a JSON language tag (e.g.,
```json) so the claims snippet is properly highlighted and markdown lint passes.
In `@docs/security/rls-lockdown-plan.md`:
- Around line 64-68: Replace the hard-coded project URL in the curl check with a
parameterized environment variable and mark the fenced code block as bash:
change the request URL string to use
"${SUPABASE_URL}/rest/v1/users?select=id&limit=1" instead of the fixed
"https://jxqcmjqtjlpuxfrxmrdv.supabase.co/..." and add the fence language
identifier "bash" to the triple-backtick that opens the snippet so the block
reads as a bash example; keep the header flags (-H "apikey: <ANON_KEY>" -H
"Authorization: Bearer <ANON_KEY>") unchanged.
In `@docs/security/storage-hardening-plan.md`:
- Line 40: Add explicit validation rules to the design for the POST
/api/issue-reports/screenshot endpoint: state that
request_limits.read_within_limit will enforce a max file size of 5MB (consistent
with other UI assets) and that the content-type allowlist will accept image/png,
image/jpeg, image/webp (list these MIME types explicitly); describe that the
endpoint is auth-gated via get_session_user_id and that validation failures
return a 400 JSON response with { error: "validation_failed", details:
"<reason>" } while auth failures return 401 and storage/upload errors return 500
with { error: "upload_failed" }; note that uploads are stored to
issues-media-files using the same service-key flow as careers._upload_resume and
that a per-user rate limit of 10 uploads per hour should be enforced (or call
out that an equivalent existing rate-limiter must be applied), so implementers
know the exact size, types, error format, storage target, auth hook, and
rate-limiting expectation.
- Around line 23-24: The change to make the `issues-media-files` and
`application_resumes` buckets private breaks existing values in
`issue_reports.screenshot_urls` (and any admin code expecting public URLs);
implement a migration and compatibility plan: write a DB migration script that
backfills existing public URLs in `issue_reports.screenshot_urls` to signed URLs
(using the same signing logic you will deploy), and add a fallback in the admin
handlers (the code paths that read `issue_reports.screenshot_urls` and the admin
signed-URL view, plus `careers.py` for resumes) to accept both legacy public-URL
strings and new signed-URL strings during a transition window; publish a clear
timeline and contract change in the SQL deployment notes, add a signed-URL cache
or short TTL mapping if you need to avoid re-signing on every read, and ensure
tests cover backfill, fallback parsing, and expiration behavior before flipping
the bucket privacy flag.
- Line 41: The Signed-URL helper for private buckets lacks concrete TTL, refresh
and error-handling requirements; update the design to specify a short TTL (e.g.,
5 minutes), a client refresh strategy (frontend should request a new signed URL
on-demand when a URL is older than 4 minutes or when a 401/403 occurs, and
proactively refresh visible items when an admin opens the list), failure
handling (backend should return a clear 5xx/4xx error with retryable flag when
service_role key is unavailable; frontend should show a retry UI and backoff
retries, and fall back to a placeholder image or “unavailable” state), caching
policy (do not cache signed URLs in long-lived storage; allow in-memory per-view
caching for the TTL only and invalidate on refresh), and browser cache headers
(backend must set Cache-Control: private, max-age=0, no-store or max-age equal
to TTL if safe and include no-transform to avoid intermediaries caching stale
URLs). Implement these concrete values and flows in the Signed-URL helper and
the admin-view code paths before proceeding to implementation.
---
Nitpick comments:
In `@docs/security/storage-hardening-plan.md`:
- Around line 48-50: Add explicit, runnable SQL and verification commands to the
checklist: (1) a SELECT against storage.buckets filtering name IN
('issues-media-files','application_resumes','avatars') returning the public
column and asserting issues-media-files and application_resumes have public =
false while avatars = true; (2) a SELECT against the PostgreSQL policy metadata
for the storage schema (pg_policies or the pg_catalog view) filtering on
table_name = 'objects' (and/or table_name = 'buckets' if used) to confirm the
two former {public} policies are absent; (3) a SELECT against storage.objects
(or listing via the storage API) to confirm object visibility matches bucket
public flags; (4) a concrete anon-role upload test (curl or psql INSERT attempt
authenticated as anon) that expects a 4xx denial; and (5) a signed-URL GET test
(curl) that expects 200; for each step include the exact command to run and the
expected result/output assertion so the checks can be automated in
runbooks/tests (reference symbols: storage.buckets, storage.objects,
pg_policies, anon role, signed URL).
🪄 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: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: fc9f6d25-9286-4b5f-9117-d8df225ed12c

📥 Commits

Reviewing files that changed from the base of the PR and between a27b688 and 5b1aabb.

📒 Files selected for processing (5)
  • backend/db/security/rls_lockdown.sql
  • backend/db/security/rls_lockdown_rollback.sql
  • docs/security/realtime-jwt-bridge-design.md
  • docs/security/rls-lockdown-plan.md
  • docs/security/storage-hardening-plan.md

Comment threadbackend/db/security/rls_lockdown.sql
Comment on lines +21 to +23
```
claims: { sub: <user_id>, role: "authenticated", aud: "authenticated", exp: now+1h, iat: now }
```

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Add a language tag to the claims code fence.

This keeps markdown lint clean and improves rendering/tool support.

Suggested doc patch
-```+```json
claims: { sub: <user_id>, role: "authenticated", aud: "authenticated", exp: now+1h, iat: now }
</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **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.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)

[warning] 21-21: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🤖 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/security/realtime-jwt-bridge-design.md` around lines 21 - 23, The code
fence showing the JWT claims (claims: { sub: <user_id>, role: "authenticated",
aud: "authenticated", exp: now+1h, iat: now }) lacks a language tag; update the
fenced block to use a JSON language tag (e.g., ```json) so the claims snippet is
properly highlighted and markdown lint passes.

Source: Linters/SAST tools

Comment on lines +64 to +68
```
curl -s -o /dev/null -w "%{http_code}\n" \
"https://jxqcmjqtjlpuxfrxmrdv.supabase.co/rest/v1/users?select=id&limit=1" \
-H "apikey: <ANON_KEY>" -H "Authorization: Bearer <ANON_KEY>"
```

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Parameterize the REST verification URL and annotate the fence language.

The check currently hard-codes one project URL, which makes branch/prod verification error-prone. Also add the missing code-block language for lint/tooling consistency.

Suggested doc patch
- ```+ ```bash
curl -s -o /dev/null -w "%{http_code}\n" \
- "https://jxqcmjqtjlpuxfrxmrdv.supabase.co/rest/v1/users?select=id&limit=1" \+ "${SUPABASE_URL}/rest/v1/users?select=id&limit=1" \
-H "apikey: <ANON_KEY>" -H "Authorization: Bearer <ANON_KEY>"
```
📝 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
```
curl -s -o /dev/null -w "%{http_code}\n" \
"https://jxqcmjqtjlpuxfrxmrdv.supabase.co/rest/v1/users?select=id&limit=1" \
-H "apikey: <ANON_KEY>" -H "Authorization: Bearer <ANON_KEY>"
```
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)

[warning] 64-64: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🤖 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/security/rls-lockdown-plan.md` around lines 64 - 68, Replace the
hard-coded project URL in the curl check with a parameterized environment
variable and mark the fenced code block as bash: change the request URL string
to use "${SUPABASE_URL}/rest/v1/users?select=id&limit=1" instead of the fixed
"https://jxqcmjqtjlpuxfrxmrdv.supabase.co/..." and add the fence language
identifier "bash" to the triple-backtick that opens the snippet so the block
reads as a bash example; keep the header flags (-H "apikey: <ANON_KEY>" -H
"Authorization: Bearer <ANON_KEY>") unchanged.

Source: Linters/SAST tools

Comment on lines +23 to +24
| `issues-media-files` | **false** | new backend endpoint (multipart → service-key upload), reusing `request_limits.read_within_limit` + content-type allowlist (the #220/#229 pattern) | backend signed URL (admin view) |
| `application_resumes` | **false** | already backend (`careers.py`) | backend signed URL (admin view) |

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Address the breaking change: existing issue reports with public screenshot URLs will fail once buckets go private.

The plan correctly proposes making issues-media-files and application_resumes private, but existing rows in issue_reports.screenshot_urls (and any admin code reading them) currently store/expect public URLs. After the buckets are private, those URLs will return 400/403.

Define a migration strategy:

  • Should existing public URLs be migrated to signed URLs (backfill)?
  • Should the admin view handle both old public-URL format and new signed-URL format during a transition window?
  • Should old URLs be invalidated or mapped to a signed-URL cache?

Clarify the timeline and contract change before applying the SQL.

Also applies to: 40-41

🤖 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/security/storage-hardening-plan.md` around lines 23 - 24, The change to
make the `issues-media-files` and `application_resumes` buckets private breaks
existing values in `issue_reports.screenshot_urls` (and any admin code expecting
public URLs); implement a migration and compatibility plan: write a DB migration
script that backfills existing public URLs in `issue_reports.screenshot_urls` to
signed URLs (using the same signing logic you will deploy), and add a fallback
in the admin handlers (the code paths that read `issue_reports.screenshot_urls`
and the admin signed-URL view, plus `careers.py` for resumes) to accept both
legacy public-URL strings and new signed-URL strings during a transition window;
publish a clear timeline and contract change in the SQL deployment notes, add a
signed-URL cache or short TTL mapping if you need to avoid re-signing on every
read, and ensure tests cover backfill, fallback parsing, and expiration behavior
before flipping the bucket privacy flag.

No new storage.objects policies are needed: backend uploads/reads use `service_role` (bypasses storage RLS). `avatars` stays `public=true` so its objects remain readable without a policy.

### Backend
- New `POST /api/issue-reports/screenshot` (auth-gated via `get_session_user_id`): accepts the file, validates type+size with the shared `request_limits` helpers, uploads to `issues-media-files` with the service key (mirror `careers._upload_resume`), returns the storage path (not a public URL).

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Clarify validation logic for the new screenshot upload endpoint.

The plan references "reusing request_limits.read_within_limit + content-type allowlist (the #220/#229 pattern)" but does not detail:

  • What content types are allowed for issue-report screenshots?
  • What is the file-size limit?
  • How are validation errors returned to the frontend?
  • Is there rate-limiting on uploads per user/session?

Document these constraints explicitly so the implementation is not ambiguous.

🤖 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/security/storage-hardening-plan.md` at line 40, Add explicit validation
rules to the design for the POST /api/issue-reports/screenshot endpoint: state
that request_limits.read_within_limit will enforce a max file size of 5MB
(consistent with other UI assets) and that the content-type allowlist will
accept image/png, image/jpeg, image/webp (list these MIME types explicitly);
describe that the endpoint is auth-gated via get_session_user_id and that
validation failures return a 400 JSON response with { error:
"validation_failed", details: "<reason>" } while auth failures return 401 and
storage/upload errors return 500 with { error: "upload_failed" }; note that
uploads are stored to issues-media-files using the same service-key flow as
careers._upload_resume and that a per-user rate limit of 10 uploads per hour
should be enforced (or call out that an equivalent existing rate-limiter must be
applied), so implementers know the exact size, types, error format, storage
target, auth hook, and rate-limiting expectation.


### Backend
- New `POST /api/issue-reports/screenshot` (auth-gated via `get_session_user_id`): accepts the file, validates type+size with the shared `request_limits` helpers, uploads to `issues-media-files` with the service key (mirror `careers._upload_resume`), returns the storage path (not a public URL).
- Signed-URL helper for private buckets (admin views of screenshots/résumés): backend issues a short-TTL signed URL via the storage REST API with the service key.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Specify signed-URL TTL, refresh strategy, and error handling.

The plan mentions "short-TTL signed URL" for private-bucket reads but does not specify:

  • Exact TTL value (seconds/minutes)?
  • How the frontend admin view handles token expiry if a user is viewing a list for >TTL?
  • What happens if signed-URL generation fails (e.g., service_role key temporarily unavailable)?
  • Should signed URLs be cached/pre-fetched or generated on-demand?
  • Interaction with browser cache headers (Cache-Control) to avoid stale signed URLs.

Add concrete values and error paths before implementation.

🤖 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/security/storage-hardening-plan.md` at line 41, The Signed-URL helper
for private buckets lacks concrete TTL, refresh and error-handling requirements;
update the design to specify a short TTL (e.g., 5 minutes), a client refresh
strategy (frontend should request a new signed URL on-demand when a URL is older
than 4 minutes or when a 401/403 occurs, and proactively refresh visible items
when an admin opens the list), failure handling (backend should return a clear
5xx/4xx error with retryable flag when service_role key is unavailable; frontend
should show a retry UI and backoff retries, and fall back to a placeholder image
or “unavailable” state), caching policy (do not cache signed URLs in long-lived
storage; allow in-memory per-view caching for the TTL only and invalidate on
refresh), and browser cache headers (backend must set Cache-Control: private,
max-age=0, no-store or max-age equal to TTL if safe and include no-transform to
avoid intermediaries caching stale URLs). Implement these concrete values and
flows in the Signed-URL helper and the admin-view code paths before proceeding
to implementation.

@Jose-Gael-Cruz-Lopez

Copy link
Copy Markdown
MemberAuthor

Superseded for the docs: the three planning docs are landing via #233 (docs-only). This PR is kept as the applied-to-prod record of the RLS lockdown SQL (rls_lockdown.sql / rls_lockdown_rollback.sql) — already applied directly to production on 2026-06-13 (anon confirmed locked out). Do not merge (the SQL ran straight on prod; nothing re-runs it from the repo). Keep open as the record, or close at your discretion.

Jose-Gael-Cruz-Lopez added a commit that referenced this pull request Jun 14, 2026
…n plans (#231)
Carved from the #232 draft so the planning docs live in main (the SQL stays in
#232 as the applied-to-prod record, not merged). The RLS lockdown is already
applied to production (anon locked out, confirmed). #230 references the
realtime-jwt-bridge design doc landed here.
Jose-Gael-Cruz-Lopez added a commit that referenced this pull request Jun 17, 2026
…us (#231)
Resolve the APPLIED-vs-draft contradiction: Phase 1 (application_resumes →
private) was genuinely applied to prod on 2026-06-15 (commit d14b14f, MCP,
verified public=false, with the CDN-cache caveat learned during verification),
so it is now framed as an after-the-fact RECORD across the SQL file and plan
doc. Phase 2 (issues-media-files) stays clearly draft/not-applied. Removed the
stale top-line "Nothing applied to prod" / "apply now" wording that conflicted
with the recorded Phase 1.
Also:
- Wrap Phase 1 UPDATE in BEGIN/COMMIT for copy-paste safety + Phase 2 parity.
- Phase 2b drops use DROP POLICY IF EXISTS (name drift won't fail the txn);
rollback CREATEs guarded with DROP IF EXISTS to stay idempotent.
- Document the cross-PR ordering constraint: #238 supersedes the old copy of
storage-hardening-plan.md in #232, so #238 must merge AFTER #232 (or #232
must drop the file) to avoid a silent revert.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- rls_lockdown.sql: make ALTER DEFAULT PRIVILEGES REVOKE robust across
table-creating roles (per-role scoping) with a documented discovery query
and FOR ROLE placeholders so a future migration run as another role can't
silently re-grant anon. Mirror the per-role GRANT in the rollback.
- rls-lockdown-plan.md: mark the "anon has zero table DML" query as a
recurring BLOCKING check to wire into CI/cron; clarify verification step 3
so a permission-denied error OR an empty 200 [] both count as PASS (RLS vs
REVOKE), not a false failure.
- realtime-jwt-bridge-design.md: add a presence/typing channel section
(presence:${roomId} in Social.tsx) noting it is NOT broken by the anon DML
REVOKE and that its authorization must be re-verified under setAuth(jwt)/
private channels; promote the asymmetric-vs-HS256 signing-key-type check to
a BLOCKING precondition (wrong key type = silent 401 on setAuth).
- storage-hardening-plan.md: add SUPERSEDED-by-#238 banner (and cross-link in
the lockdown plan) since #238 rewrites the storage plan and corrects two
factual errors in this copy.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Jose-Gael-Cruz-Lopez added a commit that referenced this pull request Jun 17, 2026
…231)
Reconcile the inconsistent résumé/object count (table said 13, prose/SQL
said 12) to a consistent ~12-13 with a snapshot caveat (observed 2026-06-15,
re-verify at apply), since prod cannot be queried here.
Correct the cross-PR ordering notes: the file is add/add between #232 and
#238, so the second merge raises an explicit CONFLICT (add/add) — not a
"silent overwrite/revert". Resolve in #238's favor; recommendation (merge
#232 first or drop the file from #232) unchanged.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Jose-Gael-Cruz-Lopez added a commit that referenced this pull request Jun 24, 2026
…ount
Three correctness fixes to the plan doc (still DO NOT MERGE/APPLY):
- Major: Phase 2a (route issue-report screenshots through the backend) has
already landed on main (feedback.py POST /api/issue-reports/screenshot,
ReportIssueFlow.tsx off supabase.storage, test_issue_screenshot_auth.py).
Update the status block and Phase 2 section so the only remaining storage
step is Phase 2b (flip issues-media-files private).
- Minor: the doc already exists on main (it is a modification, not an add),
so there is no add/add conflict with #232. Replace the cross-PR ordering
section with an accurate note.
- Minor: drop the loose/stale ~12-13 object count; the bucket holds résumé
PII regardless of count, so no number is pinned.
@Jose-Gael-Cruz-Lopez

Copy link
Copy Markdown
MemberAuthor

Re-verified #231 storage against prod (read-only) on 2026-07-01: the storage portion of this draft is largely done already — application_resumes is private, screenshots upload via the auth-gated POST /api/issue-reports/screenshot (service role, feedback.py), and the anon "Allow uploads" policy is scoped to issues-media-files. The only residual (making issues-media-files private + dropping the 2 anon storage.objects policies) is now a focused migration in #304 (0029_storage_lockdown_231.sql).

Suggest this draft's storage scope be closed as superseded by #304; its realtime-RLS content (a separate concern) is untouched and can proceed on its own.

@Jose-Gael-Cruz-Lopez

Copy link
Copy Markdown
MemberAuthor

Closing without merge — this draft served its purpose and everything in it is either live or superseded:

One follow-up remains: backend/db/security/rls_lockdown.sql + rls_lockdown_rollback.sql exist only on this branch and are the sole in-repo copy of DDL that is live on prod. The branch security/231-rls-lockdown-drafts is intentionally not deleted until that SQL is extracted into a numbered migration (the 0029 precedent). The realtime JWT-bridge design (option a) also remains unbuilt — design doc is on main at docs/security/realtime-jwt-bridge-design.md.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@Jose-Gael-Cruz-Lopez
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

DRAFT (#231): RLS lockdown SQL + storage & realtime(a) remediation plans — do not merge/apply - #232

Closed
Jose-Gael-Cruz-Lopez wants to merge 2 commits into
mainfrom
security/231-rls-lockdown-drafts
Closed

DRAFT (#231): RLS lockdown SQL + storage & realtime(a) remediation plans — do not merge/apply#232
Jose-Gael-Cruz-Lopez wants to merge 2 commits into
mainfrom
security/231-rls-lockdown-drafts

Conversation

@Jose-Gael-Cruz-Lopez

@Jose-Gael-Cruz-LopezJose-Gael-Cruz-Lopez commented Jun 13, 2026

Copy link
Copy Markdown
Member

DRAFT for review — do NOT merge or apply. Remediation artifacts for #231 (public anon key has full DML on 38/40 RLS-disabled tables → potential full-DB read/write via PostgREST, bypassing the backend). No code or prod changes here.

URGENT — RLS lockdown (one-time DDL, not a #197 migration)

  • backend/db/security/rls_lockdown.sql: ENABLE ROW LEVEL SECURITY on the 38 tables that lack it + REVOKE all anon DML across the public schema, plus ALTER DEFAULT PRIVILEGES so future tables don't silently re-grant anon.
  • Verified safe for the backend:service_role has rolbypassrls=true (live-checked), so every backend query is unaffected; only anon/authenticated (bypass=false) are constrained.
  • rls_lockdown_rollback.sql: emergency revert (restores the insecure state).
  • docs/security/rls-lockdown-plan.md: apply/verify/rollback checklist (incl. the anon-blocked REST check and the service_role-bypass proof) + Supabase-branching note (branching was unreachable via MCP).
  • Accepted breakage: anon realtime on room_messages pauses until option (a) lands; chat still loads/refreshes via the service-role REST path (fix(social): re-fetch realtime messages via decrypting REST + drop dead reactions sub (#124, #231) #230).

Storage hardening plan

docs/security/storage-hardening-plan.md: issues-media-files + application_resumesprivate + signed URLs + backend-side uploads; avatars stays public-read with the blanket public INSERT revoked; drop the global storage.objects public-INSERT policy. (Live finding: chat-images/cosmetic-assets buckets don't exist — dead code.)

Realtime option (a) design

docs/security/realtime-jwt-bridge-design.md: mint a Supabase JWT at login + realtime.setAuth, membership-scoped RLS SELECT policy on room_messages; postgres_changes + RLS (minimal) vs private channels. JWT refresh flagged (30d session vs ~1h JWT). Builds after the lockdown.

How to use

Review the SQL, then you apply rls_lockdown.sql (branch-first if available). I did not apply anything. Storage SQL + realtime are plans to turn into their own PRs once you approve the approach.

Summary by CodeRabbit

  • Documentation

    • Added security planning guides documenting database access control implementation, real-time authentication architecture, and storage hardening strategies with verification procedures.
  • Chores

    • Added database security configuration scripts and corresponding rollback procedures to support infrastructure security improvements.

…drafts (#231)
DRAFTS ONLY — do not apply; for review. Addresses the #231 finding that the
public anon key has full DML on 38/40 RLS-disabled public tables (potential
full-DB read/write via PostgREST, bypassing the backend).
- backend/db/security/rls_lockdown.sql — enable RLS on the 38 tables + revoke
anon DML across the public schema (+ default privileges). Verified safe:
service_role has rolbypassrls=true, so the backend is unaffected.
- backend/db/security/rls_lockdown_rollback.sql — emergency revert (restores
the insecure state).
- docs/security/rls-lockdown-plan.md — apply/verify/rollback checklist; branch
note (branching unreachable via MCP).
- docs/security/storage-hardening-plan.md — issues-media-files + application_
resumes -> private + signed URLs + backend uploads; avatars public-read,
revoke blanket public INSERT; kill the global storage public-INSERT policy.
- docs/security/realtime-jwt-bridge-design.md — option (a): mint Supabase JWT
at login + setAuth, room_messages membership RLS policy; flags JWT refresh
(30d session vs ~1h JWT). Lands after the lockdown to restore realtime.
No code/prod changes in this PR.
@coderabbitai

coderabbitaiBot commented Jun 13, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@Jose-Gael-Cruz-Lopez, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 58 minutes and 13 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 0ed9b2de-7de8-4558-8028-28bd9d97574d

📥 Commits

Reviewing files that changed from the base of the PR and between 5b1aabb and 07fca30.

📒 Files selected for processing (5)
  • backend/db/security/rls_lockdown.sql
  • backend/db/security/rls_lockdown_rollback.sql
  • docs/security/realtime-jwt-bridge-design.md
  • docs/security/rls-lockdown-plan.md
  • docs/security/storage-hardening-plan.md
📝 Walkthrough

Walkthrough

This PR establishes a project-wide security hardening initiative by introducing RLS lockdown DDL scripts, verification procedures, and design plans for restoring Realtime functionality and hardening storage access—all without requiring backend code changes since the service role bypasses RLS.

Changes

Security Hardening Initiative

Layer / File(s)Summary
RLS Lockdown DDL Apply and Rollback Scripts
backend/db/security/rls_lockdown.sql, backend/db/security/rls_lockdown_rollback.sql
Transactional SQL scripts enable RLS on 38 public tables and revoke all anon DML permissions (SELECT/INSERT/UPDATE/DELETE) plus future default privileges; rollback re-grants anon access and disables RLS while preserving achievement table state.
RLS Lockdown Verification and Deployment Plan
docs/security/rls-lockdown-plan.md
Documents lockdown scope, backend safety via service_role bypass, expected anon realtime breakage on room_messages, test-first and prod deployment procedures, pre-apply snapshots, post-apply verification checklist (RLS status, anon grants, REST auth, backend pytest, realtime behavior), rollback steps, and follow-up notes on authenticated grants and storage hardening.
Realtime JWT Bridge Design for RLS-Protected Room Messages
docs/security/realtime-jwt-bridge-design.md
Proposes backend JWT-minting endpoint with SUPABASE_JWT_SECRET, membership-scoped SELECT policy on room_messages using auth.uid matched to room_members, frontend token fetch post-login with realtime.setAuth, JWT refresh loop with reconnect/sleep handling and REST fallback, plus optional room_reactions follow-up.
Storage Bucket Hardening Plan
docs/security/storage-hardening-plan.md
Draft plan to restrict public-read buckets (issues-media-files, application_resumes) via SQL, drop public INSERT policies, route writes through backend service_role, provide signed URLs for private reads, leave avatars public, with corresponding backend upload/signing endpoints and frontend wiring plus verification checklist.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related issues

  • #231: The RLS lockdown DDL scripts and realtime-JWT bridge design address the same tables, policies, and realtime-auth gap (room_messages/room_reactions membership RLS and JWT-based Realtime authentication) described in that issue.

Poem

A rabbit stands guard at the gate today,
Locking down rows in the SQL way—
Anon must ask now, through tokens bright,
While service_role dances through the night. 🐰🔐

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check nameStatusExplanation
Title check✅ PassedThe title accurately references the RLS lockdown (#231) and lists the main remediation artifacts (SQL, storage & realtime plans), marking it as DRAFT — clearly indicating this is a review-stage document.
Description check✅ PassedThe description covers objectives (RLS lockdown, storage hardening, realtime design), key files/changes, verification notes, and usage instructions, but deviates from the template structure with narrative format instead of structured sections.
Docstring Coverage✅ PassedNo functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check✅ PassedCheck skipped because no linked issues were found for this pull request.
Out of Scope Changes check✅ PassedCheck skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch security/231-rls-lockdown-drafts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@cloudflare-workers-and-pages

cloudflare-workers-and-pagesBot commented Jun 13, 2026

Copy link
Copy Markdown

Deploying with Cloudflare Workers Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

StatusNameLatest CommitPreview URLUpdated (UTC)
✅ Deployment successful!
View logs
frontend07fca30Commit Preview URL

Branch Preview URL
Jun 17 2026, 01:24 AM

@coderabbitaicoderabbitaiBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🧹 Nitpick comments (1)
docs/security/storage-hardening-plan.md (1)

48-50: ⚡ Quick win

Provide concrete SQL queries for the verification checklist.

The verification steps are described narratively (e.g., "storage.buckets: issues-media-files and application_resumes show public=false") but would be more reliable if accompanied by concrete SQL queries that can be automated in tests or runbooks. For example:

  • Query to check storage.buckets.public flag
  • Query to list storage.objects policies and confirm the two public ones are gone
  • Query to test anon-role upload attempt (expect denial)

Add explicit SQL commands so verifiers can run them post-apply.

🤖 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/security/storage-hardening-plan.md` around lines 48 - 50, Add explicit,
runnable SQL and verification commands to the checklist: (1) a SELECT against
storage.buckets filtering name IN
('issues-media-files','application_resumes','avatars') returning the public
column and asserting issues-media-files and application_resumes have public =
false while avatars = true; (2) a SELECT against the PostgreSQL policy metadata
for the storage schema (pg_policies or the pg_catalog view) filtering on
table_name = 'objects' (and/or table_name = 'buckets' if used) to confirm the
two former {public} policies are absent; (3) a SELECT against storage.objects
(or listing via the storage API) to confirm object visibility matches bucket
public flags; (4) a concrete anon-role upload test (curl or psql INSERT attempt
authenticated as anon) that expects a 4xx denial; and (5) a signed-URL GET test
(curl) that expects 200; for each step include the exact command to run and the
expected result/output assertion so the checks can be automated in
runbooks/tests (reference symbols: storage.buckets, storage.objects,
pg_policies, anon role, signed URL).
🤖 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 `@backend/db/security/rls_lockdown.sql`:
- Around line 79-83: Update the ALTER DEFAULT PRIVILEGES statements to target
the table-creator role(s) explicitly: in backend/db/security/rls_lockdown.sql
(lines 79-83) change the generic "ALTER DEFAULT PRIVILEGES IN SCHEMA public
REVOKE ... FROM anon" to "ALTER DEFAULT PRIVILEGES FOR ROLE <creator_role> IN
SCHEMA public REVOKE SELECT, INSERT, UPDATE, DELETE ON TABLES FROM anon;" for
each relevant creator role, and in backend/db/security/rls_lockdown_rollback.sql
(lines 19-21) mirror that change so the rollback uses "ALTER DEFAULT PRIVILEGES
FOR ROLE <creator_role> IN SCHEMA public GRANT SELECT, INSERT, UPDATE, DELETE ON
TABLES TO anon;" (replace <creator_role> with the actual DB role(s) that create
tables); after applying, verify pg_default_acl contains no default table
privileges granting anon in the public schema.
In `@docs/security/realtime-jwt-bridge-design.md`:
- Around line 21-23: The code fence showing the JWT claims (claims: { sub:
<user_id>, role: "authenticated", aud: "authenticated", exp: now+1h, iat: now })
lacks a language tag; update the fenced block to use a JSON language tag (e.g.,
```json) so the claims snippet is properly highlighted and markdown lint passes.
In `@docs/security/rls-lockdown-plan.md`:
- Around line 64-68: Replace the hard-coded project URL in the curl check with a
parameterized environment variable and mark the fenced code block as bash:
change the request URL string to use
"${SUPABASE_URL}/rest/v1/users?select=id&limit=1" instead of the fixed
"https://jxqcmjqtjlpuxfrxmrdv.supabase.co/..." and add the fence language
identifier "bash" to the triple-backtick that opens the snippet so the block
reads as a bash example; keep the header flags (-H "apikey: <ANON_KEY>" -H
"Authorization: Bearer <ANON_KEY>") unchanged.
In `@docs/security/storage-hardening-plan.md`:
- Line 40: Add explicit validation rules to the design for the POST
/api/issue-reports/screenshot endpoint: state that
request_limits.read_within_limit will enforce a max file size of 5MB (consistent
with other UI assets) and that the content-type allowlist will accept image/png,
image/jpeg, image/webp (list these MIME types explicitly); describe that the
endpoint is auth-gated via get_session_user_id and that validation failures
return a 400 JSON response with { error: "validation_failed", details:
"<reason>" } while auth failures return 401 and storage/upload errors return 500
with { error: "upload_failed" }; note that uploads are stored to
issues-media-files using the same service-key flow as careers._upload_resume and
that a per-user rate limit of 10 uploads per hour should be enforced (or call
out that an equivalent existing rate-limiter must be applied), so implementers
know the exact size, types, error format, storage target, auth hook, and
rate-limiting expectation.
- Around line 23-24: The change to make the `issues-media-files` and
`application_resumes` buckets private breaks existing values in
`issue_reports.screenshot_urls` (and any admin code expecting public URLs);
implement a migration and compatibility plan: write a DB migration script that
backfills existing public URLs in `issue_reports.screenshot_urls` to signed URLs
(using the same signing logic you will deploy), and add a fallback in the admin
handlers (the code paths that read `issue_reports.screenshot_urls` and the admin
signed-URL view, plus `careers.py` for resumes) to accept both legacy public-URL
strings and new signed-URL strings during a transition window; publish a clear
timeline and contract change in the SQL deployment notes, add a signed-URL cache
or short TTL mapping if you need to avoid re-signing on every read, and ensure
tests cover backfill, fallback parsing, and expiration behavior before flipping
the bucket privacy flag.
- Line 41: The Signed-URL helper for private buckets lacks concrete TTL, refresh
and error-handling requirements; update the design to specify a short TTL (e.g.,
5 minutes), a client refresh strategy (frontend should request a new signed URL
on-demand when a URL is older than 4 minutes or when a 401/403 occurs, and
proactively refresh visible items when an admin opens the list), failure
handling (backend should return a clear 5xx/4xx error with retryable flag when
service_role key is unavailable; frontend should show a retry UI and backoff
retries, and fall back to a placeholder image or “unavailable” state), caching
policy (do not cache signed URLs in long-lived storage; allow in-memory per-view
caching for the TTL only and invalidate on refresh), and browser cache headers
(backend must set Cache-Control: private, max-age=0, no-store or max-age equal
to TTL if safe and include no-transform to avoid intermediaries caching stale
URLs). Implement these concrete values and flows in the Signed-URL helper and
the admin-view code paths before proceeding to implementation.
---
Nitpick comments:
In `@docs/security/storage-hardening-plan.md`:
- Around line 48-50: Add explicit, runnable SQL and verification commands to the
checklist: (1) a SELECT against storage.buckets filtering name IN
('issues-media-files','application_resumes','avatars') returning the public
column and asserting issues-media-files and application_resumes have public =
false while avatars = true; (2) a SELECT against the PostgreSQL policy metadata
for the storage schema (pg_policies or the pg_catalog view) filtering on
table_name = 'objects' (and/or table_name = 'buckets' if used) to confirm the
two former {public} policies are absent; (3) a SELECT against storage.objects
(or listing via the storage API) to confirm object visibility matches bucket
public flags; (4) a concrete anon-role upload test (curl or psql INSERT attempt
authenticated as anon) that expects a 4xx denial; and (5) a signed-URL GET test
(curl) that expects 200; for each step include the exact command to run and the
expected result/output assertion so the checks can be automated in
runbooks/tests (reference symbols: storage.buckets, storage.objects,
pg_policies, anon role, signed URL).
🪄 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: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: fc9f6d25-9286-4b5f-9117-d8df225ed12c

📥 Commits

Reviewing files that changed from the base of the PR and between a27b688 and 5b1aabb.

📒 Files selected for processing (5)
  • backend/db/security/rls_lockdown.sql
  • backend/db/security/rls_lockdown_rollback.sql
  • docs/security/realtime-jwt-bridge-design.md
  • docs/security/rls-lockdown-plan.md
  • docs/security/storage-hardening-plan.md

Comment threadbackend/db/security/rls_lockdown.sql
Comment on lines +21 to +23
```
claims: { sub: <user_id>, role: "authenticated", aud: "authenticated", exp: now+1h, iat: now }
```

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Add a language tag to the claims code fence.

This keeps markdown lint clean and improves rendering/tool support.

Suggested doc patch
-```+```json
claims: { sub: <user_id>, role: "authenticated", aud: "authenticated", exp: now+1h, iat: now }
</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **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.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)

[warning] 21-21: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🤖 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/security/realtime-jwt-bridge-design.md` around lines 21 - 23, The code
fence showing the JWT claims (claims: { sub: <user_id>, role: "authenticated",
aud: "authenticated", exp: now+1h, iat: now }) lacks a language tag; update the
fenced block to use a JSON language tag (e.g., ```json) so the claims snippet is
properly highlighted and markdown lint passes.

Source: Linters/SAST tools

Comment on lines +64 to +68
```
curl -s -o /dev/null -w "%{http_code}\n" \
"https://jxqcmjqtjlpuxfrxmrdv.supabase.co/rest/v1/users?select=id&limit=1" \
-H "apikey: <ANON_KEY>" -H "Authorization: Bearer <ANON_KEY>"
```

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Parameterize the REST verification URL and annotate the fence language.

The check currently hard-codes one project URL, which makes branch/prod verification error-prone. Also add the missing code-block language for lint/tooling consistency.

Suggested doc patch
- ```+ ```bash
curl -s -o /dev/null -w "%{http_code}\n" \
- "https://jxqcmjqtjlpuxfrxmrdv.supabase.co/rest/v1/users?select=id&limit=1" \+ "${SUPABASE_URL}/rest/v1/users?select=id&limit=1" \
-H "apikey: <ANON_KEY>" -H "Authorization: Bearer <ANON_KEY>"
```
📝 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
```
curl -s -o /dev/null -w "%{http_code}\n" \
"https://jxqcmjqtjlpuxfrxmrdv.supabase.co/rest/v1/users?select=id&limit=1" \
-H "apikey: <ANON_KEY>" -H "Authorization: Bearer <ANON_KEY>"
```
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)

[warning] 64-64: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🤖 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/security/rls-lockdown-plan.md` around lines 64 - 68, Replace the
hard-coded project URL in the curl check with a parameterized environment
variable and mark the fenced code block as bash: change the request URL string
to use "${SUPABASE_URL}/rest/v1/users?select=id&limit=1" instead of the fixed
"https://jxqcmjqtjlpuxfrxmrdv.supabase.co/..." and add the fence language
identifier "bash" to the triple-backtick that opens the snippet so the block
reads as a bash example; keep the header flags (-H "apikey: <ANON_KEY>" -H
"Authorization: Bearer <ANON_KEY>") unchanged.

Source: Linters/SAST tools

Comment on lines +23 to +24
| `issues-media-files` | **false** | new backend endpoint (multipart → service-key upload), reusing `request_limits.read_within_limit` + content-type allowlist (the #220/#229 pattern) | backend signed URL (admin view) |
| `application_resumes` | **false** | already backend (`careers.py`) | backend signed URL (admin view) |

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Address the breaking change: existing issue reports with public screenshot URLs will fail once buckets go private.

The plan correctly proposes making issues-media-files and application_resumes private, but existing rows in issue_reports.screenshot_urls (and any admin code reading them) currently store/expect public URLs. After the buckets are private, those URLs will return 400/403.

Define a migration strategy:

  • Should existing public URLs be migrated to signed URLs (backfill)?
  • Should the admin view handle both old public-URL format and new signed-URL format during a transition window?
  • Should old URLs be invalidated or mapped to a signed-URL cache?

Clarify the timeline and contract change before applying the SQL.

Also applies to: 40-41

🤖 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/security/storage-hardening-plan.md` around lines 23 - 24, The change to
make the `issues-media-files` and `application_resumes` buckets private breaks
existing values in `issue_reports.screenshot_urls` (and any admin code expecting
public URLs); implement a migration and compatibility plan: write a DB migration
script that backfills existing public URLs in `issue_reports.screenshot_urls` to
signed URLs (using the same signing logic you will deploy), and add a fallback
in the admin handlers (the code paths that read `issue_reports.screenshot_urls`
and the admin signed-URL view, plus `careers.py` for resumes) to accept both
legacy public-URL strings and new signed-URL strings during a transition window;
publish a clear timeline and contract change in the SQL deployment notes, add a
signed-URL cache or short TTL mapping if you need to avoid re-signing on every
read, and ensure tests cover backfill, fallback parsing, and expiration behavior
before flipping the bucket privacy flag.

No new storage.objects policies are needed: backend uploads/reads use `service_role` (bypasses storage RLS). `avatars` stays `public=true` so its objects remain readable without a policy.

### Backend
- New `POST /api/issue-reports/screenshot` (auth-gated via `get_session_user_id`): accepts the file, validates type+size with the shared `request_limits` helpers, uploads to `issues-media-files` with the service key (mirror `careers._upload_resume`), returns the storage path (not a public URL).

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Clarify validation logic for the new screenshot upload endpoint.

The plan references "reusing request_limits.read_within_limit + content-type allowlist (the #220/#229 pattern)" but does not detail:

  • What content types are allowed for issue-report screenshots?
  • What is the file-size limit?
  • How are validation errors returned to the frontend?
  • Is there rate-limiting on uploads per user/session?

Document these constraints explicitly so the implementation is not ambiguous.

🤖 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/security/storage-hardening-plan.md` at line 40, Add explicit validation
rules to the design for the POST /api/issue-reports/screenshot endpoint: state
that request_limits.read_within_limit will enforce a max file size of 5MB
(consistent with other UI assets) and that the content-type allowlist will
accept image/png, image/jpeg, image/webp (list these MIME types explicitly);
describe that the endpoint is auth-gated via get_session_user_id and that
validation failures return a 400 JSON response with { error:
"validation_failed", details: "<reason>" } while auth failures return 401 and
storage/upload errors return 500 with { error: "upload_failed" }; note that
uploads are stored to issues-media-files using the same service-key flow as
careers._upload_resume and that a per-user rate limit of 10 uploads per hour
should be enforced (or call out that an equivalent existing rate-limiter must be
applied), so implementers know the exact size, types, error format, storage
target, auth hook, and rate-limiting expectation.


### Backend
- New `POST /api/issue-reports/screenshot` (auth-gated via `get_session_user_id`): accepts the file, validates type+size with the shared `request_limits` helpers, uploads to `issues-media-files` with the service key (mirror `careers._upload_resume`), returns the storage path (not a public URL).
- Signed-URL helper for private buckets (admin views of screenshots/résumés): backend issues a short-TTL signed URL via the storage REST API with the service key.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Specify signed-URL TTL, refresh strategy, and error handling.

The plan mentions "short-TTL signed URL" for private-bucket reads but does not specify:

  • Exact TTL value (seconds/minutes)?
  • How the frontend admin view handles token expiry if a user is viewing a list for >TTL?
  • What happens if signed-URL generation fails (e.g., service_role key temporarily unavailable)?
  • Should signed URLs be cached/pre-fetched or generated on-demand?
  • Interaction with browser cache headers (Cache-Control) to avoid stale signed URLs.

Add concrete values and error paths before implementation.

🤖 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/security/storage-hardening-plan.md` at line 41, The Signed-URL helper
for private buckets lacks concrete TTL, refresh and error-handling requirements;
update the design to specify a short TTL (e.g., 5 minutes), a client refresh
strategy (frontend should request a new signed URL on-demand when a URL is older
than 4 minutes or when a 401/403 occurs, and proactively refresh visible items
when an admin opens the list), failure handling (backend should return a clear
5xx/4xx error with retryable flag when service_role key is unavailable; frontend
should show a retry UI and backoff retries, and fall back to a placeholder image
or “unavailable” state), caching policy (do not cache signed URLs in long-lived
storage; allow in-memory per-view caching for the TTL only and invalidate on
refresh), and browser cache headers (backend must set Cache-Control: private,
max-age=0, no-store or max-age equal to TTL if safe and include no-transform to
avoid intermediaries caching stale URLs). Implement these concrete values and
flows in the Signed-URL helper and the admin-view code paths before proceeding
to implementation.

@Jose-Gael-Cruz-Lopez

Copy link
Copy Markdown
MemberAuthor

Superseded for the docs: the three planning docs are landing via #233 (docs-only). This PR is kept as the applied-to-prod record of the RLS lockdown SQL (rls_lockdown.sql / rls_lockdown_rollback.sql) — already applied directly to production on 2026-06-13 (anon confirmed locked out). Do not merge (the SQL ran straight on prod; nothing re-runs it from the repo). Keep open as the record, or close at your discretion.

Jose-Gael-Cruz-Lopez added a commit that referenced this pull request Jun 14, 2026
…n plans (#231)
Carved from the #232 draft so the planning docs live in main (the SQL stays in
#232 as the applied-to-prod record, not merged). The RLS lockdown is already
applied to production (anon locked out, confirmed). #230 references the
realtime-jwt-bridge design doc landed here.
Jose-Gael-Cruz-Lopez added a commit that referenced this pull request Jun 17, 2026
…us (#231)
Resolve the APPLIED-vs-draft contradiction: Phase 1 (application_resumes →
private) was genuinely applied to prod on 2026-06-15 (commit d14b14f, MCP,
verified public=false, with the CDN-cache caveat learned during verification),
so it is now framed as an after-the-fact RECORD across the SQL file and plan
doc. Phase 2 (issues-media-files) stays clearly draft/not-applied. Removed the
stale top-line "Nothing applied to prod" / "apply now" wording that conflicted
with the recorded Phase 1.
Also:
- Wrap Phase 1 UPDATE in BEGIN/COMMIT for copy-paste safety + Phase 2 parity.
- Phase 2b drops use DROP POLICY IF EXISTS (name drift won't fail the txn);
rollback CREATEs guarded with DROP IF EXISTS to stay idempotent.
- Document the cross-PR ordering constraint: #238 supersedes the old copy of
storage-hardening-plan.md in #232, so #238 must merge AFTER #232 (or #232
must drop the file) to avoid a silent revert.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- rls_lockdown.sql: make ALTER DEFAULT PRIVILEGES REVOKE robust across
table-creating roles (per-role scoping) with a documented discovery query
and FOR ROLE placeholders so a future migration run as another role can't
silently re-grant anon. Mirror the per-role GRANT in the rollback.
- rls-lockdown-plan.md: mark the "anon has zero table DML" query as a
recurring BLOCKING check to wire into CI/cron; clarify verification step 3
so a permission-denied error OR an empty 200 [] both count as PASS (RLS vs
REVOKE), not a false failure.
- realtime-jwt-bridge-design.md: add a presence/typing channel section
(presence:${roomId} in Social.tsx) noting it is NOT broken by the anon DML
REVOKE and that its authorization must be re-verified under setAuth(jwt)/
private channels; promote the asymmetric-vs-HS256 signing-key-type check to
a BLOCKING precondition (wrong key type = silent 401 on setAuth).
- storage-hardening-plan.md: add SUPERSEDED-by-#238 banner (and cross-link in
the lockdown plan) since #238 rewrites the storage plan and corrects two
factual errors in this copy.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Jose-Gael-Cruz-Lopez added a commit that referenced this pull request Jun 17, 2026
…231)
Reconcile the inconsistent résumé/object count (table said 13, prose/SQL
said 12) to a consistent ~12-13 with a snapshot caveat (observed 2026-06-15,
re-verify at apply), since prod cannot be queried here.
Correct the cross-PR ordering notes: the file is add/add between #232 and
#238, so the second merge raises an explicit CONFLICT (add/add) — not a
"silent overwrite/revert". Resolve in #238's favor; recommendation (merge
#232 first or drop the file from #232) unchanged.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Jose-Gael-Cruz-Lopez added a commit that referenced this pull request Jun 24, 2026
…ount
Three correctness fixes to the plan doc (still DO NOT MERGE/APPLY):
- Major: Phase 2a (route issue-report screenshots through the backend) has
already landed on main (feedback.py POST /api/issue-reports/screenshot,
ReportIssueFlow.tsx off supabase.storage, test_issue_screenshot_auth.py).
Update the status block and Phase 2 section so the only remaining storage
step is Phase 2b (flip issues-media-files private).
- Minor: the doc already exists on main (it is a modification, not an add),
so there is no add/add conflict with #232. Replace the cross-PR ordering
section with an accurate note.
- Minor: drop the loose/stale ~12-13 object count; the bucket holds résumé
PII regardless of count, so no number is pinned.
@Jose-Gael-Cruz-Lopez

Copy link
Copy Markdown
MemberAuthor

Re-verified #231 storage against prod (read-only) on 2026-07-01: the storage portion of this draft is largely done already — application_resumes is private, screenshots upload via the auth-gated POST /api/issue-reports/screenshot (service role, feedback.py), and the anon "Allow uploads" policy is scoped to issues-media-files. The only residual (making issues-media-files private + dropping the 2 anon storage.objects policies) is now a focused migration in #304 (0029_storage_lockdown_231.sql).

Suggest this draft's storage scope be closed as superseded by #304; its realtime-RLS content (a separate concern) is untouched and can proceed on its own.

@Jose-Gael-Cruz-Lopez

Copy link
Copy Markdown
MemberAuthor

Closing without merge — this draft served its purpose and everything in it is either live or superseded:

One follow-up remains: backend/db/security/rls_lockdown.sql + rls_lockdown_rollback.sql exist only on this branch and are the sole in-repo copy of DDL that is live on prod. The branch security/231-rls-lockdown-drafts is intentionally not deleted until that SQL is extracted into a numbered migration (the 0029 precedent). The realtime JWT-bridge design (option a) also remains unbuilt — design doc is on main at docs/security/realtime-jwt-bridge-design.md.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@Jose-Gael-Cruz-Lopez
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

DRAFT (#231): RLS lockdown SQL + storage & realtime(a) remediation plans — do not merge/apply - #232

Closed
Jose-Gael-Cruz-Lopez wants to merge 2 commits into
mainfrom
security/231-rls-lockdown-drafts
Closed

DRAFT (#231): RLS lockdown SQL + storage & realtime(a) remediation plans — do not merge/apply#232
Jose-Gael-Cruz-Lopez wants to merge 2 commits into
mainfrom
security/231-rls-lockdown-drafts

Conversation

@Jose-Gael-Cruz-Lopez

@Jose-Gael-Cruz-LopezJose-Gael-Cruz-Lopez commented Jun 13, 2026

Copy link
Copy Markdown
Member

DRAFT for review — do NOT merge or apply. Remediation artifacts for #231 (public anon key has full DML on 38/40 RLS-disabled tables → potential full-DB read/write via PostgREST, bypassing the backend). No code or prod changes here.

URGENT — RLS lockdown (one-time DDL, not a #197 migration)

  • backend/db/security/rls_lockdown.sql: ENABLE ROW LEVEL SECURITY on the 38 tables that lack it + REVOKE all anon DML across the public schema, plus ALTER DEFAULT PRIVILEGES so future tables don't silently re-grant anon.
  • Verified safe for the backend:service_role has rolbypassrls=true (live-checked), so every backend query is unaffected; only anon/authenticated (bypass=false) are constrained.
  • rls_lockdown_rollback.sql: emergency revert (restores the insecure state).
  • docs/security/rls-lockdown-plan.md: apply/verify/rollback checklist (incl. the anon-blocked REST check and the service_role-bypass proof) + Supabase-branching note (branching was unreachable via MCP).
  • Accepted breakage: anon realtime on room_messages pauses until option (a) lands; chat still loads/refreshes via the service-role REST path (fix(social): re-fetch realtime messages via decrypting REST + drop dead reactions sub (#124, #231) #230).

Storage hardening plan

docs/security/storage-hardening-plan.md: issues-media-files + application_resumesprivate + signed URLs + backend-side uploads; avatars stays public-read with the blanket public INSERT revoked; drop the global storage.objects public-INSERT policy. (Live finding: chat-images/cosmetic-assets buckets don't exist — dead code.)

Realtime option (a) design

docs/security/realtime-jwt-bridge-design.md: mint a Supabase JWT at login + realtime.setAuth, membership-scoped RLS SELECT policy on room_messages; postgres_changes + RLS (minimal) vs private channels. JWT refresh flagged (30d session vs ~1h JWT). Builds after the lockdown.

How to use

Review the SQL, then you apply rls_lockdown.sql (branch-first if available). I did not apply anything. Storage SQL + realtime are plans to turn into their own PRs once you approve the approach.

Summary by CodeRabbit

  • Documentation

    • Added security planning guides documenting database access control implementation, real-time authentication architecture, and storage hardening strategies with verification procedures.
  • Chores

    • Added database security configuration scripts and corresponding rollback procedures to support infrastructure security improvements.

…drafts (#231)
DRAFTS ONLY — do not apply; for review. Addresses the #231 finding that the
public anon key has full DML on 38/40 RLS-disabled public tables (potential
full-DB read/write via PostgREST, bypassing the backend).
- backend/db/security/rls_lockdown.sql — enable RLS on the 38 tables + revoke
anon DML across the public schema (+ default privileges). Verified safe:
service_role has rolbypassrls=true, so the backend is unaffected.
- backend/db/security/rls_lockdown_rollback.sql — emergency revert (restores
the insecure state).
- docs/security/rls-lockdown-plan.md — apply/verify/rollback checklist; branch
note (branching unreachable via MCP).
- docs/security/storage-hardening-plan.md — issues-media-files + application_
resumes -> private + signed URLs + backend uploads; avatars public-read,
revoke blanket public INSERT; kill the global storage public-INSERT policy.
- docs/security/realtime-jwt-bridge-design.md — option (a): mint Supabase JWT
at login + setAuth, room_messages membership RLS policy; flags JWT refresh
(30d session vs ~1h JWT). Lands after the lockdown to restore realtime.
No code/prod changes in this PR.
@coderabbitai

coderabbitaiBot commented Jun 13, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@Jose-Gael-Cruz-Lopez, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 58 minutes and 13 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 0ed9b2de-7de8-4558-8028-28bd9d97574d

📥 Commits

Reviewing files that changed from the base of the PR and between 5b1aabb and 07fca30.

📒 Files selected for processing (5)
  • backend/db/security/rls_lockdown.sql
  • backend/db/security/rls_lockdown_rollback.sql
  • docs/security/realtime-jwt-bridge-design.md
  • docs/security/rls-lockdown-plan.md
  • docs/security/storage-hardening-plan.md
📝 Walkthrough

Walkthrough

This PR establishes a project-wide security hardening initiative by introducing RLS lockdown DDL scripts, verification procedures, and design plans for restoring Realtime functionality and hardening storage access—all without requiring backend code changes since the service role bypasses RLS.

Changes

Security Hardening Initiative

Layer / File(s)Summary
RLS Lockdown DDL Apply and Rollback Scripts
backend/db/security/rls_lockdown.sql, backend/db/security/rls_lockdown_rollback.sql
Transactional SQL scripts enable RLS on 38 public tables and revoke all anon DML permissions (SELECT/INSERT/UPDATE/DELETE) plus future default privileges; rollback re-grants anon access and disables RLS while preserving achievement table state.
RLS Lockdown Verification and Deployment Plan
docs/security/rls-lockdown-plan.md
Documents lockdown scope, backend safety via service_role bypass, expected anon realtime breakage on room_messages, test-first and prod deployment procedures, pre-apply snapshots, post-apply verification checklist (RLS status, anon grants, REST auth, backend pytest, realtime behavior), rollback steps, and follow-up notes on authenticated grants and storage hardening.
Realtime JWT Bridge Design for RLS-Protected Room Messages
docs/security/realtime-jwt-bridge-design.md
Proposes backend JWT-minting endpoint with SUPABASE_JWT_SECRET, membership-scoped SELECT policy on room_messages using auth.uid matched to room_members, frontend token fetch post-login with realtime.setAuth, JWT refresh loop with reconnect/sleep handling and REST fallback, plus optional room_reactions follow-up.
Storage Bucket Hardening Plan
docs/security/storage-hardening-plan.md
Draft plan to restrict public-read buckets (issues-media-files, application_resumes) via SQL, drop public INSERT policies, route writes through backend service_role, provide signed URLs for private reads, leave avatars public, with corresponding backend upload/signing endpoints and frontend wiring plus verification checklist.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related issues

  • #231: The RLS lockdown DDL scripts and realtime-JWT bridge design address the same tables, policies, and realtime-auth gap (room_messages/room_reactions membership RLS and JWT-based Realtime authentication) described in that issue.

Poem

A rabbit stands guard at the gate today,
Locking down rows in the SQL way—
Anon must ask now, through tokens bright,
While service_role dances through the night. 🐰🔐

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check nameStatusExplanation
Title check✅ PassedThe title accurately references the RLS lockdown (#231) and lists the main remediation artifacts (SQL, storage & realtime plans), marking it as DRAFT — clearly indicating this is a review-stage document.
Description check✅ PassedThe description covers objectives (RLS lockdown, storage hardening, realtime design), key files/changes, verification notes, and usage instructions, but deviates from the template structure with narrative format instead of structured sections.
Docstring Coverage✅ PassedNo functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check✅ PassedCheck skipped because no linked issues were found for this pull request.
Out of Scope Changes check✅ PassedCheck skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch security/231-rls-lockdown-drafts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@cloudflare-workers-and-pages

cloudflare-workers-and-pagesBot commented Jun 13, 2026

Copy link
Copy Markdown

Deploying with Cloudflare Workers Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

StatusNameLatest CommitPreview URLUpdated (UTC)
✅ Deployment successful!
View logs
frontend07fca30Commit Preview URL

Branch Preview URL
Jun 17 2026, 01:24 AM

@coderabbitaicoderabbitaiBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🧹 Nitpick comments (1)
docs/security/storage-hardening-plan.md (1)

48-50: ⚡ Quick win

Provide concrete SQL queries for the verification checklist.

The verification steps are described narratively (e.g., "storage.buckets: issues-media-files and application_resumes show public=false") but would be more reliable if accompanied by concrete SQL queries that can be automated in tests or runbooks. For example:

  • Query to check storage.buckets.public flag
  • Query to list storage.objects policies and confirm the two public ones are gone
  • Query to test anon-role upload attempt (expect denial)

Add explicit SQL commands so verifiers can run them post-apply.

🤖 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/security/storage-hardening-plan.md` around lines 48 - 50, Add explicit,
runnable SQL and verification commands to the checklist: (1) a SELECT against
storage.buckets filtering name IN
('issues-media-files','application_resumes','avatars') returning the public
column and asserting issues-media-files and application_resumes have public =
false while avatars = true; (2) a SELECT against the PostgreSQL policy metadata
for the storage schema (pg_policies or the pg_catalog view) filtering on
table_name = 'objects' (and/or table_name = 'buckets' if used) to confirm the
two former {public} policies are absent; (3) a SELECT against storage.objects
(or listing via the storage API) to confirm object visibility matches bucket
public flags; (4) a concrete anon-role upload test (curl or psql INSERT attempt
authenticated as anon) that expects a 4xx denial; and (5) a signed-URL GET test
(curl) that expects 200; for each step include the exact command to run and the
expected result/output assertion so the checks can be automated in
runbooks/tests (reference symbols: storage.buckets, storage.objects,
pg_policies, anon role, signed URL).
🤖 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 `@backend/db/security/rls_lockdown.sql`:
- Around line 79-83: Update the ALTER DEFAULT PRIVILEGES statements to target
the table-creator role(s) explicitly: in backend/db/security/rls_lockdown.sql
(lines 79-83) change the generic "ALTER DEFAULT PRIVILEGES IN SCHEMA public
REVOKE ... FROM anon" to "ALTER DEFAULT PRIVILEGES FOR ROLE <creator_role> IN
SCHEMA public REVOKE SELECT, INSERT, UPDATE, DELETE ON TABLES FROM anon;" for
each relevant creator role, and in backend/db/security/rls_lockdown_rollback.sql
(lines 19-21) mirror that change so the rollback uses "ALTER DEFAULT PRIVILEGES
FOR ROLE <creator_role> IN SCHEMA public GRANT SELECT, INSERT, UPDATE, DELETE ON
TABLES TO anon;" (replace <creator_role> with the actual DB role(s) that create
tables); after applying, verify pg_default_acl contains no default table
privileges granting anon in the public schema.
In `@docs/security/realtime-jwt-bridge-design.md`:
- Around line 21-23: The code fence showing the JWT claims (claims: { sub:
<user_id>, role: "authenticated", aud: "authenticated", exp: now+1h, iat: now })
lacks a language tag; update the fenced block to use a JSON language tag (e.g.,
```json) so the claims snippet is properly highlighted and markdown lint passes.
In `@docs/security/rls-lockdown-plan.md`:
- Around line 64-68: Replace the hard-coded project URL in the curl check with a
parameterized environment variable and mark the fenced code block as bash:
change the request URL string to use
"${SUPABASE_URL}/rest/v1/users?select=id&limit=1" instead of the fixed
"https://jxqcmjqtjlpuxfrxmrdv.supabase.co/..." and add the fence language
identifier "bash" to the triple-backtick that opens the snippet so the block
reads as a bash example; keep the header flags (-H "apikey: <ANON_KEY>" -H
"Authorization: Bearer <ANON_KEY>") unchanged.
In `@docs/security/storage-hardening-plan.md`:
- Line 40: Add explicit validation rules to the design for the POST
/api/issue-reports/screenshot endpoint: state that
request_limits.read_within_limit will enforce a max file size of 5MB (consistent
with other UI assets) and that the content-type allowlist will accept image/png,
image/jpeg, image/webp (list these MIME types explicitly); describe that the
endpoint is auth-gated via get_session_user_id and that validation failures
return a 400 JSON response with { error: "validation_failed", details:
"<reason>" } while auth failures return 401 and storage/upload errors return 500
with { error: "upload_failed" }; note that uploads are stored to
issues-media-files using the same service-key flow as careers._upload_resume and
that a per-user rate limit of 10 uploads per hour should be enforced (or call
out that an equivalent existing rate-limiter must be applied), so implementers
know the exact size, types, error format, storage target, auth hook, and
rate-limiting expectation.
- Around line 23-24: The change to make the `issues-media-files` and
`application_resumes` buckets private breaks existing values in
`issue_reports.screenshot_urls` (and any admin code expecting public URLs);
implement a migration and compatibility plan: write a DB migration script that
backfills existing public URLs in `issue_reports.screenshot_urls` to signed URLs
(using the same signing logic you will deploy), and add a fallback in the admin
handlers (the code paths that read `issue_reports.screenshot_urls` and the admin
signed-URL view, plus `careers.py` for resumes) to accept both legacy public-URL
strings and new signed-URL strings during a transition window; publish a clear
timeline and contract change in the SQL deployment notes, add a signed-URL cache
or short TTL mapping if you need to avoid re-signing on every read, and ensure
tests cover backfill, fallback parsing, and expiration behavior before flipping
the bucket privacy flag.
- Line 41: The Signed-URL helper for private buckets lacks concrete TTL, refresh
and error-handling requirements; update the design to specify a short TTL (e.g.,
5 minutes), a client refresh strategy (frontend should request a new signed URL
on-demand when a URL is older than 4 minutes or when a 401/403 occurs, and
proactively refresh visible items when an admin opens the list), failure
handling (backend should return a clear 5xx/4xx error with retryable flag when
service_role key is unavailable; frontend should show a retry UI and backoff
retries, and fall back to a placeholder image or “unavailable” state), caching
policy (do not cache signed URLs in long-lived storage; allow in-memory per-view
caching for the TTL only and invalidate on refresh), and browser cache headers
(backend must set Cache-Control: private, max-age=0, no-store or max-age equal
to TTL if safe and include no-transform to avoid intermediaries caching stale
URLs). Implement these concrete values and flows in the Signed-URL helper and
the admin-view code paths before proceeding to implementation.
---
Nitpick comments:
In `@docs/security/storage-hardening-plan.md`:
- Around line 48-50: Add explicit, runnable SQL and verification commands to the
checklist: (1) a SELECT against storage.buckets filtering name IN
('issues-media-files','application_resumes','avatars') returning the public
column and asserting issues-media-files and application_resumes have public =
false while avatars = true; (2) a SELECT against the PostgreSQL policy metadata
for the storage schema (pg_policies or the pg_catalog view) filtering on
table_name = 'objects' (and/or table_name = 'buckets' if used) to confirm the
two former {public} policies are absent; (3) a SELECT against storage.objects
(or listing via the storage API) to confirm object visibility matches bucket
public flags; (4) a concrete anon-role upload test (curl or psql INSERT attempt
authenticated as anon) that expects a 4xx denial; and (5) a signed-URL GET test
(curl) that expects 200; for each step include the exact command to run and the
expected result/output assertion so the checks can be automated in
runbooks/tests (reference symbols: storage.buckets, storage.objects,
pg_policies, anon role, signed URL).
🪄 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: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: fc9f6d25-9286-4b5f-9117-d8df225ed12c

📥 Commits

Reviewing files that changed from the base of the PR and between a27b688 and 5b1aabb.

📒 Files selected for processing (5)
  • backend/db/security/rls_lockdown.sql
  • backend/db/security/rls_lockdown_rollback.sql
  • docs/security/realtime-jwt-bridge-design.md
  • docs/security/rls-lockdown-plan.md
  • docs/security/storage-hardening-plan.md

Comment threadbackend/db/security/rls_lockdown.sql
Comment on lines +21 to +23
```
claims: { sub: <user_id>, role: "authenticated", aud: "authenticated", exp: now+1h, iat: now }
```

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Add a language tag to the claims code fence.

This keeps markdown lint clean and improves rendering/tool support.

Suggested doc patch
-```+```json
claims: { sub: <user_id>, role: "authenticated", aud: "authenticated", exp: now+1h, iat: now }
</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **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.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)

[warning] 21-21: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🤖 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/security/realtime-jwt-bridge-design.md` around lines 21 - 23, The code
fence showing the JWT claims (claims: { sub: <user_id>, role: "authenticated",
aud: "authenticated", exp: now+1h, iat: now }) lacks a language tag; update the
fenced block to use a JSON language tag (e.g., ```json) so the claims snippet is
properly highlighted and markdown lint passes.

Source: Linters/SAST tools

Comment on lines +64 to +68
```
curl -s -o /dev/null -w "%{http_code}\n" \
"https://jxqcmjqtjlpuxfrxmrdv.supabase.co/rest/v1/users?select=id&limit=1" \
-H "apikey: <ANON_KEY>" -H "Authorization: Bearer <ANON_KEY>"
```

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Parameterize the REST verification URL and annotate the fence language.

The check currently hard-codes one project URL, which makes branch/prod verification error-prone. Also add the missing code-block language for lint/tooling consistency.

Suggested doc patch
- ```+ ```bash
curl -s -o /dev/null -w "%{http_code}\n" \
- "https://jxqcmjqtjlpuxfrxmrdv.supabase.co/rest/v1/users?select=id&limit=1" \+ "${SUPABASE_URL}/rest/v1/users?select=id&limit=1" \
-H "apikey: <ANON_KEY>" -H "Authorization: Bearer <ANON_KEY>"
```
📝 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
```
curl -s -o /dev/null -w "%{http_code}\n" \
"https://jxqcmjqtjlpuxfrxmrdv.supabase.co/rest/v1/users?select=id&limit=1" \
-H "apikey: <ANON_KEY>" -H "Authorization: Bearer <ANON_KEY>"
```
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)

[warning] 64-64: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🤖 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/security/rls-lockdown-plan.md` around lines 64 - 68, Replace the
hard-coded project URL in the curl check with a parameterized environment
variable and mark the fenced code block as bash: change the request URL string
to use "${SUPABASE_URL}/rest/v1/users?select=id&limit=1" instead of the fixed
"https://jxqcmjqtjlpuxfrxmrdv.supabase.co/..." and add the fence language
identifier "bash" to the triple-backtick that opens the snippet so the block
reads as a bash example; keep the header flags (-H "apikey: <ANON_KEY>" -H
"Authorization: Bearer <ANON_KEY>") unchanged.

Source: Linters/SAST tools

Comment on lines +23 to +24
| `issues-media-files` | **false** | new backend endpoint (multipart → service-key upload), reusing `request_limits.read_within_limit` + content-type allowlist (the #220/#229 pattern) | backend signed URL (admin view) |
| `application_resumes` | **false** | already backend (`careers.py`) | backend signed URL (admin view) |

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Address the breaking change: existing issue reports with public screenshot URLs will fail once buckets go private.

The plan correctly proposes making issues-media-files and application_resumes private, but existing rows in issue_reports.screenshot_urls (and any admin code reading them) currently store/expect public URLs. After the buckets are private, those URLs will return 400/403.

Define a migration strategy:

  • Should existing public URLs be migrated to signed URLs (backfill)?
  • Should the admin view handle both old public-URL format and new signed-URL format during a transition window?
  • Should old URLs be invalidated or mapped to a signed-URL cache?

Clarify the timeline and contract change before applying the SQL.

Also applies to: 40-41

🤖 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/security/storage-hardening-plan.md` around lines 23 - 24, The change to
make the `issues-media-files` and `application_resumes` buckets private breaks
existing values in `issue_reports.screenshot_urls` (and any admin code expecting
public URLs); implement a migration and compatibility plan: write a DB migration
script that backfills existing public URLs in `issue_reports.screenshot_urls` to
signed URLs (using the same signing logic you will deploy), and add a fallback
in the admin handlers (the code paths that read `issue_reports.screenshot_urls`
and the admin signed-URL view, plus `careers.py` for resumes) to accept both
legacy public-URL strings and new signed-URL strings during a transition window;
publish a clear timeline and contract change in the SQL deployment notes, add a
signed-URL cache or short TTL mapping if you need to avoid re-signing on every
read, and ensure tests cover backfill, fallback parsing, and expiration behavior
before flipping the bucket privacy flag.

No new storage.objects policies are needed: backend uploads/reads use `service_role` (bypasses storage RLS). `avatars` stays `public=true` so its objects remain readable without a policy.

### Backend
- New `POST /api/issue-reports/screenshot` (auth-gated via `get_session_user_id`): accepts the file, validates type+size with the shared `request_limits` helpers, uploads to `issues-media-files` with the service key (mirror `careers._upload_resume`), returns the storage path (not a public URL).

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Clarify validation logic for the new screenshot upload endpoint.

The plan references "reusing request_limits.read_within_limit + content-type allowlist (the #220/#229 pattern)" but does not detail:

  • What content types are allowed for issue-report screenshots?
  • What is the file-size limit?
  • How are validation errors returned to the frontend?
  • Is there rate-limiting on uploads per user/session?

Document these constraints explicitly so the implementation is not ambiguous.

🤖 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/security/storage-hardening-plan.md` at line 40, Add explicit validation
rules to the design for the POST /api/issue-reports/screenshot endpoint: state
that request_limits.read_within_limit will enforce a max file size of 5MB
(consistent with other UI assets) and that the content-type allowlist will
accept image/png, image/jpeg, image/webp (list these MIME types explicitly);
describe that the endpoint is auth-gated via get_session_user_id and that
validation failures return a 400 JSON response with { error:
"validation_failed", details: "<reason>" } while auth failures return 401 and
storage/upload errors return 500 with { error: "upload_failed" }; note that
uploads are stored to issues-media-files using the same service-key flow as
careers._upload_resume and that a per-user rate limit of 10 uploads per hour
should be enforced (or call out that an equivalent existing rate-limiter must be
applied), so implementers know the exact size, types, error format, storage
target, auth hook, and rate-limiting expectation.


### Backend
- New `POST /api/issue-reports/screenshot` (auth-gated via `get_session_user_id`): accepts the file, validates type+size with the shared `request_limits` helpers, uploads to `issues-media-files` with the service key (mirror `careers._upload_resume`), returns the storage path (not a public URL).
- Signed-URL helper for private buckets (admin views of screenshots/résumés): backend issues a short-TTL signed URL via the storage REST API with the service key.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Specify signed-URL TTL, refresh strategy, and error handling.

The plan mentions "short-TTL signed URL" for private-bucket reads but does not specify:

  • Exact TTL value (seconds/minutes)?
  • How the frontend admin view handles token expiry if a user is viewing a list for >TTL?
  • What happens if signed-URL generation fails (e.g., service_role key temporarily unavailable)?
  • Should signed URLs be cached/pre-fetched or generated on-demand?
  • Interaction with browser cache headers (Cache-Control) to avoid stale signed URLs.

Add concrete values and error paths before implementation.

🤖 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/security/storage-hardening-plan.md` at line 41, The Signed-URL helper
for private buckets lacks concrete TTL, refresh and error-handling requirements;
update the design to specify a short TTL (e.g., 5 minutes), a client refresh
strategy (frontend should request a new signed URL on-demand when a URL is older
than 4 minutes or when a 401/403 occurs, and proactively refresh visible items
when an admin opens the list), failure handling (backend should return a clear
5xx/4xx error with retryable flag when service_role key is unavailable; frontend
should show a retry UI and backoff retries, and fall back to a placeholder image
or “unavailable” state), caching policy (do not cache signed URLs in long-lived
storage; allow in-memory per-view caching for the TTL only and invalidate on
refresh), and browser cache headers (backend must set Cache-Control: private,
max-age=0, no-store or max-age equal to TTL if safe and include no-transform to
avoid intermediaries caching stale URLs). Implement these concrete values and
flows in the Signed-URL helper and the admin-view code paths before proceeding
to implementation.

@Jose-Gael-Cruz-Lopez

Copy link
Copy Markdown
MemberAuthor

Superseded for the docs: the three planning docs are landing via #233 (docs-only). This PR is kept as the applied-to-prod record of the RLS lockdown SQL (rls_lockdown.sql / rls_lockdown_rollback.sql) — already applied directly to production on 2026-06-13 (anon confirmed locked out). Do not merge (the SQL ran straight on prod; nothing re-runs it from the repo). Keep open as the record, or close at your discretion.

Jose-Gael-Cruz-Lopez added a commit that referenced this pull request Jun 14, 2026
…n plans (#231)
Carved from the #232 draft so the planning docs live in main (the SQL stays in
#232 as the applied-to-prod record, not merged). The RLS lockdown is already
applied to production (anon locked out, confirmed). #230 references the
realtime-jwt-bridge design doc landed here.
Jose-Gael-Cruz-Lopez added a commit that referenced this pull request Jun 17, 2026
…us (#231)
Resolve the APPLIED-vs-draft contradiction: Phase 1 (application_resumes →
private) was genuinely applied to prod on 2026-06-15 (commit d14b14f, MCP,
verified public=false, with the CDN-cache caveat learned during verification),
so it is now framed as an after-the-fact RECORD across the SQL file and plan
doc. Phase 2 (issues-media-files) stays clearly draft/not-applied. Removed the
stale top-line "Nothing applied to prod" / "apply now" wording that conflicted
with the recorded Phase 1.
Also:
- Wrap Phase 1 UPDATE in BEGIN/COMMIT for copy-paste safety + Phase 2 parity.
- Phase 2b drops use DROP POLICY IF EXISTS (name drift won't fail the txn);
rollback CREATEs guarded with DROP IF EXISTS to stay idempotent.
- Document the cross-PR ordering constraint: #238 supersedes the old copy of
storage-hardening-plan.md in #232, so #238 must merge AFTER #232 (or #232
must drop the file) to avoid a silent revert.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- rls_lockdown.sql: make ALTER DEFAULT PRIVILEGES REVOKE robust across
table-creating roles (per-role scoping) with a documented discovery query
and FOR ROLE placeholders so a future migration run as another role can't
silently re-grant anon. Mirror the per-role GRANT in the rollback.
- rls-lockdown-plan.md: mark the "anon has zero table DML" query as a
recurring BLOCKING check to wire into CI/cron; clarify verification step 3
so a permission-denied error OR an empty 200 [] both count as PASS (RLS vs
REVOKE), not a false failure.
- realtime-jwt-bridge-design.md: add a presence/typing channel section
(presence:${roomId} in Social.tsx) noting it is NOT broken by the anon DML
REVOKE and that its authorization must be re-verified under setAuth(jwt)/
private channels; promote the asymmetric-vs-HS256 signing-key-type check to
a BLOCKING precondition (wrong key type = silent 401 on setAuth).
- storage-hardening-plan.md: add SUPERSEDED-by-#238 banner (and cross-link in
the lockdown plan) since #238 rewrites the storage plan and corrects two
factual errors in this copy.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Jose-Gael-Cruz-Lopez added a commit that referenced this pull request Jun 17, 2026
…231)
Reconcile the inconsistent résumé/object count (table said 13, prose/SQL
said 12) to a consistent ~12-13 with a snapshot caveat (observed 2026-06-15,
re-verify at apply), since prod cannot be queried here.
Correct the cross-PR ordering notes: the file is add/add between #232 and
#238, so the second merge raises an explicit CONFLICT (add/add) — not a
"silent overwrite/revert". Resolve in #238's favor; recommendation (merge
#232 first or drop the file from #232) unchanged.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Jose-Gael-Cruz-Lopez added a commit that referenced this pull request Jun 24, 2026
…ount
Three correctness fixes to the plan doc (still DO NOT MERGE/APPLY):
- Major: Phase 2a (route issue-report screenshots through the backend) has
already landed on main (feedback.py POST /api/issue-reports/screenshot,
ReportIssueFlow.tsx off supabase.storage, test_issue_screenshot_auth.py).
Update the status block and Phase 2 section so the only remaining storage
step is Phase 2b (flip issues-media-files private).
- Minor: the doc already exists on main (it is a modification, not an add),
so there is no add/add conflict with #232. Replace the cross-PR ordering
section with an accurate note.
- Minor: drop the loose/stale ~12-13 object count; the bucket holds résumé
PII regardless of count, so no number is pinned.
@Jose-Gael-Cruz-Lopez

Copy link
Copy Markdown
MemberAuthor

Re-verified #231 storage against prod (read-only) on 2026-07-01: the storage portion of this draft is largely done already — application_resumes is private, screenshots upload via the auth-gated POST /api/issue-reports/screenshot (service role, feedback.py), and the anon "Allow uploads" policy is scoped to issues-media-files. The only residual (making issues-media-files private + dropping the 2 anon storage.objects policies) is now a focused migration in #304 (0029_storage_lockdown_231.sql).

Suggest this draft's storage scope be closed as superseded by #304; its realtime-RLS content (a separate concern) is untouched and can proceed on its own.

@Jose-Gael-Cruz-Lopez

Copy link
Copy Markdown
MemberAuthor

Closing without merge — this draft served its purpose and everything in it is either live or superseded:

One follow-up remains: backend/db/security/rls_lockdown.sql + rls_lockdown_rollback.sql exist only on this branch and are the sole in-repo copy of DDL that is live on prod. The branch security/231-rls-lockdown-drafts is intentionally not deleted until that SQL is extracted into a numbered migration (the 0029 precedent). The realtime JWT-bridge design (option a) also remains unbuilt — design doc is on main at docs/security/realtime-jwt-bridge-design.md.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@Jose-Gael-Cruz-Lopez
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

DRAFT (#231): RLS lockdown SQL + storage & realtime(a) remediation plans — do not merge/apply - #232

Closed
Jose-Gael-Cruz-Lopez wants to merge 2 commits into
mainfrom
security/231-rls-lockdown-drafts
Closed

DRAFT (#231): RLS lockdown SQL + storage & realtime(a) remediation plans — do not merge/apply#232
Jose-Gael-Cruz-Lopez wants to merge 2 commits into
mainfrom
security/231-rls-lockdown-drafts

Conversation

@Jose-Gael-Cruz-Lopez

@Jose-Gael-Cruz-LopezJose-Gael-Cruz-Lopez commented Jun 13, 2026

Copy link
Copy Markdown
Member

DRAFT for review — do NOT merge or apply. Remediation artifacts for #231 (public anon key has full DML on 38/40 RLS-disabled tables → potential full-DB read/write via PostgREST, bypassing the backend). No code or prod changes here.

URGENT — RLS lockdown (one-time DDL, not a #197 migration)

  • backend/db/security/rls_lockdown.sql: ENABLE ROW LEVEL SECURITY on the 38 tables that lack it + REVOKE all anon DML across the public schema, plus ALTER DEFAULT PRIVILEGES so future tables don't silently re-grant anon.
  • Verified safe for the backend:service_role has rolbypassrls=true (live-checked), so every backend query is unaffected; only anon/authenticated (bypass=false) are constrained.
  • rls_lockdown_rollback.sql: emergency revert (restores the insecure state).
  • docs/security/rls-lockdown-plan.md: apply/verify/rollback checklist (incl. the anon-blocked REST check and the service_role-bypass proof) + Supabase-branching note (branching was unreachable via MCP).
  • Accepted breakage: anon realtime on room_messages pauses until option (a) lands; chat still loads/refreshes via the service-role REST path (fix(social): re-fetch realtime messages via decrypting REST + drop dead reactions sub (#124, #231) #230).

Storage hardening plan

docs/security/storage-hardening-plan.md: issues-media-files + application_resumesprivate + signed URLs + backend-side uploads; avatars stays public-read with the blanket public INSERT revoked; drop the global storage.objects public-INSERT policy. (Live finding: chat-images/cosmetic-assets buckets don't exist — dead code.)

Realtime option (a) design

docs/security/realtime-jwt-bridge-design.md: mint a Supabase JWT at login + realtime.setAuth, membership-scoped RLS SELECT policy on room_messages; postgres_changes + RLS (minimal) vs private channels. JWT refresh flagged (30d session vs ~1h JWT). Builds after the lockdown.

How to use

Review the SQL, then you apply rls_lockdown.sql (branch-first if available). I did not apply anything. Storage SQL + realtime are plans to turn into their own PRs once you approve the approach.

Summary by CodeRabbit

  • Documentation

    • Added security planning guides documenting database access control implementation, real-time authentication architecture, and storage hardening strategies with verification procedures.
  • Chores

    • Added database security configuration scripts and corresponding rollback procedures to support infrastructure security improvements.

…drafts (#231)
DRAFTS ONLY — do not apply; for review. Addresses the #231 finding that the
public anon key has full DML on 38/40 RLS-disabled public tables (potential
full-DB read/write via PostgREST, bypassing the backend).
- backend/db/security/rls_lockdown.sql — enable RLS on the 38 tables + revoke
anon DML across the public schema (+ default privileges). Verified safe:
service_role has rolbypassrls=true, so the backend is unaffected.
- backend/db/security/rls_lockdown_rollback.sql — emergency revert (restores
the insecure state).
- docs/security/rls-lockdown-plan.md — apply/verify/rollback checklist; branch
note (branching unreachable via MCP).
- docs/security/storage-hardening-plan.md — issues-media-files + application_
resumes -> private + signed URLs + backend uploads; avatars public-read,
revoke blanket public INSERT; kill the global storage public-INSERT policy.
- docs/security/realtime-jwt-bridge-design.md — option (a): mint Supabase JWT
at login + setAuth, room_messages membership RLS policy; flags JWT refresh
(30d session vs ~1h JWT). Lands after the lockdown to restore realtime.
No code/prod changes in this PR.
@coderabbitai

coderabbitaiBot commented Jun 13, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@Jose-Gael-Cruz-Lopez, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 58 minutes and 13 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 0ed9b2de-7de8-4558-8028-28bd9d97574d

📥 Commits

Reviewing files that changed from the base of the PR and between 5b1aabb and 07fca30.

📒 Files selected for processing (5)
  • backend/db/security/rls_lockdown.sql
  • backend/db/security/rls_lockdown_rollback.sql
  • docs/security/realtime-jwt-bridge-design.md
  • docs/security/rls-lockdown-plan.md
  • docs/security/storage-hardening-plan.md
📝 Walkthrough

Walkthrough

This PR establishes a project-wide security hardening initiative by introducing RLS lockdown DDL scripts, verification procedures, and design plans for restoring Realtime functionality and hardening storage access—all without requiring backend code changes since the service role bypasses RLS.

Changes

Security Hardening Initiative

Layer / File(s)Summary
RLS Lockdown DDL Apply and Rollback Scripts
backend/db/security/rls_lockdown.sql, backend/db/security/rls_lockdown_rollback.sql
Transactional SQL scripts enable RLS on 38 public tables and revoke all anon DML permissions (SELECT/INSERT/UPDATE/DELETE) plus future default privileges; rollback re-grants anon access and disables RLS while preserving achievement table state.
RLS Lockdown Verification and Deployment Plan
docs/security/rls-lockdown-plan.md
Documents lockdown scope, backend safety via service_role bypass, expected anon realtime breakage on room_messages, test-first and prod deployment procedures, pre-apply snapshots, post-apply verification checklist (RLS status, anon grants, REST auth, backend pytest, realtime behavior), rollback steps, and follow-up notes on authenticated grants and storage hardening.
Realtime JWT Bridge Design for RLS-Protected Room Messages
docs/security/realtime-jwt-bridge-design.md
Proposes backend JWT-minting endpoint with SUPABASE_JWT_SECRET, membership-scoped SELECT policy on room_messages using auth.uid matched to room_members, frontend token fetch post-login with realtime.setAuth, JWT refresh loop with reconnect/sleep handling and REST fallback, plus optional room_reactions follow-up.
Storage Bucket Hardening Plan
docs/security/storage-hardening-plan.md
Draft plan to restrict public-read buckets (issues-media-files, application_resumes) via SQL, drop public INSERT policies, route writes through backend service_role, provide signed URLs for private reads, leave avatars public, with corresponding backend upload/signing endpoints and frontend wiring plus verification checklist.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related issues

  • #231: The RLS lockdown DDL scripts and realtime-JWT bridge design address the same tables, policies, and realtime-auth gap (room_messages/room_reactions membership RLS and JWT-based Realtime authentication) described in that issue.

Poem

A rabbit stands guard at the gate today,
Locking down rows in the SQL way—
Anon must ask now, through tokens bright,
While service_role dances through the night. 🐰🔐

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check nameStatusExplanation
Title check✅ PassedThe title accurately references the RLS lockdown (#231) and lists the main remediation artifacts (SQL, storage & realtime plans), marking it as DRAFT — clearly indicating this is a review-stage document.
Description check✅ PassedThe description covers objectives (RLS lockdown, storage hardening, realtime design), key files/changes, verification notes, and usage instructions, but deviates from the template structure with narrative format instead of structured sections.
Docstring Coverage✅ PassedNo functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check✅ PassedCheck skipped because no linked issues were found for this pull request.
Out of Scope Changes check✅ PassedCheck skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch security/231-rls-lockdown-drafts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@cloudflare-workers-and-pages

cloudflare-workers-and-pagesBot commented Jun 13, 2026

Copy link
Copy Markdown

Deploying with Cloudflare Workers Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

StatusNameLatest CommitPreview URLUpdated (UTC)
✅ Deployment successful!
View logs
frontend07fca30Commit Preview URL

Branch Preview URL
Jun 17 2026, 01:24 AM

@coderabbitaicoderabbitaiBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🧹 Nitpick comments (1)
docs/security/storage-hardening-plan.md (1)

48-50: ⚡ Quick win

Provide concrete SQL queries for the verification checklist.

The verification steps are described narratively (e.g., "storage.buckets: issues-media-files and application_resumes show public=false") but would be more reliable if accompanied by concrete SQL queries that can be automated in tests or runbooks. For example:

  • Query to check storage.buckets.public flag
  • Query to list storage.objects policies and confirm the two public ones are gone
  • Query to test anon-role upload attempt (expect denial)

Add explicit SQL commands so verifiers can run them post-apply.

🤖 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/security/storage-hardening-plan.md` around lines 48 - 50, Add explicit,
runnable SQL and verification commands to the checklist: (1) a SELECT against
storage.buckets filtering name IN
('issues-media-files','application_resumes','avatars') returning the public
column and asserting issues-media-files and application_resumes have public =
false while avatars = true; (2) a SELECT against the PostgreSQL policy metadata
for the storage schema (pg_policies or the pg_catalog view) filtering on
table_name = 'objects' (and/or table_name = 'buckets' if used) to confirm the
two former {public} policies are absent; (3) a SELECT against storage.objects
(or listing via the storage API) to confirm object visibility matches bucket
public flags; (4) a concrete anon-role upload test (curl or psql INSERT attempt
authenticated as anon) that expects a 4xx denial; and (5) a signed-URL GET test
(curl) that expects 200; for each step include the exact command to run and the
expected result/output assertion so the checks can be automated in
runbooks/tests (reference symbols: storage.buckets, storage.objects,
pg_policies, anon role, signed URL).
🤖 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 `@backend/db/security/rls_lockdown.sql`:
- Around line 79-83: Update the ALTER DEFAULT PRIVILEGES statements to target
the table-creator role(s) explicitly: in backend/db/security/rls_lockdown.sql
(lines 79-83) change the generic "ALTER DEFAULT PRIVILEGES IN SCHEMA public
REVOKE ... FROM anon" to "ALTER DEFAULT PRIVILEGES FOR ROLE <creator_role> IN
SCHEMA public REVOKE SELECT, INSERT, UPDATE, DELETE ON TABLES FROM anon;" for
each relevant creator role, and in backend/db/security/rls_lockdown_rollback.sql
(lines 19-21) mirror that change so the rollback uses "ALTER DEFAULT PRIVILEGES
FOR ROLE <creator_role> IN SCHEMA public GRANT SELECT, INSERT, UPDATE, DELETE ON
TABLES TO anon;" (replace <creator_role> with the actual DB role(s) that create
tables); after applying, verify pg_default_acl contains no default table
privileges granting anon in the public schema.
In `@docs/security/realtime-jwt-bridge-design.md`:
- Around line 21-23: The code fence showing the JWT claims (claims: { sub:
<user_id>, role: "authenticated", aud: "authenticated", exp: now+1h, iat: now })
lacks a language tag; update the fenced block to use a JSON language tag (e.g.,
```json) so the claims snippet is properly highlighted and markdown lint passes.
In `@docs/security/rls-lockdown-plan.md`:
- Around line 64-68: Replace the hard-coded project URL in the curl check with a
parameterized environment variable and mark the fenced code block as bash:
change the request URL string to use
"${SUPABASE_URL}/rest/v1/users?select=id&limit=1" instead of the fixed
"https://jxqcmjqtjlpuxfrxmrdv.supabase.co/..." and add the fence language
identifier "bash" to the triple-backtick that opens the snippet so the block
reads as a bash example; keep the header flags (-H "apikey: <ANON_KEY>" -H
"Authorization: Bearer <ANON_KEY>") unchanged.
In `@docs/security/storage-hardening-plan.md`:
- Line 40: Add explicit validation rules to the design for the POST
/api/issue-reports/screenshot endpoint: state that
request_limits.read_within_limit will enforce a max file size of 5MB (consistent
with other UI assets) and that the content-type allowlist will accept image/png,
image/jpeg, image/webp (list these MIME types explicitly); describe that the
endpoint is auth-gated via get_session_user_id and that validation failures
return a 400 JSON response with { error: "validation_failed", details:
"<reason>" } while auth failures return 401 and storage/upload errors return 500
with { error: "upload_failed" }; note that uploads are stored to
issues-media-files using the same service-key flow as careers._upload_resume and
that a per-user rate limit of 10 uploads per hour should be enforced (or call
out that an equivalent existing rate-limiter must be applied), so implementers
know the exact size, types, error format, storage target, auth hook, and
rate-limiting expectation.
- Around line 23-24: The change to make the `issues-media-files` and
`application_resumes` buckets private breaks existing values in
`issue_reports.screenshot_urls` (and any admin code expecting public URLs);
implement a migration and compatibility plan: write a DB migration script that
backfills existing public URLs in `issue_reports.screenshot_urls` to signed URLs
(using the same signing logic you will deploy), and add a fallback in the admin
handlers (the code paths that read `issue_reports.screenshot_urls` and the admin
signed-URL view, plus `careers.py` for resumes) to accept both legacy public-URL
strings and new signed-URL strings during a transition window; publish a clear
timeline and contract change in the SQL deployment notes, add a signed-URL cache
or short TTL mapping if you need to avoid re-signing on every read, and ensure
tests cover backfill, fallback parsing, and expiration behavior before flipping
the bucket privacy flag.
- Line 41: The Signed-URL helper for private buckets lacks concrete TTL, refresh
and error-handling requirements; update the design to specify a short TTL (e.g.,
5 minutes), a client refresh strategy (frontend should request a new signed URL
on-demand when a URL is older than 4 minutes or when a 401/403 occurs, and
proactively refresh visible items when an admin opens the list), failure
handling (backend should return a clear 5xx/4xx error with retryable flag when
service_role key is unavailable; frontend should show a retry UI and backoff
retries, and fall back to a placeholder image or “unavailable” state), caching
policy (do not cache signed URLs in long-lived storage; allow in-memory per-view
caching for the TTL only and invalidate on refresh), and browser cache headers
(backend must set Cache-Control: private, max-age=0, no-store or max-age equal
to TTL if safe and include no-transform to avoid intermediaries caching stale
URLs). Implement these concrete values and flows in the Signed-URL helper and
the admin-view code paths before proceeding to implementation.
---
Nitpick comments:
In `@docs/security/storage-hardening-plan.md`:
- Around line 48-50: Add explicit, runnable SQL and verification commands to the
checklist: (1) a SELECT against storage.buckets filtering name IN
('issues-media-files','application_resumes','avatars') returning the public
column and asserting issues-media-files and application_resumes have public =
false while avatars = true; (2) a SELECT against the PostgreSQL policy metadata
for the storage schema (pg_policies or the pg_catalog view) filtering on
table_name = 'objects' (and/or table_name = 'buckets' if used) to confirm the
two former {public} policies are absent; (3) a SELECT against storage.objects
(or listing via the storage API) to confirm object visibility matches bucket
public flags; (4) a concrete anon-role upload test (curl or psql INSERT attempt
authenticated as anon) that expects a 4xx denial; and (5) a signed-URL GET test
(curl) that expects 200; for each step include the exact command to run and the
expected result/output assertion so the checks can be automated in
runbooks/tests (reference symbols: storage.buckets, storage.objects,
pg_policies, anon role, signed URL).
🪄 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: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: fc9f6d25-9286-4b5f-9117-d8df225ed12c

📥 Commits

Reviewing files that changed from the base of the PR and between a27b688 and 5b1aabb.

📒 Files selected for processing (5)
  • backend/db/security/rls_lockdown.sql
  • backend/db/security/rls_lockdown_rollback.sql
  • docs/security/realtime-jwt-bridge-design.md
  • docs/security/rls-lockdown-plan.md
  • docs/security/storage-hardening-plan.md

Comment threadbackend/db/security/rls_lockdown.sql
Comment on lines +21 to +23
```
claims: { sub: <user_id>, role: "authenticated", aud: "authenticated", exp: now+1h, iat: now }
```

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Add a language tag to the claims code fence.

This keeps markdown lint clean and improves rendering/tool support.

Suggested doc patch
-```+```json
claims: { sub: <user_id>, role: "authenticated", aud: "authenticated", exp: now+1h, iat: now }
</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **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.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)

[warning] 21-21: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🤖 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/security/realtime-jwt-bridge-design.md` around lines 21 - 23, The code
fence showing the JWT claims (claims: { sub: <user_id>, role: "authenticated",
aud: "authenticated", exp: now+1h, iat: now }) lacks a language tag; update the
fenced block to use a JSON language tag (e.g., ```json) so the claims snippet is
properly highlighted and markdown lint passes.

Source: Linters/SAST tools

Comment on lines +64 to +68
```
curl -s -o /dev/null -w "%{http_code}\n" \
"https://jxqcmjqtjlpuxfrxmrdv.supabase.co/rest/v1/users?select=id&limit=1" \
-H "apikey: <ANON_KEY>" -H "Authorization: Bearer <ANON_KEY>"
```

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Parameterize the REST verification URL and annotate the fence language.

The check currently hard-codes one project URL, which makes branch/prod verification error-prone. Also add the missing code-block language for lint/tooling consistency.

Suggested doc patch
- ```+ ```bash
curl -s -o /dev/null -w "%{http_code}\n" \
- "https://jxqcmjqtjlpuxfrxmrdv.supabase.co/rest/v1/users?select=id&limit=1" \+ "${SUPABASE_URL}/rest/v1/users?select=id&limit=1" \
-H "apikey: <ANON_KEY>" -H "Authorization: Bearer <ANON_KEY>"
```
📝 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
```
curl -s -o /dev/null -w "%{http_code}\n" \
"https://jxqcmjqtjlpuxfrxmrdv.supabase.co/rest/v1/users?select=id&limit=1" \
-H "apikey: <ANON_KEY>" -H "Authorization: Bearer <ANON_KEY>"
```
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)

[warning] 64-64: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🤖 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/security/rls-lockdown-plan.md` around lines 64 - 68, Replace the
hard-coded project URL in the curl check with a parameterized environment
variable and mark the fenced code block as bash: change the request URL string
to use "${SUPABASE_URL}/rest/v1/users?select=id&limit=1" instead of the fixed
"https://jxqcmjqtjlpuxfrxmrdv.supabase.co/..." and add the fence language
identifier "bash" to the triple-backtick that opens the snippet so the block
reads as a bash example; keep the header flags (-H "apikey: <ANON_KEY>" -H
"Authorization: Bearer <ANON_KEY>") unchanged.

Source: Linters/SAST tools

Comment on lines +23 to +24
| `issues-media-files` | **false** | new backend endpoint (multipart → service-key upload), reusing `request_limits.read_within_limit` + content-type allowlist (the #220/#229 pattern) | backend signed URL (admin view) |
| `application_resumes` | **false** | already backend (`careers.py`) | backend signed URL (admin view) |

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Address the breaking change: existing issue reports with public screenshot URLs will fail once buckets go private.

The plan correctly proposes making issues-media-files and application_resumes private, but existing rows in issue_reports.screenshot_urls (and any admin code reading them) currently store/expect public URLs. After the buckets are private, those URLs will return 400/403.

Define a migration strategy:

  • Should existing public URLs be migrated to signed URLs (backfill)?
  • Should the admin view handle both old public-URL format and new signed-URL format during a transition window?
  • Should old URLs be invalidated or mapped to a signed-URL cache?

Clarify the timeline and contract change before applying the SQL.

Also applies to: 40-41

🤖 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/security/storage-hardening-plan.md` around lines 23 - 24, The change to
make the `issues-media-files` and `application_resumes` buckets private breaks
existing values in `issue_reports.screenshot_urls` (and any admin code expecting
public URLs); implement a migration and compatibility plan: write a DB migration
script that backfills existing public URLs in `issue_reports.screenshot_urls` to
signed URLs (using the same signing logic you will deploy), and add a fallback
in the admin handlers (the code paths that read `issue_reports.screenshot_urls`
and the admin signed-URL view, plus `careers.py` for resumes) to accept both
legacy public-URL strings and new signed-URL strings during a transition window;
publish a clear timeline and contract change in the SQL deployment notes, add a
signed-URL cache or short TTL mapping if you need to avoid re-signing on every
read, and ensure tests cover backfill, fallback parsing, and expiration behavior
before flipping the bucket privacy flag.

No new storage.objects policies are needed: backend uploads/reads use `service_role` (bypasses storage RLS). `avatars` stays `public=true` so its objects remain readable without a policy.

### Backend
- New `POST /api/issue-reports/screenshot` (auth-gated via `get_session_user_id`): accepts the file, validates type+size with the shared `request_limits` helpers, uploads to `issues-media-files` with the service key (mirror `careers._upload_resume`), returns the storage path (not a public URL).

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Clarify validation logic for the new screenshot upload endpoint.

The plan references "reusing request_limits.read_within_limit + content-type allowlist (the #220/#229 pattern)" but does not detail:

  • What content types are allowed for issue-report screenshots?
  • What is the file-size limit?
  • How are validation errors returned to the frontend?
  • Is there rate-limiting on uploads per user/session?

Document these constraints explicitly so the implementation is not ambiguous.

🤖 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/security/storage-hardening-plan.md` at line 40, Add explicit validation
rules to the design for the POST /api/issue-reports/screenshot endpoint: state
that request_limits.read_within_limit will enforce a max file size of 5MB
(consistent with other UI assets) and that the content-type allowlist will
accept image/png, image/jpeg, image/webp (list these MIME types explicitly);
describe that the endpoint is auth-gated via get_session_user_id and that
validation failures return a 400 JSON response with { error:
"validation_failed", details: "<reason>" } while auth failures return 401 and
storage/upload errors return 500 with { error: "upload_failed" }; note that
uploads are stored to issues-media-files using the same service-key flow as
careers._upload_resume and that a per-user rate limit of 10 uploads per hour
should be enforced (or call out that an equivalent existing rate-limiter must be
applied), so implementers know the exact size, types, error format, storage
target, auth hook, and rate-limiting expectation.


### Backend
- New `POST /api/issue-reports/screenshot` (auth-gated via `get_session_user_id`): accepts the file, validates type+size with the shared `request_limits` helpers, uploads to `issues-media-files` with the service key (mirror `careers._upload_resume`), returns the storage path (not a public URL).
- Signed-URL helper for private buckets (admin views of screenshots/résumés): backend issues a short-TTL signed URL via the storage REST API with the service key.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Specify signed-URL TTL, refresh strategy, and error handling.

The plan mentions "short-TTL signed URL" for private-bucket reads but does not specify:

  • Exact TTL value (seconds/minutes)?
  • How the frontend admin view handles token expiry if a user is viewing a list for >TTL?
  • What happens if signed-URL generation fails (e.g., service_role key temporarily unavailable)?
  • Should signed URLs be cached/pre-fetched or generated on-demand?
  • Interaction with browser cache headers (Cache-Control) to avoid stale signed URLs.

Add concrete values and error paths before implementation.

🤖 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/security/storage-hardening-plan.md` at line 41, The Signed-URL helper
for private buckets lacks concrete TTL, refresh and error-handling requirements;
update the design to specify a short TTL (e.g., 5 minutes), a client refresh
strategy (frontend should request a new signed URL on-demand when a URL is older
than 4 minutes or when a 401/403 occurs, and proactively refresh visible items
when an admin opens the list), failure handling (backend should return a clear
5xx/4xx error with retryable flag when service_role key is unavailable; frontend
should show a retry UI and backoff retries, and fall back to a placeholder image
or “unavailable” state), caching policy (do not cache signed URLs in long-lived
storage; allow in-memory per-view caching for the TTL only and invalidate on
refresh), and browser cache headers (backend must set Cache-Control: private,
max-age=0, no-store or max-age equal to TTL if safe and include no-transform to
avoid intermediaries caching stale URLs). Implement these concrete values and
flows in the Signed-URL helper and the admin-view code paths before proceeding
to implementation.

@Jose-Gael-Cruz-Lopez

Copy link
Copy Markdown
MemberAuthor

Superseded for the docs: the three planning docs are landing via #233 (docs-only). This PR is kept as the applied-to-prod record of the RLS lockdown SQL (rls_lockdown.sql / rls_lockdown_rollback.sql) — already applied directly to production on 2026-06-13 (anon confirmed locked out). Do not merge (the SQL ran straight on prod; nothing re-runs it from the repo). Keep open as the record, or close at your discretion.

Jose-Gael-Cruz-Lopez added a commit that referenced this pull request Jun 14, 2026
…n plans (#231)
Carved from the #232 draft so the planning docs live in main (the SQL stays in
#232 as the applied-to-prod record, not merged). The RLS lockdown is already
applied to production (anon locked out, confirmed). #230 references the
realtime-jwt-bridge design doc landed here.
Jose-Gael-Cruz-Lopez added a commit that referenced this pull request Jun 17, 2026
…us (#231)
Resolve the APPLIED-vs-draft contradiction: Phase 1 (application_resumes →
private) was genuinely applied to prod on 2026-06-15 (commit d14b14f, MCP,
verified public=false, with the CDN-cache caveat learned during verification),
so it is now framed as an after-the-fact RECORD across the SQL file and plan
doc. Phase 2 (issues-media-files) stays clearly draft/not-applied. Removed the
stale top-line "Nothing applied to prod" / "apply now" wording that conflicted
with the recorded Phase 1.
Also:
- Wrap Phase 1 UPDATE in BEGIN/COMMIT for copy-paste safety + Phase 2 parity.
- Phase 2b drops use DROP POLICY IF EXISTS (name drift won't fail the txn);
rollback CREATEs guarded with DROP IF EXISTS to stay idempotent.
- Document the cross-PR ordering constraint: #238 supersedes the old copy of
storage-hardening-plan.md in #232, so #238 must merge AFTER #232 (or #232
must drop the file) to avoid a silent revert.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- rls_lockdown.sql: make ALTER DEFAULT PRIVILEGES REVOKE robust across
table-creating roles (per-role scoping) with a documented discovery query
and FOR ROLE placeholders so a future migration run as another role can't
silently re-grant anon. Mirror the per-role GRANT in the rollback.
- rls-lockdown-plan.md: mark the "anon has zero table DML" query as a
recurring BLOCKING check to wire into CI/cron; clarify verification step 3
so a permission-denied error OR an empty 200 [] both count as PASS (RLS vs
REVOKE), not a false failure.
- realtime-jwt-bridge-design.md: add a presence/typing channel section
(presence:${roomId} in Social.tsx) noting it is NOT broken by the anon DML
REVOKE and that its authorization must be re-verified under setAuth(jwt)/
private channels; promote the asymmetric-vs-HS256 signing-key-type check to
a BLOCKING precondition (wrong key type = silent 401 on setAuth).
- storage-hardening-plan.md: add SUPERSEDED-by-#238 banner (and cross-link in
the lockdown plan) since #238 rewrites the storage plan and corrects two
factual errors in this copy.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Jose-Gael-Cruz-Lopez added a commit that referenced this pull request Jun 17, 2026
…231)
Reconcile the inconsistent résumé/object count (table said 13, prose/SQL
said 12) to a consistent ~12-13 with a snapshot caveat (observed 2026-06-15,
re-verify at apply), since prod cannot be queried here.
Correct the cross-PR ordering notes: the file is add/add between #232 and
#238, so the second merge raises an explicit CONFLICT (add/add) — not a
"silent overwrite/revert". Resolve in #238's favor; recommendation (merge
#232 first or drop the file from #232) unchanged.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Jose-Gael-Cruz-Lopez added a commit that referenced this pull request Jun 24, 2026
…ount
Three correctness fixes to the plan doc (still DO NOT MERGE/APPLY):
- Major: Phase 2a (route issue-report screenshots through the backend) has
already landed on main (feedback.py POST /api/issue-reports/screenshot,
ReportIssueFlow.tsx off supabase.storage, test_issue_screenshot_auth.py).
Update the status block and Phase 2 section so the only remaining storage
step is Phase 2b (flip issues-media-files private).
- Minor: the doc already exists on main (it is a modification, not an add),
so there is no add/add conflict with #232. Replace the cross-PR ordering
section with an accurate note.
- Minor: drop the loose/stale ~12-13 object count; the bucket holds résumé
PII regardless of count, so no number is pinned.
@Jose-Gael-Cruz-Lopez

Copy link
Copy Markdown
MemberAuthor

Re-verified #231 storage against prod (read-only) on 2026-07-01: the storage portion of this draft is largely done already — application_resumes is private, screenshots upload via the auth-gated POST /api/issue-reports/screenshot (service role, feedback.py), and the anon "Allow uploads" policy is scoped to issues-media-files. The only residual (making issues-media-files private + dropping the 2 anon storage.objects policies) is now a focused migration in #304 (0029_storage_lockdown_231.sql).

Suggest this draft's storage scope be closed as superseded by #304; its realtime-RLS content (a separate concern) is untouched and can proceed on its own.

@Jose-Gael-Cruz-Lopez

Copy link
Copy Markdown
MemberAuthor

Closing without merge — this draft served its purpose and everything in it is either live or superseded:

One follow-up remains: backend/db/security/rls_lockdown.sql + rls_lockdown_rollback.sql exist only on this branch and are the sole in-repo copy of DDL that is live on prod. The branch security/231-rls-lockdown-drafts is intentionally not deleted until that SQL is extracted into a numbered migration (the 0029 precedent). The realtime JWT-bridge design (option a) also remains unbuilt — design doc is on main at docs/security/realtime-jwt-bridge-design.md.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@Jose-Gael-Cruz-Lopez