Studio: clearer MCP server validation when stdio is disabled (#5928)
Improve the rejection message when an MCP server address is not an http(s) URL. It now points to the expected http(s):// form with an example, and only mentions that local commands are disabled when the value contains whitespace (a reliable command signal), since a lone token may just be a scheme-less URL. Wording is host-scoped rather than desktop-only because self-hosted hosts can opt in via an env var. Backend only, with tests; accepted input is unchanged.
This commit is contained in:
parent
a0b85bcc1d
commit
0cbf641a1e
2 changed files with 52 additions and 6 deletions
|
|
@ -32,14 +32,21 @@ logger = structlog.get_logger(__name__)
|
|||
router = APIRouter()
|
||||
|
||||
|
||||
def _looks_like_command(value: str) -> bool:
|
||||
"""Whitespace is a one-way signal: a URL can't hold an unencoded space, so a
|
||||
value with whitespace is definitely a command. No whitespace proves nothing
|
||||
(a lone token may be a single-arg command or a scheme-less URL)."""
|
||||
return any(ch.isspace() for ch in value)
|
||||
|
||||
|
||||
def _validate_url(url: str) -> str:
|
||||
trimmed = (url or "").strip()
|
||||
if not trimmed:
|
||||
raise HTTPException(status_code = 400, detail = "url must not be empty")
|
||||
# When stdio is enabled on this host, a non-HTTP value is a local command.
|
||||
# Reuse this field so stdio servers ride the existing CRUD/storage with no
|
||||
# schema change. When stdio is disabled the value falls through to the
|
||||
# http-only validation below, so non-HTTP input is just a bad URL (400).
|
||||
# schema change. A lone token (example.com, /usr/bin/srv) is ambiguous, so we
|
||||
# keep the existing behaviour and treat any non-HTTP value as a command here.
|
||||
if stdio_mcp_enabled() and is_stdio(trimmed):
|
||||
try:
|
||||
parts = parse_stdio_command(trimmed)
|
||||
|
|
@ -58,10 +65,15 @@ def _validate_url(url: str) -> str:
|
|||
return trimmed
|
||||
parsed = urlparse(trimmed)
|
||||
if parsed.scheme not in ("http", "https"):
|
||||
raise HTTPException(
|
||||
status_code = 400,
|
||||
detail = "url must start with http:// or https://",
|
||||
detail = (
|
||||
"MCP server address must start with http:// or https:// "
|
||||
"(for example https://example.com/mcp)."
|
||||
)
|
||||
# Host-scoped wording ("this server"), not "desktop only": self-hosted
|
||||
# hosts can opt in via the env var.
|
||||
if _looks_like_command(trimmed):
|
||||
detail += " Running a local command is not enabled on this server."
|
||||
raise HTTPException(status_code = 400, detail = detail)
|
||||
if not parsed.netloc:
|
||||
raise HTTPException(status_code = 400, detail = "url is missing a host")
|
||||
return trimmed
|
||||
|
|
|
|||
|
|
@ -200,12 +200,40 @@ def test_validate_url_gate_off_rejects_stdio(monkeypatch):
|
|||
from routes.mcp_servers import _validate_url
|
||||
|
||||
assert _validate_url("https://example.com/mcp") == "https://example.com/mcp"
|
||||
for bad in ["npx server", "python -m mod", "ftp://host"]:
|
||||
# urlparse reads "localhost:8000" scheme as "localhost", so it lands here too.
|
||||
for bad in [
|
||||
"npx server",
|
||||
"python -m mod",
|
||||
"ftp://host",
|
||||
"example.com",
|
||||
"localhost:8000",
|
||||
r"C:\node\node.exe server.js",
|
||||
]:
|
||||
with pytest.raises(HTTPException) as exc:
|
||||
_validate_url(bad)
|
||||
assert exc.value.status_code == 400
|
||||
|
||||
|
||||
def test_validate_url_gate_off_message_depends_on_whitespace(monkeypatch):
|
||||
# The message names a command only when the value has whitespace, and never
|
||||
# says "desktop app only" (self-hosted hosts can opt in via the env var).
|
||||
_disable(monkeypatch)
|
||||
from routes.mcp_servers import _validate_url
|
||||
|
||||
with pytest.raises(HTTPException) as exc:
|
||||
_validate_url("npx -y @modelcontextprotocol/server-filesystem /tmp")
|
||||
cmd = exc.value.detail.lower()
|
||||
assert "http://" in cmd and "https://" in cmd
|
||||
assert "local command" in cmd
|
||||
assert "desktop app" not in cmd
|
||||
|
||||
with pytest.raises(HTTPException) as exc:
|
||||
_validate_url("example.com")
|
||||
lone = exc.value.detail.lower()
|
||||
assert "http://" in lone and "https://" in lone
|
||||
assert "local command" not in lone
|
||||
|
||||
|
||||
def test_validate_url_gate_on_accepts_stdio(monkeypatch):
|
||||
_enable(monkeypatch)
|
||||
from routes.mcp_servers import _validate_url
|
||||
|
|
@ -217,6 +245,12 @@ def test_validate_url_gate_on_accepts_stdio(monkeypatch):
|
|||
assert _validate_url("npx server --url https://x/mcp") == (
|
||||
"npx server --url https://x/mcp"
|
||||
)
|
||||
# A lone token is ambiguous; keep the prior behaviour and accept it as a
|
||||
# command rather than guessing it's a URL (no regression for single binaries).
|
||||
assert (
|
||||
_validate_url("/usr/local/bin/my-mcp-server") == "/usr/local/bin/my-mcp-server"
|
||||
)
|
||||
assert _validate_url("mcp-server-sqlite") == "mcp-server-sqlite"
|
||||
# empty / unparseable still rejected
|
||||
for bad in [" ", '"unclosed']:
|
||||
with pytest.raises(HTTPException) as exc:
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue