diff --git a/api-extractor/reports/mongodb-mcp-server.public.api.md b/api-extractor/reports/mongodb-mcp-server.public.api.md index b80cb204e..fc0335e87 100644 --- a/api-extractor/reports/mongodb-mcp-server.public.api.md +++ b/api-extractor/reports/mongodb-mcp-server.public.api.md @@ -260,6 +260,14 @@ export class ConfigOverrideError extends Error { constructor(message: string); } +// @public +export type ConfirmationResult = { + ok: true; +} | { + ok: false; + reason: "declined" | "no-elicitation-support"; +}; + // @public (undocumented) export type ConnectionErrorHandled = { errorHandled: true; @@ -515,11 +523,20 @@ export class Elicitation { }; required: string[]; }; - requestConfirmation(message: string): Promise; + requestConfirmation(message: string): Promise; requestInput(message: string, schema: ElicitRequestFormParams["requestedSchema"]): Promise; supportsElicitation(): boolean; } +// @public (undocumented) +export type ElicitedInputResult = { + accepted: true; + fields: Record; +} | { + accepted: false; + fields?: undefined; +}; + // @public (undocumented) export enum ErrorCodes { // (undocumented) diff --git a/api-extractor/reports/tools.public.api.md b/api-extractor/reports/tools.public.api.md index 00bf56744..e079ef56e 100644 --- a/api-extractor/reports/tools.public.api.md +++ b/api-extractor/reports/tools.public.api.md @@ -1565,7 +1565,7 @@ export abstract class ToolBase; // (undocumented) protected verifyAllowed(): boolean; - verifyConfirmed(args: ToolArgs): Promise; + verifyConfirmed(args: ToolArgs): Promise; } // @public diff --git a/api-extractor/reports/web.public.api.md b/api-extractor/reports/web.public.api.md index 683f9e800..e91a0dd2d 100644 --- a/api-extractor/reports/web.public.api.md +++ b/api-extractor/reports/web.public.api.md @@ -295,6 +295,14 @@ export class CompositeLogger extends LoggerBase { protected readonly type?: LoggerType; } +// @public +export type ConfirmationResult = { + ok: true; +} | { + ok: false; + reason: "declined" | "no-elicitation-support"; +}; + // @public (undocumented) export type ConnectionErrorHandled = { errorHandled: true; @@ -506,7 +514,7 @@ export class Elicitation { }; required: string[]; }; - requestConfirmation(message: string): Promise; + requestConfirmation(message: string): Promise; requestInput(message: string, schema: ElicitRequestFormParams["requestedSchema"]): Promise; supportsElicitation(): boolean; } @@ -970,7 +978,7 @@ export abstract class ToolBase; // (undocumented) protected verifyAllowed(): boolean; - verifyConfirmed(args: ToolArgs): Promise; + verifyConfirmed(args: ToolArgs): Promise; } // @public diff --git a/src/common/logging/loggingDefinitions.ts b/src/common/logging/loggingDefinitions.ts index e3fcf76d8..d816b24e0 100644 --- a/src/common/logging/loggingDefinitions.ts +++ b/src/common/logging/loggingDefinitions.ts @@ -35,6 +35,7 @@ export const LogId = { toolExecuteFailure: mongoLogId(1_003_002), toolDisabled: mongoLogId(1_003_003), toolMetadataChange: mongoLogId(1_003_004), + toolBlockedNoElicitation: mongoLogId(1_003_005), mongodbConnectFailure: mongoLogId(1_004_001), mongodbDisconnectFailure: mongoLogId(1_004_002), diff --git a/src/elicitation.ts b/src/elicitation.ts index 13cb423ab..d94829b86 100644 --- a/src/elicitation.ts +++ b/src/elicitation.ts @@ -5,6 +5,25 @@ export type ElicitedInputResult = | { accepted: true; fields: Record } | { accepted: false; fields?: undefined }; +/** + * Outcome of a confirmation elicitation. + * + * `ok: true` means the user explicitly confirmed and the caller may + * proceed. Both `ok: false` reasons mean the caller MUST refuse the + * operation; they are kept distinct so the caller can produce a + * different, actionable error message for each — a user who declined + * sees a different message than a client that can't show a prompt at + * all. + * + * This replaces a previous `Promise` shape that conflated the + * two failure modes and returned `true` (proceed) when the client did + * not advertise elicitation support. See OWASP MCP Top 10 (2025) item + * MCP06 — Intent Flow Subversion — for the rationale. + */ +export type ConfirmationResult = + | { ok: true } + | { ok: false; reason: "declined" | "no-elicitation-support" }; + const ELICITATION_TIMEOUT_MS = 300_000; // 5 minutes for user interaction export class Elicitation { @@ -24,12 +43,21 @@ export class Elicitation { /** * Requests a boolean confirmation from the user. + * + * Fails closed: if the connected client does not advertise the + * `elicitation` capability there is no way to obtain explicit + * consent, so the caller MUST refuse to proceed + * (`{ ok: false, reason: "no-elicitation-support" }`). The previous + * behaviour returned `true` in that case, which let + * confirmation-gated tools execute without any user prompt against + * clients that don't support elicitation — a silent bypass of + * `confirmationRequiredTools`. See OWASP MCP06. + * * @param message - The message to display to the user. - * @returns True if the user confirms the action or the client does not support elicitation, false otherwise. */ - public async requestConfirmation(message: string): Promise { + public async requestConfirmation(message: string): Promise { if (!this.supportsElicitation()) { - return true; + return { ok: false, reason: "no-elicitation-support" }; } const result = await this.server.elicitInput( @@ -40,7 +68,10 @@ export class Elicitation { }, { timeout: ELICITATION_TIMEOUT_MS } ); - return result.action === "accept" && result.content?.confirmation === "Yes"; + if (result.action === "accept" && result.content?.confirmation === "Yes") { + return { ok: true }; + } + return { ok: false, reason: "declined" }; } /** diff --git a/src/lib.ts b/src/lib.ts index 5d970ad2b..de6d5d52a 100644 --- a/src/lib.ts +++ b/src/lib.ts @@ -88,7 +88,7 @@ export type { TelemetryEvents, TelemetryConfig } from "./telemetry/telemetry.js" export { EventCache } from "./telemetry/eventCache.js"; export { Keychain, registerGlobalSecretToRedact } from "./common/keychain.js"; export type { Secret } from "./common/keychain.js"; -export { Elicitation } from "./elicitation.js"; +export { Elicitation, type ConfirmationResult, type ElicitedInputResult } from "./elicitation.js"; export { applyConfigOverrides, ConfigOverrideError } from "./common/config/configOverrides.js"; export { SessionStore, diff --git a/src/tools/tool.ts b/src/tools/tool.ts index 15f56750e..38c7aed4b 100644 --- a/src/tools/tool.ts +++ b/src/tools/tool.ts @@ -7,7 +7,7 @@ import type { Telemetry } from "../telemetry/telemetry.js"; import type { ConnectionMetadata, TelemetryToolMetadata, ToolEvent } from "../telemetry/types.js"; import type { UserConfig } from "../common/config/userConfig.js"; import type { Server } from "../server.js"; -import type { Elicitation } from "../elicitation.js"; +import type { ConfirmationResult, Elicitation } from "../elicitation.js"; import type { PreviewFeature } from "../common/schemas.js"; import type { UIRegistry } from "../ui/registry/index.js"; import { createUIResource, type UIResource } from "@mcp-ui/server"; @@ -489,7 +489,31 @@ export abstract class ToolBase< try { if (this.requiresConfirmation()) { - if (!(await this.verifyConfirmed(args))) { + const confirmation = await this.verifyConfirmed(args); + if (!confirmation.ok) { + if (confirmation.reason === "no-elicitation-support") { + // OWASP MCP06: refuse the confirmation-gated tool when + // the client cannot show a prompt. The user never had + // the chance to consent, so silently proceeding (the + // previous behaviour) would bypass + // `confirmationRequiredTools`. + this.session.logger.warning({ + id: LogId.toolBlockedNoElicitation, + context: "tool", + message: `Refused to execute the \`${this.name}\` tool: the connected MCP client does not advertise the elicitation capability, so user confirmation cannot be obtained.`, + noRedaction: true, + }); + return { + content: [ + { + type: "text", + text: `Refused to execute \`${this.name}\`: this tool requires explicit user confirmation, and the connected MCP client does not support the elicitation capability. Use a client that supports elicitation, or remove \`${this.name}\` from the \`confirmationRequiredTools\` configuration.`, + }, + ], + isError: true, + }; + } + this.session.logger.debug({ id: LogId.toolExecute, context: "tool", @@ -598,17 +622,17 @@ export abstract class ToolBase< * Check if the user has confirmed the tool execution (if required by * configuration). * - * This method automatically checks if the tool name is in the - * `confirmationRequiredTools` configuration list and requests user - * confirmation via the elicitation service if needed. + * Returns a typed result so the caller can distinguish a user-level + * decline ("user clicked no") from an environment-level inability + * to obtain consent ("client doesn't support elicitation"). Both + * block execution, but the two cases want different error messages + * and different audit-log severities. See OWASP MCP06. * * @param args - The tool arguments - * @returns A promise resolving to `true` if confirmed or confirmation not - * required, `false` otherwise */ - public async verifyConfirmed(args: ToolArgs): Promise { + public async verifyConfirmed(args: ToolArgs): Promise { if (!this.requiresConfirmation()) { - return true; + return { ok: true }; } return this.elicitation.requestConfirmation(this.getConfirmationMessage(args)); diff --git a/src/web.ts b/src/web.ts index 3a84f2ac2..2578d1efd 100644 --- a/src/web.ts +++ b/src/web.ts @@ -35,7 +35,7 @@ export type { ConnectionErrorUnhandled, ConnectionErrorHandlerContext, } from "./common/connectionErrorHandler.js"; -export { Elicitation, type ElicitedInputResult } from "./elicitation.js"; +export { Elicitation, type ConfirmationResult, type ElicitedInputResult } from "./elicitation.js"; export type { ConnectionStateConnecting, ConnectionSettings, diff --git a/tests/unit/elicitation.test.ts b/tests/unit/elicitation.test.ts index b13ffee5b..c62904f83 100644 --- a/tests/unit/elicitation.test.ts +++ b/tests/unit/elicitation.test.ts @@ -60,23 +60,23 @@ describe("Elicitation", () => { describe("requestConfirmation", () => { const testMessage = "Are you sure you want to proceed?"; - it("should return true when client does not support elicitation", async () => { + it("fails closed with reason 'no-elicitation-support' when client cannot elicit (OWASP MCP06)", async () => { mockGetClientCapabilities.mockReturnValue({}); const result = await elicitation.requestConfirmation(testMessage); - expect(result).toBe(true); + expect(result).toEqual({ ok: false, reason: "no-elicitation-support" }); expect(mockGetClientCapabilities).toHaveBeenCalledTimes(1); expect(mockElicitInput.mock).not.toHaveBeenCalled(); }); - it("should return true when user confirms with 'Yes' and action is 'accept'", async () => { + it("returns ok=true when user confirms with 'Yes' and action is 'accept'", async () => { mockGetClientCapabilities.mockReturnValue({ elicitation: {} }); mockElicitInput.confirmYes(); const result = await elicitation.requestConfirmation(testMessage); - expect(result).toBe(true); + expect(result).toEqual({ ok: true }); expect(mockGetClientCapabilities).toHaveBeenCalledTimes(1); expect(mockElicitInput.mock).toHaveBeenCalledTimes(1); expect(mockElicitInput.mock).toHaveBeenCalledWith( @@ -89,47 +89,47 @@ describe("Elicitation", () => { ); }); - it("should return false when user selects 'No' with action 'accept'", async () => { + it("returns ok=false with reason 'declined' when user selects 'No'", async () => { mockGetClientCapabilities.mockReturnValue({ elicitation: {} }); mockElicitInput.confirmNo(); const result = await elicitation.requestConfirmation(testMessage); - expect(result).toBe(false); + expect(result).toEqual({ ok: false, reason: "declined" }); expect(mockElicitInput.mock).toHaveBeenCalledTimes(1); }); - it("should return false when content is undefined", async () => { + it("returns ok=false with reason 'declined' when content is undefined", async () => { mockGetClientCapabilities.mockReturnValue({ elicitation: {} }); mockElicitInput.acceptWith(undefined); const result = await elicitation.requestConfirmation(testMessage); - expect(result).toBe(false); + expect(result).toEqual({ ok: false, reason: "declined" }); expect(mockElicitInput.mock).toHaveBeenCalledTimes(1); }); - it("should return false when confirmation field is missing", async () => { + it("returns ok=false with reason 'declined' when confirmation field is missing", async () => { mockGetClientCapabilities.mockReturnValue({ elicitation: {} }); mockElicitInput.acceptWith({}); const result = await elicitation.requestConfirmation(testMessage); - expect(result).toBe(false); + expect(result).toEqual({ ok: false, reason: "declined" }); expect(mockElicitInput.mock).toHaveBeenCalledTimes(1); }); - it("should return false when user cancels", async () => { + it("returns ok=false with reason 'declined' when user cancels", async () => { mockGetClientCapabilities.mockReturnValue({ elicitation: {} }); mockElicitInput.cancel(); const result = await elicitation.requestConfirmation(testMessage); - expect(result).toBe(false); + expect(result).toEqual({ ok: false, reason: "declined" }); expect(mockElicitInput.mock).toHaveBeenCalledTimes(1); }); - it("should handle elicitInput erroring", async () => { + it("propagates elicitInput errors", async () => { mockGetClientCapabilities.mockReturnValue({ elicitation: {} }); const error = new Error("Elicitation failed"); mockElicitInput.rejectWith(error); diff --git a/tests/unit/toolBase.test.ts b/tests/unit/toolBase.test.ts index 9f319e6f8..12af85c39 100644 --- a/tests/unit/toolBase.test.ts +++ b/tests/unit/toolBase.test.ts @@ -26,7 +26,9 @@ describe("ToolBase", () => { let mockConfig: UserConfig; let mockTelemetry: Telemetry; let mockElicitation: Elicitation; - let mockRequestConfirmation: MockedFunction<(message: string) => Promise>; + let mockRequestConfirmation: MockedFunction< + (message: string) => Promise<{ ok: true } | { ok: false; reason: "declined" | "no-elicitation-support" }> + >; let testTool: TestTool; let mockMetrics: MockMetrics; @@ -78,48 +80,59 @@ describe("ToolBase", () => { }); describe("verifyConfirmed", () => { - it("should return true when tool is not in confirmationRequiredTools list", async () => { + it("returns ok=true when tool is not in confirmationRequiredTools list", async () => { mockConfig.confirmationRequiredTools = ["other-tool", "another-tool"]; const args = { param1: "test" }; const result = await testTool.verifyConfirmed(args); - expect(result).toBe(true); + expect(result).toEqual({ ok: true }); expect(mockRequestConfirmation).not.toHaveBeenCalled(); }); - it("should return true when confirmationRequiredTools list is empty", async () => { + it("returns ok=true when confirmationRequiredTools list is empty", async () => { mockConfig.confirmationRequiredTools = []; const args = { param1: "test" }; const result = await testTool.verifyConfirmed(args); - expect(result).toBe(true); + expect(result).toEqual({ ok: true }); expect(mockRequestConfirmation).not.toHaveBeenCalled(); }); - it("should call requestConfirmation when tool is in confirmationRequiredTools list", async () => { + it("calls requestConfirmation when tool is in confirmationRequiredTools list", async () => { mockConfig.confirmationRequiredTools = ["test-tool"]; - mockRequestConfirmation.mockResolvedValue(true); + mockRequestConfirmation.mockResolvedValue({ ok: true }); const args = { param1: "test", param2: 42 }; const result = await testTool.verifyConfirmed(args); - expect(result).toBe(true); + expect(result).toEqual({ ok: true }); expect(mockRequestConfirmation).toHaveBeenCalledTimes(1); expect(mockRequestConfirmation).toHaveBeenCalledWith( "You are about to execute the `test-tool` tool which requires additional confirmation. Would you like to proceed?" ); }); - it("should return false when user declines confirmation", async () => { + it("propagates ok=false reason='declined' when user declines confirmation", async () => { mockConfig.confirmationRequiredTools = ["test-tool"]; - mockRequestConfirmation.mockResolvedValue(false); + mockRequestConfirmation.mockResolvedValue({ ok: false, reason: "declined" }); const args = { param1: "test" }; const result = await testTool.verifyConfirmed(args); - expect(result).toBe(false); + expect(result).toEqual({ ok: false, reason: "declined" }); + expect(mockRequestConfirmation).toHaveBeenCalledTimes(1); + }); + + it("propagates ok=false reason='no-elicitation-support' when client lacks elicitation (MCP06)", async () => { + mockConfig.confirmationRequiredTools = ["test-tool"]; + mockRequestConfirmation.mockResolvedValue({ ok: false, reason: "no-elicitation-support" }); + + const args = { param1: "test" }; + const result = await testTool.verifyConfirmed(args); + + expect(result).toEqual({ ok: false, reason: "no-elicitation-support" }); expect(mockRequestConfirmation).toHaveBeenCalledTimes(1); }); }); diff --git a/tests/unit/toolContext.test.ts b/tests/unit/toolContext.test.ts index ad8a53f18..1b80a806d 100644 --- a/tests/unit/toolContext.test.ts +++ b/tests/unit/toolContext.test.ts @@ -106,7 +106,7 @@ describe("Tool Context", () => { } as unknown as Telemetry; const elicitation = { - requestConfirmation: () => Promise.resolve(true), + requestConfirmation: () => Promise.resolve({ ok: true } as const), } as unknown as Elicitation; const customContext: CustomContext = { @@ -178,7 +178,7 @@ describe("Tool Context", () => { } as unknown as Telemetry; const elicitation = { - requestConfirmation: () => Promise.resolve(true), + requestConfirmation: () => Promise.resolve({ ok: true } as const), } as unknown as Elicitation; const customContext: CustomContext = { diff --git a/tests/unit/tools/atlas/streams/build.test.ts b/tests/unit/tools/atlas/streams/build.test.ts index af1991b89..a31241605 100644 --- a/tests/unit/tools/atlas/streams/build.test.ts +++ b/tests/unit/tools/atlas/streams/build.test.ts @@ -53,7 +53,7 @@ describe("StreamsBuildTool", () => { } as unknown as Telemetry; mockElicitation = { - requestConfirmation: vi.fn().mockResolvedValue(true), + requestConfirmation: vi.fn().mockResolvedValue({ ok: true }), requestInput: vi.fn().mockResolvedValue({ accepted: false }), }; diff --git a/tests/unit/tools/atlas/streams/manage.test.ts b/tests/unit/tools/atlas/streams/manage.test.ts index 112bdad84..478f2e1ae 100644 --- a/tests/unit/tools/atlas/streams/manage.test.ts +++ b/tests/unit/tools/atlas/streams/manage.test.ts @@ -61,7 +61,7 @@ describe("StreamsManageTool", () => { } as unknown as Telemetry; const mockElicitation = { - requestConfirmation: vi.fn().mockResolvedValue(true), + requestConfirmation: vi.fn().mockResolvedValue({ ok: true }), } as unknown as Elicitation; const params: ToolConstructorParams = { diff --git a/tests/unit/tools/atlas/streams/teardown.test.ts b/tests/unit/tools/atlas/streams/teardown.test.ts index 4df461342..e3c68da9c 100644 --- a/tests/unit/tools/atlas/streams/teardown.test.ts +++ b/tests/unit/tools/atlas/streams/teardown.test.ts @@ -55,7 +55,7 @@ describe("StreamsTeardownTool", () => { } as unknown as Telemetry; const mockElicitation = { - requestConfirmation: vi.fn().mockResolvedValue(true), + requestConfirmation: vi.fn().mockResolvedValue({ ok: true }), } as unknown as Elicitation; const params: ToolConstructorParams = {