diff --git a/cli/src/commands/app/app.ts b/cli/src/commands/app/app.ts index f50027201a..deafaafe8b 100644 --- a/cli/src/commands/app/app.ts +++ b/cli/src/commands/app/app.ts @@ -144,31 +144,59 @@ export function executionModeFromAppFile( if (app?.["guests"] ?? isExecutionModeGuest(app)) { return "guest"; } - // `public`/`guests` are the only modes the file records, so "neither" covers - // both publisher and viewer: keep the deployed one rather than demoting a - // viewer app to publisher on every push. - if (deployedPolicy?.execution_mode === "viewer") { + // A pull records the mode as `public`/`guests` and nothing else, so a file + // saying neither may still be a viewer app: honour an explicit `viewer`, and + // otherwise keep the deployed mode rather than demoting one on every push. A + // file stating `publisher` does not demote a deployed viewer — that block is + // vestigial (only older pulls wrote it), and viewer is the stricter of the + // two, running each viewer's own identity instead of the publisher's. + if ( + app?.["policy"]?.["execution_mode"] === "viewer" || + deployedPolicy?.execution_mode === "viewer" + ) { return "viewer"; } return "publisher"; } +/** The policy fields a deploy derives from the app it is deploying. Carrying one + * over from either side would leave a grant keyed to sources this deploy just + * replaced: legacy `triggerables` are folded into `triggerables_v2` on read, and + * `s3_inputs` / `allowed_s3_keys` are what the backend enforces S3 access + * against. A low-code deploy recomputes these; a raw one produces none of them. + * `triggerables_v2` is not listed because both deploys overwrite it outright. */ +const DERIVED_POLICY_FIELDS = [ + "triggerables", + "s3_inputs", + "allowed_s3_keys", +] as const; + /** * What a push's regenerated policy starts from. The app file states only the * access mode, so regenerating from the local sources alone silently drops every - * other policy field the deployed app carries — its run-as identity, sandbox - * isolation, SDK scopes. Anything the file does state still wins. + * other policy field the deployed app carries — its sandbox isolation, SDK + * scopes. Anything else the file does state still wins. * - * Legacy `triggerables` are dropped: the backend folds them into - * `triggerables_v2` on read, so carrying them over would keep granting runnables - * this deploy no longer contains. + * The run-as identity is never taken from the file: the deployed policy owns it, + * and a push claims a different one only through `preserve_on_behalf_of`. Older + * pulls wrote the whole policy into the app file, so repos still carry + * `on_behalf_of` keys — a stale one, or an explicit `null`, would otherwise + * re-permission the app from checked-in content. */ export function deployedPolicyBase( deployedPolicy: Policy | undefined, localPolicy: Policy | undefined, ): Policy { - const { triggerables: _legacy, ...deployed } = deployedPolicy ?? {}; - return { ...deployed, ...(localPolicy ?? {}) } as Policy; + const { + on_behalf_of: _obo, + on_behalf_of_email: _oboEmail, + ...local + } = localPolicy ?? {}; + const base: Record = { ...(deployedPolicy ?? {}), ...local }; + for (const field of DERIVED_POLICY_FIELDS) { + delete base[field]; + } + return base as Policy; } export async function pushApp( workspace: string, diff --git a/cli/src/commands/app/raw_apps.ts b/cli/src/commands/app/raw_apps.ts index 917798dc88..c1a5ac30a1 100644 --- a/cli/src/commands/app/raw_apps.ts +++ b/cli/src/commands/app/raw_apps.ts @@ -447,16 +447,14 @@ export async function pushRawApp( // Submitting a policy is how the backend reads a claim on the app's execution // identity; without the flag it rewrites on_behalf_of to whoever ran the push. + // Only ever claims the deployed identity — on create the backend applies the + // folder default instead, as on the low-code path. const preserveFields: { preserve_on_behalf_of?: boolean } = {}; - if ( - permissionedAsContext?.userIsAdminOrDeployer && - appForPolicy.policy.on_behalf_of - ) { + if (permissionedAsContext?.userIsAdminOrDeployer && remotePolicy?.on_behalf_of) { preserveFields.preserve_on_behalf_of = true; log.info( `Preserving ${ - appForPolicy.policy.on_behalf_of_email ?? - appForPolicy.policy.on_behalf_of + remotePolicy.on_behalf_of_email ?? remotePolicy.on_behalf_of } as permissioned_as for app ${remotePath}`, ); } @@ -537,7 +535,6 @@ export async function pushRawApp( summary: localApp.summary, policy: appForPolicy.policy, deployment_message: message, - ...preserveFields, // Preserve any user draft at this path (see backend skip_draft_deletion). skip_draft_deletion: true, ...(localApp.custom_path diff --git a/cli/src/core/permissioned_as.ts b/cli/src/core/permissioned_as.ts index 57570b3549..046e1e55b7 100644 --- a/cli/src/core/permissioned_as.ts +++ b/cli/src/core/permissioned_as.ts @@ -130,7 +130,7 @@ export async function preCheckPermissionedAs( const label = typeStr === "script" ? "(script owner)" : "(flow owner)"; wouldChangeItems.push({ path: change.path, currentOwner: label }); - } else if (typeStr === "app") { + } else if (typeStr === "app" || typeStr === "raw_app") { wouldChangeItems.push({ path: change.path, currentOwner: "(app policy owner)", @@ -177,7 +177,7 @@ export async function preCheckPermissionedAs( } } continue; - } else if (typeStr === "app") { + } else if (typeStr === "app" || typeStr === "raw_app") { wouldChangeItems.push({ path: change.path, currentOwner: "(app policy owner)", diff --git a/cli/test/app_access_mode_unit.test.ts b/cli/test/app_access_mode_unit.test.ts index 12185d6d99..0d21b74ac3 100644 --- a/cli/test/app_access_mode_unit.test.ts +++ b/cli/test/app_access_mode_unit.test.ts @@ -1,5 +1,6 @@ import { expect, test } from "bun:test"; import { + deployedPolicyBase, executionModeFromAppFile, generatingPolicy, markAccessFromPolicy, @@ -26,3 +27,59 @@ test("the access mode survives the app.yaml round trip", async () => { expect(executionModeFromAppFile({ policy: { execution_mode: "publisher" } })).toBe("publisher"); expect(executionModeFromAppFile({})).toBe("publisher"); }); + +// `viewer` has no representation in the app file, so a push that read the mode +// off the file alone demoted every viewer app to publisher — running the +// publisher's identity for viewers instead of each viewer's own. +test("a deployed viewer mode is not demoted by a file that cannot express it", () => { + expect(executionModeFromAppFile({}, { execution_mode: "viewer" } as any)).toBe("viewer"); + expect(executionModeFromAppFile({ policy: { execution_mode: "viewer" } })).toBe("viewer"); + // Widening still has to come from the file, and dropping `public` still demotes. + expect(executionModeFromAppFile({}, { execution_mode: "anonymous" } as any)).toBe("publisher"); + expect( + executionModeFromAppFile({ public: true }, { execution_mode: "viewer" } as any), + ).toBe("anonymous"); +}); + +// A push regenerates the policy, so whatever `deployedPolicyBase` does not carry +// over is dropped from the deployed app. Older pulls wrote the whole policy into +// the app file, so repos still ship `on_behalf_of` keys — letting one win would +// hand the app's execution identity to checked-in content, which is what +// `preserve_on_behalf_of` exists to prevent. +test("deployedPolicyBase: the file never sets the run-as identity", () => { + const deployed: any = { + on_behalf_of: "u/svc", + on_behalf_of_email: "svc@corp", + sandbox: true, + }; + for ( + const local of [ + undefined, + { on_behalf_of: null, on_behalf_of_email: null }, + { on_behalf_of: "u/stale", on_behalf_of_email: "stale@corp" }, + ] as any[] + ) { + expect(deployedPolicyBase(deployed, local)).toMatchObject({ + on_behalf_of: "u/svc", + on_behalf_of_email: "svc@corp", + sandbox: true, + }); + } +}); + +// Grants keyed to the sources a deploy replaces must not survive it, from either +// side of the merge: the backend folds legacy `triggerables` into +// `triggerables_v2` on read and enforces S3 access against `s3_inputs`. +test("deployedPolicyBase: derived grants are never carried over", () => { + const stale: any = { + triggerables: { "gone:script/f/gone": {} }, + s3_inputs: [{ allowed_resources: ["u/gone"] }], + allowed_s3_keys: [{ s3_path: "gone" }], + }; + for (const [deployed, local] of [[stale, undefined], [{}, stale]] as any[]) { + const base = deployedPolicyBase(deployed, local) as any; + expect(base.triggerables).toBeUndefined(); + expect(base.s3_inputs).toBeUndefined(); + expect(base.allowed_s3_keys).toBeUndefined(); + } +}); diff --git a/cli/test/raw_app_sync.test.ts b/cli/test/raw_app_sync.test.ts index 42943514fd..d5e0fb6c6f 100644 --- a/cli/test/raw_app_sync.test.ts +++ b/cli/test/raw_app_sync.test.ts @@ -812,14 +812,39 @@ syncBehavior: v1`, "utf-8"); expect(pushResult2.code).toEqual(0); await waitForDeploymentJobs(backend); - const getResp = await backend.apiRequest!( - `/api/w/${backend.workspace}/apps/get/p/f/test/policy_app` - ); - expect(getResp.status).toEqual(200); - const policy = (await getResp.json()).policy; + async function readPolicy() { + const resp = await backend.apiRequest!( + `/api/w/${backend.workspace}/apps/get/p/f/test/policy_app` + ); + expect(resp.status).toEqual(200); + return (await resp.json()).policy; + } + const policy = await readPolicy(); expect(policy.on_behalf_of).toEqual("u/svc"); expect(policy.on_behalf_of_email).toEqual("svc@windmill.dev"); expect(policy.sandbox).toEqual(true); expect(policy.execution_mode).toEqual("viewer"); + + // The carry-over must not make the access mode sticky in the widening + // direction: `public: true` promotes, and dropping it demotes again. + const yamlPath = path.join(appDir, "raw_app.yaml"); + const baseYaml = await readFileContent(yamlPath); + await writeFile(yamlPath, `${baseYaml}public: true\n`, "utf-8"); + expect((await backend.runCLICommand( + ["sync", "push", "--yes"], tempDir, "raw_app_policy_test" + )).code).toEqual(0); + await waitForDeploymentJobs(backend); + expect(await readPolicy()).toMatchObject({ execution_mode: "anonymous" }); + + await writeFile(yamlPath, baseYaml, "utf-8"); + expect((await backend.runCLICommand( + ["sync", "push", "--yes"], tempDir, "raw_app_policy_test" + )).code).toEqual(0); + await waitForDeploymentJobs(backend); + expect(await readPolicy()).toMatchObject({ + execution_mode: "publisher", + sandbox: true, + on_behalf_of: "u/svc", + }); }); });