fix(security): patch 9 vulnerabilities across SQL functions and edge endpoints#1735
Conversation
added 4 commits
March 3, 2026 02:44
…ndpoint
Remove body.user_id override that allowed any authenticated user to inject
events into arbitrary organization Realtime channels. The endpoint now always
uses the authenticated user's ID from JWT (c.get('auth').userId).
Replace the permissive USING(true) SELECT policy on the manifest table with an org-scoped policy that joins through app_versions to verify the requesting user belongs to the same organization. Prevents cross-org leak of S3 paths, file hashes, and sizes.
Restrict PUT and POST routes to ['all','write'] and DELETE routes to ['all'] only. Previously all routes accepted read and upload key modes, allowing read-only API keys to perform destructive operations (update, create, delete organizations and members).
Return a generic 400 'validation_failed' error for all failure modes instead of distinct 401 (invalid credentials) and 403 (not a member) codes. This prevents unauthenticated attackers from using the endpoint as a credential oracle to enumerate valid email/password combinations and org membership.
Contributor
📝 WalkthroughWalkthroughDerives orgId strictly from authenticated identity for events; genericizes password/compliance failures to a 400 Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Func as HTTP Function
participant Auth as Auth Context
participant RBAC as RBAC Service
participant DB as Database
Client->>Func: HTTP request (key/token, body with app_id)
Func->>Auth: read identity (c.get('auth')?.userId)
Auth-->>Func: auth.userId (orgId)
Func->>RBAC: checkPermission(app.upload_bundle, app_id, orgId)
RBAC-->>Func: allowed / denied
alt allowed
Func->>DB: query/update app/manifest/app_versions scoped by orgId
DB-->>Func: rows / success
Func-->>Client: 200 OK
else denied
Func-->>Client: 403 no_permission / 400 validation_failed
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/functions/_backend/public/organization/index.ts`:
- Line 52: The DELETE /members route is using middlewareKey(['all','write']) but
must require only all-mode keys; update the route registration to call
middlewareKey with only ['all'] for the handler registered on
app.delete('/members', ...), ensuring any related auth checks or tests reference
the stricter mode (middlewareKey(['all']) instead of
middlewareKey(['all','write'])).
In `@supabase/migrations/20260303000000_fix_manifest_rls_crossorg.sql`:
- Around line 15-18: The RLS check is calling get_identity() directly inside the
check_min_rights call; because av.app_id is in scope you must replace the
identity resolver with get_identity_org_appid() so the app-aware identity is
used. Update the call inside the check that invokes "public"."check_min_rights"
(the clause currently passing "public"."get_identity"()) to instead pass
"public"."get_identity_org_appid"(), leaving av.owner_org and av.app_id
parameters in place so check_min_rights receives the app-aware identity.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
supabase/functions/_backend/private/events.tssupabase/functions/_backend/private/validate_password_compliance.tssupabase/functions/_backend/public/organization/index.tssupabase/migrations/20260303000000_fix_manifest_rls_crossorg.sql
added 2 commits
March 3, 2026 02:55
- DELETE /members restricted to ['all'] only (was ['all','write']) - SQL migration uses get_identity_org_appid() instead of get_identity() to follow project coding guidelines for RLS policies with app_id - Added 'anon' role to manifest RLS policy to match existing patterns
…check to update_metadata - Revoke anon+authenticated grants on upsert_version_meta, record_build_time, delete_old_deleted_versions (backend-only functions exposed to anonymous callers) - Revoke anon grants on 7 info-disclosure SECURITY DEFINER functions (get_app_metrics, get_plan_usage_percent_detailed, get_current_plan_max_org, get_current_plan_name_org, get_total_storage_size_org, get_total_app_storage_size_orgs, is_good_plan_v5_org) - Add missing checkPermission() call to bundle/update_metadata endpoint (every other bundle endpoint enforces RBAC, this one was skipped)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/migrations/20260303000001_revoke_dangerous_security_definer_grants.sql`:
- Around line 40-66: The listed functions (get_app_metrics overloads,
get_plan_usage_percent_detailed overloads, get_current_plan_max_org,
get_current_plan_name_org, get_total_storage_size_org,
get_total_app_storage_size_orgs, is_good_plan_v5_org) still allow the
authenticated role to bypass org authorization; either remove the remaining
GRANT ALL TO "authenticated" entries in this migration for those functions or
add explicit org-membership checks (e.g., using auth.uid(),
get_identity_org_appid, or equivalent guard logic) inside each function body to
validate the caller’s org before returning data; implement the safer short-term
fix by revoking authenticated rights for the listed functions in this patch, or
alternatively add the guard checks to each function (reference the function
names above when editing).
- Around line 19-66: This migration revokes EXECUTE only from "anon" and
"authenticated" but leaves PUBLIC with permissions; add explicit REVOKE ALL ON
FUNCTION ... FROM PUBLIC for each listed function signature (e.g.,
upsert_version_meta(p_app_id,p_version_id,p_size),
record_build_time(p_org_id,p_user_id,p_build_id,p_platform,p_build_time_unit),
delete_old_deleted_versions(), get_app_metrics(...),
get_plan_usage_percent_detailed(...), get_current_plan_max_org(orgid),
get_current_plan_name_org(orgid), get_total_storage_size_org(org_id),
get_total_app_storage_size_orgs(org_id,app_id), is_good_plan_v5_org(orgid)) so
every targeted function revokes privileges from PUBLIC as done in prior
migrations.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
supabase/functions/_backend/public/bundle/update_metadata.tssupabase/migrations/20260303000001_revoke_dangerous_security_definer_grants.sql
Added REVOKE ALL ... FROM PUBLIC for all targeted SECURITY DEFINER functions. This closes the privilege gap where PUBLIC pseudo-role could retain EXECUTE. Note: authenticated role is intentionally kept for 7 info-disclosure functions because the frontend (Usage.vue, supabase.ts) calls these via authenticated JWT. Adding auth.uid() + org membership checks inside function bodies is the proper follow-up to close the cross-tenant vector without breaking the UI.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
supabase/migrations/20260303000001_revoke_dangerous_security_definer_grants.sql (1)
39-47: Cross-tenant risk documented; follow-up needed.The comment correctly identifies that authenticated users can still query any org's data. Since revoking
authenticatedwould break the frontend, the documented follow-up—addingauth.uid()+ org membership guards inside each function body—is the appropriate remediation path.Consider creating a tracking issue for this follow-up to ensure it doesn't slip through.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/migrations/20260303000001_revoke_dangerous_security_definer_grants.sql` around lines 39 - 47, The SQL migration notes a cross-tenant risk where authenticated users can access any org's metrics; fix by adding explicit authorization checks inside each affected function (use auth.uid() and verify org membership/role) — update every function referenced in this migration to validate the caller's org membership before returning org-scoped data (e.g., add membership checks in the bodies of the metrics/plan/usage functions mentioned in the comment), and open a tracked issue (or link an existing ticket ID) to ensure the follow-up work to implement these auth guards across all functions is completed and reviewed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@supabase/migrations/20260303000001_revoke_dangerous_security_definer_grants.sql`:
- Around line 39-47: The SQL migration notes a cross-tenant risk where
authenticated users can access any org's metrics; fix by adding explicit
authorization checks inside each affected function (use auth.uid() and verify
org membership/role) — update every function referenced in this migration to
validate the caller's org membership before returning org-scoped data (e.g., add
membership checks in the bodies of the metrics/plan/usage functions mentioned in
the comment), and open a tracked issue (or link an existing ticket ID) to ensure
the follow-up work to implement these auth guards across all functions is
completed and reviewed.
|
Member
|
this is not OK, security should be disclosed privately banned |
Member
|
AI SPAM |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR addresses 9 security vulnerabilities found during a comprehensive audit, organized in two waves.
Wave 1: Edge Function & RLS Fixes
body.user_idoverridesauth.uid(), allowing cross-user event injectionauth.uid()middlewareKey(['read'])— read-only keys can delete membersmiddlewareKey(['all'])Wave 2: SECURITY DEFINER Function Grants & RBAC
Migration Files
20260303000000_fix_manifest_rls_crossorg.sql— Wave 1 RLS fix20260303000001_revoke_dangerous_security_definer_grants.sql— Wave 2 REVOKE statementsFollow-up Recommendation
The 7 info-disclosure functions (item #7) still allow any
authenticateduser to query metrics for any org. Addingauth.uid()+ org membership checks inside each function body is recommended as a follow-up to fully close this vector.Related
Test plan
/private/eventswith abody.user_iddifferent from the JWT user — verify the event is attributed to the JWT identity, notbody.user_id.SELECT * FROM manifest— verify only manifests for org-1 apps are returned, not org-2 manifests.DELETE /organization/members— should return 403/unauthorized./private/validate_password_compliancewith invalid credentials vs valid credentials with wrong org — both should return identical 400validation_failedresponse.SELECT upsert_version_meta(...)as anon role — should fail with permission denied.POST /bundle/update_metadatawithout proper app permissions — should returnno_permissionerror.Screenshots
N/A — backend-only changes (Edge Functions + SQL migrations). No UI changes.
Checklist
bun run lint:backend && bun run lint.