fix(pg-delta): preserve REVOKE EXECUTE FROM PUBLIC

.changeset/preserve-public-execute-revoke.md

@@ -0,0 +1,5 @@
+---
+"@supabase/pg-delta": patch
+---
+
+Preserve explicit `REVOKE EXECUTE FROM PUBLIC` statements when diffing functions and procedures.

packages/pg-delta/src/core/objects/procedure/procedure.diff.test.ts

@@ -12,6 +12,10 @@ import {
 import { CreateCommentOnProcedure } from "./changes/procedure.comment.ts";
 import { CreateProcedure } from "./changes/procedure.create.ts";
 import { DropProcedure } from "./changes/procedure.drop.ts";
+import {
+  GrantProcedurePrivileges,
+  RevokeProcedurePrivileges,
+} from "./changes/procedure.privilege.ts";
 import { diffProcedures } from "./procedure.diff.ts";
 import { Procedure, type ProcedureProps } from "./procedure.model.ts";
 
@@ -55,6 +59,88 @@ const testContext = {
   mainRoles: {},
 };
 
+function procedure(overrides: Partial<ProcedureProps> = {}): Procedure {
+  return new Procedure({
+    ...base,
+    owner: "postgres",
+    privileges: [],
+    ...overrides,
+  });
+}
+
+function publicExecutePrivilege() {
+  return [{ grantee: "PUBLIC", privilege: "EXECUTE", grantable: false }];
+}
+
+function defaultPrivilegesWithPublicExecute(): DefaultPrivilegeState {
+  const defaultPrivilegeState = new DefaultPrivilegeState({});
+  defaultPrivilegeState.applyGrant("postgres", "f", null, "PUBLIC", [
+    { privilege: "EXECUTE", grantable: false },
+  ]);
+  return defaultPrivilegeState;
+}
+
+function defaultPrivilegesWithSchemaRoutineGrant(): DefaultPrivilegeState {
+  const defaultPrivilegeState = defaultPrivilegesWithPublicExecute();
+  defaultPrivilegeState.applyGrant("postgres", "f", "public", "anon", [
+    { privilege: "EXECUTE", grantable: false },
+  ]);
+  return defaultPrivilegeState;
+}
+
+function contextWith(
+  overrides: Partial<typeof testContext> & {
+    skipDefaultPrivilegeSubtraction?: boolean;
+  } = {},
+) {
+  return {
+    ...testContext,
+    defaultPrivilegeState: new DefaultPrivilegeState({}),
+    ...overrides,
+  };
+}
+
+function expectPublicRevoke(
+  changes: ReturnType<typeof diffProcedures>,
+  expectedSql = "REVOKE ALL ON FUNCTION public.fn1() FROM PUBLIC",
+): RevokeProcedurePrivileges {
+  const revokes = changes.filter(
+    (change) => change instanceof RevokeProcedurePrivileges,
+  ) as RevokeProcedurePrivileges[];
+
+  expect(revokes).toHaveLength(1);
+  expect(revokes[0].grantee).toBe("PUBLIC");
+  expect(
+    revokes[0].privileges.map(({ privilege, grantable }) => ({
+      privilege,
+      grantable,
+    })),
+  ).toEqual([{ privilege: "EXECUTE", grantable: false }]);
+  expect(revokes[0].serialize()).toBe(expectedSql);
+  return revokes[0];
+}
+
+function expectPublicGrant(
+  changes: ReturnType<typeof diffProcedures>,
+): GrantProcedurePrivileges {
+  const grants = changes.filter(
+    (change) => change instanceof GrantProcedurePrivileges,
+  ) as GrantProcedurePrivileges[];
+
+  expect(grants).toHaveLength(1);
+  expect(grants[0].grantee).toBe("PUBLIC");
+  expect(
+    grants[0].privileges.map(({ privilege, grantable }) => ({
+      privilege,
+      grantable,
+    })),
+  ).toEqual([{ privilege: "EXECUTE", grantable: false }]);
+  expect(grants[0].serialize()).toBe(
+    "GRANT ALL ON FUNCTION public.fn1() TO PUBLIC",
+  );
+  return grants[0];
+}
+
 describe.concurrent("procedure.diff", () => {
   test("create and drop", () => {
     const p = new Procedure(base);
@@ -281,4 +367,140 @@ describe.concurrent("procedure.diff", () => {
       changes.some((change) => change instanceof CreateCommentOnProcedure),
     ).toBe(true);
   });
+
+  test("create emits PUBLIC EXECUTE revoke when the default is absent in the target", () => {
+    const p = procedure();
+    const changes = diffProcedures(
+      contextWith({
+        defaultPrivilegeState: defaultPrivilegesWithPublicExecute(),
+      }),
+      {},
+      { [p.stableId]: p },
+    );
+
+    expect(changes[0]).toBeInstanceOf(CreateProcedure);
+    expectPublicRevoke(changes);
+  });
+
+  test("create emits PUBLIC EXECUTE revoke for procedures as well as functions", () => {
+    const p = procedure({
+      name: "proc1",
+      kind: "p",
+      return_type: "void",
+      definition:
+        "CREATE PROCEDURE public.proc1() LANGUAGE sql AS $$ SELECT 1 $$",
+    });
+    const changes = diffProcedures(
+      contextWith({
+        defaultPrivilegeState: defaultPrivilegesWithPublicExecute(),
+      }),
+      {},
+      { [p.stableId]: p },
+    );
+
+    expect(changes[0]).toBeInstanceOf(CreateProcedure);
+    expectPublicRevoke(
+      changes,
+      "REVOKE ALL ON PROCEDURE public.proc1() FROM PUBLIC",
+    );
+  });
+
+  test("create does not emit PUBLIC EXECUTE grant when the target keeps the built-in default", () => {
+    const p = procedure({ privileges: publicExecutePrivilege() });
+    const changes = diffProcedures(
+      contextWith({
+        defaultPrivilegeState: defaultPrivilegesWithPublicExecute(),
+      }),
+      {},
+      { [p.stableId]: p },
+    );
+
+    expect(changes).toHaveLength(1);
+    expect(changes[0]).toBeInstanceOf(CreateProcedure);
+  });
+
+  test("create combines global routine defaults with schema-specific routine defaults", () => {
+    const p = procedure({ privileges: publicExecutePrivilege() });
+    const changes = diffProcedures(
+      contextWith({
+        defaultPrivilegeState: defaultPrivilegesWithSchemaRoutineGrant(),
+      }),
+      {},
+      { [p.stableId]: p },
+    );
+
+    const revokes = changes.filter(
+      (change) => change instanceof RevokeProcedurePrivileges,
+    ) as RevokeProcedurePrivileges[];
+
+    expect(changes[0]).toBeInstanceOf(CreateProcedure);
+    expect(
+      changes.some((change) => change instanceof GrantProcedurePrivileges),
+    ).toBe(false);
+    expect(revokes).toHaveLength(1);
+    expect(revokes[0].grantee).toBe("anon");
+    expect(revokes[0].serialize()).toBe(
+      "REVOKE ALL ON FUNCTION public.fn1() FROM anon",
+    );
+  });
+
+  test("alter emits PUBLIC EXECUTE revoke when an existing routine removes the built-in default", () => {
+    const main = procedure({ privileges: publicExecutePrivilege() });
+    const branch = procedure();
+    const changes = diffProcedures(
+      contextWith(),
+      { [main.stableId]: main },
+      { [branch.stableId]: branch },
+    );
+
+    expectPublicRevoke(changes);
+  });
+
+  test("alter emits PUBLIC EXECUTE grant when an existing routine restores the built-in default", () => {
+    const main = procedure();
+    const branch = procedure({ privileges: publicExecutePrivilege() });
+    const changes = diffProcedures(
+      contextWith(),
+      { [main.stableId]: main },
+      { [branch.stableId]: branch },
+    );
+
+    expectPublicGrant(changes);
+  });
+
+  test("create after a global default privilege revoke does not emit a redundant routine revoke", () => {
+    const p = procedure();
+    const changes = diffProcedures(
+      contextWith({ defaultPrivilegeState: new DefaultPrivilegeState({}) }),
+      {},
+      { [p.stableId]: p },
+    );
+
+    expect(changes).toHaveLength(1);
+    expect(changes[0]).toBeInstanceOf(CreateProcedure);
+  });
+
+  test("declarative create does not emit PUBLIC EXECUTE grant when the target keeps the built-in default", () => {
+    const p = procedure({ privileges: publicExecutePrivilege() });
+    const changes = diffProcedures(
+      contextWith({ skipDefaultPrivilegeSubtraction: true }),
+      {},
+      { [p.stableId]: p },
+    );
+
+    expect(changes).toHaveLength(1);
+    expect(changes[0]).toBeInstanceOf(CreateProcedure);
+  });
+
+  test("declarative create emits PUBLIC EXECUTE revoke when the target removes the built-in default", () => {
+    const p = procedure();
+    const changes = diffProcedures(
+      contextWith({ skipDefaultPrivilegeSubtraction: true }),
+      {},
+      { [p.stableId]: p },
+    );
+
+    expect(changes[0]).toBeInstanceOf(CreateProcedure);
+    expectPublicRevoke(changes);
+  });
 });

packages/pg-delta/src/core/objects/procedure/procedure.diff.ts

@@ -2,8 +2,8 @@ import { diffObjects } from "../base.diff.ts";
 import {
   diffPrivileges,
   emitObjectPrivilegeChanges,
-  filterPublicBuiltInDefaults,
 } from "../base.privilege-diff.ts";
+import type { PrivilegeProps } from "../base.privilege-diff.ts";
 import type { ObjectDiffContext } from "../diff-context.ts";
 import { diffSecurityLabels } from "../security-label.types.ts";
 import { deepEqual, hasNonAlterableChanges } from "../utils.ts";
@@ -34,6 +34,32 @@ import {
 import type { ProcedureChange } from "./changes/procedure.types.ts";
 import type { Procedure } from "./procedure.model.ts";
 
+const PUBLIC_EXECUTE_DEFAULT: PrivilegeProps = {
+  grantee: "PUBLIC",
+  privilege: "EXECUTE",
+  grantable: false,
+  columns: null,
+};
+
+function privilegeKey(privilege: PrivilegeProps): string {
+  return [
+    privilege.grantee,
+    privilege.privilege,
+    String(privilege.grantable),
+    [...(privilege.columns ?? [])].sort().join(","),
+  ].join(":");
+}
+
+function deduplicatePrivileges<T extends PrivilegeProps>(privileges: T[]): T[] {
+  const seen = new Set<string>();
+  return privileges.filter((privilege) => {
+    const key = privilegeKey(privilege);
+    if (seen.has(key)) return false;
+    seen.add(key);
+    return true;
+  });
+}
+
 /**
  * Diff two sets of procedures from main and branch catalogs.
  *
@@ -45,7 +71,10 @@ import type { Procedure } from "./procedure.model.ts";
 export function diffProcedures(
   ctx: Pick<
     ObjectDiffContext,
-    "version" | "currentUser" | "defaultPrivilegeState"
+    | "version"
+    | "currentUser"
+    | "defaultPrivilegeState"
+    | "skipDefaultPrivilegeSubtraction"
   >,
   main: Record<string, Procedure>,
   branch: Record<string, Procedure>,
@@ -85,28 +114,45 @@ export function diffProcedures(
     // so objects are created with the default privileges state in effect.
     // We compare default privileges against desired privileges to generate REVOKE/GRANT statements
     // needed to reach the final desired state.
-    const effectiveDefaults = ctx.defaultPrivilegeState.getEffectiveDefaults(
-      ctx.currentUser,
-      "procedure",
-      proc.schema ?? "",
-    );
+    // Declarative/self-contained output intentionally ignores role-configured
+    // defaults, but PostgreSQL still creates routines with PUBLIC EXECUTE.
+    const effectiveDefaults = (() => {
+      if (ctx.skipDefaultPrivilegeSubtraction) {
+        return [PUBLIC_EXECUTE_DEFAULT];
+      }
+
+      const schema = proc.schema ?? "";
+      // getEffectiveDefaults returns a schema's own ALTER DEFAULT PRIVILEGES
+      // entries OR, as a fallback, the global ones -- never both. In PostgreSQL
+      // per-schema entries are additive to the global routine defaults and can
+      // only add privileges, never subtract ones granted globally (including the
+      // built-in PUBLIC EXECUTE), so fetch the global defaults separately and
+      // union them in rather than letting a schema-specific entry mask them.
+      const schemaDefaults = ctx.defaultPrivilegeState.getEffectiveDefaults(
+        ctx.currentUser,
+        "procedure",
+        schema,
+      );
+      const globalDefaults = schema
+        ? ctx.defaultPrivilegeState.getEffectiveDefaults(
+            ctx.currentUser,
+            "procedure",
+            "",
+          )
+        : [];
+
+      return deduplicatePrivileges([...globalDefaults, ...schemaDefaults]);
+    })();
     const creatorFilteredDefaults =
       proc.owner !== ctx.currentUser
         ? effectiveDefaults.filter((p) => p.grantee !== ctx.currentUser)
         : effectiveDefaults;
-    // Filter out PUBLIC's built-in default EXECUTE privilege (PostgreSQL grants it automatically)
-    // Reference: https://www.postgresql.org/docs/17/ddl-priv.html Table 5.2
-    // This prevents generating unnecessary "GRANT EXECUTE TO PUBLIC" statements
-    const desiredPrivileges = filterPublicBuiltInDefaults(
-      "procedure",
-      proc.privileges,
-    );
     // Filter out owner privileges - owner always has ALL privileges implicitly
     // and shouldn't be compared. Note: we use the final owner (proc.owner), not the
     // current user, because ownership change happens before privilege diffing.
     const privilegeResults = diffPrivileges(
-      filterPublicBuiltInDefaults("procedure", creatorFilteredDefaults),
-      desiredPrivileges,
+      creatorFilteredDefaults,
+      proc.privileges,
       proc.owner,
     );
 
@@ -364,22 +410,11 @@ export function diffProcedures(
       // a name change would be handled as drop + create by diffObjects()
 
       // PRIVILEGES
-      // Filter out PUBLIC's built-in default EXECUTE privilege from main catalog
-      // (PostgreSQL grants it automatically, so we shouldn't compare it)
-      const mainPrivilegesFiltered = filterPublicBuiltInDefaults(
-        "procedure",
-        mainProcedure.privileges,
-      );
-      // Filter out PUBLIC's built-in default EXECUTE privilege from branch catalog
-      const branchPrivilegesFiltered = filterPublicBuiltInDefaults(
-        "procedure",
-        branchProcedure.privileges,
-      );
       // Filter out owner privileges - owner always has ALL privileges implicitly
       // and shouldn't be compared. Use branch owner as the reference.
       const privilegeResults = diffPrivileges(
-        mainPrivilegesFiltered,
-        branchPrivilegesFiltered,
+        mainProcedure.privileges,
+        branchProcedure.privileges,
         branchProcedure.owner,
       );