[johncybersage]

Summary

Fixes #547 by changing PostgreSQL default privileges from fail-open to fail-closed for future objects created by the postgres migration role.

Previously, the public schema granted broad default privileges to anon and authenticated for newly-created tables, sequences, and functions.

This migration removes those implicit client grants and requires future client-facing objects to explicitly declare their intended access.

It also removes PostgreSQL's built-in default PUBLIC EXECUTE privilege for future functions created by postgres.

Security impact

Before:

new database object
→ implicit client privileges
→ developer must explicitly revoke unintended access

After:

new database object
→ no implicit anon/authenticated access
→ developer must explicitly GRANT intended access

This reduces the risk of future migrations accidentally exposing new Data API objects.

Regression protection

Added pgTAP coverage that creates scratch table, sequence, and function objects and verifies effective privileges for:

• anon
• authenticated

The function test specifically protects against PostgreSQL's built-in PUBLIC EXECUTE default.

The test also verifies the postgres owner retains expected table ownership control.

All test objects are created inside a transaction and rolled back.

Scope

This PR intentionally changes only FUTURE default privileges.

Existing object ACLs are not modified.

Specific existing-object fixes such as #538 remain separate.

Compatibility

Existing application tables and RPCs retain their existing ACLs.

Future client-facing objects must explicitly GRANT the privileges required by PostgREST/Supabase Data API.

Validation

List the exact commands actually executed and their real results.

Do not claim tests passed unless they actually passed.

[kshivang]

@claude please review this head against dev. Focus on the default privilege migration and pgTAP validity, particularly has_table_privilege(..., ALL), and identify unrelated Kotlin changes versus already merged #482 and current browser work. #586 is the older duplicate default-privilege proposal; do not modify other PRs.

[claude]

Claude finished @kshivang's task in 6m 53s —— View job

---

Review: PR #592 head vs origin/dev

• Fetch origin/dev and compute the real diff
• Review 20260912000000_secure_default_privileges.sql
• Review pgTAP validity, esp. has_table_privilege(..., ALL)
• Separate the Kotlin changes from fix(secrets): preserve hostnames in the host matching utility #482 / current browser work
• Post findings

I could not run Postgres in this job (no psql/docker, and Bash approvals are restricted here), so the two SQL findings below are read off the PostgreSQL semantics plus this repo's own baseline migration. Both are cheap to confirm with supabase db start && supabase test db, and I give the exact expected error text so you can check it against the real run. I ran no build or test commands.

---

Summary

Three commits are on this branch, and only one of them is about #547:

Commit	Files	Relationship to dev
3384dbd default privileges	2 SQL (new)	genuinely new, but has two blocking defects
c71a087 browser secret domain matching	WebsiteMatchingUtil.kt + its test	revert of already-merged #482
9a92fce native disposal exceptions	BrowserHandleImpl.kt	revert of ~466 lines of current browser work on dev

The PR body's own Validation section is still the unfilled instruction template ("List the exact commands actually executed... Do not claim tests passed unless they actually passed"), which matches what I found: the pgTAP file cannot pass as written, for two independent reasons.

---

1. Blocking - has_table_privilege(..., 'ALL') is not a valid privilege type

supabase/tests/default_privileges_effective_test.sql:43

SELECT ok(
    has_table_privilege('postgres', 'public._scratch_test_table', 'ALL'),
    'postgres retains expected ownership control'
);

has_table_privilege parses its third argument through convert_table_priv_string in acl.c, whose map holds only SELECT, INSERT, UPDATE, DELETE, TRUNCATE, REFERENCES, TRIGGER, MAINTAIN (plus each WITH GRANT OPTION form and the ignored legacy RULE). There is no ALL entry, so this raises:

ERROR:  unrecognized privilege type: "ALL"

That aborts the transaction. In pgTAP that does not surface as one failing assertion - the remaining statements error with current transaction is aborted, finish() never runs, and supabase test db fails the whole file. The 🧪 Database (pgTAP) job at .github/workflows/build.yml:375 runs on every PR, so this is CI-red, not a latent nit.

Corroborating evidence that ALL was never exercised: every other privilege assertion in supabase/tests/ names a single privilege ('EXECUTE', 'SELECT', 'execute'), and supabase/config.toml pins major_version = 17, so there is no older-version escape hatch.

A comma list is not the fix either - convert_any_priv_string ORs the modes and aclmask is called with "any", so 'SELECT,INSERT,UPDATE' answers any of these, which is not "ALL" and would pass for a role holding only SELECT. Since what the assertion actually wants to pin is ownership, use pgTAP's own predicate:

SELECT table_owner_is(
    'public', '_scratch_test_table', 'postgres',
    'postgres retains ownership of a newly created table'
);

While you are in there, two additions worth the plan bump - a service_role pin (the migration deliberately leaves service_role's default ALL in place, and a future over-broad revoke breaking the service key should fail a test rather than production), and the same owner/service_role treatment for the sequence and the function.

Fix this →

---

2. Blocking - the schema-less REVOKE ... FROM PUBLIC is inverted: no effect where it is needed, broad effect where it is not

supabase/migrations/20260912000000_secure_default_privileges.sql:6-8

-- Defeat the built-in PostgreSQL PUBLIC default
-- This must be schema-less to successfully override the global PostgreSQL default.
ALTER DEFAULT PRIVILEGES FOR ROLE "postgres" REVOKE EXECUTE ON FUNCTIONS FROM PUBLIC;

The comment's premise is the thing to check, because the mechanics run the other way. The PostgreSQL docs for ALTER DEFAULT PRIVILEGES state that default privileges specified per-schema are added to whatever the global default privileges are for that object type (get_user_default_acl merges the global and per-schema pg_default_acl rows rather than letting either win).

Now trace this database's actual rows. supabase/migrations/20251023000014_grants.sql:696-699 already created a per-schema row for (postgres, public, FUNCTIONS), and ALTER DEFAULT PRIVILEGES ... GRANT builds that row by applying the grants on top of acldefault() - which for functions includes =X/postgres, the built-in PUBLIC EXECUTE. So:

• Line 4 of this migration revokes from anon and authenticated only, leaving the per-schema row as roughly {postgres=X/postgres, =X/postgres, service_role=X/postgres} - PUBLIC EXECUTE still in it.
• Line 8 creates a global row {postgres=X/postgres} with no PUBLIC.
• For a function created by postgres in public, the two are merged additively, so PUBLIC EXECUTE survives.

Consequences, both bad:

• In public the migration does not do what it says. Since anon and authenticated are members of PUBLIC, a new function created by a future migration in public is still executable by both, which is the precise hole Track: ALTER DEFAULT PRIVILEGES grants anon/authenticated on all later objects (3 recurrence sites) #547 is about. Assertions 5 and 6 of the new pgTAP file are the ones that would catch this - so after you fix finding 1, expect those two to fail. That is a second, independent reason this suite has not been run.
• In every other schema it does something nobody asked for. With no IN SCHEMA, the global row applies to future functions postgres creates in extensions, auth, storage, vault, graphql_public, pgbouncer and anywhere else - including functions created by CREATE EXTENSION and by extension upgrade scripts, which routinely rely on the default PUBLIC EXECUTE. Losing it there is how gen_random_uuid() in a column default or uuid_generate_v4() stops being callable by application roles. supabase/config.toml puts extensions on extra_search_path, so this is a live schema for this project, not a hypothetical.

The targeted form does the right thing on both counts, because it subtracts from the same row the 2025 baseline created:

-- PUBLIC EXECUTE for functions comes from acldefault(), which the 2025 baseline's
-- GRANT statements materialised into this same per-schema row. Subtract it here.
-- A schema-less REVOKE would instead create a global row that this per-schema row is
-- ADDED to - leaving PUBLIC EXECUTE intact in public (the one schema that matters)
-- while stripping it from future functions in extensions/auth/storage/vault.
ALTER DEFAULT PRIVILEGES FOR ROLE "postgres" IN SCHEMA "public" REVOKE EXECUTE ON FUNCTIONS FROM PUBLIC;

Please confirm against a real run rather than on my reading - \ddp before and after, plus assertions 5 and 6, settle it in one command.

Fix this →

---

3. The two Kotlin commits are reverts of dev, not additions to it

This is the part that most needs a decision, because a merge here loses shipped work silently. The branch is cut from main, so both Kotlin commits predate what dev carries and git diff origin/dev..HEAD shows them as deletions.

WebsiteMatchingUtil.kt - dev already has #482, in a strictly stronger form. This PR's version would reintroduce two defects dev closed:

• Vacuous empty-string exact match. dev opens calculateMatchScore with an explicit secretNorm.isEmpty() || domainNorm.isEmpty() -> MatchScore(0.0f, "no_match") guard. This PR has no such guard, so "" == "" satisfies the exact-match branch and returns 1.0f, "exact". The path is reachable: matchSecretsForDomain:103 falls back to secret.website.lowercase() when extractMainDomain returns null, so a secret saved with a blank website scores a full exact match against any page whose domain extraction also fell through to "". dev's test blank sides do not vacuously exact-match each other pins exactly this, and this PR deletes that test.
• Brand spoofing in getDisplayName. dev returns the full domain whenever it has more than one dot, and switches on the whole domain.lowercase(). This PR switches on nameWithoutTld.lowercase() instead, so getDisplayName("google.com.evil.com") takes .split(".").first() = "google" and renders the attacker domain as "Google" in the credential picker. dev's KDoc names this case (google.com.evil.com → google.com.evil.com) as the reason for the guard.

It also replaces dev's 16 named tests, including the matchSecretsForDomain end-to-end cases and the www.-normalization pair, with 4 coarse ones. (The joinToString line unwrap is cosmetic only - .editorconfig sets max_line_length = 140, so ktlint does not care either way.)

BrowserHandleImpl.kt - 371 insertions against 466 deletions relative to dev. dev has a structured native-disposal mechanism that this commit is a hand-rolled precursor of; merging this deletes, among others:

-internal fun isTransportFailure(e: Throwable): Boolean =
-internal fun navigationMainFrameOrNull(
-    private val nativeDisposal =
-    internal suspend fun awaitNativeDisposal() = nativeDisposal.awaitCompletion()
-    private val handleCall = BoundedBrowserCall("boss-browser-call-$id")
-    private val swipeNavGate = SwipeNavGate()
-    private val audioSource = TabAudioSource { SwingUtilities.invokeLater(it) }
-    private val pageInjection =
-    private val ownedExecutors =
-    private suspend fun injectPageHelpers(frame: Frame) {

SwipeNavGate is the two-finger swipe work AGENTS.md documents, and awaitNativeDisposal is the same concern 9a92fce is reaching for, already solved.

Separately, 9a92fce would not pass CI on its own merits: it wraps the ~140-line NavigationFinished body in try { without re-indenting the body, leaving the closing brace at 14 spaces (BrowserHandleImpl.kt:1259 on this head) and the enclosed comment and statements at their old depth, which ./gradlew ktlintCheck rejects on the indent rule. It also flips // Skip injection to // skip injection for no reason, and its five catch (e: ObjectClosedException) blocks never use e (catch (_: ...) is the idiom).

My recommendation is to rebase onto origin/dev keeping only the two SQL files, and let the browser disposal question be raised against dev's nativeDisposal if a gap remains there. Per your instruction I have not touched #586 or any other PR.

Fix this →

---

Smaller notes on the SQL

• plan(7) is correct for the seven assertions as written, and has_sequence_privilege(..., 'USAGE') / has_function_privilege(..., 'EXECUTE') with the public._scratch_test_func() signature form are all valid. The bug is confined to 'ALL'.
• SET ROLE postgres before the CREATEs is the right instinct and worth keeping: the default ACL is keyed on the object owner, so pinning the creating role makes the test independent of whoever supabase test db happens to connect as.
• BEGIN / ROLLBACK around the scratch objects is correct, though the file uses uppercase SQL while every neighbouring suite in supabase/tests/ is lowercase (begin; select plan(33); ... rollback;). Cosmetic.
• Trailing whitespace on the ok( argument lines; .editorconfig sets trim_trailing_whitespace = true for [*].
• The migration has no header comment. Every other migration in this directory explains its reasoning at the top, and the reasoning here is subtle enough to deserve it - in particular that service_role and postgres keep their default ALL on purpose, so "fail closed" means closed to client roles rather than to everything. Referencing Track: ALTER DEFAULT PRIVILEGES grants anon/authenticated on all later objects (3 recurrence sites) #547 and noting that existing object ACLs are untouched (as the PR body says) would land that in the file rather than only in the PR description.
• Residual scope worth stating in that header: the revokes are FOR ROLE "postgres" only, matching the baseline, so anything created by a different owner is not covered.

---

Verdict

The default-privilege direction is right and worth landing. As it stands the PR cannot merge: finding 1 fails the pgTAP job outright, finding 2 means the migration misses its target schema while touching schemas it should not, and finding 3 means merging into dev would silently revert #482 and the current browser work. Finding 3 is the one I would resolve first, since it decides what the diff even is.
| Branch: fix/issue-547-default-privileges

[kshivang]

@johncybersage Consolidating the database implementation into the older #586, which now explicitly credits your future-default migration and carries your SET ROLE postgres scratch-object setup at 8f1a484. Both migrations revoke the same three public-schema client defaults and the global PUBLIC EXECUTE default, at the same migration version 20260912000000. #586 covers every client verb and adds service_role and outside-public regression checks (16/16 focused scratch pgTAP assertions passed).

Your other contributions are already accounted for: c71a087 is an ancestor of current dev through merged #482, which preserves your four author test methods; 9a92fce is exactly the closed #442 browser proposal previously verified as superseded by merged #400's native frame containment and PID/injection cleanup. No distinct implementation is being dropped or taken over from another group.

The original test's has_table_privilege(..., 'ALL') errors with unrecognized privilege type: "ALL" on PostgreSQL 17; the surviving suite uses valid per-verb assertions. The retarget to dev exposed existing Kotlin conflicts; those obsolete changes are not reintroduced. Closing as fully superseded, not merging. Replacement: #586 . Final replacement CI/review is still pending and is not represented as green here.

[kshivang]

Review disposition after consolidation: finding 1 is confirmed on PostgreSQL 17 (unrecognized privilege type: "ALL") and is avoided by #586's per-verb assertions. The obsolete Kotlin changes were not carried over; their original contribution/provenance is recorded above.

Finding 2's claim that the schema default row materializes built-in PUBLIC EXECUTE is incorrect. I reproduced the exact 20251023000014 default grants in a dedicated scratch PostgreSQL 17 database, disabled the public-schema event trigger to avoid masking the result, then applied this PR's four default revokes. Before revocation the schema-level function ACL was {postgres=X/postgres,anon=X/postgres,authenticated=X/postgres,service_role=X/postgres} with NO =X/postgres entry. After creating a new public function: anon EXECUTE=false, authenticated EXECUTE=false, service_role EXECUTE=true. The transaction was rolled back. A per-schema revoke cannot remove built-in global PUBLIC EXECUTE; replacing the global revoke as suggested would reintroduce it. PostgreSQL documents this at https://www.postgresql.org/docs/17/sql-alterdefaultprivileges.html .

The global scope is real and is explicitly documented and tested in surviving #586. No existing function ACL changes from that default statement. The separate handle_new_user ACL change is documented. No live service was migrated.