From 82fc97dfc5c8483cc972501aa341a7f7a660a348 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Fri, 4 Sep 2026 09:33:29 +0200 Subject: [PATCH] fix: address round-10 review on the guest JWT entry Save the guest JWT key before the Enterprise-only default-app and rate-limit writes, so a refused write cannot swallow a valid key change on CE. Name the JWT entry in the Guests card summary. Complete verify()'s doc with the email and app_path rules. Anchor the refusal suite with a positive control and make enable_guests assert its status, so a broken fixture cannot pass it vacuously. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01VF3v6LA9399gNphmZaHYG3 --- backend/tests/app_guest_jwt_entry.rs | 10 ++++- backend/windmill-common/src/guest_jwt.rs | 41 +++++++++++++------ .../(logged)/workspace_settings/+page.svelte | 18 ++++---- 3 files changed, 46 insertions(+), 23 deletions(-) diff --git a/backend/tests/app_guest_jwt_entry.rs b/backend/tests/app_guest_jwt_entry.rs index e2b58b2ffd..b114a4b87b 100644 --- a/backend/tests/app_guest_jwt_entry.rs +++ b/backend/tests/app_guest_jwt_entry.rs @@ -86,7 +86,7 @@ fn bearer(claims: &Claims, priv_pem: &str, alg: Algorithm) -> String { } async fn enable_guests(port: u16, ws: &str, on: bool) -> anyhow::Result<()> { - authed( + let resp = authed( client().post(format!( "http://localhost:{port}/api/w/{ws}/workspaces/edit_guest_access" )), @@ -95,6 +95,7 @@ async fn enable_guests(port: u16, ws: &str, on: bool) -> anyhow::Result<()> { .json(&json!({ "guest_access_enabled": on })) .send() .await?; + assert_eq!(resp.status(), 200, "{}", resp.text().await?); Ok(()) } @@ -243,6 +244,13 @@ async fn guest_jwt_refusals(db: Pool) -> anyhow::Result<()> { create_app(port, ws, app(APP_PATH, "guest", false)).await?; create_app(port, ws, app("u/test-user/members_app", "publisher", false)).await?; + // Positive control: a token valid against this exact fixture is admitted. Without it a + // broken setup would 401 every bearer below and the whole suite would pass vacuously. + let control = whoami(port, ws, &bearer(&Claims::valid(), PRIV1, Algorithm::ES256)) + .send() + .await?; + assert_eq!(control.status(), 200, "{}", control.text().await?); + // wrong workspace: the claim must name the route's workspace. let mut c = Claims::valid(); c.workspace_id = "other-ws".to_string(); diff --git a/backend/windmill-common/src/guest_jwt.rs b/backend/windmill-common/src/guest_jwt.rs index 1e487a010b..2b5cb82ead 100644 --- a/backend/windmill-common/src/guest_jwt.rs +++ b/backend/windmill-common/src/guest_jwt.rs @@ -67,11 +67,13 @@ pub async fn key_source(db: &DB, w_id: &str) -> Result .fetch_optional(db) .await .map_err(|e| Error::internal_err(format!("reading guest JWT key of {w_id}: {e:#}")))?; - Ok(row.and_then(|r| match (r.guest_jwt_public_key, r.guest_jwt_jwks_url) { - (Some(pem), _) => Some(GuestJwtKeySource::Pem(pem)), - (None, Some(url)) => Some(GuestJwtKeySource::JwksUrl(url)), - (None, None) => None, - })) + Ok( + row.and_then(|r| match (r.guest_jwt_public_key, r.guest_jwt_jwks_url) { + (Some(pem), _) => Some(GuestJwtKeySource::Pem(pem)), + (None, Some(url)) => Some(GuestJwtKeySource::JwksUrl(url)), + (None, None) => None, + }), + ) } /// Parse a PEM public key and the algorithms it may verify: RSA keys the RS/PS family, @@ -86,8 +88,7 @@ pub fn decoding_key_from_pem(pem: &str) -> Result<(DecodingKey, &'static [Algori return Ok((key, &EC_ALGORITHMS)); } Err(Error::BadRequest( - "not an RSA or EC public key in PEM form (expected -----BEGIN PUBLIC KEY-----)" - .to_string(), + "not an RSA or EC public key in PEM form (expected -----BEGIN PUBLIC KEY-----)".to_string(), )) } @@ -113,7 +114,9 @@ pub fn jwk_algorithms(jwk: &Jwk) -> Option> { return None; } match (&jwk.algorithm, jwk.common.algorithm) { - (AlgorithmParameters::RSA(_), Some(alg)) if RSA_ALGORITHMS.contains(&alg) => Some(vec![alg]), + (AlgorithmParameters::RSA(_), Some(alg)) if RSA_ALGORITHMS.contains(&alg) => { + Some(vec![alg]) + } (AlgorithmParameters::RSA(_), None) => Some(RSA_ALGORITHMS.to_vec()), (AlgorithmParameters::EllipticCurve(_), Some(alg)) if EC_ALGORITHMS.contains(&alg) => { Some(vec![alg]) @@ -129,7 +132,8 @@ pub fn jwk_algorithms(jwk: &Jwk) -> Option> { /// Verify `token` against `key`, honouring only the accepted `algorithms`, and check /// every claim rule that needs no database: signature, `exp` (mandatory), `nbf` and -/// `iat` when present, the lifetime cap, and that the token names `w_id`. +/// `iat` when present, the lifetime cap, that the token names `w_id`, that `email` is a +/// valid address bounded to 254 bytes, and that `app_path` is a canonical path. pub fn verify( token: &str, key: &DecodingKey, @@ -289,7 +293,9 @@ fn servable(entry: Arc) -> Result> { fn spawn_jwks_refresh(url: String) { tokio::spawn(async move { let lock = JWKS_FETCH_LOCKS - .get_or_insert_with(url.as_str(), || Ok::<_, ()>(Arc::new(tokio::sync::Mutex::new(())))) + .get_or_insert_with(url.as_str(), || { + Ok::<_, ()>(Arc::new(tokio::sync::Mutex::new(()))) + }) .unwrap(); let Ok(_guard) = lock.try_lock() else { return }; match fetch_jwks(&url).await { @@ -510,7 +516,10 @@ mod tests { #[test] fn a_non_key_pem_is_rejected() { - assert!(decoding_key_from_pem("-----BEGIN CERTIFICATE-----\nnope\n-----END CERTIFICATE-----").is_err()); + assert!(decoding_key_from_pem( + "-----BEGIN CERTIFICATE-----\nnope\n-----END CERTIFICATE-----" + ) + .is_err()); } #[tokio::test] @@ -546,10 +555,16 @@ mod tests { let mut handles = Vec::new(); for _ in 0..10 { let u = url.clone(); - handles.push(tokio::spawn(async move { cached_jwks(&u).await.map(|e| e.keys.len()) })); + handles.push(tokio::spawn(async move { + cached_jwks(&u).await.map(|e| e.keys.len()) + })); } for h in handles { - assert_eq!(h.await.unwrap().unwrap(), 1, "each caller resolves the one key"); + assert_eq!( + h.await.unwrap().unwrap(), + 1, + "each caller resolves the one key" + ); } assert_eq!( hits.load(Ordering::SeqCst), diff --git a/frontend/src/routes/(root)/(logged)/workspace_settings/+page.svelte b/frontend/src/routes/(root)/(logged)/workspace_settings/+page.svelte index 8c8c126fc9..079dcb0d64 100644 --- a/frontend/src/routes/(root)/(logged)/workspace_settings/+page.svelte +++ b/frontend/src/routes/(root)/(logged)/workspace_settings/+page.svelte @@ -539,23 +539,23 @@ } async function saveDefaultAppSettings(): Promise { - // Guests first: the only write of this card available on every plan, so a refused - // Enterprise-only write after it cannot swallow it. + // Guest access and the guest JWT key are the writes of this card available on every plan; + // save them first so a refused Enterprise-only write after cannot swallow them. if (guestAccessEnabled !== initialGuestAccessEnabled) { await editGuestAccess() } - if (workspaceDefaultAppPath !== initialWorkspaceDefaultAppPath) { - await editWorkspaceDefaultApp() - } - if (publicAppRateLimitPerMinute !== initialPublicAppRateLimitPerMinute) { - await editPublicAppRateLimit() - } if ( effectiveGuestJwt.pem !== initialGuestJwtPublicKey || effectiveGuestJwt.jwks !== initialGuestJwtJwksUrl ) { await editGuestJwtKey() } + if (workspaceDefaultAppPath !== initialWorkspaceDefaultAppPath) { + await editWorkspaceDefaultApp() + } + if (publicAppRateLimitPerMinute !== initialPublicAppRateLimitPerMinute) { + await editPublicAppRateLimit() + } } async function editGuestJwtKey(): Promise { @@ -2228,7 +2228,7 @@ export async function main(