From b317002dd60144a47a663fda7ac1be3d4f65a52a Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Wed, 2 Sep 2026 07:19:41 +0000 Subject: [PATCH] fix: the label alone governs a guest; refuse guests with accounts; unserialize discovery Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01BayTppRCstWX6qTf3LMco5 --- backend/Cargo.lock | 2 + backend/Cargo.toml | 2 + backend/ee-repo-ref.txt | 2 +- backend/tests/app_guest_execution_mode.rs | 89 +++++++++++++++++++ backend/windmill-api-auth/src/auth.rs | 9 ++ backend/windmill-api-auth/src/scopes.rs | 8 ++ backend/windmill-api-users/src/users.rs | 13 +++ backend/windmill-api/src/apps.rs | 17 +++- frontend/src/lib/components/Login.svelte | 48 +++++----- .../apps/editor/PublicAppFrame.svelte | 9 +- frontend/src/routes/a/[...path]/+page.svelte | 8 +- .../[workspace]/[...secret]/+page.svelte | 8 +- 12 files changed, 181 insertions(+), 34 deletions(-) diff --git a/backend/Cargo.lock b/backend/Cargo.lock index c79abcb78e..fe16e7b83b 100644 --- a/backend/Cargo.lock +++ b/backend/Cargo.lock @@ -14790,6 +14790,7 @@ dependencies = [ "tikv-jemallocator", "tokio", "tokio-stream", + "tower-cookies", "tracing", "tracing-subscriber", "url", @@ -14801,6 +14802,7 @@ dependencies = [ "windmill-api-client", "windmill-api-scripts", "windmill-api-settings", + "windmill-api-users", "windmill-autoscaling", "windmill-common", "windmill-dep-map", diff --git a/backend/Cargo.toml b/backend/Cargo.toml index 491f3cb48a..abc0e67b8c 100644 --- a/backend/Cargo.toml +++ b/backend/Cargo.toml @@ -351,6 +351,8 @@ windmill-trigger-sqs.workspace = true windmill-trigger-gcp.workspace = true windmill-trigger-azure.workspace = true windmill-api-auth.workspace = true +tower-cookies.workspace = true +windmill-api-users.workspace = true axum.workspace = true serde.workspace = true windmill-api-client.workspace = true diff --git a/backend/ee-repo-ref.txt b/backend/ee-repo-ref.txt index 9d1b850042..10885abffe 100644 --- a/backend/ee-repo-ref.txt +++ b/backend/ee-repo-ref.txt @@ -1 +1 @@ -60ea364b42bd6e099d46522f3399274d25925ba7 +fdb2f5b5df33776bc440a12e39bcc6b65981bd37 diff --git a/backend/tests/app_guest_execution_mode.rs b/backend/tests/app_guest_execution_mode.rs index db1797d4e0..4f3b429df5 100644 --- a/backend/tests/app_guest_execution_mode.rs +++ b/backend/tests/app_guest_execution_mode.rs @@ -554,3 +554,92 @@ async fn a_guest_minted_embed_token_stays_a_guest(db: Pool) -> anyhow: Ok(()) } + +/// The label is the single source of truth: a guest-labelled credential is governed +/// as a guest even if its scopes carry no sentinel. Otherwise every mint that derives +/// a token from a guest session is one forgotten `push` away from an ungoverned +/// non-member credential. +#[sqlx::test(fixtures("base"))] +async fn a_guest_label_is_governed_without_the_sentinel(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 scopes: Vec = guest_scopes().into_iter().filter(|s| s != "guest").collect(); + sqlx::query( + "INSERT INTO token (token_hash, token_prefix, token, email, label, scopes, workspace_id, expiration) + VALUES (encode(sha256($1::bytea), 'hex'), 'NOSENTINE_', $2, 'guest@example.com', + 'guest_session', $3, 'test-workspace', now() + interval '8 hours')", + ) + .bind(b"NOSENTINEL".as_slice()) + .bind("NOSENTINEL") + .bind(scopes) + .execute(&db) + .await?; + + let resp = authed(client().get(format!("{ws}/users/whoami")), "NOSENTINEL") + .send() + .await?; + assert_eq!(resp.status(), 200); + let me: serde_json::Value = resp.json().await?; + assert_eq!( + me["role"], + json!("guest"), + "the label alone must make a credential a guest" + ); + let resp = authed(client().get(format!("{ws}/jobs/list")), "NOSENTINEL") + .send() + .await?; + assert_eq!(resp.status(), 403, "and confine it like one"); + + Ok(()) +} + +/// A guest is someone with no account at all — including a deactivated one. The +/// sign-in path's own account lookup filters on `disabled = false`, so a disabled +/// account reads as absent there; the mint has to refuse on its own or deactivation +/// (manual or SCIM, whose revocation is "delete the tokens") walks straight back in. +#[sqlx::test(fixtures("base"))] +async fn a_disabled_account_cannot_become_a_guest(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"); + + authed( + client().post(format!("{ws}/workspaces/edit_guest_access")), + ADMIN_TOKEN, + ) + .json(&json!({ "guest_access_enabled": true })) + .send() + .await?; + 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?); + sqlx::query( + "INSERT INTO password (email, password_hash, login_type, super_admin, verified, disabled) + VALUES ('gone@example.com', 'x', 'password', false, true, true)", + ) + .execute(&db) + .await?; + + let mut tx = db.begin().await?; + let cookies = tower_cookies::Cookies::default(); + let minted = windmill_api_users::users::create_guest_session_token( + "gone@example.com", + "test-workspace", + APP_PATH, + &mut tx, + cookies, + ) + .await; + assert!( + matches!(minted, Err(windmill_common::error::Error::NotAuthorized(_))), + "a deactivated account must be refused a guest session, got {minted:?}" + ); + + Ok(()) +} diff --git a/backend/windmill-api-auth/src/auth.rs b/backend/windmill-api-auth/src/auth.rs index 978a22ac59..b37557d21b 100644 --- a/backend/windmill-api-auth/src/auth.rs +++ b/backend/windmill-api-auth/src/auth.rs @@ -527,6 +527,15 @@ impl AuthCache { // scopes on a user-minted token are whatever // the caller typed. None if is_guest_session => { + // The label is the grant; every guest + // control downstream keys on the `guest` + // sentinel. Pin the two together here so + // a credential that carries the label is + // governed as a guest whatever its scopes + // say — nothing else may decide that. + let scopes = Some(crate::scopes::with_guest_sentinel( + scopes.unwrap_or_default(), + )); Some(ApiAuthed { username: email.clone(), email, diff --git a/backend/windmill-api-auth/src/scopes.rs b/backend/windmill-api-auth/src/scopes.rs index a3e9b51ab5..1880e3f7f9 100644 --- a/backend/windmill-api-auth/src/scopes.rs +++ b/backend/windmill-api-auth/src/scopes.rs @@ -785,6 +785,14 @@ pub fn has_guest_sentinel(scopes: Option<&[String]>) -> bool { scopes.is_some_and(|s| s.iter().any(|x| x == GUEST_SENTINEL)) } +/// `scopes` with the guest sentinel present exactly once. +pub fn with_guest_sentinel(mut scopes: Vec) -> Vec { + if !scopes.iter().any(|x| x == GUEST_SENTINEL) { + scopes.push(GUEST_SENTINEL.to_string()); + } + scopes +} + /// Sentinel in raw-app SDK tokens. Grants nothing; `check_route_access` uses it /// to narrow the declared scopes to what the viewer's prompt promised. pub const RAW_APP_SDK_SENTINEL: &str = "raw_app_sdk"; diff --git a/backend/windmill-api-users/src/users.rs b/backend/windmill-api-users/src/users.rs index 767e4c727e..84199262e3 100644 --- a/backend/windmill-api-users/src/users.rs +++ b/backend/windmill-api-users/src/users.rs @@ -2994,6 +2994,19 @@ pub async fn create_guest_session_token<'c>( }; let scopes = guest_session_scopes(app_path); + // A guest is someone with no account at all. Checked here, not only by the caller, + // because the sign-in path's own account lookup filters on `disabled = false` — + // a deactivated account (manual or SCIM, whose revocation is "delete the tokens") + // would otherwise read as absent and walk straight back in as a guest. + let has_account: bool = sqlx::query_scalar("SELECT EXISTS(SELECT 1 FROM password WHERE email = $1)") + .bind(email) + .fetch_one(&mut **tx) + .await?; + if has_account { + return Err(Error::NotAuthorized( + "an existing account cannot hold a guest session".to_string(), + )); + } if !windmill_common::workspaces::guest_app_admits(&mut **tx, w_id, app_path).await? { return Err(Error::NotAuthorized(format!( "app {app_path} is not open to guests" diff --git a/backend/windmill-api/src/apps.rs b/backend/windmill-api/src/apps.rs index 077ffd837a..84fbb3e803 100644 --- a/backend/windmill-api/src/apps.rs +++ b/backend/windmill-api/src/apps.rs @@ -1540,7 +1540,22 @@ pub async fn build_embed_token_response( && opt_authed.is_some() && !policy.frontend_sdk_scopes.is_empty() { - Some(policy.frontend_sdk_scopes.clone()) + // An SDK token runs as the viewer, and a guest's session is the ceiling on what + // it may delegate — the mint enforces that. Advertise only what a guest can + // actually be granted, so the consent prompt never promises a scope the mint + // would then refuse. + let declared = policy.frontend_sdk_scopes.clone(); + let offered = match opt_authed { + Some(a) if windmill_api_auth::scopes::has_guest_sentinel(a.scopes.as_deref()) => { + let held = a.scopes.as_deref().unwrap_or_default(); + declared + .into_iter() + .filter(|sc| held.iter().any(|h| h == sc)) + .collect::>() + } + _ => declared, + }; + (!offered.is_empty()).then_some(offered) } else { None }; diff --git a/frontend/src/lib/components/Login.svelte b/frontend/src/lib/components/Login.svelte index c8aa77d949..77410f35ab 100644 --- a/frontend/src/lib/components/Login.svelte +++ b/frontend/src/lib/components/Login.svelte @@ -404,7 +404,9 @@ if (autoRedirect && autoLogin && !error && !shouldSkipAutoRedirect()) { if (autoLogin === 'saml' && saml) { autoRedirecting = true - if (!redirectSaml()) autoRedirecting = false + redirectSaml().then((ok) => { + if (!ok) autoRedirecting = false + }) } else if (logins?.some((l) => l.type === autoLogin)) { autoRedirecting = true if (!storeRedirect(autoLogin)) { @@ -596,16 +598,14 @@ console.log('oauth: popup closed before login completed') return } + // A guest session is pinned to its workspace and cannot answer the global + // probe; an ordinary session for a non-member cannot answer the workspace + // one. Either answering means the popup signed someone in. + const guestWorkspace = guestApp?.split('/')[0] + const probes: Promise[] = [UserService.getCurrentEmail()] + if (guestWorkspace) probes.push(UserService.whoami({ workspace: guestWorkspace })) try { - // A guest session is pinned to its workspace and cannot authenticate on - // any workspace-less route, so the global probe would 401 forever and this - // fallback would never complete a guest sign-in. - const guestWorkspace = guestApp?.split('/')[0] - if (guestWorkspace) { - await UserService.whoami({ workspace: guestWorkspace }) - } else { - await UserService.getCurrentEmail() - } + await Promise.any(probes) } catch { return } @@ -614,24 +614,26 @@ }, 1500) } - /** The SAML counterpart of the cookie the OAuth `login` handler writes server-side - * (`set_unsensitive_cookie`), including clearing it when this sign-in is not a - * guest entry. `login_externally` consumes it. `SameSite=None` is required: the - * SAML ACS is a cross-site POST from the IdP, and a Lax cookie is not sent on - * those. `None` needs `Secure`, so this only survives the round trip over https; - * over plain http the browser drops it and a guest sign-in falls through to - * ordinary provisioning. Host-only: a `COOKIE_DOMAIN` deployment that serves the - * ACS from a different host than this page would need the domain set here too. */ - function setGuestAppCookie(value: string | undefined) { + /** Have the server write the guest-entry cookie, as the OAuth `login` handler does + * on its own path. SAML goes straight to the IdP and never passes through `login`, + * and a browser-set cookie would be host-only — on a `COOKIE_DOMAIN` deployment + * whose ACS answers on a sibling host it would never arrive. Server-set, it carries + * the same attributes as every other session cookie. Cleared (empty) when this + * sign-in is not a guest entry. `login_externally` consumes it. */ + async function setGuestAppCookie(value: string | undefined) { try { - const secure = window.location.protocol === 'https:' ? '; Secure' : '' - document.cookie = `guest_app=${encodeURIComponent(value ?? '')}; path=/; SameSite=None${secure}` + await fetch(`${base}/api/oauth/guest_app`, { + method: 'POST', + credentials: 'include', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ guest_app: value ?? '' }) + }) } catch (e) { console.error('Could not set the guest app cookie', e) } } - function redirectSaml(): boolean { + async function redirectSaml(): Promise { if (!saml) { sendUserToast('No SAML login available', true) return false @@ -644,7 +646,7 @@ // too. Client-set is safe: the callback still checks that the named app is in // guest mode and that the workspace allows guests, so the worst a forged value // can do is give its own author a narrower session than they'd otherwise get. - setGuestAppCookie(guestApp) + await setGuestAppCookie(guestApp) let target = saml let relayStateSet = false // Carry the SP-initiated deep link through the IdP round-trip via SAML diff --git a/frontend/src/lib/components/apps/editor/PublicAppFrame.svelte b/frontend/src/lib/components/apps/editor/PublicAppFrame.svelte index 97e0f04936..7c237a3a41 100644 --- a/frontend/src/lib/components/apps/editor/PublicAppFrame.svelte +++ b/frontend/src/lib/components/apps/editor/PublicAppFrame.svelte @@ -50,7 +50,8 @@ onViewerReady, viewer, viewerUrl, - guestAppPath = undefined + guestAppPath = undefined, + guestEntryResolved = true }: { /** Embedder-side: validate access + mint the scoped token. Throws with a * `.status` of 401 (login required) or 404 (not found). Pass @@ -74,6 +75,10 @@ * up front and pass it down — otherwise a signed-out visitor is offered an * ordinary sign-in that creates an account and still cannot open the app. */ guestAppPath?: string | undefined + /** False while the page is still finding out whether the app admits guests. The + * sign-in card waits for it (a configured auto-login would otherwise start an + * ordinary sign-in); nothing else does. */ + guestEntryResolved?: boolean } = $props() const EMBED_PARAM = 'wm_embed' @@ -542,6 +547,8 @@ onContinue={onSdkConsentContinue} onDecline={onSdkConsentDecline} /> +{:else if status === 'noPermission' && !guestEntryResolved} + {:else if status === 'noPermission'} diff --git a/frontend/src/routes/a/[...path]/+page.svelte b/frontend/src/routes/a/[...path]/+page.svelte index 6f50e38eb4..3167cad77b 100644 --- a/frontend/src/routes/a/[...path]/+page.svelte +++ b/frontend/src/routes/a/[...path]/+page.svelte @@ -61,8 +61,9 @@ /** `/` when this app is open to guests. Resolved eagerly: * PublicAppFrame renders its sign-in gate before `onViewerReady` fires. */ let guestAppPath: string | undefined = $state(undefined) - /** The frame's sign-in gate must not mount before this is known: a configured - * auto-login would otherwise fire an ordinary sign-in and provision an account. */ + /** The frame's sign-in card must not mount before this is known: a configured + * auto-login would otherwise fire an ordinary sign-in and provision an account. + * Only the card waits — the app load itself runs in parallel with discovery. */ let guestEntryResolved = $state(false) async function loadGuestEntry() { @@ -148,11 +149,11 @@ } -{#if guestEntryResolved} { refresh = requestTokenRefresh loadApp() @@ -170,4 +171,3 @@ > {/snippet} -{/if} diff --git a/frontend/src/routes/public/[workspace]/[...secret]/+page.svelte b/frontend/src/routes/public/[workspace]/[...secret]/+page.svelte index 0d39bd1593..ca71bd8b19 100644 --- a/frontend/src/routes/public/[workspace]/[...secret]/+page.svelte +++ b/frontend/src/routes/public/[workspace]/[...secret]/+page.svelte @@ -20,8 +20,9 @@ * offer a guest session rather than a dead end. 404 (the common case) leaves it * undefined. */ let guestAppPath: string | undefined = $state(undefined) - /** The frame's sign-in gate must not mount before this is known: a configured - * auto-login would otherwise fire an ordinary sign-in and provision an account. */ + /** The frame's sign-in card must not mount before this is known: a configured + * auto-login would otherwise fire an ordinary sign-in and provision an account. + * Only the card waits — the app load itself runs in parallel with discovery. */ let guestEntryResolved = $state(false) function parseSecret(secret: string): { secret: string; jwt: string | undefined } { @@ -124,11 +125,11 @@ } -{#if guestEntryResolved} { refresh = requestTokenRefresh loadApp() @@ -146,4 +147,3 @@ > {/snippet} -{/if}