fix(security): Onda 20 — RE-GRANT t38 regression + bilateral SECURITY DEFINER gate#224
Conversation
… DEFINER gate Fixes a critical RLS regression introduced by PR #192 (t38_deploy_hardening_final) which revoked EXECUTE on is_admin_or_above and is_coord_or_above from authenticated. These functions are used in 83+29 RLS policies, so every logged-in operation broke with `42501: permission denied for function`. Also closes the SECURITY DEFINER ACL gate failure on org_has_any_members (created without REVOKE FROM PUBLIC/anon by 20260514000000). Root fix: rewrites audit_security_definer_acl() to detect 4 categories instead of 3, adding Caso 4 — SECURITY DEFINER citada em pg_policy sem EXECUTE para authenticated (the exact anti-pattern that t38 created and that the old gate did not catch). Docs: docs/SECURITY-DEFINER-PATTERN.md — canonical templates for the 3 valid patterns (RLS helper, RPC backend-only, public-intent). claude-code@atomicabr.com.br
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
WalkthroughMigração que formaliza padrão de hardening SECURITY DEFINER com novo documento de referência, reescreve audit_security_definer_acl() como bilateral gate detectando 4 categorias de violação (PUBLIC, anon, trigger, RLS policy), corrige regressões t38 em is_admin_or_above/is_coord_or_above, restringe org_has_any_members, e executa validação fail-fast. ChangesHardening SECURITY DEFINER com bilateral audit gate
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR addresses a production-breaking RLS regression caused by missing EXECUTE grants on SECURITY DEFINER helpers, and strengthens the CI “SECURITY DEFINER ACL gate” by adding a new detection case for SECURITY DEFINER functions referenced by RLS policies without EXECUTE for authenticated.
Changes:
- Re-grants
EXECUTEtoauthenticatedforis_admin_or_above(uuid)andis_coord_or_above(uuid)to restore RLS evaluation. - Revokes
EXECUTEonorg_has_any_members(uuid)fromPUBLICandanon. - Rewrites
audit_security_definer_acl()to add a new “used in RLS policy but missing EXECUTE for authenticated” detection case and adds a fail-fastRAISE EXCEPTIONblock. - Adds documentation describing canonical SECURITY DEFINER patterns and the gate behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| supabase/migrations/20260515150000_onda20_fix_t38_regression_and_bilateral_gate.sql | Fixes function grants involved in RLS breakage and expands the SECURITY DEFINER ACL audit gate with a new “missing EXECUTE for authenticated” category plus fail-fast behavior. |
| docs/SECURITY-DEFINER-PATTERN.md | Documents the SECURITY DEFINER + RLS patterns and the expected GRANT/REVOKE workflow aligned with the CI gate. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| -- (criada por 20260514000000 com default PUBLIC do PostgreSQL) | ||
| -- ───────────────────────────────────────────────────────────────── | ||
| REVOKE EXECUTE ON FUNCTION public.org_has_any_members(_org_id uuid) FROM PUBLIC; | ||
| REVOKE EXECUTE ON FUNCTION public.org_has_any_members(_org_id uuid) FROM anon; |
| FROM defs d | ||
| LEFT JOIN LATERAL ( | ||
| SELECT (aclexplode(d.proacl)).grantee | ||
| ) a ON true | ||
| WHERE a.grantee IS NOT NULL | ||
| ), |
| REVOKE EXECUTE ON FUNCTION public.minha_rpc_admin(text) FROM PUBLIC; | ||
| REVOKE EXECUTE ON FUNCTION public.minha_rpc_admin(text) FROM anon; | ||
| REVOKE EXECUTE ON FUNCTION public.minha_rpc_admin(text) FROM authenticated; | ||
| -- service_role mantém grant default |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@docs/SECURITY-DEFINER-PATTERN.md`:
- Around line 32-34: O bloco de erro no documento está sem linguagem e aciona o
lint MD040; edite o fence que contém "ERROR: 42501: permission denied for
function is_admin_or_above" para declarar a linguagem, por exemplo trocando ```
para ```text de forma que o bloco comece com ```text e termine com ```,
garantindo que o código seja tratado como texto e o warning seja eliminado.
In
`@supabase/migrations/20260515150000_onda20_fix_t38_regression_and_bilateral_gate.sql`:
- Around line 83-96: O CTE acl_expanded usa aclexplode(d.proacl) e ignora
objetos com proacl IS NULL (ACL implícita), por isso a função
audit_security_definer_acl() não detecta permissões efetivas vindas de
acldefault; corrija substituindo/estendendo a expansão para também usar
acldefault quando proacl IS NULL (por exemplo aclexplode(coalesce(d.proacl,
acldefault(...))) ou fazendo um UNION LATERAL com aclexplode(acldefault(...)))
garantindo que a lógica que verifica grantee/public_intent em acl_expanded (e o
mesmo padrão aplicado ao caso 4) passe a considerar as permissões default além
das ACLs explícitas.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cce0878e-5c7e-467c-a735-12415ca60aae
📒 Files selected for processing (2)
docs/SECURITY-DEFINER-PATTERN.mdsupabase/migrations/20260515150000_onda20_fix_t38_regression_and_bilateral_gate.sql
| ``` | ||
| ERROR: 42501: permission denied for function is_admin_or_above | ||
| ``` |
There was a problem hiding this comment.
Defina linguagem no bloco cercado para eliminar warning de lint.
O bloco de erro está sem linguagem e dispara MD040.
💡 Ajuste sugerido
-```
+```text
ERROR: 42501: permission denied for function is_admin_or_above</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.1)</summary>
[warning] 32-32: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
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-DEFINER-PATTERN.md around lines 32 - 34, O bloco de erro no
documento está sem linguagem e aciona o lint MD040; edite o fence que contém
"ERROR: 42501: permission denied for function is_admin_or_above" para declarar a
linguagem, por exemplo trocando paratext de forma que o bloco comece com
text e termine com , garantindo que o código seja tratado como texto e o
warning seja eliminado.
</details>
<!-- fingerprinting:phantom:poseidon:hawk -->
<!-- This is an auto-generated comment by CodeRabbit -->
| acl_expanded AS ( | ||
| SELECT | ||
| d.oid, | ||
| d.proname, | ||
| d.args, | ||
| d.is_trigger, | ||
| d.public_intent, | ||
| a.grantee::regrole::text AS grantee | ||
| FROM defs d | ||
| LEFT JOIN LATERAL ( | ||
| SELECT (aclexplode(d.proacl)).grantee | ||
| ) a ON true | ||
| WHERE a.grantee IS NOT NULL | ||
| ), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files "supabase/migrations/*.sql" | head -20Repository: adm01-debug/Promo_Gifts
Length of output: 1161
🏁 Script executed:
cat -n supabase/migrations/20260515150000_onda20_fix_t38_regression_and_bilateral_gate.sql | head -150Repository: adm01-debug/Promo_Gifts
Length of output: 7627
🏁 Script executed:
# Verificar tamanho do arquivo para leitura completa
wc -l supabase/migrations/20260515150000_onda20_fix_t38_regression_and_bilateral_gate.sqlRepository: adm01-debug/Promo_Gifts
Length of output: 155
🏁 Script executed:
cat -n supabase/migrations/20260515150000_onda20_fix_t38_regression_and_bilateral_gate.sql | tail -30Repository: adm01-debug/Promo_Gifts
Length of output: 1743
🏁 Script executed:
# Verificar se existem funções SECURITY DEFINER no public com proacl NULL
# usando readymade queries PostgreSQL (syntax check)
cat > /tmp/test_acl_logic.sql << 'EOF'
-- Teste 1: aclexplode() com NULL
SELECT COUNT(*) FROM aclexplode(NULL::aclitem[]);
-- Teste 2: acldefault() para função
SELECT acldefault('f'::char, 1::oid);
-- Teste 3: Comparar resultados
WITH test_null AS (
SELECT (aclexplode(NULL::aclitem[])).grantee AS grantee_null
),
test_default AS (
SELECT (aclexplode(acldefault('f'::char, 1::oid))).grantee AS grantee_default
)
SELECT
(SELECT COUNT(*) FROM test_null) AS null_count,
(SELECT COUNT(*) FROM test_default) AS default_count;
EOF
cat /tmp/test_acl_logic.sqlRepository: adm01-debug/Promo_Gifts
Length of output: 560
🏁 Script executed:
# Verificar a doc PostgreSQL: aclexplode() behavior
# Além disso, verificar se há referência à proacl no código ou comentários
rg "proacl|acldefault|aclexplode" --type sql supabase/migrations/ -nRepository: adm01-debug/Promo_Gifts
Length of output: 521
🌐 Web query:
PostgreSQL aclexplode NULL behavior proacl functions default permissions
💡 Result:
In PostgreSQL, the aclexplode function is a system function used to decompose an aclitem array (such as those found in system catalog columns like relacl) into a set of rows, where each row represents a specific privilege grant [1][2]. Regarding NULL behavior and default permissions: 1. NULL in ACL Columns: When an ACL column (like relacl in pg_class) is NULL, it signifies that the object is using the system's hard-wired default privileges [3][4]. It does not mean that no privileges exist; rather, it indicates that the default access rules apply [3]. 2. aclexplode and NULLs: The aclexplode function does not automatically resolve NULL values into the hard-wired default privileges [5]. If you pass a NULL value to aclexplode, it will return no rows, as there are no explicit grants to explode [1][5]. 3. Default Privileges: To determine the effective privileges for an object, you must account for both explicit grants (stored in the ACL column) and the implicit hard-wired defaults [3][6]. Because aclexplode only processes the explicit ACL array, it is not sufficient on its own to reconstruct the full set of effective permissions if the ACL column is NULL [5]. 4. Handling NULLs: If you need to programmatically determine effective permissions, you must implement logic to check if the ACL column is NULL and, if so, apply the corresponding hard-wired default privileges for that object type [5][3]. The system catalog pg_default_acl is used for user-defined default privileges (set via ALTER DEFAULT PRIVILEGES), but it is only consulted during object creation and does not replace the hard-wired defaults for existing objects [3][7][6]. In summary, aclexplode only explodes explicit grants [1][8]. A NULL value in an ACL column represents the presence of hard-wired default privileges, which aclexplode does not expand [5][3].
Citations:
- 1: https://pgpedia.info/a/aclexplode.html
- 2: https://stackoverflow.com/questions/43637442/can-i-rely-on-the-aclexplode-function-in-postgresql
- 3: https://www.postgresql.org/docs/current/catalog-pg-default-acl.html
- 4: https://postgrespro.com/docs/postgresql/14/catalog-pg-default-acl.html
- 5: https://www.postgresql.org/message-id/19426.1322432990%40sss.pgh.pa.us
- 6: https://www.postgresql.org/docs/current/sql-alterdefaultprivileges.html
- 7: https://www.postgresql.org/docs/15/sql-alterdefaultprivileges.html
- 8: https://pgxn.org/dist/aclexplode/doc/aclexplode.html
🏁 Script executed:
# Verificar se proowner está disponível e check lógica proposta
rg "proowner" supabase/migrations/ -A 2 -B 2Repository: adm01-debug/Promo_Gifts
Length of output: 962
🏁 Script executed:
# Procurar por CREATE FUNCTION sem GRANT explícito nas migrations
rg "CREATE.*FUNCTION.*public\." supabase/migrations/ | head -20Repository: adm01-debug/Promo_Gifts
Length of output: 2996
🏁 Script executed:
# Verificar funções SECURITY DEFINER públicas neste repo
rg "CREATE.*FUNCTION.*public\." supabase/migrations/ -A 5 | grep -E "CREATE|SECURITY DEFINER" | head -40Repository: adm01-debug/Promo_Gifts
Length of output: 4747
🏁 Script executed:
# Buscar especificamente por funções SECURITY DEFINER que não têm GRANT explícito logo após
rg "SECURITY DEFINER" supabase/migrations/ -B 2 -A 8 | head -80Repository: adm01-debug/Promo_Gifts
Length of output: 7464
Gate de auditoria ACL deixa falso negativo: proacl IS NULL não é expandido para permissões efetivas default.
O gate audit_security_definer_acl() usa aclexplode(d.proacl) em acl_expanded, mas quando proacl IS NULL (ACL hard-wired default, não explícita), aclexplode não retorna linhas. Funções SECURITY DEFINER sem GRANT explícito têm proacl=NULL por padrão e herdam permissões efetivas via acldefault()—casos que o gate não detecta.
Risco crítico: Funções com PUBLIC EXECUTE efetivo podem passar pelo gate sem alerta, replicando o problema que a migration tenta corrigir.
Patch objetivo
WITH defs AS (
SELECT
p.oid,
p.proname,
pg_get_function_identity_arguments(p.oid) AS args,
+ p.proowner,
p.proacl,
(pg_get_function_result(p.oid) = 'trigger') AS is_trigger,
(p.proname IN ('submit_quote_response', 'get_quote_token_by_value')) AS public_intent
FROM pg_proc p
JOIN pg_namespace n ON n.oid = p.pronamespace
WHERE n.nspname = 'public' AND p.prosecdef = true
),
acl_expanded AS (
SELECT
d.oid,
d.proname,
d.args,
d.is_trigger,
d.public_intent,
a.grantee::regrole::text AS grantee
FROM defs d
LEFT JOIN LATERAL (
- SELECT (aclexplode(d.proacl)).grantee
+ SELECT (aclexplode(COALESCE(d.proacl, acldefault('f', d.proowner)))).grantee
) a ON true
WHERE a.grantee IS NOT NULL
),Também afeta Caso 4 (linhas 128–137): funções em policies sem EXECUTE pra authenticated não serão detectadas se usarem ACL default.
🤖 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
`@supabase/migrations/20260515150000_onda20_fix_t38_regression_and_bilateral_gate.sql`
around lines 83 - 96, O CTE acl_expanded usa aclexplode(d.proacl) e ignora
objetos com proacl IS NULL (ACL implícita), por isso a função
audit_security_definer_acl() não detecta permissões efetivas vindas de
acldefault; corrija substituindo/estendendo a expansão para também usar
acldefault quando proacl IS NULL (por exemplo aclexplode(coalesce(d.proacl,
acldefault(...))) ou fazendo um UNION LATERAL com aclexplode(acldefault(...)))
garantindo que a lógica que verifica grantee/public_intent em acl_expanded (e o
mesmo padrão aplicado ao caso 4) passe a considerar as permissões default além
das ACLs explícitas.
![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: ef1be60522
ℹ️ 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".
| REVOKE EXECUTE ON FUNCTION public.org_has_any_members(_org_id uuid) FROM PUBLIC; | ||
| REVOKE EXECUTE ON FUNCTION public.org_has_any_members(_org_id uuid) FROM anon; |
There was a problem hiding this comment.
In preview/partial schemas where 20260514000000_fix_policy_idempotency_and_security.sql skipped creating org_has_any_members because organization_members did not exist (that migration wraps the function creation in an IF EXISTS table check), this unconditional REVOKE EXECUTE ON FUNCTION public.org_has_any_members(...) aborts with “function does not exist” before the rest of the migration can run. The surrounding migrations are explicitly defensive for those environments, so this fix should also check pg_proc or use dynamic guarded SQL before revoking from this function.
Useful? React with 👍 / 👎.
| GRANT EXECUTE ON FUNCTION public.is_admin_or_above(_user_id uuid) TO authenticated; | ||
| GRANT EXECUTE ON FUNCTION public.is_coord_or_above(_user_id uuid) TO authenticated; |
There was a problem hiding this comment.
In any preview or rebuilt environment that does not already have these legacy helper functions, these unconditional GRANT EXECUTE ON FUNCTION ... statements abort with “function does not exist” before the migration reaches the new ACL gate. The preceding t38 migration used pg_proc checks before touching the same functions, so this repair migration needs the same guard or dynamic SQL to remain safe for the partial schemas this migration set already supports.
Useful? React with 👍 / 👎.
| (p.proname IN ('submit_quote_response', 'get_quote_token_by_value')) AS public_intent | ||
| FROM pg_proc p | ||
| JOIN pg_namespace n ON n.oid = p.pronamespace | ||
| WHERE n.nspname = 'public' AND p.prosecdef = true |
There was a problem hiding this comment.
Because defs is restricted to p.prosecdef = true, Case 4 never inspects SECURITY INVOKER functions used by RLS policies. In this repo 20260513000001_t37a_security_invoker_safe_batch.sql already converted both is_admin_or_above(uuid) and is_coord_or_above(uuid) to SECURITY INVOKER, so this new gate would not have caught the exact t38 revocation regression it is meant to prevent if the explicit re-grants were removed or a future migration repeats the revoke.
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Onda 20 — Eliminar a categoria inteira de bug SECURITY DEFINER + RLS
Contexto
CI do run
25917921566(PR #221) ficou vermelho em 2 jobs após PR #192 / t38 ter sido mergeado hoje cedo:🔐 SECURITY DEFINER ACL gate(link compartilhado pelo PO)Run tests with coverage(escopo da Onda 21, fora desta PR)Investigação descobriu 2 bugs independentes que se combinaram:
Bug 1 — Regressão crítica em runtime (severidade > gate)
A migration
20260514000001_t38_deploy_hardening_final.sqlrevogouEXECUTEdeauthenticatedem:public.is_admin_or_above(uuid)— usada em 83 policies RLSpublic.is_coord_or_above(uuid)— usada em 29 policies RLSConfirmado em prod (
doufsxqlfjyuvxuezpln):Toda operação de usuário autenticado em ≥112 contextos de RLS está quebrada desde t38 ter sido aplicada. Sistema não está em prod real ainda (memória do PO), nenhum cliente afetado — mas tem que entrar verde antes do go-live.
Bug 2 — Gate falhando legitimamente
public.org_has_any_members(uuid), criada por20260514000000_fix_policy_idempotency_and_security.sql, sem REVOKE FROM PUBLIC. PostgreSQL grantia EXECUTE pra PUBLIC + anon por default →audit_security_definer_acl()reporta 2 violações.Causa raiz arquitetural — gate unilateral
O gate atual (
audit_security_definer_acl()) detecta excesso de privilégio (PUBLIC/anon), mas não detecta carência (função SECURITY DEFINER usada em policy sem EXECUTE praauthenticated). Por isso PR #192 passou no CI pré-merge. Bug não pegaria em revisão humana sem rodar prod.O que esta PR faz
Migration
supabase/migrations/20260515150000_onda20_fix_t38_regression_and_bilateral_gate.sql(162 linhas):GRANT EXECUTE TO authenticatedemis_admin_or_aboveeis_coord_or_above→ desbloqueia 112 policiesREVOKE FROM PUBLICeREVOKE FROM anonemorg_has_any_members(mantém authenticated + service_role)audit_security_definer_acl()com gate bilateral — 4 categorias em vez de 3:pg_policysem EXECUTE pra authenticated — RLS quebra com 42501RAISE EXCEPTIONfail-fast no final: se sobrar violação após a cirurgia, transação inteira aborta (rollback)Doc novo
docs/SECURITY-DEFINER-PATTERN.md(164 linhas):SECURITY DEFINERnão dispensa GRANT pro callerValidação
Dry-run em prod (DO block com RAISE para rollback intencional)
Inventário pré-fix vs pós-fix esperado
is_admin_or_aboveis_coord_or_aboveorg_has_any_memberscan_access_quoteNenhuma outra função SECURITY DEFINER em
publicestá em estado inconsistente — confirmado por query exaustiva contra prod.Plano de Ondas seguintes (decisão prévia do PO)
docs/HANDOFF-AUDIT-CLOSURE-2026-05-15.mdconsolidando os 63 achados da auditoria de 10/mai → PR/decisão/data, escudo contra perda de contexto entre sessões ClaudeRisco / Rollback
RAISE EXCEPTIONno fail-fast aborta automaticamente se algo escapouaudit_security_definer_acl()éSECURITY INVOKER(não toca o gatecheck-security-definer-hardening.mjs)Refs:
docs/AUDITORIA-PROFUNDA-PROMOGIFTS-PRE-PROD.mdabc6a8c(PR feat(db): t38 — deploy hardening final (security, indexes, autovacuum, FK) #192)20260514000000_fix_policy_idempotency_and_security.sqlSummary by cubic
Fixes the t38 RLS regression by restoring
EXECUTEforauthenticatedonpublic.is_admin_or_aboveandpublic.is_coord_or_above, and revokingPUBLIC/anononpublic.org_has_any_members. Adds a bilateralaudit_security_definer_acl()so CI catches both over- and under-privilegedSECURITY DEFINERfunctions, plus docs with canonical patterns.Bug Fixes
EXECUTEtoauthenticatedonis_admin_or_above(uuid)andis_coord_or_above(uuid)to unblock 112 RLS policies (fixed 42501 errors).PUBLIC/anonEXECUTEonorg_has_any_members(uuid)to close the ACL gate violation.New Features
audit_security_definer_acl()to add Case 4: function used in RLS but missingauthenticatedEXECUTE; includes fail-fastRAISE EXCEPTIONif violations remain.docs/SECURITY-DEFINER-PATTERN.mdwith templates and a checklist forSECURITY DEFINERfunctions (RLS helper, backend-only RPC, public-intent).Written for commit ef1be60. Summary will update on new commits.
Summary by CodeRabbit
Notas de Lançamento
Documentação
Correções