From a3ffd0b41e552ae6ec5f13fa7a852945589973b2 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Tue, 4 Aug 2026 08:52:13 +0000 Subject: [PATCH] fix: validate a dispatch address against the principal's live binding --- ...955c38de41c29e8ee182ddfada92ea6c7ff34.json | 22 +++++++ ...6df004f8cb7091588309a1f484b91fe7183fc.json | 35 +++++++++++ backend/windmill-common/src/auth.rs | 58 ++++++++++++++----- .../tests/permissioned_as_authz.rs | 50 ++++++++++++++++ 4 files changed, 150 insertions(+), 15 deletions(-) create mode 100644 backend/.sqlx/query-61ecedf0e73a136073dfbcfbc2f955c38de41c29e8ee182ddfada92ea6c7ff34.json create mode 100644 backend/.sqlx/query-dada076fe622e9902606bf95f9e6df004f8cb7091588309a1f484b91fe7183fc.json create mode 100644 backend/windmill-common/tests/permissioned_as_authz.rs diff --git a/backend/.sqlx/query-61ecedf0e73a136073dfbcfbc2f955c38de41c29e8ee182ddfada92ea6c7ff34.json b/backend/.sqlx/query-61ecedf0e73a136073dfbcfbc2f955c38de41c29e8ee182ddfada92ea6c7ff34.json new file mode 100644 index 0000000000..6742c20a28 --- /dev/null +++ b/backend/.sqlx/query-61ecedf0e73a136073dfbcfbc2f955c38de41c29e8ee182ddfada92ea6c7ff34.json @@ -0,0 +1,22 @@ +{ + "db_name": "PostgreSQL", + "query": "SELECT email FROM password WHERE (username = $1 OR email = $1) AND super_admin = true ORDER BY email LIMIT 1", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "email", + "type_info": "Varchar" + } + ], + "parameters": { + "Left": [ + "Text" + ] + }, + "nullable": [ + false + ] + }, + "hash": "61ecedf0e73a136073dfbcfbc2f955c38de41c29e8ee182ddfada92ea6c7ff34" +} diff --git a/backend/.sqlx/query-dada076fe622e9902606bf95f9e6df004f8cb7091588309a1f484b91fe7183fc.json b/backend/.sqlx/query-dada076fe622e9902606bf95f9e6df004f8cb7091588309a1f484b91fe7183fc.json new file mode 100644 index 0000000000..669e78201d --- /dev/null +++ b/backend/.sqlx/query-dada076fe622e9902606bf95f9e6df004f8cb7091588309a1f484b91fe7183fc.json @@ -0,0 +1,35 @@ +{ + "db_name": "PostgreSQL", + "query": "SELECT is_admin, operator, email FROM usr where username = $1 AND workspace_id = $2 AND disabled = false", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "is_admin", + "type_info": "Bool" + }, + { + "ordinal": 1, + "name": "operator", + "type_info": "Bool" + }, + { + "ordinal": 2, + "name": "email", + "type_info": "Varchar" + } + ], + "parameters": { + "Left": [ + "Text", + "Text" + ] + }, + "nullable": [ + false, + false, + false + ] + }, + "hash": "dada076fe622e9902606bf95f9e6df004f8cb7091588309a1f484b91fe7183fc" +} diff --git a/backend/windmill-common/src/auth.rs b/backend/windmill-common/src/auth.rs index 8dfc3fc324..b072999501 100644 --- a/backend/windmill-common/src/auth.rs +++ b/backend/windmill-common/src/auth.rs @@ -377,6 +377,44 @@ async fn fetch_authed_from_permissioned_as_inner( w_id: &str, conn: &mut sqlx::PgConnection, ) -> Result { + // The `usr` row is the live binding between a `u/` principal and an address, and it is read + // here anyway for the workspace role. Callers may hand us a cached address, so read it before + // anything is granted: `super_admin` and `email_to_igroup` below are keyed on the address + // while the role is keyed on the principal, and an address that no longer belongs to this + // principal — a username freed and reassigned while its previous holder keeps a privileged + // account — would mix one account's role with another's instance privileges. + let member = match permissioned_as.split_once('/') { + Some(("u", name)) => sqlx::query!( + "SELECT is_admin, operator, email FROM usr where username = $1 AND \ + workspace_id = $2 AND disabled = false", + name, + &w_id + ) + .fetch_optional(&mut *conn) + .await?, + _ => None, + }; + let resolved_email; + let email = match member.as_ref() { + Some(m) => m.email.as_str(), + // No `usr` row: the principal names an account only `password` knows, so validate against + // the same fallback `resolve_username_to_email` uses. Reached only for a superadmin acting + // outside their workspaces, which is why it is worth a query the member path does not pay. + None => match permissioned_as.split_once('/') { + Some(("u", name)) => { + resolved_email = sqlx::query_scalar!( + "SELECT email FROM password WHERE (username = $1 OR email = $1) \ + AND super_admin = true ORDER BY email LIMIT 1", + name + ) + .fetch_optional(&mut *conn) + .await?; + resolved_email.as_deref().unwrap_or(email) + } + _ => email, + }, + }; + let is_super_admin = permissioned_as == SUPERADMIN_SYNC_EMAIL || email == SUPERADMIN_SECRET_EMAIL || email == SUPERADMIN_NOTIFICATION_EMAIL @@ -390,22 +428,12 @@ async fn fetch_authed_from_permissioned_as_inner( if prefix == "u" { let (is_admin, is_operator) = if is_super_admin { (true, false) + } else if let Some(m) = member.as_ref() { + (m.is_admin, m.operator) } else { - let r = sqlx::query!( - "SELECT is_admin, operator FROM usr where username = $1 AND \ - workspace_id = $2 AND disabled = false", - name, - &w_id - ) - .fetch_optional(&mut *conn) - .await?; - if let Some(r) = r { - (r.is_admin, r.operator) - } else { - return Err(Error::NotFound(format!( - "user {name} not found in workspace {w_id}" - ))); - } + return Err(Error::NotFound(format!( + "user {name} not found in workspace {w_id}" + ))); }; let groups = get_groups_for_user(w_id, &name, email, &mut *conn).await?; diff --git a/backend/windmill-common/tests/permissioned_as_authz.rs b/backend/windmill-common/tests/permissioned_as_authz.rs new file mode 100644 index 0000000000..c4a382d59b --- /dev/null +++ b/backend/windmill-common/tests/permissioned_as_authz.rs @@ -0,0 +1,50 @@ +use sqlx::{Pool, Postgres}; +use windmill_common::auth::fetch_authed_from_permissioned_as; + +/// The address handed to `fetch_authed_from_permissioned_as` may come from a cache that a +/// username reassignment has outrun. It must not be believed: the workspace role is keyed on the +/// principal while `super_admin` and `email_to_igroup` are keyed on the address, so trusting a +/// stale one would run the new holder's job with the previous holder's instance privileges. +#[sqlx::test(migrations = "../migrations", fixtures("base"))] +async fn test_stale_address_cannot_carry_the_previous_holders_privileges(db: Pool) { + // `test-user` in the fixture is a superadmin with the address `test@windmill.dev`. Free the + // username and hand it to somebody who is not, exactly as an offboard-then-onboard would. + sqlx::query("DELETE FROM usr WHERE workspace_id = 'test-workspace' AND username = 'test-user'") + .execute(&db) + .await + .expect("free the username"); + sqlx::query( + "INSERT INTO password(email, password_hash, login_type, super_admin, verified, name) + VALUES ('newcomer@windmill.dev', 'x', 'password', false, true, 'Newcomer')", + ) + .execute(&db) + .await + .expect("create the new account"); + sqlx::query( + "INSERT INTO usr(workspace_id, email, username, is_admin, role) + VALUES ('test-workspace', 'newcomer@windmill.dev', 'test-user', false, 'User')", + ) + .execute(&db) + .await + .expect("reassign the username"); + + // What a replica that has not yet consumed the eviction would pass: the principal is the + // reassigned username, the address is the one it cached for the previous holder. + let authed = fetch_authed_from_permissioned_as( + "u/test-user", + "test@windmill.dev", + "test-workspace", + &db, + ) + .await + .expect("should authenticate the current holder"); + + assert_eq!( + authed.email, "newcomer@windmill.dev", + "the principal's live address must win over the one supplied" + ); + assert!( + !authed.is_admin, + "the new holder must not inherit the previous holder's superadmin" + ); +}