Repository navigation
fix(security): restrict default public privileges - #586
AdityaK-iiita wants to merge 31 commits into
Conversation
…o real orgs
The BOSS Supabase anon key is compiled into this repo, which is public. Anything
`anon` can execute is therefore executable by anyone. Verified on 2026-09-08:
* get_encryption_key() returned the Vault master encryption key, unauthenticated.
decrypt_text() was anon-callable beside it, making a decryption oracle over
everything encrypt_text protects - user secrets and the QBO/Google broker
credentials.
* find_user_by_email() confirmed any address and returned its uuid.
* list_shareable_recipients() returned 152 users WITH full email addresses to
any self-registered account, and 82 @risalabs.ai addresses on a search.
* arcade_leaderboard() and arcade_bs_standings() published the roster of
everyone who had opened the Arcade, unauthenticated.
None of it was granted on purpose. PostgreSQL hardwires EXECUTE to PUBLIC on
every new function, PUBLIC includes anon, and this project's default privileges
add anon on top - so a SECURITY DEFINER function is internet-callable from the
moment it is created. Both revokes are traps in mirror image: `from public`
leaves the explicit anon grant, `from anon` leaves the hardwired PUBLIC one, and
ALTER DEFAULT PRIVILEGES cannot remove the latter at all (pg_default_acl loses
the entry and new functions still get `=X`).
20260908000000 event trigger: revoke PUBLIC+anon on every function created in
schema public. An explicit `grant ... to anon` after the create
still wins - anon access must be stated, never inherited.
20260908010000 org_visible_users / org_is_vetted / user_display_name: ONE
definition of "may this account learn who that account is",
shared by the Arcade and poker rather than copied. Vetted means
a human approved the join, which excludes the catch-all `boss`
org every account joins on signup (153 members, 20 domains).
20260908020000 route list_shareable_recipients through both. It was already
org-scoped, which is why it looked right; it accepted ANY
shared org.
20260908030000 one-time sweep of the ~30 functions that already had the
inherited grant. The RBAC mutators fail closed
(is_user_admin(NULL) is false), so this is defence in depth,
not a patched escalation.
The rule is a SET, not a per-target predicate: as a per-row qual it is pushed
below a DISTINCT ON and evaluated once per underlying row - 2.7s over 29k score
rows for 45 players, against 37ms.
supabase/audit/identity_disclosure_audit.sql is the standing check, because "we
fixed the leak" is not a durable claim. All four checks report HEALTHY.
Left deliberately anon-callable, each documented where it is kept: the plugin
store browse API, the three helpers called from RLS policies on anon-readable
tables, and custom_access_token_hook. Verified after the sweep that anonymous
plugin-store browse and signed-in RBAC reads both still work.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
get_encryption_key() was anon-callable, so the Vault master_encryption_key was retrievable by anyone holding the project anon key - which ships compiled into this public repo. Revoking access does not un-disclose a key, so it was rotated on 2026-09-09 and the 184 rows encrypted under it re-encrypted. Scope of the original exposure, stated precisely because the capability was worse than the reach: decrypt_text() was anon-callable beside it, so an attacker holding ciphertext from ANY channel - a backup, an export, a screenshot - could decrypt it through the API without even needing the key. But no bulk read path to that ciphertext existed: qbo_token_state and google_token_state have no client grants at all, and all 178 secrets rows have org_id NULL, so the secrets policy reduces to owner-only. This was a serious latent vulnerability - one org-shared secret, one RLS slip, or anyone with backup access would have had plaintext - rather than a confirmed bulk disclosure. The script is kept because rotation is not a one-off. It is atomic (one DO block), self-verifying (md5 of every row's plaintext captured before, re-derived after through decrypt_text so the check covers both the swap and the data), and a mismatch or row-count drift raises and rolls back, so a broken rotation cannot commit. Verified after the run: the live key differs from the disclosed one, 184/184 rows decrypt under it, and the disclosed key opens 0 rows. The old key is retained in the Vault under an explicit name. Do NOT delete it while pre-rotation backups exist - they are encrypted under it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…po/prod drift Addresses the handoff's database-integration item and the substantiated findings from the current-head review. Everything below was executed against the PR's own Supabase preview branch, not asserted from reading. **The blocker: rotation refused to run once risa-labs-inc#417 landed.** The guard was a blanket "TOTP is installed, refuse everything". It is now an adapter. The stored form is 'v1:' || encrypt_text(...) - same cipher, same key, only the framing differs - so the column map gained an envelope prefix and rotates like the rest once the prefix is stripped and re-applied. Three things this needed that reading the diff would not have shown: * risa-labs-inc#417's trigger RAISES on a v1: value ('TOTP input must be plaintext, not a storage envelope'), so the re-encryption update is rejected outright. The reviewed trigger is now disabled for the update and restored after, inside the same transaction. * safe_decrypt_recovery_codes returns jsonb, not text, so verification through the real read path needed a cast. Both fingerprints now go through each column's OWN application read path rather than decrypt_text, which also removes the step-1/step-5 asymmetry raised as U5. * a step 0 pre-check reports rows that are already unreadable, instead of letting a safe_decrypt_* NULL surface at the end as "a row was missed". The rejection contract is preserved, not removed: an unknown envelope version, an unmapped safe_decrypt_* wrapper, or an unreviewed BEFORE trigger on a mapped table each still refuse. All three are now regression-tested, along with the recovery-code column that was previously never exercised (its fixture was vacuous - create_secret writes no secret_metadata row without p_twofa_enabled). Verified: three consecutive rotations, TOTP/recovery/password all intact, v1 envelope preserved, trigger re-enabled and still enforcing its own contract. **CREATE EXTENSION was impossible.** The fail-closed guard aborted the first routine of any extension installed into public: `create extension pgtap` failed with "Anonymous EXECUTE remains on public.pg_version()". A guard that forces operators to disable it in order to install an extension is a guard that ends up disabled. Extension-owned routines are now exempted with a warning, and the exposure stays visible as advisory CHECK 1x rather than vanishing - it reports pgtap (1079) on the preview. The sweep skips them too: pgcrypto-style helpers are called from column DEFAULTs and CHECK constraints, which are evaluated with the DML role's privileges. No extension owns a function in public on a clean database, so no deployed behaviour changes. **The repo was weaker than production.** find_user_by_email, get_session_status and update_api_key_last_used were locked down on the live project on 2026-09-08 but never captured as a migration, so a fresh deploy re-opened them. find_user_by_email is a user-enumeration oracle over auth.users and signup is open. The standing audit caught this itself on the preview - CHECK 2 named it - which is the drift that check exists for. Both remaining callers use SUPABASE_SERVICE_ROLE_KEY, verified on this branch, so service_role keeps EXECUTE and neither path changes. Also: the guard no longer hard-fails on a database without the Supabase roles (a plain restore), and the vault fixtures no longer assume the key is absent (U3) - both suites failed outright on any database that already had one. Retained-key assertions measure this run's delta instead of an absolute count. Suites on the preview: rotation 16/16, db access audit, and visibility 54/54 all clean; the standing audit reports no failing gated check. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The keep-list assertions only checked has_function_privilege for the seven allowlisted signatures. That proves the grant, not the path: an RLS policy expression is evaluated as the QUERYING role, so revoking a policy helper turns an anonymous SELECT into 'permission denied for function ...' rather than an empty result, and no privilege assertion can see it. The seeding is the part that matters. A policy expression runs PER ROW, so on an empty table it is never evaluated - and plugins and user_roles are both empty on a fresh database, which made the first version of this probe pass no matter what had been revoked. Caught by trying to make it fail. Sensitivity is stated from measurement, not assumption: with a row present, revoking can_view_plugin_row from anon does fail the probe and revoking authorize does not, because permissive policies are ORed and short-circuit. A helper in a policy that never has to be evaluated cannot be detected this way by any test, so the comment says so rather than implying broader coverage. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both halves of this are the same failure: a suite that is green while proving nothing. I hit that twice writing the previous commits - a recovery-code fixture over a secret_metadata row create_secret never wrote, and an anonymous-read probe over an empty table whose RLS policy therefore never evaluated - so these are not hypothetical. Finding 6. The four CI-GATED checks (1, 1b, 4, 5) only ever asserted HEALTHY, while every advisory check already had a fixture-based detection test. An inverted `not exists`, a wrong role literal or a typo in the evtenabled test would have left the gate permanently green. Each is now broken deliberately and asserted to report it: a routine granted to anon AFTER creation (which is how one gets past the event trigger), a revoked deliberate anonymous grant, a disabled event trigger, and a crypto routine exposed to authenticated. They have teeth by construction - the assertion is that the finding appears, so a broken audit query fails them. Finding 8. The generator gained --check, which regenerates in memory and compares without writing, and orphan removal for a generated suite whose .sql.in has been renamed or deleted - that file otherwise keeps being executed forever with stale content. Orphans are identified by a provenance banner rather than a hard-coded list, so a hand-written suite is never touched; verified that explicit_anon_and_org_visibility_test.sql is untouched. Each guard was proven by making it fire: a hand edit and a planted orphan are both reported by --check, and generate removes the orphan. CI now runs generate and --check as their own step, so a preparation failure is visible in the log instead of being attributed to the test run. Preview: rotation 16/16, db access audit 15/15, visibility 56/56. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…anon-grants-and-org-visibility Stop anon reaching schema public by inheritance; one org-visibility rule
|
@claude please review the current head against current dev, including the merged #423 event-trigger changes. Check default ACL correctness, interaction with that guard, service_role compatibility and meaningful regression coverage. #592 duplicates the default-privilege migration; #596 is a separate existing-object audit. |
|
Claude finished @kshivang's task in 8m 37s —— View job Review: default-privilege restriction vs current
|
| # | lines | guards this PR? | why not |
|---|---|---|---|
| 1 | 22-33 | ✗ | dev 20260908000000 already revokes anon on tables |
| 2 | 35-46 | ✓ | |
| 3 | 49-66 | ✗ | dev already revokes anon on sequences |
| 4 | 68-85 | ✓ | |
| 5 | 88-95 | ✗ | #423's event trigger revokes public, anon from the created function |
| 6 | 97-104 | ✓ (conflated) | passes via either the authenticated revoke or the global PUBLIC revoke |
| 7-9 | 107-132 | vacuous | postgres owns these objects; has_*_privilege is true regardless of any default ACL |
| 10 | 135-142 | ✗ | dev 20260908030000 sweep already revoked anon |
| 11 | 144-151 | ✓ |
plan(11) matches the 11 ok() calls, and supabase test db (build.yml:407) auto-discovers the file, so it does run in CI. Suggested additions:
service_rolepositives —has_table_privilege('service_role', …, 'SELECT'),has_sequence_privilege(… ,'USAGE'),has_function_privilege(…, 'EXECUTE'). This is the assertion that catches the over-revoke. Fix this →- A direct PUBLIC assertion, so security: stop DigiCert signing secrets leaking into public CI logs #6 stops conflating two mechanisms:
SELECT ok(NOT EXISTS ( SELECT 1 FROM pg_proc p JOIN pg_namespace n ON n.oid = p.pronamespace CROSS JOIN LATERAL aclexplode(coalesce(p.proacl, acldefault('f', p.proowner))) a WHERE n.nspname = 'public' AND p.proname = 'default_privilege_guard_function' AND a.grantee = 0 AND a.privilege_type = 'EXECUTE' ), 'PUBLIC has no EXECUTE on a newly created public function');
- Assert
pg_default_aclitself, not only a derived object — default privileges are a database-wide setting changeable from the dashboard outside migrations, and (per the table above) the object-based anon checks would pass with the event trigger doing all the work. Fix this → - If §2 is kept, a test that pins its blast radius — create a scratch schema,
CREATE FUNCTIONin it, assert what PUBLIC gets — so the cross-schema tradeoff is reviewable instead of implicit. - Drop or replace 7-9 and lower
plan(). - Mutation-verify, per this repo's convention: re-add each revoked default grant one at a time and confirm a named test fails. Three of the eleven currently will not.
7. 🟡 handle_new_user() — good catch, wrong stated cause, and one questionable grant
This closes a real gap, and specifically one dev's standing audit cannot see: CHECK 5x filters p.prorettype not in ('trigger'::regtype, 'event_trigger'::regtype), so a client grant on a trigger function never appears in the advisory inventory. Three notes:
- The comment at lines 31-33 is inaccurate. It was exposed by explicit grants —
20251023000014_grants.sql:468-470GRANT ALL ON FUNCTION "public"."handle_new_user"() TO "anon"/"authenticated"/"service_role"— not by default privileges. That also makes this hunk an existing-object fix, i.e. test(db): gate the two client-grant shapes that produced #547 three times #596's scope rather than this migration's; fine to keep, just label it. GRANT EXECUTE … TO service_role(lines 38-39) looks unnecessary and sits oddly under a comment saying clients must not invoke it directly —service_roleis a client credential. EXECUTE on a trigger function is checked atCREATE TRIGGERtime, not when the trigger fires, and theauth.usersinsert runs assupabase_auth_admin; dev already revokedPUBLIC, anonfrom it with signup still working, which is decent evidence there's no runtime check. Impact is ~nil either way (a direct call errors with "trigger functions can only be called as triggers"), but the line wants a reason or removal. Fix this →- Add it to the audit's
CHECK 5allowlist so it's standing-verified rather than only covered by a transaction-local pgTAP test. Fix this →
Summary
The direction is right and the authenticated half is a genuine, uncovered hole that #423 deliberately left open, in a place its own audit is blind to. Two things to settle before merge: drop (or document) the unqualified global PUBLIC revoke, which #423 rejected by name and which can break writes on the next extension upgrade, and rebase onto dev so the three duplicate statements and the policy inversion in §4 are visible in the PR's own diff. Then add the service_role and pg_default_acl assertions — as written, the suite's anon and owner assertions pass with this migration reverted.
Not verified: no supabase CLI here and container startup was not permitted, so I could not run supabase test db or confirm the ACL merge behaviour empirically. §2/§3 rest on PostgreSQL's get_user_default_acl/aclmerge semantics and agree with what dev's own migration comments assert. Worth a supabase db reset && supabase test db on a disposable local DB, plus a \ddp before/after and an ALTER EXTENSION pgcrypto UPDATE smoke test, before merging.
· branch fix/547-default-privileges
|
Addressed at f045fcb, following current-dev integration and 8f1a484. The global revoke is removed: #423 already strips PUBLIC inside public, and we retain its explicit extension/outside-schema compatibility boundary. The final suite has 17 assertions: service_role positives, direct default-ACL checks and outside-public compatibility probes in addition to the original tests. All pass in a dedicated scratch PG17 fixture with real baseline defaults and current-dev guard. Mutation checks: regranting authenticated defaults fails table assertion 2, sequence 4, routine 6 plus catalog assertion 15; a global PUBLIC revoke fails compatibility assertions 16/17. The authenticated default change is the authorized #547 scope; final description now states the current-dev delta and future explicit-GRANT requirement. The handle_new_user comment names its actual explicit grants and explains that service_role's existing grant is retained. The database-specific test already standing-verifies this ACL each CI run; expanding the separate identity audit is not needed here. Original owner assertions remain harmless compatibility checks, not evidence for the revoke. Base is now current dev via a normal merge, preserving author history. No live migration. The review's no-merge-base statement came from its checkout, not the full local repository: normal origin/dev integration succeeded without conflicts. No author defect attributed to that. Final-head CI/root quality remain pending. No unresolved substantive review finding; no repeated review requested. |
|
@AdityaK-iiita Your authenticated-default revocation closes the remaining future-object gap in #547. The consolidated PR also credits @johncybersage and preserves your original commits. All substantive Claude findings have been addressed or rejected with evidence in the preceding review response. Validated repairs are pushed at The remaining blocker is validation infrastructure, not an identified author defect: the current-head build https://github.com/risa-labs-inc/BossConsole/actions/runs/34736793815 remains queued before version-check/shell steps have started. Required full database, code-quality, platform builds and compatibility checks therefore have no current-head passing result. Local root ktlintCheck/detekt are also waiting on the shared build lock. Earlier-head green checks cannot substitute for this head. Under the updated bounded-review policy I am marking this PR draft rather than bypassing checks or polling indefinitely. Maintainer next step: allow queued CI to execute (or restore runner capacity), inspect any actual failures, and require current-head database/ktlint/detekt/platform/compatibility checks to pass. Then recheck current dev integration and mark ready before a match-head squash merge into dev. If an actual code failure appears, please address that specific evidence; no speculative author repair is requested now. Contributor credit and all validated work are preserved. |
75ebc71 to
628f152
Compare
924c565 to
7e62348
Compare
072a2bd to
2ae2565
Compare
New public tables, sequences and routines currently inherit authenticated client privileges. This migration requires explicit client grants for future objects created by postgres, while retaining existing application ACLs and service_role defaults. PUBLIC EXECUTE inside public is removed by the existing #423 event trigger; global defaults outside public remain intact. The migration also explicitly restricts the internal handle_new_user trigger function.
The current dev #423 guard already handles anonymous routine access and anon table/sequence defaults. This change closes the remaining authenticated default grants while retaining #423's deliberate schema scope. It is not a retroactive audit or repair of every existing object. #536/#538 remain distinct existing-object fixes, and #596 adds a separate structural audit. Future client-facing migrations must explicitly GRANT intended privileges after CREATE/CREATE OR REPLACE.
Consolidation and contributors:
#592's inherited hostname commit c71a087 is already an ancestor of dev through merged #482, which retains its author tests. Its inherited browser commit is the exact closed #442 proposal, previously verified as superseded by merged #400. Neither is reintroduced here.
Validation: 17/17 pgTAP assertions pass on a dedicated scratch PostgreSQL 17 database using the real legacy defaults, the migration, and current dev's routine guard. Regranting authenticated table/sequence/function defaults fails assertions 2/4/6 respectively and catalog assertion 15. Reintroducing the global revoke fails compatibility assertions 16/17. This is a focused replica, not a claim of a full clean Supabase reset. Original head had all nine CI jobs green; final-head CI and shared-lock root ktlintCheck/detekt are pending. One completed Claude review addressed at f045fcb; no repeated review requests.
No live migration has been applied. Manual staging checks still needed: existing signup and intended RPCs continue to work; fresh ungranted objects refuse client access and explicitly granted objects work. The migration predates dev's latest timestamp, so deployment must include this pending migration according to the normal backfill process.
Addresses #547's future-default portion; it does not close the remaining audit scope.