fix: close three holes in the operator builder boundary

Found by the local Codex and Claude review passes. Each one let an operator with
builder rights reach code the composition check was supposed to keep out.

- Version-pinned flow steps. A step carrying a `hash` is dispatched by that hash
  alone: `script_to_payload` ignores the path beside it, reads the row with root
  permissions, and takes that script's tag and `on_behalf_of` identity. A builder
  could pin the hash of a script it cannot read under a path it can, and run that
  instead, possibly as the identity that script runs as. The walk now reports
  every `(path, hash)` pair and `validate_operator_composed_flow` verifies each
  against the caller's own permissions.

- Raw-script triggerables in a builder-authored app policy. `execute_component`'s
  run mode authorizes caller-supplied `raw_code` by the policy's
  `rawscript/<sha>` key alone; its operator guard only covers preview mode. A
  builder could deploy a clean value whose policy pinned an arbitrary sha, then
  execute matching code on a worker. Both triggerables maps are now refused such
  a key, which also closes the policy-supplied worker tag that rode with it.

- `inlineScript` without a `language`. `traverse_app_inline_scripts` only reports
  a script whose language parses and stops descending at the key, which is right
  for locking and wrong for an authorization check the author's fields control.
  Replaced with `app_value_has_inline_script`, which refuses the key itself.

Also: the builder flag gates writes and was cached per process, so revoking it
left every other replica authorizing until its own entry expired. An
`AFTER UPDATE OF operator_settings` trigger now emits `notify_operator_settings_change`
and `process_notify_event` drops the entry, the same way the other
authorization-adjacent caches propagate.

And `CreateActionsMenu`'s option list read the store outside a reactive context,
so switching workspace without a reload kept the previous workspace's kinds.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Ruben Fiszel
2026-08-16 22:00:23 +00:00
co-authored by Claude Opus 5
parent 6e9e0de481
commit b4a4cd8fdd
12 changed files with 348 additions and 68 deletions
@@ -0,0 +1,24 @@
{
"db_name": "PostgreSQL",
"query": "SELECT EXISTS(SELECT 1 FROM script WHERE workspace_id = $1 AND path = $2 AND hash = $3)",
"describe": {
"columns": [
{
"ordinal": 0,
"name": "exists",
"type_info": "Bool"
}
],
"parameters": {
"Left": [
"Text",
"Text",
"Int8"
]
},
"nullable": [
null
]
},
"hash": "298e0e30afdc973c517a9b4ea99f5dd1616a62c6ecbaaea312c686038d21e6fd"
}
@@ -0,0 +1,2 @@
DROP TRIGGER IF EXISTS operator_settings_change_trigger ON workspace_settings;
DROP FUNCTION IF EXISTS notify_operator_settings_change();
@@ -0,0 +1,19 @@
-- `operator_settings.builder` is an authorization decision (it gates flow and app writes for
-- operators) and is read through a per-process cache. Without this, revoking builder rights on one
-- API replica leaves every other replica authorizing writes until its own entry expires.
-- SECURITY DEFINER so the INSERT runs as the function owner: windmill_user fires this trigger and
-- has no rights on notify_event.
CREATE OR REPLACE FUNCTION notify_operator_settings_change()
RETURNS TRIGGER AS $$
BEGIN
INSERT INTO notify_event (channel, payload)
VALUES ('notify_operator_settings_change', NEW.workspace_id);
RETURN NEW;
END;
$$ LANGUAGE plpgsql SECURITY DEFINER;
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
EXECUTE FUNCTION notify_operator_settings_change();
+7
View File
@@ -1745,6 +1745,13 @@ async fn process_notify_event(
);
windmill_common::workspaces::TEAM_PLAN_CACHE.remove(payload);
}
"notify_operator_settings_change" => {
tracing::info!(
"Operator settings change detected, invalidating operator builder cache: {}",
payload
);
windmill_common::workspaces::invalidate_operator_builder_cache(payload);
}
"notify_workspace_rate_limit_change" => {
tracing::info!(
"Workspace rate limit change detected, invalidating rate limit cache: {}",
+57 -29
View File
@@ -537,6 +537,7 @@ async fn validate_flow(
new_flow: &NewFlow,
authed: &ApiAuthed,
db: &DB,
user_db: &UserDB,
w_id: &str,
) -> error::Result<()> {
#[cfg(not(feature = "enterprise"))]
@@ -555,6 +556,7 @@ async fn validate_flow(
&new_flow.tag,
authed,
db,
user_db,
w_id,
)
.await?;
@@ -564,18 +566,22 @@ async fn validate_flow(
}
/// Runs on every write and every preview of a flow authored by an operator with builder rights.
/// The tags a step pins are authorized here rather than in the walk: without it a builder could
/// route a job onto a privileged worker group.
/// The walk in `check_flow_is_composition_only` only sees the value; what it collects is
/// authorized here against the caller's own permissions.
pub async fn validate_operator_composed_flow(
value: &FlowValue,
flow_tag: &Option<String>,
authed: &ApiAuthed,
db: &DB,
user_db: &UserDB,
w_id: &str,
) -> error::Result<()> {
let mut tags = windmill_common::flows::check_flow_is_composition_only(value)?;
tags.extend(flow_tag.clone());
for tag in tags.iter().filter(|t| !t.is_empty()) {
let mut refs = windmill_common::flows::check_flow_is_composition_only(value)?;
// A tag is how a step picks the worker group it runs on: unauthorized, a builder could route
// a job onto a privileged one.
refs.tags.extend(flow_tag.clone());
for tag in refs.tags.iter().filter(|t| !t.is_empty()) {
windmill_common::jobs::check_tag_available_for_workspace_internal(
db,
w_id,
@@ -585,6 +591,31 @@ 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)",
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(())
}
@@ -616,7 +647,7 @@ async fn create_flow(
return Err(Error::PermissionDenied(msg));
}
validate_flow(&nf, &authed, &db, &w_id).await?;
validate_flow(&nf, &authed, &db, &user_db, &w_id).await?;
if *CLOUD_HOSTED {
let nb_flows =
sqlx::query_scalar!("SELECT COUNT(*) FROM flow WHERE workspace_id = $1", &w_id)
@@ -649,8 +680,7 @@ async fn create_flow(
// Apply folder default_permissioned_as on create when the caller did not
// explicitly preserve a value and the user can preserve.
let explicit_preserve = (nf.on_behalf_of_email.is_some()
|| nf.on_behalf_of.is_some())
let explicit_preserve = (nf.on_behalf_of_email.is_some() || nf.on_behalf_of.is_some())
&& nf.preserve_on_behalf_of.unwrap_or(false)
&& windmill_common::can_preserve_on_behalf_of(&authed);
if !explicit_preserve && windmill_common::can_preserve_on_behalf_of(&authed) {
@@ -670,16 +700,15 @@ async fn create_flow(
check_schedule_conflict(&mut tx, &w_id, &nf.path).await?;
let schema_str = nf.schema.and_then(|x| serde_json::to_string(&x.0).ok());
let resolved_on_behalf_of =
windmill_common::resolve_on_behalf_of(
nf.on_behalf_of_email.as_deref(),
nf.on_behalf_of.as_deref(),
nf.preserve_on_behalf_of.unwrap_or(false),
&authed,
&w_id,
&db,
)
.await?;
let resolved_on_behalf_of = windmill_common::resolve_on_behalf_of(
nf.on_behalf_of_email.as_deref(),
nf.on_behalf_of.as_deref(),
nf.preserve_on_behalf_of.unwrap_or(false),
&authed,
&w_id,
&db,
)
.await?;
// Written beside the principal only while a worker that still reads it may be live.
let legacy_on_behalf_of_email =
windmill_common::legacy_on_behalf_of_email(resolved_on_behalf_of.as_deref(), &w_id, &db)
@@ -1174,7 +1203,7 @@ async fn update_flow(
return Err(Error::PermissionDenied(msg));
}
validate_flow(&nf, &authed, &db, &w_id).await?;
validate_flow(&nf, &authed, &db, &user_db, &w_id).await?;
let authed = maybe_refresh_folders(&flow_path, &w_id, authed, &db).await;
let mut tx = user_db.clone().begin(&authed).await?;
@@ -1193,16 +1222,15 @@ async fn update_flow(
let old_dep_job = not_found_if_none(old_dep_job, "Flow", flow_path)?;
let is_new_path = nf.path != flow_path;
let schema_str = schema.and_then(|x| serde_json::to_string(&x).ok());
let resolved_on_behalf_of =
windmill_common::resolve_on_behalf_of(
nf.on_behalf_of_email.as_deref(),
nf.on_behalf_of.as_deref(),
nf.preserve_on_behalf_of.unwrap_or(false),
&authed,
&w_id,
&db,
)
.await?;
let resolved_on_behalf_of = windmill_common::resolve_on_behalf_of(
nf.on_behalf_of_email.as_deref(),
nf.on_behalf_of.as_deref(),
nf.preserve_on_behalf_of.unwrap_or(false),
&authed,
&w_id,
&db,
)
.await?;
// Written beside the principal only while a worker that still reads it may be live.
let legacy_on_behalf_of_email =
windmill_common::legacy_on_behalf_of_email(resolved_on_behalf_of.as_deref(), &w_id, &db)
@@ -178,6 +178,49 @@ async fn test_operator_builder_rights_boundary(db: Pool<Postgres>) -> anyhow::Re
"a builder must not create a low-code app"
);
// 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(
"INSERT INTO script (workspace_id, hash, path, content, language, kind, created_by, schema,
summary, description, lock, extra_perms)
VALUES ($1, 4242, 'u/operator/pinned', 'x', 'bun', 'script', 'operator', '{}', '', '', '', '{}')",
)
.bind(WS)
.execute(&db)
.await?;
let pinned = |hash: &str| {
json!({
"path": "u/operator/f3", "summary": "", "description": "", "schema": {},
"value": {"modules": [{
"id": "a",
"value": {
"type": "script", "path": "u/operator/pinned", "hash": hash,
"input_transforms": {}
}
}]}
})
};
let resp = c
.post(format!("{api}/flows/create"))
.json(&pinned("0000000000000000"))
.send()
.await?;
assert!(
!resp.status().is_success(),
"a builder must not pin a hash that is not a version of the step's path"
);
let resp = c
.post(format!("{api}/flows/create"))
.json(&pinned("0000000000001092"))
.send()
.await?;
assert!(
resp.status().is_success(),
"a builder must be able to pin the real version of a readable script: {}",
resp.text().await?
);
invalidate_operator_builder_cache(WS);
Ok(())
}
+84 -5
View File
@@ -56,7 +56,7 @@ use std::str;
use windmill_audit::audit_oss::{audit_log, AuditAuthorable};
use windmill_audit::ActionKind;
use windmill_common::{
apps::{traverse_app_inline_scripts, AppScriptId, ListAppQuery, APP_WORKSPACED_ROUTE},
apps::{app_value_has_inline_script, AppScriptId, ListAppQuery, APP_WORKSPACED_ROUTE},
auth::TOKEN_PREFIX_LEN,
cache::{self, future::FutureCachedExt},
db::{DbWithOptAuthed, UserDB},
@@ -1914,18 +1914,34 @@ fn check_operator_composed_app(
}
if let Some(value) = value {
let value: serde_json::Value = serde_json::from_str(value.get()).map_err(to_anyhow)?;
traverse_app_inline_scripts(&value, None, &mut |_, _| {
Err(Error::NotAuthorized(
if app_value_has_inline_script(&value) {
return Err(Error::NotAuthorized(
"Operators with builder rights cannot deploy an app carrying inline scripts"
.to_string(),
))
})?;
));
}
}
let Some(policy) = policy else {
return Err(Error::BadRequest(
"Operators with builder rights must deploy an app with its policy".to_string(),
));
};
// A `rawscript/<sha>` triggerable is the deployed app's authorization to run caller-supplied
// `raw_code` whose content hashes to it (see `execute_component`'s run mode, which authorizes
// inline code by this key alone). Pinning one would hand a builder arbitrary code execution
// through an app whose *value* passed the inline-script check above. A composition-only app
// has none, so refusing them costs nothing.
fn pins_raw_script<'a, T>(map: &'a Option<HashMap<String, T>>) -> bool {
map.iter().flat_map(|m| m.keys()).any(|k| {
k.starts_with("rawscript/") || k.contains(":rawscript/")
})
}
if pins_raw_script(&policy.triggerables) || pins_raw_script(&policy.triggerables_v2) {
return Err(Error::NotAuthorized(
"Operators with builder rights cannot deploy an app whose policy pins inline code"
.to_string(),
));
}
if policy.sandbox == Some(false) {
return Err(Error::NotAuthorized(
"Operators with builder rights can only deploy sandboxed apps".to_string(),
@@ -5573,3 +5589,66 @@ mod embed_token_tests {
assert!(parse_embed_policy("not json").is_err());
}
}
#[cfg(test)]
mod operator_app_tests {
use super::{check_operator_composed_app, Policy};
use windmill_common::error::Result;
use windmill_common::worker::to_raw_value;
fn builder_policy(triggerables_v2: serde_json::Value) -> Policy {
serde_json::from_value(serde_json::json!({
"execution_mode": "publisher",
"triggerables_v2": triggerables_v2,
}))
.unwrap()
}
fn composed_app(
value: serde_json::Value,
policy: &mut Policy,
) -> Result<()> {
let value = to_raw_value(&value);
check_operator_composed_app(true, Some(&value), Some(policy), false)
}
#[test]
fn operator_app_check_forces_the_sandbox_and_refuses_code() {
let clean = serde_json::json!({"files": {}, "runnables": {
"a": {"name": "a", "type": "runnableByPath", "path": "f/x/s", "runType": "script"}
}});
// 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();
assert_eq!(policy.sandbox, Some(true));
let mut policy = builder_policy(serde_json::json!({}));
policy.sandbox = Some(false);
assert!(composed_app(clean.clone(), &mut policy).is_err());
// An inline script is refused even with no `language`, which the locking traversal skips.
let mut policy = builder_policy(serde_json::json!({}));
assert!(composed_app(
serde_json::json!({"runnables": {"a": {"inlineScript": {"content": "x"}}}}),
&mut policy
)
.is_err());
// A `rawscript/<sha>` triggerable is what authorizes caller-supplied `raw_code` on the
// deployed app, so pinning one would be arbitrary code execution behind a clean value.
for key in ["rawscript/abc", "a:rawscript/abc"] {
let mut policy = builder_policy(serde_json::json!({ key: {"static_inputs": {}, "one_of_inputs": {}} }));
assert!(
composed_app(clean.clone(), &mut policy).is_err(),
"{key} must be refused"
);
}
// Low-code apps and kind conversion stay closed.
let value = to_raw_value(&clean);
let mut policy = builder_policy(serde_json::json!({}));
assert!(check_operator_composed_app(false, Some(&value), Some(&mut policy), false).is_err());
assert!(check_operator_composed_app(true, Some(&value), Some(&mut policy), true).is_err());
}
}
+15 -5
View File
@@ -8675,6 +8675,7 @@ pub struct RunFlowDependenciesResponse {
async fn push_flow_dependencies_job(
authed: &ApiAuthed,
db: &DB,
user_db: &UserDB,
w_id: &str,
req: RunFlowDependenciesRequest,
) -> error::Result<Uuid> {
@@ -8684,7 +8685,7 @@ async fn push_flow_dependencies_job(
// composition-only flow has none, so validating here costs a builder nothing and keeps the
// lock step from becoming the way to run code the write path refuses.
if authed.is_operator {
validate_operator_composed_flow(&req.flow_value, &None, authed, db, w_id).await?;
validate_operator_composed_flow(&req.flow_value, &None, authed, db, user_db, w_id).await?;
}
if req.raw_deps.is_some() {
@@ -8750,20 +8751,22 @@ async fn push_flow_dependencies_job(
async fn run_flow_dependencies_job(
authed: ApiAuthed,
Extension(db): Extension<DB>,
Extension(user_db): Extension<UserDB>,
Path(w_id): Path<String>,
Json(req): Json<RunFlowDependenciesRequest>,
) -> error::Result<Response> {
let uuid = push_flow_dependencies_job(&authed, &db, &w_id, req).await?;
let uuid = push_flow_dependencies_job(&authed, &db, &user_db, &w_id, req).await?;
run_wait_result(&db, uuid, &w_id, None, false, &authed.username).await
}
async fn run_flow_dependencies_job_async(
authed: ApiAuthed,
Extension(db): Extension<DB>,
Extension(user_db): Extension<UserDB>,
Path(w_id): Path<String>,
Json(req): Json<RunFlowDependenciesRequest>,
) -> error::Result<(StatusCode, String)> {
let uuid = push_flow_dependencies_job(&authed, &db, &w_id, req).await?;
let uuid = push_flow_dependencies_job(&authed, &db, &user_db, &w_id, req).await?;
Ok((StatusCode::CREATED, uuid.to_string()))
}
@@ -9072,8 +9075,15 @@ async fn run_preview_flow_job(
// A builder must be able to test what it composes, but the submitted value is not the stored
// one: without this the preview is a way to run inline code the write path refuses.
if authed.is_operator {
validate_operator_composed_flow(&raw_flow.value, &raw_flow.tag, &authed, &db, &w_id)
.await?;
validate_operator_composed_flow(
&raw_flow.value,
&raw_flow.tag,
&authed,
&db,
&user_db,
&w_id,
)
.await?;
}
let scheduled_for = run_query.get_scheduled_for(&db).await?;
let tag = run_query.tag.clone().or(raw_flow.tag.clone());
+16
View File
@@ -18,6 +18,22 @@ lazy_static::lazy_static! {
pub static ref APP_WORKSPACED_ROUTE: AtomicBool = AtomicBool::new(false);
}
/// Whether the app value carries an `inlineScript` anywhere.
///
/// Deliberately not built on [`traverse_app_inline_scripts`], which only reports a script whose
/// `language` parses and stops descending as soon as it sees an `inlineScript` key. That is right
/// for locking (nothing to lock without a language) and wrong for an authorization check, where
/// the author picks the fields: this refuses the key itself, whatever it contains.
pub fn app_value_has_inline_script(value: &Value) -> bool {
match value {
Value::Object(object) => {
object.contains_key("inlineScript") || object.values().any(app_value_has_inline_script)
}
Value::Array(array) => array.iter().any(app_value_has_inline_script),
_ => false,
}
}
/// Traverse FlowValue while invoking provided by caller callback on leafs
// #[async_recursion::async_recursion(?Send)]
pub fn traverse_app_inline_scripts<
+50 -20
View File
@@ -17,6 +17,7 @@ use crate::{
cache::{self, FlowExtras},
db::DB,
error::{to_anyhow, Error},
scripts::ScriptHash,
utils::{http_get_from_hub, StripPath},
worker::{to_raw_value, Connection},
DEFAULT_HUB_BASE_URL, HUB_BASE_URL, PRIVATE_HUB_MIN_VERSION,
@@ -250,35 +251,46 @@ pub async fn resolve_modules(
/// all an operator with builder rights may author. Walks the modules, the preprocessor and failure
/// modules, every branch, and the `tools` of an AI agent step.
///
/// Returns the worker tags its steps pin, for the caller to authorize against its own scope: a tag
/// is how a step picks the worker group it runs on.
pub fn check_flow_is_composition_only(value: &FlowValue) -> Result<Vec<String>, Error> {
let mut tags = Vec::new();
/// 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.
pub fn check_flow_is_composition_only(value: &FlowValue) -> Result<ComposedFlowRefs, Error> {
let mut refs = ComposedFlowRefs::default();
for module in value
.modules
.iter()
.chain(value.preprocessor_module.as_deref())
.chain(value.failure_module.as_deref())
{
check_module_is_composition_only(module, &mut tags)?;
check_module_is_composition_only(module, &mut refs)?;
}
Ok(tags)
Ok(refs)
}
/// What [`check_flow_is_composition_only`] collects for the caller to authorize.
#[derive(Default)]
pub struct ComposedFlowRefs {
pub tags: Vec<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.
pub pinned_scripts: Vec<(String, ScriptHash)>,
}
fn check_module_is_composition_only(
module: &FlowModule,
tags: &mut Vec<String>,
refs: &mut ComposedFlowRefs,
) -> Result<(), Error> {
let value = module
.get_value()
.map_err(|e| Error::BadRequest(format!("Step {} could not be read: {e}", module.id)))?;
check_module_value_is_composition_only(&value, &module.id, tags)
check_module_value_is_composition_only(&value, &module.id, refs)
}
fn check_module_value_is_composition_only(
value: &FlowModuleValue,
id: &str,
tags: &mut Vec<String>,
refs: &mut ComposedFlowRefs,
) -> Result<(), Error> {
let refuse = |what: &str| {
Err(Error::NotAuthorized(format!(
@@ -297,7 +309,7 @@ fn check_module_value_is_composition_only(
};
let mut push_tag = |tag: &Option<String>| {
if let Some(tag) = tag.as_deref().filter(|t| !t.is_empty()) {
tags.push(tag.to_string());
refs.tags.push(tag.to_string());
}
};
@@ -307,27 +319,30 @@ fn check_module_value_is_composition_only(
return refuse("references code stored outside the flow")
}
FlowModuleValue::Identity => {}
FlowModuleValue::Script { path, tag_override, .. } => {
FlowModuleValue::Script { path, hash, tag_override, .. } => {
check_composable_path(path, id)?;
push_tag(tag_override);
if let Some(hash) = hash {
refs.pinned_scripts.push((path.clone(), *hash));
}
}
FlowModuleValue::Flow { path, .. } => check_composable_path(path, id)?,
FlowModuleValue::ForloopFlow { modules, modules_node, .. }
| FlowModuleValue::WhileloopFlow { modules, modules_node, .. } => {
refuse_node(modules_node)?;
for module in modules {
check_module_is_composition_only(module, tags)?;
check_module_is_composition_only(module, refs)?;
}
}
FlowModuleValue::BranchOne { branches, default, default_node } => {
refuse_node(default_node)?;
for module in default {
check_module_is_composition_only(module, tags)?;
check_module_is_composition_only(module, refs)?;
}
check_branches_are_composition_only(branches, id, tags)?;
check_branches_are_composition_only(branches, id, refs)?;
}
FlowModuleValue::BranchAll { branches, .. } => {
check_branches_are_composition_only(branches, id, tags)?
check_branches_are_composition_only(branches, id, refs)?
}
FlowModuleValue::AIAgent { tools, tag, agent, .. } => {
// A linked agent resolves its tools from an `ai_agent` resource at run time, and
@@ -339,7 +354,7 @@ fn check_module_value_is_composition_only(
push_tag(tag);
for tool in tools {
if let ToolValue::FlowModule(value) = &tool.value {
check_module_value_is_composition_only(value, &tool.id, tags)?;
check_module_value_is_composition_only(value, &tool.id, refs)?;
}
}
}
@@ -350,7 +365,7 @@ fn check_module_value_is_composition_only(
fn check_branches_are_composition_only(
branches: &[Branch],
id: &str,
tags: &mut Vec<String>,
refs: &mut ComposedFlowRefs,
) -> Result<(), Error> {
for branch in branches {
if branch.modules_node.is_some() {
@@ -361,7 +376,7 @@ fn check_branches_are_composition_only(
)));
}
for module in &branch.modules {
check_module_is_composition_only(module, tags)?;
check_module_is_composition_only(module, refs)?;
}
}
Ok(())
@@ -419,7 +434,7 @@ mod tests {
#[test]
fn composition_check_accepts_a_composed_flow_and_collects_its_tags() {
let tags = check_flow_is_composition_only(&flow(serde_json::json!({"modules": [{
let refs = check_flow_is_composition_only(&flow(serde_json::json!({"modules": [{
"id": "a",
"value": {"type": "forloopflow", "iterator": {"type": "static", "value": []},
"parallel": false, "modules": [
@@ -431,7 +446,7 @@ mod tests {
]}
}]})))
.unwrap();
assert_eq!(tags, vec!["gpu".to_string(), "ai".to_string()]);
assert_eq!(refs.tags, vec!["gpu".to_string(), "ai".to_string()]);
}
/// The walk covers `modules`, so a node reference is a way past it: it names code hoisted
@@ -480,6 +495,21 @@ mod tests {
}
}
/// A step carrying a `hash` dispatches on that hash alone: the caller must verify the pair
/// exists and is readable, so the walk has to surface it rather than pass it through.
#[test]
fn composition_check_reports_version_pinned_steps() {
let refs = check_flow_is_composition_only(&flow(serde_json::json!({"modules": [
{"id": "a", "value": {"type": "script", "path": "f/x/s", "hash": "000000000000007b"}},
{"id": "b", "value": {"type": "script", "path": "f/x/t"}}
]})))
.unwrap();
assert_eq!(
refs.pinned_scripts,
vec![("f/x/s".to_string(), ScriptHash(123))]
);
}
#[test]
fn composition_check_rejects_hub_runnables() {
for kind in ["script", "flow"] {
+21 -4
View File
@@ -5,8 +5,12 @@ compose flows and full-code apps out of runnables that already exist. It does no
authors: the boundary the operator role draws is **authoring code and running arbitrary code**,
and builder rights do not move it.
Read the flag with `windmill_common::workspaces::operator_builder_enabled` (60s cache, invalidated
on write by `update_operator_settings`). Gate a write with `check_operator_can_build`.
Read the flag with `windmill_common::workspaces::operator_builder_enabled` (60s cache). Gate a
write with `check_operator_can_build`. The cache is per process, so revoking the setting has to
reach every replica: an `AFTER UPDATE OF operator_settings` trigger writes a
`notify_operator_settings_change` row and `process_notify_event` drops the entry. Keep both ends
if you touch either, or a revoked workspace keeps authorizing writes on every other replica until
its own entry expires.
## What the check has to cover
@@ -23,8 +27,14 @@ anything that carries code. Three of its rules exist because the obvious walk mi
resource at run time, and operators may write resources, so the tool list is outside this check
and can be swapped for a raw script after the flow is approved.
It also returns the worker tags the steps pin. Authorize them (`check_tag_available_for_workspace_internal`)
or a builder routes a job onto a privileged worker group.
It also returns what a value-only walk cannot authorize, for the caller to check against its own
permissions:
- **the worker tags the steps pin**, or a builder routes a job onto a privileged worker group;
- **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.
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
@@ -45,6 +55,13 @@ A builder-authored app is forced to `policy.sandbox = true` (`check_operator_com
is what makes it safe to let an operator publish a bundle nobody reviewed: without it the bundle
runs same-origin with each viewer's Windmill session.
The same check refuses a `rawscript/<sha>` key in either triggerables map. That key is the
deployed app's authorization to run caller-supplied `raw_code` hashing to it (`execute_component`'s
run mode authorizes inline code by the policy alone; its operator guard only covers preview mode),
so pinning one would be arbitrary code execution behind a value that passed the inline-script
check. It also uses `app_value_has_inline_script` rather than `traverse_app_inline_scripts`: the
latter only reports a script whose `language` parses, and the author picks the fields.
## Accepted risks
- A builder raw app may declare `frontend_sdk_scopes`, and `mint_raw_app_sdk_token` mints as the
@@ -25,6 +25,7 @@
import { importStore } from '$lib/components/apps/store'
import { conditionalMelt, getLocalSetting, storeLocalSetting } from '$lib/utils'
import { operatorBuilderRights } from '$lib/stores'
import { untrack } from 'svelte'
import { createDropdownMenu, melt } from '@melt-ui/svelte'
import YAML from 'yaml'
@@ -230,13 +231,17 @@
// A builder composes runnables that already exist, so only the two kinds it may author are
// offered. Everything else here writes code, which the backend refuses from an operator.
const options: Option[] = $operatorBuilderRights
? allOptions.filter((o) => o.key === 'flow' || o.key === 'app-fullcode')
: allOptions
// Derived, not computed once: switching workspace only sets `workspaceStore`, it does not
// remount this component, so a snapshot would keep the previous workspace's kinds.
const options: Option[] = $derived(
$operatorBuilderRights
? allOptions.filter((o) => o.key === 'flow' || o.key === 'app-fullcode')
: allOptions
)
let activeKey = $state(options[0]?.key)
let activeKey = $state(untrack(() => options)[0]?.key)
// every option's import action, surfaced together under the bottom "Import" submenu
const importActions: Extra[] = options.flatMap((o) => o.extras ?? [])
const importActions: Extra[] = $derived(options.flatMap((o) => o.extras ?? []))
// melt dropdown menu: arrow-key nav, typeahead, focus management and outside/escape
// close all come for free; we only drive the doc panel off the highlighted item.