From 25e5f3a6e8fb0435dde559607146a2b417a79c63 Mon Sep 17 00:00:00 2001 From: Cliff Hall Date: Sat, 6 Jun 2026 21:47:20 -0400 Subject: [PATCH] fix(everything): prevent URL elicitation error-path infinite loop (#4285) * url elicitation * single tool for normal and error path * Address review feedback on URL elicitation tool - Remove src/everything/pnpm-lock.yaml (monorepo uses npm workspaces) - Drop redundant TriggerUrlElicitationSchema.parse(args) in the handler; destructure directly from args like the other tools, since the SDK validates against the registered schema before invoking the handler - Add explicit accept/decline/cancel messaging in the request path, mirroring trigger-elicitation-request.ts - Clarify the registerTool count comment in registrations.test.ts (task-based tools register via registerToolTask, counted separately) - Add tests covering the no-url / undefined-capability registration guards, the randomUUID() elicitationId fallback, and the decline/cancel response paths (100% coverage of the tool) Co-Authored-By: Claude Opus 4.8 (1M context) * Add tool annotations to trigger-url-elicitation Match the annotations block on sibling elicitation tools and the CLAUDE.md guidance. Uses openWorldHint: true since the tool drives an external browser flow. Co-Authored-By: Claude Opus 4.8 (1M context) * fix(everything): prevent URL elicitation error-path infinite loop The trigger-url-elicitation tool's error path (errorPath=true) threw UrlElicitationRequiredError (-32042) carrying the same URL that failed and re-threw it on every retry, so clients looped (and tripped their own loop detection). - Point the prerequisite elicitation at a different URL (https://modelcontextprotocol.io) than the failing request. - Track issued prerequisites by the inputs a client resends on retry (session + url + caller-supplied elicitationId); a plain retry is then recognized, ignores errorPath, and proceeds via the request path instead of re-throwing the prerequisite. - Update tests and docs accordingly. Co-Authored-By: Claude Opus 4.8 (1M context) * fix(everything): address review on URL elicitation error path - Replace raw NUL byte delimiters in errorPathKey with the \u0000 escape sequence so the source stays plain text and the diff is reviewable (same collision-proof delimiter at runtime). - Add a one-shot test reset helper + beforeEach so the suite is robust to the module-level marker state regardless of order. - Use a dedicated prerequisite message instead of reusing the caller's. - Document the unbounded-growth tradeoff as a demo simplification and note the one-shot marker semantics in docs/features.md. Co-Authored-By: Claude Opus 4.8 (1M context) --------- Co-authored-by: evalstate <1936278+evalstate@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 (1M context) --- src/everything/__tests__/tools.test.ts | 90 ++++++++++++++++--- src/everything/docs/features.md | 2 +- src/everything/docs/structure.md | 2 +- .../tools/trigger-url-elicitation.ts | 84 +++++++++++++++-- 4 files changed, 158 insertions(+), 20 deletions(-) diff --git a/src/everything/__tests__/tools.test.ts b/src/everything/__tests__/tools.test.ts index 0a457f3e5b..eaa8f721b6 100644 --- a/src/everything/__tests__/tools.test.ts +++ b/src/everything/__tests__/tools.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect, vi } from 'vitest'; +import { describe, it, expect, vi, beforeEach } from 'vitest'; import { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js'; import { registerEchoTool, EchoSchema } from '../tools/echo.js'; import { registerGetSumTool } from '../tools/get-sum.js'; @@ -13,7 +13,10 @@ import { registerToggleSimulatedLoggingTool } from '../tools/toggle-simulated-lo import { registerToggleSubscriberUpdatesTool } from '../tools/toggle-subscriber-updates.js'; import { registerTriggerSamplingRequestTool } from '../tools/trigger-sampling-request.js'; import { registerTriggerElicitationRequestTool } from '../tools/trigger-elicitation-request.js'; -import { registerTriggerUrlElicitationTool } from '../tools/trigger-url-elicitation.js'; +import { + registerTriggerUrlElicitationTool, + __resetIssuedErrorPathElicitations, +} from '../tools/trigger-url-elicitation.js'; import { registerGetRootsListTool } from '../tools/get-roots-list.js'; import { registerGZipFileAsResourceTool } from '../tools/gzip-file-as-resource.js'; @@ -708,6 +711,12 @@ describe('Tools', () => { }); describe('trigger-url-elicitation', () => { + // The error-path marker is module-level state shared across cases; reset it + // so tests are independent of order and of each other's leftover keys. + beforeEach(() => { + __resetIssuedErrorPathElicitations(); + }); + it('should not register when client does not support URL elicitation', () => { const handlers: Map = new Map(); const mockServer = { @@ -913,7 +922,7 @@ describe('Tools', () => { ); }); - it('should throw MCP error -32042 with required URL elicitation data when errorPath is true', async () => { + it('should throw MCP error -32042 with a prerequisite elicitation pointing at a different URL when errorPath is true', async () => { const handlers: Map = new Map(); const mockServer = { registerTool: vi.fn((name: string, config: any, handler: Function) => { @@ -928,7 +937,7 @@ describe('Tools', () => { const handler = handlers.get('trigger-url-elicitation')!; - expect.assertions(2); + expect.assertions(5); try { await handler( @@ -942,13 +951,74 @@ describe('Tools', () => { ); } catch (error: any) { expect(error.code).toBe(-32042); - expect(error.data.elicitations[0]).toEqual({ - mode: 'url', - url: 'https://example.com/connect', - message: 'Authorization is required to continue.', - elicitationId: 'elicitation-xyz', - }); + const prerequisite = error.data.elicitations[0]; + expect(prerequisite.mode).toBe('url'); + // The prerequisite must NOT reuse the failing URL, otherwise the client + // would complete it, retry, hit the same error, and loop forever. + expect(prerequisite.url).toBe('https://modelcontextprotocol.io'); + expect(prerequisite.url).not.toBe('https://example.com/connect'); + // It carries its own elicitation id for the prerequisite itself. + expect(typeof prerequisite.elicitationId).toBe('string'); + } + }); + + it('should ignore errorPath and take the request path when the same call is retried after the prerequisite', async () => { + const handlers: Map = new Map(); + const mockSendRequest = vi.fn().mockResolvedValue({ action: 'accept' }); + + const mockServer = { + registerTool: vi.fn((name: string, config: any, handler: Function) => { + handlers.set(name, handler); + }), + server: { + getClientCapabilities: vi.fn(() => ({ elicitation: { url: {} } })), + }, + } as unknown as McpServer; + + registerTriggerUrlElicitationTool(mockServer); + + const handler = handlers.get('trigger-url-elicitation')!; + // A real client retries with the *same* arguments and does not echo the + // prerequisite's elicitationId. Note these args omit elicitationId, so the + // correlation must rely on stable inputs (session + url), not a per-call + // random id. + const args = { + url: 'https://example.com/connect', + message: 'Authorization is required to continue.', + errorPath: true, + }; + const extra = { sessionId: 'session-1', sendRequest: mockSendRequest }; + + // First call: error path issues the prerequisite and throws -32042. + let prerequisiteUrl: string | undefined; + try { + await handler(args, extra); + throw new Error('expected first call to throw'); + } catch (error: any) { + expect(error.code).toBe(-32042); + prerequisiteUrl = error.data.elicitations[0].url; + expect(prerequisiteUrl).toBe('https://modelcontextprotocol.io'); + expect(mockSendRequest).not.toHaveBeenCalled(); } + + // Plain retry with identical arguments: errorPath is ignored and the call + // proceeds via the request path instead of throwing the prerequisite again. + const result = await handler({ ...args }, extra); + + expect(mockSendRequest).toHaveBeenCalledWith( + expect.objectContaining({ + method: 'elicitation/create', + params: expect.objectContaining({ + mode: 'url', + url: 'https://example.com/connect', + }), + }), + expect.anything(), + expect.anything() + ); + expect(result.content[0].text).toContain( + '✅ User completed the URL elicitation flow.' + ); }); }); diff --git a/src/everything/docs/features.md b/src/everything/docs/features.md index 3fc6f34a23..a5429ac600 100644 --- a/src/everything/docs/features.md +++ b/src/everything/docs/features.md @@ -23,7 +23,7 @@ - `toggle-simulated-logging` (tools/toggle-simulated-logging.ts): Starts or stops simulated, random‑leveled logging for the invoking session. Respects the client’s selected minimum logging level. - `toggle-subscriber-updates` (tools/toggle-subscriber-updates.ts): Starts or stops simulated resource update notifications for URIs the invoking session has subscribed to. - `trigger-elicitation-request` (tools/trigger-elicitation-request.ts): Issues an `elicitation/create` request using form-mode fields (strings, numbers, booleans, enums, and format validation) and returns the resulting action/content. -- `trigger-url-elicitation` (tools/trigger-url-elicitation.ts): Issues an `elicitation/create` request in URL mode (`mode: "url"`) with an `elicitationId`, or throws MCP error `-32042` (`UrlElicitationRequiredError`) when `errorPath=true`. Requires client capability `elicitation.url`. +- `trigger-url-elicitation` (tools/trigger-url-elicitation.ts): Issues an `elicitation/create` request in URL mode (`mode: "url"`) with an `elicitationId`, or throws MCP error `-32042` (`UrlElicitationRequiredError`) when `errorPath=true`. On the error path the prerequisite elicitation it returns points at a different URL than the failing request (`https://modelcontextprotocol.io`); when the client satisfies it and retries the same call, the retry ignores `errorPath` and proceeds via the request path, so the client does not loop on the same error. The retry marker is one-shot per `(session, url, elicitationId)`: it is cleared on the recognized retry, so re-running the error path with identical arguments without an intervening prerequisite is treated as a retry and proceeds. Requires client capability `elicitation.url`. - `trigger-sampling-request` (tools/trigger-sampling-request.ts): Issues a `sampling/createMessage` request to the client/LLM using provided `prompt` and optional generation controls; returns the LLM's response payload. - `simulate-research-query` (tools/simulate-research-query.ts): Demonstrates MCP Tasks (SEP-1686) with a simulated multi-stage research operation. Accepts `topic` and `ambiguous` parameters. Returns a task that progresses through stages with status updates. If `ambiguous` is true and client supports elicitation, sends an elicitation request directly to gather clarification before completing. - `trigger-sampling-request-async` (tools/trigger-sampling-request-async.ts): Demonstrates bidirectional tasks where the server sends a sampling request that the client executes as a background task. Server polls for status and retrieves the LLM result when complete. Requires client to support `tasks.requests.sampling.createMessage`. diff --git a/src/everything/docs/structure.md b/src/everything/docs/structure.md index f6089cd31e..bd3d70b95c 100644 --- a/src/everything/docs/structure.md +++ b/src/everything/docs/structure.md @@ -156,7 +156,7 @@ src/everything - `trigger-elicitation-request.ts` - Registers a `trigger-elicitation-request` tool that sends an `elicitation/create` request to the client/LLM and returns the elicitation result. - `trigger-url-elicitation.ts` - - Registers a `trigger-url-elicitation` tool that either sends an out-of-band URL-mode `elicitation/create` request (`mode: "url"`) including an `elicitationId` (request path) or throws `UrlElicitationRequiredError` (`-32042`) for client-handled URL elicitation (error path). + - Registers a `trigger-url-elicitation` tool that either sends an out-of-band URL-mode `elicitation/create` request (`mode: "url"`) including an `elicitationId` (request path) or throws `UrlElicitationRequiredError` (`-32042`) for client-handled URL elicitation (error path). On the error path the carried prerequisite elicitation points at a different URL than the failing one (`https://modelcontextprotocol.io`), and when the client satisfies it and retries the same call, the retry ignores `errorPath` and proceeds via the request path — so the client does not loop on the same error. - `trigger-elicitation-request-async.ts` - Registers a `trigger-elicitation-request-async` tool that demonstrates bidirectional MCP tasks for elicitation. Sends an elicitation request with task metadata, then polls the client's `tasks/get` endpoint for completion status before fetching the final result. - `trigger-sampling-request.ts` diff --git a/src/everything/tools/trigger-url-elicitation.ts b/src/everything/tools/trigger-url-elicitation.ts index 0f5d2082b4..a7de878de7 100644 --- a/src/everything/tools/trigger-url-elicitation.ts +++ b/src/everything/tools/trigger-url-elicitation.ts @@ -26,7 +26,9 @@ const TriggerUrlElicitationSchema = z.object({ "Controls which elicitation mechanism is used. " + "When false (default), sends an elicitation/create request (request path). " + "When true, throws a UrlElicitationRequiredError (MCP error code -32042) so the client handles " + - "the URL elicitation via the error path rather than waiting for a response." + "the URL elicitation via the error path rather than waiting for a response. " + + "To clear the error, satisfy the prerequisite and retry this call with the same arguments; the " + + "retry ignores errorPath and proceeds, so the client does not loop on the same error." ), }); @@ -48,6 +50,32 @@ const config = { }, }; +/** + * Tracks requests for which an error-path prerequisite has already been issued, + * keyed by the stable inputs a client resends when it retries the original tool + * call (session + URL + caller-supplied elicitationId). + * + * When the client satisfies the prerequisite and retries the same call, the + * matching entry lets us recognize the retry, ignore `errorPath`, and proceed + * via the request path instead of re-throwing `UrlElicitationRequiredError` — + * which would otherwise loop forever (throw -> client satisfies prerequisite -> + * retry -> throw -> ...). + * + * Demo simplification: entries are only removed on a recognized retry, so a + * client that triggers the error path and never retries leaves its key behind. + * That is acceptable for this reference server; a production implementation + * serving many long-lived sessions should evict entries (e.g. a + * `Map` with TTL-based cleanup). + */ +const issuedErrorPathElicitations = new Set(); + +/** + * Test-only helper to reset the module-level error-path state between cases. + * Not part of the tool's public behavior. + */ +export const __resetIssuedErrorPathElicitations = () => + issuedErrorPathElicitations.clear(); + /** * Registers the 'trigger-url-elicitation' tool. * @@ -56,8 +84,11 @@ const config = { * * Depending on the `errorPath` argument it either: * - Sends an `elicitation/create` request and awaits the result (request path), or - * - Throws a `UrlElicitationRequiredError` (MCP error -32042) for the client to - * handle via the error path. + * - Throws a `UrlElicitationRequiredError` (MCP error -32042) carrying a + * prerequisite elicitation for the client to handle (error path). When the + * client satisfies the prerequisite and retries the same call, the retry + * ignores `errorPath` and proceeds via the request path, so the client does + * not loop on the same error. * * @param {McpServer} server - The McpServer instance where the tool will be registered. */ @@ -85,6 +116,16 @@ export const registerTriggerUrlElicitationTool = (server: McpServer) => { } = args; const elicitationId = requestedElicitationId ?? randomUUID(); + const sessionId = extra.sessionId ?? "default"; + + // Key the one-shot error-path marker on inputs the client resends + // verbatim when it retries the original tool call. A real client retries + // with the *same* arguments and does NOT echo the prerequisite's + // (server-generated) elicitationId, so we must key on stable inputs: + // the session, the requested URL, and the caller-supplied elicitationId + // (if any). Keying on the resolved/random elicitationId would change on + // every call and never match, re-throwing the prerequisite forever. + const errorPathKey = `${sessionId}\u0000${url}\u0000${requestedElicitationId ?? ""}`; const elicitationParams: ElicitRequestURLParams = { mode: "url", @@ -93,12 +134,39 @@ export const registerTriggerUrlElicitationTool = (server: McpServer) => { elicitationId, }; - // Error path: throw UrlElicitationRequiredError (-32042) for the client to handle + // Error path: signal the client via UrlElicitationRequiredError (-32042) + // so it handles a prerequisite URL elicitation before this request can + // proceed. Two things keep the client from looping forever: + // + // 1. The prerequisite points at a *different* URL than the one that + // failed. Reusing the original `url` would make the client complete + // the prerequisite, retry, and hit the same -32042 error endlessly. + // 2. We remember that we issued a prerequisite for this request. When + // the client satisfies it and retries the same call, we recognize + // the retry, *ignore* errorPath, and fall through to the request + // path. Without this, the retry would re-enter the error path and + // re-request the prerequisite URL — another loop. if (errorPath) { - throw new UrlElicitationRequiredError( - [elicitationParams], - "This request requires browser-based authorization." - ); + if (issuedErrorPathElicitations.has(errorPathKey)) { + // Retry of a satisfied prerequisite: clear the one-shot marker and + // ignore errorPath, falling through to the request path below. + issuedErrorPathElicitations.delete(errorPathKey); + } else { + // Originating call: record that we issued a prerequisite for this + // request, then signal the client via -32042. + issuedErrorPathElicitations.add(errorPathKey); + const prerequisiteElicitation: ElicitRequestURLParams = { + mode: "url", + url: "https://modelcontextprotocol.io", + message: + "Open this link to satisfy the prerequisite, then retry the request.", + elicitationId: randomUUID(), + }; + throw new UrlElicitationRequiredError( + [prerequisiteElicitation], + "This request requires browser-based authorization." + ); + } } // Request path: send elicitation/create and await the user's response