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) } }