fix: guests stop at the launched-by-me job grant; canonical app paths at the mint and discovery; the toggle ends on the stored value

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01BayTppRCstWX6qTf3LMco5
This commit is contained in:
Ruben Fiszel
2026-09-04 09:07:39 +02:00
co-authored by Claude Fable 5.1
parent fbcb0728f7
commit 2b4631cedc
7 changed files with 82 additions and 26 deletions
+1 -1
View File
@@ -1 +1 @@
7d737d774b03b580999b4994b6a05e53ccddb2a7
6bfe1e25001e5de37d51b17108b7b2a335005fd8
+50 -1
View File
@@ -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<Postgres>) -> 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"))]
+3 -15
View File
@@ -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<Vec<String>> {
fn guest_session_scopes(app_path: &str) -> Result<Vec<String>> {
// 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(),
+5 -1
View File
@@ -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")));
}
@@ -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<bool> {
// 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<bool> = sqlx::query_scalar(&format!(
"SELECT COALESCE(ws.guest_access_enabled AND app.policy->>'execution_mode' = 'guest', false)
@@ -112,7 +112,7 @@
let guestLoading = $state(false)
const guestPerPage = 50
async function loadGuestPage(nextPage: number) {
async function loadGuestPage(nextPage: number): Promise<boolean> {
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
}
@@ -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<void>
/** 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<boolean>
}
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 @@
<div class="flex flex-row gap-2 items-center mb-4">
{#key usage}
<Toggle
checked={usage.instance_enabled}
bind:checked={switchOn}
disabled={switchPending}
on:change={(e) => setInstanceSwitch(e.detail)}
options={{