From 74f15e0e510e7147642297f0bf8dae3fe9e90ea3 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Mon, 17 Aug 2026 14:00:17 +0000 Subject: [PATCH] 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/`, 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) --- backend/windmill-api-flows/src/flows.rs | 6 ++ .../tests/operator_builder_rights.rs | 69 ++++++++++++++++++ backend/windmill-api/src/apps.rs | 73 +++++++++++++++---- backend/windmill-common/src/apps.rs | 33 +++++++++ backend/windmill-common/src/flows.rs | 5 +- docs/operator-builder-rights.md | 19 ++++- 6 files changed, 186 insertions(+), 19 deletions(-) diff --git a/backend/windmill-api-flows/src/flows.rs b/backend/windmill-api-flows/src/flows.rs index 47566306ee..cb4969b55e 100644 --- a/backend/windmill-api-flows/src/flows.rs +++ b/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 diff --git a/backend/windmill-api-integration-tests/tests/operator_builder_rights.rs b/backend/windmill-api-integration-tests/tests/operator_builder_rights.rs index 34afade339..58ac9b5aa2 100644 --- a/backend/windmill-api-integration-tests/tests/operator_builder_rights.rs +++ b/backend/windmill-api-integration-tests/tests/operator_builder_rights.rs @@ -257,6 +257,75 @@ async fn test_operator_builder_rights_boundary(db: Pool) -> 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 = 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(()) } diff --git a/backend/windmill-api/src/apps.rs b/backend/windmill-api/src/apps.rs index 7c35af7373..e2652d1588 100644 --- a/backend/windmill-api/src/apps.rs +++ b/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. // `:` 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::>(); + 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!({})); diff --git a/backend/windmill-common/src/apps.rs b/backend/windmill-common/src/apps.rs index 12bfaa7410..b1fe22e944 100644 --- a/backend/windmill-common/src/apps.rs +++ b/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< diff --git a/backend/windmill-common/src/flows.rs b/backend/windmill-common/src/flows.rs index 13152c472f..d7379e7fe0 100644 --- a/backend/windmill-common/src/flows.rs +++ b/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 { let mut refs = ComposedFlowRefs::default(); for module in value diff --git a/docs/operator-builder-rights.md b/docs/operator-builder-rights.md index cfa94ada57..9e4ef77327 100644 --- a/docs/operator-builder-rights.md +++ b/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/` and `flow/` 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/` and `flow/` 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