diff --git a/.changeset/preserve-public-execute-revoke.md b/.changeset/preserve-public-execute-revoke.md new file mode 100644 index 000000000..7d618def9 --- /dev/null +++ b/.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. diff --git a/packages/pg-delta/src/core/objects/procedure/procedure.diff.test.ts b/packages/pg-delta/src/core/objects/procedure/procedure.diff.test.ts index f085b0707..f0c9f8b30 100644 --- a/packages/pg-delta/src/core/objects/procedure/procedure.diff.test.ts +++ b/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 = {}): 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 & { + skipDefaultPrivilegeSubtraction?: boolean; + } = {}, +) { + return { + ...testContext, + defaultPrivilegeState: new DefaultPrivilegeState({}), + ...overrides, + }; +} + +function expectPublicRevoke( + changes: ReturnType, + 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, +): 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); + }); }); diff --git a/packages/pg-delta/src/core/objects/procedure/procedure.diff.ts b/packages/pg-delta/src/core/objects/procedure/procedure.diff.ts index ed91ee335..174eec13a 100644 --- a/packages/pg-delta/src/core/objects/procedure/procedure.diff.ts +++ b/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(privileges: T[]): T[] { + const seen = new Set(); + 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, branch: Record, @@ -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, ); diff --git a/packages/pg-delta/tests/integration/revoke-execute-from-public.test.ts b/packages/pg-delta/tests/integration/revoke-execute-from-public.test.ts new file mode 100644 index 000000000..d5645312d --- /dev/null +++ b/packages/pg-delta/tests/integration/revoke-execute-from-public.test.ts @@ -0,0 +1,157 @@ +import { describe, expect, test } from "bun:test"; +import dedent from "dedent"; +import type { Pool } from "pg"; +import { createPlan } from "../../src/core/plan/create.ts"; +import { flattenPlanStatements } from "../../src/core/plan/render.ts"; +import { POSTGRES_VERSIONS } from "../constants.ts"; +import { roundtripFidelityTest } from "../integration/roundtrip.ts"; +import { withDbIsolated } from "../utils.ts"; + +const FUNCTION_SIGNATURE = "public.secret_fn()"; +const CHECK_FUNCTION_BODIES = "SET check_function_bodies = false"; + +function createFunctionSql(name: string): string { + return [ + `CREATE FUNCTION public.${name}()`, + " RETURNS integer", + " LANGUAGE sql", + "AS $function$ SELECT 1 $function$", + ].join("\n"); +} + +async function generatedSql( + main: Pool, + branch: Pool, + options: Parameters[2] = {}, +): Promise { + const planResult = await createPlan(main, branch, options); + return planResult ? flattenPlanStatements(planResult.plan) : []; +} + +async function publicCanExecute(db: Pool): Promise { + const { rows } = await db.query( + `SELECT has_function_privilege('probe_nopriv', '${FUNCTION_SIGNATURE}', 'EXECUTE') AS ok`, + ); + return rows[0].ok === true; +} + +for (const pgVersion of POSTGRES_VERSIONS) { + describe(`revoke execute from PUBLIC (pg${pgVersion})`, () => { + test( + "create preserves REVOKE EXECUTE FROM PUBLIC and applies the locked-down behavior", + withDbIsolated(pgVersion, async (db) => { + await roundtripFidelityTest({ + mainSession: db.main, + branchSession: db.branch, + initialSetup: "CREATE ROLE probe_nopriv NOLOGIN", + testSql: dedent` + CREATE FUNCTION ${FUNCTION_SIGNATURE} + RETURNS int + LANGUAGE sql + AS $$ SELECT 1 $$; + + REVOKE EXECUTE ON FUNCTION ${FUNCTION_SIGNATURE} FROM PUBLIC; + `, + assertSqlStatements: (statements) => { + expect(statements).toEqual([ + CHECK_FUNCTION_BODIES, + createFunctionSql("secret_fn"), + "REVOKE ALL ON FUNCTION public.secret_fn() FROM PUBLIC", + ]); + }, + }); + + expect(await publicCanExecute(db.branch)).toBe(false); + expect(await publicCanExecute(db.main)).toBe(false); + }), + ); + + test( + "alter emits REVOKE EXECUTE FROM PUBLIC for an existing function", + withDbIsolated(pgVersion, async (db) => { + const setup = dedent` + CREATE FUNCTION ${FUNCTION_SIGNATURE} + RETURNS int + LANGUAGE sql + AS $$ SELECT 1 $$; + `; + await db.main.query(setup); + await db.branch.query(setup); + await db.branch.query( + `REVOKE EXECUTE ON FUNCTION ${FUNCTION_SIGNATURE} FROM PUBLIC`, + ); + + const sql = await generatedSql(db.main, db.branch); + + expect(sql).toEqual([ + CHECK_FUNCTION_BODIES, + "REVOKE ALL ON FUNCTION public.secret_fn() FROM PUBLIC", + ]); + }), + ); + + test( + "planned global default revoke prevents redundant per-function revoke on create", + withDbIsolated(pgVersion, async (db) => { + await roundtripFidelityTest({ + mainSession: db.main, + branchSession: db.branch, + testSql: dedent` + ALTER DEFAULT PRIVILEGES + REVOKE EXECUTE ON FUNCTIONS FROM PUBLIC; + + CREATE FUNCTION ${FUNCTION_SIGNATURE} + RETURNS int + LANGUAGE sql + AS $$ SELECT 1 $$; + `, + assertSqlStatements: (statements) => { + expect(statements).toEqual([ + CHECK_FUNCTION_BODIES, + "ALTER DEFAULT PRIVILEGES FOR ROLE postgres REVOKE ALL ON ROUTINES FROM PUBLIC", + createFunctionSql("secret_fn"), + ]); + expect( + statements.filter((statement) => + statement.startsWith("REVOKE ALL ON FUNCTION"), + ), + ).toEqual([]); + }, + }); + }), + ); + + test( + "self-contained plan emits only the explicit PUBLIC revoke needed for locked-down functions", + withDbIsolated(pgVersion, async (db) => { + await db.branch.query(dedent` + CREATE FUNCTION public.open_fn() + RETURNS int + LANGUAGE sql + AS $$ SELECT 1 $$; + + CREATE FUNCTION public.secret_fn() + RETURNS int + LANGUAGE sql + AS $$ SELECT 1 $$; + + REVOKE EXECUTE ON FUNCTION public.secret_fn() FROM PUBLIC; + `); + + const sql = await generatedSql(db.main, db.branch, { + skipDefaultPrivilegeSubtraction: true, + }); + + expect(sql).toEqual([ + CHECK_FUNCTION_BODIES, + createFunctionSql("open_fn"), + createFunctionSql("secret_fn"), + "REVOKE ALL ON FUNCTION public.secret_fn() FROM PUBLIC", + ]); + expect(sql.join("\n")).not.toContain( + "GRANT ALL ON FUNCTION public.open_fn() TO PUBLIC", + ); + }), + ); + }); +}