diff --git a/backend/ee-repo-ref.txt b/backend/ee-repo-ref.txt index d0238e0076..d1fd6020d8 100644 --- a/backend/ee-repo-ref.txt +++ b/backend/ee-repo-ref.txt @@ -1 +1 @@ -7d737d774b03b580999b4994b6a05e53ccddb2a7 \ No newline at end of file +6bfe1e25001e5de37d51b17108b7b2a335005fd8 \ No newline at end of file diff --git a/backend/tests/app_guest_execution_mode.rs b/backend/tests/app_guest_execution_mode.rs index f3249b48cc..e3ddb7a868 100644 --- a/backend/tests/app_guest_execution_mode.rs +++ b/backend/tests/app_guest_execution_mode.rs @@ -477,13 +477,62 @@ async fn a_scope_metacharacter_in_the_app_path_is_refused( ) .await; assert!( - matches!(minted, Err(windmill_common::error::Error::BadRequest(ref m)) if m.contains("cannot be scoped")), + matches!(minted, Err(windmill_common::error::Error::BadRequest(ref m)) if m.contains("Invalid path")), "{path}: {minted:?}" ); } Ok(()) } +/// A guest reads the jobs it launched and nothing else: with no membership behind it, +/// it must stop where an app embed token stops, before the share-token and ACL grants +/// a member would get, and with the same "not found" so it cannot probe for jobs. +#[sqlx::test(fixtures("base"))] +async fn a_guest_cannot_read_a_job_it_did_not_launch(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?; + insert_guest_token(&db, "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 resp = authed( + client().post(format!("{ws}/jobs/run/p/u/test-user/noop")), + ADMIN_TOKEN, + ) + .json(&json!({})) + .send() + .await?; + assert_eq!(resp.status(), 201, "{}", 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(), + 404, + "another caller's job is not found for a guest: {}", + resp.text().await? + ); + + Ok(()) +} + /// The superadmin switch sits above every workspace's: off, no guest session stands and /// no app discovers as open, whatever the workspace and the app say. #[sqlx::test(fixtures("base"))] diff --git a/backend/windmill-api-users/src/users.rs b/backend/windmill-api-users/src/users.rs index 73ea69ce54..ba565c6376 100644 --- a/backend/windmill-api-users/src/users.rs +++ b/backend/windmill-api-users/src/users.rs @@ -2945,22 +2945,10 @@ lazy_static::lazy_static! { /// /// The `guest` sentinel here only narrows. What makes the session a guest at all is the /// server-minted label ([`windmill_common::auth::GUEST_SESSION_LABEL`]). -pub fn guest_session_scopes(app_path: &str) -> Result> { +fn guest_session_scopes(app_path: &str) -> Result> { // The path is spliced into a scope, whose parser reads `,` as a resource separator - // and `*` as a wildcard: a path carrying either would name more than one app. - let canonical = app_path.split('/').count() >= 3 - && app_path.split('/').all(|seg| { - !seg.is_empty() - && seg - .chars() - .all(|c| c.is_ascii_alphanumeric() || "_-.".contains(c)) - }); - if !canonical { - return Err(Error::BadRequest(format!( - "app path {app_path} cannot be scoped: only letters, digits, `_`, `-` and `.` \ - in `/`-separated segments" - ))); - } + // and `*` as a wildcard; a canonical path carries neither. + windmill_common::utils::check_proper_path(app_path)?; Ok(vec![ windmill_api_auth::scopes::GUEST_SENTINEL.to_string(), "jobs:read".to_string(), diff --git a/backend/windmill-api/src/jobs.rs b/backend/windmill-api/src/jobs.rs index 53044ee020..8b1dfe18d6 100644 --- a/backend/windmill-api/src/jobs.rs +++ b/backend/windmill-api/src/jobs.rs @@ -1594,7 +1594,11 @@ pub(crate) async fn require_job_read_access( // this token, and letting it reach any job merely visible to the viewer would // expose unrelated runs' results/logs. Stop at the launched-by-viewer grant. // NotFound (not PermissionDenied) so the untrusted app can't probe job existence. - if windmill_api_auth::scopes::has_app_embed_sentinel(authed.scopes.as_deref()) { + // A guest stops here too: it has no membership behind it, so a share token whose + // audience is the workspace's members must not read for it either. + if windmill_api_auth::scopes::has_app_embed_sentinel(authed.scopes.as_deref()) + || windmill_api_auth::scopes::has_guest_sentinel(authed.scopes.as_deref()) + { return Err(Error::NotFound(format!("Job {job_id} not found"))); } diff --git a/backend/windmill-common/src/workspaces.rs b/backend/windmill-common/src/workspaces.rs index 4ec545b602..dc535e3d51 100644 --- a/backend/windmill-common/src/workspaces.rs +++ b/backend/windmill-common/src/workspaces.rs @@ -933,6 +933,10 @@ pub async fn guest_app_admits<'c, E: sqlx::Executor<'c, Database = sqlx::Postgre w_id: &str, app_path: &str, ) -> Result { + // The mint refuses a path it cannot scope, so discovery must not advertise one. + if crate::utils::check_proper_path(app_path).is_err() { + return Ok(false); + } let instance_admits = instance_admits_guests_sql(); let admits: Option = sqlx::query_scalar(&format!( "SELECT COALESCE(ws.guest_access_enabled AND app.policy->>'execution_mode' = 'guest', false) diff --git a/frontend/src/lib/components/SuperadminSettingsInner.svelte b/frontend/src/lib/components/SuperadminSettingsInner.svelte index 51ceed9aba..97b15fe443 100644 --- a/frontend/src/lib/components/SuperadminSettingsInner.svelte +++ b/frontend/src/lib/components/SuperadminSettingsInner.svelte @@ -112,7 +112,7 @@ let guestLoading = $state(false) const guestPerPage = 50 - async function loadGuestPage(nextPage: number) { + async function loadGuestPage(nextPage: number): Promise { guestLoading = true try { const res = await UserService.listGuests({ page: nextPage, perPage: guestPerPage }) @@ -121,8 +121,10 @@ ? res : { usage: res.usage, guests: [...guestList.guests, ...res.guests] } guestHasMore = res.guests.length === guestPerPage + return true } catch (e) { sendUserToast(`Failed to load guests: ${e}`, true) + return false } finally { guestLoading = false } diff --git a/frontend/src/lib/components/instanceSettings/GuestActivityList.svelte b/frontend/src/lib/components/instanceSettings/GuestActivityList.svelte index 000ca4eace..98fb15c4bd 100644 --- a/frontend/src/lib/components/instanceSettings/GuestActivityList.svelte +++ b/frontend/src/lib/components/instanceSettings/GuestActivityList.svelte @@ -14,24 +14,30 @@ hasMore: boolean loading: boolean onLoadMore: () => void - /** The instance switch was written; the caller re-reads usage and resolves once - * the toggle may show the stored value again. */ - onInstanceSwitch: () => Promise + /** The instance switch was written; the caller re-reads usage and says whether + * that read succeeded, so the toggle can show what is actually stored. */ + onInstanceSwitch: () => Promise } let { usage, guests, hasMore, loading, onLoadMore, onInstanceSwitch }: Props = $props() const loadMoreSize = 50 - // One write at a time, and the toggle shows the stored value again after either - // outcome: a refused write must not leave it showing the click. + // One write at a time, and the toggle always ends on what is stored: the reloaded + // value when the reload succeeds, else the write's outcome. let switchPending = $state(false) + let switchOn = $state(usage.instance_enabled) + $effect(() => { + switchOn = usage.instance_enabled + }) async function setInstanceSwitch(enabled: boolean) { switchPending = true + let written = false try { await SettingService.setGlobal({ key: 'guest_access_disabled', requestBody: { value: !enabled } }) + written = true sendUserToast( enabled ? 'Guests can sign in again where a workspace allows them' @@ -40,7 +46,10 @@ } catch (e) { sendUserToast(`Could not change the instance guest switch: ${e}`, true) } - await onInstanceSwitch() + const reloaded = await onInstanceSwitch() + if (!reloaded) { + switchOn = written ? enabled : usage.instance_enabled + } switchPending = false } // A capped instance refuses the next stranger as soon as the allowance is used up. @@ -59,7 +68,7 @@
{#key usage} setInstanceSwitch(e.detail)} options={{