From deccd6538daff01213287e3caea007dc6c1b1ebb Mon Sep 17 00:00:00 2001 From: hugocasa Date: Wed, 16 Sep 2026 10:44:20 +0200 Subject: [PATCH] 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 --- backend/src/monitor.rs | 6 ++- .../tests/mcp_token_query_param.rs | 28 +++++++++++-- .../components/home/HomeConnectDrawer.svelte | 5 ++- .../src/lib/components/instanceSettings.ts | 2 +- .../components/settings/CreateToken.svelte | 39 +++++++++++++++---- frontend/src/lib/mcpAuth.ts | 18 ++++----- 6 files changed, 74 insertions(+), 24 deletions(-) diff --git a/backend/src/monitor.rs b/backend/src/monitor.rs index df7a220c05..3dc74c671c 100644 --- a/backend/src/monitor.rs +++ b/backend/src/monitor.rs @@ -286,8 +286,10 @@ pub async fn initial_load( ); if let Some(db) = conn.as_sql() { - // Outside the `server_mode` block below: `MODE=mcp` serves the MCP routes with - // `server_mode` false, and that deployment is the one most likely to set this. + // Outside the `server_mode` block below: a `MODE=mcp` process serves the MCP routes + // with `server_mode` false and would otherwise never read this at all. That mode + // joins no monitor loop, so there — as for every global setting, `base_url` + // included — this pass is the only read, and a change lands on restart. pass.setting( MCP_DISABLE_TOKEN_QUERY_PARAM_SETTING, false, diff --git a/backend/windmill-api-integration-tests/tests/mcp_token_query_param.rs b/backend/windmill-api-integration-tests/tests/mcp_token_query_param.rs index 1231f5f992..1f7b459bfe 100644 --- a/backend/windmill-api-integration-tests/tests/mcp_token_query_param.rs +++ b/backend/windmill-api-integration-tests/tests/mcp_token_query_param.rs @@ -1,9 +1,14 @@ //! 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 router. Reordering -//! that stack, or adding a third MCP mount without it, leaves the switch inert while every -//! other MCP test still passes, so the two mounts are pinned here together. +//! 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). +//! +//! What it does not cover: the `global_settings` load path. The switch is read straight from +//! the atomic here, so `monitor.rs` reaching it is not pinned by this test. #![cfg(feature = "mcp")] use std::sync::atomic::Ordering; @@ -26,6 +31,10 @@ async fn insert_mcp_token(db: &Pool) -> anyhow::Result<()> { 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 { Ok(reqwest::Client::new() .post(url) @@ -69,6 +78,19 @@ async fn test_mcp_token_query_param_switch(db: Pool) -> anyhow::Result ); } + // 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")) diff --git a/frontend/src/lib/components/home/HomeConnectDrawer.svelte b/frontend/src/lib/components/home/HomeConnectDrawer.svelte index 09cc51dc7f..94a8a56a5b 100644 --- a/frontend/src/lib/components/home/HomeConnectDrawer.svelte +++ b/frontend/src/lib/components/home/HomeConnectDrawer.svelte @@ -26,7 +26,10 @@ wmill sync pull`) export function openDrawer(tab: ConnectTab = 'cli') { selectedTab = tab openVersion += 1 - void mcpTokenUrlDisabled().then((v) => (tokenUrlDisabled = v)) + // Only drives this blurb's wording; CreateToken below surfaces a failed check itself. + void mcpTokenUrlDisabled() + .then((v) => (tokenUrlDisabled = v)) + .catch(() => (tokenUrlDisabled = false)) drawer?.openDrawer() } diff --git a/frontend/src/lib/components/instanceSettings.ts b/frontend/src/lib/components/instanceSettings.ts index d1f1995834..4acc11e758 100644 --- a/frontend/src/lib/components/instanceSettings.ts +++ b/frontend/src/lib/components/instanceSettings.ts @@ -689,7 +689,7 @@ export const settings: Record = { { label: 'Disable token in MCP URLs', description: - 'Reject the ?token= query parameter on the MCP endpoints, so MCP clients authenticate with an Authorization header or through the OAuth flow. A token in a URL is a credential that ends up in browser history, proxy logs and referrers. Existing MCP URLs carrying a token stop working.', + 'Reject the ?token= query parameter on the MCP endpoints, so MCP clients authenticate with an Authorization header or through the OAuth flow. A token in a URL is a credential that ends up in browser history, proxy logs and referrers. Existing MCP URLs carrying a token stop working. Servers and workers pick this up within a minute; dedicated MCP servers (MODE=mcp) apply it when they next restart.', key: 'mcp_disable_token_query_param', fieldType: 'boolean', storage: 'setting' diff --git a/frontend/src/lib/components/settings/CreateToken.svelte b/frontend/src/lib/components/settings/CreateToken.svelte index f7f904dec9..259c7eae2e 100644 --- a/frontend/src/lib/components/settings/CreateToken.svelte +++ b/frontend/src/lib/components/settings/CreateToken.svelte @@ -1,7 +1,7 @@