* feat(app,app-bridge): guard requests sent before handshake completes Calling host-bound methods before the ui/initialize → ui/notifications/initialized handshake completes can race the handshake on strict hosts and leave the iframe permanently hidden. App (View side): - All 8 host-bound methods (callServerTool, readServerResource, listServerResources, sendMessage, updateModelContext, openLink, downloadFile, requestDisplayMode) now check that connect() has sent the initialized notification. - New AppOptions.strict (default false): console.error when false, throw when true. AppOptions is now exported. - useApp() forwards strict to the App constructor. - Flag resets on reconnect. AppBridge (Host side): - replaceRequestHandler is overridden to wrap every host-bound handler with a console.warn if the request arrives before ui/notifications/initialized. Never throws. ui/initialize and ping use setRequestHandler directly and are exempt; notifications are unaffected. - Catches Views that hand-roll postMessage without the SDK. - Flag resets on reconnect. Refs anthropics/claude-ai-mcp#61, anthropics/claude-ai-mcp#149. * fix(docs): drop AppOptions from intentionallyNotExported now that it is exported
This commit is contained in:
Olivier Chafik
GitHub
parent
739c0e049d
commit
94c94216d4
5 changed files with 200 additions and 9 deletions
|
|
@ -1,4 +1,4 @@
|
|||
import { describe, it, expect, beforeEach, afterEach } from "bun:test";
|
||||
import { describe, it, expect, beforeEach, afterEach, spyOn } from "bun:test";
|
||||
import { InMemoryTransport } from "@modelcontextprotocol/sdk/inMemory.js";
|
||||
import type { Client } from "@modelcontextprotocol/sdk/client/index.js";
|
||||
import type { ServerCapabilities } from "@modelcontextprotocol/sdk/types.js";
|
||||
|
|
@ -780,6 +780,102 @@ describe("App <-> AppBridge integration", () => {
|
|||
);
|
||||
});
|
||||
|
||||
describe("pre-handshake guard (claude-ai-mcp#149)", () => {
|
||||
const guardMsg =
|
||||
/called before connect\(\) completed the ui\/initialize handshake/;
|
||||
|
||||
it("callServerTool warns when called before connect() completes", async () => {
|
||||
const errSpy = spyOn(console, "error").mockImplementation(() => {});
|
||||
bridge.oncalltool = async () => ({ content: [] });
|
||||
await bridge.connect(bridgeTransport);
|
||||
|
||||
const connecting = app.connect(appTransport);
|
||||
// Handshake is in flight; _initializedSent is still false.
|
||||
await app.callServerTool({ name: "t", arguments: {} }).catch(() => {});
|
||||
|
||||
expect(errSpy).toHaveBeenCalledTimes(1);
|
||||
expect(errSpy.mock.calls[0][0]).toMatch(guardMsg);
|
||||
expect(errSpy.mock.calls[0][0]).toContain("App.callServerTool()");
|
||||
|
||||
await connecting;
|
||||
errSpy.mockRestore();
|
||||
});
|
||||
|
||||
it("callServerTool does not warn after connect() resolves", async () => {
|
||||
const errSpy = spyOn(console, "error").mockImplementation(() => {});
|
||||
bridge.oncalltool = async () => ({ content: [] });
|
||||
await bridge.connect(bridgeTransport);
|
||||
await app.connect(appTransport);
|
||||
|
||||
await app.callServerTool({ name: "t", arguments: {} });
|
||||
|
||||
expect(errSpy).not.toHaveBeenCalled();
|
||||
errSpy.mockRestore();
|
||||
});
|
||||
|
||||
it("sendMessage and readServerResource also warn before handshake", async () => {
|
||||
const errSpy = spyOn(console, "error").mockImplementation(() => {});
|
||||
|
||||
await app.sendMessage({ role: "user", content: [] }).catch(() => {});
|
||||
await app.readServerResource({ uri: "test://r" }).catch(() => {});
|
||||
|
||||
expect(errSpy).toHaveBeenCalledTimes(2);
|
||||
expect(errSpy.mock.calls[0][0]).toContain("App.sendMessage()");
|
||||
expect(errSpy.mock.calls[1][0]).toContain("App.readServerResource()");
|
||||
errSpy.mockRestore();
|
||||
});
|
||||
|
||||
it("throws instead of warning when strict: true", async () => {
|
||||
const errSpy = spyOn(console, "error").mockImplementation(() => {});
|
||||
const strictApp = new App(
|
||||
testAppInfo,
|
||||
{},
|
||||
{ autoResize: false, strict: true },
|
||||
);
|
||||
|
||||
await expect(
|
||||
strictApp.callServerTool({ name: "t", arguments: {} }),
|
||||
).rejects.toThrow(guardMsg);
|
||||
expect(errSpy).not.toHaveBeenCalled();
|
||||
|
||||
errSpy.mockRestore();
|
||||
});
|
||||
|
||||
it("AppBridge warns on tools/call from a View that skipped the handshake", async () => {
|
||||
const warnSpy = spyOn(console, "warn").mockImplementation(() => {});
|
||||
bridge.oncalltool = async () => ({ content: [] });
|
||||
await bridge.connect(bridgeTransport);
|
||||
|
||||
// Simulate a hand-rolled View (no SDK, no handshake) sending tools/call.
|
||||
await appTransport.start();
|
||||
appTransport.send({
|
||||
jsonrpc: "2.0",
|
||||
id: 1,
|
||||
method: "tools/call",
|
||||
params: { name: "t", arguments: {} },
|
||||
});
|
||||
await flush();
|
||||
|
||||
expect(warnSpy).toHaveBeenCalledTimes(1);
|
||||
expect(warnSpy.mock.calls[0][0]).toContain(
|
||||
"received 'tools/call' before ui/notifications/initialized",
|
||||
);
|
||||
warnSpy.mockRestore();
|
||||
});
|
||||
|
||||
it("AppBridge does not warn after initialized is received", async () => {
|
||||
const warnSpy = spyOn(console, "warn").mockImplementation(() => {});
|
||||
bridge.oncalltool = async () => ({ content: [] });
|
||||
await bridge.connect(bridgeTransport);
|
||||
await app.connect(appTransport);
|
||||
|
||||
await app.callServerTool({ name: "t", arguments: {} });
|
||||
|
||||
expect(warnSpy).not.toHaveBeenCalled();
|
||||
warnSpy.mockRestore();
|
||||
});
|
||||
});
|
||||
|
||||
it("onlistresources setter registers handler for resources/list requests", async () => {
|
||||
const requestParams = {};
|
||||
const resources = [{ uri: "test://resource", name: "Test" }];
|
||||
|
|
|
|||
|
|
@ -39,6 +39,7 @@ import {
|
|||
ToolListChangedNotificationSchema,
|
||||
} from "@modelcontextprotocol/sdk/types.js";
|
||||
import {
|
||||
Protocol,
|
||||
ProtocolOptions,
|
||||
RequestOptions,
|
||||
} from "@modelcontextprotocol/sdk/shared/protocol.js";
|
||||
|
|
@ -307,6 +308,37 @@ export class AppBridge extends ProtocolWithEvents<
|
|||
private _appCapabilities?: McpUiAppCapabilities;
|
||||
private _hostContext: McpUiHostContext = {};
|
||||
private _appInfo?: Implementation;
|
||||
private _initializedReceived = false;
|
||||
|
||||
/**
|
||||
* Wrap every handler registered via `replaceRequestHandler` with a check
|
||||
* that the View has sent `ui/notifications/initialized`. Warns (never
|
||||
* throws) so lenient hosts keep working while still surfacing the
|
||||
* misordering that leaves strict hosts with a permanently hidden iframe.
|
||||
* `ui/initialize` and `ping` use `setRequestHandler` directly and are
|
||||
* intentionally exempt.
|
||||
*
|
||||
* @see {@link https://github.com/anthropics/claude-ai-mcp/issues/149 claude-ai-mcp#149}
|
||||
*/
|
||||
private _baseReplaceRequestHandler = this.replaceRequestHandler;
|
||||
protected override replaceRequestHandler: Protocol<
|
||||
AppRequest,
|
||||
AppNotification,
|
||||
AppResult
|
||||
>["setRequestHandler"] = (schema, handler) => {
|
||||
this._baseReplaceRequestHandler(schema, (request, extra) => {
|
||||
if (!this._initializedReceived) {
|
||||
console.warn(
|
||||
`[ext-apps] AppBridge received '${request.method}' before ` +
|
||||
`ui/notifications/initialized. The View is calling host ` +
|
||||
`methods before completing the handshake; it should await ` +
|
||||
`app.connect() first. See ` +
|
||||
`https://github.com/anthropics/claude-ai-mcp/issues/149`,
|
||||
);
|
||||
}
|
||||
return handler(request, extra);
|
||||
});
|
||||
};
|
||||
|
||||
protected readonly eventSchemas = {
|
||||
sizechange: McpUiSizeChangedNotificationSchema,
|
||||
|
|
@ -357,6 +389,10 @@ export class AppBridge extends ProtocolWithEvents<
|
|||
) {
|
||||
super(options);
|
||||
|
||||
this.addEventListener("initialized", () => {
|
||||
this._initializedReceived = true;
|
||||
});
|
||||
|
||||
this._hostContext = options?.hostContext || {};
|
||||
|
||||
this.setRequestHandler(McpUiInitializeRequestSchema, (request) =>
|
||||
|
|
@ -1758,6 +1794,7 @@ export class AppBridge extends ProtocolWithEvents<
|
|||
"AppBridge is already connected. Call close() before connecting again.",
|
||||
);
|
||||
}
|
||||
this._initializedReceived = false;
|
||||
if (this._client) {
|
||||
// When a client was passed to the constructor, automatically forward
|
||||
// MCP requests/notifications between the view and the server
|
||||
|
|
|
|||
51
src/app.ts
51
src/app.ts
|
|
@ -144,7 +144,7 @@ export const RESOURCE_MIME_TYPE = "text/html;profile=mcp-app";
|
|||
*
|
||||
* @see `ProtocolOptions` from @modelcontextprotocol/sdk for inherited options
|
||||
*/
|
||||
type AppOptions = ProtocolOptions & {
|
||||
export type AppOptions = ProtocolOptions & {
|
||||
/**
|
||||
* Automatically report size changes to the host using `ResizeObserver`.
|
||||
*
|
||||
|
|
@ -155,6 +155,19 @@ type AppOptions = ProtocolOptions & {
|
|||
* @default true
|
||||
*/
|
||||
autoResize?: boolean;
|
||||
/**
|
||||
* Throw on detected misuse instead of logging a console error.
|
||||
*
|
||||
* Currently this affects calling host-bound methods (e.g.
|
||||
* {@link App.callServerTool `callServerTool`}, {@link App.sendMessage `sendMessage`})
|
||||
* before {@link App.connect `connect`} has completed the `ui/initialize`
|
||||
* handshake. With `strict: false` (default) a `console.error` is emitted;
|
||||
* with `strict: true` an `Error` is thrown.
|
||||
*
|
||||
* @remarks Throwing will become the default in a future release.
|
||||
* @default false
|
||||
*/
|
||||
strict?: boolean;
|
||||
};
|
||||
|
||||
type RequestHandlerExtra = Parameters<
|
||||
|
|
@ -244,6 +257,32 @@ export class App extends ProtocolWithEvents<
|
|||
private _hostCapabilities?: McpUiHostCapabilities;
|
||||
private _hostInfo?: Implementation;
|
||||
private _hostContext?: McpUiHostContext;
|
||||
private _initializedSent = false;
|
||||
|
||||
/**
|
||||
* Warn if a host-bound method is called before {@link connect `connect`} has
|
||||
* completed the `ui/initialize` → `ui/notifications/initialized` handshake.
|
||||
*
|
||||
* Calling these methods early can race the handshake on strict hosts and
|
||||
* leave the iframe permanently hidden. See
|
||||
* {@link https://github.com/anthropics/claude-ai-mcp/issues/61 claude-ai-mcp#61} /
|
||||
* {@link https://github.com/anthropics/claude-ai-mcp/issues/149 #149}.
|
||||
*
|
||||
* @remarks This will become a thrown `Error` in a future minor release.
|
||||
*/
|
||||
private _assertInitialized(method: string): void {
|
||||
if (this._initializedSent) return;
|
||||
const msg =
|
||||
`[ext-apps] App.${method}() called before connect() completed the ` +
|
||||
`ui/initialize handshake. Await app.connect() before calling this ` +
|
||||
`method, or move data loading to an ontoolresult handler. ` +
|
||||
`See https://github.com/anthropics/claude-ai-mcp/issues/149`;
|
||||
if (this.options?.strict) {
|
||||
throw new Error(msg);
|
||||
}
|
||||
// TODO(next-minor): make `strict: true` the default.
|
||||
console.error(`${msg}. This will throw in a future release.`);
|
||||
}
|
||||
|
||||
protected readonly eventSchemas = {
|
||||
toolinput: McpUiToolInputNotificationSchema,
|
||||
|
|
@ -851,6 +890,7 @@ export class App extends ProtocolWithEvents<
|
|||
params: CallToolRequest["params"],
|
||||
options?: RequestOptions,
|
||||
): Promise<CallToolResult> {
|
||||
this._assertInitialized("callServerTool");
|
||||
if (typeof params === "string") {
|
||||
throw new Error(
|
||||
`callServerTool() expects an object as its first argument, but received a string ("${params}"). ` +
|
||||
|
|
@ -915,6 +955,7 @@ export class App extends ProtocolWithEvents<
|
|||
params: ReadResourceRequest["params"],
|
||||
options?: RequestOptions,
|
||||
): Promise<ReadResourceResult> {
|
||||
this._assertInitialized("readServerResource");
|
||||
return await this.request(
|
||||
{ method: "resources/read", params },
|
||||
ReadResourceResultSchema,
|
||||
|
|
@ -962,6 +1003,7 @@ export class App extends ProtocolWithEvents<
|
|||
params?: ListResourcesRequest["params"],
|
||||
options?: RequestOptions,
|
||||
): Promise<ListResourcesResult> {
|
||||
this._assertInitialized("listServerResources");
|
||||
return await this.request(
|
||||
{ method: "resources/list", params },
|
||||
ListResourcesResultSchema,
|
||||
|
|
@ -1020,6 +1062,7 @@ export class App extends ProtocolWithEvents<
|
|||
* @see {@link McpUiMessageRequest `McpUiMessageRequest`} for request structure
|
||||
*/
|
||||
sendMessage(params: McpUiMessageRequest["params"], options?: RequestOptions) {
|
||||
this._assertInitialized("sendMessage");
|
||||
return this.request(
|
||||
<McpUiMessageRequest>{
|
||||
method: "ui/message",
|
||||
|
|
@ -1113,6 +1156,7 @@ export class App extends ProtocolWithEvents<
|
|||
params: McpUiUpdateModelContextRequest["params"],
|
||||
options?: RequestOptions,
|
||||
) {
|
||||
this._assertInitialized("updateModelContext");
|
||||
return this.request(
|
||||
<McpUiUpdateModelContextRequest>{
|
||||
method: "ui/update-model-context",
|
||||
|
|
@ -1149,6 +1193,7 @@ export class App extends ProtocolWithEvents<
|
|||
* @see {@link McpUiOpenLinkResult `McpUiOpenLinkResult`} for result structure
|
||||
*/
|
||||
openLink(params: McpUiOpenLinkRequest["params"], options?: RequestOptions) {
|
||||
this._assertInitialized("openLink");
|
||||
return this.request(
|
||||
<McpUiOpenLinkRequest>{
|
||||
method: "ui/open-link",
|
||||
|
|
@ -1229,6 +1274,7 @@ export class App extends ProtocolWithEvents<
|
|||
params: McpUiDownloadFileRequest["params"],
|
||||
options?: RequestOptions,
|
||||
) {
|
||||
this._assertInitialized("downloadFile");
|
||||
return this.request(
|
||||
<McpUiDownloadFileRequest>{
|
||||
method: "ui/download-file",
|
||||
|
|
@ -1313,6 +1359,7 @@ export class App extends ProtocolWithEvents<
|
|||
params: McpUiRequestDisplayModeRequest["params"],
|
||||
options?: RequestOptions,
|
||||
) {
|
||||
this._assertInitialized("requestDisplayMode");
|
||||
return this.request(
|
||||
<McpUiRequestDisplayModeRequest>{
|
||||
method: "ui/request-display-mode",
|
||||
|
|
@ -1475,6 +1522,7 @@ export class App extends ProtocolWithEvents<
|
|||
"App is already connected. Call close() before connecting again.",
|
||||
);
|
||||
}
|
||||
this._initializedSent = false;
|
||||
await super.connect(transport);
|
||||
|
||||
try {
|
||||
|
|
@ -1502,6 +1550,7 @@ export class App extends ProtocolWithEvents<
|
|||
await this.notification(<McpUiInitializedNotification>{
|
||||
method: "ui/notifications/initialized",
|
||||
});
|
||||
this._initializedSent = true;
|
||||
|
||||
if (this.options?.autoResize) {
|
||||
this.setupSizeChangedNotifications();
|
||||
|
|
|
|||
|
|
@ -1,20 +1,25 @@
|
|||
import { useEffect, useState } from "react";
|
||||
import { Implementation } from "@modelcontextprotocol/sdk/types.js";
|
||||
import { Client } from "@modelcontextprotocol/sdk/client";
|
||||
import { App, McpUiAppCapabilities, PostMessageTransport } from "../app";
|
||||
import {
|
||||
App,
|
||||
AppOptions,
|
||||
McpUiAppCapabilities,
|
||||
PostMessageTransport,
|
||||
} from "../app";
|
||||
export * from "../app";
|
||||
|
||||
/**
|
||||
* Options for configuring the {@link useApp `useApp`} hook.
|
||||
*
|
||||
* Note: This interface does NOT expose {@link App `App`} options like `autoResize`.
|
||||
* The hook creates the `App` with default options (`autoResize: true`). If you
|
||||
* need custom `App` options, create the `App` manually instead of using this hook.
|
||||
* The `strict` option is forwarded to the underlying {@link App `App`}
|
||||
* instance. For other {@link AppOptions `AppOptions`}, create the `App`
|
||||
* manually instead of using this hook.
|
||||
*
|
||||
* @see {@link useApp `useApp`} for the hook that uses these options
|
||||
* @see {@link useAutoResize `useAutoResize`} for manual auto-resize control with custom `App` options
|
||||
*/
|
||||
export interface UseAppOptions {
|
||||
export interface UseAppOptions extends Pick<AppOptions, "strict"> {
|
||||
/** App identification (name and version) */
|
||||
appInfo: Implementation;
|
||||
/**
|
||||
|
|
@ -121,6 +126,7 @@ export function useApp({
|
|||
appInfo,
|
||||
capabilities,
|
||||
onAppCreated,
|
||||
strict,
|
||||
}: UseAppOptions): AppState {
|
||||
const [app, setApp] = useState<App | null>(null);
|
||||
const [isConnected, setIsConnected] = useState(false);
|
||||
|
|
@ -135,7 +141,10 @@ export function useApp({
|
|||
window.parent,
|
||||
window.parent,
|
||||
);
|
||||
const app = new App(appInfo, capabilities);
|
||||
const app = new App(appInfo, capabilities, {
|
||||
autoResize: true,
|
||||
strict,
|
||||
});
|
||||
|
||||
// Register handlers BEFORE connecting
|
||||
onAppCreated?.(app);
|
||||
|
|
|
|||
|
|
@ -28,7 +28,7 @@ const config = {
|
|||
],
|
||||
excludePrivate: true,
|
||||
excludeInternal: false,
|
||||
intentionallyNotExported: ["AppOptions", "MethodSchema"],
|
||||
intentionallyNotExported: ["MethodSchema"],
|
||||
blockTags: [...OptionDefaults.blockTags, "@description"],
|
||||
jsDocCompatibility: {
|
||||
exampleTag: false,
|
||||
|
|
|
|||
Loading…
Reference in a new issue