From 47dbd96a92f9ef891a4b3e9759e759a299dba2e9 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Fri, 4 Sep 2026 09:21:16 +0200 Subject: [PATCH 1/2] fix: only the scope grammar's own characters bar an app path from guests, refused at deploy as well as at the mint Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01BayTppRCstWX6qTf3LMco5 --- backend/tests/app_guest_execution_mode.rs | 50 +++++++++++++++-------- backend/windmill-api-users/src/users.rs | 8 ++-- backend/windmill-api/src/apps.rs | 14 +++++++ backend/windmill-common/src/auth.rs | 7 ++++ backend/windmill-common/src/workspaces.rs | 2 +- 5 files changed, 60 insertions(+), 21 deletions(-) diff --git a/backend/tests/app_guest_execution_mode.rs b/backend/tests/app_guest_execution_mode.rs index e3ddb7a868..6cd0eed98c 100644 --- a/backend/tests/app_guest_execution_mode.rs +++ b/backend/tests/app_guest_execution_mode.rs @@ -8,9 +8,10 @@ //! could type into `users/tokens/create`; //! * the confinement — a guest reaches the one app it was let in for and nothing //! else; -//! * the two gates — an app's own `execution_mode: guest` is inert unless the -//! workspace switch is on, checked at the door rather than only where a policy -//! is written (git-sync and the CLI push policies past every UI). +//! * the switches — an app's own `execution_mode: guest` is inert unless the +//! workspace and the instance allow guests, checked at the door rather than only +//! where a policy is written (git-sync and the CLI push policies past every UI); +//! the allowance on top of them has a binary of its own. //! //! The token is inserted directly: how a guest session is minted is the identity //! provider's business (EE), what one can do is this file's. @@ -454,33 +455,48 @@ async fn guest_cannot_run_another_guest_app(db: Pool) -> anyhow::Resul Ok(()) } -/// The app path is spliced into the session's scopes, whose parser splits resources on -/// `,` and reads `*` as a wildcard: a path carrying either would scope the guest to more -/// than the one app it was let in for, so the mint refuses it before anything else. +/// The app path is spliced into the session's scopes, whose grammar reserves `:`, `,` +/// and `*`: a path carrying one would scope the guest to more than the one app it was +/// let in for, so the mint refuses it before anything else. Anything else in a path +/// (spaces, `@`) is literal to that grammar and stays admissible. #[sqlx::test(fixtures("base"))] async fn a_scope_metacharacter_in_the_app_path_is_refused( db: Pool, ) -> anyhow::Result<()> { initialize_tracing().await; + let mint = |path: &'static str| { + let db = db.clone(); + async move { + let mut tx = db.begin().await?; + let minted = windmill_api_users::users::create_guest_session_token( + "guest@example.com", + "test-workspace", + path, + &mut tx, + tower_cookies::Cookies::default(), + ) + .await; + anyhow::Ok(minted) + } + }; for path in [ "u/test-user/entry,u/test-user/hidden", "u/test-user/*", - "u/test-user/a b", + "u/test-user/entry:run", ] { - let mut tx = db.begin().await?; - let minted = windmill_api_users::users::create_guest_session_token( - "guest@example.com", - "test-workspace", - path, - &mut tx, - tower_cookies::Cookies::default(), - ) - .await; + let minted = mint(path).await?; assert!( - matches!(minted, Err(windmill_common::error::Error::BadRequest(ref m)) if m.contains("Invalid path")), + matches!(minted, Err(windmill_common::error::Error::BadRequest(ref m)) if m.contains("cannot be scoped")), "{path}: {minted:?}" ); } + for path in ["u/test-user/My App", "u/admin@windmill.dev/x"] { + let minted = mint(path).await?; + assert!( + !matches!(minted, Err(windmill_common::error::Error::BadRequest(ref m)) if m.contains("cannot be scoped")), + "{path} is literal to the scope grammar and must get past the guard: {minted:?}" + ); + } Ok(()) } diff --git a/backend/windmill-api-users/src/users.rs b/backend/windmill-api-users/src/users.rs index ba565c6376..31f8dea2ee 100644 --- a/backend/windmill-api-users/src/users.rs +++ b/backend/windmill-api-users/src/users.rs @@ -2946,9 +2946,11 @@ lazy_static::lazy_static! { /// The `guest` sentinel here only narrows. What makes the session a guest at all is the /// server-minted label ([`windmill_common::auth::GUEST_SESSION_LABEL`]). fn guest_session_scopes(app_path: &str) -> Result> { - // The path is spliced into a scope, whose parser reads `,` as a resource separator - // and `*` as a wildcard; a canonical path carries neither. - windmill_common::utils::check_proper_path(app_path)?; + if !windmill_common::auth::is_scope_literal_path(app_path) { + return Err(Error::BadRequest(format!( + "app path {app_path} cannot be scoped: `:`, `,` and `*` are reserved in scopes" + ))); + } Ok(vec![ windmill_api_auth::scopes::GUEST_SENTINEL.to_string(), "jobs:read".to_string(), diff --git a/backend/windmill-api/src/apps.rs b/backend/windmill-api/src/apps.rs index ba11f1a033..6823e9cd96 100644 --- a/backend/windmill-api/src/apps.rs +++ b/backend/windmill-api/src/apps.rs @@ -321,6 +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. +/// 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. +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!( + "app {path} cannot be set to Guests: `:`, `,` and `*` in a path cannot be scoped" + ))); + } + Ok(()) +} + fn deployment_rule_for_mode(mode: ExecutionMode) -> Option { match mode { ExecutionMode::Anonymous => Some(ProtectionRuleKind::RestrictAnonymousAppDeployment), @@ -2499,6 +2511,7 @@ async fn create_app_internal<'a>( // Pin the mode the app is created under, so the stored policy states one // even when the caller did not. app.policy.set_execution_mode(app.policy.execution_mode()); + refuse_unscopable_guest_app(&app.path, app.policy.execution_mode())?; if let Some(rule) = deployment_rule_for_mode(app.policy.execution_mode()) { if let RuleCheckResult::Blocked(msg) = check_user_against_rule( w_id, @@ -3520,6 +3533,7 @@ async fn update_app_internal<'a>( .unwrap_or_default(), ); } + refuse_unscopable_guest_app(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 cfdf9cd601..cba28fd3ed 100644 --- a/backend/windmill-common/src/auth.rs +++ b/backend/windmill-common/src/auth.rs @@ -81,6 +81,13 @@ pub fn is_guest_session_label(label: Option<&str>) -> bool { label == Some(GUEST_SESSION_LABEL) } +/// 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, `@`). +pub fn is_scope_literal_path(path: &str) -> bool { + !path.is_empty() && !path.chars().any(|c| matches!(c, ':' | ',' | '*')) +} + /// Whether `label` is the one minted for a browser session at login. [`is_server_minted_label`] /// stops a member minting it directly, but `/users/refresh_token` hands one to any authenticated /// caller, so this attributes a request to the UI without proving it: never gate authority on it. diff --git a/backend/windmill-common/src/workspaces.rs b/backend/windmill-common/src/workspaces.rs index dc535e3d51..b6d10cb7df 100644 --- a/backend/windmill-common/src/workspaces.rs +++ b/backend/windmill-common/src/workspaces.rs @@ -934,7 +934,7 @@ pub async fn guest_app_admits<'c, E: sqlx::Executor<'c, Database = sqlx::Postgre app_path: &str, ) -> Result { // The mint refuses a path it cannot scope, so discovery must not advertise one. - if crate::utils::check_proper_path(app_path).is_err() { + if !crate::auth::is_scope_literal_path(app_path) { return Ok(false); } let instance_admits = instance_admits_guests_sql(); From 0241ba52b483e274b284682c06231f7dd771fa7c Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Fri, 4 Sep 2026 09:32:26 +0200 Subject: [PATCH 2/2] 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`]