From 4917f79935acd4ecf9fb5a21d3131dec9ae9da44 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Wed, 15 Jul 2026 01:56:41 +0200 Subject: [PATCH] =?UTF-8?q?fix(mcp):=20let=20MCP=20tokens=20call=20preview?= =?UTF-8?q?=20run=20tools=20(jobs:run=20scope)=20=E2=80=94=20Fixes=20GIT-9?= =?UTF-8?q?20=20(#10107)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(mcp): let MCP tokens call preview run tools (jobs:run scope) The MCP proxy mints an internal JWT scoped to exactly `scope_for_route` for the endpoint it forwards to. For preview run routes (`run/preview`, `run/preview_bundle`, `run/preview_flow`, `run_wait_result/preview`, `run_wait_result/preview_flow`), `determine_kind_from_route` matched the `SCRIPT_JOBS` prefix `jobs/run_wait_result/p` (because "preview" starts with "p") and derived `jobs:run:scripts`. But the preview handlers run arbitrary request-supplied code with no deployed path and require the broad `jobs:run` scope, so `jobs:run:scripts` was rejected with 403 "Required scope: jobs:run". Preview/bundle routes now carry no runnable kind, so the derived scope is the broad `jobs:run` the handlers expect. This also aligns the route-level access check with the handler check for these routes. Fixes GIT-920 Co-Authored-By: Claude Opus 4.8 (1M context) * fix(mcp): anchor preview-route match to endpoint segment Address CI review: `route_path.contains("preview")` also matched by-path runs of a deployed runnable whose path contains "preview" (e.g. `jobs/run_wait_result/p/f/team/preview_report`). Since determine_kind_from_route also feeds check_route_access, such a route would derive the broad `jobs:run` and reject a legitimately kind-scoped `jobs:run:scripts:*`/`jobs:run:flows:*` token with 403. Anchor the exception to the actual preview endpoints (`jobs/run/preview*`, `jobs/run_wait_result/preview*`) so by-path runs keep their kind. Add regression tests for preview-named by-path paths, and trim the comments per AGENTS.md. Co-Authored-By: Claude Opus 4.8 (1M context) --------- Co-authored-by: Claude Opus 4.8 (1M context) --- backend/windmill-api-auth/src/scopes.rs | 47 +++++++++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/backend/windmill-api-auth/src/scopes.rs b/backend/windmill-api-auth/src/scopes.rs index 041885369c..3c09185650 100644 --- a/backend/windmill-api-auth/src/scopes.rs +++ b/backend/windmill-api-auth/src/scopes.rs @@ -596,6 +596,17 @@ fn map_http_method_to_action(method: &str, route_path: &str) -> ScopeAction { /// Returns `"flows"` or `"scripts"` based on the match, or `None` if no match is found. fn determine_kind_from_route(route_path: &str) -> Option { if route_path.starts_with("jobs") { + // Preview/bundle runs execute arbitrary code with no deployed path, so + // their handlers require the broad `jobs:run` scope: they must carry no + // kind, else the derived scope is narrower than the handler demands. + // Anchor to the endpoint segment so by-path runs of a deployed runnable + // whose path contains "preview" (e.g. `run/p/f/team/preview_report`) are + // still classified by their kind. + if route_path.starts_with("jobs/run/preview") + || route_path.starts_with("jobs/run_wait_result/preview") + { + return None; + } if FLOW_JOBS.iter().any(|path| route_path.starts_with(path)) { return Some("flows".to_string()); } else if SCRIPT_JOBS.iter().any(|path| route_path.starts_with(path)) { @@ -1296,6 +1307,42 @@ mod tests { Some("jobs:run:flows") ); + // Preview/bundle runs have no deployed path and their handlers require the + // broad `jobs:run` scope, so the derived scope must not carry a kind. + for path in [ + "/api/w/ws/jobs/run/preview", + "/api/w/ws/jobs/run/preview_bundle", + "/api/w/ws/jobs/run/preview_flow", + "/api/w/ws/jobs/run_wait_result/preview", + "/api/w/ws/jobs/run_wait_result/preview_flow", + ] { + assert_eq!( + scope_for_route("POST", path).as_deref(), + Some("jobs:run"), + "preview route {path} must derive the broad jobs:run scope" + ); + } + + // By-path runs of a deployed runnable whose path contains "preview" must + // still derive their kind (not be swept into the broad jobs:run above), + // otherwise a `jobs:run:scripts:*`/`jobs:run:flows:*` token is denied. + assert_eq!( + scope_for_route("POST", "/api/w/ws/jobs/run/p/u/alice/preview_report").as_deref(), + Some("jobs:run:scripts") + ); + assert_eq!( + scope_for_route( + "POST", + "/api/w/ws/jobs/run_wait_result/p/f/team/preview_report" + ) + .as_deref(), + Some("jobs:run:scripts") + ); + assert_eq!( + scope_for_route("POST", "/api/w/ws/jobs/run/f/f/team/preview_report").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());