diff --git a/routes/mcp/mcp_routes.py b/routes/mcp/mcp_routes.py index 94c83f8dd..0e2b30276 100644 --- a/routes/mcp/mcp_routes.py +++ b/routes/mcp/mcp_routes.py @@ -181,10 +181,17 @@ def setup_mcp_routes(mcp_manager: McpManager): if transport == "http" and not url: raise HTTPException(400, "url is required for HTTP transport") - # Parse JSON fields - try: - parsed_args = json.loads(args) if args else [] - except json.JSONDecodeError: + # Parse JSON fields. args is not defaulted on a parse failure: an + # unparseable value is silently discarded downstream (stdio spawns + # with an empty argv), so the caller must be told instead. + if args: + try: + parsed_args = json.loads(args) + except json.JSONDecodeError: + raise HTTPException(400, "args must be valid JSON, e.g. [\"-y\", \"pkg\"]") + if not isinstance(parsed_args, list): + raise HTTPException(400, "args must be a JSON array, e.g. [\"-y\", \"pkg\"]") + else: parsed_args = [] try: parsed_env = json.loads(env) if env else {} diff --git a/static/js/admin.js b/static/js/admin.js index 6fd4ce057..de07dc8e7 100644 --- a/static/js/admin.js +++ b/static/js/admin.js @@ -2366,6 +2366,7 @@ function initMcpForm() { if (transport === 'stdio' && !command) { msg.textContent = 'Command is required for stdio'; msg.className = 'admin-error'; return; } if (transport === 'sse' && !url) { msg.textContent = 'URL is required for SSE'; msg.className = 'admin-error'; return; } try { JSON.parse(env); } catch { msg.textContent = 'Env must be valid JSON'; msg.className = 'admin-error'; return; } + try { JSON.parse(args); } catch { msg.textContent = 'Args must be valid JSON, e.g. ["-y", "pkg"]'; msg.className = 'admin-error'; return; } const fd = new FormData(); fd.append('name', name); fd.append('transport', transport); fd.append('command', command); fd.append('args', args); fd.append('env', env); fd.append('url', url); // If preset has oauthFile config, send credentials for file generation @@ -2386,6 +2387,10 @@ function initMcpForm() { try { const res = await fetch('/api/mcp/servers', { method: 'POST', body: fd, credentials: 'same-origin' }); const data = await res.json(); + if (!res.ok) { + msg.textContent = data.detail || `Failed (${res.status})`; msg.className = 'admin-error'; + return; + } if (data.needs_oauth) { msg.innerHTML = `Added ${esc(name)} — Authorize with Google to connect`; msg.className = 'admin-success'; diff --git a/static/js/settings.js b/static/js/settings.js index 09552b26c..a8e0478b5 100644 --- a/static/js/settings.js +++ b/static/js/settings.js @@ -5036,7 +5036,11 @@ async function initUnifiedIntegrations() { fd.append('transport', transport); if (transport === 'stdio') { fd.append('command', el('uf-mcp-cmd').value); - let args = '[]'; try { args = JSON.stringify(JSON.parse(el('uf-mcp-args').value || '[]')); } catch (_) {} + // Unlike env below, an unparseable args value is not silently + // defaulted: it would spawn the subprocess with an empty argv. + let args; + try { args = JSON.stringify(JSON.parse(el('uf-mcp-args').value || '[]')); } + catch (_) { el('uf-mcp-msg').textContent = 'Args must be valid JSON, e.g. ["-y", "pkg"]'; return; } let env = '{}'; try { env = JSON.stringify(JSON.parse(el('uf-mcp-env').value || '{}')); } catch (_) {} fd.append('args', args); fd.append('env', env); diff --git a/tests/test_mcp_add_server_args_validation.py b/tests/test_mcp_add_server_args_validation.py new file mode 100644 index 000000000..3550c85dc --- /dev/null +++ b/tests/test_mcp_add_server_args_validation.py @@ -0,0 +1,147 @@ +"""Regression test for issue #6211: a malformed Args value on the "Add MCP +Server" form must not be silently discarded into an empty argv. + +routes/mcp/mcp_routes.py's add_server() wrapped json.loads(args) in a bare +except that fell back to `[]`, so a non-JSON Args value registered the +server as "Connected" while forwarding no arguments to the spawned stdio +subprocess at all, with no error surfaced anywhere. +""" +import asyncio +import json +from unittest.mock import AsyncMock, MagicMock + +import pytest +from fastapi import HTTPException + +from routes.mcp import mcp_routes + + +class _FakeSession: + """Stands in for core.database.SessionLocal(); add_server only adds+commits.""" + + def __init__(self): + self.added = [] + + def add(self, obj): + self.added.append(obj) + + def commit(self): + pass + + def close(self): + pass + + +def _add_server(monkeypatch): + """Register add_server on the shared module-level router and return the + freshly-added route's raw endpoint function, bypassing HTTP/Form parsing + (require_admin is the only other thing the function touches via `request`). + + Callers must pass every Form(...) parameter add_server reads past the args + check (url, oauth_file, oauth_config): calling the endpoint directly skips + FastAPI's dependency resolution, so an omitted one arrives as the Form + marker object itself rather than its declared default, and later code + (e.g. `if oauth_file:`) reads that marker as truthy. + """ + monkeypatch.setattr(mcp_routes, "require_admin", lambda request: None) + manager = MagicMock() + manager.connect_server = AsyncMock(return_value=True) + manager.get_server_status = MagicMock(return_value={"status": "connected", "tool_count": 1}) + router = mcp_routes.setup_mcp_routes(manager) + # setup_mcp_routes appends new APIRoute objects to the shared router on + # every call, so take the LAST "add_server" route: the one just registered + # with our fake manager, not an earlier registration from importing app.py. + route = [r for r in router.routes if getattr(r, "name", None) == "add_server"][-1] + return route.endpoint, manager + + +def test_add_server_rejects_malformed_args_instead_of_defaulting(monkeypatch): + add_server, manager = _add_server(monkeypatch) + monkeypatch.setattr(mcp_routes, "SessionLocal", lambda: (_ for _ in ()).throw( + AssertionError("must not reach the DB when args is rejected"))) + + with pytest.raises(HTTPException) as exc: + asyncio.run(add_server( + request=None, + name="filesystem", + transport="stdio", + command="mcp-server-filesystem", + args="/app/data/jarvis-files", # the exact value from issue #6211 + env="{}", + url=None, + oauth_file=None, + oauth_config=None, + )) + + assert exc.value.status_code == 400 + manager.connect_server.assert_not_called() + + +def test_add_server_still_accepts_valid_json_args(monkeypatch): + add_server, manager = _add_server(monkeypatch) + fake_session = _FakeSession() + monkeypatch.setattr(mcp_routes, "SessionLocal", lambda: fake_session) + + result = asyncio.run(add_server( + request=None, + name="filesystem", + transport="stdio", + command="mcp-server-filesystem", + args=json.dumps(["/app/data/jarvis-files"]), + env="{}", + url=None, + oauth_file=None, + oauth_config=None, + )) + + assert result["connected"] is True + manager.connect_server.assert_awaited_once() + assert manager.connect_server.call_args.kwargs["args"] == ["/app/data/jarvis-files"] + assert fake_session.added[0].args == json.dumps(["/app/data/jarvis-files"]) + + +def test_add_server_rejects_valid_json_args_that_is_not_a_list(monkeypatch): + """Valid JSON that is not a list (e.g. args=5) must not reach + StdioServerParameters(args=5), which raises an unhandled TypeError when + the error formatter later does " ".join([command, *args]).""" + add_server, manager = _add_server(monkeypatch) + monkeypatch.setattr(mcp_routes, "SessionLocal", lambda: (_ for _ in ()).throw( + AssertionError("must not reach the DB when args has the wrong shape"))) + + with pytest.raises(HTTPException) as exc: + asyncio.run(add_server( + request=None, + name="filesystem", + transport="stdio", + command="mcp-server-filesystem", + args="5", + env="{}", + url=None, + oauth_file=None, + oauth_config=None, + )) + + assert exc.value.status_code == 400 + manager.connect_server.assert_not_called() + + +def test_add_server_still_defaults_empty_args_to_empty_list(monkeypatch): + """No behavior change for the common case of an empty Args field.""" + add_server, manager = _add_server(monkeypatch) + fake_session = _FakeSession() + monkeypatch.setattr(mcp_routes, "SessionLocal", lambda: fake_session) + + result = asyncio.run(add_server( + request=None, + name="no-args-server", + transport="stdio", + command="some-command", + args="", + env="{}", + url=None, + oauth_file=None, + oauth_config=None, + )) + + assert result["connected"] is True + assert manager.connect_server.call_args.kwargs["args"] == []