From dc27db68de21c4d13033222cdaadbc4e2e732fe8 Mon Sep 17 00:00:00 2001 From: hugocasa Date: Sat, 22 Aug 2026 12:06:54 +0200 Subject: [PATCH] fix: require item read scope on workspace tarball export (#10797) * fix: require item read scope on workspace tarball export Co-Authored-By: Claude Opus 5 * fix: accept a wildcard path grant for whole-domain scope checks Co-Authored-By: Claude Opus 5 * fix: let a wildcard path grant delegate the unqualified scope Co-Authored-By: Claude Opus 5 --------- Co-authored-by: Claude Opus 5 Co-authored-by: Ruben Fiszel --- backend/tests/workspace_export.rs | 91 +++++++++++++++++++ backend/windmill-api-auth/src/lib.rs | 21 ++++- backend/windmill-api-auth/src/scopes.rs | 24 ++++- backend/windmill-api/src/workspaces_export.rs | 32 ++++--- 4 files changed, 153 insertions(+), 15 deletions(-) diff --git a/backend/tests/workspace_export.rs b/backend/tests/workspace_export.rs index ca988562fd..8568b8ff24 100644 --- a/backend/tests/workspace_export.rs +++ b/backend/tests/workspace_export.rs @@ -196,3 +196,94 @@ async fn test_tarball_export_all_tables(db: Pool) -> anyhow::Result<() Ok(()) } +/// The archive carries every resource's and variable's `value`, which the per-item +/// routes gate on `resources:read:` / `variables:read:`. A token holding +/// only `workspaces:read` (what the route itself needs) must not collect them, and a +/// path-scoped token cannot stand in for the whole workspace either. +#[sqlx::test(fixtures("base"))] +async fn test_tarball_export_gates_values_on_item_scopes(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + let server = ApiServer::start(db.clone()).await?; + let base_url = format!("http://localhost:{}", server.addr.port()); + + sqlx::query( + r#"INSERT INTO resource (workspace_id, path, value, resource_type, created_by) + VALUES ('test-workspace', 'u/test-user/creds', + '{"password": "RESOURCE_VALUE"}'::jsonb, 'postgresql', 'test-user')"#, + ) + .execute(&db) + .await?; + + sqlx::query( + r#"INSERT INTO variable (workspace_id, path, value, is_secret, description) + VALUES ('test-workspace', 'u/test-user/plain', 'VARIABLE_VALUE', false, '')"#, + ) + .execute(&db) + .await?; + + sqlx::query( + r#"INSERT INTO token (token_hash, token_prefix, token, email, label, super_admin, scopes) VALUES + (encode(sha256('WS_READ_TOKEN'::bytea), 'hex'), 'WS_READ_TO', 'WS_READ_TOKEN', + 'test@windmill.dev', 'workspaces:read only', false, '{workspaces:read}'), + (encode(sha256('PATH_SCOPED_TOKEN'::bytea), 'hex'), 'PATH_SCOPE', 'PATH_SCOPED_TOKEN', + 'test@windmill.dev', 'path-scoped item read', false, + '{workspaces:read,resources:read:u/test-user/creds,variables:read:u/test-user/plain}'), + (encode(sha256('ITEM_READ_TOKEN'::bytea), 'hex'), 'ITEM_READ_', 'ITEM_READ_TOKEN', + 'test@windmill.dev', 'item read', false, + '{workspaces:read,resources:read,variables:read}'), + (encode(sha256('WILDCARD_TOKEN'::bytea), 'hex'), 'WILDCARD_T', 'WILDCARD_TOKEN', + 'test@windmill.dev', 'wildcard item read', false, + '{workspaces:read,resources:read:*,variables:read:*}')"#, + ) + .execute(&db) + .await?; + + let export = async |token: &str, query: &str| -> anyhow::Result<(u16, String)> { + let resp = reqwest::Client::new() + .get(format!( + "{base_url}/api/w/test-workspace/workspaces/tarball?{query}" + )) + .bearer_auth(token) + .send() + .await?; + let status = resp.status().as_u16(); + // Lossy: a successful export is a tar, not UTF-8. Only the values matter here. + Ok(( + status, + String::from_utf8_lossy(&resp.bytes().await?).into_owned(), + )) + }; + + for token in ["WS_READ_TOKEN", "PATH_SCOPED_TOKEN"] { + let (status, body) = export(token, "").await?; + assert_eq!(status, 403, "{token} exported values: {body}"); + assert!( + !body.contains("RESOURCE_VALUE"), + "{token} leaked a resource" + ); + + let (status, body) = export(token, "skip_resources=true").await?; + assert_eq!(status, 403, "{token} exported variables: {body}"); + assert!( + !body.contains("VARIABLE_VALUE"), + "{token} leaked a variable" + ); + + // Skipping both kinds leaves an export the route's own scope covers. + let (status, body) = export(token, "skip_resources=true&skip_variables=true").await?; + assert_eq!(status, 200, "{token} denied a value-free export: {body}"); + } + + // `*` is a resource path the scope picker mints, and it spans the whole domain, + // so it must export exactly as the unqualified grant does. + for token in ["ITEM_READ_TOKEN", "WILDCARD_TOKEN"] { + let (status, body) = export(token, "").await?; + assert_eq!(status, 200, "{token} denied: {body}"); + assert!( + body.contains("RESOURCE_VALUE") && body.contains("VARIABLE_VALUE"), + "{token} exported no values" + ); + } + + Ok(()) +} diff --git a/backend/windmill-api-auth/src/lib.rs b/backend/windmill-api-auth/src/lib.rs index 658367c81f..f2392839ba 100644 --- a/backend/windmill-api-auth/src/lib.rs +++ b/backend/windmill-api-auth/src/lib.rs @@ -631,8 +631,11 @@ fn scope_contains(caller: &ScopeDefinition, requested: &ScopeDefinition) -> bool match (&caller.resource, &requested.resource) { // Caller is unrestricted on resources: covers everything. (None, _) => true, - // Caller is resource-restricted but the request is not: broader. - (Some(_), None) => false, + // Caller is resource-restricted but the request is not: broader, unless the + // caller lists `*` and so already spans every path. Kept in step with + // `ScopeDefinition::includes`, which accepts that same grant for a + // whole-collection read: what a token may exercise, it may also delegate. + (Some(caller_resources), None) => caller_resources.iter().any(|r| r == "*"), (Some(caller_resources), Some(requested_resources)) => { resource_set_contains(caller_resources, requested_resources) } @@ -1810,6 +1813,20 @@ mod tests { opt_scopes(Some(vec!["users:read", "if_jobs:filter_tags:default"])).as_deref() ) .is_ok()); + // A `*` path grant spans the domain, so it may mint the unqualified form a + // whole-collection read requires; a listed path may not. + let wildcard = authed_with_scopes(Some(vec!["resources:read:*"])); + assert!(ensure_scopes_within_caller( + &wildcard, + opt_scopes(Some(vec!["resources:read"])).as_deref() + ) + .is_ok()); + let path_scoped = authed_with_scopes(Some(vec!["resources:read:f/team/db"])); + assert!(ensure_scopes_within_caller( + &path_scoped, + opt_scopes(Some(vec!["resources:read"])).as_deref() + ) + .is_err()); // Apps `write` covers `run`, so an app-editor token can mint the run-only // credential for the same app — but only within its own resource subtree, // and the equivalence stays Apps-only. diff --git a/backend/windmill-api-auth/src/scopes.rs b/backend/windmill-api-auth/src/scopes.rs index 93b8e6c0e9..da24453553 100644 --- a/backend/windmill-api-auth/src/scopes.rs +++ b/backend/windmill-api-auth/src/scopes.rs @@ -147,7 +147,11 @@ impl ScopeDefinition { (Some(self_resources), Some(other_resources)) => { resources_match(self_resources, other_resources) } - (Some(_), None) => false, + // A requirement naming no path is the whole domain, so only a grant that + // itself spans every path satisfies it. `*` is that grant — the scope UI + // accepts it as a resource path and `resources_match` already reads it as + // everything — while any listed path leaves the collection unauthorized. + (Some(self_resources), None) => self_resources.iter().any(|r| r == "*"), (None, _) => true, } } @@ -1434,6 +1438,24 @@ mod tests { .is_ok()); } + // Whole-collection reads (the workspace export, `apps:read`, ...) require the + // domain with no path. Only a grant spanning every path may satisfy that. + #[test] + fn test_unqualified_requirement_needs_a_whole_domain_grant() { + let unqualified = ScopeDefinition::new("resources", "read", None, None); + + let wildcard = ScopeDefinition::new("resources", "read", None, Some(vec!["*".to_string()])); + assert!(wildcard.includes(&unqualified)); + + let path_scoped = ScopeDefinition::new( + "resources", + "read", + None, + Some(vec!["f/team/db".to_string(), "u/alice/db".to_string()]), + ); + assert!(!path_scoped.includes(&unqualified)); + } + #[test] fn test_resource_array_matching() { // Test wildcard access diff --git a/backend/windmill-api/src/workspaces_export.rs b/backend/windmill-api/src/workspaces_export.rs index df6e732574..9116021626 100644 --- a/backend/windmill-api/src/workspaces_export.rs +++ b/backend/windmill-api/src/workspaces_export.rs @@ -634,23 +634,27 @@ pub(crate) async fn tarball_workspace( skip_resources ); - // The route is gated by workspaces:read, but exporting DECRYPTED secrets is a - // variable-read capability beyond workspace metadata. Require variables:read - // only on the plaintext-secret path: ordinary tarball pulls (structure and - // encrypted-only values) keep working with workspaces:read, and the workspace - // key itself takes an admin *and* an unscoped token (include_key), since it - // decrypts those same secrets offline. No-op for unscoped tokens. - let export_plain_secrets = plain_secret.or(plain_secrets).unwrap_or(false) - && !skip_secrets.unwrap_or(false) - && !skip_variables.unwrap_or(false); - if export_plain_secrets { - check_scopes(&authed, || "variables:read".to_string())?; - } + // The workspace key decrypts every secret offline, so it takes an admin *and* an + // unscoped token. Checked before the item scopes below so that a scoped token + // asking for the key is told about the key rather than about a scope no token + // holding the key would need anyway. if include_key.unwrap_or(false) { require_admin(authed.is_admin, &authed.username)?; windmill_api_auth::forbid_scoped_token_workspace_key(&authed)?; } + // The route is gated by workspaces:read, but the tarball also carries the item + // values that the per-item routes gate on their own domain (get_resource_value, + // get_variable). A whole-workspace export cannot be confined to a path, so it + // takes the unrestricted domain scope: a path-scoped token has to skip that kind. + // No-op for unscoped tokens. + if !skip_resources.unwrap_or(false) { + check_scopes(&authed, || "resources:read".to_string())?; + } + if !skip_variables.unwrap_or(false) { + check_scopes(&authed, || "variables:read".to_string())?; + } + // Opt-in behavior for surfacing per-resource ACLs on flow/app rows. // Folder and group rows have always carried `extra_perms` in source and // continue to do so unconditionally (`KeepEvenEmpty`) so existing @@ -695,6 +699,10 @@ pub(crate) async fn tarball_workspace( Some(t) => Err(Error::BadRequest(format!("Invalid Archive Type {t}"))), }?; + let export_plain_secrets = plain_secret.or(plain_secrets).unwrap_or(false) + && !skip_secrets.unwrap_or(false) + && !skip_variables.unwrap_or(false); + // Record what the export is about to disclose, once nothing left can reject the // request: an entry written before the gates above would claim a disclosure that // a 403 or an invalid archive type then prevented. On the pool and before the RLS