diff --git a/backend/tests/app_guest_jwt_entry.rs b/backend/tests/app_guest_jwt_entry.rs index b3492a0591..6f6d4f8d28 100644 --- a/backend/tests/app_guest_jwt_entry.rs +++ b/backend/tests/app_guest_jwt_entry.rs @@ -279,14 +279,11 @@ async fn guest_jwt_refusals(db: Pool) -> anyhow::Result<()> { c.exp = now() + 25 * 3600; let over_lifetime_cap = bearer(&c, PRIV1, Algorithm::ES256); - // an email that is not a plain address would become the guest's username and could - // be read as a `u/` or `g/` principal; both are refused. + // an email with no `@` would become the guest's username and could be read as a + // `u/` or `g/` principal; refused. let mut c = Claims::valid(); c.email = "group-admins".to_string(); let group_shaped_email = bearer(&c, PRIV1, Algorithm::ES256); - let mut c = Claims::valid(); - c.email = "u/x@example.com".to_string(); - let slash_in_email = bearer(&c, PRIV1, Algorithm::ES256); // an email longer than the `guest_activity.email` column: refused before auth, so a // guest is never admitted without the activity row and audit event the count needs. @@ -294,6 +291,11 @@ async fn guest_jwt_refusals(db: Pool) -> anyhow::Result<()> { c.email = format!("{}@example.com", "a".repeat(250)); let oversized_email = bearer(&c, PRIV1, Algorithm::ES256); + // an app_path carrying a scope metacharacter would widen the guest's scopes. + let mut c = Claims::valid(); + c.app_path = "u/test-user/*".to_string(); + let wildcard_app_path = bearer(&c, PRIV1, Algorithm::ES256); + for (label, token) in [ ("wrong workspace", wrong_ws), ("wrong key", wrong_key), @@ -304,8 +306,8 @@ async fn guest_jwt_refusals(db: Pool) -> anyhow::Result<()> { ("mixed-case account", mixed_case_account), ("over the 24h lifetime cap", over_lifetime_cap), ("group-shaped email", group_shaped_email), - ("slash in email", slash_in_email), ("oversized email", oversized_email), + ("wildcard app_path", wildcard_app_path), ] { let resp = whoami(port, ws, &token).send().await?; assert_eq!(resp.status(), 401, "{label} must be refused"); diff --git a/backend/windmill-common/src/guest_jwt.rs b/backend/windmill-common/src/guest_jwt.rs index 797e19b55a..a284c00d4d 100644 --- a/backend/windmill-common/src/guest_jwt.rs +++ b/backend/windmill-common/src/guest_jwt.rs @@ -160,19 +160,25 @@ pub fn verify( "guest JWT refused: workspace_id does not match the workspace".to_string(), )); } - // The email becomes the guest's username, and `username_to_permissioned_as` reads a - // name with no `@` (or the `group-` prefix) as a `u/` or `g/` - // principal. Require a plain, bounded email: so a guest's name can only ever be its - // own principal, never a user's or a group's, and so it fits the - // `guest_activity.email` column (a longer one fails that insert while the guest is - // admitted uncounted). - if !claims.email.contains('@') - || claims.email.contains('/') - || claims.email.chars().any(char::is_whitespace) - || claims.email.len() > 254 + // The email becomes the guest's username; require the address shape the `usr` table + // accepts (`VALID_EMAIL`), so it always carries an `@` and `username_to_permissioned_as` + // can only ever read it as its own principal, never a `u/` or `g/`. + // Bound it to fit the `guest_activity.email` column: a longer one fails that insert + // while the guest is admitted uncounted. + if !crate::users::VALID_EMAIL.is_match(&claims.email) || claims.email.len() > 254 { + return Err(Error::NotAuthorized( + "guest JWT refused: email is not a valid, bounded email address".to_string(), + )); + } + // The app path is interpolated into `apps:read:` and `apps:run:` scopes; + // a scope metacharacter would widen them past the one app the guest is confined to. + if claims.app_path.is_empty() + || claims + .app_path + .contains(|c: char| c == '*' || c == ',' || c == ':' || c.is_whitespace()) { return Err(Error::NotAuthorized( - "guest JWT refused: email is not a plain, bounded email address".to_string(), + "guest JWT refused: app_path contains an invalid character".to_string(), )); } Ok(claims) @@ -291,9 +297,9 @@ fn spawn_jwks_refresh(url: String) { } /// The workspace's JWKS. A fresh entry is served directly; a stale-but-good one is -/// served while a refresh runs off the request path, so a slow issuer never stalls a -/// request. Only a cold or negative entry blocks, under a per-URL lock so a burst -/// triggers one fetch; a failed fetch serves the last good keys, or a short-lived empty +/// served while a refresh runs off the request path (`spawn_jwks_refresh`), so a slow +/// issuer never stalls a request. Only a cold or negative entry blocks, under a per-URL +/// lock so a burst triggers one fetch; a failed fetch there caches a short-lived empty /// entry that reads as "issuer unreachable", so an unreachable issuer is hit at most /// once per `JWKS_NEGATIVE_TTL`. Fetches follow a schedule, never a per-request, /// attacker-chosen `kid`. @@ -336,15 +342,17 @@ async fn cached_jwks(url: &str) -> Result> { Ok(entry) } Err(e) => { - let keys = JWKS_CACHE.get(url).map(|stale| stale.keys.clone()).unwrap_or_default(); - let entry = - Arc::new(JwksEntry { keys, expires_at: Instant::now() + JWKS_NEGATIVE_TTL }); - JWKS_CACHE.insert(url.to_string(), entry.clone()); - // Nothing good was ever cached: surface the fetch error itself. - if entry.keys.is_empty() { - return Err(e); - } - Ok(entry) + // The blocking path is reached only with no good keys to serve (a stale-good + // entry is served by the fast path above). Cache a short negative entry so + // the next requests do not each refetch, and surface the error. + JWKS_CACHE.insert( + url.to_string(), + Arc::new(JwksEntry { + keys: Arc::new(HashMap::new()), + expires_at: Instant::now() + JWKS_NEGATIVE_TTL, + }), + ); + Err(e) } } } @@ -513,5 +521,6 @@ mod tests { 1, "single-flight: a concurrent cold burst makes one fetch" ); + unsafe { std::env::remove_var("ALLOW_PRIVATE_GUEST_JWKS_URLS") }; } } diff --git a/frontend/src/routes/a/[...path]/+page.svelte b/frontend/src/routes/a/[...path]/+page.svelte index ef06a88d02..ebfa9773ef 100644 --- a/frontend/src/routes/a/[...path]/+page.svelte +++ b/frontend/src/routes/a/[...path]/+page.svelte @@ -19,11 +19,15 @@ let jwtError = $state(false) function isJwt(t: string) { - // simply check that the first part is a valid base64 encoded json + // A JWT is three dot-separated base64url segments; check the header decodes to + // JSON. `atob` wants standard base64, so normalise base64url first (a `kid` or a + // signature routinely contains `-`/`_`), or a valid token is taken for a path. try { const parts = t.split('.') - const header = atob(parts[0]) - JSON.parse(header) + if (parts.length !== 3) return false + const b64 = parts[0].replace(/-/g, '+').replace(/_/g, '/') + const pad = b64.length % 4 === 0 ? '' : '='.repeat(4 - (b64.length % 4)) + JSON.parse(atob(b64 + pad)) return true } catch (e) { return false