From a31893873f11f29314da60f696f87696563d8de0 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Fri, 22 May 2026 14:30:01 +0000 Subject: [PATCH] fix(auth): tighten token-owner fallback for unscoped tokens (WIN-1978) (#9293) * fix(auth): reject unscoped tokens with cross-workspace forged owners (WIN-1978) An unscoped token (workspace_id IS NULL) whose `owner` field references a user, group, or unprefixed value that is not present in the target workspace must not authenticate. The previous fallback in the `u/` branch granted `(is_admin=false, is_operator=true)` when no `usr` row matched in the target workspace, letting a token holder who could mutate the `token` table cross workspace boundaries with operator privileges. The `g/` branch likewise silently accepted any group name as a "group user", and the no-prefix branch granted operator state from arbitrary owner strings. Both are now rejected unless the owner matches a real user/group membership in the target workspace. Adds an integration regression covering all three forged-owner shapes. Co-Authored-By: Claude Opus 4.7 (1M context) * chore: drop integration regression for auth fallback The test added in the previous commit relies on a sqlx::query! that requires offline-cache regeneration; removing per code-review preference to keep this PR scoped to the auth-layer fix. Co-Authored-By: Claude Opus 4.7 (1M context) --------- Co-authored-by: Claude Opus 4.7 (1M context) --- backend/windmill-api-auth/src/auth.rs | 173 +++++++++++++++----------- 1 file changed, 101 insertions(+), 72 deletions(-) diff --git a/backend/windmill-api-auth/src/auth.rs b/backend/windmill-api-auth/src/auth.rs index bda9f54bdd..314ba99450 100644 --- a/backend/windmill-api-auth/src/auth.rs +++ b/backend/windmill-api-auth/src/auth.rs @@ -250,92 +250,121 @@ impl AuthCache { let username_override = username_override_from_label(label); if let Some((prefix, name)) = owner.split_once('/') { if prefix == "u" { - let (is_admin, is_operator) = if super_admin { - (true, false) + let lookup = if super_admin { + Some((true, false)) } else { - let r = sqlx::query!( + sqlx::query!( "SELECT is_admin, operator FROM usr where username = $1 AND \ workspace_id = $2 AND disabled = false", name, &w_id.as_ref().unwrap() ) - .fetch_one(&self.db) + .fetch_optional(&self.db) .await - .ok(); - if let Some(r) = r { - (r.is_admin, r.operator) - } else { - (false, true) - } + .ok() + .flatten() + .map(|r| (r.is_admin, r.operator)) }; - let w_id = &w_id.unwrap(); - let groups = - get_groups_for_user(w_id, &name, &email, &self.db) - .await - .ok() - .unwrap_or_default(); + if let Some((is_admin, is_operator)) = lookup { + let w_id = &w_id.unwrap(); + let groups = + get_groups_for_user(w_id, &name, &email, &self.db) + .await + .ok() + .unwrap_or_default(); - let folders = - get_folders_for_user(w_id, &name, &groups, &self.db) - .await - .ok() - .unwrap_or_default(); + let folders = get_folders_for_user( + w_id, &name, &groups, &self.db, + ) + .await + .ok() + .unwrap_or_default(); - Some(ApiAuthed { - email: email, - username: name.to_string(), - is_admin, - is_operator, - groups, - folders, - scopes: None, - username_override, - token_prefix: Some(safe_token_prefix(token)), - read_only, - }) + Some(ApiAuthed { + email: email, + username: name.to_string(), + is_admin, + is_operator, + groups, + folders, + scopes: None, + username_override, + token_prefix: Some(safe_token_prefix(token)), + read_only, + }) + } else { + tracing::warn!( + "Token owner u/{} is not a member of workspace {}; rejecting auth", + name, + w_id.as_deref().unwrap_or("") + ); + None + } + } else if prefix == "g" { + let group_exists = if super_admin { + true + } else { + sqlx::query_scalar!( + "SELECT EXISTS(SELECT 1 FROM group_ WHERE workspace_id = $1 AND name = $2)", + &w_id.as_ref().unwrap(), + name, + ) + .fetch_one(&self.db) + .await + .ok() + .flatten() + .unwrap_or(false) + }; + + if group_exists { + let groups = vec![name.to_string()]; + let folders = get_folders_for_user( + &w_id.unwrap(), + "", + &groups, + &self.db, + ) + .await + .ok() + .unwrap_or_default(); + Some(ApiAuthed { + email: email, + username: format!( + "{}{name}", + windmill_common::users::USERNAME_GROUP_PREFIX + ), + is_admin: false, + groups, + is_operator: false, + folders, + scopes: None, + username_override, + token_prefix: Some(safe_token_prefix(token)), + read_only, + }) + } else { + tracing::warn!( + "Token owner g/{} is not a group in workspace {}; rejecting auth", + name, + w_id.as_deref().unwrap_or("") + ); + None + } } else { - let groups = vec![name.to_string()]; - let folders = get_folders_for_user( - &w_id.unwrap(), - "", - &groups, - &self.db, - ) - .await - .ok() - .unwrap_or_default(); - Some(ApiAuthed { - email: email, - username: format!( - "{}{name}", - windmill_common::users::USERNAME_GROUP_PREFIX - ), - is_admin: false, - groups, - is_operator: false, - folders, - scopes: None, - username_override, - token_prefix: Some(safe_token_prefix(token)), - read_only, - }) + tracing::warn!( + "Token owner '{}' has unrecognised prefix '{}'; rejecting auth", + owner, + prefix + ); + None } } else { - let groups = vec![]; - let folders = vec![]; - Some(ApiAuthed { - email: email, - username: owner, - is_admin: super_admin, - is_operator: true, - groups, - folders, - scopes: None, - username_override, - token_prefix: Some(safe_token_prefix(token)), - read_only, - }) + tracing::warn!( + "Token owner '{}' is missing a prefix (expected u/ or g/); rejecting auth", + owner + ); + None } } (_, Some(email), super_admin, scopes, label, read_only) => {