Conversation
### Description Ports the `basic-host` example from `@modelcontextprotocol/ext-apps` to `scripts/mcp-host` to allow developers to test MCP apps locally in a secure sandbox environment. Modified to use `tsx` instead of `bun` for serving. ### Scenarios Tested - Verified that `npm install` works in `scripts/mcp-host`. - Verified that `npm start` starts the host server on port 8080 and sandbox on 8081. ### Sample Commands ```bash cd scripts/mcp-host npm start ```
| sandboxApp.get(["/", "/sandbox.html"], (req, res) => { | ||
| // Parse CSP config from query param: ?csp=<url-encoded-json> | ||
| let cspConfig: McpUiResourceCsp | undefined; | ||
| if (typeof req.query.csp === "string") { | ||
| try { | ||
| cspConfig = JSON.parse(req.query.csp); | ||
| } catch (e) { | ||
| console.warn("[Sandbox] Invalid CSP query param:", e); | ||
| } | ||
| } | ||
|
|
||
| // Set CSP via HTTP header - tamper-proof unlike meta tags | ||
| const cspHeader = buildCspHeader(cspConfig); | ||
| res.setHeader("Content-Security-Policy", cspHeader); | ||
|
|
||
| // Prevent caching to ensure fresh CSP on each load | ||
| res.setHeader("Cache-Control", "no-cache, no-store, must-revalidate"); | ||
| res.setHeader("Pragma", "no-cache"); | ||
| res.setHeader("Expires", "0"); | ||
|
|
||
| res.sendFile(join(DIRECTORY, "sandbox.html")); | ||
| }); |
There was a problem hiding this comment.
Code Review
This pull request introduces a basic host implementation for Model Context Protocol (MCP) applications, featuring a React-based host UI, a sandboxed proxy for secure UI rendering, and a dual-port Express server to ensure origin isolation. The review feedback focuses on improving code robustness and maintainability by adding error handling for environment variable parsing, removing unused properties, and replacing magic numbers with constants. Furthermore, several suggestions emphasize strict adherence to the TypeScript style guide, specifically regarding the avoidance of any types and non-null assertions in favor of explicit null checks and proper interface definitions.
| const SERVERS: string[] = process.env.SERVERS | ||
| ? JSON.parse(process.env.SERVERS) | ||
| : ["http://localhost:3001/mcp"]; |
There was a problem hiding this comment.
The JSON.parse call on process.env.SERVERS will throw an exception if the environment variable is set to an invalid JSON string, causing the script to crash. It's safer to wrap this in a try-catch block and provide a helpful error message.
| const SERVERS: string[] = process.env.SERVERS | |
| ? JSON.parse(process.env.SERVERS) | |
| : ["http://localhost:3001/mcp"]; | |
| let SERVERS: string[] = ["http://localhost:3001/mcp"]; | |
| if (process.env.SERVERS) { | |
| try { | |
| SERVERS = JSON.parse(process.env.SERVERS); | |
| } catch (e) { | |
| console.error("Invalid SERVERS environment variable. Expected a JSON array of strings."); | |
| process.exit(1); | |
| } | |
| } |
| client: Client; | ||
| tools: Map<string, Tool>; | ||
| resources: Map<string, Resource>; | ||
| appHtmlCache: Map<string, string>; |
| log.info("Resource content._meta:", (content as any)._meta); | ||
|
|
||
| // Try both _meta (spec) and meta (Python SDK quirk) for content-level | ||
| const contentMeta = (content as any)._meta || (content as any).meta; | ||
|
|
||
| // Get listing-level metadata as fallback | ||
| const listingResource = serverInfo.resources.get(uri); | ||
| const listingMeta = (listingResource as any)?._meta; | ||
| log.info("Resource listing._meta:", listingMeta); | ||
|
|
||
| // Content-level takes precedence, fall back to listing-level | ||
| const uiMeta = contentMeta?.ui ?? listingMeta?.ui; | ||
| const csp = uiMeta?.csp; | ||
| const permissions = uiMeta?.permissions; |
There was a problem hiding this comment.
Avoid using any as an escape hatch. Defining a proper interface for the metadata improves type safety and adheres to the repository's TypeScript best practices.
| log.info("Resource content._meta:", (content as any)._meta); | |
| // Try both _meta (spec) and meta (Python SDK quirk) for content-level | |
| const contentMeta = (content as any)._meta || (content as any).meta; | |
| // Get listing-level metadata as fallback | |
| const listingResource = serverInfo.resources.get(uri); | |
| const listingMeta = (listingResource as any)?._meta; | |
| log.info("Resource listing._meta:", listingMeta); | |
| // Content-level takes precedence, fall back to listing-level | |
| const uiMeta = contentMeta?.ui ?? listingMeta?.ui; | |
| const csp = uiMeta?.csp; | |
| const permissions = uiMeta?.permissions; | |
| interface McpUiMeta { ui?: { csp?: McpUiResourceCsp; permissions?: McpUiResourcePermissions } } | |
| const contentWithMeta = content as { _meta?: McpUiMeta; meta?: McpUiMeta }; | |
| log.info("Resource content._meta:", contentWithMeta._meta); | |
| // Try both _meta (spec) and meta (Python SDK quirk) for content-level | |
| const contentMeta = contentWithMeta._meta || contentWithMeta.meta; | |
| // Get listing-level metadata as fallback | |
| const listingResource = serverInfo.resources.get(uri); | |
| const listingMeta = (listingResource as { _meta?: McpUiMeta } | undefined)?._meta; | |
| log.info("Resource listing._meta:", listingMeta); | |
| // Content-level takes precedence, fall back to listing-level | |
| const uiMeta = contentMeta?.ui ?? listingMeta?.ui; | |
| const csp = uiMeta?.csp; | |
| const permissions = uiMeta?.permissions; |
References
- Never use any or unknown as an escape hatch. Define proper interfaces/types or use type guards. (link)
| await appBridge.connect( | ||
| new PostMessageTransport(iframe.contentWindow!, iframe.contentWindow!), | ||
| ); |
There was a problem hiding this comment.
Avoid using the non-null assertion operator !. The repository style guide recommends handling null or undefined explicitly. Check if iframe.contentWindow exists before using it.
| await appBridge.connect( | |
| new PostMessageTransport(iframe.contentWindow!, iframe.contentWindow!), | |
| ); | |
| if (!iframe.contentWindow) { | |
| throw new Error("iframe.contentWindow is not available"); | |
| } | |
| await appBridge.connect( | |
| new PostMessageTransport(iframe.contentWindow, iframe.contentWindow), | |
| ); |
References
- Use strict null checks and handle undefined/null explicitly. (link)
| styles: { | ||
| variables: HOST_STYLE_VARIABLES, | ||
| }, | ||
| containerDimensions: options?.containerDimensions ?? { maxHeight: 6000 }, |
| createRoot(document.getElementById("root")!).render( | ||
| <StrictMode> | ||
| <ErrorBoundary> | ||
| <Host serversPromise={connectToAllServers()} /> | ||
| </ErrorBoundary> | ||
| </StrictMode>, | ||
| ); |
There was a problem hiding this comment.
Avoid using the non-null assertion operator !. It's safer to verify that the element exists before calling createRoot.
| createRoot(document.getElementById("root")!).render( | |
| <StrictMode> | |
| <ErrorBoundary> | |
| <Host serversPromise={connectToAllServers()} /> | |
| </ErrorBoundary> | |
| </StrictMode>, | |
| ); | |
| const rootElement = document.getElementById("root"); | |
| if (!rootElement) { | |
| throw new Error("Root element not found"); | |
| } | |
| createRoot(rootElement).render( | |
| <StrictMode> | |
| <ErrorBoundary> | |
| <Host serversPromise={connectToAllServers()} /> | |
| </ErrorBoundary> | |
| </StrictMode>, | |
| ); |
References
- Use strict null checks and handle undefined/null explicitly. (link)
| // This MUST throw a SecurityError -- if `window.top` is accessible, the sandbox | ||
| // configuration is dangerously broken and untrusted content could escape. | ||
| try { | ||
| window.top!.alert("If you see this, the sandbox is not setup securely."); |
There was a problem hiding this comment.
Avoid using the non-null assertion operator !. Even in a security self-test, it's better to follow the project's coding standards regarding explicit null checks.
References
- Use strict null checks and handle undefined/null explicitly. (link)
Description
Ports the
basic-hostexample from@modelcontextprotocol/ext-appstoscripts/mcp-hostto allow developers to test MCP apps locally in a secure sandbox environment. This is useful, since the platforms we generally test on don't yet support MCP apps.Scenarios Tested
npm installworks inscripts/mcp-host.npm startstarts the host server on port 8080 and sandbox on 8081.Sample Commands
cd scripts/mcp-host npm start