mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-09-21 00:02:30 +00:00
* feat: add an instance setting to refuse a token in MCP URLs
MCP clients are commonly configured with the token in the URL
(`/api/mcp/w/{workspace}/mcp?token=...`). A URL-borne credential ends up in
browser history, proxy logs and referrers, so an instance can now turn that
channel off with the `mcp_disable_token_query_param` global setting and leave
the Authorization header as the only way in, which sends MCP clients through
the OAuth flow the endpoints already advertise.
The rejection is a middleware on both the workspaced and the gateway MCP
mounts, layered outside everything that reads a token and inside the
WWW-Authenticate layer, so the 401 carries the resource pointer a client needs
to start OAuth discovery.
Off by default. With it on, the token drawer and the home connect drawer stop
offering to mint a token for an MCP URL and hand over the bare URL instead.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix: read the MCP URL policy when a URL is asked for, and drop the all-workspaces option
Two review findings on the token drawer:
The policy was read once per page load and cached for the browser session, so a
superadmin turning the setting on left every open tab handing out `?token=` URLs
the server now refuses. Both entry points now read it when the user actually asks
for an MCP URL: when MCP mode is entered, and when the connect drawer opens.
The workspace picker offered "All workspaces / Multi-workspace", but the gateway's
consent screen binds the token it issues to the one workspace picked there, so
OAuth has no multi-workspace grant to hand out. That entry is now token-only.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix: don't guess the MCP URL policy, and say where the switch lands on restart
Review findings:
The comment on the settings load claimed `MODE=mcp` as the target deployment, but
that mode joins no monitor loop, so the startup pass is its only read and a change
lands on restart. That is true of every global setting there, `base_url` included;
the comment now says so, and the setting description tells an operator running
dedicated MCP servers what to expect.
A failed settings probe resolved to "tokens allowed", so with the switch on the
drawer would mint a non-expiring token and hand over a URL the server refuses for
as long as it exists. The probe now propagates its error and the panel reports it
with a retry, creating nothing until the answer is known.
The test passed a valid token, so it could not tell a rejection before
authentication from one after it. It now also sends a token that was never valid
and asserts the middleware's own message, which fails if the layer moves inward.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix: withhold the MCP URL until a workspace is picked
With no persisted workspace the store starts undefined, so opening the drawer from
/user/workspaces before choosing one rendered a copyable
`/api/mcp/w/undefined/mcp`. It reads like a real URL and a client pointed at it
would never connect. The panel now asks for a workspace instead, matching the guard
the token branch already has on its generate button.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* docs: drop the coverage-status note from the MCP switch test
It documented what the test does not reach rather than a constraint the next
reader could break; that belongs in the PR, not the module doc. The layer-order
rationale, which is what a future edit would break, stays.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* refactor: fall back to the bare MCP URL instead of alerting on a failed read
When the setting read fails, show the bare URL rather than an error with a retry.
It works whichever way the setting is, so no alert is needed, and it still never
mints a token for a URL the server may refuse. The connect drawer's wording falls
back the same way so the blurb matches the panel.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* refactor: move the MCP URL token setting to Core
It sat in the Auth/OAuth/SAML list, which the settings sidebar shows under SSO,
suggesting a dependency on SSO that does not exist: MCP OAuth has Windmill act as
the authorization server, and any login method, password included, completes it.
It is an instance-wide credential policy, so it now lives with the other ones in
Core, kept out of quick setup like its neighbours.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
103 lines
4.2 KiB
Rust
103 lines
4.2 KiB
Rust
//! The `mcp_disable_token_query_param` switch closes the URL-borne credential path.
|
|
//!
|
|
//! The rejection is a middleware layered between the `WWW-Authenticate` decorator and
|
|
//! everything that reads a token, on both the workspaced and the gateway mount. Each half of
|
|
//! that sandwich is pinned: the `WWW-Authenticate` header on the refusal catches the layer
|
|
//! being moved outward (a client would lose the pointer that starts OAuth discovery), and
|
|
//! refusing a token that was never valid catches it being moved inward past authentication
|
|
//! (the URL-borne token would be hashed and looked up before anything refused it).
|
|
#![cfg(feature = "mcp")]
|
|
|
|
use std::sync::atomic::Ordering;
|
|
|
|
use serde_json::json;
|
|
use sqlx::{Pool, Postgres};
|
|
|
|
use windmill_common::global_settings::MCP_DISABLE_TOKEN_QUERY_PARAM;
|
|
use windmill_test_utils::*;
|
|
|
|
/// Workspace-less with an `mcp:` scope, which is what the gateway mount requires; the
|
|
/// workspaced mount takes its workspace from the path, so one token reaches both.
|
|
async fn insert_mcp_token(db: &Pool<Postgres>) -> anyhow::Result<()> {
|
|
sqlx::query(
|
|
"INSERT INTO token (token_hash, token_prefix, token, email, label, super_admin, scopes)
|
|
VALUES (encode(sha256('MCP_TOKEN'::bytea), 'hex'), 'MCP_TOK', 'MCP_TOKEN', 'test@windmill.dev', 'mcp token', true, ARRAY['mcp:all'])",
|
|
)
|
|
.execute(db)
|
|
.await?;
|
|
Ok(())
|
|
}
|
|
|
|
/// A token that is not in `token` at all. Authentication would refuse it on its own, so a
|
|
/// refusal carrying the middleware's own wording is evidence nothing looked it up first.
|
|
const BOGUS_TOKEN: &str = "NOT_A_REAL_TOKEN";
|
|
|
|
async fn tools_list(url: &str) -> anyhow::Result<reqwest::Response> {
|
|
Ok(reqwest::Client::new()
|
|
.post(url)
|
|
.header("Accept", "application/json, text/event-stream")
|
|
.json(&json!({ "jsonrpc": "2.0", "id": 1, "method": "tools/list", "params": {} }))
|
|
.send()
|
|
.await?)
|
|
}
|
|
|
|
#[sqlx::test(migrations = "../migrations", fixtures("base"))]
|
|
async fn test_mcp_token_query_param_switch(db: Pool<Postgres>) -> anyhow::Result<()> {
|
|
initialize_tracing().await;
|
|
set_jwt_secret().await;
|
|
insert_mcp_token(&db).await?;
|
|
let server = ApiServer::start_mcp(db.clone()).await?;
|
|
let port = server.addr.port();
|
|
let workspaced =
|
|
format!("http://localhost:{port}/api/mcp/w/test-workspace/mcp?token=MCP_TOKEN");
|
|
let gateway = format!("http://localhost:{port}/api/mcp/gateway?token=MCP_TOKEN");
|
|
|
|
assert_eq!(
|
|
tools_list(&workspaced).await?.status(),
|
|
200,
|
|
"a URL-borne token is the documented default and must keep working while the switch is off"
|
|
);
|
|
assert_eq!(tools_list(&gateway).await?.status(), 200);
|
|
|
|
MCP_DISABLE_TOKEN_QUERY_PARAM.store(true, Ordering::Relaxed);
|
|
|
|
for url in [&workspaced, &gateway] {
|
|
let resp = tools_list(url).await?;
|
|
assert_eq!(
|
|
resp.status(),
|
|
401,
|
|
"{url} still admitted a token in the URL"
|
|
);
|
|
// What sends the client into the OAuth flow rather than leaving it stuck on a 401.
|
|
assert!(
|
|
resp.headers().contains_key("www-authenticate"),
|
|
"{url} rejected without pointing at the authorization server"
|
|
);
|
|
}
|
|
|
|
// Refused before authentication, not after: an invalid token gets the middleware's own
|
|
// message rather than the generic 401 that looking it up would produce.
|
|
let resp = tools_list(&format!(
|
|
"http://localhost:{port}/api/mcp/w/test-workspace/mcp?token={BOGUS_TOKEN}"
|
|
))
|
|
.await?;
|
|
assert_eq!(resp.status(), 401);
|
|
assert!(
|
|
resp.text().await?.contains("does not accept a token in the MCP URL"),
|
|
"an invalid URL token was answered by authentication, so the token was read before \
|
|
the switch refused it"
|
|
);
|
|
|
|
// The header stays open: it is the channel the OAuth flow itself hands tokens over on.
|
|
let resp = reqwest::Client::new()
|
|
.post(format!("http://localhost:{port}/api/mcp/gateway"))
|
|
.header("Accept", "application/json, text/event-stream")
|
|
.header("Authorization", "Bearer MCP_TOKEN")
|
|
.json(&json!({ "jsonrpc": "2.0", "id": 1, "method": "tools/list", "params": {} }))
|
|
.send()
|
|
.await?;
|
|
assert_eq!(resp.status(), 200);
|
|
|
|
Ok(())
|
|
}
|