diff --git a/.changeset/27402-mcp-include-refuses.md b/.changeset/27402-mcp-include-refuses.md new file mode 100644 index 00000000000..50283ef44bc --- /dev/null +++ b/.changeset/27402-mcp-include-refuses.md @@ -0,0 +1,21 @@ +--- +'@objectstack/connector-mcp': minor +--- + +fix(connector-mcp)!: a declarative `mcp` instance refuses a `providerConfig.include` it cannot read, and an entry naming a tool the server does not list, instead of exposing every tool + +Clause-②: no (narrowing) +Metadata-protocol: narrowed + + + +**BREAKING** accept-set narrowing of `providerConfig.include` on `provider: 'mcp'` connector instances (`@objectstack/connector-mcp`). + +- **Before.** An `include` that was not an array (a string such as `include: 'web_search'`, an object, `null`) was dropped and read as "no allowlist": the instance exposed **every** tool the server lists. Non-string entries were filtered out silently (`['web_search', 7]` exposed `web_search`). An entry naming a tool the server does not list was ignored: `include: ['nope']` brought the instance up `ready` with no actions and no error. +- **Now.** `include` keeps the `openapi` provider's meaning: absent means every tool; otherwise it must be an array of non-empty tool names. + - Any other shape is refused before connecting, with `connector-mcp provider: connector 'NAME' providerConfig.include must be an array of tool names (strings): ...`. + - An entry the server's `tools/list` does not carry is refused after listing, naming the entry and the tools the server lists (`... providerConfig.include names 'nope', which the MCP server does not list. ...`); the connection is closed first. + - Both are configuration faults, thrown plain: fatal at boot, a skipped entry on a reload. The instance never falls back to exposing every tool. +- **Who sees it.** An app whose `mcp` connector instance writes `include` as anything but an array of tool names, or lists a tool its server does not (a typo, or a tool the server renamed or dropped). + +**The remedy.** Write `include` as an array of the server's tool names (`include: ['web_search']`), or remove it to expose every tool. For an entry the refusal names, use a name from the tool list the refusal prints, or delete the entry. diff --git a/content/docs/automation/connectors.mdx b/content/docs/automation/connectors.mdx index ca7f97ba0ba..ae19d1dcbe0 100644 --- a/content/docs/automation/connectors.mdx +++ b/content/docs/automation/connectors.mdx @@ -231,6 +231,16 @@ connect, handshake or `tools/list`) is a configuration fault instead: the instance is degraded naming the refusal, as described under *A credential the upstream refuses* below. +`include` has the same allowlist meaning as the `openapi` provider's: list the +tools the instance should expose and every other tool stays out — the way to +keep a destructive tool away from your flows and agents. It must be an array of +tool names; any other shape (a string such as `include: 'web_search'`, an +object) fails the boot before connecting, and never falls back to exposing +every tool. An entry the server's `tools/list` does not carry — a typo, or a +tool the server renamed or dropped — fails the boot with an error naming the +entry and listing the server's tools; it is never ignored. Leave `include` out +to expose every tool. + **Declarative stdio transports are denied by default.** Materializing one spawns a local process *from metadata* — which a runtime Studio publish can diff --git a/packages/connectors/connector-mcp/src/mcp-provider.test.ts b/packages/connectors/connector-mcp/src/mcp-provider.test.ts index a1995174943..0cd06896566 100644 --- a/packages/connectors/connector-mcp/src/mcp-provider.test.ts +++ b/packages/connectors/connector-mcp/src/mcp-provider.test.ts @@ -5,12 +5,15 @@ // fake client here), listing its tools, and mapping them to actions. import { describe, it, expect } from 'vitest'; +import { LiteKernel } from '@objectstack/core'; +import { AutomationServicePlugin, type AutomationEngine } from '@objectstack/service-automation'; import { UnauthorizedError } from '@modelcontextprotocol/sdk/client/auth.js'; import { StreamableHTTPError } from '@modelcontextprotocol/sdk/client/streamableHttp.js'; import { McpError } from '@modelcontextprotocol/sdk/types.js'; import type { ConnectorProviderContext } from '@objectstack/spec/integration'; import { isConnectorCredentialRejected, isConnectorUpstreamUnavailable } from '@objectstack/spec/integration'; import type { McpClientLike, McpToolDescriptor, McpTransport } from './mcp-connector.js'; +import { ConnectorMcpPlugin } from './connector-mcp-plugin.js'; import { createMcpProviderFactory, MCP_PROVIDER_KEY } from './mcp-provider.js'; const TOOLS: McpToolDescriptor[] = [ @@ -364,3 +367,143 @@ describe('mcp provider: declarative stdio working directory (#22423)', () => { expect(seen.transport).toBeUndefined(); }); }); + +// #27402 — `providerConfig.include` is the allowlist that keeps destructive +// tools away from flows and agents, with the `openapi` provider's one meaning +// (#26709, #27387): a shape it cannot read is refused before connecting, an +// entry naming a tool the server does not list is refused after listing, and +// the instance never falls back to exposing every tool. +describe('mcp provider: providerConfig.include refuses loudly (#27402)', () => { + const SERVER_TOOLS: McpToolDescriptor[] = [ + { name: 'web_search', description: 'Search the web' }, + { name: 'delete_everything', description: 'Destructive' }, + ]; + const http = { kind: 'http', url: 'https://mcp.vendor.example/mcp' }; + + /** A fake server listing SERVER_TOOLS; records whether it was connected and closed. */ + function server() { + const state = { connected: false, closed: false }; + const clientFactory = async (): Promise => { + state.connected = true; + return { + listTools: async () => SERVER_TOOLS, + callTool: async () => ({ content: [] }), + close: async () => { state.closed = true; }, + }; + }; + return { clientFactory, state }; + } + + async function refusal(include: unknown) { + const { clientFactory, state } = server(); + const factory = createMcpProviderFactory({ clientFactory }); + const err: unknown = await Promise.resolve( + factory(ctx({ name: 'vendor', providerConfig: { transport: http, include } })), + ).then( + (mat) => { throw new Error(`expected a refusal, got actions ${Object.keys(mat.handlers).join(', ')}`); }, + (e: unknown) => e, + ); + return { err, state }; + } + + /** A configuration fault: plain, neither retryable nor a credential refusal. */ + function expectConfigurationFault(err: unknown) { + expect(err).toBeInstanceOf(Error); + expect(isConnectorUpstreamUnavailable(err)).toBe(false); + expect(isConnectorCredentialRejected(err)).toBe(false); + } + + it.each([ + ['a string', 'web_search'], + ['an array with a non-string entry', ['web_search', 7]], + ['an array with an empty name', ['web_search', '']], + ['an object', { web_search: true }], + ['null', null], + ])('refuses an include that is %s, before connecting', async (_shape, include) => { + const { err, state } = await refusal(include); + expectConfigurationFault(err); + expect((err as Error).message).toMatch( + /^connector-mcp provider: connector 'vendor' providerConfig\.include must be an array of tool names \(strings\)/, + ); + expect(state.connected).toBe(false); + }); + + it('refuses an entry naming a tool the server does not list, naming it and the listed tools, and closes the client', async () => { + const { err, state } = await refusal(['nope']); + expectConfigurationFault(err); + expect((err as Error).message).toMatch( + /^connector-mcp provider: connector 'vendor' providerConfig\.include names 'nope', which the MCP server does not list\./, + ); + expect((err as Error).message).toContain("the server lists 2 tool(s): 'web_search', 'delete_everything'."); + expect(state.closed).toBe(true); + }); + + it('refuses when one entry is listed and another is not: no partial instance', async () => { + const { err } = await refusal(['web_search', 'nope', 'nope']); + expectConfigurationFault(err); + expect((err as Error).message).toContain("providerConfig.include names 'nope', which"); + }); + + it('an include of listed tools exposes exactly those tools', async () => { + const { clientFactory, state } = server(); + const mat = await createMcpProviderFactory({ clientFactory })( + ctx({ name: 'vendor', providerConfig: { transport: http, include: ['web_search'] } }), + ); + expect(Object.keys(mat.handlers)).toEqual(['web_search']); + expect(mat.def.actions?.map((a) => a.key)).toEqual(['web_search']); + expect(state.closed).toBe(false); + }); + + it('control: no include exposes every tool the server lists', async () => { + const { clientFactory } = server(); + const mat = await createMcpProviderFactory({ clientFactory })( + ctx({ name: 'vendor', providerConfig: { transport: http } }), + ); + expect(Object.keys(mat.handlers)).toEqual(['web_search', 'delete_everything']); + }); + + // Through the materializer: a refused include fails the boot naming the + // instance and the fault, and no connector exposing the server's tools is + // ever registered under the declared name. + it('a declarative instance with a string include fails boot, never registering every tool', async () => { + const { clientFactory, state } = server(); + const declared = [ + { + name: 'vendor_mcp', + label: 'Vendor MCP', + type: 'api', + provider: 'mcp', + providerConfig: { transport: http, include: 'web_search' }, + }, + ]; + const kernel = new LiteKernel({ logger: { level: 'silent' } } as never); + kernel.use(new AutomationServicePlugin()); + kernel.use({ + name: 'test.connector-metadata', + type: 'standard', + version: '1.0.0', + dependencies: ['com.objectstack.service-automation'], + async init(c: { registerService(name: string, service: unknown): void }) { + c.registerService('objectql', { + registry: { listItems: (type: string) => (type === 'connector' ? declared : []) }, + }); + }, + async start() {}, + } as never); + kernel.use(new ConnectorMcpPlugin({ clientFactory })); + try { + const booted: unknown = await kernel.bootstrap().then(() => undefined, (e: unknown) => e); + expect(booted).toBeInstanceOf(Error); + expect((booted as Error).message).toContain( + "failed to materialize connector instance 'vendor_mcp' via provider 'mcp'", + ); + expect((booted as Error).message).toContain('providerConfig.include must be an array of tool names'); + expect(state.connected).toBe(false); + const engine = kernel.getService('automation') as AutomationEngine | undefined; + const descriptor = engine?.getConnectorDescriptors().find((d) => d.name === 'vendor_mcp'); + expect(descriptor?.actions ?? []).toEqual([]); + } finally { + await kernel.shutdown().catch(() => {}); + } + }); +}); diff --git a/packages/connectors/connector-mcp/src/mcp-provider.ts b/packages/connectors/connector-mcp/src/mcp-provider.ts index 999154b7fd3..ae66f3def90 100644 --- a/packages/connectors/connector-mcp/src/mcp-provider.ts +++ b/packages/connectors/connector-mcp/src/mcp-provider.ts @@ -208,6 +208,13 @@ async function credentialRejectionStatus(err: unknown): Promise { @@ -222,10 +229,33 @@ export function createMcpProviderFactory(deps: McpProviderDeps = {}): ConnectorP transport = { ...transport, cwd: await ctx.resolvePackagePath('.') }; } } - const includeList = Array.isArray(cfg.include) - ? cfg.include.filter((x): x is string => typeof x === 'string') + // `include` keeps the `openapi` provider's meaning (one allowlist, two + // providers): absent ⇒ every tool; otherwise an array of non-empty tool + // names. Any other shape is a configuration fault, thrown plain before + // connecting — fatal at boot, a skipped entry on reload. An allowlist that + // dropped a shape it could not read would expose every tool the server + // has, the very ones the author meant to keep out of flows and agents. + if ( + cfg.include !== undefined && + (!Array.isArray(cfg.include) || + cfg.include.some((entry) => typeof entry !== 'string' || entry.length === 0)) + ) { + throw new Error( + `connector-mcp provider: connector '${ctx.name}' providerConfig.include must be an array of ` + + `tool names (strings): each the name of a tool the MCP server lists in tools/list.`, + ); + } + const includeList = cfg.include as readonly string[] | undefined; + // The allowlist rides the connector's own `include` predicate; every tool + // name the server lists is recorded on the way, so an entry nothing + // matched can be named in the refusal below. + const listedTools: string[] = []; + const include = includeList + ? (toolName: string) => { + listedTools.push(toolName); + return includeList.includes(toolName); + } : undefined; - const include = includeList ? (toolName: string) => includeList.includes(toolName) : undefined; let bundle; try { @@ -270,6 +300,29 @@ export function createMcpProviderFactory(deps: McpProviderDeps = {}): ConnectorP { cause: err }, ); } + if (includeList) { + // An entry naming a tool the server does not list is a configuration + // fault, thrown plain — fatal at boot, a skipped entry on reload — after + // the connection is released. Ignoring it would bring the instance up + // without the tool the author listed (a typo, or a tool the server + // renamed or dropped), and the first sign would be a flow failing at + // run time. + const offered = new Set(listedTools); + const unknown = [...new Set(includeList)].filter((entry) => !offered.has(entry)); + if (unknown.length > 0) { + await bundle.close().catch(() => {}); + // At most 20 of the server's tool names, as the openapi refusal lists. + const sample = [...offered].slice(0, 20); + const more = offered.size > sample.length ? `, … ${offered.size - sample.length} more` : ''; + throw new Error( + `connector-mcp provider: connector '${ctx.name}' providerConfig.include names ` + + `${unknown.map((entry) => `'${entry}'`).join(', ')}, which the MCP server does not list. ` + + `Each entry must be the name of a tool from the server's tools/list; ` + + `the server lists ${offered.size} tool(s): ` + + `${sample.map((name) => `'${name}'`).join(', ')}${more}.`, + ); + } + } // [#27237] The handshake carried the instance's credential only over // http, and only as a header (an api-key `paramName` is not sent): then // the server's answer proved it.