fix(security): close anon + lock authenticated SECURITY DEFINER RPC exposure (ADR-006)#8
Merged
Merged
Conversation
…R-006), part 1/2
Anon (public-internet) surface lockdown. 73 SECURITY DEFINER non-trigger functions were
anon-EXECUTEable via PostgREST with the public anon key, bypassing the FastAPI role gate.
Several privileged with no internal caller check (fn_admin_grant_tokens grants tokens to
anyone; epoch clone/delete; RP grants; get_user_emails_batch leaks all emails).
Per-function classification (backend caller client vs public.py anon surface vs RLS deps
vs pg_proc read/write + auth.uid self-validation):
- Tier A (22): privileged, admin/service_role-only or SQL-internal -> REVOKE
PUBLIC+anon+authenticated; GRANT service_role.
- Tier B (46): legitimate backend caller, NOT reachable from any anon public.py endpoint
-> REVOKE PUBLIC+anon; GRANT authenticated+service_role.
- KEEP (5): 4 RLS helpers (revoking breaks 129 policy refs) + get_bleed_status (the one
genuine public.py anon read).
GRANT mechanics: EXECUTE flowed via a mix of the PUBLIC default grant (`=X`) and direct
role grants; REVOKE FROM anon alone is a no-op when PUBLIC also grants. Fix = REVOKE FROM
PUBLIC+anon(+authenticated) then GRANT back the needed roles; service_role re-granted
explicitly. Array-driven loop = single source of truth; self-verifying DO block (4
invariants) aborts the txn on any hole/breakage.
Fresh-eyes review caught 5 functions first mis-classified as KEEP (fn_compute_cycle_scores,
fn_increment_progress, fn_get_ward_strength, get_broadsheet_source_data,
get_message_reactions) — the matching public.py methods do plain table SELECTs and never
call these RPCs, so leaving anon was a hole. Now in Tier B (anon revoked).
Verified local (psql + idempotent supabase CLI): anon-callable secdef non-trigger 73 -> 5
(exactly KEEP). content_drafts get_user_emails_batch path confirmed service_role via
get_effective_supabase + non-fatal.
PART 2/2 = migration 258 (the authenticated lockdown, immediate continuation): of the
Tier-B writes only 3 self-validate via auth.uid() (fn_purchase_tokens,
fn_update_user_byok_keys, toggle_message_reaction) and stay authenticated; the other ~35
trust ID params with no guard -> switch backend call sites to service_role + REVOKE
authenticated. The 6 Tier-B reads keep authenticated.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…06), part 2/2 Continues dfee580 (part 1/2, migration 257). Migration 257 closed the anon surface and split the survivors into Tier A (service_role-only) and Tier B (anon revoked, authenticated kept). This finishes the lockdown for Tier B. WHY. Of the 46 Tier-B functions, pg_proc inspection (prosrc, not name-match) shows only a minority self-validate the caller. The other 37 are WRITES that trust their ID parameters with no internal guard, so any logged-in user could POST /rest/v1/rpc/<fn> against a foreign epoch / simulation / user — a horizontal privilege escalation that bypasses the FastAPI Depends() role gate (e.g. fn_purchase_feature deducts an arbitrary user's tokens; fn_spend_rp_atomic / fn_join_epoch_atomic mutate foreign epoch state; fn_materialize_shard processes another user's draft). This is the ADR-006 / incident-147 class. WHAT. Migration 258 revokes authenticated (and re-asserts PUBLIC/anon revoked, service_role granted) on the 37 writes, making them service_role-only. Same ACL-shape-agnostic pattern as 257 (REVOKE FROM PUBLIC, anon, authenticated; GRANT service_role) and the same fail-closed self-verifying DO block. 12 backend call sites that reached these writes with the router-injected user-JWT client are switched to the service-role client (get_admin_supabase_client()), so the revoke does not break them. The switch is behaviourally neutral: SECURITY DEFINER already runs the body as the owner and none of the 37 reference auth.uid(), so only the EXECUTE-check role changes; surrounding RLS-enforced table ops stay on the user client. Switched: forge_feature_service (purchase/refund/darkroom), scoring_service (compute_cycle_scores), epoch_participation_service (join_epoch/join_team), lore_service (delete/reorder), cycle_resolution_service (_grant_rp_batch/spend_rp/grant_rp — shared chokepoints, mixed callers), forge_orchestrator_service (materialize_shard). The other ~25 writes were already reached only via admin/scheduler clients (heartbeat, epoch-cycle resolution, mission resolution, cipher redeem) or have no caller at all (fn_deploy_operative_atomic, fn_dissolve_alliance_atomic are defined-but-dead); those are a pure revoke, no code change. KEPT authenticated (verified genuine guards, not name-match): 3 self-validating writes (fn_purchase_tokens, fn_update_user_byok_keys, toggle_message_reaction), 6 reads, and the 6 DRIFT player actions (fn_travel_move/_complete/_abandon/ _run_open, fn_quest_accept/_advance) which each guard auth.uid() <> p_user. RLS helpers + get_bleed_status untouched. DEFERRED (P1 follow-up, not closed here to keep this write-focused): 4 ID-trusting reads (fn_get_wallet_summary, fn_get_ward_strength, fn_user_byok_allowed, fn_user_has_byok_bypass) — a low-severity horizontal info-leak, no privilege/PII. VERIFICATION. Migration applied locally + self-verify passed; independent grant audit confirms the 37 are service_role-only (anon=0, auth=0, srole=37), the 15 keep-auth functions remain authenticated, anon surface unchanged (5), RLS helpers intact. Test seam: new conftest fixture route_secdef_admin points get_admin_supabase_client() at a test's mock (11 unit tests updated — the RPC now lands on the service-role client, not the passed client). 2043 unit + 20 integration tests pass; ruff (incl. --select S security gate) + backend lints clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Systemic prevention for the migration-257/258 exposure class: a new SECURITY DEFINER function left anon-callable would silently reopen the hole. This adds a deterministic guard rather than relying on review. scripts/lint-no-secdef-public-grant.sh runs the post-migration grant audit against the DB and fails if any SECURITY DEFINER, non-trigger function is anon-EXECUTEable outside the allowlist (get_bleed_status + the 4 RLS helpers). Wired into the CI test-backend job, right after migrations are applied via psql (where the DB + psql are available). Validated both ways: passes on the current tree; correctly flags a function dropped from the allowlist. WHY a DB-state check and NOT `ALTER DEFAULT PRIVILEGES` (the obvious first idea): verified 2026-06-15 that `ALTER DEFAULT PRIVILEGES IN SCHEMA public REVOKE EXECUTE ON FUNCTIONS FROM PUBLIC` does NOT stop the exposure on Supabase — a freshly-created function was still anon/authenticated-executable afterward, because pg_default_acl grants EXECUTE *directly* to anon/authenticated on new public functions (for both the `postgres` role and the managed `supabase_admin` role). Revoking those defaults instead is fragile: prod `postgres` is not a superuser and cannot alter supabase_admin's defaults, Supabase platform ops may re-apply them, and it would force an explicit GRANT on every legitimate new RPC. The authoritative signal is the actual grant state after migrations — exactly what this guard checks. Also expands the CLAUDE.md NEVER rule: the service-role call pattern (get_admin_supabase_client for the .rpc(), user client for RLS-enforced table ops), the two narrow authenticated exceptions (self-validating fns + genuine public reads), the CI guard, and the ALTER-DEFAULT-PRIVILEGES caveat. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…evoke # Conflicts: # .github/workflows/ci.yml
…migration 258 fallout) PR #8's Test Backend job hung 54+ min (normal ~5 min) and never failed: there is no per-test or job timeout, so one wedged test runs until GitHub's 6h default. Root-caused to a deterministic cross-event-loop deadlock, reproduced locally with pytest-timeout (thread method) — the worker thread's loop blocks forever in asyncio base_events `_run_once -> selector.select(None)`. Why now: migration 258 (ADR-006 part 2) switched CycleResolutionService's RP-grant chokepoints (`_grant_rp_batch`, `spend_rp`, `grant_rp_single`) to obtain their RPC client from the process-global `get_admin_supabase_client()` cache instead of the caller-supplied client. `test_race_conditions` deliberately drives `resolve_cycle` from two threads, each with its own `asyncio.run` loop, to exercise the DB optimistic lock under real concurrency. The first thread populates the global cache with a client bound to its loop; the second reuses that cached client on a different loop, so its httpx socket is registered on a now-dead loop and the live loop's selector waits forever — exactly the "NOT thread-safe / event-loop affinity" hazard documented in `supabase_admin_cache.py`. (PR #8's earlier run failed fast at 5.6 min rather than hanging because the table-grant step was absent; tests short-circuited on 42501 before reaching this path. With #9's grant step merged in, the test now runs to the hang.) Test-only fix — production is unaffected: uvicorn runs one event loop for the process lifetime, and a repo-wide grep confirms zero `run_in_executor` / `ThreadPoolExecutor` / `threading.Thread` / `asyncio.run(` in non-test backend code, so the global cache is only touched from a single loop in prod. The cache's docstring already mandates that a thread with its own loop "must construct its own client inside the thread's loop." The fix honours that: monkeypatch `cycle_resolution_service.get_admin_supabase_client` to a per-call factory that builds a fresh service-role client on whatever loop is running, so each worker thread gets a loop-local client. Real concurrency and the optimistic-lock assertion are preserved; mirrors the existing `route_secdef_admin` conftest seam. Verification: - test_concurrent_resolve_does_not_double_increment: 0.28s (was 54+ min hang) - Full backend suite: 3430 passed, 11 skipped, 0 failed (55.6s), thread-timeout armed - ruff check: clean Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mleihs
added a commit
that referenced
this pull request
Jun 16, 2026
…ts (P2)
WHY: heartbeat.py + news_scanner.py annotated their endpoints as bare
`-> SuccessResponse:` / `-> PaginatedResponse:` while backend/models/heartbeat.py
and backend/models/news_scanner.py defined matching *Response models that were
never used (dead). CLAUDE.md: "Return type annotation is the single source of
truth ... Response models live in backend/models/<domain>.py."
RISK handled: a return annotation makes FastAPI FILTER the response through the
model -- dropping any FE-read field the model lacks, and 500-ing on any Literal
value / required-field-null it can't accept. So each endpoint was VERIFIED, and
the audit's "just wire them" would have shipped TWO latent 500s:
- entry_type Literal was MISSING 'resonance_mood' (present in the live CHECK
constraint) -> typing list_heartbeat_entries as-is would 500 the user-facing
chronicle feed. Added it to HeartbeatEntryType.
- get_heartbeat_overview's no-heartbeat early return {"last_tick": 0} omitted
the required simulation_id -> 500. Fixed the service to return a complete
overview ({simulation_id, last_tick: 0}; other fields default).
VERIFICATION per endpoint (verify-don't-trust): all Literals checked vs live
CHECK constraints (severity / arc_type / arc-status / bureau response_type+status
/ anchor-status all matched; only entry_type drifted); all list SELECTs confirmed
select("*"); all required model fields confirmed NOT NULL in the live schema; the
two admin dashboards confirmed built key-for-key to their models; all mutation
services confirmed to return full rows (response.data[0]). Frontend consumption
mapped to ensure no read-field is dropped.
TYPED (18 endpoints): heartbeat overview, entries (+public), arcs, bureau
responses (list/create/cancel), attunements (list/set/remove), anchors
(list/create/join/leave), admin dashboard, cascade-rules; news dashboard +
candidates. Candidates keeps its FE-locked {items, meta, recommended_threshold}
shape via a NEW ScanCandidateListResponse envelope -- NOT switched to the
standard paginated() {data, meta}, which would break the admin UI's
.items/.recommended_threshold reads (the audit's proposal there was unsafe).
LEFT UNTYPED + documented: get_daily_briefing (no model), force_tick
(process-summary), news list_adapters (team models adapters as list[dict]),
toggle_adapter/reject_candidate (status dicts), trigger_scan (cycle metrics),
approve_candidate (returns a resonance), update_candidate (dead), scan_log
(no model). paginated()-generic suggestion skipped (static-only; the per-endpoint
annotation already carries the element type for FastAPI).
VERIFICATION: ruff clean; app.openapi() builds (471 paths); 528
route/heartbeat/scanner tests green; representative payloads (incl. resonance_mood
entry, fixed early-return, candidates envelope, arbitrary adapter dicts) validate
through the typed wrappers. No SQL, no migration.
[Task 5 audit remediation -- routers+models #8, P2]
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
Closes the pre-existing SECURITY DEFINER RPC exposure (ADR-006 / incident-147 class): PostgREST exposes every EXECUTE-granted function at
/rest/v1/rpc/<fn>, and a SECURITY DEFINER function runs as its owner — so an anon/authenticated grant lets a caller bypass the FastAPIDepends()role gate. 73 such functions were anon-callable on prod; several were catastrophic (fn_admin_grant_tokenslet anyone with the public anon key grant tokens to any user).Commits
dfee580— 257, anon close. anon-callable SECDEF (non-trigger) 73 → 5 (get_bleed_status+ 4 RLS helpers). 22 Tier-A admin functions → service_role-only; 41 Tier-B → anon-revoked, authenticated kept. ACL-shape-agnostic pattern (REVOKE FROM PUBLIC, anon[, authenticated]thenGRANTback) + fail-closed self-verifying DO block.362d341— 258, authenticated lockdown. Of the Tier-B writes, 37 trust their ID params with no caller guard → revoked fromauthenticated, making them service_role-only (horizontal-priv-esc fix). 12 backend call sites that reached them with the user-JWT client switched to the service-role client (get_admin_supabase_client()) — behaviorally neutral (SECURITY DEFINER already runs as owner; none referenceauth.uid()), surrounding RLS-enforced table ops stay on the user client. Keptauthenticated: 3 self-validating writes + 6 reads + 6 DRIFT player actions (each guardsauth.uid() <> p_user). 11 unit tests updated via a sharedroute_secdef_adminconftest fixture.462e45c— CI guard.scripts/lint-no-secdef-public-grant.shpins the anon surface (post-migration DB-state check in thetest-backendjob; allowlist = the 5 known-safe). CLAUDE.md rule expanded. (ALTER DEFAULT PRIVILEGESwas tested and is ineffective on Supabase —pg_default_aclgrants anon/auth directly — so the CI guard is the prevention.)Verification
--select S) + backend bash lints clean.Deferred (P1, intentionally out of scope)
4 ID-trusting reads (
fn_get_wallet_summary,fn_get_ward_strength,fn_user_byok_allowed,fn_user_has_byok_bypass) — low-severity horizontal info-leak, no privilege/PII.db pushis unusable).🤖 Generated with Claude Code