From ececd7e913f2afd68d99fedb5896850db7702498 Mon Sep 17 00:00:00 2001 From: Rod Boev Date: Thu, 16 Jul 2026 20:50:34 -0400 Subject: [PATCH 1/3] fix(mcp): reject local targets over HTTP transport Signed-off-by: Rod Boev --- src/skillspector/mcp_server.py | 36 ++++++++- tests/unit/test_mcp_server.py | 137 +++++++++++++++++++++++++++++++++ 2 files changed, 170 insertions(+), 3 deletions(-) diff --git a/src/skillspector/mcp_server.py b/src/skillspector/mcp_server.py index e8aadedc..b6d250a2 100644 --- a/src/skillspector/mcp_server.py +++ b/src/skillspector/mcp_server.py @@ -27,6 +27,7 @@ from __future__ import annotations +from pathlib import Path from typing import TYPE_CHECKING, Any from skillspector import __version__ @@ -44,11 +45,30 @@ VALID_FORMATS = ("json", "markdown", "sarif", "terminal") +def _is_local_target(target: str) -> bool: + """Return True when ``target`` names local filesystem content.""" + stripped = target.strip() + if stripped.startswith("file://"): + return True + if stripped.startswith(("http://", "https://", "git@", "ssh://", "git+ssh://")): + return False + if stripped.startswith(("\\\\", "//")): + return True + + candidate = Path(stripped).expanduser() + if candidate.is_absolute() or candidate.drive: + return True + if "://" in stripped: + return False + return candidate.exists() + + async def run_scan( target: str, *, use_llm: bool = True, output_format: str = "json", + allow_local_targets: bool = True, yara_rules_dir: str | None = None, ) -> dict[str, Any]: """Invoke the SkillSpector graph and return a structured verdict. @@ -61,6 +81,9 @@ async def run_scan( what actually happened. output_format: Format of the embedded ``report`` string. One of :data:`VALID_FORMATS`. + allow_local_targets: Whether local filesystem targets are allowed. + HTTP MCP calls set this to ``False`` so routable servers do not + accept caller-controlled local paths. yara_rules_dir: Optional directory of additional YARA rules. Returns: @@ -72,6 +95,8 @@ async def run_scan( """ if output_format not in VALID_FORMATS: raise ValueError(f"output_format must be one of {VALID_FORMATS}, got {output_format!r}") + if not allow_local_targets and _is_local_target(target): + raise ValueError("local targets are disabled for this MCP transport") llm_available, _ = is_llm_available() llm_used = use_llm and llm_available @@ -136,7 +161,7 @@ async def run_scan( cleanup_result(result) -def build_server(name: str = "skillspector") -> FastMCP: +def build_server(name: str = "skillspector", *, allow_local_targets: bool = True) -> FastMCP: """Construct the FastMCP server exposing the ``scan_skill`` tool. Requires the optional ``mcp`` dependency (``pip install 'skillspector[mcp]'``). @@ -169,14 +194,19 @@ async def scan_skill( actually ran, so a low score from a static-only scan is not mistaken for a clean full scan. """ - return await run_scan(target, use_llm=use_llm, output_format=output_format) + return await run_scan( + target, + use_llm=use_llm, + output_format=output_format, + allow_local_targets=allow_local_targets, + ) return server def run(transport: str = "stdio", host: str = "127.0.0.1", port: int = 8000) -> None: """Run the MCP server over ``stdio`` (local agents) or ``http`` (remote/A2A).""" - server = build_server() + server = build_server(allow_local_targets=transport != "http") if transport == "stdio": server.run(transport="stdio") elif transport == "http": diff --git a/tests/unit/test_mcp_server.py b/tests/unit/test_mcp_server.py index 12149095..0d357eb6 100644 --- a/tests/unit/test_mcp_server.py +++ b/tests/unit/test_mcp_server.py @@ -19,6 +19,8 @@ import os import sys from pathlib import Path +from types import SimpleNamespace +from unittest.mock import AsyncMock, MagicMock import pytest @@ -222,6 +224,141 @@ async def failed_execution_result(state: dict, config: dict) -> dict: assert verdict["execution_successful"] is False +async def test_run_scan_rejects_local_target_when_disallowed( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """HTTP-style scans reject local targets before the graph is invoked.""" + graph_ainvoke = AsyncMock() + monkeypatch.setattr(mcp_server.graph, "ainvoke", graph_ainvoke) + monkeypatch.setattr(mcp_server, "is_llm_available", lambda: (False, "no llm")) + + with pytest.raises(ValueError, match="local targets are disabled"): + await run_scan(str(tmp_path), allow_local_targets=False) + + assert graph_ainvoke.await_count == 0 + + +async def test_run_scan_rejects_file_url_when_local_targets_disallowed( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """The same HTTP guard rejects file:// targets before any scan runs.""" + graph_ainvoke = AsyncMock() + monkeypatch.setattr(mcp_server.graph, "ainvoke", graph_ainvoke) + monkeypatch.setattr(mcp_server, "is_llm_available", lambda: (False, "no llm")) + + with pytest.raises(ValueError, match="local targets are disabled"): + await run_scan(tmp_path.as_uri(), allow_local_targets=False) + + assert graph_ainvoke.await_count == 0 + + +@pytest.mark.parametrize( + ("target", "expected"), + [ + (r"\\server\share\skill", True), + ("//server/share/skill", True), + ("git@github.com:NVIDIA/SkillSpector.git", False), + ("ssh://git@github.com/NVIDIA/SkillSpector.git", False), + ("git+ssh://git@github.com/NVIDIA/SkillSpector.git", False), + ("custom://example/skill", False), + ], +) +def test_is_local_target_classifies_protocol_edges(target: str, expected: bool) -> None: + """Classifier treats UNC-style paths as local and known remote schemes as remote.""" + assert mcp_server._is_local_target(target) is expected + + +def test_is_local_target_checks_relative_paths_from_cwd( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Existing relative paths are local; missing relative paths stay unresolved.""" + (tmp_path / "skill").mkdir() + monkeypatch.chdir(tmp_path) + + assert mcp_server._is_local_target("skill") is True + assert mcp_server._is_local_target("missing-skill") is False + + +async def test_run_scan_allows_remote_target_when_local_targets_disallowed( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Remote HTTP targets still reach the resolver path when local targets are blocked.""" + graph_ainvoke = AsyncMock( + return_value={ + "risk_score": 0, + "risk_severity": "low", + "risk_recommendation": "safe", + "filtered_findings": [], + "report_body": "ok", + } + ) + monkeypatch.setattr(mcp_server.graph, "ainvoke", graph_ainvoke) + monkeypatch.setattr(mcp_server, "is_llm_available", lambda: (False, "no llm")) + + target = "https://example.com/skills/safe.git" + result = await run_scan(target, allow_local_targets=False) + + assert result["target"] == target + assert graph_ainvoke.await_count == 1 + assert graph_ainvoke.await_args.args[0]["input_path"] == target + + +async def test_run_scan_keeps_default_local_target_compatibility( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """The default run_scan path still accepts local targets.""" + graph_ainvoke = AsyncMock( + return_value={ + "risk_score": 0, + "risk_severity": "low", + "risk_recommendation": "safe", + "filtered_findings": [], + "report_body": "ok", + } + ) + monkeypatch.setattr(mcp_server.graph, "ainvoke", graph_ainvoke) + monkeypatch.setattr(mcp_server, "is_llm_available", lambda: (False, "no llm")) + + result = await run_scan(str(tmp_path)) + + assert result["target"] == str(tmp_path) + assert graph_ainvoke.await_count == 1 + assert graph_ainvoke.await_args.args[0]["input_path"] == str(tmp_path) + + +@pytest.mark.parametrize( + ("transport", "expected_allow_local_targets"), + [("stdio", True), ("http", False)], +) +def test_run_passes_transport_local_target_policy( + transport: str, + expected_allow_local_targets: bool, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """run() keeps stdio local scans available and disables them for HTTP.""" + captured: dict[str, bool] = {} + server = SimpleNamespace( + settings=SimpleNamespace(host=None, port=None), + run=MagicMock(), + ) + + def fake_build_server(*, allow_local_targets: bool = True): + captured["allow_local_targets"] = allow_local_targets + return server + + monkeypatch.setattr(mcp_server, "build_server", fake_build_server) + + mcp_server.run(transport=transport, host="0.0.0.0", port=9000) + + assert captured["allow_local_targets"] is expected_allow_local_targets + if transport == "http": + assert server.settings.host == "0.0.0.0" + assert server.settings.port == 9000 + server.run.assert_called_once_with(transport="streamable-http") + else: + server.run.assert_called_once_with(transport="stdio") + + async def test_build_server_registers_scan_skill() -> None: """build_server wires up the scan_skill tool (requires the mcp extra).""" pytest.importorskip("mcp") From 48f05a0cff40a3f6c136255c26d88ec195bdaea4 Mon Sep 17 00:00:00 2001 From: Rod Boev Date: Fri, 31 Jul 2026 12:17:05 -0400 Subject: [PATCH 2/3] fix(mcp): close direct builder local-target bypass Signed-off-by: Rod Boev --- src/skillspector/mcp_server.py | 3 ++- tests/unit/test_mcp_server.py | 21 ++++++++++++++++++++- 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/src/skillspector/mcp_server.py b/src/skillspector/mcp_server.py index b6d250a2..7c157c7c 100644 --- a/src/skillspector/mcp_server.py +++ b/src/skillspector/mcp_server.py @@ -161,10 +161,11 @@ async def run_scan( cleanup_result(result) -def build_server(name: str = "skillspector", *, allow_local_targets: bool = True) -> FastMCP: +def build_server(name: str = "skillspector", *, allow_local_targets: bool = False) -> FastMCP: """Construct the FastMCP server exposing the ``scan_skill`` tool. Requires the optional ``mcp`` dependency (``pip install 'skillspector[mcp]'``). + Local targets stay disabled unless the caller selects a trusted transport. """ try: from mcp.server.fastmcp import FastMCP diff --git a/tests/unit/test_mcp_server.py b/tests/unit/test_mcp_server.py index 0d357eb6..55cd40de 100644 --- a/tests/unit/test_mcp_server.py +++ b/tests/unit/test_mcp_server.py @@ -342,7 +342,7 @@ def test_run_passes_transport_local_target_policy( run=MagicMock(), ) - def fake_build_server(*, allow_local_targets: bool = True): + def fake_build_server(*, allow_local_targets: bool = False): captured["allow_local_targets"] = allow_local_targets return server @@ -368,6 +368,25 @@ async def test_build_server_registers_scan_skill() -> None: assert "scan_skill" in {tool.name for tool in tools} +async def test_build_server_disables_local_targets_by_default( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """Direct server construction remains fail-closed before transport selection.""" + pytest.importorskip("mcp") + from mcp.server.fastmcp.exceptions import ToolError + + graph_ainvoke = AsyncMock() + monkeypatch.setattr(mcp_server.graph, "ainvoke", graph_ainvoke) + monkeypatch.setattr(mcp_server, "is_llm_available", lambda: (False, "no llm")) + + server = mcp_server.build_server() + + with pytest.raises(ToolError, match="local targets are disabled"): + await server.call_tool("scan_skill", {"target": str(tmp_path)}) + + assert graph_ainvoke.await_count == 0 + + async def test_mcp_stdio_initialize_registers_scan_skill() -> None: """The real stdio CLI must initialize and expose the scan_skill tool.""" pytest.importorskip("mcp") From 73b0ac433bd65332382293e594b918451f2539d4 Mon Sep 17 00:00:00 2001 From: Rod Boev Date: Fri, 31 Jul 2026 12:27:59 -0400 Subject: [PATCH 3/3] fix(mcp): keep target policy fail closed across paths Signed-off-by: Rod Boev --- src/skillspector/mcp_server.py | 14 ++++++--- tests/unit/test_mcp_server.py | 54 ++++++++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+), 4 deletions(-) diff --git a/src/skillspector/mcp_server.py b/src/skillspector/mcp_server.py index 7c157c7c..e377d972 100644 --- a/src/skillspector/mcp_server.py +++ b/src/skillspector/mcp_server.py @@ -55,7 +55,10 @@ def _is_local_target(target: str) -> bool: if stripped.startswith(("\\\\", "//")): return True - candidate = Path(stripped).expanduser() + try: + candidate = Path(stripped).expanduser() + except RuntimeError: + return True if candidate.is_absolute() or candidate.drive: return True if "://" in stripped: @@ -95,8 +98,11 @@ async def run_scan( """ if output_format not in VALID_FORMATS: raise ValueError(f"output_format must be one of {VALID_FORMATS}, got {output_format!r}") - if not allow_local_targets and _is_local_target(target): - raise ValueError("local targets are disabled for this MCP transport") + if not allow_local_targets: + local_target = _is_local_target(target) + local_yara_rules = yara_rules_dir is not None and _is_local_target(yara_rules_dir) + if local_target or local_yara_rules: + raise ValueError("local targets are disabled for this MCP transport") llm_available, _ = is_llm_available() llm_used = use_llm and llm_available @@ -207,7 +213,7 @@ async def scan_skill( def run(transport: str = "stdio", host: str = "127.0.0.1", port: int = 8000) -> None: """Run the MCP server over ``stdio`` (local agents) or ``http`` (remote/A2A).""" - server = build_server(allow_local_targets=transport != "http") + server = build_server(allow_local_targets=transport == "stdio") if transport == "stdio": server.run(transport="stdio") elif transport == "http": diff --git a/tests/unit/test_mcp_server.py b/tests/unit/test_mcp_server.py index 55cd40de..3e7243c3 100644 --- a/tests/unit/test_mcp_server.py +++ b/tests/unit/test_mcp_server.py @@ -252,6 +252,24 @@ async def test_run_scan_rejects_file_url_when_local_targets_disallowed( assert graph_ainvoke.await_count == 0 +async def test_run_scan_rejects_local_yara_rules_when_targets_are_disallowed( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """The HTTP policy covers local YARA configuration as well as scan targets.""" + graph_ainvoke = AsyncMock() + monkeypatch.setattr(mcp_server.graph, "ainvoke", graph_ainvoke) + monkeypatch.setattr(mcp_server, "is_llm_available", lambda: (False, "no llm")) + + with pytest.raises(ValueError, match="local targets are disabled"): + await run_scan( + "https://example.com/skills/safe.git", + allow_local_targets=False, + yara_rules_dir=str(tmp_path), + ) + + assert graph_ainvoke.await_count == 0 + + @pytest.mark.parametrize( ("target", "expected"), [ @@ -279,6 +297,19 @@ def test_is_local_target_checks_relative_paths_from_cwd( assert mcp_server._is_local_target("missing-skill") is False +def test_is_local_target_fails_closed_when_home_cannot_be_resolved( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Unresolvable tilde paths remain local instead of leaking a runtime error.""" + + def fail_to_expanduser(self: Path) -> Path: + raise RuntimeError("Could not determine home directory") + + monkeypatch.setattr(Path, "expanduser", fail_to_expanduser) + + assert mcp_server._is_local_target("~nosuchuser/skill") is True + + async def test_run_scan_allows_remote_target_when_local_targets_disallowed( monkeypatch: pytest.MonkeyPatch, ) -> None: @@ -359,6 +390,29 @@ def fake_build_server(*, allow_local_targets: bool = False): server.run.assert_called_once_with(transport="stdio") +def test_run_rejects_unknown_transport_without_allowing_local_targets( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Unknown transports fail closed before a server can start.""" + captured: dict[str, bool] = {} + server = SimpleNamespace( + settings=SimpleNamespace(host=None, port=None), + run=MagicMock(), + ) + + def fake_build_server(*, allow_local_targets: bool = False): + captured["allow_local_targets"] = allow_local_targets + return server + + monkeypatch.setattr(mcp_server, "build_server", fake_build_server) + + with pytest.raises(ValueError, match="transport must be"): + mcp_server.run(transport="sse") + + assert captured["allow_local_targets"] is False + server.run.assert_not_called() + + async def test_build_server_registers_scan_skill() -> None: """build_server wires up the scan_skill tool (requires the mcp extra).""" pytest.importorskip("mcp")