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/<path>/guest.<jwt>`, `/public/<ws>/<secret>/guest.<jwt>`). 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/<jwt>`) 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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VF3v6LA9399gNphmZaHYG3

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<Postgres>) -> 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(())
-}

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<Postgres>) -> 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(())
+}

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
 }
 

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/<workspace>/<secret>/guest/<jwt>`. 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,

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/<jwt>`) 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 }
 	}

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 }
 	}