diff --git a/backend/.sqlx/query-19f1cb1c7a1974920549917a6392ff0d56f31ca5662062f4a71fed0ccf859cc4.json b/backend/.sqlx/query-19f1cb1c7a1974920549917a6392ff0d56f31ca5662062f4a71fed0ccf859cc4.json new file mode 100644 index 0000000000..e576cf8fae --- /dev/null +++ b/backend/.sqlx/query-19f1cb1c7a1974920549917a6392ff0d56f31ca5662062f4a71fed0ccf859cc4.json @@ -0,0 +1,35 @@ +{ + "db_name": "PostgreSQL", + "query": "SELECT kind::text as \"kind!\", parent_job, runnable_path\n FROM v2_job WHERE id = $1 AND workspace_id = $2", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "kind!", + "type_info": "Text" + }, + { + "ordinal": 1, + "name": "parent_job", + "type_info": "Uuid" + }, + { + "ordinal": 2, + "name": "runnable_path", + "type_info": "Varchar" + } + ], + "parameters": { + "Left": [ + "Uuid", + "Text" + ] + }, + "nullable": [ + null, + true, + true + ] + }, + "hash": "19f1cb1c7a1974920549917a6392ff0d56f31ca5662062f4a71fed0ccf859cc4" +} diff --git a/backend/.sqlx/query-c167db39eeed526449dc064eaeb26e648aa9455cb81bab86fd106f76537701ba.json b/backend/.sqlx/query-c167db39eeed526449dc064eaeb26e648aa9455cb81bab86fd106f76537701ba.json new file mode 100644 index 0000000000..2d1e21019e --- /dev/null +++ b/backend/.sqlx/query-c167db39eeed526449dc064eaeb26e648aa9455cb81bab86fd106f76537701ba.json @@ -0,0 +1,23 @@ +{ + "db_name": "PostgreSQL", + "query": "SELECT runnable_path FROM v2_job WHERE id = $1 AND workspace_id = $2", + "describe": { + "columns": [ + { + "ordinal": 0, + "name": "runnable_path", + "type_info": "Varchar" + } + ], + "parameters": { + "Left": [ + "Uuid", + "Text" + ] + }, + "nullable": [ + true + ] + }, + "hash": "c167db39eeed526449dc064eaeb26e648aa9455cb81bab86fd106f76537701ba" +} diff --git a/backend/windmill-api-auth/src/auth.rs b/backend/windmill-api-auth/src/auth.rs index 314ba99450..a6a0fc4a8e 100644 --- a/backend/windmill-api-auth/src/auth.rs +++ b/backend/windmill-api-auth/src/auth.rs @@ -191,7 +191,11 @@ impl AuthCache { is_operator: claims.is_operator, groups: claims.groups, folders: claims.folders, - scopes: None, + // Honor the scopes embedded in the JWT (mirrors the EE + // jwt_ext_ branch). The route middleware only enforces + // scopes when Some, so a None-scoped JWT (e.g. the job + // WM_TOKEN) keeps full user privileges as before. + scopes: claims.scopes, username_override, token_prefix: claims.audit_span, read_only: false, diff --git a/backend/windmill-api-auth/src/scopes.rs b/backend/windmill-api-auth/src/scopes.rs index 87ca3a8862..bf69f3e9d5 100644 --- a/backend/windmill-api-auth/src/scopes.rs +++ b/backend/windmill-api-auth/src/scopes.rs @@ -699,6 +699,23 @@ pub fn check_read_only_for_route(route_path: &str, http_method: &str) -> Result< } } +/// The minimal scope string that grants access to exactly `{method} {path}`, as +/// `check_route_access` would require it. Used to mint a least-privilege JWT for +/// a single proxied request (the MCP endpoint proxy), so the minted token can do +/// only that one operation rather than acting as a blank check. +/// +/// `path` is the request path (e.g. `/api/w/{workspace}/variables/get/...`). +/// Returns `None` if the route's domain can't be determined — the caller should +/// then fail closed. +pub fn scope_for_route(method: &str, path: &str) -> Option { + let action = map_http_method_to_action(method, path); + let (domain, kind, _suffix) = extract_domain_from_route(path).ok()?; + Some(match (domain, action, kind) { + (ScopeDomain::Jobs, ScopeAction::Run, Some(kind)) => format!("jobs:run:{}", kind), + (domain, action, _) => format!("{}:{}", domain.as_str(), action.as_str()), + }) +} + /// Helper function to check if scopes allow access to a route pub fn check_scopes_for_route( token_scopes: Option<&[String]>, @@ -1083,4 +1100,38 @@ mod tests { let scopes = vec!["jobs:read".to_string(), "mcp:all".to_string()]; assert!(check_route_access(&scopes, "/api/w/test_workspace/mcp/something", "GET").is_ok()); } + + #[test] + fn test_scope_for_route() { + // The minted scope must be exactly what check_route_access requires for + // the same route, so a JWT carrying it passes for that one route only. + assert_eq!( + scope_for_route("GET", "/api/w/ws/variables/get/u/x/y").as_deref(), + Some("variables:read") + ); + assert_eq!( + scope_for_route("POST", "/api/w/ws/variables/create").as_deref(), + Some("variables:write") + ); + assert_eq!( + scope_for_route("DELETE", "/api/w/ws/resources/delete/u/x/y").as_deref(), + Some("resources:write") + ); + // jobs run paths carry the runnable kind. + assert_eq!( + scope_for_route("POST", "/api/w/ws/jobs/run/p/u/x/y").as_deref(), + Some("jobs:run:scripts") + ); + assert_eq!( + scope_for_route("POST", "/api/w/ws/jobs/run/f/u/x/y").as_deref(), + Some("jobs:run:flows") + ); + + // The minted scope actually satisfies the route check it targets. + let s = scope_for_route("POST", "/api/w/ws/variables/create").unwrap(); + assert!(check_route_access(&[s], "/api/w/ws/variables/create", "POST").is_ok()); + + // Unknown route -> None so the caller fails closed. + assert!(scope_for_route("GET", "/healthz").is_none()); + } } diff --git a/backend/windmill-api/src/apps.rs b/backend/windmill-api/src/apps.rs index 0615735081..bfc76eefd8 100644 --- a/backend/windmill-api/src/apps.rs +++ b/backend/windmill-api/src/apps.rs @@ -1245,8 +1245,6 @@ async fn create_app_raw<'a>( ) .await?; - check_scopes(&authed, || format!("apps:write:{}", path))?; - webhook.send_message( w_id.clone(), WebhookMessage::CreateApp { workspace: w_id, path: path.clone() }, @@ -1290,7 +1288,6 @@ async fn create_app( )); } let path = app.path.clone(); - check_scopes(&authed, || format!("apps:write:{}", &path))?; if let RuleCheckResult::Blocked(msg) = check_deploy_rules( &w_id, @@ -1304,6 +1301,7 @@ async fn create_app( return Err(Error::PermissionDenied(msg)); } + // scope is enforced inside create_app_internal, before any persistence. let (new_tx, _path, _id) = create_app_internal(authed, db, user_db, &w_id, false, app).await?; new_tx.commit().await?; @@ -1350,6 +1348,10 @@ async fn create_app_internal<'a>( raw_app: bool, mut app: CreateApp, ) -> Result<(sqlx::Transaction<'a, sqlx::Postgres>, String, i64)> { + // Enforce scope before any persistence: the raw-app create path commits + // inside process_app_multipart!, so checking after this call would leave a + // denied app committed in the DB. + check_scopes(&authed, || format!("apps:write:{}", &app.path))?; if *CLOUD_HOSTED { let nb_apps = sqlx::query_scalar!("SELECT COUNT(*) FROM app WHERE workspace_id = $1", &w_id) @@ -1891,6 +1893,13 @@ async fn update_app_internal<'a>( ns: EditApp, ) -> Result<(sqlx::Transaction<'a, sqlx::Postgres>, String, i64)> { use sql_builder::prelude::*; + + // A rename moves the app to ns.path, so the destination must also be within + // the token's write scope, not just the source path. + if let Some(npath) = ns.path.as_deref() { + check_scopes(&authed, || format!("apps:write:{}", npath))?; + } + let mut tx = user_db.clone().begin(&authed).await?; let mut preserved_on_behalf_of: Option = None; diff --git a/backend/windmill-api/src/jobs.rs b/backend/windmill-api/src/jobs.rs index c6d978e0d6..30dc64d8e2 100644 --- a/backend/windmill-api/src/jobs.rs +++ b/backend/windmill-api/src/jobs.rs @@ -4022,11 +4022,17 @@ fn conditionally_require_authed_user( } pub async fn create_job_signature( - _authed: ApiAuthed, + authed: ApiAuthed, Extension(db): Extension, Path((w_id, job_id, resume_id)): Path<(String, Uuid, u32)>, Query(approver): Query, ) -> error::Result { + // The HMAC is treated as full authority by the resume endpoints, so minting + // it requires run scope on the suspended job's flow — not merely any + // jobs:run scope. No-op for unscoped tokens (incl. the in-flow substep token + // used by wmill.get_resume_urls()). + let flow_path = resume_target_flow_path(&db, &w_id, job_id).await?; + check_scopes(&authed, || format!("jobs:run:flows:{}", flow_path))?; let key = get_workspace_key(&w_id, &db).await?; create_signature(key, job_id, resume_id, approver.approver) } @@ -4109,11 +4115,17 @@ fn build_resume_url( } pub async fn get_resume_urls( - _authed: ApiAuthed, + authed: ApiAuthed, Extension(db): Extension, Path((w_id, job_id, resume_id)): Path<(String, Uuid, u32)>, Query(approver): Query, ) -> error::JsonResult { + // These URLs embed a resume signature (full resume capability), so a scoped + // token must hold run scope on the suspended job's flow. No-op for unscoped + // tokens (incl. the in-flow substep token). Trusted internal callers use + // get_resume_urls_internal directly and are unaffected. + let flow_path = resume_target_flow_path(&db, &w_id, job_id).await?; + check_scopes(&authed, || format!("jobs:run:flows:{}", flow_path))?; get_resume_urls_internal( Extension(db), Path((w_id, job_id, resume_id)), @@ -4180,6 +4192,46 @@ pub async fn get_resume_urls_internal( Ok(Json(res)) } +/// Resolve the runnable path of the flow a (possibly step) job belongs to, used +/// to scope-check resume-signature minting against `jobs:run:flows:`. +/// Returns an empty string when the path can't be resolved (e.g. previews or an +/// unknown job); an empty path only matters for path-restricted tokens, which +/// would not be running such a flow. Never hard-fails, so it can't break resume +/// for unscoped tokens (the in-flow `get_resume_urls()` path). +async fn resume_target_flow_path(db: &DB, w_id: &str, job_id: Uuid) -> error::Result { + let job = sqlx::query!( + r#"SELECT kind::text as "kind!", parent_job, runnable_path + FROM v2_job WHERE id = $1 AND workspace_id = $2"#, + job_id, + w_id + ) + .fetch_optional(db) + .await?; + let Some(job) = job else { + return Ok(String::new()); + }; + // All flow kinds: the job itself is the flow whose path scopes the resume. + if matches!( + job.kind.as_str(), + "flow" | "flowpreview" | "flownode" | "singlestepflow" + ) { + return Ok(job.runnable_path.unwrap_or_default()); + } + // Otherwise it's a step; its parent is the flow. + if let Some(parent) = job.parent_job { + return Ok(sqlx::query_scalar!( + "SELECT runnable_path FROM v2_job WHERE id = $1 AND workspace_id = $2", + parent, + w_id + ) + .fetch_optional(db) + .await? + .flatten() + .unwrap_or_default()); + } + Ok(job.runnable_path.unwrap_or_default()) +} + /// Get the flow ID for a job. If the job is a flow, returns the job_id. /// If the job is a step in a flow, returns the parent flow ID. async fn get_flow_id_for_job(db: &DB, job_id: Uuid) -> error::Result { @@ -9488,16 +9540,31 @@ mod approval_view_gate_tests { fn anonymous_cannot_view_when_auth_required() { // The regression: an unauthenticated holder of the approval token must see nothing. let c = Some(conds(true, vec![])); - assert!(!can_view(&None, &c, Some("f/team/flow"), "trigger@example.com")); + assert!(!can_view( + &None, + &c, + Some("f/team/flow"), + "trigger@example.com" + )); } #[test] fn anonymous_can_view_when_no_auth_required() { // Unchanged behaviour: token alone is sufficient when auth isn't required. let c = Some(conds(false, vec![])); - assert!(can_view(&None, &c, Some("f/team/flow"), "trigger@example.com")); + assert!(can_view( + &None, + &c, + Some("f/team/flow"), + "trigger@example.com" + )); // No approval conditions at all also allows token-only view. - assert!(can_view(&None, &None, Some("f/team/flow"), "trigger@example.com")); + assert!(can_view( + &None, + &None, + Some("f/team/flow"), + "trigger@example.com" + )); } #[test] @@ -9522,7 +9589,17 @@ mod approval_view_gate_tests { let member = Some(authed("carol", false, vec!["approvers".to_string()])); let outsider = Some(authed("dave", false, vec!["other".to_string()])); // Use a non-owned folder path so ownership doesn't short-circuit the check. - assert!(can_view(&member, &c, Some("f/team/flow"), "trigger@example.com")); - assert!(!can_view(&outsider, &c, Some("f/team/flow"), "trigger@example.com")); + assert!(can_view( + &member, + &c, + Some("f/team/flow"), + "trigger@example.com" + )); + assert!(!can_view( + &outsider, + &c, + Some("f/team/flow"), + "trigger@example.com" + )); } } diff --git a/backend/windmill-api/src/mcp/oauth_server.rs b/backend/windmill-api/src/mcp/oauth_server.rs index 7cfabd2319..c5142c3031 100644 --- a/backend/windmill-api/src/mcp/oauth_server.rs +++ b/backend/windmill-api/src/mcp/oauth_server.rs @@ -18,6 +18,7 @@ use windmill_common::{ }; use crate::db::ApiAuthed; +use windmill_mcp::parse_mcp_scopes; /// Token expiration for MCP OAuth tokens (1 week in seconds) const MCP_OAUTH_TOKEN_EXPIRATION_SECS: u64 = 7 * 24 * 60 * 60; @@ -585,6 +586,8 @@ async fn handle_refresh_token_grant( Some(&new_access_token) }; let new_refresh_token = rd_string(32); + // Re-issues the already-approved (hence already-contained) scopes verbatim; + // containment is enforced once at approval time, so no re-check here. let scopes = token_row.scopes; // Create new access token (rejects archived workspaces inline) @@ -820,6 +823,40 @@ async fn oauth_approve_inner( .map(|s| s.to_string()) .collect(); + // The approver's own token bounds what it may grant: a scope-restricted MCP + // token must not approve a broader one (e.g. mcp:scripts:f/x -> mcp:all). An + // unrestricted approver (interactive session, scopes None) grants freely, + // which is the normal consent flow. This is the legitimate MCP-narrowing + // path, so it uses MCP-pattern containment rather than the byte-identical + // rule ensure_scopes_within_caller applies on the generic token endpoints. + let caller_restricted = authed + .scopes + .as_deref() + .is_some_and(|s| s.iter().any(|x| !x.starts_with("if_jobs:filter_tags:"))); + if caller_restricted { + // An empty grant would mint a token the auth layer treats as unscoped + // (full privileges), so a restricted approver must not produce one. + if scopes.is_empty() { + return Err(Error::NotAuthorized( + "A scope-restricted token cannot approve an empty scope grant".to_string(), + )); + } + if scopes.iter().any(|s| !s.starts_with("mcp:")) { + return Err(Error::NotAuthorized( + "A scope-restricted token can only approve MCP (mcp:*) scopes".to_string(), + )); + } + let caller_config = parse_mcp_scopes(authed.scopes.as_deref().unwrap_or(&[])) + .map_err(|e| Error::InternalErr(format!("Failed to parse caller MCP scopes: {e}")))?; + let requested_config = parse_mcp_scopes(&scopes) + .map_err(|e| Error::BadRequest(format!("Failed to parse requested MCP scopes: {e}")))?; + if !caller_config.contains(&requested_config) { + return Err(Error::NotAuthorized( + "Requested scopes exceed the approving token's own MCP scopes".to_string(), + )); + } + } + sqlx::query!( "INSERT INTO mcp_oauth_server_code (code, client_id, user_email, workspace_id, scopes, redirect_uri, code_challenge, code_challenge_method) diff --git a/backend/windmill-api/src/mcp/utils.rs b/backend/windmill-api/src/mcp/utils.rs index 4073758ba9..2a0c34ae8d 100644 --- a/backend/windmill-api/src/mcp/utils.rs +++ b/backend/windmill-api/src/mcp/utils.rs @@ -455,9 +455,35 @@ pub async fn create_http_request( } }; + // Bound the minted JWT to exactly this proxied route so a scope-restricted + // MCP token can't be widened into a full-privilege blank check. The + // endpoint-name gate (in the MCP runner) already authorized *which* endpoint + // may be called; this constrains what the resulting request can do. Unscoped + // callers (cookie / full-privilege tokens) keep an unscoped JWT to preserve + // existing behavior. A scope-restricted caller whose route can't be resolved + // fails closed. + let caller_restricted = api_authed + .scopes + .as_deref() + .is_some_and(|s| s.iter().any(|x| !x.starts_with("if_jobs:filter_tags:"))); + let scopes = if caller_restricted { + let parsed = reqwest::Url::parse(url) + .map_err(|e| ErrorData::internal_error(format!("Invalid proxied URL: {}", e), None))?; + let scope = + windmill_api_auth::scopes::scope_for_route(method, parsed.path()).ok_or_else(|| { + ErrorData::internal_error( + "Could not derive route scope for proxied MCP endpoint".to_string(), + None, + ) + })?; + Some(vec![scope]) + } else { + None + }; + // Add authorization header let authed = Authed::from(api_authed.clone()); - let token = create_jwt_token(authed, workspace_id, 3600, None, None, None, None) + let token = create_jwt_token(authed, workspace_id, 3600, None, None, None, scopes) .await .map_err(|e| ErrorData::internal_error(e.to_string(), None))?; request_builder = request_builder.header("Authorization", format!("Bearer {}", token)); diff --git a/backend/windmill-api/src/workspaces_export.rs b/backend/windmill-api/src/workspaces_export.rs index dae1be0614..78d052c222 100644 --- a/backend/windmill-api/src/workspaces_export.rs +++ b/backend/windmill-api/src/workspaces_export.rs @@ -12,6 +12,8 @@ use crate::db::ApiAuthed; use crate::{apps::AppWithLastVersion, db::DB, folders::Folder}; +use windmill_api_auth::check_scopes; + #[cfg(any( feature = "http_trigger", feature = "websocket", @@ -582,6 +584,18 @@ 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 stays admin-only (include_key). No-op for unscoped tokens. + if plain_secret.or(plain_secrets).unwrap_or(false) + && !skip_secrets.unwrap_or(false) + && !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 @@ -594,6 +608,24 @@ pub(crate) async fn tarball_workspace( let mut tx = user_db.begin(&authed).await?; + // Exporting decrypted secrets in bulk is the same capability as a per-item + // secret read, so record it for parity with variables.decrypt_secret. + if plain_secret.or(plain_secrets).unwrap_or(false) + && !skip_variables.unwrap_or(false) + && !skip_secrets.unwrap_or(false) + { + windmill_audit::audit_oss::audit_log( + &mut *tx, + &authed, + "variables.decrypt_secret", + windmill_audit::ActionKind::Execute, + &w_id, + Some("workspace_tarball_export"), + None, + ) + .await?; + } + // Source-of-truth for fork-ness: the workspace's parent_workspace_id column. // The wm-fork-* prefix is a creation-time naming convention that could in // principle drift (rename, manual SQL); the column is the contract that diff --git a/backend/windmill-mcp/src/common/scope.rs b/backend/windmill-mcp/src/common/scope.rs index 7da4e2cebc..f43095d740 100644 --- a/backend/windmill-mcp/src/common/scope.rs +++ b/backend/windmill-mcp/src/common/scope.rs @@ -38,6 +38,71 @@ impl McpScopeConfig { is_resource_allowed(path, patterns) } + + /// Directional subset check: does this config grant at least everything + /// `requested` grants? Used to enforce monotonic containment when an MCP + /// OAuth approval mints a token (the granted scopes must be within the + /// approving token's own scopes). + /// + /// Unlike `is_allowed` (which tests a single concrete path with OR + /// semantics), this requires every requested pattern to be covered by some + /// caller pattern — so `mcp:scripts:f/x` cannot widen into `mcp:scripts:*`. + pub fn contains(&self, requested: &McpScopeConfig) -> bool { + if self.all { + return true; + } + if requested.all { + return false; + } + if requested.favorites && !self.favorites { + return false; + } + if let Some(req_hub) = requested.hub_apps.as_ref() { + match self.hub_apps.as_ref() { + Some(caller_hub) => { + let caller_apps: std::collections::HashSet<&str> = + caller_hub.split(',').map(|s| s.trim()).collect(); + if !req_hub + .split(',') + .map(|s| s.trim()) + .all(|a| caller_apps.contains(a)) + { + return false; + } + } + None => return false, + } + } + resource_list_covers(&self.scripts, &requested.scripts) + && resource_list_covers(&self.flows, &requested.flows) + && resource_list_covers(&self.endpoints, &requested.endpoints) + } +} + +/// Every requested pattern must be covered by some caller pattern. +fn resource_list_covers(caller: &[String], requested: &[String]) -> bool { + requested + .iter() + .all(|req| caller.iter().any(|c| pattern_covers(c, req))) +} + +/// Directional: does the single caller pattern cover `requested`? `caller` may +/// be `*`, an exact path/name, or a `/*` subtree; `requested` may itself +/// be a subtree wildcard, in which case the whole requested subtree must fall +/// within the caller's. Mirrors the route-scope containment in windmill-api-auth. +fn pattern_covers(caller: &str, requested: &str) -> bool { + if caller == "*" || caller == requested { + return true; + } + // An exact caller pattern only covers itself (handled above); a wildcard + // requested can never be covered by a non-`*` exact caller. + let Some(prefix) = caller.strip_suffix("/*") else { + return false; + }; + let requested_base = requested.strip_suffix("/*").unwrap_or(requested); + requested_base == prefix + || (requested_base.starts_with(prefix) + && requested_base.as_bytes().get(prefix.len()) == Some(&b'/')) } /// Parse MCP scopes from token scope strings @@ -254,4 +319,51 @@ mod tests { assert!(config.is_allowed("flow", "f/automation/test")); assert!(!config.is_allowed("flow", "f/other/test")); } + + fn cfg(scopes: &[&str]) -> McpScopeConfig { + parse_mcp_scopes(&scopes.iter().map(|s| s.to_string()).collect::>()).unwrap() + } + + #[test] + fn test_contains_subset_and_widening() { + // mcp:all contains anything. + assert!(cfg(&["mcp:all"]).contains(&cfg(&["mcp:scripts:f/x"]))); + assert!(cfg(&["mcp:all"]).contains(&cfg(&["mcp:all"]))); + + // A wildcard caller covers narrower requests, but not other domains/all. + let star = cfg(&["mcp:scripts:*"]); + assert!(star.contains(&cfg(&["mcp:scripts:f/x"]))); + assert!(star.contains(&cfg(&["mcp:scripts:*"]))); + assert!(!star.contains(&cfg(&["mcp:all"]))); + assert!(!star.contains(&cfg(&["mcp:flows:f/x"]))); + + // The core regression: a single-path caller must NOT widen into `*` or + // into another path. + let narrow = cfg(&["mcp:scripts:f/x"]); + assert!(narrow.contains(&cfg(&["mcp:scripts:f/x"]))); + assert!(!narrow.contains(&cfg(&["mcp:scripts:*"]))); + assert!(!narrow.contains(&cfg(&["mcp:scripts:f/y"]))); + assert!(!narrow.contains(&cfg(&["mcp:all"]))); + + // Subtree wildcard covers paths within it but not a sibling subtree. + let subtree = cfg(&["mcp:scripts:f/team/*"]); + assert!(subtree.contains(&cfg(&["mcp:scripts:f/team/sub"]))); + assert!(subtree.contains(&cfg(&["mcp:scripts:f/team/sub/*"]))); + assert!(!subtree.contains(&cfg(&["mcp:scripts:f/other/x"]))); + } + + #[test] + fn test_contains_favorites_and_endpoints() { + assert!(cfg(&["mcp:favorites"]).contains(&cfg(&["mcp:favorites"]))); + // A caller without favorites cannot grant favorites. + assert!(!cfg(&["mcp:scripts:*"]).contains(&cfg(&["mcp:favorites"]))); + + // Endpoint names match exactly (or via `*`). + let ep = cfg(&["mcp:endpoints:getVariable"]); + assert!(ep.contains(&cfg(&["mcp:endpoints:getVariable"]))); + assert!(!ep.contains(&cfg(&["mcp:endpoints:getResource"]))); + assert!(!ep.contains(&cfg(&["mcp:all"]))); + // mcp:all grants all endpoints. + assert!(cfg(&["mcp:all"]).contains(&cfg(&["mcp:endpoints:getResource"]))); + } } diff --git a/backend/windmill-store/src/resources.rs b/backend/windmill-store/src/resources.rs index 44db0a1b68..7da6d75f4b 100644 --- a/backend/windmill-store/src/resources.rs +++ b/backend/windmill-store/src/resources.rs @@ -1634,6 +1634,12 @@ async fn update_resource( let path = path.to_path(); check_scopes(&authed, || format!("resources:write:{}", path))?; + // A rename moves the resource (and its linked variable) to ns.path, so the + // destination must also be within the token's write scope, not just the + // source path. + if let Some(npath) = ns.path.as_deref() { + check_scopes(&authed, || format!("resources:write:{}", npath))?; + } if let RuleCheckResult::Blocked(msg) = check_deploy_rules( &w_id, AuditAuthorable::username(&authed), diff --git a/backend/windmill-store/src/variables.rs b/backend/windmill-store/src/variables.rs index 72ed20cb71..d5a906392d 100644 --- a/backend/windmill-store/src/variables.rs +++ b/backend/windmill-store/src/variables.rs @@ -1037,6 +1037,12 @@ async fn update_variable( let path = path.to_path(); check_scopes(&authed, || format!("variables:write:{}", path))?; + // A rename moves the (possibly secret) variable to ns.path, so the + // destination must also be within the token's write scope, not just the + // source path. + if let Some(npath) = ns.path.as_deref() { + check_scopes(&authed, || format!("variables:write:{}", npath))?; + } let authed = maybe_refresh_folders(&path, &w_id, authed, &db).await; let mut sqlb = SqlBuilder::update_table("variable"); diff --git a/backend/windmill-trigger-http/src/handler.rs b/backend/windmill-trigger-http/src/handler.rs index 7b617e1b8d..17321b7775 100644 --- a/backend/windmill-trigger-http/src/handler.rs +++ b/backend/windmill-trigger-http/src/handler.rs @@ -7,7 +7,7 @@ use axum::{extract::Path, routing::post, Extension, Json, Router}; use http::StatusCode; use sqlx::PgConnection; use std::collections::HashSet; -use windmill_api_auth::ApiAuthed; +use windmill_api_auth::{check_scopes, ApiAuthed}; use windmill_audit::{audit_oss::audit_log, ActionKind}; use windmill_common::global_settings::HTTP_ROUTE_WORKSPACED_ROUTE; use windmill_common::{ @@ -262,6 +262,12 @@ pub async fn create_many_http_triggers( let mut route_path_keys = Vec::with_capacity(new_http_triggers.len()); for new_http_trigger in new_http_triggers.iter() { + // Per-item write scope, matching the single-create handler. The bulk + // endpoint must not let a path-scoped token create triggers outside it. + check_scopes(&authed, || { + format!("http_triggers:write:{}", &new_http_trigger.base.path) + })?; + handler .validate_new(&db, &w_id, &new_http_trigger.config) .await @@ -373,7 +379,8 @@ impl TriggerCrud for HttpTrigger { const TABLE_NAME: &'static str = "http_trigger"; const TRIGGER_TYPE: &'static str = "http"; - const DRAFT_KIND: windmill_common::user_drafts::UserDraftItemKind = windmill_common::user_drafts::UserDraftItemKind::TriggerHttp; + const DRAFT_KIND: windmill_common::user_drafts::UserDraftItemKind = + windmill_common::user_drafts::UserDraftItemKind::TriggerHttp; const SUPPORTS_SERVER_STATE: bool = false; const SUPPORTS_TEST_CONNECTION: bool = false; const ROUTE_PREFIX: &'static str = "/http_triggers"; diff --git a/backend/windmill-trigger-websocket/src/handler.rs b/backend/windmill-trigger-websocket/src/handler.rs index ed8e95b164..529e5902cc 100644 --- a/backend/windmill-trigger-websocket/src/handler.rs +++ b/backend/windmill-trigger-websocket/src/handler.rs @@ -4,7 +4,7 @@ use async_trait::async_trait; use itertools::Itertools; use serde_json::value::RawValue; use sqlx::{types::Json as SqlxJson, PgConnection}; -use windmill_api_auth::ApiAuthed; +use windmill_api_auth::{check_scopes, ApiAuthed}; use windmill_common::DB; use windmill_common::{ db::UserDB, @@ -15,10 +15,39 @@ use windmill_git_sync::DeployedObject; use windmill_trigger::{Trigger, TriggerCrud, TriggerData}; use super::{ - get_url_from_runnable_value, proxy::connect_async_with_proxy, validate_websocket_url_for_ssrf, - TestWebsocketConfig, WebsocketConfig, WebsocketConfigRequest, WebsocketTrigger, + get_url_from_runnable_value, listener::InitialMessage, proxy::connect_async_with_proxy, + validate_websocket_url_for_ssrf, TestWebsocketConfig, WebsocketConfig, WebsocketConfigRequest, + WebsocketTrigger, }; +/// A websocket_triggers:write token can configure secondary runnables that the +/// listener later executes under the trigger owner's identity: a `$flow:`/ +/// `$script:` URL resolver and `initial_messages` of kind `runnable_result`. +/// That execution happens in a background task where the reconstructed authed is +/// scopeless (so its check_scopes is a no-op), so enforce run scope here, at +/// create/update time, against the API caller's token. +fn check_secondary_runnable_scopes( + authed: &ApiAuthed, + config: &WebsocketConfigRequest, +) -> Result<()> { + if let Some(rest) = config.url.strip_prefix("$flow:") { + check_scopes(authed, || format!("jobs:run:flows:{}", rest))?; + } else if let Some(rest) = config.url.strip_prefix("$script:") { + check_scopes(authed, || format!("jobs:run:scripts:{}", rest))?; + } + if let Some(messages) = config.initial_messages.as_ref() { + for msg in messages { + if let Ok(InitialMessage::RunnableResult { path, is_flow, .. }) = + serde_json::from_value::(msg.clone()) + { + let kind = if is_flow { "flows" } else { "scripts" }; + check_scopes(authed, || format!("jobs:run:{}:{}", kind, path))?; + } + } + } + Ok(()) +} + #[async_trait] impl TriggerCrud for WebsocketTrigger { type TriggerConfig = WebsocketConfig; @@ -101,6 +130,7 @@ impl TriggerCrud for WebsocketTrigger { w_id: &str, trigger: TriggerData, ) -> Result<()> { + check_secondary_runnable_scopes(authed, &trigger.config)?; let resolved_edited_by = trigger.base.resolve_edited_by(authed); let resolved_permissioned_as = trigger.base.resolve_permissioned_as(authed); let filters = trigger @@ -178,6 +208,7 @@ impl TriggerCrud for WebsocketTrigger { path: &str, trigger: TriggerData, ) -> Result<()> { + check_secondary_runnable_scopes(authed, &trigger.config)?; let resolved_edited_by = trigger.base.resolve_edited_by(authed); let resolved_permissioned_as = trigger.base.resolve_permissioned_as(authed); let filters = trigger diff --git a/backend/windmill-trigger-websocket/src/listener.rs b/backend/windmill-trigger-websocket/src/listener.rs index 31e3f83995..d46faf1b1a 100644 --- a/backend/windmill-trigger-websocket/src/listener.rs +++ b/backend/windmill-trigger-websocket/src/listener.rs @@ -509,7 +509,7 @@ impl Clone for ReturnMessageChannels { } #[derive(Debug, Deserialize)] -enum InitialMessage { +pub(crate) enum InitialMessage { #[serde(rename = "raw_message")] RawMessage(String), #[serde(rename = "runnable_result")]