diff --git a/README.md b/README.md index 73e0db18..cc2c0a5f 100644 --- a/README.md +++ b/README.md @@ -1327,6 +1327,18 @@ mcp-cli --log-file ~/.mcp-cli/logs/debug.log --server sqlite - **XSS Prevention**: Tool names and user-supplied content are HTML-escaped before template injection - **URL Scheme Validation**: `ui/open-link` only allows `http://` and `https://` schemes - **Tool Name Validation**: Bridge rejects tool names not matching the MCP spec character set +- **WebSocket Origin Validation** (v0.20.1+): The local app-host server rejects any WebSocket handshake whose `Origin` header doesn't match its own `http://localhost:` host page, so an unrelated webpage can't attach to a running app's bridge +- **Tool Permission Enforcement** (v0.20.1+): When a resource declares an allow-list of tools it may call, the bridge enforces it on every `tools/call` โ€” not just a tool-name syntax check +- **SSRF-Safe Resource Fetch** (v0.20.1+): Direct HTTP(S) resource fetches are validated against private/loopback/link-local address ranges before connecting, and re-validated at every redirect hop + +### Dashboard Security (v0.20.1+) +- **WebSocket Origin Validation**: The dashboard's WebSocket server applies the same Origin check as MCP Apps +- **Sanitized Markdown Rendering**: Assistant chat messages are rendered through DOMPurify rather than a hand-rolled sanitizer +- **Escaped View Metadata**: Server-declared view names and icons are HTML-escaped before being inserted into panel headers +- **Sanitized Agent/Session Paths**: Agent and session identifiers are sanitized before use as filesystem path components + +### Plan Execution Security (v0.20.1+) +- **Tool Confirmation Enforced**: Plan execution (`/plan`, `plan_create_and_execute`) honors the same confirm-tools preference and trusted-domain policy as the interactive chat path; if confirmation is required and no prompt is available, the call is declined rather than executed unconfirmed ## ๐Ÿš€ Performance Features diff --git a/docs/MCP_APPS.md b/docs/MCP_APPS.md index 975c3e16..1b22e2a6 100644 --- a/docs/MCP_APPS.md +++ b/docs/MCP_APPS.md @@ -134,6 +134,18 @@ The bridge rejects tool names not matching `^[a-zA-Z0-9_\-./]+$` per the MCP spe `_safe_json_dumps()` falls back to `_to_serializable()` on `TypeError`/`ValueError`, with circular reference protection via a visited-object set. +### WebSocket Origin Validation (v0.20.1+) + +The local app-host server validates the `Origin` header on every WebSocket upgrade request against the `http://localhost:` (or `127.0.0.1`) host page origin, via `mcp_cli.utils.loopback_origin.is_allowed_origin()`. Browsers attach an `Origin` header to WebSocket handshakes automatically but don't enforce same-origin policy on the connection itself โ€” enforcement has to happen server-side. Handshakes with a mismatched or missing Origin are rejected with HTTP 403 before the upgrade completes. + +### Tool Permission Enforcement (v0.20.1+) + +`AppInfo.permissions` (from the resource's `_meta.ui.permissions`) is enforced in `AppBridge._handle_tool_call()`: if a resource declares a `tools` allow-list, only tools on that list can be invoked via the bridge, in addition to the existing tool-name syntax check. A resource that declares no permissions keeps the previous unrestricted behavior. + +### SSRF-Safe Resource Fetch (v0.20.1+) + +Direct HTTP(S) fetches (for `resource_uri` or a tool result's `viewUrl`) are validated with `mcp_cli.utils.url_safety.is_safe_fetch_url()`, which resolves the hostname and rejects private, loopback, link-local, and other non-public address ranges. Redirects are followed manually so each hop is re-validated rather than trusted after the first check. + ## Session Reliability ### Deferred Tool Result Delivery diff --git a/pyproject.toml b/pyproject.toml index 498ca4ef..cafe249c 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "setuptools.build_meta" [project] name = "mcp-cli" -version = "0.20.0" +version = "0.20.1" description = "A cli for the Model Context Provider" requires-python = ">=3.11" readme = "README.md" diff --git a/src/mcp_cli/agents/group_store.py b/src/mcp_cli/agents/group_store.py index 8abcec37..78f55722 100644 --- a/src/mcp_cli/agents/group_store.py +++ b/src/mcp_cli/agents/group_store.py @@ -19,6 +19,8 @@ from pathlib import Path from typing import TYPE_CHECKING, Any +from mcp_cli.chat.session_store import _sanitize_path_component + if TYPE_CHECKING: from mcp_cli.agents.manager import AgentManager @@ -69,9 +71,11 @@ async def save_group( } ) - # Save session if available + # Save session if available. agent_id can originate from an + # LLM-controllable agent_spawn tool call, so sanitize it before + # using it as a path component. ctx = snapshot["context"] - agent_dir = base / agent_id + agent_dir = base / _sanitize_path_component(agent_id) agent_dir.mkdir(parents=True, exist_ok=True) try: history = getattr(ctx, "conversation_history", []) diff --git a/src/mcp_cli/apps/bridge.py b/src/mcp_cli/apps/bridge.py index 12d746ab..f152d22a 100644 --- a/src/mcp_cli/apps/bridge.py +++ b/src/mcp_cli/apps/bridge.py @@ -146,6 +146,24 @@ async def _handle_tool_call(self, msg_id: Any, params: dict[str, Any]) -> str: } ) + if not self._is_tool_permitted(tool_name): + logger.warning( + "App %s attempted to call tool %r outside its declared " + "permission scope", + self.app_info.tool_name, + tool_name, + ) + return json.dumps( + { + "jsonrpc": "2.0", + "id": msg_id, + "error": { + "code": -32603, + "message": f"Tool not permitted: {tool_name!r}", + }, + } + ) + logger.debug( "App %s calling tool %s with %s", self.app_info.tool_name, @@ -210,6 +228,25 @@ async def _handle_tool_call(self, msg_id: Any, params: dict[str, Any]) -> str: } ) + def _is_tool_permitted(self, tool_name: str) -> bool: + """Check *tool_name* against the resource's declared permission scope. + + ``self.app_info.permissions`` comes from the resource's own + ``_meta.ui.permissions`` โ€” a scope the *server* declared, not + something mcp-cli invents. When a resource declares a ``tools`` + allow-list, only those tools may be invoked via this bridge. A + resource that declares no permissions (or a permissions dict with + no ``tools`` key) hasn't opted into scoping, so nothing here + restricts it beyond the existing tool-name syntax check. + """ + permissions = self.app_info.permissions + if not permissions: + return True + allowed_tools = permissions.get("tools") + if not isinstance(allowed_tools, list): + return True + return tool_name in allowed_tools + # ------------------------------------------------------------------ # # Handler: resources/read # # ------------------------------------------------------------------ # diff --git a/src/mcp_cli/apps/host.py b/src/mcp_cli/apps/host.py index c1ff9d8d..41e7d0ff 100644 --- a/src/mcp_cli/apps/host.py +++ b/src/mcp_cli/apps/host.py @@ -38,6 +38,8 @@ DEFAULT_APP_MAX_CONCURRENT, DEFAULT_HTTP_REQUEST_TIMEOUT, ) +from mcp_cli.utils.loopback_origin import is_allowed_origin as _is_allowed_origin +from mcp_cli.utils.url_safety import is_safe_fetch_url if TYPE_CHECKING: from mcp_cli.tools.manager import ToolManager @@ -355,6 +357,27 @@ def process_request( websockets.Headers({"Content-Length": str(len(body))}), body, ) + + # Reject cross-origin WebSocket upgrades. Browsers attach an + # Origin header to WS handshakes but do not enforce same-origin + # policy on them the way they do for fetch()/XHR โ€” enforcement + # is the server's responsibility, so any page that knows (or + # scans for) this port could otherwise attach to the bridge. + origin = request.headers.get("Origin") + if not _is_allowed_origin(origin, app_info.port): + logger.warning( + "Rejected WebSocket connection for app %s: disallowed Origin %r", + app_info.tool_name, + origin, + ) + body = b"Forbidden" + return Response( + http.HTTPStatus.FORBIDDEN, + "Forbidden", + websockets.Headers({"Content-Length": str(len(body))}), + body, + ) + # Return None to proceed with WebSocket upgrade for /ws return None @@ -419,28 +442,51 @@ async def _find_available_port(self) -> int: f"Could not find available port after {max_attempts} attempts" ) + _MAX_REDIRECTS = 5 + @staticmethod async def _fetch_http_resource(url: str) -> tuple[str, dict[str, Any]]: - """Fetch HTML content directly from an HTTP/HTTPS URL.""" + """Fetch HTML content directly from an HTTP/HTTPS URL. + + *url* comes from the connected MCP server (resource_uri or a tool + result's viewUrl), so it's validated against is_safe_fetch_url() + before every request โ€” including each redirect hop, since a + same-origin-looking URL could redirect to an internal address. + """ import httpx + current_url = url async with httpx.AsyncClient( - follow_redirects=True, timeout=DEFAULT_HTTP_REQUEST_TIMEOUT + follow_redirects=False, timeout=DEFAULT_HTTP_REQUEST_TIMEOUT ) as client: - resp = await client.get(url) - resp.raise_for_status() - html = resp.text - # Wrap in a resource-like structure for CSP/permissions extraction - resource = { - "contents": [ - { - "uri": url, - "mimeType": resp.headers.get("content-type", "text/html"), - "text": html, - } - ] - } - return html, resource + for _ in range(AppHostServer._MAX_REDIRECTS + 1): + if not is_safe_fetch_url(current_url): + raise RuntimeError( + f"Refusing to fetch disallowed URL: {current_url}" + ) + resp = await client.get(current_url) + if resp.is_redirect: + location = resp.headers.get("location") + if not location: + resp.raise_for_status() + break + current_url = str(resp.url.join(location)) + continue + resp.raise_for_status() + html = resp.text + # Wrap in a resource-like structure for CSP/permissions extraction + resource = { + "contents": [ + { + "uri": current_url, + "mimeType": resp.headers.get("content-type", "text/html"), + "text": html, + } + ] + } + return html, resource + + raise RuntimeError(f"Too many redirects fetching {url}") @staticmethod def _extract_html(resource: dict[str, Any]) -> str: diff --git a/src/mcp_cli/chat/attachments.py b/src/mcp_cli/chat/attachments.py index eb970ae6..46602671 100644 --- a/src/mcp_cli/chat/attachments.py +++ b/src/mcp_cli/chat/attachments.py @@ -267,6 +267,16 @@ def process_browser_file( ValueError If the file is too large or has an unsupported extension. """ + # Reject grossly oversized payloads from the encoded length alone, + # before decoding the whole thing into memory (base64 inflates size by + # ~4/3, so this is a cheap upper-bound check ahead of the exact one + # below). + if len(data_b64) * 3 // 4 > DEFAULT_MAX_ATTACHMENT_SIZE_BYTES: + raise ValueError( + f"File too large: ~{len(data_b64) * 3 // 4:,} bytes " + f"(max {DEFAULT_MAX_ATTACHMENT_SIZE_BYTES:,})" + ) + raw = base64.b64decode(data_b64) size = len(raw) if size > DEFAULT_MAX_ATTACHMENT_SIZE_BYTES: diff --git a/src/mcp_cli/chat/conversation.py b/src/mcp_cli/chat/conversation.py index ea722eeb..48a04496 100644 --- a/src/mcp_cli/chat/conversation.py +++ b/src/mcp_cli/chat/conversation.py @@ -161,7 +161,8 @@ async def process_conversation(self, max_turns: int = 100): max_turns: Maximum number of conversation turns before forcing exit (default: 100) """ turn_count = 0 - last_tool_signature = None # Track last tool call to detect true duplicates + # Track last tool call to detect true duplicates + last_tool_signature: str | None = None tools_for_completion = None # Will be set based on context after_tool_calls = False # True when resuming after tool execution diff --git a/src/mcp_cli/chat/session_store.py b/src/mcp_cli/chat/session_store.py index 23e2abac..1672d2f9 100644 --- a/src/mcp_cli/chat/session_store.py +++ b/src/mcp_cli/chat/session_store.py @@ -18,6 +18,16 @@ logger = logging.getLogger(__name__) +def _sanitize_path_component(value: str) -> str: + """Strip path separators and '..' so *value* can't escape its parent dir. + + Used for any identifier (session_id, agent_id) that ends up as a path + component but may originate from LLM-controllable input (e.g. an + agent_spawn tool call), not just direct user input. + """ + return value.replace("/", "_").replace("\\", "_").replace("..", "_") + + class SessionMetadata(BaseModel): """Metadata for a saved session.""" @@ -57,16 +67,18 @@ def __init__( if sessions_dir is None: sessions_dir = Path(DEFAULT_SESSIONS_DIR).expanduser() self.agent_id = agent_id - # Agent-namespaced subdirectory - self.sessions_dir = sessions_dir / agent_id + # Agent-namespaced subdirectory โ€” sanitized since agent_id can + # originate from an LLM-controllable agent_spawn tool call, not + # just direct user input. + safe_agent_id = _sanitize_path_component(agent_id) + self.sessions_dir = sessions_dir / safe_agent_id self.sessions_dir.mkdir(parents=True, exist_ok=True) # Keep reference to root for backward-compat migration self._root_dir = sessions_dir def _session_path(self, session_id: str) -> Path: """Get the file path for a session.""" - # Sanitize session_id to prevent path traversal - safe_id = session_id.replace("/", "_").replace("\\", "_").replace("..", "_") + safe_id = _sanitize_path_component(session_id) return self.sessions_dir / f"{safe_id}.json" def save(self, data: SessionData) -> Path: @@ -120,7 +132,7 @@ def _migrate_from_root(self, session_id: str) -> Path | None: Returns the new path if migration succeeded, None otherwise. """ - safe_id = session_id.replace("/", "_").replace("\\", "_").replace("..", "_") + safe_id = _sanitize_path_component(session_id) legacy_path = self._root_dir / f"{safe_id}.json" if not legacy_path.exists() or not legacy_path.is_file(): return None diff --git a/src/mcp_cli/commands/cmd/cmd.py b/src/mcp_cli/commands/cmd/cmd.py index 97bb48d0..53169765 100644 --- a/src/mcp_cli/commands/cmd/cmd.py +++ b/src/mcp_cli/commands/cmd/cmd.py @@ -392,7 +392,7 @@ async def _handle_tool_calls( messages=messages, max_tokens=4096, ) - return final_response.get("response", response_text) + return str(final_response.get("response", response_text)) except Exception: return response_text diff --git a/src/mcp_cli/commands/plan/plan.py b/src/mcp_cli/commands/plan/plan.py index a9640f81..e6c2c931 100644 --- a/src/mcp_cli/commands/plan/plan.py +++ b/src/mcp_cli/commands/plan/plan.py @@ -12,6 +12,7 @@ CommandResult, ) from mcp_cli.config.enums import PlanAction +from mcp_cli.planning.backends import ConfirmPromptCallback logger = logging.getLogger(__name__) @@ -139,7 +140,7 @@ async def execute(self, **kwargs) -> CommandResult: success=False, error="Plan ID required. Usage: /plan resume ", ) - return await self._resume_plan(planning_context, remainder.strip()) + return await self._resume_plan(planning_context, remainder.strip(), kwargs) else: return CommandResult( @@ -301,6 +302,18 @@ async def on_tool_complete(tool_name, result_text, success, elapsed): if display: await display.stop_tool_execution(result_text, success) + confirm_prompt: ConfirmPromptCallback | None = None + if ui_manager is not None and hasattr(ui_manager, "do_confirm_tool_execution"): + + async def _confirm_prompt(tool_name: str, arguments: dict) -> bool: + return bool( + await ui_manager.do_confirm_tool_execution( + tool_name=tool_name, arguments=arguments + ) + ) + + confirm_prompt = _confirm_prompt + # Get model_manager for LLM-driven execution model_manager = (kwargs or {}).get("model_manager") @@ -311,6 +324,7 @@ async def on_tool_complete(tool_name, result_text, success, elapsed): on_step_complete=on_step_complete, on_tool_start=on_tool_start, on_tool_complete=on_tool_complete, + confirm_prompt=confirm_prompt, ) result = await runner.execute_plan(plan_data, dry_run=dry_run) @@ -347,7 +361,9 @@ async def _delete_plan(self, context, plan_id: str) -> CommandResult: error=f"Plan not found: {plan_id}", ) - async def _resume_plan(self, context, plan_id: str) -> CommandResult: + async def _resume_plan( + self, context, plan_id: str, kwargs: dict | None = None + ) -> CommandResult: """Resume an interrupted plan.""" from chuk_term.ui import output from mcp_cli.planning.executor import PlanRunner @@ -359,7 +375,20 @@ async def _resume_plan(self, context, plan_id: str) -> CommandResult: error=f"Plan not found: {plan_id}", ) - runner = PlanRunner(context) + ui_manager = (kwargs or {}).get("ui_manager") + confirm_prompt: ConfirmPromptCallback | None = None + if ui_manager is not None and hasattr(ui_manager, "do_confirm_tool_execution"): + + async def _confirm_prompt(tool_name: str, arguments: dict) -> bool: + return bool( + await ui_manager.do_confirm_tool_execution( + tool_name=tool_name, arguments=arguments + ) + ) + + confirm_prompt = _confirm_prompt + + runner = PlanRunner(context, confirm_prompt=confirm_prompt) checkpoint = runner.load_checkpoint(plan_id) if not checkpoint: diff --git a/src/mcp_cli/config/defaults.py b/src/mcp_cli/config/defaults.py index a6ed2e08..85a23aaa 100644 --- a/src/mcp_cli/config/defaults.py +++ b/src/mcp_cli/config/defaults.py @@ -367,6 +367,10 @@ DEFAULT_MEMORY_MAX_PROMPT_CHARS = 2000 """Maximum characters for memory section in system prompt.""" +DEFAULT_MEMORY_MAX_ENTRY_CHARS = 10_000 +"""Maximum characters for a single memory entry's content, to bound +on-disk growth from repeated remember() calls with large payloads.""" + # ================================================================ # Planning Defaults (Tier 6) diff --git a/src/mcp_cli/dashboard/server.py b/src/mcp_cli/dashboard/server.py index 65c1fc2e..b6f9a323 100644 --- a/src/mcp_cli/dashboard/server.py +++ b/src/mcp_cli/dashboard/server.py @@ -32,6 +32,8 @@ "Dashboard support requires websockets. Install with: pip install mcp-cli[dashboard]" ) +from mcp_cli.utils.loopback_origin import is_allowed_origin + logger = logging.getLogger(__name__) _STATIC_DIR = Path(__file__).parent / "static" @@ -43,6 +45,7 @@ class DashboardServer: def __init__(self) -> None: self._clients: set[ServerConnection] = set() self._server: Any = None + self._port: int = 0 # Called when a browser user sends USER_MESSAGE / USER_ACTION / REQUEST_TOOL self.on_browser_message: Callable[..., Any] | None = None # Called when a new WebSocket client connects (before message loop starts) @@ -66,6 +69,7 @@ def has_clients(self) -> bool: async def start(self, port: int = 0) -> int: """Find an available port and start the server. Returns the bound port.""" bound_port = await self._find_port(port) + self._port = bound_port self._server = await ws_serve( self._ws_handler, @@ -186,8 +190,27 @@ def _process_request( ) -> Response | None: path = request.path.split("?")[0] # strip query string - # WebSocket upgrade โ€” let the library handle it if path == "/ws": + # Reject cross-origin WebSocket upgrades. Browsers attach an + # Origin header to WS handshakes but do not enforce same-origin + # policy on them the way they do for fetch()/XHR โ€” enforcement + # is the server's responsibility, so any page that knows (or + # scans for) this port could otherwise attach to the dashboard + # and read/inject conversation state. + origin = request.headers.get("Origin") + if not is_allowed_origin(origin, self._port): + logger.warning( + "Rejected dashboard WebSocket connection: disallowed Origin %r", + origin, + ) + body = b"Forbidden" + return Response( + http.HTTPStatus.FORBIDDEN, + "Forbidden", + websockets.Headers({"Content-Length": str(len(body))}), + body, + ) + # Let the library proceed with the WebSocket upgrade return None # Serve static files diff --git a/src/mcp_cli/dashboard/static/js/layout.js b/src/mcp_cli/dashboard/static/js/layout.js index 40c01e90..561e172a 100644 --- a/src/mcp_cli/dashboard/static/js/layout.js +++ b/src/mcp_cli/dashboard/static/js/layout.js @@ -9,7 +9,7 @@ import { setPanels, incPanelCounter, setLayoutConfig, isSidebarView, _sidebarOpen, } from './state.js'; -import { makeDraggable, showToast } from './utils.js'; +import { esc, makeDraggable, showToast } from './utils.js'; import { getOrCreateView, attachViewToSlot, iconForView, labelForView, srcForView, updatePanelHeader, switchPanelView, populateViewMenu, postToIframe, @@ -90,8 +90,8 @@ export function createPanelSlot(viewId, rowEl) { header.className = 'panel-header'; header.draggable = true; header.innerHTML = ` - ${iconForView(resolvedViewId)} - + ${esc(iconForView(resolvedViewId))} +
diff --git a/src/mcp_cli/dashboard/static/views/agent-terminal.html b/src/mcp_cli/dashboard/static/views/agent-terminal.html index 3193c10e..062954fd 100644 --- a/src/mcp_cli/dashboard/static/views/agent-terminal.html +++ b/src/mcp_cli/dashboard/static/views/agent-terminal.html @@ -6,6 +6,8 @@ Agent Terminal + + @@ -654,15 +656,19 @@ function renderBubbleContent(bubble, text, streaming) { const contentEl = bubble.querySelector('.msg-content'); if (!contentEl) return; - if (typeof marked !== 'undefined' && !streaming) { + // Assistant text can be steered by anything an MCP server returns (prompt + // injection), so treat marked's HTML output as untrusted and run it + // through DOMPurify rather than a hand-rolled regex blocklist โ€” a regex + // approach missed things like (no leading whitespace + // before the attribute) and