From d0e80f2d47bdbc1ce7342c63990b22d657c4a207 Mon Sep 17 00:00:00 2001 From: hugocasa Date: Fri, 17 Jul 2026 17:37:32 +0200 Subject: [PATCH] fix: cap the admin-or-devops gate at workspace admin for job tokens MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit require_admin_or_devops (the EE critical-alerts endpoints) grants when the caller is a workspace admin OR an instance devops. is_devops_email is true for superadmins, so a WM_TOKEN running on-behalf of a superadmin who is not a member of the target workspace could clear the devops branch and read/ack that workspace's critical alerts (GHSA-hfh4-cx4h-3fcr). This gate takes a bare email, not an ApiAuthed, so the token-layer cap could not see it. Thread the caller's job-token provenance and reject the devops branch for job tokens, matching require_devops_role. The workspace-admin branch stays allowed — that is the cap ceiling. Adds an enterprise-gated regression proving the bypass is closed and a real superadmin token still clears the gate. Found while auditing the PR for bare-email gates the choke-point cap misses. Co-Authored-By: Claude Opus 4.8 --- backend/tests/wm_token_superadmin_guard.rs | 48 ++++++++++++++++++++++ backend/windmill-api/src/workspaces.rs | 4 +- backend/windmill-common/src/utils.rs | 8 +++- 3 files changed, 57 insertions(+), 3 deletions(-) diff --git a/backend/tests/wm_token_superadmin_guard.rs b/backend/tests/wm_token_superadmin_guard.rs index b3a0db4eab..6a46c4ef27 100644 --- a/backend/tests/wm_token_superadmin_guard.rs +++ b/backend/tests/wm_token_superadmin_guard.rs @@ -364,3 +364,51 @@ async fn test_wm_token_rejected_by_require_devops_role(db: Pool) -> an Ok(()) } + +/// A job token must not clear an *admin-or-devops* gate via the devops branch. +/// `require_admin_or_devops` (the EE critical-alerts endpoints) grants when the +/// caller is a workspace admin OR an instance `devops`; since `is_devops_email` +/// is true for superadmins, a WM_TOKEN running on-behalf of a superadmin who is +/// NOT a member of the target workspace would otherwise gain workspace-scoped +/// devops access to a workspace it has no admin rights in (GHSA-hfh4-cx4h-3fcr). +/// The workspace-admin branch stays allowed — that is the cap ceiling. +#[cfg(feature = "enterprise")] +#[sqlx::test(fixtures("preserve_on_behalf_of"))] +async fn test_wm_token_rejected_by_admin_or_devops_gate(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + set_jwt_secret().await; + + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + let base = format!("http://localhost:{port}/api/w/test-workspace/workspaces"); + + // superadmin-external is a superadmin but not a member of test-workspace, so + // its workspace-level is_admin is false — the exact exploit precondition. + let sa_wm = wm_token("superadmin-external@windmill.dev", false).await; + let resp = authed(client().get(format!("{base}/critical_alerts")), &sa_wm) + .send() + .await?; + assert_eq!( + resp.status(), + 403, + "superadmin WM_TOKEN must not clear the admin-or-devops gate on a workspace it isn't admin of: {}", + resp.text().await? + ); + + // No false positive: the same superadmin's real API token (not a job token) + // still clears the gate via the devops branch. + let resp = authed( + client().get(format!("{base}/critical_alerts")), + "EXTERNAL_SUPERADMIN_TOKEN", + ) + .send() + .await?; + assert_ne!( + resp.status(), + 403, + "a real superadmin token must still clear the admin-or-devops gate: {}", + resp.text().await? + ); + + Ok(()) +} diff --git a/backend/windmill-api/src/workspaces.rs b/backend/windmill-api/src/workspaces.rs index 1e26a20e05..23ce68fb80 100644 --- a/backend/windmill-api/src/workspaces.rs +++ b/backend/windmill-api/src/workspaces.rs @@ -214,7 +214,7 @@ pub async fn get_critical_alerts( authed: ApiAuthed, Query(params): Query, ) -> JsonResult { - require_admin_or_devops(authed.is_admin, &authed.username, &authed.email, &db).await?; + require_admin_or_devops(authed.is_admin, &authed.username, &authed.email, authed.job_id.is_some(), &db).await?; crate::utils::get_critical_alerts(db, params, Some(w_id)).await } @@ -230,7 +230,7 @@ pub async fn acknowledge_critical_alert( Path((w_id, id)): Path<(String, i32)>, authed: ApiAuthed, ) -> Result { - require_admin_or_devops(authed.is_admin, &authed.username, &authed.email, &db).await?; + require_admin_or_devops(authed.is_admin, &authed.username, &authed.email, authed.job_id.is_some(), &db).await?; crate::utils::acknowledge_critical_alert(db, Some(w_id), id).await } diff --git a/backend/windmill-common/src/utils.rs b/backend/windmill-common/src/utils.rs index 17d9cfd804..b4828e38d1 100644 --- a/backend/windmill-common/src/utils.rs +++ b/backend/windmill-common/src/utils.rs @@ -246,10 +246,16 @@ pub async fn require_admin_or_devops( is_admin: bool, username: &str, email: &str, + // True when the caller is a job token (`$WM_TOKEN`). `devops` is instance-level + // and `is_devops_email` is true for superadmins, so a job token whose on_behalf_of + // a `wm_deployers` member pointed at a superadmin would otherwise clear the devops + // branch on a workspace it isn't admin of (GHSA-hfh4-cx4h-3fcr). Workspace admin + // (`is_admin`) stays allowed — that is the cap ceiling. + is_job_token: bool, db: &DB, ) -> Result<()> { if !is_admin { - if !is_devops_email(db, email).await? { + if is_job_token || !is_devops_email(db, email).await? { return Err(Error::RequireAdmin(username.to_string())); } }