Security hardening: WebSocket Origin validation, SSRF, XSS, plan confirmation bypass, path traversal (v0.20.1) - #252
Merged
Conversation
Two independent gaps in the local WebSocket bridge combined into a full cross-site WebSocket hijacking primitive: any web page the browser loads, unrelated to a running MCP App, could silently connect to ws://localhost:<port>/ws and invoke any tool exposed by the backing MCP server, bypassing the resource's own declared tool permissions. 1. process_request() in host.py never read request.headers, so no Origin check existed anywhere in the handshake path, and ws_serve() was called without an origins= restriction. WebSocket handshakes are not subject to the same-origin policy the way fetch()/XHR are - a browser attaches an Origin header automatically but does not block the connection based on it, so enforcement has to happen server-side. Fixed by validating the Origin header against the http://localhost:<port> (or 127.0.0.1) host page origin before allowing the WebSocket upgrade to proceed. 2. AppInfo.permissions (populated from the resource's own _meta.ui.permissions) was extracted and stored but never read anywhere in bridge.py - the only gate on tools/call was a character-set regex on the tool name, which validates syntax, not authorization. Fixed by enforcing the declared tools allow-list (if any) in _handle_tool_call() before proxying to execute_tool(). A resource that declares no permissions at all keeps today's behavior (unrestricted), so this doesn't break existing apps - it just makes the permissions field, which was previously inert, actually enforce what a resource opts into declaring. Verified against a live AppHostServer + real websockets client: cross-origin and no-Origin connections are now rejected at the handshake (HTTP 403) before a single message can be sent, and same-origin calls to a tool outside the declared scope are rejected by _handle_tool_call with a -32603 error instead of executing. Signed-off-by: chris hay <chris.hay@uk.ibm.com>
mcp_cli.dashboard.server.DashboardServer has the same unauthenticated local WebSocket pattern as the MCP Apps bridge fixed earlier on this branch: _process_request() never read request.headers, and ws_serve() was called with no origins= restriction. Any web page the browser loads while --dashboard is running could attach to /ws and: - read every broadcast: full conversation history, the system prompt in full, and every tool call's name/arguments/result as it happens - inject fake USER_MESSAGE/USER_ACTION content into the chat loop as if the user typed it - send REQUEST_TOOL to execute any tool with any arguments - this path had no validation at all, not even the tool-name character-set regex the apps bridge already had - switch the active model/provider, rewrite the system prompt, or delete/rename saved sessions Extracted the Origin-matching logic from mcp_cli.apps.host into a shared mcp_cli.utils.loopback_origin module and applied it to both local servers, so there's one implementation to keep correct instead of two copies drifting apart. Updated the existing mocked process_request tests and the live websockets-client integration tests to send a matching Origin header, and added tests asserting cross-origin and missing-Origin connections are rejected (HTTP 403) for both servers. Verified against a live DashboardServer + real websockets client: cross-origin and no-Origin connections are rejected at the handshake before a single message reaches on_browser_message; matching-origin connections are unaffected. Signed-off-by: chris hay <chris.hay@uk.ibm.com>
Five independently-verified security gaps found during a broader review of mcp-cli beyond the CSWSH fixes already on this branch: 1. SSRF (src/mcp_cli/apps/host.py): _fetch_http_resource() fetched resource_uri/viewUrl - both controlled by the connected MCP server - with follow_redirects=True and no host/IP validation, letting a malicious server point mcp-cli at internal-only services or cloud metadata endpoints. Added mcp_cli/utils/url_safety.py, which resolves the hostname and rejects private/loopback/link-local/reserved addresses; validated before the initial request and again at every redirect hop. 2. XSS in the dashboard's markdown renderer (dashboard/static/views/agent-terminal.html): marked.parse() output went through a hand-rolled regex "sanitizer" that missed <svg/onload=...> (no leading whitespace before the attribute) and <iframe srcdoc="..."> entirely. Assistant text can be steered via prompt injection from any connected MCP server. Replaced with DOMPurify - verified against both bypasses plus normal markdown output using a real jsdom+DOMPurify run. 3. XSS via unescaped view name/icon (dashboard/static/js/layout.js): server-declared _meta.ui.name/.icon went straight into innerHTML, inconsistent with views.js's use of .textContent for the same values. Now escaped with the existing esc() helper. 4. Planning executor bypassed tool confirmation entirely (planning/backends.py, planning/executor.py): McpToolBackend called ToolManager.execute_tool() directly with no is_trusted_domain/ should_confirm_tool check anywhere in the planning subsystem, unlike the interactive chat path. Reachable via the always-available /plan command or autonomously via the LLM when --plan-tools is enabled (prompt-injectable). Added a confirm_prompt callback wired through PlanRunner from the three real call sites (planning/tools.py, commands/plan/plan.py's run and resume paths); when confirmation is required but no prompt is available, the call is declined rather than silently executed. 5. Path traversal via agent_spawn's agent_id (agents/tools.py, chat/session_store.py, agents/group_store.py): agent_id is built from an LLM-supplied name with only whitespace substitution, unlike session_id a few lines away which strips '/', '\', and '..'. Added a shared _sanitize_path_component() in session_store.py and applied it to both SessionStore's agent-scoped directory and group_store.py's per-agent save directory. Also: capped memory entry content length (DEFAULT_MEMORY_MAX_ENTRY_CHARS) to bound on-disk growth from repeated remember() calls, and moved the attachment size check ahead of the full base64 decode so an oversized payload is rejected from the encoded length alone rather than after allocating the full decoded buffer. Not fixed here: the OAuth `state` CSRF parameter is generated but never validated in the primary authorization-code flow. That code now lives in the external chuk-mcp-client-oauth dependency, not this repository - needs reporting upstream. Full test suite: 4489 passed, 12 skipped. Lint/format clean. mypy shows only the same 3 pre-existing errors present on unmodified main. Signed-off-by: chris hay <chris.hay@uk.ibm.com>
Chasing a test-coverage gap on the two new security modules surfaced a real bug: urlsplit()/.hostname/.port can each raise ValueError on malformed input (a broken IPv6 literal, a non-numeric port), but which one raises is Python-version-dependent - confirmed via this repo's pinned 3.14, where urlsplit() itself raises for bad IPv6 but .port raises separately for a bad port. Both loopback_origin.is_allowed_origin and url_safety.is_safe_fetch_url only wrapped the urlsplit() call, so a crafted Origin header or fetch URL could have raised an uncaught ValueError out of the WebSocket handshake / resource-fetch path instead of cleanly returning False. Widened both try blocks to cover hostname/port extraction too, and added regression tests for the malformed-IPv6 and non-numeric-port cases, plus a few remaining branch-coverage gaps (unresolvable hostname's empty getaddrinfo() result, the trusted-domain bypass path in McpToolBackend._should_confirm when server URL resolution succeeds). url_safety.py and loopback_origin.py are now at 100% coverage. Signed-off-by: chris hay <chris.hay@uk.ibm.com>
…ity fixes) - planning/executor.py: annotate re.Match[str] so replacer() doesn't return Any from a function declared -> str. - commands/cmd/cmd.py: coerce final_response.get(...) with str() since client is typed Any, same no-any-return class of issue. - chat/conversation.py: annotate last_tool_signature: str | None = None. Without it mypy infers the type as bare None from its initial assignment and never sees the later reassignment to a real string, so it flagged the subsequent `and` chain as unreachable dead code - a false positive, not an actual duplicate-detection bug. mypy now reports zero errors (previously 3) across all 167 source files. Full suite still green: 4497 passed, 12 skipped. Signed-off-by: chris hay <chris.hay@uk.ibm.com>
Version bump for the security fixes on this branch (CSWSH + permission bypass in MCP Apps and the dashboard, SSRF in resource fetch, XSS in dashboard markdown rendering and view metadata, tool-confirmation bypass in plan execution, path traversal via agent_spawn). Documents the new controls in README.md's MCP Apps/Dashboard/Plan Execution Security sections and docs/MCP_APPS.md's Security Model, without reproducing exploit specifics. Signed-off-by: chris hay <chris.hay@uk.ibm.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Five independently-verified security fixes, bumping to v0.20.1:
Cross-Site WebSocket Hijacking (CSWSH) in MCP Apps (
src/mcp_cli/apps/host.py) — the local app-host WebSocket server accepted connections from anyOrigin, letting an unrelated webpage attach to a running app's bridge and invoke tools, bypassing the resource's declared tool permissions entirely (AppInfo.permissionswas extracted but never enforced). Fixed with Origin validation on the handshake (mcp_cli/utils/loopback_origin.py) and enforcement of the resource's declaredtoolsallow-list inAppBridge._handle_tool_call().Same CSWSH gap in the dashboard (
src/mcp_cli/dashboard/server.py) — same missing-Origin-validation pattern, larger blast radius: full conversation history / system prompt disclosure via broadcast, arbitrary tool execution viaREQUEST_TOOLwith zero validation (not even a tool-name regex), and prompt injection via fakeUSER_MESSAGE. This matches the independently-filed GHSA-2jhq-9cjp-44gp (CVSS 8.8 High, CWE-346) for the dashboard-specific case. Fixed with the same shared Origin-validation module.SSRF in MCP Apps resource fetch —
_fetch_http_resource()fetchedresource_uri/viewUrl(both controlled by the connected MCP server) withfollow_redirects=Trueand no host/IP validation. Addedmcp_cli/utils/url_safety.py, validated before the initial request and at every redirect hop.XSS in the dashboard's markdown renderer and view metadata — a hand-rolled regex "sanitizer" missed
<svg/onload=...>and<iframe srcdoc="...">; replaced with DOMPurify (verified against both bypasses). Server-declared view name/icon also went unescaped intoinnerHTMLin one call site; now escaped.Planning executor bypassed tool confirmation entirely —
McpToolBackendcalledToolManager.execute_tool()directly with nois_trusted_domain/should_confirm_toolcheck anywhere in the planning subsystem, reachable via the always-available/plancommand or autonomously via the LLM when--plan-toolsis enabled. Added aconfirm_promptgate wired through all three real call sites; fails closed (declines) if confirmation is required but no prompt is available.Also fixed: path traversal via
agent_spawn's unsanitizedagent_id, a memory-entry size cap, an attachment-decode-before-size-check ordering issue, and two Python-version-dependent unhandledValueErrorbugs in the new Origin/URL validators found while improving test coverage (both now at 100%). Plus the 3 pre-existing (unrelated) mypy errors onmain.Not fixed here: the OAuth
stateCSRF parameter is generated but never validated in the primary authorization-code flow — that code now lives in the externalchuk-mcp-client-oauthdependency, not this repository.Test plan
url_safety.py,loopback_origin.py) at 100%ruff check/ruff format --checkclean (aside from pre-existing unrelated files)mypy srcclean (0 errors, down from 3 pre-existing)