From 636c027b589750b20717e8ef52e5b2b56fac97d2 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Wed, 2 Sep 2026 08:30:55 +0000 Subject: [PATCH] fix: a guest may use anonymous apps; await the stale-session logout; trim comments Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BayTppRCstWX6qTf3LMco5 --- backend/windmill-api-auth/src/auth.rs | 10 +++--- backend/windmill-api/openapi.yaml | 2 +- backend/windmill-api/src/apps.rs | 33 +++++++++++-------- .../apps/editor/PublicAppFrame.svelte | 18 ++++++---- 4 files changed, 36 insertions(+), 27 deletions(-) diff --git a/backend/windmill-api-auth/src/auth.rs b/backend/windmill-api-auth/src/auth.rs index 3fb8852d16..af6e8287fb 100644 --- a/backend/windmill-api-auth/src/auth.rs +++ b/backend/windmill-api-auth/src/auth.rs @@ -144,12 +144,10 @@ impl AuthCache { // from a job's WM_TOKEN, even on an AUTH_CACHE hit whose cached authed // predates this field. opt_job_authed.authed.job_id = opt_job_authed.job_id; - // The workspace's guest switch is enforced here, at the door, for every - // request a guest makes — not in the handlers, where each guest-reachable - // route would have to remember to re-check it. Uncached and per request, so - // turning guests off takes effect on the next request of every guest - // session and every token derived from one. Guests are a small share of - // traffic; the read is one primary-key lookup. + // The workspace's guest switch is enforced here, once, for every guest request + // — not per handler, where each guest-reachable route would have to remember + // it. Uncached, so turning guests off takes effect on the next request of every + // guest session and every token derived from one. if crate::scopes::has_guest_sentinel(opt_job_authed.authed.scopes.as_deref()) { let Some(w_id) = w_id else { return None }; match windmill_common::workspaces::is_guest_access_enabled(&self.db, &w_id).await { diff --git a/backend/windmill-api/openapi.yaml b/backend/windmill-api/openapi.yaml index 6a129cc52f..5f8b28779a 100644 --- a/backend/windmill-api/openapi.yaml +++ b/backend/windmill-api/openapi.yaml @@ -33223,7 +33223,7 @@ components: 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 is - checked when the session is minted and on every guest request after + checked when the session is minted and again on every guest request 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 1c23de116e..b7ffbdc8d7 100644 --- a/backend/windmill-api/src/apps.rs +++ b/backend/windmill-api/src/apps.rs @@ -292,9 +292,9 @@ pub enum ExecutionMode { /// Login required, workspace membership not: anyone the instance's identity /// provider authenticates may open the app, and the runnables execute as the /// publisher exactly as in [`ExecutionMode::Publisher`]. Such a viewer holds a - /// guest session — no `usr` row, no `password` row, no seat — so it is only - /// honored where `workspace_settings.guest_access_enabled` is on, checked - /// where the session is minted rather than here. + /// guest session — no `usr` row, no `password` row, no seat — honored only where + /// `workspace_settings.guest_access_enabled` is on, which `AuthCache` re-reads on + /// every guest request. Guest, /// Default for a policy that omits `execution_mode`. It MUST stay a mode /// that requires an authenticated viewer: an omitted field must never be @@ -1841,8 +1841,10 @@ async fn get_app_embed_token( let authed_for_token = if policy.anonymous_execution { // Anonymous app: still mint a scoped token if the viewer happens to be - // logged in (so the app sees their identity), otherwise stay anonymous. - opt_authed + // logged in (so the app sees their identity), otherwise stay anonymous. A + // guest's session names another app and cannot contain this one's scopes, + // so it renders anonymously here rather than being refused. + opt_authed.filter(|a| !windmill_api_auth::scopes::has_guest_sentinel(a.scopes.as_deref())) } else { let mode = if policy.guest_execution { ExecutionMode::Guest @@ -3748,14 +3750,16 @@ 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. + // A guest acts only through an app open to guests — or to everyone. A members-only + // mode means the policy changed after the session was issued. Decided here, in the + // one resolver every on-behalf path (runs, S3 reads, uploads) goes through. 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) + && !matches!( + policy.execution_mode(), + ExecutionMode::Guest | ExecutionMode::Anonymous + ) { return Err(Error::PermissionDenied( "this app is not open to guests".to_string(), @@ -4094,10 +4098,13 @@ 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. The app's mode and the workspace's switch are - // decided elsewhere: the on-behalf resolver and `AuthCache` respectively. + // naming the one app it may run — except an anonymous app, open to a guest as to + // anyone. Mode and workspace switch are decided by the on-behalf resolver and + // `AuthCache` respectively. if let Some(authed) = opt_authed.as_ref().filter(|_| is_guest_caller) { - check_scopes(authed, || format!("apps:run:{}", path))?; + if !matches!(policy.execution_mode(), ExecutionMode::Anonymous) { + check_scopes(authed, || format!("apps:run:{}", path))?; + } } // Execution is publisher and an user is authenticated: check if the user is authorized to diff --git a/frontend/src/lib/components/apps/editor/PublicAppFrame.svelte b/frontend/src/lib/components/apps/editor/PublicAppFrame.svelte index 7b8dcde346..d4924be17c 100644 --- a/frontend/src/lib/components/apps/editor/PublicAppFrame.svelte +++ b/frontend/src/lib/components/apps/editor/PublicAppFrame.svelte @@ -225,14 +225,18 @@ ) 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. + // The stale guest session must be gone 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. The card waits on + // `staleGuestCleared`, and a failed logout fails closed rather than offering a + // sign-in that could not complete. let staleGuestCleared = $state(false) + let staleGuestLogoutFailed = $state(false) $effect(() => { if (deniedStatus === 403 && guestEntry === 'guest' && !staleGuestCleared) { - staleGuestCleared = true - UserService.logout().catch(() => {}) + UserService.logout() + .then(() => (staleGuestCleared = true)) + .catch(() => (staleGuestLogoutFailed = true)) } }) let embedToken: string | null = $state(null) @@ -573,9 +577,9 @@ onContinue={onSdkConsentContinue} onDecline={onSdkConsentDecline} /> -{:else if offerSignIn && guestEntry === 'pending'} +{:else if offerSignIn && (guestEntry === 'pending' || (deniedStatus === 403 && guestEntry === 'guest' && !staleGuestCleared && !staleGuestLogoutFailed))} -{:else if offerSignIn && guestEntry === 'error'} +{:else if offerSignIn && (guestEntry === 'error' || staleGuestLogoutFailed)}
The app could not be reached to find out who may open it. Reload to try again.