diff --git a/backend/.sqlx/query-126f72d28d1bfcea764311f3aedb2d1bbe51c9d5ba2cd12890de867ac51573ea.json b/backend/.sqlx/query-126f72d28d1bfcea764311f3aedb2d1bbe51c9d5ba2cd12890de867ac51573ea.json deleted file mode 100644 index 1e397d4f9e..0000000000 --- a/backend/.sqlx/query-126f72d28d1bfcea764311f3aedb2d1bbe51c9d5ba2cd12890de867ac51573ea.json +++ /dev/null @@ -1,29 +0,0 @@ -{ - "db_name": "PostgreSQL", - "query": "SELECT path, policy->>'execution_mode' as execution_mode\n FROM app WHERE id = $1 AND workspace_id = $2", - "describe": { - "columns": [ - { - "ordinal": 0, - "name": "path", - "type_info": "Varchar" - }, - { - "ordinal": 1, - "name": "execution_mode", - "type_info": "Text" - } - ], - "parameters": { - "Left": [ - "Int8", - "Text" - ] - }, - "nullable": [ - false, - null - ] - }, - "hash": "126f72d28d1bfcea764311f3aedb2d1bbe51c9d5ba2cd12890de867ac51573ea" -} diff --git a/backend/.sqlx/query-c0da3f1f2c55900dbdf92b16ebbfdb7b4cc11a648460f175e4f57d080a0005a5.json b/backend/.sqlx/query-31ed2fb85c0c726e3cf6392be2a73c62bae004b2842a4828c6807232570f83a1.json similarity index 54% rename from backend/.sqlx/query-c0da3f1f2c55900dbdf92b16ebbfdb7b4cc11a648460f175e4f57d080a0005a5.json rename to backend/.sqlx/query-31ed2fb85c0c726e3cf6392be2a73c62bae004b2842a4828c6807232570f83a1.json index 774b47f825..556bd7c317 100644 --- a/backend/.sqlx/query-c0da3f1f2c55900dbdf92b16ebbfdb7b4cc11a648460f175e4f57d080a0005a5.json +++ b/backend/.sqlx/query-31ed2fb85c0c726e3cf6392be2a73c62bae004b2842a4828c6807232570f83a1.json @@ -1,6 +1,6 @@ { "db_name": "PostgreSQL", - "query": "UPDATE token SET label = $1\n WHERE email = $2 AND token_prefix = $3\n AND (label IS NULL OR (\n label <> 'session'\n AND lower(label) NOT LIKE 'ephemeral%'\n AND label <> 'debugger-token'\n AND label NOT LIKE 'mcp-oauth-%'\n ))\n RETURNING token_prefix", + "query": "UPDATE token SET label = $1\n WHERE email = $2 AND token_prefix = $3\n AND (label IS NULL OR (\n label <> 'session'\n AND label <> 'guest_session'\n AND lower(label) NOT LIKE 'ephemeral%'\n AND label <> 'debugger-token'\n AND label NOT LIKE 'mcp-oauth-%'\n ))\n RETURNING token_prefix", "describe": { "columns": [ { @@ -20,5 +20,5 @@ false ] }, - "hash": "c0da3f1f2c55900dbdf92b16ebbfdb7b4cc11a648460f175e4f57d080a0005a5" + "hash": "31ed2fb85c0c726e3cf6392be2a73c62bae004b2842a4828c6807232570f83a1" } diff --git a/backend/.sqlx/query-391139a04bd48319a5512e7859b63e81438c7483ac892b971fe8a20709555cc1.json b/backend/.sqlx/query-391139a04bd48319a5512e7859b63e81438c7483ac892b971fe8a20709555cc1.json new file mode 100644 index 0000000000..8fcb8140f4 --- /dev/null +++ b/backend/.sqlx/query-391139a04bd48319a5512e7859b63e81438c7483ac892b971fe8a20709555cc1.json @@ -0,0 +1,23 @@ +{ + "db_name": "PostgreSQL", + "query": "SELECT path FROM app WHERE id = $1 AND workspace_id = $2", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "path", + "type_info": "Varchar" + } + ], + "parameters": { + "Left": [ + "Int8", + "Text" + ] + }, + "nullable": [ + false + ] + }, + "hash": "391139a04bd48319a5512e7859b63e81438c7483ac892b971fe8a20709555cc1" +} diff --git a/backend/.sqlx/query-a7a20412e303568b271f949642de55e9880ef05786fe59f05de5e025ef315726.json b/backend/.sqlx/query-d17645b5001d7f8da1dc451c5d35ea3c9346271b8404863256071cfdf884036a.json similarity index 65% rename from backend/.sqlx/query-a7a20412e303568b271f949642de55e9880ef05786fe59f05de5e025ef315726.json rename to backend/.sqlx/query-d17645b5001d7f8da1dc451c5d35ea3c9346271b8404863256071cfdf884036a.json index d507609d8e..c3134373f6 100644 --- a/backend/.sqlx/query-a7a20412e303568b271f949642de55e9880ef05786fe59f05de5e025ef315726.json +++ b/backend/.sqlx/query-d17645b5001d7f8da1dc451c5d35ea3c9346271b8404863256071cfdf884036a.json @@ -1,6 +1,6 @@ { "db_name": "PostgreSQL", - "query": "UPDATE token SET scopes = $1\n WHERE email = $2 AND token_prefix = $3\n RETURNING token_prefix", + "query": "UPDATE token SET scopes = $1\n WHERE email = $2 AND token_prefix = $3\n AND (label IS NULL OR label <> 'guest_session')\n RETURNING token_prefix", "describe": { "columns": [ { @@ -20,5 +20,5 @@ false ] }, - "hash": "a7a20412e303568b271f949642de55e9880ef05786fe59f05de5e025ef315726" + "hash": "d17645b5001d7f8da1dc451c5d35ea3c9346271b8404863256071cfdf884036a" } diff --git a/backend/tests/app_guest_execution_mode.rs b/backend/tests/app_guest_execution_mode.rs index fd5bf3b901..891407d054 100644 --- a/backend/tests/app_guest_execution_mode.rs +++ b/backend/tests/app_guest_execution_mode.rs @@ -314,14 +314,16 @@ async fn a_self_declared_guest_scope_grants_nothing(db: Pool) -> anyho } /// A guest-mode policy that names one runnable, so an `execute_component` request -/// gets past the triggerables lookup and reaches the guest gate. -fn guest_app_with_runnable(path: &str) -> serde_json::Value { +/// gets past the triggerables lookup and reaches the guest gate. `sandbox` is what +/// makes the embed-token endpoint actually mint a token. +fn guest_app_with_runnable(path: &str, sandbox: bool) -> serde_json::Value { json!({ "path": path, "summary": "Guest app", "value": {}, "policy": { "execution_mode": "guest", + "sandbox": sandbox, "triggerables_v2": { "script/u/test-user/noop": { "static_inputs": {}, "one_of_inputs": {} } } @@ -356,7 +358,7 @@ async fn execute_component_re_checks_the_workspace_switch( let ws = format!("http://localhost:{port}/api/w/test-workspace"); let resp = authed(client().post(format!("{ws}/apps/create")), ADMIN_TOKEN) - .json(&guest_app_with_runnable(APP_PATH)) + .json(&guest_app_with_runnable(APP_PATH, false)) .send() .await?; assert_eq!(resp.status(), 201, "{}", resp.text().await?); @@ -413,7 +415,7 @@ async fn guest_cannot_run_another_guest_app(db: Pool) -> anyhow::Resul .await?; let other = "u/test-user/other_guest_app"; let resp = authed(client().post(format!("{ws}/apps/create")), ADMIN_TOKEN) - .json(&guest_app_with_runnable(other)) + .json(&guest_app_with_runnable(other, false)) .send() .await?; assert_eq!(resp.status(), 201, "{}", resp.text().await?); @@ -430,3 +432,107 @@ async fn guest_cannot_run_another_guest_app(db: Pool) -> anyhow::Resul Ok(()) } + +/// The embed token a guest mints for a sandboxed app is the one credential handed to +/// untrusted app JS. It must be a guest twice over — resolve like its minter (the +/// label) and be governed like its minter (the sentinel) — or every guest control +/// silently skips the most exposed credential there is. +#[sqlx::test(fixtures("base"))] +async fn a_guest_minted_embed_token_stays_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, true)) + .send() + .await?; + assert_eq!(resp.status(), 201, "{}", resp.text().await?); + let secret: String = authed( + client().get(format!("{ws}/apps/secret_of/{APP_PATH}")), + ADMIN_TOKEN, + ) + .send() + .await? + .text() + .await?; + insert_guest_token(&db, "test-workspace").await?; + + // The guest page mints the iframe's token from the guest session. + let resp = authed( + client().get(format!("{ws}/apps_u/embed_token/{secret}")), + GUEST_TOKEN, + ) + .send() + .await?; + assert_eq!(resp.status(), 200, "a guest must be able to mint: {}", resp.text().await?); + let body: serde_json::Value = resp.json().await?; + let embed = body["token"] + .as_str() + .expect("mint must return a token for an authenticated guest") + .to_string(); + + // Resolves — and as a guest, not as the non-member superadmin shape. + let resp = authed(client().get(format!("{ws}/users/whoami")), &embed) + .send() + .await?; + assert_eq!(resp.status(), 200, "the minted token must authenticate"); + let me: serde_json::Value = resp.json().await?; + assert_eq!(me["role"], json!("guest")); + + // Governed: the workspace switch closes it, iframe or not. + authed( + client().post(format!("{ws}/workspaces/edit_guest_access")), + ADMIN_TOKEN, + ) + .json(&json!({ "guest_access_enabled": false })) + .send() + .await?; + let resp = execute(port, "test-workspace", APP_PATH, &embed) + .send() + .await?; + assert_eq!( + resp.status(), + 403, + "turning guests off must stop a guest's embed token running components" + ); + + // And its scopes are not something the guest's email can later rewrite. The + // guest session itself cannot reach `/users/*` (workspace pin), so model the real + // threat: the same email after promotion, holding an ordinary unpinned session. + sqlx::query( + "INSERT INTO token (token_hash, token_prefix, token, email, label) + VALUES (encode(sha256($1::bytea), 'hex'), 'PROMOTED_S', $2, 'guest@example.com', + 'session')", + ) + .bind(b"PROMOTED_SESSION".as_slice()) + .bind("PROMOTED_SESSION") + .execute(&db) + .await?; + for prefix in [&embed[..10], &GUEST_TOKEN[..10]] { + let resp = authed( + client().post(format!( + "http://localhost:{port}/api/users/tokens/update_scopes/{prefix}" + )), + "PROMOTED_SESSION", + ) + .json(&json!({ "scopes": null })) + .send() + .await?; + assert_eq!( + resp.status(), + 404, + "a promoted account must not be able to rescope its old guest credentials" + ); + } + + Ok(()) +} diff --git a/backend/windmill-api-users/src/users.rs b/backend/windmill-api-users/src/users.rs index 18bd58a72f..755bbe2d48 100644 --- a/backend/windmill-api-users/src/users.rs +++ b/backend/windmill-api-users/src/users.rs @@ -2931,7 +2931,7 @@ lazy_static::lazy_static! { /// there is nothing to disable when the workspace revokes guest access or the /// identity provider removes them — the expiry is the revocation. Much shorter /// than a member session for that reason. - static ref GUEST_SESSION_VALIDITY_SECONDS: i64 = std::env::var("GUEST_SESSION_VALIDITY_SECONDS") + pub static ref GUEST_SESSION_VALIDITY_SECONDS: i64 = std::env::var("GUEST_SESSION_VALIDITY_SECONDS") .ok() .and_then(|x| x.parse::().ok()) .unwrap_or(8 * 60 * 60); @@ -3311,9 +3311,15 @@ async fn update_token_scopes( let mut tx = db.begin().await?; + // A guest session's scopes are its entire confinement — the label grants the + // identity, the scopes bound it to one app. Once the same email holds a real + // account (promotion), that account could otherwise rescope the still-valid + // guest token into an unconfined non-member credential, so a guest-labelled + // token is not rescoped by anyone. Same shape as the relabel guard. let updated: Option = sqlx::query_scalar!( "UPDATE token SET scopes = $1 WHERE email = $2 AND token_prefix = $3 + AND (label IS NULL OR label <> 'guest_session') RETURNING token_prefix", req.scopes.as_deref(), &authed.email, @@ -3324,7 +3330,7 @@ async fn update_token_scopes( let prefix = updated.ok_or_else(|| { Error::NotFound(format!( - "token {token_prefix} not found or not owned by user" + "token {token_prefix} not found, not owned by user, or not rescopable" )) })?; diff --git a/backend/windmill-api-workspaces/src/workspaces.rs b/backend/windmill-api-workspaces/src/workspaces.rs index b0282cc451..78274a8549 100644 --- a/backend/windmill-api-workspaces/src/workspaces.rs +++ b/backend/windmill-api-workspaces/src/workspaces.rs @@ -4610,11 +4610,11 @@ struct EditGuestAccess { } /// Turn guest sessions on or off for this workspace. Off by default, and off is -/// authoritative: an app whose policy already says `guest` stops admitting them at -/// the next sign-in, because the switch is checked where the session is minted. -/// -/// Sessions already handed out are not revoked — a guest has no account to disable — -/// and run out on their own (`GUEST_SESSION_VALIDITY_SECONDS`). +/// 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. async fn edit_guest_access( authed: ApiAuthed, Extension(db): Extension, diff --git a/backend/windmill-api/src/apps.rs b/backend/windmill-api/src/apps.rs index a491ea0f6d..924b0d4630 100644 --- a/backend/windmill-api/src/apps.rs +++ b/backend/windmill-api/src/apps.rs @@ -1654,8 +1654,18 @@ pub async fn mint_app_embed_token( "App embed tokens cannot mint or renew embed tokens".to_string(), )); } - let expiration = - chrono::Utc::now() + chrono::Duration::hours(APP_EMBED_TOKEN_VALIDITY_HOURS); + let is_guest_minter = + windmill_api_auth::scopes::has_guest_sentinel(authed.scopes.as_deref()); + // A guest's embed token must not outlive the kind of session that minted it. + let validity = if is_guest_minter { + chrono::Duration::seconds( + (*windmill_api_users::users::GUEST_SESSION_VALIDITY_SECONDS) + .min(APP_EMBED_TOKEN_VALIDITY_HOURS * 3600), + ) + } else { + chrono::Duration::hours(APP_EMBED_TOKEN_VALIDITY_HOURS) + }; + let expiration = chrono::Utc::now() + validity; let mut scopes: Vec = APP_EMBED_SCOPES .iter() .filter(|s| **s != windmill_api_auth::scopes::APP_EMBED_SENTINEL) @@ -1677,12 +1687,15 @@ pub async fn mint_app_embed_token( // guest session is. `mint_raw_app_sdk_token` has the same shape. ensure_scopes_within_caller(authed, Some(&scopes))?; scopes.push(windmill_api_auth::scopes::APP_EMBED_SENTINEL.to_string()); - // An embed token minted by a guest has to resolve the same way the guest's - // own session does — through the label, since there is no `usr` row behind - // the email. With the ordinary label the iframe's every request would 401. - // The `app_embed` sentinel pushed above still confines it more tightly than - // the session that minted it. - let label = if windmill_api_auth::scopes::has_guest_sentinel(authed.scopes.as_deref()) { + // A guest's embed token is a guest twice over. The label is what lets it + // resolve at all — there is no `usr` row behind the email, so `AuthCache` + // admits it the same way it admits the session that minted it. The `guest` + // sentinel is what keeps every guest control on it: the workspace switch, the + // rate limit, the `whoami` role all key on the sentinel, and this is the one + // credential handed to untrusted app JS, so it must be at least as confined + // as its minter. Both sentinels compose — each is a default-deny allowlist. + let label = if is_guest_minter { + scopes.push(windmill_api_auth::scopes::GUEST_SENTINEL.to_string()); windmill_common::auth::GUEST_SESSION_LABEL.to_string() } else { format!("embed_app:{app_path}") @@ -1738,17 +1751,14 @@ async fn get_guest_entry( ) -> JsonResult { let id = get_id_from_secret(&db, &w_id, secret, None).await?; let app = sqlx::query!( - "SELECT path, policy->>'execution_mode' as execution_mode - FROM app WHERE id = $1 AND workspace_id = $2", + "SELECT path FROM app WHERE id = $1 AND workspace_id = $2", id, &w_id ) .fetch_optional(&db) .await?; let app = not_found_if_none(app, "App", id.to_string())?; - if app.execution_mode.as_deref() != Some(ExecutionMode::Guest.as_str()) - || !windmill_common::workspaces::is_guest_access_enabled(&db, &w_id).await? - { + if !windmill_common::workspaces::guest_app_admits(&db, &w_id, &app.path).await? { return Err(Error::NotFound("App is not open to guests".to_string())); } Ok(Json(GuestEntry { workspace_id: w_id, app_path: app.path })) diff --git a/frontend/src/lib/components/Login.svelte b/frontend/src/lib/components/Login.svelte index 8bec0dd309..7fd2c14773 100644 --- a/frontend/src/lib/components/Login.svelte +++ b/frontend/src/lib/components/Login.svelte @@ -606,16 +606,17 @@ }, 1500) } - /** Mirrors the server-side write in the OAuth `login` handler, including clearing - * it when this sign-in is not a guest entry. `login_externally` consumes it. - * `Secure` only over https, as the backend does with its own cookies: a browser - * drops a `Secure` cookie set from an http origin, and a SAML guest sign-in there - * would then silently provision a real account instead. Lax suffices, since the - * ACS redirect that consumes it is a same-site top-level navigation. */ + /** Mirrors the server-side write in the OAuth `login` handler (`set_unsensitive_cookie`) + * attribute for attribute, including clearing it when this sign-in is not a guest + * entry. `login_externally` consumes it. `SameSite=None` is required, not a choice: + * the SAML ACS is a cross-site POST from the IdP, and a Lax cookie is not sent on + * those. `None` needs `Secure`, so — exactly like the backend's own cookie — 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. */ function setGuestAppCookie(value: string | undefined) { try { const secure = window.location.protocol === 'https:' ? '; Secure' : '' - document.cookie = `guest_app=${encodeURIComponent(value ?? '')}; path=/; SameSite=Lax${secure}` + document.cookie = `guest_app=${encodeURIComponent(value ?? '')}; path=/; SameSite=None${secure}` } catch (e) { console.error('Could not set the guest app cookie', e) }