mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-09-21 00:02:30 +00:00
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>
This commit is contained in:
co-authored by
Claude Opus 5
parent
7206f92f63
commit
deccd6538d
@@ -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,
|
||||
|
||||
@@ -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<Postgres>) -> 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<reqwest::Response> {
|
||||
Ok(reqwest::Client::new()
|
||||
.post(url)
|
||||
@@ -69,6 +78,19 @@ async fn test_mcp_token_query_param_switch(db: Pool<Postgres>) -> 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"))
|
||||
|
||||
@@ -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()
|
||||
}
|
||||
|
||||
|
||||
@@ -689,7 +689,7 @@ export const settings: Record<string, Setting[]> = {
|
||||
{
|
||||
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'
|
||||
|
||||
@@ -1,7 +1,7 @@
|
||||
<script lang="ts">
|
||||
import { untrack } from 'svelte'
|
||||
import { userWorkspaces, workspaceStore, type UserWorkspace } from '$lib/stores'
|
||||
import { Alert, Button } from '../common'
|
||||
import { Alert, Button, Skeleton } from '../common'
|
||||
import { triggerableByAI } from '$lib/actions/triggerableByAI.svelte'
|
||||
import Toggle from '../Toggle.svelte'
|
||||
import { UserService, type NewToken } from '$lib/gen'
|
||||
@@ -55,9 +55,22 @@
|
||||
|
||||
let pickedScopes = $state<string[] | null>(null)
|
||||
let readOnly = $state(false)
|
||||
// Instance refuses `?token=` on the MCP endpoints, so a generated token would not get a
|
||||
// client in and the URL is handed over bare for the client's OAuth flow to complete.
|
||||
let tokenUrlDisabled = $state(false)
|
||||
// How this instance lets an MCP client in. `oauth` means it refuses `?token=`, so a
|
||||
// generated token would not get a client in and the URL is handed over bare instead.
|
||||
// Never assumed while unknown: guessing `token` mints a non-expiring credential for a
|
||||
// URL the server would refuse.
|
||||
type McpUrlPolicy = 'loading' | 'token' | 'oauth' | 'unavailable'
|
||||
let mcpUrlPolicy = $state<McpUrlPolicy>('loading')
|
||||
|
||||
async function loadMcpUrlPolicy() {
|
||||
mcpUrlPolicy = 'loading'
|
||||
try {
|
||||
mcpUrlPolicy = (await mcpTokenUrlDisabled()) ? 'oauth' : 'token'
|
||||
} catch (err) {
|
||||
console.error('Failed to load the MCP token setting:', err)
|
||||
mcpUrlPolicy = 'unavailable'
|
||||
}
|
||||
}
|
||||
|
||||
function ensureCurrentWorkspaceIncluded(
|
||||
workspacesList: UserWorkspace[],
|
||||
@@ -75,7 +88,7 @@
|
||||
|
||||
function enterMcpMode() {
|
||||
mcpCreationMode = true
|
||||
void mcpTokenUrlDisabled().then((v) => (tokenUrlDisabled = v))
|
||||
void loadMcpUrlPolicy()
|
||||
newTokenExpiration = undefined
|
||||
newTokenWorkspace = defaultNewTokenWorkspace ?? $workspaceStore
|
||||
newToken = undefined
|
||||
@@ -185,7 +198,7 @@
|
||||
scope chip stretch this card and push the rest of the form out of view. -->
|
||||
<div class="p-4 rounded-md mb-6 bg-surface-tertiary">
|
||||
<h3 class="pb-2 font-semibold text-emphasis text-sm">
|
||||
{mcpCreationMode && tokenUrlDisabled ? 'MCP URL' : title}
|
||||
{mcpCreationMode && mcpUrlPolicy !== 'token' ? 'MCP URL' : title}
|
||||
</h3>
|
||||
|
||||
{#if showMcpMode && !mcpOnly}
|
||||
@@ -216,7 +229,19 @@
|
||||
</div>
|
||||
{/if}
|
||||
|
||||
{#if mcpCreationMode && tokenUrlDisabled}
|
||||
{#if mcpCreationMode && mcpUrlPolicy === 'unavailable'}
|
||||
<Alert type="error" title="Could not check how this instance accepts MCP clients" size="xs">
|
||||
<div class="flex flex-col items-start gap-2">
|
||||
<span>
|
||||
Without that answer a generated token could be one this instance refuses, so nothing is
|
||||
created here until the check succeeds.
|
||||
</span>
|
||||
<Button onClick={loadMcpUrlPolicy} variant="default" unifiedSize="xs">Try again</Button>
|
||||
</div>
|
||||
</Alert>
|
||||
{:else if mcpCreationMode && mcpUrlPolicy === 'loading'}
|
||||
<Skeleton layout={[[2], 0.5, [1]]} />
|
||||
{:else if mcpCreationMode && mcpUrlPolicy === 'oauth'}
|
||||
{#if !lockWorkspace}
|
||||
<div class="mb-4 max-w-md">
|
||||
<span class="block mb-1 text-emphasis text-xs font-semibold">Workspace</span>
|
||||
|
||||
@@ -7,16 +7,14 @@ import { SettingService } from '$lib/gen'
|
||||
*
|
||||
* Deliberately uncached: callers read it at the moment an MCP URL is asked for, so a superadmin
|
||||
* flipping the setting does not leave open tabs handing out URLs the server now refuses.
|
||||
*
|
||||
* Throws rather than falling back. A caller that guessed `false` here would mint a
|
||||
* non-expiring token and hand over a URL the server refuses for as long as it exists.
|
||||
*/
|
||||
export async function mcpTokenUrlDisabled(): Promise<boolean> {
|
||||
try {
|
||||
return (
|
||||
((await SettingService.getGlobal({
|
||||
key: 'mcp_disable_token_query_param'
|
||||
})) as boolean | null) ?? false
|
||||
)
|
||||
} catch (err) {
|
||||
console.error('Failed to load the MCP token setting:', err)
|
||||
return false
|
||||
}
|
||||
return (
|
||||
((await SettingService.getGlobal({
|
||||
key: 'mcp_disable_token_query_param'
|
||||
})) as boolean | null) ?? false
|
||||
)
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user