From 47dbd96a92f9ef891a4b3e9759e759a299dba2e9 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Fri, 4 Sep 2026 09:21:16 +0200 Subject: [PATCH] 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();