From fbcb0728f791a4ce016e292d83c6b30e111e93ee Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Fri, 4 Sep 2026 08:52:20 +0200 Subject: [PATCH] fix: a guest app path is refused at the mint if it could widen the scope; the instance toggle waits for its reload Co-Authored-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_01BayTppRCstWX6qTf3LMco5 --- backend/tests/app_guest_execution_mode.rs | 42 ++++++++++++++++++- backend/windmill-api-users/src/users.rs | 23 ++++++++-- backend/windmill-api/openapi.yaml | 11 ++--- .../instanceSettings/GuestActivityList.svelte | 7 ++-- 4 files changed, 69 insertions(+), 14 deletions(-) diff --git a/backend/tests/app_guest_execution_mode.rs b/backend/tests/app_guest_execution_mode.rs index db1169169d..f3249b48cc 100644 --- a/backend/tests/app_guest_execution_mode.rs +++ b/backend/tests/app_guest_execution_mode.rs @@ -454,6 +454,36 @@ 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. +#[sqlx::test(fixtures("base"))] +async fn a_scope_metacharacter_in_the_app_path_is_refused( + db: Pool, +) -> anyhow::Result<()> { + initialize_tracing().await; + for path in [ + "u/test-user/entry,u/test-user/hidden", + "u/test-user/*", + "u/test-user/a b", + ] { + 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; + assert!( + matches!(minted, Err(windmill_common::error::Error::BadRequest(ref m)) if m.contains("cannot be scoped")), + "{path}: {minted:?}" + ); + } + 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"))] @@ -494,12 +524,20 @@ async fn the_instance_switch_closes_every_workspace(db: Pool) -> anyho let resp = authed(client().get(format!("{ws}/users/whoami")), GUEST_TOKEN) .send() .await?; - assert_eq!(resp.status(), 401, "the instance switch closes an issued session"); + assert_eq!( + resp.status(), + 401, + "the instance switch closes an issued session" + ); let resp = client() .get(format!("{ws}/apps_u/guest_entry/{secret}")) .send() .await?; - assert_eq!(resp.status(), 404, "and nothing discovers as open to guests"); + assert_eq!( + resp.status(), + 404, + "and nothing discovers as open to guests" + ); set_instance_switch(false).await?.error_for_status()?; let resp = authed(client().get(format!("{ws}/users/whoami")), GUEST_TOKEN) diff --git a/backend/windmill-api-users/src/users.rs b/backend/windmill-api-users/src/users.rs index c62311c40f..73ea69ce54 100644 --- a/backend/windmill-api-users/src/users.rs +++ b/backend/windmill-api-users/src/users.rs @@ -2945,8 +2945,23 @@ 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) -> Vec { - vec![ +pub 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 path carrying either would name more than one app. + let canonical = app_path.split('/').count() >= 3 + && app_path.split('/').all(|seg| { + !seg.is_empty() + && seg + .chars() + .all(|c| c.is_ascii_alphanumeric() || "_-.".contains(c)) + }); + if !canonical { + return Err(Error::BadRequest(format!( + "app path {app_path} cannot be scoped: only letters, digits, `_`, `-` and `.` \ + in `/`-separated segments" + ))); + } + Ok(vec![ windmill_api_auth::scopes::GUEST_SENTINEL.to_string(), "jobs:read".to_string(), "resources:run".to_string(), @@ -2954,7 +2969,7 @@ fn guest_session_scopes(app_path: &str) -> Vec { "folders:read".to_string(), format!("apps:read:{app_path}"), format!("apps:run:{app_path}"), - ] + ]) } /// Mint a browser session for someone the identity provider authenticated who is a @@ -2988,7 +3003,7 @@ pub async fn create_guest_session_token<'c>( } else { Some(&token) }; - let scopes = guest_session_scopes(app_path); + let scopes = guest_session_scopes(app_path)?; // No account at all (see `ExecutionMode::Guest`): a deactivated `password` row // counts, since the sign-in path's own lookup filters on `disabled = false` and a diff --git a/backend/windmill-api/openapi.yaml b/backend/windmill-api/openapi.yaml index 6aed163961..a2fe3ee19b 100644 --- a/backend/windmill-api/openapi.yaml +++ b/backend/windmill-api/openapi.yaml @@ -8901,8 +8901,8 @@ paths: description: >- The custom-path counterpart of `getGuestEntry`. Unauthenticated; 404 unless the app's execution mode is `guest` AND its workspace has - `guest_access_enabled`. Returns the workspace too, since a custom URL may not - carry it. + `guest_access_enabled` AND the instance has not set `guest_access_disabled`. + Returns the workspace too, since a custom URL may not carry it. operationId: getGuestEntryByCustomPath tags: - app @@ -13082,9 +13082,10 @@ paths: description: >- Unauthenticated: what a signed-out visitor reads to learn that signing in would let them in. 404 unless the app's execution mode is `guest` AND the - workspace has `guest_access_enabled`, so it says nothing about apps that are - not open to guests. Discloses only the app path, to a caller already holding - the share secret. + workspace has `guest_access_enabled` AND the instance has not set the + `guest_access_disabled` global setting, so it says nothing about apps that + are not open to guests. Discloses only the app path, to a caller already + holding the share secret. operationId: getGuestEntry tags: - app diff --git a/frontend/src/lib/components/instanceSettings/GuestActivityList.svelte b/frontend/src/lib/components/instanceSettings/GuestActivityList.svelte index 80655a6a67..000ca4eace 100644 --- a/frontend/src/lib/components/instanceSettings/GuestActivityList.svelte +++ b/frontend/src/lib/components/instanceSettings/GuestActivityList.svelte @@ -14,8 +14,9 @@ hasMore: boolean loading: boolean onLoadMore: () => void - /** The instance switch was written; the caller re-reads usage. */ - onInstanceSwitch: () => void + /** The instance switch was written; the caller re-reads usage and resolves once + * the toggle may show the stored value again. */ + onInstanceSwitch: () => Promise } let { usage, guests, hasMore, loading, onLoadMore, onInstanceSwitch }: Props = $props() @@ -39,8 +40,8 @@ } catch (e) { sendUserToast(`Could not change the instance guest switch: ${e}`, true) } + await onInstanceSwitch() switchPending = false - onInstanceSwitch() } // A capped instance refuses the next stranger as soon as the allowance is used up. let pastAllowance = $derived(