diff --git a/backend/.sqlx/query-3dccc8e745f4a0973541088f66172e74af6828c1ab52cf7a6fd10b305deade85.json b/backend/.sqlx/query-3dccc8e745f4a0973541088f66172e74af6828c1ab52cf7a6fd10b305deade85.json new file mode 100644 index 0000000000..c6e572863a --- /dev/null +++ b/backend/.sqlx/query-3dccc8e745f4a0973541088f66172e74af6828c1ab52cf7a6fd10b305deade85.json @@ -0,0 +1,23 @@ +{ + "db_name": "PostgreSQL", + "query": "SELECT EXISTS(SELECT 1 FROM flow WHERE workspace_id = $1 AND path = $2)", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "exists", + "type_info": "Bool" + } + ], + "parameters": { + "Left": [ + "Text", + "Text" + ] + }, + "nullable": [ + null + ] + }, + "hash": "3dccc8e745f4a0973541088f66172e74af6828c1ab52cf7a6fd10b305deade85" +} diff --git a/backend/.sqlx/query-4bc47050c74a02ab3169c3165898b2af07e995de71564d867b171f97f719fbde.json b/backend/.sqlx/query-4bc47050c74a02ab3169c3165898b2af07e995de71564d867b171f97f719fbde.json new file mode 100644 index 0000000000..9b2b79808d --- /dev/null +++ b/backend/.sqlx/query-4bc47050c74a02ab3169c3165898b2af07e995de71564d867b171f97f719fbde.json @@ -0,0 +1,23 @@ +{ + "db_name": "PostgreSQL", + "query": "SELECT EXISTS(SELECT 1 FROM script WHERE workspace_id = $1 AND path = $2)", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "exists", + "type_info": "Bool" + } + ], + "parameters": { + "Left": [ + "Text", + "Text" + ] + }, + "nullable": [ + null + ] + }, + "hash": "4bc47050c74a02ab3169c3165898b2af07e995de71564d867b171f97f719fbde" +} diff --git a/backend/migrations/20260816212109_notify_operator_settings_change.up.sql b/backend/migrations/20260816212109_notify_operator_settings_change.up.sql index a513aa637e..55a52f7320 100644 --- a/backend/migrations/20260816212109_notify_operator_settings_change.up.sql +++ b/backend/migrations/20260816212109_notify_operator_settings_change.up.sql @@ -16,4 +16,5 @@ DROP TRIGGER IF EXISTS operator_settings_change_trigger ON workspace_settings; CREATE TRIGGER operator_settings_change_trigger AFTER UPDATE OF operator_settings ON workspace_settings FOR EACH ROW +WHEN (OLD.operator_settings IS DISTINCT FROM NEW.operator_settings) EXECUTE FUNCTION notify_operator_settings_change(); diff --git a/backend/windmill-api-flows/src/flows.rs b/backend/windmill-api-flows/src/flows.rs index 67faad23f8..47566306ee 100644 --- a/backend/windmill-api-flows/src/flows.rs +++ b/backend/windmill-api-flows/src/flows.rs @@ -592,30 +592,56 @@ pub async fn validate_operator_composed_flow( .await?; } - // A version-pinned step dispatches on its `hash` alone, reading the row with root permissions - // and taking that script's tag and `on_behalf_of` identity; the `path` beside it is never - // consulted. So the pair has to be real, and readable by this caller, or a builder pins the - // hash of a script it cannot reach and runs that instead. - if !refs.pinned_scripts.is_empty() { - let mut tx = user_db.clone().begin(authed).await?; - for (path, hash) in &refs.pinned_scripts { - let exists = sqlx::query_scalar!( - "SELECT EXISTS(SELECT 1 FROM script WHERE workspace_id = $1 AND path = $2 AND hash = $3)", + if refs.runnables.is_empty() && refs.pinned_scripts.is_empty() { + return Ok(()); + } + // 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 + // check. A pinned `hash` needs its own comparison on top: the dispatch ignores the path beside + // it, so a readable path paired with another script's hash still runs that other script. + let mut tx = user_db.clone().begin(authed).await?; + for (is_flow, path) in &refs.runnables { + let readable = if *is_flow { + sqlx::query_scalar!( + "SELECT EXISTS(SELECT 1 FROM flow WHERE workspace_id = $1 AND path = $2)", + w_id, + path, + ) + } else { + sqlx::query_scalar!( + "SELECT EXISTS(SELECT 1 FROM script WHERE workspace_id = $1 AND path = $2)", w_id, path, - hash.0, ) - .fetch_one(&mut *tx) - .await? - .unwrap_or(false); - if !exists { - return Err(Error::NotAuthorized(format!( - "Version {hash} is not a readable version of {path}" - ))); - } } - tx.commit().await?; + .fetch_one(&mut *tx) + .await? + .unwrap_or(false); + if !readable { + return Err(Error::NotAuthorized(format!( + "{} {path} does not exist or is not readable by you", + if *is_flow { "Flow" } else { "Script" } + ))); + } } + for (path, hash) in &refs.pinned_scripts { + let exists = sqlx::query_scalar!( + "SELECT EXISTS(SELECT 1 FROM script WHERE workspace_id = $1 AND path = $2 AND hash = $3)", + w_id, + path, + hash.0, + ) + .fetch_one(&mut *tx) + .await? + .unwrap_or(false); + if !exists { + return Err(Error::NotAuthorized(format!( + "Version {hash} is not a readable version of {path}" + ))); + } + } + tx.commit().await?; Ok(()) } 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 14ca6f502b..34afade339 100644 --- a/backend/windmill-api-integration-tests/tests/operator_builder_rights.rs +++ b/backend/windmill-api-integration-tests/tests/operator_builder_rights.rs @@ -31,6 +31,10 @@ async fn set_builder(db: &Pool, enabled: bool) -> anyhow::Result<()> { } fn composition_flow(path: &str) -> serde_json::Value { + composition_flow_at(path, "u/operator/some_script") +} + +fn composition_flow_at(path: &str, step_path: &str) -> serde_json::Value { json!({ "path": path, "summary": "", @@ -38,7 +42,7 @@ fn composition_flow(path: &str) -> serde_json::Value { "schema": {}, "value": {"modules": [{ "id": "a", - "value": {"type": "script", "path": "u/operator/some_script", "input_transforms": {}} + "value": {"type": "script", "path": step_path, "input_transforms": {}} }]} }) } @@ -71,6 +75,17 @@ async fn test_operator_builder_rights_boundary(db: Pool) -> anyhow::Re let api = format!("http://localhost:{port}/api/w/{WS}"); let c = operator_client(); + // A composition-only flow references a runnable that exists and the builder can read; the + // check now rejects anything else, so the fixture needs one. + sqlx::query( + "INSERT INTO script (workspace_id, hash, path, content, language, kind, created_by, schema, + summary, description, lock, extra_perms) + VALUES ($1, 4241, 'u/operator/some_script', 'x', 'bun', 'script', 'operator', '{}', '', '', '', '{}')", + ) + .bind(WS) + .execute(&db) + .await?; + set_builder(&db, false).await?; let resp = c .post(format!("{api}/flows/create")) @@ -178,6 +193,27 @@ async fn test_operator_builder_rights_boundary(db: Pool) -> anyhow::Re "a builder must not create a low-code app" ); + // 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`. `permissions_test` gives the operator + // fixture no rights on `u/alice/**`. + sqlx::query( + "INSERT INTO script (workspace_id, hash, path, content, language, kind, created_by, schema, + summary, description, lock, extra_perms) + VALUES ($1, 4243, 'u/alice/private', 'x', 'bun', 'script', 'alice', '{}', '', '', '', '{}')", + ) + .bind(WS) + .execute(&db) + .await?; + let resp = c + .post(format!("{api}/flows/create")) + .json(&composition_flow_at("u/operator/f4", "u/alice/private")) + .send() + .await?; + assert!( + !resp.status().is_success(), + "a builder must not compose a runnable it cannot read" + ); + // A version-pinned step dispatches on its hash alone, so the pair must be real and readable: // otherwise a builder pins the hash of a script it cannot reach and runs that instead. sqlx::query( diff --git a/backend/windmill-api/src/apps.rs b/backend/windmill-api/src/apps.rs index 3cd7279f9d..7c35af7373 100644 --- a/backend/windmill-api/src/apps.rs +++ b/backend/windmill-api/src/apps.rs @@ -1901,12 +1901,15 @@ async fn store_raw_app_file<'a>( /// so isolation is what makes it safe to let an operator publish one: `sandbox` renders it in an /// opaque-origin iframe instead of handing it the viewer's Windmill session. Inline scripts are /// the low-code side's way of carrying code and must not appear either. +/// The value-and-policy half: everything decidable without the DB. Returns the workspace +/// runnables the policy authorizes the app to invoke, as `(is_flow, path)`, for +/// [`validate_operator_composed_app`] to authorize against the builder's permissions. fn check_operator_composed_app( raw_app: bool, value: Option<&RawValue>, policy: Option<&mut Policy>, allow_kind_change: bool, -) -> Result<()> { +) -> Result> { if !raw_app || allow_kind_change { return Err(Error::NotAuthorized( "Operators with builder rights can only author full-code apps, and cannot convert an existing app into one".to_string(), @@ -1948,6 +1951,78 @@ fn check_operator_composed_app( )); } policy.sandbox = Some(true); + + // 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.sort(); + referenced.dedup(); + for (_, path) in &referenced { + if path.starts_with("hub/") { + return Err(Error::NotAuthorized(format!( + "Operators with builder rights cannot reference the hub runnable {path}. Deploy it to the workspace first." + ))); + } + } + Ok(referenced) +} + +/// Runs on every app write by an operator with builder rights, from inside the create/update +/// internals so both raw-app endpoints are covered by one check. +/// +/// `execute_component` resolves the runnable it picks with the root DB handle, so an unreadable +/// path in the policy means an admin who merely opens the app runs code the builder could not +/// see, as themselves. RLS on this transaction is the check. +async fn validate_operator_composed_app( + authed: &ApiAuthed, + user_db: &UserDB, + w_id: &str, + raw_app: bool, + value: Option<&RawValue>, + policy: Option<&mut Policy>, + allow_kind_change: bool, +) -> Result<()> { + let referenced = check_operator_composed_app(raw_app, value, policy, allow_kind_change)?; + if referenced.is_empty() { + return Ok(()); + } + let mut tx = user_db.clone().begin(authed).await?; + for (is_flow, path) in referenced { + let readable = if is_flow { + sqlx::query_scalar!( + "SELECT EXISTS(SELECT 1 FROM flow WHERE workspace_id = $1 AND path = $2)", + w_id, + path, + ) + } else { + sqlx::query_scalar!( + "SELECT EXISTS(SELECT 1 FROM script WHERE workspace_id = $1 AND path = $2)", + w_id, + path, + ) + } + .fetch_one(&mut *tx) + .await? + .unwrap_or(false); + if !readable { + return Err(Error::NotAuthorized(format!( + "{} {path} does not exist or is not readable by you", + if is_flow { "Flow" } else { "Script" } + ))); + } + } + tx.commit().await?; Ok(()) } @@ -2163,7 +2238,16 @@ async fn create_app_internal<'a>( check_scopes(&authed, || format!("apps:write:{}", &app.path))?; validate_frontend_sdk_scopes(&app.policy)?; if authed.is_operator { - check_operator_composed_app(raw_app, Some(&app.value.0), Some(&mut app.policy), false)?; + validate_operator_composed_app( + &authed, + &user_db, + w_id, + raw_app, + Some(&app.value.0), + Some(&mut app.policy), + false, + ) + .await?; } if *CLOUD_HOSTED { let nb_apps = @@ -3055,12 +3139,16 @@ async fn update_app_internal<'a>( } if authed.is_operator { - check_operator_composed_app( + validate_operator_composed_app( + &authed, + &user_db, + w_id, raw_app, ns.value.as_ref().map(|v| v.0.as_ref()), ns.policy.as_mut(), ns.allow_kind_change.unwrap_or(false), - )?; + ) + .await?; } let mut tx = user_db.clone().begin(&authed).await?; @@ -3569,9 +3657,20 @@ async fn execute_component( })?; // A builder testing the app it is composing only ever previews a deployed runnable // (`path`) or a persisted app script (`id`), both confined below to what it may read. - // Inline `raw_code` is authoring code, so it stays closed to every operator. + // Inline `raw_code` is authoring code, so it stays closed to every operator. So is a hub + // path, which `require_path_read_access_for_preview` admits for everyone and + // `get_payload_tag_from_prefixed_path` then downloads and enqueues: it is exactly the + // unreviewed code the composition check refuses in a flow. + let previews_hub = payload.path.as_deref().is_some_and(|p| { + p.strip_prefix("script/") + .or_else(|| p.strip_prefix("flow/")) + .unwrap_or(p) + .starts_with("hub/") + }); if authed.is_operator - && (payload.raw_code.is_some() || !operator_builder_enabled(&db, &w_id).await?) + && (payload.raw_code.is_some() + || previews_hub + || !operator_builder_enabled(&db, &w_id).await?) { return Err(Error::NotAuthorized( "Operators cannot run preview jobs for security reasons".to_string(), @@ -5607,7 +5706,7 @@ mod operator_app_tests { fn composed_app( value: serde_json::Value, policy: &mut Policy, - ) -> Result<()> { + ) -> Result> { let value = to_raw_value(&value); check_operator_composed_app(true, Some(&value), Some(policy), false) } @@ -5620,8 +5719,16 @@ mod operator_app_tests { // A composition-only app goes through, sandboxed whether or not it asked to be. let mut policy = builder_policy(serde_json::json!({"a:script/f/x/s": {"static_inputs": {}, "one_of_inputs": {}}})); - composed_app(clean.clone(), &mut policy).unwrap(); + let referenced = composed_app(clean.clone(), &mut policy).unwrap(); assert_eq!(policy.sandbox, Some(true)); + // The runnables the policy authorizes are handed back for the caller to authorize. + assert_eq!(referenced, vec![(false, "f/x/s".to_string())]); + + // A hub reference is unreviewed code, refused like it is in a flow. + let mut policy = builder_policy( + serde_json::json!({"a:script/hub/1/x": {"static_inputs": {}, "one_of_inputs": {}}}), + ); + assert!(composed_app(clean.clone(), &mut policy).is_err()); let mut policy = builder_policy(serde_json::json!({})); policy.sandbox = Some(false); diff --git a/backend/windmill-common/src/flows.rs b/backend/windmill-common/src/flows.rs index ba05f65efe..13152c472f 100644 --- a/backend/windmill-common/src/flows.rs +++ b/backend/windmill-common/src/flows.rs @@ -271,9 +271,12 @@ pub fn check_flow_is_composition_only(value: &FlowValue) -> Result, + /// Every workspace runnable a step references, as `(is_flow, path)`. The worker resolves + /// these with the root DB handle and adopts the referenced runnable's `on_behalf_of`, so + /// composing a path is enough to run it, and to run it as whoever it runs as. + pub runnables: Vec<(bool, String)>, /// Version-pinned script steps. A step carrying a `hash` is dispatched by that hash alone, - /// with the path ignored and the row read with root permissions, so an unverified pair runs - /// some other script's code under some other script's `on_behalf_of` identity. + /// with the path beside it ignored, so the pair has to be checked on top of the path. pub pinned_scripts: Vec<(String, ScriptHash)>, } @@ -322,11 +325,15 @@ fn check_module_value_is_composition_only( FlowModuleValue::Script { path, hash, tag_override, .. } => { check_composable_path(path, id)?; push_tag(tag_override); + refs.runnables.push((false, path.clone())); if let Some(hash) = hash { refs.pinned_scripts.push((path.clone(), *hash)); } } - FlowModuleValue::Flow { path, .. } => check_composable_path(path, id)?, + FlowModuleValue::Flow { path, .. } => { + check_composable_path(path, id)?; + refs.runnables.push((true, path.clone())); + } FlowModuleValue::ForloopFlow { modules, modules_node, .. } | FlowModuleValue::WhileloopFlow { modules, modules_node, .. } => { refuse_node(modules_node)?; diff --git a/docs/operator-builder-rights.md b/docs/operator-builder-rights.md index 3f365e5814..cfa94ada57 100644 --- a/docs/operator-builder-rights.md +++ b/docs/operator-builder-rights.md @@ -31,10 +31,20 @@ It also returns what a value-only walk cannot authorize, for the caller to check permissions: - **the worker tags the steps pin**, or a builder routes a job onto a privileged worker group; +- **every runnable a step references**. `script_to_payload` resolves a step's path with the root DB + handle (`db_authed = None`) and returns the referenced runnable's `on_behalf_of`, which + `worker_flow` then applies to the step job. So composing a path is enough to run it, and to run + it as whoever it runs as: `validate_operator_composed_flow` re-checks each path under the + caller's RLS. This is the general case; the one below is on top of it, not instead of it. - **the `(path, hash)` of every version-pinned step**. A step carrying a `hash` is dispatched by - that hash alone: `script_to_payload` ignores the path, reads the row with root permissions, and - takes that script's tag and `on_behalf_of` identity. An unverified pair therefore runs some other - script's code, possibly as some other identity, from behind a path the builder may read. + 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: +`require_path_read_access_for_preview` admits `hub/` for everyone. Call it on every write **and** every preview: `run_preview_flow_job` and `push_flow_dependencies_job` both take a request-supplied flow value, so leaving either out makes diff --git a/frontend/src/lib/components/common/table/AppRow.svelte b/frontend/src/lib/components/common/table/AppRow.svelte index a179faeb43..099f21d522 100644 --- a/frontend/src/lib/components/common/table/AppRow.svelte +++ b/frontend/src/lib/components/common/table/AppRow.svelte @@ -209,7 +209,7 @@ // list endpoint only surfaces own/legacy draft-only rows), so // discarding it never requires write permission on the path. disabled: !showEditButton, - hide: $userStore?.operator && !$operatorBuilderRights + hide: $userStore?.operator && !($operatorBuilderRights && app.raw_app) }, { displayName: $userStore?.operator ? 'View JSON' : 'View/Edit JSON', @@ -271,7 +271,7 @@ displayName: 'Deployments', icon: History, action: () => appDeploymentHistory?.open(), - hide: $userStore?.operator && !$operatorBuilderRights + hide: $userStore?.operator && !($operatorBuilderRights && app.raw_app) }, { displayName: 'Permissions', @@ -279,7 +279,7 @@ action: () => { shareModal.openDrawer && shareModal.openDrawer(path, 'app') }, - hide: $userStore?.operator && !$operatorBuilderRights + hide: $userStore?.operator && !($operatorBuilderRights && app.raw_app) }, { displayName: 'Copy path', @@ -325,7 +325,7 @@ }, type: 'delete', disabled: !canEdit, - hide: $userStore?.operator && !$operatorBuilderRights + hide: $userStore?.operator && !($operatorBuilderRights && app.raw_app) } ] }}