From b5a17bdcb701a93791442915649c8f446b65199c Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Mon, 25 May 2026 14:38:28 +0000 Subject: [PATCH] fix(auth): filter resource/variable listings by token scope (WIN-1981) (#9302) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A token scoped to a single resource (e.g. `resources:read:u/alice/foo`) could call `GET /api/w/{w}/resources/list_search` and receive `path` and `value` for unrelated resources in the workspace. Route-level scope checks only validate `domain:action`; per-resource handlers do a `check_scopes` against the path, but the listing endpoints did not — leaking integration credentials, API keys, and other secrets stored as resource values to narrowly-scoped tokens. Add `build_scope_path_predicate` to `windmill-api-auth` (mirrors `check_scopes` semantics but parses the token's scopes once, suitable for filtering many rows). Apply it to `list_search_resources`, `list_resources`, `list_names` (resources) and `list_variables` (non-secret value leak), so a scope-restricted token only ever sees the paths it is authorized to read. Unscoped tokens and tokens whose only scopes are `if_jobs:filter_tags:*` are unaffected. Includes regression tests covering: unscoped, tag-filter-only, single-resource, wildcard, wrong-domain, and write-implies-read. Fixes WIN-1981 Co-authored-by: Claude Opus 4.7 (1M context) --- backend/windmill-api-auth/src/lib.rs | 106 ++++++++++++++++++++++++ backend/windmill-store/src/resources.rs | 14 +++- backend/windmill-store/src/variables.rs | 11 ++- 3 files changed, 126 insertions(+), 5 deletions(-) diff --git a/backend/windmill-api-auth/src/lib.rs b/backend/windmill-api-auth/src/lib.rs index b9bc2e2417..99fa677cee 100644 --- a/backend/windmill-api-auth/src/lib.rs +++ b/backend/windmill-api-auth/src/lib.rs @@ -235,6 +235,50 @@ where Ok(()) } +/// Returns a predicate that checks whether `path` is within the token's +/// scope for `{domain}:{action}:{path}`. For tokens without scope +/// restrictions (no scopes at all, or only `if_jobs:filter_tags:*` scopes), +/// the predicate always returns `true`. +/// +/// Pre-parses the token's scopes once so the returned closure can cheaply +/// filter large listings without re-parsing on each call. +pub fn build_scope_path_predicate( + authed: &ApiAuthed, + domain: &str, + action: &str, +) -> impl Fn(&str) -> bool { + // Mirror check_scopes semantics: a token is "scope-restricted" iff it has + // at least one non-`if_jobs:filter_tags:` scope. Unparseable scopes still + // count as restrictive — they just match nothing. + let (is_scoped_token, parsed): (bool, Vec) = match authed.scopes.as_ref() { + Some(scopes) => { + let mut is_scoped = false; + let parsed = scopes + .iter() + .filter(|s| !s.starts_with("if_jobs:filter_tags:")) + .inspect(|_| is_scoped = true) + .filter_map(|s| ScopeDefinition::from_scope_string(s).ok()) + .collect(); + (is_scoped, parsed) + } + None => (false, Vec::new()), + }; + let domain = domain.to_string(); + let action = action.to_string(); + + move |path: &str| -> bool { + if !is_scoped_token { + return true; + } + let required = + match ScopeDefinition::from_scope_string(&format!("{}:{}:{}", domain, action, path)) { + Ok(r) => r, + Err(_) => return false, + }; + parsed.iter().any(|s| s.includes(&required)) + } +} + pub async fn require_devops_role(db: &DB, email: &str) -> error::Result<()> { let is_devops = is_devops_email(db, email).await?; @@ -803,3 +847,65 @@ pub fn require_path_read_access_for_preview( ))), } } + +#[cfg(test)] +mod tests { + use super::*; + + fn authed_with_scopes(scopes: Option>) -> ApiAuthed { + ApiAuthed { + scopes: scopes.map(|v| v.into_iter().map(String::from).collect()), + ..Default::default() + } + } + + #[test] + fn predicate_no_scopes_allows_all() { + let authed = authed_with_scopes(None); + let allowed = build_scope_path_predicate(&authed, "resources", "read"); + assert!(allowed("u/alice/anything")); + assert!(allowed("u/bob/other")); + } + + #[test] + fn predicate_tag_filter_only_allows_all() { + let authed = authed_with_scopes(Some(vec!["if_jobs:filter_tags:default"])); + let allowed = build_scope_path_predicate(&authed, "resources", "read"); + assert!(allowed("u/alice/foo")); + } + + #[test] + fn predicate_single_resource_scope_filters_others() { + // Regression test for WIN-1981: a token scoped to one resource must + // not match unrelated paths in listings (e.g. /resources/list_search). + let authed = authed_with_scopes(Some(vec!["resources:read:u/alice/allowed_resource"])); + let allowed = build_scope_path_predicate(&authed, "resources", "read"); + assert!(allowed("u/alice/allowed_resource")); + assert!(!allowed("u/alice/other_resource")); + assert!(!allowed("u/bob/foo")); + } + + #[test] + fn predicate_wildcard_scope_matches_subtree() { + let authed = authed_with_scopes(Some(vec!["resources:read:f/team/*"])); + let allowed = build_scope_path_predicate(&authed, "resources", "read"); + assert!(allowed("f/team/db")); + assert!(allowed("f/team/sub/nested")); + assert!(!allowed("f/other/db")); + } + + #[test] + fn predicate_wrong_domain_is_rejected() { + let authed = authed_with_scopes(Some(vec!["variables:read:u/alice/secret"])); + let allowed = build_scope_path_predicate(&authed, "resources", "read"); + assert!(!allowed("u/alice/secret")); + } + + #[test] + fn predicate_write_implies_read() { + let authed = authed_with_scopes(Some(vec!["resources:write:u/alice/foo"])); + let allowed = build_scope_path_predicate(&authed, "resources", "read"); + assert!(allowed("u/alice/foo")); + assert!(!allowed("u/alice/bar")); + } +} diff --git a/backend/windmill-store/src/resources.rs b/backend/windmill-store/src/resources.rs index 5c17308bbf..3e5292e805 100644 --- a/backend/windmill-store/src/resources.rs +++ b/backend/windmill-store/src/resources.rs @@ -10,8 +10,8 @@ use std::collections::HashMap; use std::net::IpAddr; use windmill_api_auth::{ - check_scopes, maybe_refresh_folders, require_owner_of_path, require_super_admin, ApiAuthed, - Tokened, + build_scope_path_predicate, check_scopes, maybe_refresh_folders, require_owner_of_path, + require_super_admin, ApiAuthed, Tokened, }; use windmill_common::db::DB; use windmill_common::workspaces::{check_deploy_rules, RuleCheckResult}; @@ -194,6 +194,7 @@ async fn list_names( Extension(user_db): Extension, ) -> JsonResult> { let mut tx = user_db.begin(&authed).await?; + let allowed = build_scope_path_predicate(&authed, "resources", "read"); let rows = sqlx::query!( "SELECT value->>'name' as name, path from resource WHERE resource_type = $1 AND workspace_id = $2", rt, @@ -203,6 +204,7 @@ async fn list_names( .await? .into_iter() .filter_map(|x| x.name.map(|name| NamePath { name, path: x.path })) + .filter(|np| allowed(&np.path)) .collect::>(); tx.commit().await?; Ok(Json(rows)) @@ -225,6 +227,7 @@ async fn list_search_resources( #[cfg(not(feature = "enterprise"))] let n = 3; + let allowed = build_scope_path_predicate(&authed, "resources", "read"); let rows = sqlx::query_as!( SearchResource, "SELECT path, value from resource WHERE workspace_id = $1 LIMIT $2", @@ -234,6 +237,7 @@ async fn list_search_resources( .fetch_all(&mut *tx) .await? .into_iter() + .filter(|r| allowed(&r.path)) .collect::>(); tx.commit().await?; Ok(Json(rows)) @@ -338,9 +342,13 @@ async fn list_resources( let sql = sqlb.sql().map_err(|e| Error::internal_err(e.to_string()))?; let mut tx = user_db.begin(&authed).await?; + let allowed = build_scope_path_predicate(&authed, "resources", "read"); let rows = sqlx::query_as::<_, ListableResource>(&sql) .fetch_all(&mut *tx) - .await?; + .await? + .into_iter() + .filter(|r| allowed(&r.path)) + .collect::>(); tx.commit().await?; diff --git a/backend/windmill-store/src/variables.rs b/backend/windmill-store/src/variables.rs index 4c2a0ed670..2d767b393b 100644 --- a/backend/windmill-store/src/variables.rs +++ b/backend/windmill-store/src/variables.rs @@ -6,7 +6,10 @@ * LICENSE-AGPL for a copy of the license. */ -use windmill_api_auth::{check_scopes, maybe_refresh_folders, require_owner_of_path, ApiAuthed}; +use windmill_api_auth::{ + build_scope_path_predicate, check_scopes, maybe_refresh_folders, require_owner_of_path, + ApiAuthed, +}; use windmill_common::db::DB; use windmill_common::workspaces::{check_deploy_rules, RuleCheckResult}; @@ -188,9 +191,13 @@ async fn list_variables( let sql = sqlb.sql().map_err(|e| Error::internal_err(e.to_string()))?; let mut tx = user_db.begin(&authed).await?; + let allowed = build_scope_path_predicate(&authed, "variables", "read"); let rows = sqlx::query_as::<_, ListableVariable>(&sql) .fetch_all(&mut *tx) - .await?; + .await? + .into_iter() + .filter(|r| allowed(&r.path)) + .collect::>(); tx.commit().await?; Ok(Json(rows))