fix: bound what a builder-authored app can invoke at run time

Round 2 caught that the app-side check was bound to the wrong surface. It
checked the policy triggerables, on the premise that they are what
`execute_component` resolves. That holds for `rawscript/<sha>`, which is why the
raw-script refusal binds, but not for `script/`/`flow/`: in `ExecutionMode::Viewer`
run mode falls back to a default triggerable for any such path, so a path absent
from the map is invocable rather than forbidden, and the job runs as the viewer.
A builder could therefore ship an app pointing at a runnable nobody granted them,
and an admin who merely opened it would run it as themselves.

- `ExecutionMode::Viewer` is refused for a builder-authored app. `Publisher` and
  `Anonymous` have no fallback, so the triggerables are exhaustive for them and
  the deploy-time checks below are an authorization boundary rather than advice.
- The value's `runnableByPath` entries are authorized too. They are a separate
  surface from the policy: the deployed bundle resolves a `runnable_id` against
  the stored `runnables` and sends that path, so an app with an empty
  triggerables map still reaches one.

Also from round 2: `check_flow_is_composition_only`'s doc now states what it
actually returns, and the flow-side refs are deduped, so a flow stepping through
one script thirty times stops issuing thirty round trips on every write, preview
and dependency job.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

backend/windmill-api-flows/src/flows.rs

@@ -595,6 +595,12 @@ pub async fn validate_operator_composed_flow(
     if refs.runnables.is_empty() && refs.pinned_scripts.is_empty() {
         return Ok(());
     }
+    // A flow can step through the same script thirty times; this runs on every write, preview and
+    // dependency job.
+    refs.runnables.sort();
+    refs.runnables.dedup();
+    refs.pinned_scripts.sort_by_key(|(path, hash)| (path.clone(), hash.0));
+    refs.pinned_scripts.dedup_by_key(|(path, hash)| (path.clone(), hash.0));
     // Composing a runnable is enough to run it: the worker resolves a step's path with the root DB
     // handle and adopts that runnable's `on_behalf_of`, so an unreadable path would let a builder
     // execute code it cannot see, as whoever that code runs as. RLS on this transaction is the

backend/windmill-api-integration-tests/tests/operator_builder_rights.rs

@@ -257,6 +257,75 @@ async fn test_operator_builder_rights_boundary(db: Pool<Postgres>) -> anyhow::Re
         resp.text().await?
     );
 
+    // A full-code app is deployed multipart, so the endpoint is exercised the way the editor and
+    // the CLI use it. The value's `runnableByPath` entries are a separate surface from the policy
+    // triggerables: the empty triggerables map is what distinguishes checking the value from
+    // re-checking the same keys twice.
+    let raw_app = |path: &str, mode: &str, runnable_path: &str| {
+        reqwest::multipart::Form::new()
+            .part(
+                "app",
+                reqwest::multipart::Part::text(
+                    json!({
+                        "path": path,
+                        "summary": "",
+                        "value": {"files": {}, "runnables": {"r": {
+                            "name": "r", "type": "runnableByPath", "runType": "script",
+                            "path": runnable_path
+                        }}},
+                        "policy": {"execution_mode": mode, "triggerables_v2": {}}
+                    })
+                    .to_string(),
+                )
+                .mime_str("application/json")
+                .unwrap(),
+            )
+            .part(
+                "js",
+                reqwest::multipart::Part::text("console.log(1)").file_name("app.js"),
+            )
+    };
+    let resp = c
+        .post(format!("{api}/apps/create_raw"))
+        .multipart(raw_app("u/operator/a3", "publisher", "u/alice/private"))
+        .send()
+        .await?;
+    assert!(
+        !resp.status().is_success(),
+        "a builder must not deploy an app referencing a runnable it cannot read"
+    );
+    let resp = c
+        .post(format!("{api}/apps/create_raw"))
+        .multipart(raw_app("u/operator/a4", "viewer", "u/operator/some_script"))
+        .send()
+        .await?;
+    assert!(
+        !resp.status().is_success(),
+        "a builder must not deploy a viewer-mode app: the policy stops bounding what it can invoke"
+    );
+    let resp = c
+        .post(format!("{api}/apps/create_raw"))
+        .multipart(raw_app("u/operator/a5", "publisher", "u/operator/some_script"))
+        .send()
+        .await?;
+    assert!(
+        resp.status().is_success(),
+        "a builder must be able to deploy a full-code app over readable runnables: {}",
+        resp.text().await?
+    );
+    let sandbox: Option<bool> = sqlx::query_scalar(
+        "SELECT (policy->>'sandbox')::boolean FROM app WHERE workspace_id = $1 AND path = $2",
+    )
+    .bind(WS)
+    .bind("u/operator/a5")
+    .fetch_one(&db)
+    .await?;
+    assert_eq!(
+        sandbox,
+        Some(true),
+        "a builder-authored app must be stored sandboxed"
+    );
+
     invalidate_operator_builder_cache(WS);
     Ok(())
 }

backend/windmill-api/src/apps.rs

@@ -56,7 +56,10 @@ use std::str;
 use windmill_audit::audit_oss::{audit_log, AuditAuthorable};
 use windmill_audit::ActionKind;
 use windmill_common::{
-    apps::{app_value_has_inline_script, AppScriptId, ListAppQuery, APP_WORKSPACED_ROUTE},
+    apps::{
+        app_value_has_inline_script, app_value_runnable_paths, AppScriptId, ListAppQuery,
+        APP_WORKSPACED_ROUTE,
+    },
     auth::TOKEN_PREFIX_LEN,
     cache::{self, future::FutureCachedExt},
     db::{DbWithOptAuthed, UserDB},
@@ -1915,6 +1918,7 @@ fn check_operator_composed_app(
             "Operators with builder rights can only author full-code apps, and cannot convert an existing app into one".to_string(),
         ));
     }
+    let mut referenced: Vec<(bool, String)> = Vec::new();
     if let Some(value) = value {
         let value: serde_json::Value = serde_json::from_str(value.get()).map_err(to_anyhow)?;
         if app_value_has_inline_script(&value) {
@@ -1923,6 +1927,7 @@ fn check_operator_composed_app(
                     .to_string(),
             ));
         }
+        referenced.extend(app_value_runnable_paths(&value));
     }
     let Some(policy) = policy else {
         return Err(Error::BadRequest(
@@ -1952,20 +1957,33 @@ fn check_operator_composed_app(
     }
     policy.sandbox = Some(true);
 
+    // In `Viewer` mode `execute_component` falls back to a default triggerable for any
+    // `script/`/`flow/` path, so the policy stops being the list of what the app may invoke, and
+    // the job runs as the *viewer*. A builder-authored app would then let an admin who merely
+    // opens it run anything in the workspace as themselves. `Publisher` and `Anonymous` have no
+    // such fallback, so the triggerables checked below are exhaustive for them.
+    if matches!(policy.execution_mode, ExecutionMode::Viewer) {
+        return Err(Error::NotAuthorized(
+            "Operators with builder rights cannot deploy an app that runs as its viewer. Deploy it on behalf of yourself instead."
+                .to_string(),
+        ));
+    }
+
     // The triggerables are the deployed app's authorization to invoke a runnable.
     // `<component>:` prefixes the key when the app scopes it to one component.
-    let mut referenced = policy
-        .triggerables
-        .iter()
-        .flat_map(|t| t.keys())
-        .chain(policy.triggerables_v2.iter().flat_map(|t| t.keys()))
-        .filter_map(|key| {
-            let key = key.split_once(':').map_or(key.as_str(), |(_, rest)| rest);
-            key.strip_prefix("script/")
-                .map(|p| (false, p.to_string()))
-                .or_else(|| key.strip_prefix("flow/").map(|p| (true, p.to_string())))
-        })
-        .collect::<Vec<_>>();
+    referenced.extend(
+        policy
+            .triggerables
+            .iter()
+            .flat_map(|t| t.keys())
+            .chain(policy.triggerables_v2.iter().flat_map(|t| t.keys()))
+            .filter_map(|key| {
+                let key = key.split_once(':').map_or(key.as_str(), |(_, rest)| rest);
+                key.strip_prefix("script/")
+                    .map(|p| (false, p.to_string()))
+                    .or_else(|| key.strip_prefix("flow/").map(|p| (true, p.to_string())))
+            }),
+    );
     referenced.sort();
     referenced.dedup();
     for (_, path) in &referenced {
@@ -5703,6 +5721,12 @@ mod operator_app_tests {
         .unwrap()
     }
 
+    fn path_runnable(path: &str, run_type: &str) -> serde_json::Value {
+        serde_json::json!({"files": {}, "runnables": {
+            "a": {"name": "a", "type": "runnableByPath", "path": path, "runType": run_type}
+        }})
+    }
+
     fn composed_app(
         value: serde_json::Value,
         policy: &mut Policy,
@@ -5752,6 +5776,29 @@ mod operator_app_tests {
             );
         }
 
+        // What the deployed bundle actually asks to run comes from the value's `runnableByPath`
+        // entries, which are a separate surface from the policy: both are reported.
+        let mut policy = builder_policy(serde_json::json!({}));
+        assert_eq!(
+            composed_app(path_runnable("f/x/s", "script"), &mut policy).unwrap(),
+            vec![(false, "f/x/s".to_string())]
+        );
+        let mut policy = builder_policy(serde_json::json!({}));
+        assert_eq!(
+            composed_app(path_runnable("f/x/f", "flow"), &mut policy).unwrap(),
+            vec![(true, "f/x/f".to_string())]
+        );
+        let mut policy = builder_policy(serde_json::json!({}));
+        assert!(composed_app(path_runnable("hub/1/x", "hubscript"), &mut policy).is_err());
+
+        // `Viewer` mode makes `execute_component` accept any script/flow path, triggerables or
+        // not, and run it as the viewer, so the checks above would stop binding.
+        let mut policy: Policy = serde_json::from_value(serde_json::json!({
+            "execution_mode": "viewer", "triggerables_v2": {}
+        }))
+        .unwrap();
+        assert!(composed_app(clean.clone(), &mut policy).is_err());
+
         // Low-code apps and kind conversion stay closed.
         let value = to_raw_value(&clean);
         let mut policy = builder_policy(serde_json::json!({}));

backend/windmill-common/src/apps.rs

@@ -34,6 +34,39 @@ pub fn app_value_has_inline_script(value: &Value) -> bool {
     }
 }
 
+/// Every workspace runnable the app value points a component at, as `(is_flow, path)`.
+///
+/// This is what the deployed bundle actually asks `execute_component` to run: it resolves a
+/// `runnable_id` against the stored `runnables` and sends the referenced path. The policy's
+/// triggerables are a separate surface, so both have to be authorized.
+pub fn app_value_runnable_paths(value: &Value) -> Vec<(bool, String)> {
+    fn walk(value: &Value, out: &mut Vec<(bool, String)>) {
+        match value {
+            Value::Object(object) => {
+                let by_path = object
+                    .get("type")
+                    .and_then(Value::as_str)
+                    .is_some_and(|t| t == "runnableByPath" || t == "path");
+                if by_path {
+                    if let Some(path) = object.get("path").and_then(Value::as_str) {
+                        let is_flow =
+                            object.get("runType").and_then(Value::as_str) == Some("flow");
+                        out.push((is_flow, path.to_string()));
+                    }
+                }
+                for value in object.values() {
+                    walk(value, out);
+                }
+            }
+            Value::Array(array) => array.iter().for_each(|v| walk(v, out)),
+            _ => {}
+        }
+    }
+    let mut out = Vec::new();
+    walk(value, &mut out);
+    out
+}
+
 /// Traverse FlowValue while invoking provided by caller callback on leafs
 // #[async_recursion::async_recursion(?Send)]
 pub fn traverse_app_inline_scripts<

backend/windmill-common/src/flows.rs

@@ -252,8 +252,9 @@ pub async fn resolve_modules(
 /// modules, every branch, and the `tools` of an AI agent step.
 ///
 /// Returns what the caller still has to authorize against its own permissions, which this
-/// value-only walk cannot: the worker tags the steps pin (a tag is how a step picks the worker
-/// group it runs on) and the `(path, hash)` pairs of version-pinned script steps.
+/// value-only walk cannot: every runnable the steps reference, the worker tags they pin, and the
+/// `(path, hash)` pairs of version-pinned steps. See [`ComposedFlowRefs`] for why each one is not
+/// already settled by the walk.
 pub fn check_flow_is_composition_only(value: &FlowValue) -> Result<ComposedFlowRefs, Error> {
     let mut refs = ComposedFlowRefs::default();
     for module in value

docs/operator-builder-rights.md

@@ -40,10 +40,21 @@ permissions:
   that hash alone, with the path beside it never consulted, so a readable path paired with another
   script's hash still runs that other script.
 
-The same reasoning applies to a builder-authored app: its policy triggerables are what
-`execute_component` will resolve, also with the root DB handle, so
-`validate_operator_composed_app` checks every `script/<path>` and `flow/<path>` key the same way
-and refuses hub ones. `execute_component`'s preview branch refuses a hub path for operators too:
+The same reasoning applies to a builder-authored app, with one extra step. `execute_component`
+resolves the runnable it runs on the root handle, so `validate_operator_composed_app` checks every
+referenced path under the caller's RLS and refuses hub ones. But it has to check **two** surfaces,
+because they are not the same list: the policy's `script/<path>` and `flow/<path>` triggerables,
+and the `runnableByPath` entries in the app value, which is what the deployed bundle resolves a
+`runnable_id` against and sends.
+
+What makes those checks bind is that **`ExecutionMode::Viewer` is refused for a builder app**. In
+Viewer mode `execute_component` falls back to a default triggerable for any `script/`/`flow/`
+path, so the policy stops being the list of what the app may invoke, and the job runs as the
+*viewer*: an admin who merely opened the app would run anything in the workspace as themselves.
+`Publisher` and `Anonymous` have no such fallback. If you ever relax the Viewer refusal, the
+deploy-time path checks above stop being an authorization boundary.
+
+`execute_component`'s preview branch refuses a hub path for operators too:
 `require_path_read_access_for_preview` admits `hub/` for everyone.
 
 Call it on every write **and** every preview: `run_preview_flow_job` and