fix(mcp): let MCP tokens call preview run tools (jobs:run scope) — Fixes GIT-920

[rubenfiszel]

Problem

An MCP token (e.g. mcp:all) could not call the built-in runScriptPreviewAndWaitResult tool. Every call returned:

HTTP 403 Forbidden: Permission denied: Required scope: jobs:run
POST /w/{workspace}/jobs/run_wait_result/preview

Read tools and deployed-script tools over the same token worked fine; only ad-hoc preview runs failed. Reported as fallout from the scope-enforcement work (#9712).

Root cause

The MCP proxy (create_http_request in mcp/utils.rs) mints an internal JWT scoped to exactly scope_for_route(method, path) for the endpoint it forwards to, so a scope-restricted MCP token can't be widened into a blank check.

For preview run routes, determine_kind_from_route("jobs/run_wait_result/preview") matched the SCRIPT_JOBS prefix "jobs/run_wait_result/p" — because "preview" starts with "p" — and derived the runnable kind scripts, producing the minted scope jobs:run:scripts.

But preview handlers (run_preview_script / run_preview_flow_job) run arbitrary, request-supplied code with no deployed path, and deliberately require the broad jobs:run scope so a narrowly-scoped token can't escape its scope. jobs:run:scripts does not include jobs:run, so check_scopes rejected it with 403.

The route-derivation (scope_for_route) and the handler requirement (check_scopes(jobs:run)) had silently diverged for preview routes.

Fix

determine_kind_from_route now returns no kind for preview/bundle routes, so scope_for_route derives the broad jobs:run that the handlers require. The match is anchored to the endpoint segment (jobs/run/preview* or jobs/run_wait_result/preview*) rather than a contains("preview") substring test.

This matters because determine_kind_from_route also feeds the direct-API check_route_access middleware. A substring test would also match a by-path run of a deployed runnable whose own path contains preview (e.g. jobs/run_wait_result/p/f/team/preview_report): that route would then derive the broad jobs:run, and a legitimately kind-scoped jobs:run:scripts:* / jobs:run:flows:* token would be rejected with 403. Anchoring to the endpoint segment keeps those by-path runs on their correct kind.

scope_for_route is only consumed by the MCP proxy; the by-path run routes (run/p/*, run/f/*, etc.) keep their kind.

Validation

Unit tests (test_scope_for_route):

• All five preview routes derive jobs:run.
• By-path runs with a preview-named path still derive their kind: run/p/u/alice/preview_report and run_wait_result/p/f/team/preview_report → jobs:run:scripts; run/f/f/team/preview_report → jobs:run:flows.

End-to-end, against a local backend built with --features mcp,quickjs, before/after each change:

MCP preview path — driving the real MCP streamable-HTTP protocol with an mcp:all token calling runScriptPreviewAndWaitResult:

• Before (guard disabled, rebuilt): reproduces the exact reported 403 Required scope: jobs:run.
• After: 200, result "hello-git920".

By-path regression — a deployed bash script at u/admin/preview_report, run via POST /w/admins/jobs/run_wait_result/p/u/admin/preview_report with a token scoped only to jobs:run:scripts:u/admin/preview_report:

• With contains("preview") (rebuilt): 403 Access denied. Required scope: jobs:run — confirms the substring approach was a real functional break.
• With the endpoint-anchored fix: 200, result "bypath-git920", while the MCP preview path still returns 200.

Fixes GIT-920

🤖 Generated with Claude Code

[claude]

Claude Code is working…

I'll analyze this and get back to you.

View job run

[claude]

Claude finished @rubenfiszel's task in 2m 33s —— View job

---

PR Review — fix(mcp): let MCP tokens call preview run tools

Mergeable, but should ideally address a nit: the route_path.contains("preview") match is broader than the five enumerated preview routes and also catches by-path runs of deployed runnables whose path contains the substring preview, contradicting the PR's "by-path run routes are unaffected" claim.

• Read AGENTS.md / backend conventions
• Analyze the diff and root-cause reasoning
• Trace scope_for_route → MCP proxy usage (mcp/utils.rs)
• Verify scope-hierarchy impact of a kind-less mint
• Post review

---

The fix is correct for the reported bug and the root-cause analysis is accurate: scope_for_route minted jobs:run:scripts for preview routes while the preview handlers require the broad jobs:run, and (None, _) => {} at scopes.rs:814 means a kind-less jobs:run satisfies any handler check. Nice, well-scoped fix with a clear comment and a unit test covering all five preview routes.

Findings

P2 — contains("preview") is broader than the five preview routes (backend/windmill-api-auth/src/scopes.rs:608)

route_path here is the full route suffix (e.g. jobs/run_wait_result/p/f/team/preview_report), not just the endpoint segment. A by-path run of a deployed script/flow whose path contains the substring preview — e.g. a script at f/team/preview_report invoked via run_wait_result/p/... — will also hit this branch and derive the broad jobs:run instead of jobs:run:scripts.

Impact is low, not a functional break: the by-path handler check passes either way (jobs:run ⊇ jobs:run:scripts via (None, _) => {} at line 814), so runs still succeed. But it does two things worth avoiding:

• It contradicts the PR description's invariant that "by-path run routes (run/p/*, run/f/*, etc.) are unaffected and still carry their kind" — they are affected when the user's runnable path happens to contain preview.
• It slightly weakens the defense-in-depth scope-narrowing the MCP proxy intends (mcp/utils.rs:505 — "Bound the minted JWT to exactly this proxied route"), minting a broader-than-necessary internal JWT for those paths.

A prefix-anchored check avoids the edge case and is exactly as readable:

if route_path.starts_with("jobs/run/preview")
    || route_path.starts_with("jobs/run_wait_result/preview")
{
    return None;
}

This still covers all five routes (run/preview, run/preview_bundle, run/preview_flow, run_wait_result/preview, run_wait_result/preview_flow) while excluding jobs/run/p/... and jobs/run_wait_result/p/... by-path runs.

Fix this →

AGENTS.md compliance

No violations. The added comment records the non-obvious constraint (why preview routes must carry no kind) rather than narrating the code — consistent with the "comments record constraints" rule.

Test coverage

Backend-only change. The new test_scope_for_route assertions cover the five preview routes deriving jobs:run — good. Worth adding one negative assertion to lock in the intended boundary: a by-path run whose runnable path contains preview (e.g. /api/w/ws/jobs/run_wait_result/p/f/team/preview_report) should still derive jobs:run:scripts. With the current contains check that assertion would fail, which is precisely the edge case above; with the prefix-anchored fix it passes.

No integration test needed here — the PR notes an end-to-end MCP verification against a local --features mcp,quickjs backend, which is the right manual check. No in-app frontend surface to exercise.

[cloudflare-workers-and-pages]

Deploying windmill with    Cloudflare Pages

Latest commit:	5c2812a
Status:	✅  Deploy successful!
Preview URL:	https://872ba7f7.windmill.pages.dev
Branch Preview URL:	https://ruben-git-920-bug-mcp-token.windmill.pages.dev

View logs

[github-actions]

Pi Review

cc @rubenfiszel

Mergeable, but should ideally address nits: comment exceeds AGENTS.md 4-line limit in scopes.rs

---

P2 — Comment exceeds AGENTS.md length limit and includes drafting history

File: backend/windmill-api-auth/src/scopes.rs, lines ~599–609

The new comment before if route_path.contains("preview") is 9 lines. AGENTS.md Core Principles states:

Comments record constraints, not narration. Write a comment only for what the code can't show: why a non-obvious approach is required, what breaks if it's "simplified" away. State each invariant once, at the place where someone would break it, in ≤4 lines.

Two issues:

1. Length — 9 lines vs the ≤4-line guideline.
2. Drafting history — the clause "This diverged for the MCP proxy, which mints exactly the derived scope and was then rejected by the handler's jobs:run check" describes the prior broken state, which is meaningless to a reader who never saw the earlier iteration. AGENTS.md: "Describe the code as it is, never its drafting history."

The essential invariant — "preview routes execute arbitrary code and require jobs:run, so they must not carry a kind" — can fit in 3-4 lines.

(Similarly, the test comment at ~line 1311 is 5 lines including the GIT-920 parenthetical; trimming it to ≤4 lines would align with the same guideline, though test code is less critical.)

---

What looks good

• Correctness: The fix correctly short-circuits determine_kind_from_route for any preview route, so scope_for_route derives jobs:run (no kind suffix), matching what the handler's check_scopes(&authed, || format!("jobs:run")) actually requires. The contains("preview") check is safe — no non-preview jobs route contains that substring.
• No auth bypass: The handler run_preview_script / run_preview_flow_job still performs its own jobs:run scope check; this change only removes the false-negative at the MCP-proxy layer where a narrower scope (jobs:run:scripts) was being minted.
• Test coverage: The new test asserts all five preview routes (run/preview, run/preview_bundle, run/preview_flow, run_wait_result/preview, run_wait_result/preview_flow) derive jobs:run, and the by-path routes (run/p/*, run/f/*) are preserved with their kind suffixes. The existing test assertions remain unchanged. E2E validation with a real MCP token confirms the 403→200 fix. Coverage is appropriate for the change.

---

Test coverage

Backend (Rust): Adequate. The PR adds a unit test covering all five preview paths in test_scope_for_route and the PR description documents an end-to-end MCP protocol test against a local backend. No additional integration tests are required — the change is scoped to a single internal scope-derivation function whose only consumer (scope_for_route) is already exercised by existing tests for the non-preview paths.

Manual verification: No in-app UI surface to exercise. The fix is purely a backend MCP-proxy scope-derivation change. The e2e test described in the PR body (drive a real MCP streamable-HTTP connection with an mcp:all token calling runScriptPreviewAndWaitResult, confirm 200 and correct result) is sufficient for manual validation.

[github-actions]

Codex Review

cc @rubenfiszel
Should address issues before merging: substring preview matching breaks kind-scoped by-path runs

Found 1 issue:

1. [P1] Restrict the exception to actual preview endpoints
   backend/windmill-api-auth/src/scopes.rs:608

   route_path includes the deployed script or flow path, so contains("preview") also matches routes such as /jobs/run/p/u/alice/preview_report. Those routes now derive broad jobs:run; consequently, the middleware rejects valid jobs:run:scripts or jobs:run:flows tokens with 403. Match the five preview endpoint shapes explicitly or use segment-boundary-aware matching.

Test coverage

The added unit cases cover the intended preview endpoints but need regression cases for by-path script and flow routes containing preview.

No frontend surface changed. Manually run such a deployed path with a kind-scoped token and confirm it still succeeds, alongside the MCP preview-run scenario.

[github-actions]

Pi Review

Good to merge.

The combined two-commit diff resolves the prior reviews' substring-matching concern cleanly. starts_with("jobs/run/preview") / starts_with("jobs/run_wait_result/preview") correctly anchors to the endpoint segment — by-path runs like jobs/run/p/f/team/preview_report never hit the early-return and continue to the existing FLOW_JOBS/SCRIPT_JOBS prefix checks. The new test block covers all five preview routes deriving jobs:run, plus three by-path regression cases for deployed runnables whose path contains the substring preview (both run/p/ and run_wait_result/p/ for scripts, plus run/f/ for flows). No other files touched.

Findings: none. The only remaining delta from AGENTS.md is the 6-line comment at scopes.rs:599-604 — it exceeds the ≤4-line guideline, but every line is substantive constraint-documentation (why preview routes must be kindless, why prefix anchoring matters), and the author already trimmed it from 9 lines in response to the earlier review. Not worth blocking on.

What looks good

• No auth widening: the MCP proxy now mints jobs:run for preview routes, which matches what the preview handlers' own check_scopes(&authed, \|\| format!("jobs:run")) require. A token scoped to jobs:run:scripts:* still cannot reach preview routes — the handler-side check remains the gate.
• No regression for by-path runs: jobs:run:scripts:*/jobs:run:flows:* tokens continue to work for deployed-script/flow runs, including when the deployed path happens to contain preview. Verified by the new test assertions.
• scope_for_route consumer: only the MCP proxy (mcp/utils.rs:520) calls it, so the scope-derivation change can't affect any other code path.

Test coverage

Backend (Rust): Well-covered. The new assertions in test_scope_for_route validate:

• All five preview routes → jobs:run
• Three by-path edge cases with preview in the path name → correct kind (scripts/flows)
• The existing non-preview test assertions remain pass-through unchanged

The PR body also documents an end-to-end MCP streamable-HTTP test against a local backend confirming the 403→200 fix. No additional integration tests are needed.

Manual verification: No in-app UI surface. The e2e scenario described in the PR body (MCP streamable-HTTP with mcp:all token calling runScriptPreviewAndWaitResult, confirm 200 and correct result) is sufficient.

[github-actions]

Codex Review

Good to merge

No issues found. The latest commit resolves the prior substring-matching concern, and no new public surfaces were introduced.

Test coverage

Backend unit coverage includes all five preview endpoints and by-path regressions containing preview.

No UI surface changed. The documented MCP end-to-end check confirms the preview request succeeds with the expected result.