Conversation
dmandar
requested review from
ditman,
gspencergoog,
jacobsimionato,
nan-yu,
yjbanov and
zeroasterisk
as code owners
- Adds McpAppsCustomComponent using the official @modelcontextprotocol/ext-apps SDK. - Implements double-iframe sandbox isolation (sandbox.html, sandbox.ts). - Upgrades the backend MCP server (floor_plan_server.py) to a persistent SSE architecture using Starlette. - Configures agent.py to connect via mcp.client.sse. - Refines Vite configuration for seamless dual-origin local development.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request integrates MCP Apps into A2UI by adding a new McpAppsCustomComponent, a double-iframe sandbox for security, and a persistent SSE backend. The changes are significant and well-structured. My review focuses on improving security and configuration management. I've identified critical security issues related to postMessage usage that should be addressed. Additionally, there are opportunities to improve maintainability by removing hardcoded URLs and making the code more robust. The repository's style guide requires tests for new code (line 17), which seem to be missing for the new components and server logic. Please consider adding tests to ensure the stability and correctness of these new features.
| el.onclick = () => { | ||
| // Protocol translation to MCP App standard | ||
| window.parent.postMessage({ | ||
| jsonrpc: "2.0", | ||
| id: Date.now(), | ||
| method: "tools/call", | ||
| params: { | ||
| name: "chart_node_click", | ||
| arguments: { | ||
| clickedNodeName: mapping.contactName, | ||
| contactId: mapping.contactId, | ||
| deskId: desk.id, | ||
| source: 'modal' | ||
| } | ||
| } | ||
| }, '*'); | ||
| }; | ||
| } else { | ||
| el.style.borderColor = 'rgba(0,0,0,0.1)'; | ||
| el.style.backgroundColor = 'transparent'; | ||
| el.style.cursor = 'default'; | ||
| el.title = "Empty Desk"; | ||
| } | ||
|
|
||
| container.appendChild(el); | ||
| }); | ||
| } | ||
|
|
||
| document.body.appendChild(tooltip); | ||
|
|
||
| function updateTooltipPos(e) { | ||
| if (tooltip.style.display === 'none') return; | ||
| tooltip.style.left = (e.clientX + 15) + 'px'; | ||
| tooltip.style.top = (e.clientY + 15) + 'px'; | ||
| } | ||
|
|
||
| window.addEventListener('message', (event) => { | ||
| const data = event.data; | ||
| if (data.type === 'zoom') { | ||
| zoom(data.payload.factor); | ||
| } | ||
| }); | ||
|
|
||
| createHotspots(); | ||
|
|
||
| // MCP Handshake | ||
| window.parent.postMessage({ | ||
| jsonrpc: "2.0", | ||
| id: Date.now(), | ||
| method: "ui/initialize", | ||
| params: { | ||
| appCapabilities: {}, | ||
| clientInfo: { name: "Floor Plan App", version: "1.0.0" }, | ||
| protocolVersion: "2026-01-26" | ||
| } | ||
| }, '*'); |
There was a problem hiding this comment.
The postMessage calls on lines 224 and 264 use a wildcard * for the target origin, which is a significant security vulnerability. This allows any website to embed this content and intercept the messages. You should restrict the target origin to the specific, expected parent origin. For example, the parent frame could send its origin in an initial message, which this script could then store and use for all subsequent postMessage calls.
| } | ||
| } else { | ||
| if (inner && inner.contentWindow) { | ||
| inner.contentWindow.postMessage(event.data, "*"); |
There was a problem hiding this comment.
When forwarding messages to the inner iframe, you are using a wildcard * as the target origin. While the inner iframe is same-origin in this setup, it is a security best practice to always specify the exact target origin. You should use OWN_ORIGIN here to ensure the message is only delivered if the inner iframe's origin matches.
| inner.contentWindow.postMessage(event.data, "*"); | |
| inner.contentWindow.postMessage(event.data, OWN_ORIGIN); |
| raise ValueError("No content returned from floor plan server") | ||
|
|
||
| html_content = result.contents[0].text | ||
| except Exception as e: |
There was a problem hiding this comment.
Catching a broad Exception can hide unexpected errors and make debugging more difficult. It's better to catch more specific exceptions that you expect from the network request (e.g., connection errors) and from the logic within the try block (like the ValueError you're raising). This allows for more granular error handling and logging.
| <body> | ||
| <div id="viewport"> | ||
| <div id="container"> | ||
| <img id="floorplan" src="http://localhost:10004/static/floorplan.png" alt="Office Floor Plan"> |
There was a problem hiding this comment.
The image URL http://localhost:10004/static/floorplan.png is hardcoded within the HTML string. This will cause issues when deploying to environments other than local development. This URL should be made configurable, for example by passing it into the HTML template from the Python server, which could in turn read it from an environment variable or configuration file.
| if (typeof value === "string") { | ||
| context.push({ key, value: { literalString: value } }); | ||
| } else if (typeof value === "number") { | ||
| context.push({ key, value: { literalNumber: value } }); | ||
| } else if (typeof value === "boolean") { | ||
| context.push({ key, value: { literalBoolean: value } }); | ||
| } |
There was a problem hiding this comment.
The #dispatchAgentAction method currently only handles primitive types (string, number, boolean) for action parameters. If an action parameter is a complex object or an array, it will be skipped without an error. To make this more robust, you should consider handling these cases, for example by serializing complex values to a JSON string.
if (typeof value === "string") {
context.push({ key, value: { literalString: value } });
} else if (typeof value === "number") {
context.push({ key, value: { literalNumber: value } });
} else if (typeof value === "boolean") {
context.push({ key, value: { literalBoolean: value } });
} else if (value !== null && typeof value === 'object') {
// Handle nested objects/arrays by stringifying them.
context.push({ key, value: { literalString: JSON.stringify(value) } });
}1 participant
Description
Replace this paragraph with a description of what this PR is changing or adding, and why. Consider including before/after screenshots.
List which issues are fixed by this PR. For larger changes, raising an issue first helps reduce redundant work.
Pre-launch Checklist
If you need help, consider asking for advice on the discussion board.