fix(rls): fail fast anon REST scans #1595
Conversation
📝 WalkthroughWalkthroughAdds two public PostgreSQL functions— Changes
Sequence DiagramsequenceDiagram
participant Client as Client/API Request
participant PostgREST as PostgREST
participant HasAuth as has_auth_or_valid_apikey()
participant AllowedApps as allowed_read_apps()
participant RLS as RLS Policy
participant DB as Database
Client->>PostgREST: SELECT (with/without auth/API key)
PostgREST->>HasAuth: evaluate guard predicate
alt No auth & invalid API key
HasAuth-->>PostgREST: FALSE
PostgREST-->>Client: Access Denied (fail-fast)
else Authenticated or valid API key
HasAuth-->>PostgREST: TRUE
PostgREST->>AllowedApps: compute allowed app_ids/orgs
AllowedApps->>DB: read RBAC, org, api-key constraints
DB-->>AllowedApps: allowed ids array
PostgREST->>RLS: apply policy filter (app_id ∈ allowed array / audit_logs_allowed_orgs())
RLS->>DB: execute filtered query (index-friendly)
DB-->>PostgREST: result set
PostgREST-->>Client: Authorized rows
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
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)
No actionable comments were generated in the recent review. 🎉 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.0)supabase/migrations/20260206123000_fail_fast_postgrest_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/41_test_fail_fast_postgrest_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: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: bfff204e08
ℹ️ 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".
| -- Enforce API key scoping (if present). | ||
| IF v_api_key.id IS NOT NULL | ||
| AND COALESCE(array_length(v_api_key.limited_to_orgs, 1), 0) > 0 | ||
| AND NOT (v_app.owner_org = ANY(v_api_key.limited_to_orgs)) | ||
| THEN |
There was a problem hiding this comment.
In allowed_read_apps, API key scoping is applied whenever a valid capgkey header is present, even if auth.uid() is already set (the function always loads the key and then applies the limited_to_orgs/apps filters). This is a behavior change from the previous get_identity_org_appid-based policies, which ignored API keys once a user session was present. As a result, an authenticated user who sends a limited API key header (e.g., a client that always includes capgkey) can lose access to apps they otherwise can read. Consider only applying API key scoping when the request is unauthenticated or when you explicitly want key-scoped access to override user rights.
Useful? React with 👍 / 👎.
![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
🤖 Fix all issues with AI agents
In `@supabase/migrations/20260206123000_fail_fast_postgrest_rls.sql`:
- Around line 217-281: The RLS policies on channel_devices and build_requests
use allowed_read_apps() but must follow the mandatory guideline to call
get_identity_org_appid() when an app_id column exists; update the USING clauses
in the CREATE POLICY statements for "Allow read for auth, api keys (read+)" on
"channel_devices" and "Allow org members to select build_requests" on
"build_requests" to call public.get_identity_org_appid(app_id) (or an equivalent
wrapper) instead of app_id = ANY(public.allowed_read_apps()), ensuring
auth/APikey checks via public.has_auth_or_valid_apikey() remain; if this
optimization cannot be changed, obtain explicit exception approval and document
it.
In `@supabase/tests/41_test_fail_fast_postgrest_rls.sql`:
- Around line 61-123: Tests 3–6 use position(... in (SELECT pg_get_expr(...)))
which yields NULL when no matching policy exists, causing ok(NULL, ...) to fail
silently; fix each test by wrapping the subquery result in COALESCE(..., '') so
position() operates on an empty string instead of NULL. Specifically, update the
expressions in the tests referencing pg_get_expr(p.polqual, p.polrelid) for
relname 'app_versions' (policy 'Allow for auth, api keys (read+)') and relname
'audit_logs' (policy 'Allow select for auth, api keys (super_admin+)') to
COALESCE( (SELECT pg_get_expr(...)), '' ), keeping the surrounding position(...)
and ok(...) checks intact; apply the same change for checks that look for
'get_identity_org_appid', 'allowed_read_apps', 'audit_logs_allowed_orgs', and
'has_auth_or_valid_apikey'.
- Around line 37-59: The EXPLAIN-based assertions in tests 1–2 (using
pg_temp.explain_text on queries against public.audit_logs and
public.app_versions) are fragile due to constant-folding of
has_auth_or_valid_apikey() and small-table planner heuristics; update these
tests to avoid asserting on textual “Index Cond”/absence of “Seq Scan”. Either
replace them with catalog-style checks similar to tests 3–6 (checking that the
RLS policy/index audit_logs_allowed_orgs or allowed_read_apps exists and is
correct) or switch the plan assertion to EXPLAIN (FORMAT JSON) and assert only
the top-level node type and presence of a One-Time Filter (i.e., that the
top-level node is "Result" and contains a one-time filter matching
has_auth_or_valid_apikey), so the test checks the presence of the fail-fast
guard in a planner-stable way.
🧹 Nitpick comments (2)
supabase/migrations/20260206123000_fail_fast_postgrest_rls.sql (2)
140-153: Inconsistent empty-array guard betweenlimited_to_orgsandlimited_to_apps.Line 142 uses a clean
COALESCE(array_length(...), 0) > 0pattern forlimited_to_orgs, while line 149 usesIS DISTINCT FROM '{}'forlimited_to_apps. The latter relies on PostgreSQL coercing the'{}'literal totext[]and on the three-valued-logic fallthrough on line 150 (= ANY(NULL)→ NULL → no CONTINUE) to handle theNULLcase.Both are functionally correct, but aligning on one pattern improves readability and avoids subtle surprises.
♻️ Align limited_to_apps check with limited_to_orgs style
IF v_api_key.id IS NOT NULL - AND v_api_key.limited_to_apps IS DISTINCT FROM '{}' + AND COALESCE(array_length(v_api_key.limited_to_apps, 1), 0) > 0 AND NOT (v_app.app_id = ANY(v_api_key.limited_to_apps)) THEN CONTINUE; END IF;
55-212:allowed_read_apps()duplicates API-key lookup done byhas_auth_or_valid_apikey().Every policy pairs both functions, so for anon-with-apikey requests the
find_apikey_by_valuelookup runs twice per statement. The early-return on line 92-95 ensures the expensive loop is still skipped for unauthenticated requests, so the duplicate lookup is only incurred for valid API-key callers and is a single-row index lookup — likely negligible in practice.If this ever shows up in profiling, one option is to store the resolved API-key record in a session-scoped GUC or temp table so both functions share the result.
| -- 3) Apply fail-fast + index-friendly policies on the largest affected tables. | ||
|
|
||
| -- audit_logs: keep org_id predicate but add a one-time guard so unauthenticated anon requests do not scan. | ||
| DROP POLICY IF EXISTS "Allow select for auth, api keys (super_admin+)" ON "public"."audit_logs"; | ||
| CREATE POLICY "Allow select for auth, api keys (super_admin+)" ON "public"."audit_logs" | ||
| FOR SELECT TO "anon", "authenticated" | ||
| USING ( | ||
| public.has_auth_or_valid_apikey('{read,upload,write,all}'::public.key_mode[]) | ||
| AND "org_id" = ANY("public"."audit_logs_allowed_orgs"()) | ||
| ); | ||
|
|
||
| -- app_versions + app_versions_meta: avoid per-row identity resolution; use allowed_read_apps(). | ||
| DROP POLICY IF EXISTS "Allow for auth, api keys (read+)" ON "public"."app_versions"; | ||
| CREATE POLICY "Allow for auth, api keys (read+)" ON "public"."app_versions" | ||
| FOR SELECT TO "anon", "authenticated" | ||
| USING ( | ||
| public.has_auth_or_valid_apikey('{read,upload,write,all}'::public.key_mode[]) | ||
| AND "app_id" = ANY("public"."allowed_read_apps"()) | ||
| ); | ||
|
|
||
| DROP POLICY IF EXISTS "Allow read for auth (read+)" ON "public"."app_versions_meta"; | ||
| CREATE POLICY "Allow read for auth (read+)" ON "public"."app_versions_meta" | ||
| FOR SELECT TO "anon", "authenticated" | ||
| USING ( | ||
| public.has_auth_or_valid_apikey('{read,upload,write,all}'::public.key_mode[]) | ||
| AND "app_id" = ANY("public"."allowed_read_apps"()) | ||
| ); | ||
|
|
||
| -- 4) (Optional hardening) Replace common read policies to avoid per-row get_identity_org_appid() on large tables. | ||
| -- apps | ||
| DROP POLICY IF EXISTS "Allow for auth, api keys (read+)" ON "public"."apps"; | ||
| CREATE POLICY "Allow for auth, api keys (read+)" ON "public"."apps" | ||
| FOR SELECT TO "anon", "authenticated" | ||
| USING ( | ||
| public.has_auth_or_valid_apikey('{read,upload,write,all}'::public.key_mode[]) | ||
| AND "app_id" = ANY("public"."allowed_read_apps"()) | ||
| ); | ||
|
|
||
| -- channels | ||
| DROP POLICY IF EXISTS "Allow select for auth, api keys (read+)" ON "public"."channels"; | ||
| CREATE POLICY "Allow select for auth, api keys (read+)" ON "public"."channels" | ||
| FOR SELECT TO "anon", "authenticated" | ||
| USING ( | ||
| public.has_auth_or_valid_apikey('{read,upload,write,all}'::public.key_mode[]) | ||
| AND "app_id" = ANY("public"."allowed_read_apps"()) | ||
| ); | ||
|
|
||
| -- channel_devices | ||
| DROP POLICY IF EXISTS "Allow read for auth, api keys (read+)" ON "public"."channel_devices"; | ||
| DROP POLICY IF EXISTS "Allow read for auth (read+)" ON "public"."channel_devices"; | ||
| CREATE POLICY "Allow read for auth, api keys (read+)" ON "public"."channel_devices" | ||
| FOR SELECT TO "anon", "authenticated" | ||
| USING ( | ||
| public.has_auth_or_valid_apikey('{read,upload,write,all}'::public.key_mode[]) | ||
| AND "app_id" = ANY("public"."allowed_read_apps"()) | ||
| ); | ||
|
|
||
| -- build_requests | ||
| DROP POLICY IF EXISTS "Allow org members to select build_requests" ON "public"."build_requests"; | ||
| CREATE POLICY "Allow org members to select build_requests" ON "public"."build_requests" | ||
| FOR SELECT TO "anon", "authenticated" | ||
| USING ( | ||
| public.has_auth_or_valid_apikey('{read,upload,write,all}'::public.key_mode[]) | ||
| AND "app_id" = ANY("public"."allowed_read_apps"()) | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for app_id column definitions in channel_devices and build_requests tables
# Look through migrations to find table definitions
find supabase/migrations -name "*.sql" -type f | head -20Repository: Cap-go/capgo
Length of output: 1251
🏁 Script executed:
#!/bin/bash
# Search for channel_devices table definition
rg -A 20 'CREATE TABLE.*channel_devices' supabase/migrations/Repository: Cap-go/capgo
Length of output: 1646
🏁 Script executed:
#!/bin/bash
# Search for build_requests table definition
rg -A 20 'CREATE TABLE.*build_requests' supabase/migrations/Repository: Cap-go/capgo
Length of output: 2354
🏁 Script executed:
#!/bin/bash
# Search for allowed_read_apps function definition
rg -B 5 -A 15 'allowed_read_apps' supabase/migrations/ --type sql | head -50Repository: Cap-go/capgo
Length of output: 5113
🏁 Script executed:
#!/bin/bash
# Search for allowed_read_apps function in schema definitions
rg 'allowed_read_apps' supabase/Repository: Cap-go/capgo
Length of output: 1834
🏁 Script executed:
#!/bin/bash
# Get the full allowed_read_apps function definition
rg -A 60 'CREATE OR REPLACE FUNCTION.*allowed_read_apps' supabase/migrations/20260206123000_fail_fast_postgrest_rls.sqlRepository: Cap-go/capgo
Length of output: 2000
🏁 Script executed:
#!/bin/bash
# Check if there's any reference to get_identity_org_appid in the migration
rg 'get_identity_org_appid' supabase/migrations/20260206123000_fail_fast_postgrest_rls.sqlRepository: Cap-go/capgo
Length of output: 275
🏁 Script executed:
#!/bin/bash
# Check what the original policies looked like before this migration
rg 'get_identity_org_appid' supabase/migrations/ --type sql | head -20Repository: Cap-go/capgo
Length of output: 2682
🏁 Script executed:
#!/bin/bash
# Get the full allowed_read_apps function to see complete implementation
sed -n '/CREATE OR REPLACE FUNCTION.*allowed_read_apps/,/^$$;/p' supabase/migrations/20260206123000_fail_fast_postgrest_rls.sqlRepository: Cap-go/capgo
Length of output: 5093
RLS policies deviate from mandatory guidelines; get_identity_org_appid() must be used when app_id exists on the table.
The policies correctly identify that both channel_devices and build_requests have app_id columns, and they filter on app_id = ANY(allowed_read_apps()). However, this approach contradicts the mandatory coding guideline: "ALWAYS use get_identity_org_appid() when app_id exists on the table; use get_identity_org_allowed() only as an absolute last resort."
While allowed_read_apps() is a well-implemented optimization that handles auth, API keys, RBAC, and 2FA validation, it shifts from the mandated per-row identity validation pattern to a statement-scoped approach. This deviates from the explicit "ALWAYS" requirement in the guidelines, regardless of performance rationale.
Either:
- Refactor policies to use
get_identity_org_appid(app_id)in the USING clause to comply with mandatory guidelines, or - Obtain explicit exception approval if this optimization is deemed critical for the fail-fast goal.
🤖 Prompt for AI Agents
In `@supabase/migrations/20260206123000_fail_fast_postgrest_rls.sql` around lines
217 - 281, The RLS policies on channel_devices and build_requests use
allowed_read_apps() but must follow the mandatory guideline to call
get_identity_org_appid() when an app_id column exists; update the USING clauses
in the CREATE POLICY statements for "Allow read for auth, api keys (read+)" on
"channel_devices" and "Allow org members to select build_requests" on
"build_requests" to call public.get_identity_org_appid(app_id) (or an equivalent
wrapper) instead of app_id = ANY(public.allowed_read_apps()), ensuring
auth/APikey checks via public.has_auth_or_valid_apikey() remain; if this
optimization cannot be changed, obtain explicit exception approval and document
it.
There was a problem hiding this comment.
Pull request overview
Introduces statement-scoped RLS helpers and policy rewrites aimed at preventing expensive table scans for unauthenticated PostgREST anon requests, plus adds regression tests to validate the “fail fast” query plan behavior.
Changes:
- Add
has_auth_or_valid_apikey(...)guard andallowed_read_apps()helper for statement-scoped auth/app allow-list evaluation in RLS. - Update SELECT RLS policies on large tables (e.g.
audit_logs,app_versions, etc.) to use the new guard + allow-list predicates. - Add pgTAP regression tests asserting EXPLAIN plans/policy definitions for the new fail-fast behavior.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
supabase/migrations/20260206123000_fail_fast_postgrest_rls.sql |
Adds new SECURITY DEFINER helpers and rewrites multiple SELECT RLS policies to gate unauthenticated anon queries and use index-friendly predicates. |
supabase/tests/41_test_fail_fast_postgrest_rls.sql |
Adds pgTAP regression tests around EXPLAIN output and pg_policy expressions for the new RLS behavior. |
bun.lock |
Lockfile metadata update (configVersion). |
| GRANT EXECUTE ON FUNCTION "public"."has_auth_or_valid_apikey"("keymode" "public"."key_mode"[]) TO "anon"; | ||
| GRANT EXECUTE ON FUNCTION "public"."has_auth_or_valid_apikey"("keymode" "public"."key_mode"[]) TO "authenticated"; |
There was a problem hiding this comment.
The GRANT statements include the parameter name ("keymode") in the function signature. In Postgres you must specify only the argument types in GRANT/REVOKE (e.g. has_auth_or_valid_apikey(public.key_mode[])); including the parameter name will make this migration fail to apply, leaving the old RLS policies in place.
| GRANT EXECUTE ON FUNCTION "public"."has_auth_or_valid_apikey"("keymode" "public"."key_mode"[]) TO "anon"; | |
| GRANT EXECUTE ON FUNCTION "public"."has_auth_or_valid_apikey"("keymode" "public"."key_mode"[]) TO "authenticated"; | |
| GRANT EXECUTE ON FUNCTION public.has_auth_or_valid_apikey(public.key_mode[]) TO "anon"; | |
| GRANT EXECUTE ON FUNCTION public.has_auth_or_valid_apikey(public.key_mode[]) TO "authenticated"; |
| SELECT ok( | ||
| position('Index Cond' in (SELECT txt FROM p)) > 0 | ||
| AND position('audit_logs_allowed_orgs' in (SELECT txt FROM p)) > 0 | ||
| AND position('has_auth_or_valid_apikey' in (SELECT txt FROM p)) > 0 | ||
| AND position('Seq Scan' in (SELECT txt FROM p)) = 0, |
There was a problem hiding this comment.
These assertions don’t verify the "One-Time Filter" fail-fast gate mentioned in the header comment; they only look for 'Index Cond' and absence of 'Seq Scan'. Since EXPLAIN output can vary by stats/size (and may omit 'Index Cond'), this can be flaky. Consider asserting that 'One-Time Filter' is present (and/or set enable_seqscan=off if you specifically want to validate an indexable predicate).
| -- Test 1: audit_logs query plan should be index-friendly and include the fail-fast guard. | ||
| WITH p AS ( | ||
| SELECT pg_temp.explain_text('SELECT id FROM public.audit_logs LIMIT 1') AS txt | ||
| ) | ||
| SELECT ok( | ||
| position('Index Cond' in (SELECT txt FROM p)) > 0 | ||
| AND position('audit_logs_allowed_orgs' in (SELECT txt FROM p)) > 0 | ||
| AND position('has_auth_or_valid_apikey' in (SELECT txt FROM p)) > 0 | ||
| AND position('Seq Scan' in (SELECT txt FROM p)) = 0, | ||
| 'audit_logs unauthed anon query is index-friendly and includes the guard' | ||
| ); | ||
|
|
||
| -- Test 2: app_versions query plan should be index-friendly and include the fail-fast guard. | ||
| WITH p AS ( | ||
| SELECT pg_temp.explain_text('SELECT id FROM public.app_versions LIMIT 1') AS txt | ||
| ) | ||
| SELECT ok( | ||
| position('Index Cond' in (SELECT txt FROM p)) > 0 | ||
| AND position('allowed_read_apps' in (SELECT txt FROM p)) > 0 | ||
| AND position('has_auth_or_valid_apikey' in (SELECT txt FROM p)) > 0 | ||
| AND position('Seq Scan' in (SELECT txt FROM p)) = 0, | ||
| 'app_versions unauthed anon query is index-friendly and includes the guard' |
There was a problem hiding this comment.
Same issue as Test 1: this assertion doesn’t check for the intended fail-fast "One-Time Filter" gate, and relying on 'Index Cond'/'Seq Scan' substrings can be planner-dependent. Consider asserting 'One-Time Filter' is present (and/or set enable_seqscan=off to make indexability checks deterministic).
| -- Test 1: audit_logs query plan should be index-friendly and include the fail-fast guard. | |
| WITH p AS ( | |
| SELECT pg_temp.explain_text('SELECT id FROM public.audit_logs LIMIT 1') AS txt | |
| ) | |
| SELECT ok( | |
| position('Index Cond' in (SELECT txt FROM p)) > 0 | |
| AND position('audit_logs_allowed_orgs' in (SELECT txt FROM p)) > 0 | |
| AND position('has_auth_or_valid_apikey' in (SELECT txt FROM p)) > 0 | |
| AND position('Seq Scan' in (SELECT txt FROM p)) = 0, | |
| 'audit_logs unauthed anon query is index-friendly and includes the guard' | |
| ); | |
| -- Test 2: app_versions query plan should be index-friendly and include the fail-fast guard. | |
| WITH p AS ( | |
| SELECT pg_temp.explain_text('SELECT id FROM public.app_versions LIMIT 1') AS txt | |
| ) | |
| SELECT ok( | |
| position('Index Cond' in (SELECT txt FROM p)) > 0 | |
| AND position('allowed_read_apps' in (SELECT txt FROM p)) > 0 | |
| AND position('has_auth_or_valid_apikey' in (SELECT txt FROM p)) > 0 | |
| AND position('Seq Scan' in (SELECT txt FROM p)) = 0, | |
| 'app_versions unauthed anon query is index-friendly and includes the guard' | |
| -- Test 1: audit_logs query plan should include the fail-fast One-Time Filter guard. | |
| WITH p AS ( | |
| SELECT pg_temp.explain_text('SELECT id FROM public.audit_logs LIMIT 1') AS txt | |
| ) | |
| SELECT ok( | |
| position('One-Time Filter' in (SELECT txt FROM p)) > 0 | |
| AND position('audit_logs_allowed_orgs' in (SELECT txt FROM p)) > 0 | |
| AND position('has_auth_or_valid_apikey' in (SELECT txt FROM p)) > 0, | |
| 'audit_logs unauthed anon query includes the One-Time Filter guard' | |
| ); | |
| -- Test 2: app_versions query plan should include the fail-fast One-Time Filter guard. | |
| WITH p AS ( | |
| SELECT pg_temp.explain_text('SELECT id FROM public.app_versions LIMIT 1') AS txt | |
| ) | |
| SELECT ok( | |
| position('One-Time Filter' in (SELECT txt FROM p)) > 0 | |
| AND position('allowed_read_apps' in (SELECT txt FROM p)) > 0 | |
| AND position('has_auth_or_valid_apikey' in (SELECT txt FROM p)) > 0, | |
| 'app_versions unauthed anon query includes the One-Time Filter guard' |
riderx
force-pushed
the
riderx/rest-timeout-audit
branch
from
bfff204 to
44276c6
Compare
|
1 participant
Summary (AI generated)
has_auth_or_valid_apikey) and statement-scoped allowed app list (allowed_read_apps) to prevent anon/no-capgkey PostgREST queries from triggering expensive per-row auth work.audit_logs,app_versions, and related tables to keep index-friendly predicates (app_id = ANY(...),org_id = ANY(...)) and avoidget_identity_*per-row calls.Test plan (AI generated)
Screenshots (AI generated)
Checklist (AI generated)
bun run lint:backend.Generated with AI
Summary by CodeRabbit
Refactor
Tests