Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 80 additions & 0 deletions pr_748_replies.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
# PR #748 Review Responses

Here are proposed first-person responses you can paste into each of the review comment threads on GitHub! I've included the reviewer's exact quotes so you can easily match them up.

### 1. General PR Summary Comment (Security & Tests)
**Reviewer via gemini-code-assist:**
> ![medium]
>
> This pull request integrates MCP Apps into A2UI by adding a new `McpAppsCustomComponent`, a double-iframe sandbox for security, and a persistent SSE backend... 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... The repository's style guide requires tests for new code... Please consider adding tests...

**Reply:**
> Thanks for the thorough review! I've gone ahead and secured all the `postMessage` boundaries across the stack. Specifically, the client sandbox proxy now strictly enforces `EXPECTED_HOST_ORIGIN` validating against the `document.referrer`, and the inner `floor_plan_server` uses a stateful approach to capture and lock to the exact `hostOrigin` from the frontend handshake rather than blindly broadcasting to `*`. I've also parameterized all hardcoded URLs.
>
> As for the tests, we currently do not use an automated UI testing framework for the Python ADK backend samples, but I've manually verified the edge cases and failure modes end-to-end to ensure the connection robustly handles errors and rejected tool calls.

---

### 2. `sandbox.ts` Line (null) - Target Origin
**Reviewer via gemini-code-assist:**
> ![high]
>
> 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.

**Reply:**
> Addressed in the latest commit. I realized this was leaking through the proxy, so I swapped the forwarder destination from `*` to `OWN_ORIGIN`.

---

### 3. `floor_plan_server.py` Line (null) - Target Origin Vulnerability
**Reviewer via gemini-code-assist:**
> ![critical]
>
> 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.

**Reply:**
> Great point. I implemented exactly what you suggested: The inner iframe logic now defaults `hostOrigin` to `*` only until it receives a `sandbox-init` message from the parent proxy. It captures the `event.origin`, permanently saves it as the `hostOrigin`, and strictly uses that for all subsequent outbound MCP tool calls and the initial `ui/initialize` handshake!

---

### 4. `agent.py` Line (null) - Hardcoded SSE URL
**Reviewer via gemini-code-assist:**
> ![medium]
>
> The SSE server URL `http://127.0.0.1:8000/sse` is hardcoded. This makes the agent less flexible and harder to configure for different environments (e.g., development, staging, production). It's recommended to extract this into a configurable variable, for instance, loaded from an environment variable.

**Reply:**
> Good catch! I updated the connection logic to grab `FLOOR_PLAN_SERVER_URL` entirely from the `os.environ`. It defaults securely to the local `http://127.0.0.1:8000/sse` for the out-of-the-box demo experience, but can now easily run in deployed or CI environments without code changes.

---

### 5. `agent.py` Line 260 - Broad Exception Handler
**Reviewer via gemini-code-assist:**
> ![medium]
>
> 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.

**Reply:**
> Done. I've added a specific catch block for `ValueError` alongside the other connection handlers. If the floor plan server responds with invalid or empty data (like a 404), the agent will now catch it explicitly and gracefully yield a UI error message indicating the failure to load the floor plan, rather than swallowing a broader bug.

---

### 6. `floor_plan_server.py` Line (null) - Hardcoded Static URL
**Reviewer via gemini-code-assist:**
> ![medium]
>
> 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.

**Reply:**
> Fixed. I refactored the floor plan HTML payload injection to dynamically inject an `AGENT_STATIC_URL` variable read from the environment. It replaces `__AGENT_STATIC_URL__` in the template strings, entirely decoupling the static asset delivery from the strict local port mapping.

---

### 7. `mcp-apps-component.ts` Line 190 - Complex Action Arguments
**Reviewer via gemini-code-assist:**
> ![medium]
>
> 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.

**Reply:**
> This is a great edge case to protect against. I've updated the dispatcher's type checking logic as you suggested. It now gracefully detects complex objects or arrays and stringifies them into a generic `literalString` payload using `JSON.stringify()`. This ensures the backend `context` resolver can still extract those arguments dynamically without the frontend silently dropping them.
121 changes: 114 additions & 7 deletions samples/agent/adk/contact_multiple_surfaces/agent.py
Original file line number Diff line number Diff line change
Expand Up @@ -241,16 +241,123 @@ async def stream(self, query, session_id) -> AsyncIterable[dict[str, Any]]:
if query.startswith("ACTION:") and "view_location" in query:
logger.info("--- ContactAgent.stream: Detected view_location ACTION ---")

# Use the predefined example floor plan
json_content = load_floor_plan_example().strip()
start_idx = json_content.find("[")
end_idx = json_content.rfind("]")
if start_idx != -1 and end_idx != -1:
json_content = json_content[start_idx : end_idx + 1]
from mcp import ClientSession
from mcp.client.sse import sse_client
import os

sse_url = os.environ.get("FLOOR_PLAN_SERVER_URL", "http://127.0.0.1:8000/sse")

try:
# Connect to the persistent Starlette SSE server
async with sse_client(sse_url) as (read, write):
async with ClientSession(read, write) as mcp_session:
await mcp_session.initialize()

logger.info(
"--- ContactAgent: Fetching ui://floor-plan-server/map from"
" persistent SSE server ---"
)
result = await mcp_session.read_resource("ui://floor-plan-server/map")

if not result.contents or len(result.contents) == 0:
raise ValueError("No content returned from floor plan server")

html_content = result.contents[0].text
except ValueError as e:
logger.error(f"Invalid floor plan data: {e}")
yield {
"is_task_complete": True,
"content": f"Failed to load floor plan data: {str(e)}",
}
return
except Exception as e:
logger.error(f"Failed to fetch floor plan from SSE server: {e}")
yield {
"is_task_complete": True,
"content": f"Failed to connect to floor plan server: {str(e)}",
}
return

json_content = [
{
"beginRendering": {
"surfaceId": "location-surface",
"root": "floor-plan-card",
}
},
{
"surfaceUpdate": {
"surfaceId": "location-surface",
"components": [
{
"id": "floor-plan-card",
"component": {"Card": {"child": "floor-plan-col"}},
},
{
"id": "floor-plan-col",
"component": {
"Column": {
"children": {
"explicitList": [
"floor-plan-title",
"floor-plan-comp",
"dismiss-fp",
]
}
}
},
},
{
"id": "floor-plan-title",
"component": {
"Text": {
"usageHint": "h2",
"text": {"literalString": "Office Floor Plan"},
}
},
},
{
"id": "floor-plan-comp",
"component": {
"McpAppsCustomComponent": {
"htmlContent": html_content,
"height": 400,
"allowedTools": ["chart_node_click"],
}
},
},
{
"id": "dismiss-fp-text",
"component": {
"Text": {"text": {"literalString": "Close Map"}}
},
},
{
"id": "dismiss-fp",
"component": {
"Button": {
"child": "dismiss-fp-text",
"action": {"name": "close_modal", "context": []},
}
},
},
],
}
},
]

logger.info(f"--- ContactAgent.stream: Sending Floor Plan ---")
final_response_content = (
f"Here is the floor plan.\n---a2ui_JSON---\n{json_content}"
f"Here is the floor plan.\n---a2ui_JSON---\n{json.dumps(json_content)}"
)
yield {"is_task_complete": True, "content": final_response_content}
return

if query.startswith("ACTION:") and "close_modal" in query:
logger.info("--- ContactAgent.stream: Handling close_modal ACTION ---")
json_content = [{"deleteSurface": {"surfaceId": "location-surface"}}]
final_response_content = (
f"Modal closed.\n---a2ui_JSON---\n{json.dumps(json_content)}"
)
yield {"is_task_complete": True, "content": final_response_content}
return
Expand Down
Loading
Loading