diff --git a/backend/tests/app_guest_execution_mode.rs b/backend/tests/app_guest_execution_mode.rs index e3ddb7a868..ec6e307495 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(()) } @@ -533,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-auth/src/scopes.rs b/backend/windmill-api-auth/src/scopes.rs index 6584489f5c..26e7237511 100644 --- a/backend/windmill-api-auth/src/scopes.rs +++ b/backend/windmill-api-auth/src/scopes.rs @@ -798,9 +798,13 @@ pub fn with_guest_sentinel(mut scopes: Vec) -> Vec { /// grants. A guest has no `usr` row, so this list is the whole of what it can do. The /// single source both the mint (a signed-in guest) and the JWT auth arm build from. pub fn guest_session_scopes(app_path: &str) -> windmill_common::error::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)?; + // The path is spliced into a scope, whose grammar reserves `:`, `,`, `*` and a leading + // `/`; app paths may otherwise carry spaces and `@`, so guard only those reserved chars. + if !windmill_common::auth::is_scope_literal_path(app_path) { + return Err(windmill_common::error::Error::BadRequest(format!( + "app path {app_path} cannot be scoped: `:`, `,` and `*` are reserved in scopes" + ))); + } Ok(vec![ 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 790bcd9deb..fa0339992f 100644 --- a/backend/windmill-api/src/apps.rs +++ b/backend/windmill-api/src/apps.rs @@ -329,6 +329,19 @@ fn deployment_rule_for_mode(mode: ExecutionMode) -> Option { } } +/// 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!( + "app {path} cannot be set to Guests: `:`, `,` and `*` in a path cannot be scoped" + ))); + } + Ok(()) +} + /// 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 @@ -2503,6 +2516,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, @@ -3345,6 +3359,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 { @@ -3524,6 +3556,10 @@ async fn update_app_internal<'a>( .unwrap_or_default(), ); } + 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 cfdf9cd601..b51186f464 100644 --- a/backend/windmill-common/src/auth.rs +++ b/backend/windmill-common/src/auth.rs @@ -81,6 +81,16 @@ 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, `@`). 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.starts_with('/') + && !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/guest_jwt.rs b/backend/windmill-common/src/guest_jwt.rs index 2b5cb82ead..6b2e5c4a42 100644 --- a/backend/windmill-common/src/guest_jwt.rs +++ b/backend/windmill-common/src/guest_jwt.rs @@ -133,7 +133,7 @@ pub fn jwk_algorithms(jwk: &Jwk) -> Option> { /// Verify `token` against `key`, honouring only the accepted `algorithms`, and check /// every claim rule that needs no database: signature, `exp` (mandatory), `nbf` and /// `iat` when present, the lifetime cap, that the token names `w_id`, that `email` is a -/// valid address bounded to 254 bytes, and that `app_path` is a canonical path. +/// valid address bounded to 254 bytes, and that `app_path` carries no scope metacharacter. pub fn verify( token: &str, key: &DecodingKey, @@ -180,10 +180,13 @@ pub fn verify( )); } // The app path is spliced into `apps:read:` and `apps:run:` scopes, whose - // parser reads `,` as a separator and `*` as a wildcard; require a canonical path, the - // same guard `guest_session_scopes` applies at the mint. - crate::utils::check_proper_path(&claims.app_path) - .map_err(|e| Error::NotAuthorized(format!("guest JWT refused: {e}")))?; + // grammar reserves `:`, `,`, `*` and a leading `/`; refuse those, the same guard + // `guest_session_scopes` applies at the mint. App paths may carry spaces and `@`. + if !crate::auth::is_scope_literal_path(&claims.app_path) { + return Err(Error::NotAuthorized( + "guest JWT refused: app_path contains a character reserved in scopes".to_string(), + )); + } Ok(claims) } 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();