From b8838564ff56dba8a9ba2f0b9fa486dfa9818720 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Thu, 3 Sep 2026 22:42:58 +0200 Subject: [PATCH] fix: commit guest activity before auditing; unambiguous share-link marker; isolate the allowance test - admit_and_record_guest_jwt commits the guest_activity row before the best-effort audit, on a separate connection. The EE audit writer swallows an audit_partitioned failure but the failing statement still aborts its transaction, so auditing before the commit would roll the activity row back while the arm returned success, admitting a guest uncounted and past the allowance. - The share-link guest marker is now the prefix `guest.` glued to the token (`/a//guest.`, `/public///guest.`). The `.` cannot appear in a custom-path or secret segment, so an external-JWT link whose custom path ends in a `guest` segment (`/a/foo/guest/`) is read as before rather than hijacked. Removed the unused SHARE_LINK_SEGMENT constant. - Moved the JWT allowance test to its own binary (app_guest_jwt_allowance.rs): set_plan flips a process-global license key, so a test sharing the binary with the existing allowance test would race under --test-threads. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01VF3v6LA9399gNphmZaHYG3 --- backend/tests/app_guest_allowance.rs | 98 ------------- backend/tests/app_guest_jwt_allowance.rs | 129 ++++++++++++++++++ backend/windmill-api-auth/src/auth.rs | 16 ++- backend/windmill-common/src/guest_jwt.rs | 4 - frontend/src/routes/a/[...path]/+page.svelte | 17 ++- .../[workspace]/[...secret]/+page.svelte | 8 +- 6 files changed, 157 insertions(+), 115 deletions(-) create mode 100644 backend/tests/app_guest_jwt_allowance.rs diff --git a/backend/tests/app_guest_allowance.rs b/backend/tests/app_guest_allowance.rs index eb59130b65..9642941893 100644 --- a/backend/tests/app_guest_allowance.rs +++ b/backend/tests/app_guest_allowance.rs @@ -145,101 +145,3 @@ async fn the_allowance_caps_strangers_and_meters_an_enterprise_plan( Ok(()) } - -// --- The same allowance, reached through a guest JWT (`jwt_guest_`) --- - -const JWT_PUB: &str = "-----BEGIN PUBLIC KEY-----\nMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEzAfqyCh34iYOCW0vg4ejq/zzJlzL\nSZScjnVyPjLGTapEwo4gc6/y1Yudd/v54wKh0OdfTfzAKMPWx/2NWx/ugg==\n-----END PUBLIC KEY-----\n"; -const JWT_PRIV: &str = "-----BEGIN PRIVATE KEY-----\nMIGHAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBG0wawIBAQQgu27S2DbSwUh8BmQb\n/i4/VhNdoXV7PJekhnoceMULYLihRANCAATMB+rIKHfiJg4JbS+Dh6Or/PMmXMtJ\nlJyOdXI+MsZNqkTCjiBzr/LVi513+/njAqHQ519N/MAow9bH/Y1bH+6C\n-----END PRIVATE KEY-----\n"; - -fn guest_jwt(email: &str) -> String { - use jsonwebtoken::{encode, Algorithm, EncodingKey, Header}; - let exp = std::time::SystemTime::now() - .duration_since(std::time::UNIX_EPOCH) - .unwrap() - .as_secs() - + 3600; - let claims = json!({ - "email": email, - "workspace_id": "test-workspace", - "app_path": APP_PATH, - "exp": exp, - }); - let jwt = encode( - &Header::new(Algorithm::ES256), - &claims, - &EncodingKey::from_ec_pem(JWT_PRIV.as_bytes()).unwrap(), - ) - .unwrap(); - format!("jwt_guest_{jwt}") -} - -/// A JWT guest is subject to the same allowance as a signed-in one. Past the cap on a -/// capped instance, a stranger's JWT is refused (the auth arm returns 401; the visitor -/// message is only logged, since the arm cannot carry it), while a guest already in the -/// window is let back in. -#[sqlx::test(fixtures("base"))] -async fn a_guest_jwt_is_capped_like_a_signed_in_guest(db: Pool) -> 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"); - - authed( - client().post(format!("{ws}/workspaces/edit_guest_access")), - ADMIN_TOKEN, - ) - .json(&json!({ "guest_access_enabled": true })) - .send() - .await?; - let resp = authed( - client().post(format!("{ws}/workspaces/edit_guest_jwt_key")), - ADMIN_TOKEN, - ) - .json(&json!({ "public_key": JWT_PUB })) - .send() - .await?; - assert_eq!(resp.status(), 200, "{}", resp.text().await?); - let resp = authed(client().post(format!("{ws}/apps/create")), ADMIN_TOKEN) - .json(&json!({ - "path": APP_PATH, - "summary": "Guest app", - "value": {}, - "policy": { "execution_mode": "guest", "triggerables_v2": {} } - })) - .send() - .await?; - assert_eq!(resp.status(), 201, "{}", resp.text().await?); - // The whole allowance, used today (g1..gN). - sqlx::query( - "INSERT INTO guest_activity (email, workspace_id, day) - SELECT 'g' || i || '@example.com', 'test-workspace', CURRENT_DATE - FROM generate_series(1, $1) AS i", - ) - .bind(FREE_GUESTS_PER_WINDOW) - .execute(&db) - .await?; - set_plan(true); - - let resp = authed( - client().get(format!("{ws}/users/whoami")), - &guest_jwt("stranger@example.com"), - ) - .send() - .await?; - assert_eq!(resp.status(), 401, "a stranger's JWT is refused past the cap"); - - let resp = authed( - client().get(format!("{ws}/users/whoami")), - &guest_jwt("g1@example.com"), - ) - .send() - .await?; - assert_eq!( - resp.status(), - 200, - "a returning guest's JWT is admitted: {}", - resp.text().await? - ); - - Ok(()) -} diff --git a/backend/tests/app_guest_jwt_allowance.rs b/backend/tests/app_guest_jwt_allowance.rs new file mode 100644 index 0000000000..9bdac83df8 --- /dev/null +++ b/backend/tests/app_guest_jwt_allowance.rs @@ -0,0 +1,129 @@ +//! The guest allowance reached through a guest JWT (`jwt_guest_`). Its own binary +//! because `set_plan` flips a process-global license key, which a test sharing the +//! process could not tolerate (see `app_guest_allowance.rs`). +//! +//! Users from the `base` fixture: +//! test-user (admin, token SECRET_TOKEN) + +use serde_json::json; +use sqlx::{Pool, Postgres}; +use windmill_common::workspaces::FREE_GUESTS_PER_WINDOW; +use windmill_test_utils::*; + +const ADMIN_TOKEN: &str = "SECRET_TOKEN"; +const APP_PATH: &str = "u/test-user/guest_app"; + +fn client() -> reqwest::Client { + reqwest::Client::new() +} + +fn authed(builder: reqwest::RequestBuilder, token: &str) -> reqwest::RequestBuilder { + builder.header("Authorization", format!("Bearer {}", token)) +} + +/// Community and Pro are capped, Enterprise is metered. Only a build with both +/// `private` and `enterprise` can meter; every other build is capped whatever this says. +fn set_plan(pro: bool) { + #[cfg(feature = "private")] + windmill_common::ee::LICENSE_KEY_ID.store(std::sync::Arc::new( + if pro { "test_pro" } else { "" }.to_string(), + )); + let _ = pro; +} + + +const JWT_PUB: &str = "-----BEGIN PUBLIC KEY-----\nMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEzAfqyCh34iYOCW0vg4ejq/zzJlzL\nSZScjnVyPjLGTapEwo4gc6/y1Yudd/v54wKh0OdfTfzAKMPWx/2NWx/ugg==\n-----END PUBLIC KEY-----\n"; +const JWT_PRIV: &str = "-----BEGIN PRIVATE KEY-----\nMIGHAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBG0wawIBAQQgu27S2DbSwUh8BmQb\n/i4/VhNdoXV7PJekhnoceMULYLihRANCAATMB+rIKHfiJg4JbS+Dh6Or/PMmXMtJ\nlJyOdXI+MsZNqkTCjiBzr/LVi513+/njAqHQ519N/MAow9bH/Y1bH+6C\n-----END PRIVATE KEY-----\n"; + +fn guest_jwt(email: &str) -> String { + use jsonwebtoken::{encode, Algorithm, EncodingKey, Header}; + let exp = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_secs() + + 3600; + let claims = json!({ + "email": email, + "workspace_id": "test-workspace", + "app_path": APP_PATH, + "exp": exp, + }); + let jwt = encode( + &Header::new(Algorithm::ES256), + &claims, + &EncodingKey::from_ec_pem(JWT_PRIV.as_bytes()).unwrap(), + ) + .unwrap(); + format!("jwt_guest_{jwt}") +} + +/// A JWT guest is subject to the same allowance as a signed-in one. Past the cap on a +/// capped instance, a stranger's JWT is refused (the auth arm returns 401; the visitor +/// message is only logged, since the arm cannot carry it), while a guest already in the +/// window is let back in. +#[sqlx::test(fixtures("base"))] +async fn a_guest_jwt_is_capped_like_a_signed_in_guest(db: Pool) -> 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"); + + authed( + client().post(format!("{ws}/workspaces/edit_guest_access")), + ADMIN_TOKEN, + ) + .json(&json!({ "guest_access_enabled": true })) + .send() + .await?; + let resp = authed( + client().post(format!("{ws}/workspaces/edit_guest_jwt_key")), + ADMIN_TOKEN, + ) + .json(&json!({ "public_key": JWT_PUB })) + .send() + .await?; + assert_eq!(resp.status(), 200, "{}", resp.text().await?); + let resp = authed(client().post(format!("{ws}/apps/create")), ADMIN_TOKEN) + .json(&json!({ + "path": APP_PATH, + "summary": "Guest app", + "value": {}, + "policy": { "execution_mode": "guest", "triggerables_v2": {} } + })) + .send() + .await?; + assert_eq!(resp.status(), 201, "{}", resp.text().await?); + // The whole allowance, used today (g1..gN). + sqlx::query( + "INSERT INTO guest_activity (email, workspace_id, day) + SELECT 'g' || i || '@example.com', 'test-workspace', CURRENT_DATE + FROM generate_series(1, $1) AS i", + ) + .bind(FREE_GUESTS_PER_WINDOW) + .execute(&db) + .await?; + set_plan(true); + + let resp = authed( + client().get(format!("{ws}/users/whoami")), + &guest_jwt("stranger@example.com"), + ) + .send() + .await?; + assert_eq!(resp.status(), 401, "a stranger's JWT is refused past the cap"); + + let resp = authed( + client().get(format!("{ws}/users/whoami")), + &guest_jwt("g1@example.com"), + ) + .send() + .await?; + assert_eq!( + resp.status(), + 200, + "a returning guest's JWT is admitted: {}", + resp.text().await? + ); + + Ok(()) +} diff --git a/backend/windmill-api-auth/src/auth.rs b/backend/windmill-api-auth/src/auth.rs index 1b05d50134..7739d01b7b 100644 --- a/backend/windmill-api-auth/src/auth.rs +++ b/backend/windmill-api-auth/src/auth.rs @@ -771,6 +771,15 @@ async fn admit_and_record_guest_jwt(db: &DB, w_id: &str, email: &str, app_path: return false; } }; + if let Err(e) = tx.commit().await { + tracing::error!("guest JWT tx commit failed for {w_id}: {e:#}"); + return false; + } + GUEST_JWT_ACTIVITY_CACHE.insert(cache_key, ()); + // Audit last, best-effort, on its own connection: the EE writer swallows an + // `audit_partitioned` failure but that failing statement still aborts the + // transaction it runs in, so auditing before the commit would let the whole + // activity row roll back while this returned success, admitting an uncounted guest. if inserted { let author = windmill_common::audit::AuditAuthor { email: email.to_string(), @@ -779,7 +788,7 @@ async fn admit_and_record_guest_jwt(db: &DB, w_id: &str, email: &str, app_path: token_prefix: None, }; if let Err(e) = windmill_audit::audit_oss::audit_log( - &mut *tx, + db, &author, "users.login_guest", windmill_audit::ActionKind::Create, @@ -792,11 +801,6 @@ async fn admit_and_record_guest_jwt(db: &DB, w_id: &str, email: &str, app_path: tracing::error!("auditing guest JWT login for {w_id}: {e:#}"); } } - if let Err(e) = tx.commit().await { - tracing::error!("guest JWT tx commit failed for {w_id}: {e:#}"); - return false; - } - GUEST_JWT_ACTIVITY_CACHE.insert(cache_key, ()); true } diff --git a/backend/windmill-common/src/guest_jwt.rs b/backend/windmill-common/src/guest_jwt.rs index a284c00d4d..b4a671d687 100644 --- a/backend/windmill-common/src/guest_jwt.rs +++ b/backend/windmill-common/src/guest_jwt.rs @@ -28,10 +28,6 @@ pub const MAX_LIFETIME_SECS: u64 = 24 * 60 * 60; /// so a rotated key revokes within minutes. See the arm in `windmill-api-auth`. pub const BEARER_PREFIX: &str = "jwt_guest_"; -/// Segment that marks a guest JWT on an app share link, right before the token: -/// `/public///guest/`. Distinct from the external JWT's bare -/// trailing segment so the page can tell the two apart without parsing either. -pub const SHARE_LINK_SEGMENT: &str = "guest"; const RSA_ALGORITHMS: [Algorithm; 6] = [ Algorithm::RS256, diff --git a/frontend/src/routes/a/[...path]/+page.svelte b/frontend/src/routes/a/[...path]/+page.svelte index ebfa9773ef..673ad196dd 100644 --- a/frontend/src/routes/a/[...path]/+page.svelte +++ b/frontend/src/routes/a/[...path]/+page.svelte @@ -44,12 +44,19 @@ guestJwt: string | undefined } { const parts = customPath.split('/') - const n = parts.length - if (n > 2 && parts[n - 2] === 'guest' && isJwt(parts[n - 1])) { - return { path: parts.slice(0, -2).join('/'), jwt: undefined, guestJwt: parts[n - 1] } + const last = parts[parts.length - 1] + // A guest JWT rides the last segment prefixed `guest.`. The `.` means it can never + // be a valid custom-path segment, so a real path ending in a `guest` segment + // followed by an external JWT (`.../guest/`) is read as before, not hijacked. + if (last.startsWith('guest.') && isJwt(last.slice('guest.'.length))) { + return { + path: parts.slice(0, -1).join('/'), + jwt: undefined, + guestJwt: last.slice('guest.'.length) + } } - if (n > 1 && isJwt(parts[n - 1])) { - return { path: parts.slice(0, -1).join('/'), jwt: parts[n - 1], guestJwt: undefined } + if (parts.length > 1 && isJwt(last)) { + return { path: parts.slice(0, -1).join('/'), jwt: last, guestJwt: undefined } } return { path: customPath, jwt: undefined, guestJwt: undefined } } diff --git a/frontend/src/routes/public/[workspace]/[...secret]/+page.svelte b/frontend/src/routes/public/[workspace]/[...secret]/+page.svelte index 3e4582b893..2e1c339ec2 100644 --- a/frontend/src/routes/public/[workspace]/[...secret]/+page.svelte +++ b/frontend/src/routes/public/[workspace]/[...secret]/+page.svelte @@ -38,8 +38,12 @@ guestJwt: string | undefined } { const parts = secret.split('/') - if (parts[1] === 'guest' && parts[2]) { - return { secret: parts[0], jwt: undefined, guestJwt: parts[2] } + // The credential rides the segment after the secret: a guest JWT prefixed + // `guest.`, or an external JWT bare. The `guest.` prefix glues the marker to the + // token, so it can never be mistaken for a path or secret segment (which carry no + // `.`), and a bare token keeps the established external-JWT interpretation. + if (parts[1]?.startsWith('guest.')) { + return { secret: parts[0], jwt: undefined, guestJwt: parts[1].slice('guest.'.length) } } return { secret: parts[0], jwt: parts[1], guestJwt: undefined } }