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) <noreply@anthropic.com> * 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) <noreply@anthropic.com> * 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) <noreply@anthropic.com> * 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) <noreply@anthropic.com> --------- Co-authored-by: evalstate <1936278+evalstate@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
Cliff Hall
GitHub
parent
f5054df1fb
commit
25e5f3a6e8
4 changed files with 158 additions and 20 deletions
|
|
@ -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<string, Function> = 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<string, Function> = 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,14 +951,75 @@ 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<string, Function> = 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.'
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
describe('get-roots-list', () => {
|
||||
|
|
|
|||
|
|
@ -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`.
|
||||
|
|
|
|||
|
|
@ -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`
|
||||
|
|
|
|||
|
|
@ -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<string, timestamp>` with TTL-based cleanup).
|
||||
*/
|
||||
const issuedErrorPathElicitations = new Set<string>();
|
||||
|
||||
/**
|
||||
* 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
|
||||
|
|
|
|||
Loading…
Reference in a new issue