diff --git a/README.md b/README.md index 5da1b7d6a..7c1042ca9 100644 --- a/README.md +++ b/README.md @@ -476,6 +476,12 @@ claude mcp add skillspector -- skillspector mcp > - Local paths and `file://` URLs are **automatically rejected** over HTTP to > prevent unauthenticated callers from reading arbitrary host files. Only > remote Git and `.zip` URLs are accepted. +> +> HTTP scans accept only credential-free `https://` targets. Git scans never use +> the server's Git login or Git config. If a proxy or custom CA is needed, set +> `HTTPS_PROXY`, `NO_PROXY` or `GIT_SSL_CAINFO` in the server environment. +> Use stdio or the CLI for private repositories. Git redirects are rejected; +> use the current repository URL after a rename or transfer. ## Vulnerability Patterns diff --git a/src/skillspector/input_handler.py b/src/skillspector/input_handler.py index 767315f66..847b4ba6a 100644 --- a/src/skillspector/input_handler.py +++ b/src/skillspector/input_handler.py @@ -767,7 +767,10 @@ class InputHandler: Normalizes all inputs to a local directory path for scanning. """ - def __init__(self, transitive_budget: object | None = None) -> None: + def __init__( + self, transitive_budget: object | None = None, *, allow_git_credentials: bool = True + ) -> None: + self._allow_git_credentials = allow_git_credentials self._temp_dir: Path | None = None self._transitive_budget = transitive_budget self.primary_file_path: str | None = None @@ -1129,23 +1132,99 @@ def _resolve_tree_ref(self, repository_url: str, segments: list[str]) -> tuple[s f"{repository_url} ({'/'.join(segments)})" ) + def _git_invocation(self, url: str) -> tuple[list[str], dict[str, str] | None, str]: + """Build a Git policy for both ref discovery and cloning. + + Remote MCP callers must not borrow the server's SSH keys, helpers, + authorization headers, URL rewrites, client certificates, or netrc. + Trusted local callers retain the operator's configured Git identity. + """ + if self._allow_git_credentials: + return ["git"], None, url + parsed = urlparse(url) + if parsed.scheme != "https" or parsed.username is not None or parsed.password is not None: + raise ValueError("Untrusted Git scans require an unauthenticated HTTPS URL") + if parsed.hostname in {"gitlab.com", "bitbucket.org"}: + # Browser URLs need the Git endpoint without following a redirect + # whose destination has not passed the host/IP checks. + path = parsed.path.rstrip("/") + if path and not path.endswith(".git"): + path += ".git" + url = parsed._replace(path=path).geturl() + isolated_home = self._get_temp_dir() / "git-home" + isolated_home.mkdir(mode=0o700, exist_ok=True) + env = { + key: value + for key, value in os.environ.items() + if key.upper() + in { + "PATH", + "SYSTEMROOT", + "WINDIR", + "TEMP", + "TMP", + "TMPDIR", + "HTTP_PROXY", + "HTTPS_PROXY", + "ALL_PROXY", + "NO_PROXY", + "GIT_SSL_CAINFO", + "GIT_SSL_CAPATH", + "SSL_CERT_FILE", + "SSL_CERT_DIR", + "CURL_CA_BUNDLE", + } + } + env.update( + HOME=str(isolated_home), + USERPROFILE=str(isolated_home), + XDG_CONFIG_HOME=str(isolated_home), + GIT_CONFIG_GLOBAL=os.devnull, + GIT_CONFIG_SYSTEM=os.devnull, + GIT_CONFIG_NOSYSTEM="1", + GIT_TERMINAL_PROMPT="0", + GIT_ALLOW_PROTOCOL="https", + GIT_CEILING_DIRECTORIES=str(isolated_home.parent), + ) + return ( + [ + "git", + "-C", + str(isolated_home), + "-c", + "credential.helper=", + "-c", + "core.askPass=", + "-c", + f"core.hooksPath={os.devnull}", + "-c", + "http.extraHeader=", + "-c", + "http.followRedirects=false", + ], + env, + url, + ) + def _list_remote_refs(self, repository_url: str) -> set[str]: """Return the branch/tag names advertised by the remote repository. Bounded by the ingest deadline; the host allowlist and private-IP checks from URL validation apply. """ + git_argv, git_env, repository_url = self._git_invocation(repository_url) self._validate_url_host(repository_url, ALLOWED_GIT_HOSTS) deadline = self._deadline() self._check_deadline(deadline, "git") timeout = max(1.0, deadline - monotonic()) try: process = subprocess.run( - ["git", "ls-remote", repository_url], + [*git_argv, "ls-remote", repository_url], stdout=subprocess.PIPE, stderr=subprocess.DEVNULL, timeout=timeout, check=False, + **({"env": git_env} if git_env is not None else {}), ) except subprocess.TimeoutExpired as exc: raise IngestLimitExceededError("Git ref listing exceeded its time limit") from exc @@ -1206,6 +1285,7 @@ def _clone_git(self, url: str, *, branch: str | None = None) -> Path: self._truncate("byte_budget_exhausted", "git") if remaining_artifacts is not None and remaining_artifacts <= 0: self._truncate("artifact_budget_exhausted", "git") + git_argv, git_env, url = self._git_invocation(url) self._validate_url_host(url, ALLOWED_GIT_HOSTS) deadline = self._deadline() self._check_deadline(deadline, "git") @@ -1225,6 +1305,7 @@ def _clone_git(self, url: str, *, branch: str | None = None) -> Path: clone_command[6:6] = ["--branch", branch] if remaining_bytes is not None: clone_command.insert(6, f"--filter=blob:limit={remaining_bytes}") + clone_command[:1] = git_argv process: subprocess.Popen[bytes] | None = None final_measurement: _TreeMeasurement | None = None try: @@ -1233,6 +1314,7 @@ def _clone_git(self, url: str, *, branch: str | None = None) -> Path: stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, shell=False, + **({"env": git_env} if git_env is not None else {}), ) while True: self._check_deadline(deadline, "git") diff --git a/src/skillspector/mcp_server.py b/src/skillspector/mcp_server.py index 22b034dd2..882223e1b 100644 --- a/src/skillspector/mcp_server.py +++ b/src/skillspector/mcp_server.py @@ -31,6 +31,7 @@ from ipaddress import ip_address from pathlib import Path from typing import TYPE_CHECKING, Any +from urllib.parse import urlparse from skillspector import __version__ from skillspector.cleanup import TempDirTracker, cleanup_result @@ -116,7 +117,7 @@ async def run_scan( :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. + accept caller-controlled local paths or use ambient Git credentials. yara_rules_dir: Optional directory of additional YARA rules. Returns: @@ -133,12 +134,20 @@ async def run_scan( 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") + parsed_target = urlparse(target.strip()) + if ( + parsed_target.scheme != "https" + or parsed_target.username is not None + or parsed_target.password is not None + ): + raise ValueError("this MCP transport requires an unauthenticated HTTPS target") llm_preflight_available, _ = is_llm_available() llm_enabled = use_llm and llm_preflight_available state: dict[str, Any] = { "input_path": target, + "allow_git_credentials": allow_local_targets, "output_format": output_format, "use_llm": llm_enabled, "llm_requested": use_llm, diff --git a/src/skillspector/nodes/resolve_input.py b/src/skillspector/nodes/resolve_input.py index cead1255a..b153b1246 100644 --- a/src/skillspector/nodes/resolve_input.py +++ b/src/skillspector/nodes/resolve_input.py @@ -56,7 +56,10 @@ def resolve_input(state: SkillspectorState) -> dict[str, object]: workflow_budget = ensure_workflow_resource_budget(state) if input_path and isinstance(input_path, str) and input_path.strip(): - handler = InputHandler(transitive_budget=workflow_budget) + handler = InputHandler( + transitive_budget=workflow_budget, + allow_git_credentials=state.get("allow_git_credentials", True), + ) try: resolved, source_type = handler.resolve(input_path.strip()) temp_dir = handler.temp_dir_for_cleanup() diff --git a/src/skillspector/state.py b/src/skillspector/state.py index fdd43b246..2bb310a50 100644 --- a/src/skillspector/state.py +++ b/src/skillspector/state.py @@ -235,6 +235,8 @@ class SkillspectorState(TypedDict, total=False): # Input: resolve_input node consumes input_path or skill_path, sets skill_path input_path: str | None + # Only trusted local transports may use the operator's Git credentials. + allow_git_credentials: bool skill_path: str | None # Relative identity of a wrapped/downloaded explicit file, distinct from passive assets. primary_file_path: str | None diff --git a/tests/nodes/test_resolve_input.py b/tests/nodes/test_resolve_input.py index e8af70f95..26fde723b 100644 --- a/tests/nodes/test_resolve_input.py +++ b/tests/nodes/test_resolve_input.py @@ -91,7 +91,10 @@ def test_workflow_budget_starts_before_input_materialization( class CapturingHandler: primary_file_path = None - def __init__(self, transitive_budget: object | None = None) -> None: + def __init__( + self, transitive_budget: object | None = None, *, allow_git_credentials: bool = True + ) -> None: + assert allow_git_credentials is True assert transitive_budget is not None assert getattr(transitive_budget, "started_at", None) is not None captured.append(transitive_budget) @@ -116,7 +119,10 @@ def test_transitive_truncation_is_typed_sanitized_and_cleaned( cleaned: list[bool] = [] class TruncatedHandler: - def __init__(self, transitive_budget: object | None = None) -> None: + def __init__( + self, transitive_budget: object | None = None, *, allow_git_credentials: bool = True + ) -> None: + assert allow_git_credentials is True assert transitive_budget is not None def resolve(self, _input_path: str) -> tuple[Path, str]: @@ -323,3 +329,13 @@ def wait(self, timeout: float | None = None) -> int: assert [clone.terminated for clone in InterruptedClone.instances] == [True] assert created assert [path for path in created if path.exists()] == [] + + +def test_resolve_input_preserves_anonymous_git_policy() -> None: + with pytest.raises(ValueError, match="unauthenticated HTTPS"): + resolve_input( + { + "input_path": "git@github.com:org/private.git", + "allow_git_credentials": False, + } + ) diff --git a/tests/unit/test_input_handler.py b/tests/unit/test_input_handler.py index dbd3abe9b..bb75ae7ef 100644 --- a/tests/unit/test_input_handler.py +++ b/tests/unit/test_input_handler.py @@ -17,6 +17,7 @@ import ctypes import os +import subprocess import sys from errno import ENOENT from pathlib import Path, PurePosixPath @@ -701,3 +702,171 @@ def test_scp_ssrf_gate_fires() -> None: with patch("skillspector.input_handler._is_private_ip", return_value=True): with pytest.raises(ValueError, match="private/internal IP"): InputHandler()._validate_url_host("git@github.com:org/repo.git", ALLOWED_GIT_HOSTS) + + +@pytest.mark.parametrize("operation", ["_clone_git", "_list_remote_refs"]) +def test_anonymous_git_drops_ambient_auth_and_config( + operation: str, monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + hostile = { + "HOME": str(tmp_path), + "SSH_AUTH_SOCK": "/synthetic/agent", + "GIT_ASKPASS": "/synthetic/askpass", + "GIT_CONFIG_COUNT": "1", + "GIT_CONFIG_KEY_0": "http.extraHeader", + "GIT_CONFIG_VALUE_0": "Authorization: synthetic-secret", + "GIT_CONFIG_GLOBAL": str(tmp_path / "credentials.config"), + "GITHUB_TOKEN": "synthetic-token", + "CURL_HOME": str(tmp_path), + "SSLKEYLOGFILE": str(tmp_path / "tls-keys"), + "GIT_SSL_CERT": "/synthetic/client.pem", + "GIT_SSL_KEY": "/synthetic/client.key", + "GIT_CEILING_DIRECTORIES": "/synthetic/ceiling", + } + transport = { + key: "/synthetic/ca" + if "CA" in key or key.startswith("SSL_") + else "http://proxy.example:8080" + for key in ( + "HTTPS_PROXY", + "https_proxy", + "HTTP_PROXY", + "http_proxy", + "ALL_PROXY", + "all_proxy", + "NO_PROXY", + "no_proxy", + "GIT_SSL_CAINFO", + "GIT_SSL_CAPATH", + "SSL_CERT_FILE", + "SSL_CERT_DIR", + "CURL_CA_BUNDLE", + ) + } + for key, value in (hostile | transport).items(): + monkeypatch.setenv(key, value) + handler = InputHandler(allow_git_credentials=False) + calls: list[tuple[list[str], dict]] = [] + + def fake_git(command: list[str], **kwargs: object) -> MagicMock: + calls.append((command, kwargs)) + if "clone" in command: + Path(command[-1]).mkdir(parents=True) + process = MagicMock() + process.poll.return_value = 0 + process.wait.return_value = 0 + process.returncode = 0 + process.stdout = b"abc\trefs/heads/main\n" + return process + + try: + with ( + patch.object(handler, "_validate_url_host", return_value="github.com"), + patch("skillspector.input_handler.subprocess.Popen", side_effect=fake_git), + patch("skillspector.input_handler.subprocess.run", side_effect=fake_git), + ): + getattr(handler, operation)("https://github.com/org/public.git") + assert len(calls) == 1 + command, kwargs = calls[0] + env = kwargs["env"] + isolated_home = handler.temp_dir_for_cleanup() / "git-home" + assert command[:3] == ["git", "-C", str(isolated_home)] + assert "credential.helper=" in command + assert "core.askPass=" in command + assert "http.extraHeader=" in command + assert "http.followRedirects=false" in command + assert env["HOME"] == env["USERPROFILE"] == str(isolated_home) + assert env["GIT_CONFIG_GLOBAL"] == env["GIT_CONFIG_SYSTEM"] == os.devnull + assert env["GIT_CONFIG_NOSYSTEM"] == "1" + assert env["GIT_ALLOW_PROTOCOL"] == "https" + assert env["GIT_CEILING_DIRECTORIES"] == str(isolated_home.parent) + assert {key: env[key] for key in transport} == transport + assert env["GIT_TERMINAL_PROMPT"] == "0" + assert list(isolated_home.iterdir()) == [] + for key in hostile.keys() - {"HOME", "GIT_CONFIG_GLOBAL", "GIT_CEILING_DIRECTORIES"}: + assert key not in env + assert os.environ["HOME"] == str(tmp_path) + finally: + handler.cleanup() + + +@pytest.mark.parametrize("operation", ["_clone_git", "_list_remote_refs"]) +@pytest.mark.parametrize( + "target", + [ + "git@github.com:org/private.git", + "ssh://git@github.com/org/private.git", + "https://user:password@github.com/org/private.git", + ], +) +def test_anonymous_git_rejects_auth_targets_before_start(operation: str, target: str) -> None: + handler = InputHandler(allow_git_credentials=False) + with ( + patch("skillspector.input_handler.subprocess.Popen") as popen, + patch("skillspector.input_handler.subprocess.run") as run, + ): + with pytest.raises(ValueError, match="unauthenticated HTTPS"): + getattr(handler, operation)(target) + popen.assert_not_called() + run.assert_not_called() + assert handler.temp_dir_for_cleanup() is None + + +@pytest.mark.parametrize("operation", ["_clone_git", "_list_remote_refs"]) +@pytest.mark.parametrize( + "target, expected", + [ + ("https://gitlab.com/group/subgroup/repo", "https://gitlab.com/group/subgroup/repo.git"), + ("https://gitlab.com/group/repo.git/", "https://gitlab.com/group/repo.git"), + ("https://bitbucket.org/team/repo/", "https://bitbucket.org/team/repo.git"), + ("https://github.com/org/repo", "https://github.com/org/repo"), + ], +) +def test_anonymous_git_normalizes_repository_urls_without_redirects(operation, target, expected): + handler = InputHandler(allow_git_credentials=False) + calls = [] + + def fake_git(command, **kwargs): + calls.append(command) + if "clone" in command: + Path(command[-1]).mkdir(parents=True) + return SimpleNamespace(poll=lambda: 0, returncode=0, stdout=b"abc\trefs/heads/main\n") + + try: + with ( + patch.object(handler, "_validate_url_host"), + patch("skillspector.input_handler.subprocess.Popen", side_effect=fake_git), + patch("skillspector.input_handler.subprocess.run", side_effect=fake_git), + ): + getattr(handler, operation)(target) + assert expected in calls[0] + assert "http.followRedirects=false" in calls[0] + finally: + handler.cleanup() + + +def test_anonymous_git_cannot_read_enclosing_repository_config(tmp_path): + subprocess.run(["git", "init", "--quiet", str(tmp_path)], check=True) + subprocess.run( + [ + "git", + "-C", + str(tmp_path), + "config", + "http.https://github.com/.extraheader", + "synthetic-header", + ], + check=True, + ) + handler = InputHandler(allow_git_credentials=False) + with patch.object(handler, "_get_temp_dir", return_value=tmp_path / "scan"): + (tmp_path / "scan").mkdir() + argv, env, _ = handler._git_invocation("https://github.com/org/repo") + result = subprocess.run( + [*argv, "config", "--get-urlmatch", "http.extraheader", "https://github.com/org/repo"], + env=env, + capture_output=True, + check=False, + ) + assert b"synthetic-header" not in result.stdout + assert result.stdout.strip() == b"" diff --git a/tests/unit/test_mcp_server.py b/tests/unit/test_mcp_server.py index 277d0e84a..bb8ba96b1 100644 --- a/tests/unit/test_mcp_server.py +++ b/tests/unit/test_mcp_server.py @@ -1142,6 +1142,26 @@ def fail_to_expanduser(self: Path) -> Path: assert mcp_server._is_local_target("~nosuchuser/skill") is True +@pytest.mark.parametrize( + "target", + [ + "git@github.com:org/private.git", + "ssh://git@github.com/org/private.git", + "git+ssh://git@github.com/org/private.git", + "https://token@github.com/org/private.git", + "https://user:password@github.com/org/private.git", + ], +) +async def test_http_rejects_credentialed_targets_before_graph( + target: str, monkeypatch: pytest.MonkeyPatch +) -> None: + graph_ainvoke = AsyncMock() + monkeypatch.setattr(mcp_server.graph, "ainvoke", graph_ainvoke) + with pytest.raises(ValueError, match="unauthenticated HTTPS"): + await run_scan(target, allow_local_targets=False) + graph_ainvoke.assert_not_awaited() + + async def test_run_scan_allows_remote_target_when_local_targets_disallowed( monkeypatch: pytest.MonkeyPatch, ) -> None: @@ -1164,6 +1184,7 @@ async def test_run_scan_allows_remote_target_when_local_targets_disallowed( assert result["target"] == target assert graph_ainvoke.await_count == 1 assert graph_ainvoke.await_args.args[0]["input_path"] == target + assert graph_ainvoke.await_args.args[0]["allow_git_credentials"] is False async def test_run_scan_keeps_default_local_target_compatibility( @@ -1187,6 +1208,7 @@ async def test_run_scan_keeps_default_local_target_compatibility( assert result["target"] == str(tmp_path) assert graph_ainvoke.await_count == 1 assert graph_ainvoke.await_args.args[0]["input_path"] == str(tmp_path) + assert graph_ainvoke.await_args.args[0]["allow_git_credentials"] is True @pytest.mark.parametrize(