From bc8a62a5e0cb660df16dd6dd891fe0a4c23bb75d Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Wed, 8 Jul 2026 11:40:44 +0000 Subject: [PATCH] feat: dedicated opt-in data tables toggle with read/write scope --- backend/windmill-api/src/mcp/core.rs | 18 +- backend/windmill-api/src/mcp/oauth_server.rs | 3 +- backend/windmill-mcp/src/common/scope.rs | 178 ++++++++++++------ backend/windmill-mcp/src/server/runner.rs | 28 ++- .../components/mcp/McpScopeSelector.svelte | 146 ++++++++++---- 5 files changed, 263 insertions(+), 110 deletions(-) diff --git a/backend/windmill-api/src/mcp/core.rs b/backend/windmill-api/src/mcp/core.rs index ea3f2e9c7f..a78ac01063 100644 --- a/backend/windmill-api/src/mcp/core.rs +++ b/backend/windmill-api/src/mcp/core.rs @@ -382,16 +382,14 @@ impl McpBackend for WindmillBackend { // Filter datatable list responses down to the datatables this token // is scoped to, so restricted tokens never even see other names. if let Some(config) = &dt_scope { - if config.datatables_restricted && !config.all { - if let Some(key) = datatable_list_filter_key(&endpoint_tool.name) { - if let Value::Array(items) = &mut result { - items.retain(|item| { - item.get(key) - .and_then(|v| v.as_str()) - .map(|n| config.is_datatable_allowed(n)) - .unwrap_or(false) - }); - } + if let Some(key) = datatable_list_filter_key(&endpoint_tool.name) { + if let Value::Array(items) = &mut result { + items.retain(|item| { + item.get(key) + .and_then(|v| v.as_str()) + .map(|n| config.is_datatable_allowed(n)) + .unwrap_or(false) + }); } } } diff --git a/backend/windmill-api/src/mcp/oauth_server.rs b/backend/windmill-api/src/mcp/oauth_server.rs index 9676b17a45..6cbbea07e4 100644 --- a/backend/windmill-api/src/mcp/oauth_server.rs +++ b/backend/windmill-api/src/mcp/oauth_server.rs @@ -231,7 +231,8 @@ fn supported_scopes() -> Vec { "mcp:scripts:*".to_string(), "mcp:flows:*".to_string(), "mcp:endpoints:*".to_string(), - "mcp:datatables:*".to_string(), + "mcp:datatables:read:*".to_string(), + "mcp:datatables:write:*".to_string(), ] } diff --git a/backend/windmill-mcp/src/common/scope.rs b/backend/windmill-mcp/src/common/scope.rs index a5ec1d334c..003f5df74e 100644 --- a/backend/windmill-mcp/src/common/scope.rs +++ b/backend/windmill-mcp/src/common/scope.rs @@ -12,12 +12,15 @@ pub struct McpScopeConfig { pub flows: Vec, /// Endpoint names/patterns allowed by this token pub endpoints: Vec, - /// Datatable names/patterns this token may read/write. Only meaningful when - /// `datatables_restricted` is set; otherwise datatable access is unrestricted. + /// Whether the datatable read tools (list/query/get schema) are granted. + /// Datatable access is opt-in and off by default — it is never implied by the + /// generic endpoint or favorites scopes, only by an explicit `mcp:datatables:` + /// scope (or the legacy `mcp:all`). + pub datatables_read: bool, + /// Whether the datatable write tools (insert/update) are granted. Implies read. + pub datatables_write: bool, + /// Datatable names/patterns access is restricted to. `*` (or empty) means all. pub datatables: Vec, - /// Whether any `mcp:datatables:` scope was present. Absent = opt-out (all - /// datatables allowed, backward compatible); present = restrict to the list. - pub datatables_restricted: bool, /// Whether this is a legacy "all" scope pub all: bool, /// Whether this is a "favorites" scope @@ -39,22 +42,30 @@ impl McpScopeConfig { "script" => &self.scripts, "flow" => &self.flows, "endpoint" => &self.endpoints, - "datatable" => &self.datatables, _ => return false, }; is_resource_allowed(path, patterns) } - /// Whether the token may touch datatable `name`. Datatable filtering is - /// opt-in: a token with no `mcp:datatables:` scope (`datatables_restricted` - /// false) may reach every datatable, so existing tokens keep working. Once a - /// restriction is present, only the listed names/patterns (or `*`) pass. + /// Whether datatable `name` is within this token's datatable restriction. + /// This is only the name filter — whether datatable access is granted at all + /// (and at which level) is `datatable_tool_allowed`. An empty list or `*` + /// means all datatables. pub fn is_datatable_allowed(&self, name: &str) -> bool { - if self.all || !self.datatables_restricted { - return true; + self.datatables.is_empty() + || self.datatables.iter().any(|d| d == "*") + || is_resource_allowed(name, &self.datatables) + } + + /// Whether a datatable tool with the given access level (`"read"` or + /// `"write"`) may be exposed/called. Write tools require write access; read + /// tools require read (write implies read). + pub fn datatable_tool_allowed(&self, access: &str) -> bool { + match access { + "write" => self.datatables_write, + _ => self.datatables_read || self.datatables_write, } - is_resource_allowed(name, &self.datatables) } /// Directional subset check: does this config grant at least everything @@ -91,21 +102,27 @@ impl McpScopeConfig { None => return false, } } - // Datatable containment: an unrestricted request grants every datatable, - // so a restricted caller (without `*`) cannot cover it. A restricted - // request must be covered by the caller's list — treating an unrestricted - // caller as `*`. - if !requested.datatables_restricted { - if self.datatables_restricted && !self.datatables.iter().any(|d| d == "*") { - return false; - } - } else { - let caller = if self.datatables_restricted { - self.datatables.clone() - } else { + // Datatable containment: the caller must grant at least the requested + // access level, and its datatable name list must cover the requested one + // (empty list = all datatables, i.e. `*`). + if requested.datatables_read && !(self.datatables_read || self.datatables_write) { + return false; + } + if requested.datatables_write && !self.datatables_write { + return false; + } + if requested.datatables_read || requested.datatables_write { + let caller = if self.datatables.is_empty() { vec!["*".to_string()] + } else { + self.datatables.clone() }; - if !resource_list_covers(&caller, &requested.datatables) { + let req = if requested.datatables.is_empty() { + vec!["*".to_string()] + } else { + requested.datatables.clone() + }; + if !resource_list_covers(&caller, &req) { return false; } } @@ -152,11 +169,14 @@ pub fn parse_mcp_scopes(scopes: &[String]) -> Result { } if scope == "mcp:all" { - // Legacy scope: grant access to everything + // Legacy scope: grant access to everything, including datatable + // read+write on every datatable. config.all = true; config.scripts.push("*".to_string()); config.flows.push("*".to_string()); config.endpoints.push("*".to_string()); + config.datatables_read = true; + config.datatables_write = true; config.datatables.push("*".to_string()); continue; } @@ -204,10 +224,23 @@ pub fn parse_mcp_scopes(scopes: &[String]) -> Result { continue; } - if let Some(resources) = scope.strip_prefix("mcp:datatables:") { - // Datatable restriction: mcp:datatables:name1,name2 or mcp:datatables:* - config.datatables_restricted = true; - config.datatables.extend(parse_resource_list(resources)); + if let Some(rest) = scope.strip_prefix("mcp:datatables:") { + // Datatable access (opt-in). Forms: + // mcp:datatables:write: -> read+write on those datatables + // mcp:datatables:read: -> read-only + // mcp:datatables: -> read-only (shorthand) + let (write, names) = if let Some(n) = rest.strip_prefix("write:") { + (true, n) + } else if let Some(n) = rest.strip_prefix("read:") { + (false, n) + } else { + (false, rest) + }; + config.datatables_read = true; + if write { + config.datatables_write = true; + } + config.datatables.extend(parse_resource_list(names)); continue; } @@ -219,6 +252,23 @@ pub fn parse_mcp_scopes(scopes: &[String]) -> Result { Ok(config) } +/// Classify an MCP endpoint tool as a datatable tool and its required access +/// level (`"read"` or `"write"`), or `None` if it is not a datatable tool. +/// Datatable tools are gated by the dedicated `mcp:datatables:` scope rather +/// than the generic endpoint/all scopes, so both the tool lister and the caller +/// need this classification. +pub fn datatable_access_level(tool_name: &str) -> Option<&'static str> { + match tool_name { + "listDataTables" + | "listDataTableTables" + | "listDataTableSchemas" + | "getDataTableTableSchema" + | "queryDataTable" => Some("read"), + "insertDataTable" | "updateDataTable" => Some("write"), + _ => None, + } +} + /// Parse comma-separated resource list fn parse_resource_list(resources: &str) -> Vec { if resources.is_empty() { @@ -398,43 +448,59 @@ mod tests { } #[test] - fn test_datatable_scope_parsing_and_allow() { - // No datatable scope => unrestricted (opt-in), every datatable allowed. - let c = cfg(&["mcp:endpoints:queryDataTable"]); - assert!(!c.datatables_restricted); - assert!(c.is_datatable_allowed("main")); + fn test_datatable_scope_opt_in_and_levels() { + // Datatable access is off by default — the generic endpoint/favorites + // scopes never grant it. + let c = cfg(&["mcp:endpoints:*"]); + assert!(!c.datatables_read && !c.datatables_write); + assert!(!c.datatable_tool_allowed("read")); + assert!(!c.datatable_tool_allowed("write")); + + // mcp:all grants read+write on all datatables. + let c = cfg(&["mcp:all"]); + assert!(c.datatable_tool_allowed("read")); + assert!(c.datatable_tool_allowed("write")); assert!(c.is_datatable_allowed("anything")); - // mcp:all grants all datatables. - assert!(cfg(&["mcp:all"]).is_datatable_allowed("main")); + // Read scope grants read tools only. + let c = cfg(&["mcp:datatables:read:*"]); + assert!(c.datatable_tool_allowed("read")); + assert!(!c.datatable_tool_allowed("write")); - // Explicit restriction to specific names. - let c = cfg(&["mcp:datatables:main,analytics"]); - assert!(c.datatables_restricted); + // Write scope implies read. + let c = cfg(&["mcp:datatables:write:main,analytics"]); + assert!(c.datatable_tool_allowed("read")); + assert!(c.datatable_tool_allowed("write")); assert!(c.is_datatable_allowed("main")); assert!(c.is_datatable_allowed("analytics")); assert!(!c.is_datatable_allowed("secret")); - // Wildcard restriction allows all. - let c = cfg(&["mcp:datatables:*"]); - assert!(c.datatables_restricted); - assert!(c.is_datatable_allowed("anything")); + // Shorthand (no read/write segment) is read-only. + let c = cfg(&["mcp:datatables:main"]); + assert!(c.datatable_tool_allowed("read")); + assert!(!c.datatable_tool_allowed("write")); + assert!(c.is_datatable_allowed("main")); + assert!(!c.is_datatable_allowed("other")); } #[test] fn test_contains_datatables() { - // Unrestricted caller covers a restricted request. - assert!(cfg(&["mcp:endpoints:*"]).contains(&cfg(&["mcp:datatables:main"]))); - // Restricted caller covers a subset request. - assert!(cfg(&["mcp:datatables:main,analytics"]).contains(&cfg(&["mcp:datatables:main"]))); - // Restricted caller must NOT widen into a sibling or into unrestricted. - assert!(!cfg(&["mcp:datatables:main"]).contains(&cfg(&["mcp:datatables:analytics"]))); - assert!(!cfg(&["mcp:datatables:main"]).contains(&cfg(&["mcp:endpoints:queryDataTable"]))); - // `*` restriction covers both a subset and an unrestricted request. - assert!(cfg(&["mcp:datatables:*"]).contains(&cfg(&["mcp:datatables:main"]))); - assert!(cfg(&["mcp:datatables:*"]).contains(&cfg(&[]))); + // A write caller covers a read request on a subset. + assert!(cfg(&["mcp:datatables:write:main,analytics"]) + .contains(&cfg(&["mcp:datatables:read:main"]))); + // A read caller must NOT grant write. + assert!(!cfg(&["mcp:datatables:read:*"]).contains(&cfg(&["mcp:datatables:write:main"]))); + // No datatable caller cannot grant datatable access. + assert!(!cfg(&["mcp:endpoints:*"]).contains(&cfg(&["mcp:datatables:read:main"]))); + // Restricted caller must NOT widen into a sibling or into `*`. + assert!( + !cfg(&["mcp:datatables:read:main"]).contains(&cfg(&["mcp:datatables:read:analytics"])) + ); + assert!(!cfg(&["mcp:datatables:read:main"]).contains(&cfg(&["mcp:datatables:read:*"]))); + // `*` caller covers a subset request. + assert!(cfg(&["mcp:datatables:write:*"]).contains(&cfg(&["mcp:datatables:read:main"]))); // mcp:all covers any datatable request. - assert!(cfg(&["mcp:all"]).contains(&cfg(&["mcp:datatables:main"]))); + assert!(cfg(&["mcp:all"]).contains(&cfg(&["mcp:datatables:write:main"]))); } #[test] diff --git a/backend/windmill-mcp/src/server/runner.rs b/backend/windmill-mcp/src/server/runner.rs index 6d33573d95..605e66ff8d 100644 --- a/backend/windmill-mcp/src/server/runner.rs +++ b/backend/windmill-mcp/src/server/runner.rs @@ -244,7 +244,16 @@ impl ServerHandler for Runner { // Add endpoint tools from the generated MCP tools, filtered by scope let endpoint_tools = self.backend.all_endpoint_tools(); for endpoint_tool in endpoint_tools { - if scope_config.granular && !scope_config.is_allowed("endpoint", &endpoint_tool.name) { + // Datatable tools are opt-in via the dedicated `mcp:datatables:` scope, + // independent of the generic endpoint/favorites/all scopes. + if let Some(access) = crate::common::scope::datatable_access_level(&endpoint_tool.name) + { + if !scope_config.datatable_tool_allowed(access) { + continue; + } + } else if scope_config.granular + && !scope_config.is_allowed("endpoint", &endpoint_tool.name) + { continue; } if read_only && !crate::server::is_endpoint_read_only(&endpoint_tool) { @@ -276,8 +285,21 @@ impl ServerHandler for Runner { let endpoint_tools = self.backend.all_endpoint_tools(); for endpoint_tool in &endpoint_tools { if endpoint_tool.name.as_ref() == request.name { - // Validate endpoint scope - if scope_config.granular + // Validate endpoint scope. Datatable tools are gated by the + // dedicated datatable scope, not the generic endpoint scope. + if let Some(access) = + crate::common::scope::datatable_access_level(&endpoint_tool.name) + { + if !scope_config.datatable_tool_allowed(access) { + return Err(ErrorData::internal_error( + format!( + "Access denied: datatable {} access is not in this token's scope (tool '{}')", + access, endpoint_tool.name + ), + None, + )); + } + } else if scope_config.granular && !scope_config.is_allowed("endpoint", &endpoint_tool.name) { return Err(ErrorData::internal_error( diff --git a/frontend/src/lib/components/mcp/McpScopeSelector.svelte b/frontend/src/lib/components/mcp/McpScopeSelector.svelte index d3f7d00dd7..9f2aebfb04 100644 --- a/frontend/src/lib/components/mcp/McpScopeSelector.svelte +++ b/frontend/src/lib/components/mcp/McpScopeSelector.svelte @@ -14,6 +14,7 @@ WorkspaceService } from '$lib/gen' import { mcpEndpointTools } from '$lib/mcpEndpointTools' + import Toggle from '../Toggle.svelte' import InfoIcon from 'lucide-svelte/icons/info' import { SvelteMap } from 'svelte/reactivity' @@ -26,10 +27,24 @@ let { workspaceId, scope = $bindable(), initialScope, readOnly = false }: Props = $props() + // Datatable tools are controlled by the dedicated "Data tables" section, not + // the generic API-endpoint picker, so they're excluded from that list. + const DATATABLE_TOOL_NAMES = new Set([ + 'listDataTables', + 'listDataTableTables', + 'listDataTableSchemas', + 'getDataTableTableSchema', + 'queryDataTable', + 'insertDataTable', + 'updateDataTable' + ]) + // Endpoints we can actually advertise to a read-only MCP token. Mirrors the - // runner's filter (only GET endpoints). + // runner's filter (only GET endpoints) and drops the datatable tools. const visibleEndpointTools = $derived( - readOnly ? mcpEndpointTools.filter((e) => e.method === 'GET') : mcpEndpointTools + (readOnly ? mcpEndpointTools.filter((e) => e.method === 'GET') : mcpEndpointTools).filter( + (e) => !DATATABLE_TOOL_NAMES.has(e.name) + ) ) // When read-only flips on, prune already-selected non-GET endpoints so the @@ -54,6 +69,8 @@ let selectedFlows = $state(parsedInitial.flows) let selectedEndpoints = $state(parsedInitial.endpoints) let selectedDatatables = $state(parsedInitial.datatables) + let datatablesEnabled = $state(parsedInitial.datatablesEnabled) + let datatablesWrite = $state(parsedInitial.datatablesWrite) let allDatatables = $state([]) let loadingDatatables = $state(false) let customScriptPatterns = $state(parsedInitial.scriptPatterns) @@ -84,6 +101,8 @@ flows: string[] endpoints: string[] datatables: string[] + datatablesEnabled: boolean + datatablesWrite: boolean scriptPatterns: string flowPatterns: string hubApps: string[] @@ -97,6 +116,8 @@ flows: [], endpoints: [], datatables: [], + datatablesEnabled: false, + datatablesWrite: false, scriptPatterns: '', flowPatterns: '', hubApps: [] @@ -109,6 +130,9 @@ const byKind: Record = {} let mode: ParsedScope['mode'] = 'custom' const hubApps: string[] = [] + let datatablesEnabled = false + let datatablesWrite = false + let datatableNames: string[] = [] for (const part of parts) { if (part === 'mcp:favorites') { @@ -124,9 +148,19 @@ } else if (part.startsWith('mcp:endpoints:')) { byKind.endpoints = parsePatterns(part.slice('mcp:endpoints:'.length)) } else if (part.startsWith('mcp:datatables:')) { - byKind.datatables = parsePatterns(part.slice('mcp:datatables:'.length)) + datatablesEnabled = true + let rest = part.slice('mcp:datatables:'.length) + if (rest.startsWith('write:')) { + datatablesWrite = true + rest = rest.slice('write:'.length) + } else if (rest.startsWith('read:')) { + rest = rest.slice('read:'.length) + } + // `*` means "all data tables" — an empty explicit selection. + datatableNames = parsePatterns(rest).filter((n) => n !== '*') } } + const dt = { datatables: datatableNames, datatablesEnabled, datatablesWrite } // Detect folder mode: scripts and flows are exclusively `f/X/*` patterns // for the same set of folders, and endpoints is exactly `*`. @@ -149,7 +183,7 @@ scripts: [], flows: [], endpoints: [], - datatables: [], + ...dt, scriptPatterns: '', flowPatterns: '', hubApps @@ -157,7 +191,7 @@ } if (mode === 'favorites' || mode === 'all') { - return { ...empty, mode, hubApps } + return { ...empty, mode, hubApps, ...dt } } // Custom mode: split each list into "selectable" entries (later filtered @@ -171,7 +205,7 @@ scripts: [], flows: [], endpoints: byKind.endpoints ?? [], - datatables: byKind.datatables ?? [], + ...dt, scriptPatterns: (byKind.scripts ?? []).join(','), flowPatterns: (byKind.flows ?? []).join(','), hubApps @@ -202,12 +236,6 @@ if (selectedEndpoints.length > 0) { scopeParts.push(`mcp:endpoints:${selectedEndpoints.join(',')}`) } - - // Restrict which data tables the datatable tools may touch. Omitting - // this leaves datatable access unrestricted (all data tables). - if (selectedDatatables.length > 0) { - scopeParts.push(`mcp:datatables:${selectedDatatables.join(',')}`) - } } else if (selectedMode === 'folder') { const folderPaths = selectedFolders.map((f) => `f/${f}/*`).join(',') if (selectedFolders.length > 0) { @@ -221,6 +249,14 @@ scopeParts.push(`mcp:hub:${newMcpApps.join(',')}`) } + // Data table access is opt-in and independent of the mode. In "all" mode + // `mcp:all` already grants full datatable access, so no separate scope. + if (datatablesEnabled && selectedMode !== 'all') { + const level = datatablesWrite && !readOnly ? 'write' : 'read' + const names = selectedDatatables.length > 0 ? selectedDatatables.join(',') : '*' + scopeParts.push(`mcp:datatables:${level}:${names}`) + } + scope = scopeParts.join(' ') }) @@ -416,13 +452,21 @@ } } - // Load data table names for the custom-mode restriction picker + // Load data table names once the Data tables section is enabled. $effect(() => { - if (selectedMode === 'custom' && workspaceId) { + if (datatablesEnabled && workspaceId) { loadDatatables(workspaceId) } }) + // A read-only token can never expose the write tools, so keep the write + // sub-toggle off while read-only is on. + $effect(() => { + if (readOnly && datatablesWrite) { + datatablesWrite = false + } + }) + // One-shot: once allScripts/allFlows are loaded, split the // initial pattern text into known paths (selectedScripts/Flows) vs // remaining wildcards/unknowns (kept in pattern textbox). @@ -479,12 +523,6 @@ function clearAllEndpoints() { selectedEndpoints = [] } - function selectAllDatatables() { - selectedDatatables = [...allDatatables] - } - function clearAllDatatables() { - selectedDatatables = [] - }
@@ -550,6 +588,53 @@ {/if}
+ {#if selectedMode !== 'all'} +
+ + {#if datatablesEnabled} +

+ This grants the MCP client full access to the data in the selected data tables (all + of them if none is selected). Data tables have no per-user or row-level permissions yet + (coming soon), so only enable this for trusted or + development use. +

+ + {#if !readOnly} + + {/if} + + Restrict to specific data tables + {#if loadingDatatables} +

Loading data tables...

+ {:else if allDatatables.length > 0} + + {:else} +

No data tables configured in this workspace.

+ {/if} + {/if} +
+ {/if} + {#if selectedMode === 'custom'} {#if loadingRunnables}
@@ -605,28 +690,9 @@ />
- {#if allDatatables.length > 0 || loadingDatatables} -
- {@render sectionHeader('Data tables', selectAllDatatables, clearAllDatatables)} - {#if loadingDatatables} -

Loading data tables...

- {:else} - -

- Applies only to the datatable tools (query/insert/update/list). Leave empty to allow - every data table. -

- {/if} -
- {/if} -
Selected: {selectedScripts.length} scripts, {selectedFlows.length} flows, {selectedEndpoints.length} - endpoints{#if selectedDatatables.length > 0}, {selectedDatatables.length} data tables{/if} + endpoints