starred/windmill
1
0
Fork 0
mirror of https://github.com/windmill-labs/windmill.git synced 2026-09-07 00:01:49 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: reuse VALID_EMAIL, guard app_path, accept base64url JWT headers

Browse Source 
- Validate the email claim with `windmill_common::users::VALID_EMAIL` (the `usr`
  table's own constraint) plus the 254-byte bound, rather than a hand-rolled
  predicate. It requires an `@`, which is what keeps a guest's name its own
  principal, never a `u/<user>` or `g/<group>`.
- Reject an `app_path` claim carrying a scope metacharacter (`*`, `,`, `:`,
  whitespace) before authenticating: it is interpolated into `apps:read:<path>`
  and `apps:run:<path>`, where `*` or `,` would widen the guest past its one app.
- `isJwt` on the custom-path route normalises base64url before `atob`, so a
  header carrying `-`/`_` (a `kid`, a signature) is recognised instead of taken
  for a path segment; it also checks the three-segment structure.
- Drop the dead stale-key carry-forward in the blocking JWKS path (a stale-good
  entry is served by the fast path) and clean up the test's env var.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VF3v6LA9399gNphmZaHYG3
This commit is contained in:
Ruben Fiszel
2026-09-03 16:54:36 +00:00
co-authored by Claude Opus 4.8
parent 629ac4ba92
commit cb8fa45114
3 changed files with 47 additions and 32 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backend/tests/app_guest_jwt_entry.rs
+8 -6
View File
@@ -279,14 +279,11 @@ async fn guest_jwt_refusals(db: Pool<Postgres>) -> 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/<user>` or `g/<group>` principal; both are refused.
// an email with no `@` would become the guest's username and could be read as a
// `u/<user>` or `g/<group>` 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<Postgres>) -> 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<Postgres>) -> 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");
backend/windmill-common/src/guest_jwt.rs
+32 -23
View File
@@ -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/<user>` or `g/<group>`
// 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/<user>` or `g/<group>`.
// 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:<path>` and `apps:run:<path>` 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<Arc<JwksEntry>> {
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") };
}
}
frontend/src/routes/a/[...path]/+page.svelte
+7 -3
View File
@@ -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