From 2ecbe31e9191c2f201dae6c59d4c8af4d53a475c Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Wed, 2 Sep 2026 08:15:21 +0000 Subject: [PATCH] fix: guest app-mode decided once at the on-behalf resolver; clear a stale guest session before offering another app's sign-in Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BayTppRCstWX6qTf3LMco5 --- .../windmill-api-workspaces/src/workspaces.rs | 7 +++--- backend/windmill-api/openapi.yaml | 12 +++++----- backend/windmill-api/src/apps.rs | 22 +++++++++++------ backend/windmill-common/src/workspaces.rs | 10 ++++---- .../apps/editor/PublicAppFrame.svelte | 24 +++++++++++++++---- 5 files changed, 48 insertions(+), 27 deletions(-) diff --git a/backend/windmill-api-workspaces/src/workspaces.rs b/backend/windmill-api-workspaces/src/workspaces.rs index 78274a8549..7f39203eec 100644 --- a/backend/windmill-api-workspaces/src/workspaces.rs +++ b/backend/windmill-api-workspaces/src/workspaces.rs @@ -4611,10 +4611,9 @@ struct EditGuestAccess { /// Turn guest sessions on or off for this workspace. Off by default, and off is /// authoritative and immediate: the switch is re-read where a guest session is -/// minted, where a guest reads an app and where a guest runs its components -/// (`guest_app_admits` / `is_guest_access_enabled`), so an app whose policy already -/// says `guest` — pushed by git-sync, say — closes to guests on the next request, -/// sessions already issued included. +/// minted (`guest_app_admits`) and at the auth door on every guest request, so an app +/// whose policy already says `guest` — pushed by git-sync, say — closes to guests on +/// the next request, sessions already issued included. async fn edit_guest_access( authed: ApiAuthed, Extension(db): Extension, diff --git a/backend/windmill-api/openapi.yaml b/backend/windmill-api/openapi.yaml index 2a83be9b3e..6a129cc52f 100644 --- a/backend/windmill-api/openapi.yaml +++ b/backend/windmill-api/openapi.yaml @@ -5751,10 +5751,10 @@ paths: summary: enable or disable guest sessions for this workspace description: >- Guests are people the identity provider authenticates who are members of no - workspace, so they take no seat. Off by default. Re-read wherever a guest is - admitted — minting a session, reading an app, running its components — so - turning it off takes effect immediately, for sessions already issued and for - apps whose policy already says `guest`. + workspace, so they take no seat. Off by default. Re-read where a guest session + is minted and at the auth door on every guest request, so turning it off takes + effect immediately, for sessions already issued and for apps whose policy + already says `guest`. operationId: editGuestAccess tags: - workspace @@ -33222,8 +33222,8 @@ components: the app is already deployed under. Neither `anonymous`, which makes the app publicly executable, nor `guest`, which opens it to anyone the identity provider authenticates, is ever assumed. A guest is only - admitted where the workspace also has `guest_access_enabled`, which - gates both minting a guest session and running an app's components + admitted where the workspace also has `guest_access_enabled`, which is + checked when the session is minted and on every guest request after on_behalf_of: type: string on_behalf_of_email: diff --git a/backend/windmill-api/src/apps.rs b/backend/windmill-api/src/apps.rs index fcc1593d57..1c23de116e 100644 --- a/backend/windmill-api/src/apps.rs +++ b/backend/windmill-api/src/apps.rs @@ -3748,6 +3748,19 @@ async fn get_on_behalf_details_from_policy_and_authed( policy: &Policy, opt_authed: &Option, ) -> Result<(String, String, String)> { + // A guest acts only through an app that is open to guests. Any other mode means + // the policy changed after the session was issued. Decided here because this is + // the one resolver every on-behalf path — component runs, S3 reads, uploads — + // goes through, so no route has to remember it. + if opt_authed + .as_ref() + .is_some_and(|a| windmill_api_auth::scopes::has_guest_sentinel(a.scopes.as_deref())) + && !matches!(policy.execution_mode(), ExecutionMode::Guest) + { + return Err(Error::PermissionDenied( + "this app is not open to guests".to_string(), + )); + } let (username, permissioned_as, email) = match policy.execution_mode() { ExecutionMode::Anonymous => { let username = opt_authed @@ -4081,14 +4094,9 @@ async fn execute_component( // A guest session holds no ACL of its own, so the read-permit probe below would // deny every guest. What confines it is the scope the session was minted with, - // naming the one app it may run — and the app has to be open to guests at all. - // The workspace's guest switch is enforced by `AuthCache` on every guest request. + // naming the one app it may run. The app's mode and the workspace's switch are + // decided elsewhere: the on-behalf resolver and `AuthCache` respectively. if let Some(authed) = opt_authed.as_ref().filter(|_| is_guest_caller) { - if !matches!(policy.execution_mode(), ExecutionMode::Guest) { - return Err(Error::PermissionDenied(format!( - "app {path} is not open to guests" - ))); - } check_scopes(authed, || format!("apps:run:{}", path))?; } diff --git a/backend/windmill-common/src/workspaces.rs b/backend/windmill-common/src/workspaces.rs index 5df10dc1c9..5dd59838ab 100644 --- a/backend/windmill-common/src/workspaces.rs +++ b/backend/windmill-common/src/workspaces.rs @@ -778,11 +778,11 @@ pub struct BillableSeats { /// Whether `w_id` admits guest sessions — someone the identity provider authenticated /// who is a member of no workspace, and who therefore takes no seat. /// -/// Read uncached, at every door a guest passes: where the session is minted -/// ([`guest_app_admits`]), where a guest reads an app, and where a guest runs its -/// components. An app carries its own `execution_mode` in its definition, so git-sync -/// and the CLI can push `guest` past every deploy-time gate; re-reading here is what -/// makes turning the switch off take effect on sessions already issued. +/// Read uncached where a session is minted ([`guest_app_admits`]) and then once per +/// request at the auth door (`AuthCache::get_opt_job_authed`) for every guest. An app +/// carries its own `execution_mode` in its definition, so git-sync and the CLI can +/// push `guest` past every deploy-time gate; the per-request read is what makes +/// turning the switch off take effect on sessions already issued. pub async fn is_guest_access_enabled(db: &crate::DB, w_id: &str) -> Result { Ok(sqlx::query_scalar!( "SELECT guest_access_enabled FROM workspace_settings WHERE workspace_id = $1", diff --git a/frontend/src/lib/components/apps/editor/PublicAppFrame.svelte b/frontend/src/lib/components/apps/editor/PublicAppFrame.svelte index 4b0a93ff90..7b8dcde346 100644 --- a/frontend/src/lib/components/apps/editor/PublicAppFrame.svelte +++ b/frontend/src/lib/components/apps/editor/PublicAppFrame.svelte @@ -23,7 +23,7 @@ * bearer token (no cookie); the raw wrapper document always carries `CSP: sandbox`. */ import { BROWSER } from 'esm-env' - import { OpenAPI } from '$lib/gen' + import { OpenAPI, UserService } from '$lib/gen' import { page } from '$app/state' import { onDestroy, onMount, setContext, type Snippet } from 'svelte' import { Alert, Skeleton } from '$lib/components/common' @@ -214,13 +214,27 @@ * member of this workspace lands here. */ let signedInHere = $state(false) let deniedStatus: number | undefined = $state(undefined) - /** The sign-in card belongs on a 401, and on a 403 against an app that admits - * guests (a session for another app of the same workspace; signing in again - * replaces it). */ + /** The sign-in card belongs on a 401, and on a 403 unless discovery has settled + * that the app is not open to guests — a 403 on a guest app is a session for a + * different app of the same workspace, which a fresh sign-in replaces. While + * discovery is still pending this stays true so the skeleton shows rather than + * a flash of "Not found". */ let offerSignIn = $derived( - status === 'noPermission' || (status === 'notExists' && deniedStatus === 403 && guestEntry === 'guest') + status === 'noPermission' || + (status === 'notExists' && deniedStatus === 403 && guestEntry !== 'none') ) let signInDidNotHelp = $derived(offerSignIn && signedInHere) + + // The stale guest session must go before the card mounts: it still authenticates + // in this workspace, so the popup's success poll would see it and complete the + // sign-in before the new session ever lands. + let staleGuestCleared = $state(false) + $effect(() => { + if (deniedStatus === 403 && guestEntry === 'guest' && !staleGuestCleared) { + staleGuestCleared = true + UserService.logout().catch(() => {}) + } + }) let embedToken: string | null = $state(null) let iframeEl: HTMLIFrameElement | undefined = $state(undefined)