mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-09-07 00:01:49 +00:00
fix: a guest app path is refused at the mint if it could widen the scope; the instance toggle waits for its reload
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01BayTppRCstWX6qTf3LMco5
This commit is contained in:
co-authored by
Claude Fable 5.1
parent
9016816732
commit
fbcb0728f7
@@ -454,6 +454,36 @@ async fn guest_cannot_run_another_guest_app(db: Pool<Postgres>) -> anyhow::Resul
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// The app path is spliced into the session's scopes, whose parser splits resources on
|
||||
/// `,` and reads `*` as a wildcard: a path carrying either would scope the guest to more
|
||||
/// than the one app it was let in for, so the mint refuses it before anything else.
|
||||
#[sqlx::test(fixtures("base"))]
|
||||
async fn a_scope_metacharacter_in_the_app_path_is_refused(
|
||||
db: Pool<Postgres>,
|
||||
) -> anyhow::Result<()> {
|
||||
initialize_tracing().await;
|
||||
for path in [
|
||||
"u/test-user/entry,u/test-user/hidden",
|
||||
"u/test-user/*",
|
||||
"u/test-user/a b",
|
||||
] {
|
||||
let mut tx = db.begin().await?;
|
||||
let minted = windmill_api_users::users::create_guest_session_token(
|
||||
"guest@example.com",
|
||||
"test-workspace",
|
||||
path,
|
||||
&mut tx,
|
||||
tower_cookies::Cookies::default(),
|
||||
)
|
||||
.await;
|
||||
assert!(
|
||||
matches!(minted, Err(windmill_common::error::Error::BadRequest(ref m)) if m.contains("cannot be scoped")),
|
||||
"{path}: {minted:?}"
|
||||
);
|
||||
}
|
||||
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"))]
|
||||
@@ -494,12 +524,20 @@ async fn the_instance_switch_closes_every_workspace(db: Pool<Postgres>) -> anyho
|
||||
let resp = authed(client().get(format!("{ws}/users/whoami")), GUEST_TOKEN)
|
||||
.send()
|
||||
.await?;
|
||||
assert_eq!(resp.status(), 401, "the instance switch closes an issued session");
|
||||
assert_eq!(
|
||||
resp.status(),
|
||||
401,
|
||||
"the instance switch closes an issued session"
|
||||
);
|
||||
let resp = client()
|
||||
.get(format!("{ws}/apps_u/guest_entry/{secret}"))
|
||||
.send()
|
||||
.await?;
|
||||
assert_eq!(resp.status(), 404, "and nothing discovers as open to guests");
|
||||
assert_eq!(
|
||||
resp.status(),
|
||||
404,
|
||||
"and nothing discovers as open to guests"
|
||||
);
|
||||
|
||||
set_instance_switch(false).await?.error_for_status()?;
|
||||
let resp = authed(client().get(format!("{ws}/users/whoami")), GUEST_TOKEN)
|
||||
|
||||
@@ -2945,8 +2945,23 @@ 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`]).
|
||||
fn guest_session_scopes(app_path: &str) -> Vec<String> {
|
||||
vec![
|
||||
pub 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"
|
||||
)));
|
||||
}
|
||||
Ok(vec![
|
||||
windmill_api_auth::scopes::GUEST_SENTINEL.to_string(),
|
||||
"jobs:read".to_string(),
|
||||
"resources:run".to_string(),
|
||||
@@ -2954,7 +2969,7 @@ fn guest_session_scopes(app_path: &str) -> Vec<String> {
|
||||
"folders:read".to_string(),
|
||||
format!("apps:read:{app_path}"),
|
||||
format!("apps:run:{app_path}"),
|
||||
]
|
||||
])
|
||||
}
|
||||
|
||||
/// Mint a browser session for someone the identity provider authenticated who is a
|
||||
@@ -2988,7 +3003,7 @@ pub async fn create_guest_session_token<'c>(
|
||||
} else {
|
||||
Some(&token)
|
||||
};
|
||||
let scopes = guest_session_scopes(app_path);
|
||||
let scopes = guest_session_scopes(app_path)?;
|
||||
|
||||
// No account at all (see `ExecutionMode::Guest`): a deactivated `password` row
|
||||
// counts, since the sign-in path's own lookup filters on `disabled = false` and a
|
||||
|
||||
@@ -8901,8 +8901,8 @@ paths:
|
||||
description: >-
|
||||
The custom-path counterpart of `getGuestEntry`. Unauthenticated; 404 unless
|
||||
the app's execution mode is `guest` AND its workspace has
|
||||
`guest_access_enabled`. Returns the workspace too, since a custom URL may not
|
||||
carry it.
|
||||
`guest_access_enabled` AND the instance has not set `guest_access_disabled`.
|
||||
Returns the workspace too, since a custom URL may not carry it.
|
||||
operationId: getGuestEntryByCustomPath
|
||||
tags:
|
||||
- app
|
||||
@@ -13082,9 +13082,10 @@ paths:
|
||||
description: >-
|
||||
Unauthenticated: what a signed-out visitor reads to learn that signing in
|
||||
would let them in. 404 unless the app's execution mode is `guest` AND the
|
||||
workspace has `guest_access_enabled`, so it says nothing about apps that are
|
||||
not open to guests. Discloses only the app path, to a caller already holding
|
||||
the share secret.
|
||||
workspace has `guest_access_enabled` AND the instance has not set the
|
||||
`guest_access_disabled` global setting, so it says nothing about apps that
|
||||
are not open to guests. Discloses only the app path, to a caller already
|
||||
holding the share secret.
|
||||
operationId: getGuestEntry
|
||||
tags:
|
||||
- app
|
||||
|
||||
@@ -14,8 +14,9 @@
|
||||
hasMore: boolean
|
||||
loading: boolean
|
||||
onLoadMore: () => void
|
||||
/** The instance switch was written; the caller re-reads usage. */
|
||||
onInstanceSwitch: () => 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>
|
||||
}
|
||||
|
||||
let { usage, guests, hasMore, loading, onLoadMore, onInstanceSwitch }: Props = $props()
|
||||
@@ -39,8 +40,8 @@
|
||||
} catch (e) {
|
||||
sendUserToast(`Could not change the instance guest switch: ${e}`, true)
|
||||
}
|
||||
await onInstanceSwitch()
|
||||
switchPending = false
|
||||
onInstanceSwitch()
|
||||
}
|
||||
// A capped instance refuses the next stranger as soon as the allowance is used up.
|
||||
let pastAllowance = $derived(
|
||||
|
||||
Reference in New Issue
Block a user