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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BayTppRCstWX6qTf3LMco5

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<Postgres>) -> 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<Postgres>,
 ) -> 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(())
 }
 

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<Vec<String>> {
-    // 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(),

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<ProtectionRuleKind> {
     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)
             {

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.

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<bool> {
     // 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();