fix(pg-delta): preserve REVOKE EXECUTE FROM PUBLIC#310
Open
cgaaf wants to merge 2 commits into
Open
Conversation
…outines CREATE FUNCTION grants EXECUTE to PUBLIC by default. An explicit REVOKE EXECUTE ... FROM PUBLIC (represented as the absence of that built-in default) was dropped from generated migrations, leaving the routine PUBLIC-executable after the migration was applied. Adds unit coverage in procedure.diff.test.ts and an end-to-end integration regression (tests/integration/revoke-execute-from-public.test.ts) across PG15/17/18: create-from-empty, alter-existing, planned global default revoke, and the self-contained/declarative plan path. RED at this commit (production unchanged from 9284412): unit (src/core/objects/procedure/procedure.diff.test.ts): 18 pass, 5 fail - create emits PUBLIC EXECUTE revoke when the default is absent in the target - create emits PUBLIC EXECUTE revoke for procedures as well as functions - alter emits PUBLIC EXECUTE revoke when an existing routine removes the built-in default - alter emits PUBLIC EXECUTE grant when an existing routine restores the built-in default - declarative create emits PUBLIC EXECUTE revoke when the target removes the built-in default integration PG17 (tests/integration/revoke-execute-from-public.test.ts): 1 pass, 3 fail - create preserves REVOKE EXECUTE FROM PUBLIC and applies the locked-down behavior - alter emits REVOKE EXECUTE FROM PUBLIC for an existing function - self-contained plan emits only the explicit PUBLIC revoke needed for locked-down functions Failures are the missing REVOKE/GRANT, e.g. "expected revoke length 1, received 0". Refs supabase#308
CREATE FUNCTION grants EXECUTE to PUBLIC by default, and the procedure diff filtered that built-in out of BOTH sides before diffPrivileges ran (filterPublicBuiltInDefaults on the create and alter paths). An explicit REVOKE ... FROM PUBLIC is the absence of that default, so once both sides were filtered the revoke became invisible and was dropped. Compare the real ACLs instead: - existing routines: pass the raw source/target privilege arrays to diffPrivileges, so default<->revoked emits REVOKE/GRANT; - new routines: compare the desired ACL against the ACL the routine has right after creation -- the effective default-privilege state normally (unioning global + schema routine defaults, since per-schema ALTER DEFAULT PRIVILEGES is additive and cannot subtract the global built-in), or a minimal PUBLIC/EXECUTE baseline under skipDefaultPrivilegeSubtraction so self-contained/declarative plans still emit the explicit target ACL. Owner-privilege filtering on ownership transfer is unchanged. Scope is the procedure path (functions and procedures); aggregates and PUBLIC USAGE on types/languages are untouched. GREEN after this commit: unit: 23 pass, 0 fail integration PG15/17/18 (revoke-execute-from-public.test.ts): all pass full suite (bun run test:pg-delta): 2663 pass, 10 skip, 0 fail Fixes supabase#308
🦋 Changeset detectedLatest commit: abf6534 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
cgaaf
marked this pull request as ready for review
June 25, 2026 13:15
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
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 #308
What's wrong
CREATE FUNCTIONgrantsEXECUTEtoPUBLICby default, and PostgreSQL'sSECURITY DEFINERguidance recommends revoking that default and grantingEXECUTEselectively. pg-delta dropped thatREVOKE EXECUTE ... FROM PUBLICfrom generated migrations, so a function the target schema locks down stayed PUBLIC-executable after the migration ran. It showed up two ways:CREATE FUNCTIONwas emitted but theREVOKEwas omitted, leaving the function PUBLIC-executable.Root cause
The raw catalogs preserve the negative state correctly (
procedure.model.tsexpandsproacl IS NULLto the built-inPUBLIC/EXECUTErow; an explicit revoke materializes an ACL without that row). The loss happened in the diff:procedure.diff.tsappliedfilterPublicBuiltInDefaults("procedure", …)to both sides of the create and alter comparisons beforediffPrivilegesran. OncePUBLIC/EXECUTEwas stripped from both sides, an explicitREVOKE … FROM PUBLIC(represented as the absence of that default) became invisible, so no statement was emitted.The fix
Stop filtering the built-in out of both sides; compare the real ACL the routine will have against the desired ACL:
diffPrivileges. Default-on-both cancels; default→revoked emits aREVOKE; revoked→default emits aGRANT.ALTER DEFAULT PRIVILEGESentries are additive to the global routine defaults in PostgreSQL (a per-schema entry can't subtract the global built-inPUBLIC EXECUTE), so global and schema defaults are unioned rather than letting a schema entry mask the global built-in.skipDefaultPrivilegeSubtraction— declarative output intentionally ignores role-configured defaults, but PostgreSQL still creates routines withPUBLIC EXECUTE, so a minimal built-in baseline (PUBLIC/EXECUTE/not grantable) is used as the comparison floor.The change is local to
procedure.diff.ts(plus a small typedPUBLIC_EXECUTE_DEFAULTconstant and a private de-dup helper); no changes tobase.privilege-diff.tsor other object diffs.Scope
pg-delta's
procedurediff path covers both functions and procedures, so the fix applies to both even though the issue's reproducer is a function. Aggregates have a separate diff and are unchanged. The samefilterPublicBuiltInDefaultshelper also strips PUBLICUSAGEfor types and languages; those paths are not touched here — they're unverified and out of scope for #308.Tests
Authored test-first (RED → GREEN); the two commits are split so the
test:commit can be checked out and watched to fail.procedure.diff.test.ts): create→revoke-when-target-absent; create-for-procedures (not just functions); no-redundant-grant when the built-in is kept; global+schema default union; alter revoke; alter restore (grant); create-after-global-ADP-revoke emits nothing redundant; declarative create (both keep-default and revoke-default).tests/integration/revoke-execute-from-public.test.ts, PG15/17/18): create preserves the revoke andhas_function_privilegeconfirms an unprivileged role can't execute on either DB; alter-only revoke; planned global default revoke avoids a redundant per-function revoke; self-contained plan emits only the needed explicit revoke.patch,@supabase/pg-delta) is included.Verification
Reproduces from the CLI with
pgdelta plan --target <url> --format sql(create-from-empty omits--source); also viapgdelta sync/declarative-applyand Supabase'sdb schema declarative sync. Environment: macOS 26.5.1 (Darwin, Apple Silicon), Bun 1.3.14, Docker 29.5.3, PostgreSQL 15/17/18 via Alpine testcontainers.RED — at the
test:commit (production unchanged from base):GREEN — after the
fix:commit:Disclosure