fix: restrict cleanup_expired_demo_apps execution#1807
Conversation
|
Tracking advisory GHSA-48wc-p6jm-q5j2. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThis PR adds a SQL migration to revoke RPC authorization from anonymous and authenticated roles on the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 SQLFluff (4.0.4)supabase/migrations/20260317020500_revoke_cleanup_expired_demo_apps_public_exec.sqlUser Error: No dialect was specified. You must configure a dialect or specify one on the command line using --dialect after the command. Available dialects: Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/cleanup-expired-demo-apps-rpc.test.ts`:
- Line 12: Change the test declaration for "keeps execute privilege only for
service-role callers" from it(...) to it.concurrent(...) so the test runs in
parallel; locate the test function with that exact description in the file and
replace the it call with it.concurrent to comply with the repository-wide
parallel test contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d1252e08-5e03-4bbd-b07b-4c1e6562dc9e
📒 Files selected for processing (2)
supabase/migrations/20260317011315_revoke_cleanup_expired_demo_apps_public_exec.sqltests/cleanup-expired-demo-apps-rpc.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/cleanup-expired-demo-apps-rpc.test.ts (1)
26-29: Consider asserting row presence before privilege assertions.This makes failures more explicit if the query shape changes unexpectedly.
Proposed tweak
- expect(rows[0]?.service_role_can_execute).toBe(true) - expect(rows[0]?.anon_can_execute).toBe(false) - expect(rows[0]?.authenticated_can_execute).toBe(false) - expect(rows[0]?.public_can_execute).toBe(false) + expect(rows).toHaveLength(1) + const row = rows[0] + if (!row) + throw new Error('Expected one privilege row') + + expect(row.service_role_can_execute).toBe(true) + expect(row.anon_can_execute).toBe(false) + expect(row.authenticated_can_execute).toBe(false) + expect(row.public_can_execute).toBe(false)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cleanup-expired-demo-apps-rpc.test.ts` around lines 26 - 29, The test currently asserts privilege flags on rows[0] without first ensuring a row exists; add an explicit presence assertion (e.g., expect(rows.length).toBeGreaterThan(0) or expect(rows[0]).toBeDefined()) before the checks of rows[0]?.service_role_can_execute, rows[0]?.anon_can_execute, rows[0]?.authenticated_can_execute, and rows[0]?.public_can_execute so failures show a clear missing-row error rather than silent undefined flag assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/cleanup-expired-demo-apps-rpc.test.ts`:
- Around line 26-29: The test currently asserts privilege flags on rows[0]
without first ensuring a row exists; add an explicit presence assertion (e.g.,
expect(rows.length).toBeGreaterThan(0) or expect(rows[0]).toBeDefined()) before
the checks of rows[0]?.service_role_can_execute, rows[0]?.anon_can_execute,
rows[0]?.authenticated_can_execute, and rows[0]?.public_can_execute so failures
show a clear missing-row error rather than silent undefined flag assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4df93453-a8b0-4434-8a37-31bae41b288f
📒 Files selected for processing (1)
tests/cleanup-expired-demo-apps-rpc.test.ts
|
CI is blocked by a repo-wide backend test failure, not by this PR. The failing \ job hits a \ error, with related \ constraint failures while exercising version metadata writes. I did not patch this branch because the regression appears outside this PR; once the shared backend issue is fixed, CI should be rerun. |
|
CI is blocked by a repo-wide backend test failure, not by this PR. The failing |
|
CI still fails on the shared |
|
Run tests fails in |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/cleanup-expired-demo-apps-rpc.test.ts`:
- Line 28: Replace the single-line if statement "if (!row) throw new
Error('Expected exactly one privilege row')" with a multi-line block to satisfy
the antfu/if-newline rule: change the condition that checks the variable row
into a normal if block with braces and put the throw on the next line (e.g., if
(!row) { throw new Error('Expected exactly one privilege row') }), ensuring the
newline after the if condition and correct indentation; update the test in
cleanup-expired-demo-apps-rpc.test.ts where the variable row is asserted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a704e52a-2893-4ad2-b396-8f288907f6f8
📒 Files selected for processing (1)
tests/cleanup-expired-demo-apps-rpc.test.ts
|
PR #1807: the failure is the same |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c537cb313a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -0,0 +1,9 @@ | |||
| REVOKE ALL ON FUNCTION public.cleanup_expired_demo_apps() FROM PUBLIC; | |||
There was a problem hiding this comment.
This file is backfilled as 20260317011315, but the parent already contains later migrations such as 20260317020451_secure_remaining_helper_rpcs.sql and 20260317160518_sso_skip_org_on_sso_domain.sql. Supabase applies migrations by timestamp history, and db push only backfills older missing versions when --include-all is used, so any environment that has already advanced past those later versions will skip this revoke during the normal deploy path. In that scenario cleanup_expired_demo_apps() stays executable for anon/authenticated in existing databases even though a fresh db reset and the new test both pass.
Useful? React with 👍 / 👎.
|
@riderx We just need you. Thank you for the pull request. We just need you to reply or fix your pull request according to the AI comments. When the AI reviewer is done and the build passes in the CI, we will merge your pull request. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1594b16b35
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| GRANT EXECUTE ON FUNCTION public.cleanup_expired_demo_apps() | ||
| TO SERVICE_ROLE; |
There was a problem hiding this comment.
cleanup_expired_demo_apps() is only wired into the internal cron runner via public.cron_tasks in supabase/migrations/20260121000000_add_demo_app_support.sql, and a repo-wide search shows no direct RPC caller that needs service_role execution. Per /workspace/capgo/AGENTS.md's least-privilege rule for PostgreSQL helper functions, granting service_role here keeps a destructive maintenance routine callable by any service-role client even though the existing cron-only helpers in supabase/migrations/20260104120000_revoke_process_function_queue_public_access.sql revoke that access entirely.
Useful? React with 👍 / 👎.
|
1 participant
Summary (AI generated)
PUBLIC,anon, andauthenticatedexecute access onpublic.cleanup_expired_demo_apps()service_roleexecution so the internal cron path still worksMotivation (AI generated)
cleanup_expired_demo_apps()is an internal maintenance function. A later grant state left it callable outside the intended cron/service-role path, which made the advisory actionable even though the function is not meant to be public.Business Impact (AI generated)
This removes an avoidable data-deletion risk from a public-facing surface and keeps Capgo's maintenance-only cleanup flow constrained to internal callers.
Test Plan (AI generated)
TMPDIR=/tmp TEMP=/tmp TMP=/tmp bunx sqlfluff lint --dialect postgres supabase/migrations/20260317011315_revoke_cleanup_expired_demo_apps_public_exec.sqlTMPDIR=/tmp TEMP=/tmp TMP=/tmp bunx eslint tests/cleanup-expired-demo-apps-rpc.test.tsSUPABASE_DB_URL='postgresql://postgres:postgres@127.0.0.1:59782/postgres' TMPDIR=/tmp TEMP=/tmp TMP=/tmp bunx vitest run tests/cleanup-expired-demo-apps-rpc.test.tsGenerated with AI
Summary by CodeRabbit
Tests
Chores