From d6caffe9fc14c2271c45cdf76fae87b85683fda0 Mon Sep 17 00:00:00 2001 From: hugocasa Date: Thu, 6 Aug 2026 10:23:17 +0200 Subject: [PATCH] fix: stop job tokens minting credentials that shed their provenance Co-Authored-By: Claude Fable 5 --- .../tests/fixtures/preserve_on_behalf_of.sql | 5 + backend/tests/wm_token_superadmin_guard.rs | 107 +++++++++++++++--- backend/windmill-api-auth/src/lib.rs | 27 +++++ .../src/concurrency_groups.rs | 10 +- backend/windmill-api-users/src/users.rs | 28 +++-- 5 files changed, 150 insertions(+), 27 deletions(-) diff --git a/backend/tests/fixtures/preserve_on_behalf_of.sql b/backend/tests/fixtures/preserve_on_behalf_of.sql index ff8428a1c5..7c6c4fee57 100644 --- a/backend/tests/fixtures/preserve_on_behalf_of.sql +++ b/backend/tests/fixtures/preserve_on_behalf_of.sql @@ -36,6 +36,11 @@ INSERT INTO password(email, password_hash, login_type, super_admin, verified, na VALUES ('test2@windmill.dev', 'not-a-real-hash', 'password', false, true, 'Test User 2') ON CONFLICT DO NOTHING; +-- Instance devops user (not a superadmin): the tier a token mint must not launder +INSERT INTO password(email, password_hash, login_type, super_admin, devops, verified, name) + VALUES ('devops@windmill.dev', 'not-a-real-hash', 'password', false, true, true, 'Devops User') +ON CONFLICT DO NOTHING; + -- Deployer user (non-admin but in wm_deployers group) INSERT INTO password(email, password_hash, login_type, super_admin, verified, name) VALUES ('deployer@windmill.dev', 'not-a-real-hash', 'password', false, true, 'Deployer User') diff --git a/backend/tests/wm_token_superadmin_guard.rs b/backend/tests/wm_token_superadmin_guard.rs index 83188fc975..02b9d750d8 100644 --- a/backend/tests/wm_token_superadmin_guard.rs +++ b/backend/tests/wm_token_superadmin_guard.rs @@ -323,9 +323,12 @@ async fn test_wm_token_rejected_by_require_devops_role(db: Pool) -> an let base = format!("http://localhost:{port}/api"); let sa_wm = wm_token("test@windmill.dev", true).await; - let resp = authed(client().get(format!("{base}/service_logs/list_files")), &sa_wm) - .send() - .await?; + let resp = authed( + client().get(format!("{base}/service_logs/list_files")), + &sa_wm, + ) + .send() + .await?; assert_eq!( resp.status(), 401, @@ -423,12 +426,10 @@ async fn test_wm_token_rejected_by_instance_admin_gates(db: Pool) -> a set_jwt_secret().await; // A worker-group config carrying a static env value that must stay masked. - sqlx::query( - "INSERT INTO config (name, config) VALUES ('worker__wm2082grp', $1)", - ) - .bind(json!({ "env_vars_static": { "LEAKY": "supersecretvalue" } })) - .execute(&db) - .await?; + sqlx::query("INSERT INTO config (name, config) VALUES ('worker__wm2082grp', $1)") + .bind(json!({ "env_vars_static": { "LEAKY": "supersecretvalue" } })) + .execute(&db) + .await?; let server = ApiServer::start(db.clone()).await?; let port = server.addr.port(); @@ -466,12 +467,30 @@ async fn test_wm_token_rejected_by_instance_admin_gates(db: Pool) -> a resp.text().await? ); - // 3. Worker-group config: the static env value must be masked for a job token. - let body = authed(client().get(format!("{base}/configs/list_worker_groups")), &sa_wm) - .send() - .await? - .text() - .await?; + // 3. The sibling listing spans every workspace's concurrency keys, so it is + // gated the same way as the prune above. + let resp = authed( + client().get(format!("{base}/concurrency_groups/list")), + &sa_wm, + ) + .send() + .await?; + assert_eq!( + resp.status(), + 401, + "superadmin WM_TOKEN must not list global concurrency groups: {}", + resp.text().await? + ); + + // 4. Worker-group config: the static env value must be masked for a job token. + let body = authed( + client().get(format!("{base}/configs/list_worker_groups")), + &sa_wm, + ) + .send() + .await? + .text() + .await?; assert!( !body.contains("supersecretvalue"), "superadmin WM_TOKEN must get the obfuscated worker-group view: {body}" @@ -494,3 +513,61 @@ async fn test_wm_token_rejected_by_instance_admin_gates(db: Pool) -> a Ok(()) } + +/// Capping a `WM_TOKEN` at the gates is only durable if the token cannot trade +/// itself for one without the `job_id` those gates key off. Both credential-minting +/// routes must therefore refuse an elevated job token: `refresh_token` (which mints +/// a database-backed session token and returns it in `Set-Cookie`) and +/// `tokens/create` for the `devops` tier, whose routes are capped just like +/// superadmin's (GHSA-hfh4-cx4h-3fcr). +#[sqlx::test(fixtures("preserve_on_behalf_of"))] +async fn test_wm_token_cannot_mint_a_provenance_free_credential( + 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/users"); + + // 1. Session refresh: a superadmin-identity job token must not obtain a session + // token, which would authenticate with no job provenance at all. + let sa_wm = wm_token("test@windmill.dev", true).await; + let resp = authed(client().get(format!("{base}/refresh_token")), &sa_wm) + .send() + .await?; + assert_eq!( + resp.status(), + 401, + "superadmin WM_TOKEN must not refresh into a session token: {}", + resp.text().await? + ); + + // No false positive: a real superadmin API token still refreshes. + let resp = authed(client().get(format!("{base}/refresh_token")), "SECRET_TOKEN") + .send() + .await?; + assert_eq!( + resp.status(), + 200, + "a real superadmin token must still refresh: {}", + resp.text().await? + ); + + // 2. Token mint, devops tier: `require_devops_role` rejects this job token, so + // minting one that would pass it by email must be refused too. + let devops_wm = wm_token("devops@windmill.dev", false).await; + let resp = authed(client().post(format!("{base}/tokens/create")), &devops_wm) + .json(&json!({ "label": "from-script" })) + .send() + .await?; + assert_eq!( + resp.status(), + 401, + "devops WM_TOKEN must not mint a token: {}", + resp.text().await? + ); + + Ok(()) +} diff --git a/backend/windmill-api-auth/src/lib.rs b/backend/windmill-api-auth/src/lib.rs index 8b4ffec910..5aea84c0fe 100644 --- a/backend/windmill-api-auth/src/lib.rs +++ b/backend/windmill-api-auth/src/lib.rs @@ -354,6 +354,33 @@ pub async fn forbid_superadmin_job_token( Ok(()) } +/// Forbid *minting a durable credential* from a job token that carries an elevated +/// instance identity (superadmin or `devops`; [`is_devops_email`] covers both). +/// +/// The gates that cap `$WM_TOKEN` key off `ApiAuthed::job_id`, which only a job +/// token carries. A token minted from one is an ordinary database-backed token with +/// no such provenance, so it passes every one of those gates by email alone — the +/// cap would last only until the script exchanged its token for a fresh one +/// (GHSA-hfh4-cx4h-3fcr). Narrower than rejecting all job tokens: a script running +/// as an unprivileged identity has nothing to launder and still mints freely. +pub async fn forbid_elevated_job_token( + db: &DB, + email: &str, + job_id: Option, +) -> error::Result<()> { + if job_id.is_some() && is_devops_email(db, email).await? { + return Err(Error::NotAuthorized( + "A job token ($WM_TOKEN) running as a superadmin or devops user cannot mint a new \ + token, which would carry that identity without the job provenance that caps it. \ + If a script genuinely needs this, create a dedicated token from the User settings \ + drawer (the 'Tokens' section), store it as a secret, and use that token explicitly \ + instead of $WM_TOKEN." + .to_owned(), + )); + } + Ok(()) +} + pub fn check_scopes(authed: &ApiAuthed, required: F) -> error::Result<()> where F: FnOnce() -> String, diff --git a/backend/windmill-api-jobs/src/concurrency_groups.rs b/backend/windmill-api-jobs/src/concurrency_groups.rs index 821b468c43..2c8f900d2b 100644 --- a/backend/windmill-api-jobs/src/concurrency_groups.rs +++ b/backend/windmill-api-jobs/src/concurrency_groups.rs @@ -1,9 +1,8 @@ -use windmill_api_auth::{check_scopes, is_instance_admin, ApiAuthed}; +use windmill_api_auth::{check_scopes, is_instance_admin, require_instance_admin, ApiAuthed}; use windmill_common::{ db::{UserDB, DB}, error::Error::PermissionDenied, error::{self, JsonResult}, - utils::require_admin, }; use crate::query::{filter_list_completed_query, filter_list_queue_query}; @@ -43,7 +42,9 @@ async fn list_concurrency_groups( authed: ApiAuthed, Extension(db): Extension, ) -> JsonResult> { - require_admin(authed.is_admin, &authed.username)?; + // Instance-global: the listing spans every workspace's concurrency keys, so a job + // token's workspace-admin claim must not reach it (mirrors the prune route below). + require_instance_admin(&authed)?; let concurrency_counts = sqlx::query_as::<_, (String, i64)>( "SELECT concurrency_id, (select COUNT(*) from jsonb_object_keys(job_uuids)) as n_job_uuids FROM concurrency_counter", @@ -284,7 +285,8 @@ async fn get_concurrent_intervals( // This second transaction uses the db, so it will fetch information // potentially forbidden to the user. It must be obscured before // returning it - let running_jobs_db: Vec = if lq.success.is_none() && lq.resolved != Some(true) { + let running_jobs_db: Vec = if lq.success.is_none() && lq.resolved != Some(true) + { sqlx::query_as(&sql_q).fetch_all(&db).await? } else { vec![] diff --git a/backend/windmill-api-users/src/users.rs b/backend/windmill-api-users/src/users.rs index 8093e39628..7f2eb8fb4d 100644 --- a/backend/windmill-api-users/src/users.rs +++ b/backend/windmill-api-users/src/users.rs @@ -27,7 +27,9 @@ use axum::{ Json, Router, }; use hyper::{header::LOCATION, StatusCode}; -use windmill_api_auth::{forbid_superadmin_job_token, require_super_admin, OptJobAuthed}; +use windmill_api_auth::{ + forbid_elevated_job_token, forbid_superadmin_job_token, require_super_admin, OptJobAuthed, +}; use windmill_common::usernames::{ generate_instance_wide_unique_username, get_instance_username_or_create_pending, }; @@ -2312,12 +2314,10 @@ async fn change_user_email( // Read back inside the transaction: the address is derived at dispatch through a cache // that nothing else evicts, so without this a job pushed in the next 60s would resolve // the old address and with it the wrong superadmin flag and instance groups. - let memberships = sqlx::query_scalar!( - "SELECT workspace_id FROM usr WHERE email = $1", - &new_email - ) - .fetch_all(&mut *tx) - .await?; + let memberships = + sqlx::query_scalar!("SELECT workspace_id FROM usr WHERE email = $1", &new_email) + .fetch_all(&mut *tx) + .await?; tx.commit().await?; @@ -2762,6 +2762,18 @@ async fn refresh_token( authed: ApiAuthed, cookies: Cookies, ) -> Result { + // The session token minted below is database-backed and carries no job provenance, + // so a job token that exchanged itself for one would shed the `job_id` every + // `$WM_TOKEN` cap keys off (GHSA-hfh4-cx4h-3fcr). Only a browser session refreshes. + if authed.job_id.is_some() { + return Err(Error::NotAuthorized( + "This endpoint cannot be called with a job token ($WM_TOKEN). If a script \ + genuinely needs a token of its own, create a dedicated token from the User \ + settings drawer (the 'Tokens' section), store it as a secret, and use that \ + token explicitly instead of $WM_TOKEN." + .to_string(), + )); + } if let Some(thresh_s) = query.if_expiring_in_less_than_s { let t_hash = windmill_common::auth::hash_token(&token); let not_expired = sqlx::query_scalar!("SELECT true FROM token WHERE token_hash = $1 and expiration IS NOT NULL and expiration > now() + $2::int * '1 sec'::interval", &t_hash, thresh_s) @@ -2905,7 +2917,7 @@ async fn create_token( OptJobAuthed { job_id, .. }: OptJobAuthed, Json(token_config): Json, ) -> Result<(StatusCode, String)> { - forbid_superadmin_job_token(&db, &authed.email, job_id).await?; + forbid_elevated_job_token(&db, &authed.email, job_id).await?; check_token_create_rate_limit(&authed.username)?; // `username_override_from_label` trusts a server-minted label to name the entity acting,