From 227f216add83129842e9bf8ff26cf6efb4e37de0 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Fri, 18 Sep 2026 08:20:53 +0200 Subject: [PATCH] fix: keep ACL grants out of resource and variable permission-denied errors Co-Authored-By: Claude Opus 5 (1M context) --- ...4aa73e84d35bb68a5911985139bbf94c1d231.json | 23 --------- ...d704d6bb5d5686a02b72de2cb61e3c127cb54.json | 23 --------- ...66b0726e11ddd7511f1d503b1df29dee71419.json | 23 --------- .../tests/resources.rs | 33 ++++++++++++ .../tests/variables.rs | 33 ++++++++++++ backend/windmill-store/src/lib.rs | 31 +++++++++++ backend/windmill-store/src/resources.rs | 34 +++---------- backend/windmill-store/src/variables.rs | 51 +++---------------- 8 files changed, 111 insertions(+), 140 deletions(-) delete mode 100644 backend/.sqlx/query-108e4c505168381b51ad298b5294aa73e84d35bb68a5911985139bbf94c1d231.json delete mode 100644 backend/.sqlx/query-7813908c080e239fd6b9df3711fd704d6bb5d5686a02b72de2cb61e3c127cb54.json delete mode 100644 backend/.sqlx/query-a90924854cbd98bb16a2ed02e7966b0726e11ddd7511f1d503b1df29dee71419.json diff --git a/backend/.sqlx/query-108e4c505168381b51ad298b5294aa73e84d35bb68a5911985139bbf94c1d231.json b/backend/.sqlx/query-108e4c505168381b51ad298b5294aa73e84d35bb68a5911985139bbf94c1d231.json deleted file mode 100644 index 8f22fed492..0000000000 --- a/backend/.sqlx/query-108e4c505168381b51ad298b5294aa73e84d35bb68a5911985139bbf94c1d231.json +++ /dev/null @@ -1,23 +0,0 @@ -{ - "db_name": "PostgreSQL", - "query": "SELECT extra_perms from resource WHERE path = $1 AND workspace_id = $2", - "describe": { - "columns": [ - { - "ordinal": 0, - "name": "extra_perms", - "type_info": "Jsonb" - } - ], - "parameters": { - "Left": [ - "Text", - "Text" - ] - }, - "nullable": [ - false - ] - }, - "hash": "108e4c505168381b51ad298b5294aa73e84d35bb68a5911985139bbf94c1d231" -} diff --git a/backend/.sqlx/query-7813908c080e239fd6b9df3711fd704d6bb5d5686a02b72de2cb61e3c127cb54.json b/backend/.sqlx/query-7813908c080e239fd6b9df3711fd704d6bb5d5686a02b72de2cb61e3c127cb54.json deleted file mode 100644 index a2cd73e737..0000000000 --- a/backend/.sqlx/query-7813908c080e239fd6b9df3711fd704d6bb5d5686a02b72de2cb61e3c127cb54.json +++ /dev/null @@ -1,23 +0,0 @@ -{ - "db_name": "PostgreSQL", - "query": "SELECT extra_perms from folder WHERE name = $1 AND workspace_id = $2", - "describe": { - "columns": [ - { - "ordinal": 0, - "name": "extra_perms", - "type_info": "Jsonb" - } - ], - "parameters": { - "Left": [ - "Text", - "Text" - ] - }, - "nullable": [ - false - ] - }, - "hash": "7813908c080e239fd6b9df3711fd704d6bb5d5686a02b72de2cb61e3c127cb54" -} diff --git a/backend/.sqlx/query-a90924854cbd98bb16a2ed02e7966b0726e11ddd7511f1d503b1df29dee71419.json b/backend/.sqlx/query-a90924854cbd98bb16a2ed02e7966b0726e11ddd7511f1d503b1df29dee71419.json deleted file mode 100644 index 2eb0b4cfc3..0000000000 --- a/backend/.sqlx/query-a90924854cbd98bb16a2ed02e7966b0726e11ddd7511f1d503b1df29dee71419.json +++ /dev/null @@ -1,23 +0,0 @@ -{ - "db_name": "PostgreSQL", - "query": "SELECT extra_perms from variable WHERE path = $1 AND workspace_id = $2", - "describe": { - "columns": [ - { - "ordinal": 0, - "name": "extra_perms", - "type_info": "Jsonb" - } - ], - "parameters": { - "Left": [ - "Text", - "Text" - ] - }, - "nullable": [ - false - ] - }, - "hash": "a90924854cbd98bb16a2ed02e7966b0726e11ddd7511f1d503b1df29dee71419" -} diff --git a/backend/windmill-api-integration-tests/tests/resources.rs b/backend/windmill-api-integration-tests/tests/resources.rs index 53648ec759..a12d1f8589 100644 --- a/backend/windmill-api-integration-tests/tests/resources.rs +++ b/backend/windmill-api-integration-tests/tests/resources.rs @@ -566,6 +566,39 @@ async fn test_resource_value_cache_is_identity_scoped(db: Pool) -> any Ok(()) } +/// A denial confirms the resource exists but must not reveal who holds grants on it or on +/// its folder: test-user-2's grants must never appear in test-user-3's error. +#[sqlx::test(migrations = "../migrations", fixtures("base", "resource_cache_rls"))] +async fn test_resource_denial_hides_grants(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + sqlx::query( + "UPDATE resource SET extra_perms = '{\"u/test-user-2\": true}' + WHERE workspace_id = 'test-workspace' AND path = 'f/secret/cache_target'", + ) + .execute(&db) + .await?; + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + + for endpoint in ["get_value", "get_value_interpolated"] { + let resp = client() + .get(resource_url(port, endpoint, "f/secret/cache_target")) + .header("Authorization", "Bearer SECRET_TOKEN_3") + .send() + .await?; + assert_eq!(resp.status(), 401); + let body = resp.text().await?; + assert!( + !body.contains("test-user-2"), + "{endpoint} leaked grants: {body}" + ); + assert!(body.contains("exists but you don't have access"), "{body}"); + assert!(body.contains("folder secret"), "{body}"); + } + + Ok(()) +} + /// A resource whose value contains a `$WM_*` contextual variable (e.g. `$WM_TOKEN`) is /// job-dependent and must NEVER be cached — even when first read WITHOUT a `job_id`, where the /// placeholder is left unresolved (caching that would serve a stale placeholder to a later job diff --git a/backend/windmill-api-integration-tests/tests/variables.rs b/backend/windmill-api-integration-tests/tests/variables.rs index e5018f4f97..a53a19fee7 100644 --- a/backend/windmill-api-integration-tests/tests/variables.rs +++ b/backend/windmill-api-integration-tests/tests/variables.rs @@ -282,6 +282,39 @@ async fn test_variable_value_cache_is_identity_scoped(db: Pool) -> any Ok(()) } +/// A denial confirms the variable exists but must not reveal who holds grants on it or on +/// its folder: test-user-2's grants must never appear in test-user-3's error. +#[sqlx::test(migrations = "../migrations", fixtures("base", "variable_cache_rls"))] +async fn test_variable_denial_hides_grants(db: Pool) -> anyhow::Result<()> { + initialize_tracing().await; + sqlx::query( + "UPDATE variable SET extra_perms = '{\"u/test-user-2\": true}' + WHERE workspace_id = 'test-workspace' AND path = 'f/secret/cache_target_var'", + ) + .execute(&db) + .await?; + let server = ApiServer::start(db.clone()).await?; + let port = server.addr.port(); + + for endpoint in ["get", "get_value"] { + let resp = client() + .get(variable_url(port, endpoint, "f/secret/cache_target_var")) + .header("Authorization", "Bearer SECRET_TOKEN_3") + .send() + .await?; + assert_eq!(resp.status(), 401); + let body = resp.text().await?; + assert!( + !body.contains("test-user-2"), + "{endpoint} leaked grants: {body}" + ); + assert!(body.contains("exists but you don't have access"), "{body}"); + assert!(body.contains("folder secret"), "{body}"); + } + + Ok(()) +} + /// Secret variables ARE cached (with their per-read side effects — the EE /// `variables.decrypt_secret` audit and running-job secret registration — re-run on every /// hit; that re-emission is not observable in the OSS build since `audit_log` is a no-op). diff --git a/backend/windmill-store/src/lib.rs b/backend/windmill-store/src/lib.rs index b14466c79b..4e353577ed 100644 --- a/backend/windmill-store/src/lib.rs +++ b/backend/windmill-store/src/lib.rs @@ -17,3 +17,34 @@ pub mod variables; #[cfg(all(test, feature = "oauth2", feature = "private", feature = "enterprise"))] mod oauth_refresh_secret_backend_tests; + +use windmill_common::{db::Authable, error::Error}; + +/// Denial message for an item the caller can see the existence of but not read. It must never +/// describe the grants themselves: `extra_perms` says which users and groups hold read/write, +/// which the folder ACL model restricts to folder owners, and the caller reaching here is by +/// definition not one. The acting identity is named because a job started on behalf of another +/// user is authorized as that user, so a denial that omits it reads as a grant bug. +pub(crate) fn perm_denied_error( + kind: &str, + path: &str, + authed: Option<&(impl Authable + Sync)>, +) -> Error { + let requester = match authed { + Some(authed) => format!("{} ({})", authed.username(), authed.email()), + None => "an unauthenticated caller".to_string(), + }; + let folder = path + .strip_prefix("f/") + .and_then(|rest| rest.split('/').next()) + .filter(|folder| !folder.is_empty()); + match folder { + Some(folder) => Error::NotAuthorized(format!( + "{kind} {path} exists but you don't have access to it, requested as {requester}. \ + Ask an owner of folder {folder} to grant you access." + )), + None => Error::NotAuthorized(format!( + "{kind} {path} exists but you don't have access to it, requested as {requester}." + )), + } +} diff --git a/backend/windmill-store/src/resources.rs b/backend/windmill-store/src/resources.rs index 5bb15b456e..ac9cd5b547 100644 --- a/backend/windmill-store/src/resources.rs +++ b/backend/windmill-store/src/resources.rs @@ -651,38 +651,18 @@ pub async fn explain_resource_perm_error( db: &sqlx::Pool, authed: &ApiAuthed, ) -> windmill_common::error::Result<()> { - let extra_perms = sqlx::query_scalar!( - "SELECT extra_perms from resource WHERE path = $1 AND workspace_id = $2", + let exists = sqlx::query_scalar!( + "SELECT EXISTS(SELECT 1 FROM resource WHERE path = $1 AND workspace_id = $2)", path, w_id ) - .fetch_optional(db) + .fetch_one(db) .await? - .ok_or_else(|| Error::NotFound(format!("Resource {} not found", path)))?; - if path.starts_with("f/") { - let folder = path.split("/").nth(1).ok_or_else(|| { - Error::BadRequest(format!( - "path {} should have at least 2 components separated by /", - path - )) - })?; - let folder_extra_perms = sqlx::query_scalar!( - "SELECT extra_perms from folder WHERE name = $1 AND workspace_id = $2", - folder, - w_id - ) - .fetch_optional(db) - .await?; - return Err(Error::NotAuthorized(format!( - "Resource exists but you don't have access to it:\nresource perms: {}\nfolder perms: {}\nauthed as: {authed:?}", - serde_json::to_string_pretty(&extra_perms).unwrap_or_default(), serde_json::to_string_pretty(&folder_extra_perms).unwrap_or_default() - ))); - } else { - return Err(Error::NotAuthorized(format!( - "Resource exists but you don't have access to it:\nresource perms: {}\nauthed as: {authed:?}", - serde_json::to_string_pretty(&extra_perms).unwrap_or_default() - ))); + .unwrap_or(false); + if !exists { + return Err(Error::NotFound(format!("Resource {} not found", path))); } + Err(crate::perm_denied_error("Resource", path, Some(authed))) } async fn custom_component( diff --git a/backend/windmill-store/src/variables.rs b/backend/windmill-store/src/variables.rs index 8f288dc85e..9048f1416b 100644 --- a/backend/windmill-store/src/variables.rs +++ b/backend/windmill-store/src/variables.rs @@ -463,61 +463,24 @@ async fn get_value( .map(Json); } -/// The grants alone can't explain a denial: a job started on behalf of another -/// user is authorized as that user, so the error has to name who was actually -/// asking or it reads as a grant bug. -fn describe_authed(authed: Option<&(impl Authable + Sync)>) -> String { - match authed { - Some(authed) => format!( - "username: {}, email: {}, groups: {:?}, folders: {:?}", - authed.username(), - authed.email(), - authed.groups(), - authed.folders() - ), - None => "unauthenticated".to_string(), - } -} - async fn explain_variable_perm_error( path: &str, w_id: &str, db: &sqlx::Pool, authed: Option<&(impl Authable + Sync)>, ) -> windmill_common::error::Result<()> { - let extra_perms = sqlx::query_scalar!( - "SELECT extra_perms from variable WHERE path = $1 AND workspace_id = $2", + let exists = sqlx::query_scalar!( + "SELECT EXISTS(SELECT 1 FROM variable WHERE path = $1 AND workspace_id = $2)", path, w_id ) - .fetch_optional(db) + .fetch_one(db) .await? - .ok_or_else(|| Error::NotFound(format!("Variable {} not found", path)))?; - if path.starts_with("f/") { - let folder = path.split("/").nth(1).ok_or_else(|| { - Error::BadRequest(format!( - "path {} should have at least 2 components separated by /", - path - )) - })?; - let folder_extra_perms = sqlx::query_scalar!( - "SELECT extra_perms from folder WHERE name = $1 AND workspace_id = $2", - folder, - w_id - ) - .fetch_optional(db) - .await?; - return Err(Error::NotAuthorized(format!( - "Variable exists but you don't have access to it:\nvariable perms: {}\nfolder perms: {}\nauthed as: {}", - serde_json::to_string_pretty(&extra_perms).unwrap_or_default(), serde_json::to_string_pretty(&folder_extra_perms).unwrap_or_default(), describe_authed(authed) - ))); - } else { - return Err(Error::NotAuthorized(format!( - "Variable exists but you don't have access to it:\nvariable perms: {}\nauthed as: {}", - serde_json::to_string_pretty(&extra_perms).unwrap_or_default(), - describe_authed(authed) - ))); + .unwrap_or(false); + if !exists { + return Err(Error::NotFound(format!("Variable {} not found", path))); } + Err(crate::perm_denied_error("Variable", path, authed)) } async fn exists_variable(