From 493d19689c942e67b3e903bd0c71b2563858816e Mon Sep 17 00:00:00 2001 From: Diego Imbert Date: Wed, 9 Sep 2026 16:33:25 +0200 Subject: [PATCH] fix(cli): stop sync push from resetting an app's run-as, sandbox and execution mode A raw app's policy was regenerated from the local sources alone on every push: raw_app.yaml records only the access mode, so `on_behalf_of` was rewritten to whoever ran the push, and `sandbox`, `frontend_sdk_scopes` and a `viewer` execution mode were dropped. A CI job running `wmill sync push` silently re-permissioned every raw app it touched. Start the regenerated policy from the deployed one, so the fields nothing local states carry over, and send `preserve_on_behalf_of` the way the low-code push already does. Low-code apps went through the same regeneration, so they lost `sandbox` too; both paths now share `deployedPolicyBase`, which drops the legacy `triggerables` the backend folds into `triggerables_v2` rather than carrying stale grants forward. The deletion-driven re-push (a file removed inside a flow/app/raw-app folder) also reached `pushObj` without the permissioned-as context, so it reset ownership even where the normal path preserved it. Fixes #11046 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01MaJ9KHDeaWrWUdVsHeHQ8L --- cli/src/commands/app/app.ts | 61 +++++++++++++++++++------ cli/src/commands/app/raw_apps.ts | 43 ++++++++++++++++-- cli/src/commands/sync/sync.ts | 10 +++-- cli/src/types.ts | 2 +- cli/test/raw_app_sync.test.ts | 77 ++++++++++++++++++++++++++++++++ 5 files changed, 173 insertions(+), 20 deletions(-) diff --git a/cli/src/commands/app/app.ts b/cli/src/commands/app/app.ts index b35efc02a3..f50027201a 100644 --- a/cli/src/commands/app/app.ts +++ b/cli/src/commands/app/app.ts @@ -119,9 +119,14 @@ export function isExecutionModeAnonymous(app: any) { export function isExecutionModeGuest(app: any) { return app?.["policy"]?.["execution_mode"] == "guest"; } -export type AppExecutionMode = "anonymous" | "guest" | "publisher"; +export type AppExecutionMode = + | "anonymous" + | "guest" + | "publisher" + | "viewer"; /** The access mode is the one policy field a tracked app keeps, as `public` (anonymous) - * or `guests` (guest); the rest of the policy is regenerated on push. */ + * or `guests` (guest); the rest is carried over from the deployed policy on push + * (see {@link deployedPolicyBase}). */ export function markAccessFromPolicy(app: any) { if (isExecutionModeAnonymous(app)) { app.public = true; @@ -129,15 +134,42 @@ export function markAccessFromPolicy(app: any) { app.guests = true; } } -export function executionModeFromAppFile(app: any): AppExecutionMode { +export function executionModeFromAppFile( + app: any, + deployedPolicy?: Policy, +): AppExecutionMode { if (app?.["public"] ?? isExecutionModeAnonymous(app)) { return "anonymous"; } 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") { + return "viewer"; + } return "publisher"; } + +/** + * 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. + * + * 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. + */ +export function deployedPolicyBase( + deployedPolicy: Policy | undefined, + localPolicy: Policy | undefined, +): Policy { + const { triggerables: _legacy, ...deployed } = deployedPolicy ?? {}; + return { ...deployed, ...(localPolicy ?? {}) } as Policy; +} export async function pushApp( workspace: string, remotePath: string, @@ -161,12 +193,9 @@ export async function pushApp( //ignore } - let remoteOnBehalfOf: string | undefined; - let remoteOnBehalfOfEmail: string | undefined; - if (app?.policy) { - remoteOnBehalfOf = app.policy.on_behalf_of; - remoteOnBehalfOfEmail = app.policy.on_behalf_of_email; - } + const remotePolicy = app?.policy as Policy | undefined; + const remoteOnBehalfOf = remotePolicy?.on_behalf_of; + const remoteOnBehalfOfEmail = remotePolicy?.on_behalf_of_email; markAccessFromPolicy(app); // console.log(app); @@ -181,7 +210,11 @@ export async function pushApp( const localApp = (await yamlParseFile(path)) as AppFile; replaceInlineScripts(localApp.value, localPath, true); - await generatingPolicy(localApp, remotePath, executionModeFromAppFile(localApp)); + // Read the mode off the file before the deployed policy is merged in, so + // dropping `public` from app.yaml still demotes the app. + const executionMode = executionModeFromAppFile(localApp, remotePolicy); + localApp.policy = deployedPolicyBase(remotePolicy, localApp.policy); + await generatingPolicy(localApp, remotePath, executionMode); const preserveFields: { preserve_on_behalf_of?: boolean } = {}; if (permissionedAsContext?.userIsAdminOrDeployer) { @@ -255,7 +288,7 @@ export async function generatingPolicy( ) { log.info(colors.gray(`Generating fresh policy for app ${path}...`)); try { - app.policy = await windmillUtils.updatePolicy(app.value, undefined); + app.policy = await windmillUtils.updatePolicy(app.value, app.policy); app.policy.execution_mode = executionMode; } catch (e) { log.error(colors.red(`Error generating policy for app ${path}: ${e}`)); @@ -425,14 +458,16 @@ async function push( if (isRawAppByName || hasRawAppYaml) { const { pushRawApp } = await import("./raw_apps.ts"); const merged = await mergeConfigWithConfigFile(opts); - // Raw-app ownership preservation is not implemented on either push - // path: sync push hands pushRawApp no context either. await pushRawApp( workspace.workspaceId, remotePath, absoluteFilePath, undefined, merged.defaultTs, + await buildPermissionedAsContext( + workspace.workspaceId, + await readEffectiveSyncBehavior(opts, workspace), + ), ); log.info(colors.bold.underline.green("Raw app pushed")); } else { diff --git a/cli/src/commands/app/raw_apps.ts b/cli/src/commands/app/raw_apps.ts index 901a47593f..917798dc88 100644 --- a/cli/src/commands/app/raw_apps.ts +++ b/cli/src/commands/app/raw_apps.ts @@ -1,6 +1,9 @@ import { requireLogin } from "../../core/auth.ts"; import { resolveWorkspace, validatePath } from "../../core/context.ts"; -import { mergeConfigWithConfigFile } from "../../core/conf.ts"; +import { + mergeConfigWithConfigFile, + readEffectiveSyncBehavior, +} from "../../core/conf.ts"; import { colors } from "@cliffy/ansi/colors"; import * as log from "../../core/log.ts"; import { sep as SEP } from "node:path"; @@ -13,10 +16,15 @@ import path from "node:path"; import { readdir } from "node:fs/promises"; import { GlobalOptions, isSuperset } from "../../types.ts"; +import { + buildPermissionedAsContext, + type PermissionedAsContext, +} from "../../core/permissioned_as.ts"; import { deepEqual, readTextFile } from "../../utils/utils.ts"; import { type AppExecutionMode, + deployedPolicyBase, executionModeFromAppFile, markAccessFromPolicy, replaceInlineScripts, @@ -360,6 +368,7 @@ export async function pushRawApp( localPath: string, message?: string, defaultTs: "bun" | "deno" = "bun", + permissionedAsContext?: PermissionedAsContext, ): Promise { if (alreadySynced.includes(localPath)) { return; @@ -376,6 +385,8 @@ export async function pushRawApp( } catch { //ignore } + + const remotePolicy = app?.policy as Policy | undefined; markAccessFromPolicy(app); // console.log(app); if (app) { @@ -423,13 +434,33 @@ export async function pushRawApp( repopulateFields(runnables); // Create a temporary app object for policy generation - const appForPolicy = { ...localApp, runnables }; + const appForPolicy = { + ...localApp, + runnables, + policy: deployedPolicyBase(remotePolicy, localApp.policy), + }; await generatingPolicy( appForPolicy, remotePath, - executionModeFromAppFile(localApp), + executionModeFromAppFile(localApp, remotePolicy), ); + // 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. + const preserveFields: { preserve_on_behalf_of?: boolean } = {}; + if ( + permissionedAsContext?.userIsAdminOrDeployer && + appForPolicy.policy.on_behalf_of + ) { + preserveFields.preserve_on_behalf_of = true; + log.info( + `Preserving ${ + appForPolicy.policy.on_behalf_of_email ?? + appForPolicy.policy.on_behalf_of + } as permissioned_as for app ${remotePath}`, + ); + } + const files = await collectAppFiles(localPath); async function createBundleRaw() { log.info(colors.yellow.bold(`Creating raw app ${remotePath} bundle...`)); @@ -483,6 +514,7 @@ 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 @@ -505,6 +537,7 @@ 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 @@ -564,6 +597,10 @@ async function pushRawAppCommand( filePath, undefined, merged.defaultTs, + await buildPermissionedAsContext( + workspace.workspaceId, + await readEffectiveSyncBehavior(opts, workspace), + ), ); log.info(colors.bold.underline.green("Raw app pushed")); } diff --git a/cli/src/commands/sync/sync.ts b/cli/src/commands/sync/sync.ts index 99a098f848..6108b60a57 100644 --- a/cli/src/commands/sync/sync.ts +++ b/cli/src/commands/sync/sync.ts @@ -6142,7 +6142,7 @@ export async function push( undefined, opts.plainSecrets ?? false, alreadySynced, - { message: opts.message }, + { message: opts.message, permissionedAsContext }, ); } else { // Flow folder doesn't exist locally — delete on server @@ -6187,7 +6187,7 @@ export async function push( undefined, opts.plainSecrets ?? false, alreadySynced, - { message: opts.message }, + { message: opts.message, permissionedAsContext }, ); } else { // App folder doesn't exist locally — delete on server @@ -6233,7 +6233,11 @@ export async function push( undefined, opts.plainSecrets ?? false, alreadySynced, - { message: opts.message, defaultTs: opts.defaultTs }, + { + message: opts.message, + defaultTs: opts.defaultTs, + permissionedAsContext, + }, ); } else { // The entire raw app folder was deleted locally, diff --git a/cli/src/types.ts b/cli/src/types.ts index 9c517ff3c7..5713d06390 100644 --- a/cli/src/types.ts +++ b/cli/src/types.ts @@ -234,7 +234,7 @@ export async function pushObj( if (!rawAppName) { throw new Error(`Could not extract raw app name from path: ${p}`); } - await pushRawApp(workspace, rawAppName, buildFolderPath(rawAppName, "raw_app"), message, defaultTs); + await pushRawApp(workspace, rawAppName, buildFolderPath(rawAppName, "raw_app"), message, defaultTs, permissionedAsContext); } else if (typeEnding === "folder") { await pushFolder(workspace, p, befObj, newObj); } else if (typeEnding === "variable") { diff --git a/cli/test/raw_app_sync.test.ts b/cli/test/raw_app_sync.test.ts index 315b274d80..42943514fd 100644 --- a/cli/test/raw_app_sync.test.ts +++ b/cli/test/raw_app_sync.test.ts @@ -746,3 +746,80 @@ excludes: []`, "utf-8"); expect(lowercased).toEqual([]); }); }); + +test("Raw App: push preserves the deployed policy's run-as, sandbox and execution mode", async () => { + await withTestBackend(async (backend, tempDir) => { + const testWorkspace = { + remote: backend.baseUrl, + workspaceId: backend.workspace, + name: "raw_app_policy_test", + token: backend.token + }; + await addWorkspace(testWorkspace, { force: true, configDir: backend.testConfigDir }); + + // syncBehavior v1 is what enables ownership preservation on update. + await writeFile(`${tempDir}/wmill.yaml`, `defaultTs: bun +includes: + - "**" +excludes: [] +syncBehavior: v1`, "utf-8"); + + const appDir = path.join(tempDir, "f", "test", "policy_app.raw_app"); + await mkdir(path.join(tempDir, "f", "test"), { recursive: true }); + await createRawAppOnDisk(appDir); + + const pushResult1 = await backend.runCLICommand( + ["sync", "push", "--yes"], + tempDir, "raw_app_policy_test" + ); + expect(pushResult1.code).toEqual(0); + await waitForDeploymentJobs(backend); + + // Stand in for the deploy drawer: give the app a run-as identity that is + // not the pushing user, sandbox isolation and a non-default mode. None of + // it is recorded in raw_app.yaml, so only the deployed policy carries it. + const setPolicy = await backend.apiRequest!( + `/api/w/${backend.workspace}/apps/update/f/test/policy_app`, + { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ + policy: { + on_behalf_of: "u/svc", + on_behalf_of_email: "svc@windmill.dev", + sandbox: true, + execution_mode: "viewer", + triggerables_v2: {}, + }, + preserve_on_behalf_of: true, + }), + } + ); + expect(setPolicy.status).toEqual(200); + + // Edit a source file so the app is not up to date and gets redeployed. + const appTsxPath = path.join(appDir, "App.tsx"); + await writeFile( + appTsxPath, + (await readFileContent(appTsxPath)).replace("hello world", "hello again"), + "utf-8" + ); + + const pushResult2 = await backend.runCLICommand( + ["sync", "push", "--yes"], + tempDir, "raw_app_policy_test" + ); + 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; + 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"); + }); +});