From c8183e33e5094658d330b3f51b202452ead1f0eb Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Wed, 2 Sep 2026 15:21:17 +0000 Subject: [PATCH] fix: a guest uses an anonymous app as itself; S3 uploads confined by app mode --- backend/tests/app_guest_execution_mode.rs | 96 ++++++++++++++++--- backend/windmill-api/src/apps.rs | 29 +++--- .../apps/editor/PublicAppFrame.svelte | 11 ++- 3 files changed, 103 insertions(+), 33 deletions(-) diff --git a/backend/tests/app_guest_execution_mode.rs b/backend/tests/app_guest_execution_mode.rs index 9c8f6f8823..8c8317463a 100644 --- a/backend/tests/app_guest_execution_mode.rs +++ b/backend/tests/app_guest_execution_mode.rs @@ -38,7 +38,9 @@ fn authed(builder: reqwest::RequestBuilder, token: &str) -> reqwest::RequestBuil async fn enable_guests(port: u16, ws: &str) -> anyhow::Result<()> { authed( - client().post(format!("http://localhost:{port}/api/w/{ws}/workspaces/edit_guest_access")), + client().post(format!( + "http://localhost:{port}/api/w/{ws}/workspaces/edit_guest_access" + )), ADMIN_TOKEN, ) .json(&json!({ "guest_access_enabled": true })) @@ -330,12 +332,16 @@ async fn a_self_declared_guest_scope_grants_nothing(db: Pool) -> anyho /// 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 { + app_with_runnable(path, "guest", sandbox) +} + +fn app_with_runnable(path: &str, execution_mode: &str, sandbox: bool) -> serde_json::Value { json!({ "path": path, - "summary": "Guest app", + "summary": "App", "value": {}, "policy": { - "execution_mode": "guest", + "execution_mode": execution_mode, "sandbox": sandbox, "triggerables_v2": { "script/u/test-user/noop": { "static_inputs": {}, "one_of_inputs": {} } @@ -363,9 +369,7 @@ fn execute(port: u16, ws: &str, app: &str, token: &str) -> reqwest::RequestBuild /// git-sync and execution once an admin has turned guests off — and it closes the /// app to sessions already issued. #[sqlx::test(fixtures("base"))] -async fn the_door_re_checks_the_workspace_switch( - db: Pool, -) -> anyhow::Result<()> { +async fn the_door_re_checks_the_workspace_switch(db: Pool) -> anyhow::Result<()> { initialize_tracing().await; let server = ApiServer::start(db.clone()).await?; let port = server.addr.port(); @@ -386,7 +390,11 @@ async fn the_door_re_checks_the_workspace_switch( let resp = authed(client().get(format!("{ws}/users/whoami")), GUEST_TOKEN) .send() .await?; - assert_eq!(resp.status(), 401, "a guest must not authenticate while guests are off"); + assert_eq!( + resp.status(), + 401, + "a guest must not authenticate while guests are off" + ); let resp = execute(port, "test-workspace", APP_PATH, GUEST_TOKEN) .send() .await?; @@ -448,6 +456,59 @@ async fn guest_cannot_run_another_guest_app(db: Pool) -> anyhow::Resul Ok(()) } +/// An anonymous app is open to anyone, a guest included, and the guest uses it as +/// itself: the component run and the result read that follows are one identity, so +/// the read's launched-by-me grant matches. Acting as nobody for the run and as the +/// guest for the read would start a job whose result the page can never fetch. +#[sqlx::test(fixtures("base"))] +async fn a_guest_uses_an_anonymous_app_as_itself(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"); + + enable_guests(port, "test-workspace").await?; + let resp = authed(client().post(format!("{ws}/scripts/create")), ADMIN_TOKEN) + .json(&json!({ + "path": "u/test-user/noop", + "summary": "", + "description": "", + "content": "echo 42", + "language": "bash", + })) + .send() + .await?; + assert_eq!(resp.status(), 201, "{}", resp.text().await?); + let anon = "u/test-user/anon_app"; + let resp = authed(client().post(format!("{ws}/apps/create")), ADMIN_TOKEN) + .json(&app_with_runnable(anon, "anonymous", false)) + .send() + .await?; + assert_eq!(resp.status(), 201, "{}", resp.text().await?); + insert_guest_token(&db, "test-workspace").await?; // scoped to APP_PATH, not `anon` + + let resp = execute(port, "test-workspace", anon, GUEST_TOKEN) + .send() + .await?; + assert_eq!(resp.status(), 200, "{}", resp.text().await?); + let job_id = resp.text().await?; + + let resp = authed( + client().get(format!("{ws}/jobs_u/getupdate/{job_id}")), + GUEST_TOKEN, + ) + .send() + .await?; + assert_eq!( + resp.status(), + 200, + "the guest that started the run must be able to read it back: {}", + resp.text().await? + ); + + 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 @@ -482,7 +543,12 @@ async fn a_guest_minted_embed_token_stays_a_guest(db: Pool) -> anyhow: ) .send() .await?; - assert_eq!(resp.status(), 200, "a guest must be able to mint: {}", resp.text().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() @@ -492,11 +558,10 @@ async fn a_guest_minted_embed_token_stays_a_guest(db: Pool) -> anyhow: // Its lifetime is capped at the session that minted it: the requested embed // validity (12h) is longer than the guest session's (8h in this fixture), and the // session's expiry is a guest's only revocation. - let parent_exp: chrono::DateTime = sqlx::query_scalar( - "SELECT expiration FROM token WHERE token_prefix = 'GUEST_SECR'", - ) - .fetch_one(&db) - .await?; + let parent_exp: chrono::DateTime = + sqlx::query_scalar("SELECT expiration FROM token WHERE token_prefix = 'GUEST_SECR'") + .fetch_one(&db) + .await?; let child_exp: chrono::DateTime = body["expiration"] .as_str() .and_then(|e| e.parse().ok()) @@ -584,7 +649,10 @@ async fn a_guest_label_is_governed_without_the_sentinel(db: Pool) -> a let ws = format!("http://localhost:{port}/api/w/test-workspace"); enable_guests(port, "test-workspace").await?; - let scopes: Vec = guest_scopes().into_iter().filter(|s| s != "guest").collect(); + 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', diff --git a/backend/windmill-api/src/apps.rs b/backend/windmill-api/src/apps.rs index 36201d4a05..b5d9b16934 100644 --- a/backend/windmill-api/src/apps.rs +++ b/backend/windmill-api/src/apps.rs @@ -365,10 +365,10 @@ pub fn authorize_non_member_viewer( Ok(false) } -/// The caller an app-scoped operation proceeds with, once the app's mode is known. A -/// guest session names one app: on an anonymous app a mismatch means "act as anyone", -/// on any other it is the refusal the page answers with a fresh sign-in. Non-guest -/// scoped callers are confined at the top of each handler instead, before any read. +/// Confines a guest to its app once the app's mode is known; a no-op for every other +/// caller. An anonymous app is open to anyone, so the guest stays the caller there, +/// as itself: the run and the reads that follow it (job results, S3 provenance) must +/// carry one identity. Anywhere else a mismatch is refused. fn guest_caller_for_mode( opt_authed: Option, mode: ExecutionMode, @@ -377,16 +377,14 @@ fn guest_caller_for_mode( let Some(authed) = opt_authed.as_ref() else { return Ok(None); }; - if !windmill_api_auth::scopes::has_guest_sentinel(authed.scopes.as_deref()) { + if !windmill_api_auth::scopes::has_guest_sentinel(authed.scopes.as_deref()) + || matches!(mode, ExecutionMode::Anonymous) + { return Ok(opt_authed); } - match check_scopes(authed, || format!("apps:run:{app_path}")) - .or_else(|_| check_scopes(authed, || format!("apps:read:{app_path}"))) - { - Ok(()) => Ok(opt_authed), - Err(_) if matches!(mode, ExecutionMode::Anonymous) => Ok(None), - Err(e) => Err(e), - } + check_scopes(authed, || format!("apps:run:{app_path}")) + .or_else(|_| check_scopes(authed, || format!("apps:read:{app_path}")))?; + Ok(opt_authed) } /// [`authorize_non_member_viewer`] plus the member read-access probe, for the @@ -4461,8 +4459,7 @@ async fn upload_s3_file_from_app( request: axum::extract::Request, ) -> JsonResult { // Same path confinement as `execute_component`: without it a token scoped to app A - // could drive app B's upload policy. A guest's waits for the app's mode, inside - // `get_on_behalf_authed_from_app`. + // could drive app B's upload policy. A guest's waits for the app's mode, below. if let Some(authed) = opt_authed .as_ref() .filter(|a| !windmill_api_auth::scopes::has_guest_sentinel(a.scopes.as_deref())) @@ -4519,6 +4516,10 @@ async fn upload_s3_file_from_app( .map(|p| serde_json::from_value::(p).map_err(to_anyhow)) .transpose()? }; + let opt_authed = match policy.as_ref() { + Some(policy) => guest_caller_for_mode(opt_authed, policy.execution_mode(), path.to_path())?, + None => opt_authed, + }; let user_db = UserDB::new(db.clone()); diff --git a/frontend/src/lib/components/apps/editor/PublicAppFrame.svelte b/frontend/src/lib/components/apps/editor/PublicAppFrame.svelte index 652ae4ce59..66120610ee 100644 --- a/frontend/src/lib/components/apps/editor/PublicAppFrame.svelte +++ b/frontend/src/lib/components/apps/editor/PublicAppFrame.svelte @@ -209,10 +209,9 @@ type FrameStatus = 'loading' | 'ready' | 'noPermission' | 'notExists' | 'sdkPrompt' let status = $state('loading') /** Whether the visitor holds an account session, probed whenever the app denies - * them: `/api/users/email` answers for an account and never for a guest (pinned - * to its workspace) or for nobody. An account this app still refuses is not - * something signing in again can fix — an identity with an account is never given - * a guest session — so the card gives way to an explanation. */ + * them. An account this app still refuses is not something signing in again can + * fix — an identity with an account is never given a guest session — so the card + * gives way to an explanation. */ let accountSession = $state<'unknown' | 'none' | 'held'>('unknown') let deniedStatus: number | undefined = $state(undefined) /** The sign-in card belongs on a 401, and on a 403 unless discovery has settled @@ -227,6 +226,8 @@ let signInDidNotHelp = $derived(offerSignIn && accountSession === 'held') $effect(() => { if (offerSignIn && accountSession === 'unknown') { + // Workspace-less, so it answers for an account and never for a guest + // (pinned to its workspace) or for nobody. UserService.getCurrentEmail() .then(() => (accountSession = 'held')) .catch(() => (accountSession = 'none')) @@ -603,7 +604,7 @@ You are signed in, but this app is not open to you
- It is open to members of its workspace{guestAppPath + It is open to the people it was shared with{guestAppPath ? ', and to guests who have no Windmill account' : ''}. Ask the person who shared it to give your account access.