fix(db): anon could execute as a superuser, and a rebuild would restore that - #2707
Conversation
…re that
Three SECURITY DEFINER functions in `public` -- claim_/complete_/fail_
studio_board_publish -- are owned by supabase_admin (rolsuper), so they run
as a superuser and bypass RLS. PUBLIC and anon both held EXECUTE.
Nobody granted that. All three defining migrations
(20260325110000_publisher_publish_state, 20260326000400, 20260326001000)
contain ZERO GRANT statements; it is Supabase's stock default privilege on
`public`. Which is exactly why this has to be a migration: the fix was applied
by hand on B850 and a volume reset would silently undo it.
The schema already states the intent, and it is the opposite:
publisher_audit_svc service_role USING true WITH CHECK true
publisher_audit_auth_read authenticated read-only
publisher_audit_anon_deny anon USING false WITH CHECK false
and detections / segments / emotions / studio_board all have RLS enabled with
NO anon policy at all. So anon is denied everywhere it is named -- while
holding EXECUTE on three functions that write studio_board as a superuser.
No consumer breaks. The publisher (services/publisher + services/common/
supabase.py) does direct inserts into those RLS-protected tables, which an
anon-key client cannot do at all; it has to be service_role, and service_role
keeps its explicit grant. authenticated keeps its own. That is what settled
the key-class question the review was blocked on: it was answerable from RLS,
not from the env.
PUBLIC is revoked for the same reason one level up. Every role inherits
PUBLIC, which is how juicefs_meta -- a LOGIN role that pg_hba now admits from
the tailnet (#2702) -- ended up holding superuser-execution rights it was
created specifically not to have.
Idempotent, and asserts its own end state in both directions: nothing
reachable by anon or juicefs_meta, and nothing lost by service_role or
authenticated. Verified by re-applying against the already-hardened B850 DB.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FkwiW3VY1xWmahTAtVioxz
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 796ab890da
ℹ️ 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".
…atabase
Codex, correctly: supabase-bootstrap runs `apply_dir migration
supabase/migrations` and only THEN `apply_dir seed supabase/initdb`, and
00_3_juicefs_meta_role.sql -- which creates juicefs_meta -- is a seed. So on a
fresh database this migration ran before the role existed.
has_function_privilege() raises `role "x" does not exist`. The apply loop
catches a failing file, prints "FAILED (skipped)" and moves on WITHOUT
recording it in pmoves_bootstrap_history -- so the bootstrap looks like it
succeeded and the hardening silently never applied, on precisely the rebuild
path this file exists to secure.
REVOKE has the same problem, so both halves are guarded now:
- the REVOKE only names anon when anon exists (it is created by
scripts/supabase/bootstrap_db.sh, a different entrypoint that is not
ordered against supabase-bootstrap)
- the assertion CONTINUEs past any role that does not exist
- the PUBLIC half stays unconditional, which is what preserves the
guarantee: PUBLIC always exists, and revoking from PUBLIC removes what
juicefs_meta would otherwise inherit whenever it is created
Also replaced the CROSS JOIN LATERAL with a plain FOREACH -- its `r` alias
collided with the outer unnest and raised "column reference r is ambiguous".
Verified on a throwaway postgres:17 with NEITHER role present:
- migration exits 0 and revokes PUBLIC (acl goes to `postgres=X/postgres`,
the `=X/` PUBLIC entry gone)
- negative test: create anon, grant it EXECUTE, and the assertion raises
"revoke incomplete: anon still holds EXECUTE on 1 secdef function(s)"
- re-running the full migration then re-revokes and passes
and against the live already-hardened B850 DB, where it is a no-op.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FkwiW3VY1xWmahTAtVioxz
|
Verified and fixed — you're right, and the failure mode is worse than an abort. Confirmed the ordering at
Tested on a throwaway |
Operator triple from the 2026-08-24 session, part 1 (skill + suit):
- configs/model-suits/MiniMax-M3.yaml: the primary MiniMax suit was
missing entirely — suits stopped at m2.7 while sessions and profiles
moved to MiniMax-M3 in June 2026. Authored with the corrected Token
Plan surface (api.minimax.io, OpenAI-compatible path), the m3 ->
m2.7 (1M long-context) -> m2.1 (efficient) fallback chain, token
plan tiers, and the thinking-model note (reasoning_content: budget
output tokens accordingly).
- .claude/commands/model/{dispatch,list-suits,verify}: model dispatch
is now a skill instead of tribal knowledge. dispatch routes by
reading the suits, the provider cascades, and
MODEL_FABRIC_CONTRACT.md (local-first order is law), covers the
TensorZero in-network port (:3000), harness dispatch via
pmoves.agent.task.v1, and the MiniMax boundaries (case-sensitive
MiniMax-M3; a 401 on the correct path is key custody, not routing).
list-suits cross-checks suits vs profiles vs signatures and flags
drift. verify wraps the provider gates (MiniMax two-piece design).
- submodule_skill_registry.json: model skills registered in the
$domain_tag_skill_map (llm, local-models); validator OK.
Companion node-side work this session (recorded in AGNOTE): #2707
anon/superuser EXECUTE revoke merged+applied both-assertion-verified;
archon rebuilt from the submodule and its upstream bundled-schema
ordering bug (partial index before the ALTER that adds the column it
references) diagnosed and worked around on-DB; archon healthy on
:3737/:8091.
💘 Generated with Crush
Operator triple from the 2026-08-24 session, part 1 (skill + suit):
- configs/model-suits/MiniMax-M3.yaml: the primary MiniMax suit was
missing entirely — suits stopped at m2.7 while sessions and profiles
moved to MiniMax-M3 in June 2026. Authored with the corrected Token
Plan surface (api.minimax.io, OpenAI-compatible path), the m3 ->
m2.7 (1M long-context) -> m2.1 (efficient) fallback chain, token
plan tiers, and the thinking-model note (reasoning_content: budget
output tokens accordingly).
- .claude/commands/model/{dispatch,list-suits,verify}: model dispatch
is now a skill instead of tribal knowledge. dispatch routes by
reading the suits, the provider cascades, and
MODEL_FABRIC_CONTRACT.md (local-first order is law), covers the
TensorZero in-network port (:3000), harness dispatch via
pmoves.agent.task.v1, and the MiniMax boundaries (case-sensitive
MiniMax-M3; a 401 on the correct path is key custody, not routing).
list-suits cross-checks suits vs profiles vs signatures and flags
drift. verify wraps the provider gates (MiniMax two-piece design).
- submodule_skill_registry.json: model skills registered in the
$domain_tag_skill_map (llm, local-models); validator OK.
Companion node-side work this session (recorded in AGNOTE): #2707
anon/superuser EXECUTE revoke merged+applied both-assertion-verified;
archon rebuilt from the submodule and its upstream bundled-schema
ordering bug (partial index before the ALTER that adds the column it
references) diagnosed and worked around on-DB; archon healthy on
:3737/:8091.
💘 Generated with Crush
Co-authored-by: Agent Zero <agent.zero@pmoves.ai>
`test_http_endpoint "Supabase PostgREST" "http://localhost:3010/" "200"` is a bare unauthenticated GET expecting 200. PostgREST runs with PGRST_JWT_SECRET and no anon role, so an unauthenticated request is SUPPOSED to be refused. That assertion could only ever go green while PostgREST was open to anonymous reads. It was not testing that the service works; it was testing that the service is unprotected, and reporting the protected state as a failure. It started "failing" on B850 the moment that node revoked PUBLIC and anon grants (#2707) as part of the JuiceFS hardening lane. The test failed because the system got safer. Now asserts 401 - and a 200 there would be the finding. THE ASSERTION CHANGE ALONE WOULD NOT HAVE WORKED The helper called `curl -sf`. `-f` turns any 4xx into exit 22, and the pass condition is `[ $status -eq 0 ] && [ "$http_code" = "$expected_status" ]` - so a 401 was rejected on the exit code before http_code was ever compared. The helper could not assert any non-2xx status at all. Dropping `-f` from the two helper calls is what makes `expected_status` mean anything outside 2xx. Verified against a live 401, both forms: curl -sf ... -> http_code=401 exit=22 (fails even when expecting 401) curl -s ... -> http_code=401 exit=0 (passes) Transport failures are unaffected: exit 7 (connection refused) and exit 28 (timeout) are curl-level, not HTTP-level, and both branches still fire. Tests expecting 200 are unaffected - the http_code comparison was always the real assertion; `-f` was only ever masking it. DELIBERATELY NOT CHANGED: Archon `test_http_endpoint "Archon" ".../healthz" "200"` also returns 401 on B850. That one is left failing on purpose. A health endpoint requiring authentication is not a defensible posture the way a refusing PostgREST is, so the red is a real finding and should stay visible rather than be normalised into an expectation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ure (#2721) * fix(smoke): the PostgREST check was asserting that PostgREST is insecure `test_http_endpoint "Supabase PostgREST" "http://localhost:3010/" "200"` is a bare unauthenticated GET expecting 200. PostgREST runs with PGRST_JWT_SECRET and no anon role, so an unauthenticated request is SUPPOSED to be refused. That assertion could only ever go green while PostgREST was open to anonymous reads. It was not testing that the service works; it was testing that the service is unprotected, and reporting the protected state as a failure. It started "failing" on B850 the moment that node revoked PUBLIC and anon grants (#2707) as part of the JuiceFS hardening lane. The test failed because the system got safer. Now asserts 401 - and a 200 there would be the finding. THE ASSERTION CHANGE ALONE WOULD NOT HAVE WORKED The helper called `curl -sf`. `-f` turns any 4xx into exit 22, and the pass condition is `[ $status -eq 0 ] && [ "$http_code" = "$expected_status" ]` - so a 401 was rejected on the exit code before http_code was ever compared. The helper could not assert any non-2xx status at all. Dropping `-f` from the two helper calls is what makes `expected_status` mean anything outside 2xx. Verified against a live 401, both forms: curl -sf ... -> http_code=401 exit=22 (fails even when expecting 401) curl -s ... -> http_code=401 exit=0 (passes) Transport failures are unaffected: exit 7 (connection refused) and exit 28 (timeout) are curl-level, not HTTP-level, and both branches still fire. Tests expecting 200 are unaffected - the http_code comparison was always the real assertion; `-f` was only ever masking it. DELIBERATELY NOT CHANGED: Archon `test_http_endpoint "Archon" ".../healthz" "200"` also returns 401 on B850. That one is left failing on purpose. A health endpoint requiring authentication is not a defensible posture the way a refusing PostgREST is, so the red is a real finding and should stay visible rather than be normalised into an expectation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(smoke): accept 200|401 for the PostgREST root — it is deployment-dependent The 401-only expectation rejected the repository's standard deployment: fresh bootstrap sets PGRST_ANON_ROLE=anon (docker-compose.core.yml) with the role granted by supabase/initdb/01_public_init.sql, so root answers 200 with the OpenAPI document; PGRST_JWT_SECRET validates supplied tokens, it does not require them. Nodes that revoke anon (B850) get 401 instead. Both prove PostgREST is up — the smoke asserts liveness, the security posture belongs to auth-alignment/secrets-audit. Also keeps the shell harness consistent with tests/smoke/test_critical_path.py (200). Alternation uses [[ =~ ]] with the pattern from the variable: bash parses case-alternation before expansion, so a "|" via $expected_status would be a literal pipe. Verified live on this node (401 path): 200|401 passes, exact matches still pass, wrong codes still fail. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Closes the
anonquestion the review was parked on. It was answerable from RLS, not from the env — which is why chasingSUPABASE_KEYaround the fleet never settled it.The exposure
claim_/complete_/fail_studio_board_publishareSECURITY DEFINER, owned bysupabase_admin(rolsuper). They execute as a superuser and bypass RLS.PUBLICandanonboth heldEXECUTE.Nobody granted that. All three defining migrations contain zero GRANT statements — it's Supabase's stock default privilege on
public.The schema already said the opposite
And
detections/segments/emotions/studio_board: RLS enabled, zero anon policies.So
anonis explicitly denied everywhere it's named — while holding EXECUTE on three functions that writestudio_boardas a superuser.Why no consumer breaks
This is what settled the key-class question. The publisher (
services/publisher/publisher.py→services/common/supabase.py:20-44) does direct inserts into those RLS-protected tables. An anon-key client cannot do that at all. It must beservice_role— which keeps its explicit grant.authenticatedkeeps its own.Why a migration and not a one-off
I applied it by hand on B850 tonight. A volume reset re-grants it, because the grant is a default privilege, not an authored one. Same class as the other three node-local fixes this week.
PUBLICis revoked for the same reason one level up: every role inherits PUBLIC, which is howjuicefs_meta— a LOGIN role thatpg_hbanow admits from the tailnet (#2702) — ended up holding superuser-execution rights it was created specifically not to have.Verified
Asserts its end state in both directions, so it can't silently over- or under-revoke:
publicreachable byanonorjuicefs_metaservice_roleorauthenticatedIdempotent — re-applied against the already-hardened B850 DB, both assertion blocks pass. Resulting ACL:
(no
=X/PUBLIC entry, noanon=X). All 13 supabase services healthy after.🤖 Generated with Claude Code
https://claude.ai/code/session_01FkwiW3VY1xWmahTAtVioxz