fix(db): deny retention metrics user access#1961
Conversation
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughEnables Row Level Security on two retention metric tables and replaces existing policies with a blanket deny-all policy; adds pgTAP checks verifying RLS and the deny policy; and adds a retry/backoff loop plus a bundle-writing helper to CLI metadata tests. Changes
Sequence Diagram(s)(No sequence diagrams generated — changes are database migrations, test assertions, and test retry/helpers without a multi-component control-flow that benefits from a diagram.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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.1.0)supabase/migrations/20260427110612_retention_metrics_service_role_rls.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: supabase/tests/26_test_rls_policies.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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@supabase/tests/26_test_rls_policies.sql`:
- Around line 231-247: Add explicit checks that row-level security is enabled
for the two tables by asserting relrowsecurity is true for
public.daily_revenue_metrics and public.processed_stripe_events: locate the
existing tests that call policies_are for those tables and add corresponding
assertions that query pg_class.relrowsecurity (or use a helper/assert function)
to ensure relrowsecurity = true for both tables so the test fails if RLS is ever
turned off.
In `@tests/cli-meta.test.ts`:
- Around line 17-26: The retry loop in tests/cli-meta.test.ts currently only
catches assertion failures because sdk.checkBundleCompatibility(...) is called
outside the try, so if that SDK call throws the loop exits immediately; move the
sdk.checkBundleCompatibility call inside the try (or wrap it in its own
try/catch) so that both thrown errors from checkBundleCompatibility and
assertion failures are caught and the retry/backoff can continue, and apply the
same change to the similar block around lines 47-53 that calls
sdk.checkBundleCompatibility or equivalent.
- Around line 32-44: The test's ❌ branch only checks for presence of
localVersion and doesn't assert incompatibility; update the else branch to
assert mismatch or use the SDK compatibility field if present. Specifically, in
the block that inspects column4, replace the current else assertions with either
expect(packageEntry!.localVersion).not.toEqual(packageEntry!.remoteVersion) (or
use semver comparison/coerce if you expect range resolution) or, if the SDK
payload exposes a compatibility flag (e.g., packageEntry!.compatible or
packageEntry!.isCompatible), assert
expect(packageEntry!.compatible).toBe(false); keep references to packageEntry,
column4, column2 and column3 to locate the change.
🪄 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
Run ID: c10e06a0-3f56-4d33-8384-c44712e79716
📒 Files selected for processing (3)
supabase/migrations/20260427110612_retention_metrics_service_role_rls.sqlsupabase/tests/26_test_rls_policies.sqltests/cli-meta.test.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/cli-meta.test.ts (1)
37-44:⚠️ Potential issue | 🟠 Major
❌branch still doesn’t assert incompatibility.On Line 42 and Line 43, the
❌path only checks presence. Since Line 32 and Line 33 already validate version fields exist, this branch can still pass false positives for incompatible cases.Suggested assertion tightening
- if (column4 === '✅') { - // Compatible - versions should match (considering semver resolution) - expect(packageEntry!.remoteVersion).toBeDefined() - } - else { - // Incompatible - just verify the entry exists - expect(packageEntry!.localVersion).toBeDefined() - } + const compatibilityFlag = packageEntry!.compatible ?? packageEntry!.isCompatible + if (column4 === '✅') { + if (compatibilityFlag !== undefined) + expect(compatibilityFlag).toBe(true) + else + expect(packageEntry!.localVersion).toEqual(packageEntry!.remoteVersion) + } + else { + if (compatibilityFlag !== undefined) + expect(compatibilityFlag).toBe(false) + else + expect(packageEntry!.localVersion).not.toEqual(packageEntry!.remoteVersion) + }#!/bin/bash set -euo pipefail # Verify the canonical compatibility field exposed by the SDK payload rg -nP --type=ts -C4 '\bcheckBundleCompatibility\s*\(' rg -nP --type=ts -C4 '\b(compatible|isCompatible|status)\b'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli-meta.test.ts` around lines 37 - 44, The failing branch labeled '❌' only checks existence; tighten it to assert actual incompatibility by verifying mismatch between packageEntry versions or an explicit incompatible flag: for example, in the else branch where column4 !== '✅', assert that packageEntry!.localVersion !== packageEntry!.remoteVersion (or if the payload exposes a compatibility boolean, assert packageEntry!.compatible === false) using the existing variables column4 and packageEntry (and its localVersion/remoteVersion fields).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/cli-meta.test.ts`:
- Around line 37-44: The failing branch labeled '❌' only checks existence;
tighten it to assert actual incompatibility by verifying mismatch between
packageEntry versions or an explicit incompatible flag: for example, in the else
branch where column4 !== '✅', assert that packageEntry!.localVersion !==
packageEntry!.remoteVersion (or if the payload exposes a compatibility boolean,
assert packageEntry!.compatible === false) using the existing variables column4
and packageEntry (and its localVersion/remoteVersion fields).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7678af39-ae4f-4bfd-b120-08036644c854
📒 Files selected for processing (2)
supabase/tests/26_test_rls_policies.sqltests/cli-meta.test.ts
riderx
changed the title
|
1 participant
Summary (AI generated)
public.daily_revenue_metricsandpublic.processed_stripe_eventsas backend-only tables under the repo's deny-all RLS patternservice_rolepolicy approach with explicit deny-all policies on both internal Stripe retention tablesMotivation (AI generated)
The retention metrics tables are backend-only Stripe webhook ledgers. In environments where RLS is auto-enabled for new
publictables, they can end up with RLS enabled but no policies, which triggers the Supabase linter and leaves the intended access model implicit.For backend-only tables in this repo,
service_roleis not modeled through RLS policies because it bypasses RLS. The established pattern is to keep grants restricted and add an explicit deny-all policy so user-context access stays impossible while the linter remains satisfied.The branch also needed a small test-only fix because the existing CLI metadata test could fail on Vitest retries: once an initial upload partially succeeded, the retry re-used identical bundle contents and hit the duplicate-checksum guard before it could re-check metadata.
Business Impact (AI generated)
This removes noisy security-linter findings on admin retention analytics tables, makes the backend-only access model explicit in the same way as similar internal tables, and reduces migration review friction without changing customer-facing behavior.
The metadata test hardening lowers false-negative CI failures, which keeps schema changes from getting blocked by an unrelated flaky retry path.
Test Plan (AI generated)
bunx supabase db reset --yes --workdir .context/supabase-worktrees/f5206569bunx supabase test db supabase/tests/26_test_rls_policies.sql --workdir .context/supabase-worktrees/f5206569bunx eslint tests/cli-meta.test.tstests/cli-meta.test.tsupload path is healthy enough to re-run withoutname resolution failedGenerated with AI
Summary by CodeRabbit
Release Notes