mirror of
https://github.com/windmill-labs/windmill.git
synced 2026-10-13 16:02:59 +00:00
* feat: let the global AI chat call connected MCP servers as the user Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: address review findings on the chat MCP tools Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat: connect MCP servers from a predefined list in chat and agent steps Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat: show the OAuth redirect URL in the instance connect settings Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: clarify the OAuth redirect URL copy in instance settings Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: match the instance settings warning style and drop the redirect tooltip Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: use the standard warning alert for the redirect url mismatch Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: correct the GitHub token guidance in the MCP registry Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat: warn when an OAuth connect lacks the scopes an MCP server needs Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: request the connect's scopes when the oauth popup is opened directly Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat: connect an oauth-app MCP server without leaving the panel Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix: seed connect scopes from the instance config only Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat: make the chat use only the MCP servers you turn on Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * refactor: align the MCP connect UI with the design system * feat: make a pasted url the default way to connect an mcp server * feat: show provider icons on the suggested mcp servers * fix: make both mcp sign-in paths behave the same and stop reloading on toggle * fix: clarify the mcp tool step's server field and drop its info alert * fix: name the mcp resource in the tool step and move the transport note into the connect box * fix: drop the redundant description on the mcp resource field * fix: make the mcp connections trigger icon-only * fix: scope enabled mcp servers to the account and address review nits * fix: wait for connect scopes and create session connections in the operating workspace * feat: move mcp connections into the chat's plus menu and fix review findings * fix: show mcp servers as checkboxes so off reads as a state * feat: give menu rows an on/off switch and use it for mcp servers * fix: lead the mcp menu rows with the switch * feat: keep the menu open while toggling and simplify the connect card * fix: ask for the server before the credential in the connect card * fix: show one credential path at a time in the connect card * fix: label the path field and move token guidance into its tooltip * fix: open straight into connect and keep the server menu scannable * feat: warn when an mcp connection lands outside your own space * refactor: require the workspace on the mcp connect components and rename the oauth child * fix: replace the oauth variable on reconnect and bound every mcp result * feat: show a connected server's provider icon in the connections list * feat: resolve mcp provider icons from the url and clarify the path field * style: align the mcp connect card with the design system surfaces * style: drop the redundant oauth support line and name the scopes oauth scopes * feat: keep the mcp connect card open in the connections drawer * feat: preopen the mcp connect card under the agent step resource picker * feat: resolve a typed mcp url to its registry entry and describe the token field * style: name both mcp connect actions connect * style: name the mcp oauth actions connect with the provider * style: say in the path description what the connect action will save * style: name the resource type in the mcp connect path description * feat: cache mcp provider icons and confirm disconnect in a modal * fix: keep the mcp menu switches live and the disconnect modal above the drawer * style: fall back to the plug icon in the mcp menu rows * fix: never destroy a foreign variable or resource when connecting an mcp server * fix: prove a token variable is ours before writing it and bound mcp search failures * fix: pin an mcp oauth popup to the target it was opened for * fix: bind an mcp credential to the server and popup it was requested for * fix: bound mcp tool calls with a deadline and drop stale server listings * fix: keep the disconnect confirmation handler returning void * fix: tie the mcp tool cache to the resource revision and the grant to its scopes * fix: verify mcp read-only server-side, keep oauth connector mounted Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
185 lines
7.3 KiB
Rust
185 lines
7.3 KiB
Rust
//! Regression test for the MCP token-exfiltration vulnerability.
|
|
//!
|
|
//! `GET /api/w/{w}/resources/mcp_tools/{path}` builds an MCP client from a
|
|
//! resource whose `token` field is a `$var:` reference. Before the fix the token
|
|
//! was resolved with `get_secret_value_as_admin` on the bare DB pool — no RLS,
|
|
//! no audit — so any workspace member who could read an MCP *resource* could
|
|
//! point its token at *any* secret variable in the workspace (e.g. one in an
|
|
//! admin-only folder) and have it decrypted and shipped as a bearer token.
|
|
//!
|
|
//! The fix resolves the token through the caller's permissioned path
|
|
//! (`get_value_internal` over the authed `user_db`), so the variable RLS — the
|
|
//! same gate as `variables/get_value` — applies and the secret read is audited.
|
|
//!
|
|
//! This test pins, against the `mcp_token_exfil` fixture:
|
|
//! - a plain developer (test-user-3) who can read the MCP resource but has no
|
|
//! access to the locked secret is DENIED (401) at token resolution, before
|
|
//! any connection is attempted, and the secret never leaks;
|
|
//! - an admin (test-user) clears the variable-RLS gate, the token resolves,
|
|
//! and the request only fails later at the connect/SSRF step — proving the
|
|
//! legitimate path still resolves the token (no over-blocking).
|
|
//!
|
|
//! `POST .../resources/mcp_call_tool/{path}` reaches the same MCP server through
|
|
//! the same resource, so it is pinned to the same property here — both handlers
|
|
//! share `connect_mcp_client`, and a future split of that helper must not let
|
|
//! one of them regress.
|
|
//!
|
|
//! SSRF rejection of an author-controlled URL is covered by the unit test in
|
|
//! `windmill-mcp` (`from_resource_rejects_ssrf_url`).
|
|
#![cfg(feature = "mcp")]
|
|
|
|
use sqlx::{Pool, Postgres};
|
|
use windmill_test_utils::*;
|
|
|
|
const SECRET_VALUE: &str = "S3CRET-MCP-TOKEN-VALUE";
|
|
const RESOURCE_PATH: &str = "u/test-user-3/evil_mcp";
|
|
|
|
fn client() -> reqwest::Client {
|
|
reqwest::Client::new()
|
|
}
|
|
|
|
async fn get(base: &str, path: &str, token: &str) -> (reqwest::StatusCode, String) {
|
|
let resp = client()
|
|
.get(format!("{base}/{path}"))
|
|
.header("Authorization", format!("Bearer {token}"))
|
|
.send()
|
|
.await
|
|
.expect("request");
|
|
let status = resp.status();
|
|
let body = resp.text().await.expect("body");
|
|
(status, body)
|
|
}
|
|
|
|
async fn post(
|
|
base: &str,
|
|
path: &str,
|
|
token: &str,
|
|
body: serde_json::Value,
|
|
) -> (reqwest::StatusCode, String) {
|
|
let resp = client()
|
|
.post(format!("{base}/{path}"))
|
|
.header("Authorization", format!("Bearer {token}"))
|
|
.json(&body)
|
|
.send()
|
|
.await
|
|
.expect("request");
|
|
let status = resp.status();
|
|
let body = resp.text().await.expect("body");
|
|
(status, body)
|
|
}
|
|
|
|
/// Insert the locked secret variable with a real, workspace-key-encrypted value
|
|
/// so an authorized read genuinely decrypts it.
|
|
async fn insert_locked_secret(db: &Pool<Postgres>) -> anyhow::Result<()> {
|
|
let mc = windmill_common::variables::build_crypt(db, "test-workspace").await?;
|
|
let encrypted = windmill_common::variables::encrypt(&mc, SECRET_VALUE);
|
|
// Runtime-checked query (not the `query!` macro) so no offline `.sqlx` cache
|
|
// entry is needed for this test-only insert.
|
|
sqlx::query(
|
|
"INSERT INTO variable (workspace_id, path, value, is_secret, description, extra_perms)
|
|
VALUES ('test-workspace', 'f/locked/secret_token', $1, true, 'Locked secret', '{}')",
|
|
)
|
|
.bind(&encrypted)
|
|
.execute(db)
|
|
.await?;
|
|
Ok(())
|
|
}
|
|
|
|
#[sqlx::test(fixtures("base", "mcp_token_exfil"))]
|
|
async fn test_mcp_token_not_exfiltrated(db: Pool<Postgres>) -> anyhow::Result<()> {
|
|
initialize_tracing().await;
|
|
|
|
insert_locked_secret(&db).await?;
|
|
|
|
let server = ApiServer::start(db.clone()).await?;
|
|
let port = server.addr.port();
|
|
let base = format!("http://localhost:{port}/api/w/test-workspace/resources/mcp_tools");
|
|
let path = RESOURCE_PATH;
|
|
|
|
// ---- CORE REGRESSION: the developer can read the resource but must NOT be
|
|
// able to resolve the locked secret. They are denied (401) at the
|
|
// variable-RLS gate, before any MCP connection is attempted, and the
|
|
// secret never appears in the response.
|
|
let (status, body) = get(&base, path, "SECRET_TOKEN_3").await;
|
|
assert_eq!(
|
|
status,
|
|
reqwest::StatusCode::UNAUTHORIZED,
|
|
"developer must be denied resolving a secret they can't read (got {status}): {body}"
|
|
);
|
|
assert!(
|
|
!body.contains(SECRET_VALUE),
|
|
"the locked secret must never leak to the developer: {body}"
|
|
);
|
|
assert!(
|
|
body.contains("don't have access"),
|
|
"denial should come from the variable-RLS gate, not a connection error: {body}"
|
|
);
|
|
// Pre-fix, the token was decrypted as admin and the handler proceeded to the
|
|
// connection step; that path must no longer be reached for the developer.
|
|
assert!(
|
|
!body.contains("Failed to connect to MCP server"),
|
|
"developer must be blocked before the connection step (would mean the token was resolved): {body}"
|
|
);
|
|
|
|
// ---- NO OVER-BLOCKING: an admin clears the variable-RLS gate, so the token
|
|
// resolves and the request only fails later at the connect/SSRF step.
|
|
// A different failure mode (not 401, reaches the connection) proves the
|
|
// legitimate read still works.
|
|
let (status, body) = get(&base, path, "SECRET_TOKEN").await;
|
|
assert_ne!(
|
|
status,
|
|
reqwest::StatusCode::UNAUTHORIZED,
|
|
"admin must clear the variable-RLS gate (got {status}): {body}"
|
|
);
|
|
assert!(
|
|
body.contains("Failed to connect to MCP server"),
|
|
"admin should resolve the token and only fail at the connect/SSRF step: {body}"
|
|
);
|
|
|
|
Ok(())
|
|
}
|
|
|
|
#[sqlx::test(fixtures("base", "mcp_token_exfil"))]
|
|
async fn test_mcp_call_tool_token_not_exfiltrated(db: Pool<Postgres>) -> anyhow::Result<()> {
|
|
initialize_tracing().await;
|
|
|
|
insert_locked_secret(&db).await?;
|
|
|
|
let server = ApiServer::start(db.clone()).await?;
|
|
let port = server.addr.port();
|
|
let base = format!("http://localhost:{port}/api/w/test-workspace/resources/mcp_call_tool");
|
|
let body = serde_json::json!({ "tool": "whoami", "arguments": {} });
|
|
|
|
let (status, resp) = post(&base, RESOURCE_PATH, "SECRET_TOKEN_3", body.clone()).await;
|
|
assert_eq!(
|
|
status,
|
|
reqwest::StatusCode::UNAUTHORIZED,
|
|
"developer must be denied resolving a secret they can't read (got {status}): {resp}"
|
|
);
|
|
assert!(
|
|
!resp.contains(SECRET_VALUE),
|
|
"the locked secret must never leak to the developer: {resp}"
|
|
);
|
|
assert!(
|
|
resp.contains("don't have access"),
|
|
"denial should come from the variable-RLS gate, not a connection error: {resp}"
|
|
);
|
|
assert!(
|
|
!resp.contains("Failed to connect to MCP server"),
|
|
"developer must be blocked before the connection step (would mean the token was resolved): {resp}"
|
|
);
|
|
|
|
let (status, resp) = post(&base, RESOURCE_PATH, "SECRET_TOKEN", body).await;
|
|
assert_ne!(
|
|
status,
|
|
reqwest::StatusCode::UNAUTHORIZED,
|
|
"admin must clear the variable-RLS gate (got {status}): {resp}"
|
|
);
|
|
assert!(
|
|
resp.contains("Failed to connect to MCP server"),
|
|
"admin should resolve the token and only fail at the connect/SSRF step: {resp}"
|
|
);
|
|
|
|
Ok(())
|
|
}
|