From 0241ba52b483e274b284682c06231f7dd771fa7c Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Fri, 4 Sep 2026 09:32:26 +0200 Subject: [PATCH] fix: the deploy-time guest path guard checks the destination of a rename and refuses a leading slash Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01BayTppRCstWX6qTf3LMco5 --- backend/tests/app_guest_execution_mode.rs | 45 +++++++++++++++++++++++ backend/windmill-api/src/apps.rs | 40 +++++++++++++++----- backend/windmill-common/src/auth.rs | 7 +++- 3 files changed, 81 insertions(+), 11 deletions(-) diff --git a/backend/tests/app_guest_execution_mode.rs b/backend/tests/app_guest_execution_mode.rs index 6cd0eed98c..ec6e307495 100644 --- a/backend/tests/app_guest_execution_mode.rs +++ b/backend/tests/app_guest_execution_mode.rs @@ -549,6 +549,51 @@ async fn a_guest_cannot_read_a_job_it_did_not_launch(db: Pool) -> anyh Ok(()) } +/// Guests mode cannot land on a path the scope grammar cannot hold, however it gets +/// there: set at creation, set on update, or a rename of an app already in that mode. +#[sqlx::test(fixtures("base"))] +async fn guests_mode_needs_a_scopable_path(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + let ws = format!("http://localhost:{port}/api/w/test-workspace"); + + let resp = authed(client().post(format!("{ws}/apps/create")), ADMIN_TOKEN) + .json(&guest_app_with_runnable("u/test-user/a:b", false)) + .send() + .await?; + assert_eq!(resp.status(), 400, "created into Guests on a `:` path"); + + let resp = authed(client().post(format!("{ws}/apps/create")), ADMIN_TOKEN) + .json(&guest_app_with_runnable(APP_PATH, false)) + .send() + .await?; + assert_eq!(resp.status(), 201, "{}", resp.text().await?); + let resp = authed( + client().post(format!("{ws}/apps/update/{APP_PATH}")), + ADMIN_TOKEN, + ) + .json(&json!({ "path": "u/test-user/a,b" })) + .send() + .await?; + assert_eq!(resp.status(), 400, "renamed to a `,` path while in Guests"); + let resp = authed( + client().post(format!("{ws}/apps/update/{APP_PATH}")), + ADMIN_TOKEN, + ) + .json(&json!({ "path": "u/test-user/My App" })) + .send() + .await?; + assert_eq!( + resp.status(), + 200, + "a space is literal: {}", + resp.text().await? + ); + + Ok(()) +} + /// The superadmin switch sits above every workspace's: off, no guest session stands and /// no app discovers as open, whatever the workspace and the app say. #[sqlx::test(fixtures("base"))] diff --git a/backend/windmill-api/src/apps.rs b/backend/windmill-api/src/apps.rs index 6823e9cd96..fc5853adbc 100644 --- a/backend/windmill-api/src/apps.rs +++ b/backend/windmill-api/src/apps.rs @@ -321,9 +321,18 @@ impl ExecutionMode { /// The protection rule gating a *transition into* `mode`, if any. Anonymous and /// guest each widen who may open an app past the workspace's own members, so each /// carries its own rule; the two member-only modes are ungated. +fn deployment_rule_for_mode(mode: ExecutionMode) -> Option { + match mode { + ExecutionMode::Anonymous => Some(ProtectionRuleKind::RestrictAnonymousAppDeployment), + ExecutionMode::Guest => Some(ProtectionRuleKind::RestrictGuestAppDeployment), + ExecutionMode::Publisher | ExecutionMode::Viewer => None, + } +} + /// A guest session is scoped to its app by path, so an app whose path the scope /// grammar cannot hold as one literal (`is_scope_literal_path`) can never admit a /// guest; refuse the mode at deploy time rather than advertise an app nobody enters. +/// `path` is where the app ends up: on a rename, the destination. fn refuse_unscopable_guest_app(path: &str, mode: ExecutionMode) -> Result<()> { if matches!(mode, ExecutionMode::Guest) && !windmill_common::auth::is_scope_literal_path(path) { return Err(Error::BadRequest(format!( @@ -333,14 +342,6 @@ fn refuse_unscopable_guest_app(path: &str, mode: ExecutionMode) -> Result<()> { Ok(()) } -fn deployment_rule_for_mode(mode: ExecutionMode) -> Option { - match mode { - ExecutionMode::Anonymous => Some(ProtectionRuleKind::RestrictAnonymousAppDeployment), - ExecutionMode::Guest => Some(ProtectionRuleKind::RestrictGuestAppDeployment), - ExecutionMode::Publisher | ExecutionMode::Viewer => None, - } -} - /// Gate a viewer on the app's `execution_mode`, as far as can be decided without an /// ACL probe. `Ok(true)` means already authorized — anonymous admits anyone, guest /// admits anyone signed in; `Ok(false)` means the caller is a member and still owes @@ -3354,6 +3355,24 @@ async fn update_app_internal<'a>( // the token's write scope, not just the source path. if let Some(npath) = ns.path.as_deref() { check_scopes(&authed, || format!("apps:write:{}", npath))?; + // The destination is what a guest session would be scoped to; a rename that + // carries no policy keeps the deployed mode. + let mode = match ns.policy.as_ref().and_then(|p| p.stated_execution_mode()) { + Some(mode) => mode, + None => sqlx::query_scalar::<_, Option>( + "SELECT policy->>'execution_mode' FROM app WHERE workspace_id = $1 AND path = $2", + ) + .bind(w_id) + .bind(path) + .fetch_optional(&db) + .await? + .flatten() + .and_then(|m| { + serde_json::from_value::(serde_json::Value::String(m)).ok() + }) + .unwrap_or_default(), + }; + refuse_unscopable_guest_app(npath, mode)?; } if raw_app { @@ -3533,7 +3552,10 @@ async fn update_app_internal<'a>( .unwrap_or_default(), ); } - refuse_unscopable_guest_app(path, npolicy.execution_mode())?; + refuse_unscopable_guest_app( + ns.path.as_deref().unwrap_or(path), + npolicy.execution_mode(), + )?; if let Some(rule) = deployment_rule_for_mode(npolicy.execution_mode()).filter(|_| !authed.is_admin) { diff --git a/backend/windmill-common/src/auth.rs b/backend/windmill-common/src/auth.rs index cba28fd3ed..b51186f464 100644 --- a/backend/windmill-common/src/auth.rs +++ b/backend/windmill-common/src/auth.rs @@ -83,9 +83,12 @@ pub fn is_guest_session_label(label: Option<&str>) -> bool { /// Whether `path` can be spliced into a scope as one literal resource. The scope /// grammar reserves three characters: `:` separates the parts, `,` separates -/// resources, `*` is a wildcard. App paths are otherwise free-form (spaces, `@`). +/// resources, `*` is a wildcard. App paths are otherwise free-form (spaces, `@`). A +/// leading `/` is refused too: routes strip it, so the scope would never match. pub fn is_scope_literal_path(path: &str) -> bool { - !path.is_empty() && !path.chars().any(|c| matches!(c, ':' | ',' | '*')) + !path.is_empty() + && !path.starts_with('/') + && !path.chars().any(|c| matches!(c, ':' | ',' | '*')) } /// Whether `label` is the one minted for a browser session at login. [`is_server_minted_label`]