From c09038b10698042209fd6aa26c466fb53d6dc7d0 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Fri, 16 Oct 2026 17:28:29 +0200 Subject: [PATCH] fix: hold mcp token references to resource reference scopes (#11803) * fix: require variables:read for the token variable of an mcp resource A token scoped to resources:read/write on an MCP resource could have the API decrypt the variable the resource's `token` points at and send it as a bearer to the resource's URL. The url and header `$var:` references already required variables:read on the variable; the token reference now does too, checked before the OAuth refresh touches that variable. Co-Authored-By: Claude Opus 5.5 * fix: let an mcp resource's scope cover the variables it references A scoped token reading or calling an MCP resource resolved its token variable with the resource scope alone, but its url and header variables also needed variables:read on each. The resource scope now covers all three; RLS still bounds them to secrets the caller can read. Reverts the token-side check from the previous commit. Co-Authored-By: Claude Opus 5.5 * fix: scope resource references when they are written, not when they are read A token scoped to a resource now reads everything that resource references (`$var:`, `$jsonvar:`, `$res:`), through get_value_interpolated and the MCP tool routes alike. What makes that safe moves to the write: create, update, update_value and version restore refuse a scoped token adding a reference it holds no read scope on. References the stored value already carries are kept, since whoever put them there was allowed to. Co-Authored-By: Claude Opus 5.5 * fix: hold every resource reference writer to its own user's read rights A resource-scoped reader resolves stored references as itself, so an unscoped session could plant a reference to a secret its own user cannot read and have a reader's scoped token resolve it. Adding a reference now also requires the writer's user to see its target under RLS. A reference to nothing yet stays allowed, since a promotion pushes resources before the secrets they use. Co-Authored-By: Claude Opus 5.5 * fix: vet only variable references on write and keep nested resources scoped A nested `$res:` brought along the variables of a resource the writer could see but whose references it could not read, and what it references can change after it is written. It needs its own read scope again, and the write check covers only `$var:`/`$jsonvar:`. An MCP token stored as a bare path resolved as a variable without being vetted on write. It now needs its own variables:read scope, and the `$var:` prefix is parsed with strip_prefix, as the write check does. Co-Authored-By: Claude Opus 5.5 * fix: keep variable scopes on result-cache resources The worker writes job output to g/results/ straight to the table, so its references never pass the write check. A token scoped to such a resource still needs read scope on the variables it references. Co-Authored-By: Claude Opus 5.5 * fix: require read rights on kept references and pin result-cache resources to g/results/ Co-Authored-By: Claude Opus 5.5 * fix: check resource variable references at read time, with wildcard resource scopes covering matching variables Returns to the read-time rule: a variable a resource references needs variables:read on it, unless it is the resource's linked secret or a wildcard pattern in the token's resources: scopes matches it. The MCP token reference is now held to that rule like the url and header references. Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Claude Opus 5.5 --- backend/tests/mcp_header_var_refs.rs | 42 +++++++++++++- .../tests/resources.rs | 9 +++ backend/windmill-api/src/mcp_tools.rs | 34 ++++++----- backend/windmill-store/src/resources.rs | 56 +++++++++++++++++-- 4 files changed, 119 insertions(+), 22 deletions(-) diff --git a/backend/tests/mcp_header_var_refs.rs b/backend/tests/mcp_header_var_refs.rs index 158bc71a5f..270ae35449 100644 --- a/backend/tests/mcp_header_var_refs.rs +++ b/backend/tests/mcp_header_var_refs.rs @@ -15,7 +15,7 @@ async fn seed(db: &Pool, url: &str) -> anyhow::Result<()> { let mc = windmill_common::variables::build_crypt(db, "test-workspace").await?; sqlx::query( "INSERT INTO variable (workspace_id, path, value, is_secret, description, extra_perms) - VALUES ('test-workspace', 'u/test-user/mcp_header', $1, true, '', '{}')", + VALUES ('test-workspace', 'u/test-user/header_secret', $1, true, '', '{}')", ) .bind(windmill_common::variables::encrypt(&mc, SECRET_VALUE)) .execute(db) @@ -27,7 +27,7 @@ async fn seed(db: &Pool, url: &str) -> anyhow::Result<()> { .bind(serde_json::json!({ "name": "mcp", "url": url, - "headers": { "X-Api-Key": "$var:u/test-user/mcp_header", "X-Plain": "plain" } + "headers": { "X-Api-Key": "$var:u/test-user/header_secret", "X-Plain": "plain" } })) .execute(db) .await?; @@ -102,9 +102,45 @@ async fn test_mcp_header_var_ref_requires_variable_scope(db: Pool) -> let body = resp.text().await?; assert_eq!(status, reqwest::StatusCode::FORBIDDEN, "{body}"); assert!( - body.contains("variables:read:u/test-user/mcp_header"), + body.contains("variables:read:u/test-user/header_secret"), "{body}" ); + // The token is held to the same scope, and a wildcard resource scope covers both. + sqlx::query( + "UPDATE resource SET value = jsonb_build_object('name', 'mcp', 'url', value->>'url', + 'token', '$var:u/test-user/header_secret') WHERE path = 'u/test-user/mcp'", + ) + .execute(&db) + .await?; + for (token, scope, denied) in [ + ("EXACT_TOKEN", "resources:read:u/test-user/mcp", true), + ("WILDCARD_TOKEN", "resources:read:u/test-user/*", false), + ] { + sqlx::query( + "INSERT INTO token (token_hash, token_prefix, token, email, label, super_admin, scopes) + VALUES (encode(sha256($1::bytea), 'hex'), left($1, 10), $1, + 'test@windmill.dev', 'scoped', false, ARRAY[$2])", + ) + .bind(token) + .bind(scope) + .execute(&db) + .await?; + let resp = reqwest::Client::new() + .get(format!( + "http://localhost:{}/api/w/test-workspace/resources/mcp_tools/u/test-user/mcp", + server.addr.port() + )) + .bearer_auth(token) + .send() + .await?; + let body = resp.text().await?; + assert_eq!( + body.contains("variables:read:u/test-user/header_secret"), + denied, + "{scope}: {body}" + ); + } + Ok(()) } diff --git a/backend/windmill-api-integration-tests/tests/resources.rs b/backend/windmill-api-integration-tests/tests/resources.rs index 823c415818..ebca295339 100644 --- a/backend/windmill-api-integration-tests/tests/resources.rs +++ b/backend/windmill-api-integration-tests/tests/resources.rs @@ -648,6 +648,15 @@ async fn test_interpolated_references_need_the_token_scopes( read_probe(json!({"v": "$var:u/test-user/secret"}), with_var.clone()).await; assert_eq!((status, body.as_str()), (200, r#"{"v":"CANARY"}"#)); + // A wildcard resource pattern extends to the variables it matches, and only to those. + let wildcard = mint(json!(["resources:read:u/test-user/*"])).await; + let (status, body) = + read_probe(json!({"v": "$var:u/test-user/secret"}), wildcard.clone()).await; + assert_eq!((status, body.as_str()), (200, r#"{"v":"CANARY"}"#)); + let elsewhere = mint(json!(["resources:read:u/test-user/probe,f/other/*"])).await; + let (status, body) = read_probe(json!({"v": "$var:u/test-user/secret"}), elsewhere).await; + assert_eq!(status, 403, "{body}"); + // The resource's own linked secrets (at its path, or `_`) are part of it. for (path, value) in [ ("u/test-user/probe", "LINKED"), diff --git a/backend/windmill-api/src/mcp_tools.rs b/backend/windmill-api/src/mcp_tools.rs index a9049951a3..ee7cd5e6b7 100644 --- a/backend/windmill-api/src/mcp_tools.rs +++ b/backend/windmill-api/src/mcp_tools.rs @@ -10,7 +10,10 @@ use windmill_common::{ error::{Error, JsonResult, Result}, utils::{not_found_if_none, StripPath}, }; -use windmill_store::{resources::explain_resource_perm_error, variables::get_value_internal}; +use windmill_store::{ + resources::{check_resource_var_ref_scope, explain_resource_perm_error}, + variables::get_value_internal, +}; /// A connected MCP server is a third party the user chose, reached over a /// connection this request holds open: without a deadline one that never answers @@ -67,12 +70,22 @@ async fn connect_mcp_client( serde_json::from_str::(resource_value.0.get()) .map_err(|e| Error::BadRequest(format!("Failed to parse MCP resource: {}", e)))?; + // The token, URL and header secrets are read on the caller's behalf and sent to a URL + // the resource chose, so each needs the same scope as a resource reference. The token + // is checked before the OAuth refresh touches its variable. + let token_var_path = mcp_resource + .token + .as_deref() + .map(|t| t.trim_start_matches("$var:")) + .filter(|p| !p.trim().is_empty()); + if let Some(token_var_path) = token_var_path { + check_resource_var_ref_scope(authed, path, token_var_path)?; + } + #[cfg(feature = "oauth2")] { tracing::info!("Checking if token needs refresh before creating MCP client"); - if let Some(ref token_path) = mcp_resource.token { - let token_var_path = token_path.trim_start_matches("$var:"); - + if let Some(token_var_path) = token_var_path { let token_info = sqlx::query!( r#" SELECT @@ -113,14 +126,9 @@ async fn connect_mcp_client( // Resolve the token through the caller's permissioned (RLS + audit) path so // a developer cannot exfiltrate a secret they are not allowed to read by // pointing an MCP resource's token at it. - let token = if let Some(token_path) = &mcp_resource.token { - let token_var_path = token_path.trim_start_matches("$var:"); - if token_var_path.trim().is_empty() { - None - } else { - let db_authed = DbWithOptAuthed::from_authed(authed, db.clone(), Some(user_db.clone())); - Some(get_value_internal(&db_authed, w_id, token_var_path, false).await?) - } + let token = if let Some(token_var_path) = token_var_path { + let db_authed = DbWithOptAuthed::from_authed(authed, db.clone(), Some(user_db.clone())); + Some(get_value_internal(&db_authed, w_id, token_var_path, false).await?) } else { None }; @@ -130,7 +138,7 @@ async fn connect_mcp_client( .chain(mcp_resource.headers.iter_mut().flatten().map(|(_, v)| v)) { if let Some(var_path) = value.strip_prefix("$var:") { - check_scopes(authed, || format!("variables:read:{}", var_path))?; + check_resource_var_ref_scope(authed, path, var_path)?; let db_authed = DbWithOptAuthed::from_authed(authed, db.clone(), Some(user_db.clone())); *value = get_value_internal(&db_authed, w_id, var_path, false).await?; } diff --git a/backend/windmill-store/src/resources.rs b/backend/windmill-store/src/resources.rs index 059819a802..010eb0a90b 100644 --- a/backend/windmill-store/src/resources.rs +++ b/backend/windmill-store/src/resources.rs @@ -17,6 +17,7 @@ use windmill_api_auth::{ }; use windmill_common::db::DB; use windmill_common::per_minute_counter::PerMinuteCounter; +use windmill_common::scopes::ScopeDefinition; use windmill_common::ssrf::{ is_private_ip, private_git_host_allowed, private_git_host_hint, GitRemoteCaller, }; @@ -907,18 +908,61 @@ pub async fn transform_json_value( /// RLS resolves a reference as the token's user, not as the token: a token scoped to one /// resource would otherwise read, through that resource, every variable or resource its /// user can. Job tokens carry no scopes, so what a runnable resolves is unaffected. -/// `resource_path` is the resource being expanded: its own linked secrets -/// ([`is_owned_linked_var`]) are covered by the read of it. +/// `resource_path` is the resource being expanded: see [`check_resource_var_ref_scope`]. fn check_interpolation_scope( db_with_opt_authed: &DbWithOptAuthed<'_, ApiAuthed>, domain: &str, path: &str, resource_path: Option<&str>, ) -> Result<()> { - match db_with_opt_authed.authed() { - Some(_) if resource_path.is_some_and(|r| is_owned_linked_var(r, path)) => Ok(()), - Some(authed) => check_scopes(authed, || format!("{domain}:read:{path}")), - None => Ok(()), + match (db_with_opt_authed.authed(), resource_path) { + (Some(authed), Some(r)) if domain == "variables" => { + check_resource_var_ref_scope(authed, r, path) + } + (Some(authed), _) => check_scopes(authed, || format!("{domain}:read:{path}")), + (None, _) => Ok(()), + } +} + +/// The scope a token needs to resolve variable `var_path` referenced by resource +/// `resource_path`: `variables:read` on it, unless it is the resource's own linked secret +/// ([`is_owned_linked_var`]) or a wildcard pattern of the token's `resources:` scopes +/// matches it. An exact path, or a scope naming no path at all, grants no variable: RLS +/// lets the token resolve any variable its user can read. +pub fn check_resource_var_ref_scope( + authed: &ApiAuthed, + resource_path: &str, + var_path: &str, +) -> Result<()> { + if is_owned_linked_var(resource_path, var_path) { + return Ok(()); + } + let denied = match check_scopes(authed, || format!("variables:read:{var_path}")) { + Ok(()) => return Ok(()), + Err(e) => e, + }; + let required = ScopeDefinition::new("variables", "read", None, Some(vec![var_path.into()])); + let covered = authed.scopes.iter().flatten().any(|scope| { + let Ok(scope) = ScopeDefinition::from_scope_string(scope) else { + return false; + }; + if scope.domain != "resources" { + return false; + } + let patterns = scope + .resource + .into_iter() + .flatten() + .filter(|r| r.contains('*')) + .collect::>(); + !patterns.is_empty() + && ScopeDefinition::new("variables", &scope.action, None, Some(patterns)) + .includes(&required) + }); + if covered { + Ok(()) + } else { + Err(denied) } }