diff --git a/CHANGELOG.md b/CHANGELOG.md index 6c379b6..8ed43b1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,8 +30,18 @@ Streamable HTTP; the v0.3 `guard` proxy adds deterministic runtime *result* insp ## [Unreleased] -_Nothing yet._ +### Fixed +- **`doctor` follow-ups from the security review of #98 (DSE-1529).** `#servers` is no + longer a reserved namespace: config entries are keyed on `(map, name)` and the display + key is derived afterwards, unique by construction, so a server the user really named + `x#servers` is neither overwritten by the synthetic `mcpServers`/`servers` collision + key nor falsely flagged `WRD-DOCTOR-AMBIGUOUS-SERVER`; ambiguity is recorded per + display key. `strip_jsonc` is one string-aware linear pass (the regex form + back-tracked once per unterminated `/*` and went quadratic on a hostile VS Code + `mcp.json`). `safe_text` also neutralises the soft hyphen, `U+2060`–`U+2064`, the BOM, + and the Unicode Tags block. `docs/DOCTOR.md` now documents `--config` resolved-path + de-duplication / explicit-replaces-discovered and the `safe_text` caveat on printed URLs. ## [1.2.0] — 2026-09-04 ### Added diff --git a/DOCUMENTATION_INDEX.md b/DOCUMENTATION_INDEX.md index 84b0a4e..2258cab 100644 --- a/DOCUMENTATION_INDEX.md +++ b/DOCUMENTATION_INDEX.md @@ -58,6 +58,7 @@ lock-coverage check per server, then prints the exact `pin` command for anything | [`docs/DOCTOR.md`](docs/DOCTOR.md) | Contract — the per-platform discovery table, the symlink-escape rule, what runs per server, lock-coverage matching, the static-by-default guarantee, the `--pin` opt-in contract, exit codes | | [`src/mcp_warden/doctor_paths.py`](src/mcp_warden/doctor_paths.py) | Pure path enumeration for (platform, home, cwd, env): the per-platform well-known set, the `.git`/home-bounded project walk-up, the symlink-component guard | | [`src/mcp_warden/doctor_discovery.py`](src/mcp_warden/doctor_discovery.py) | Fail-closed loaders — union of `mcpServers` + VS Code `servers` (conflicts → `WRD-DOCTOR-AMBIGUOUS-SERVER`), `~/.claude.json` per-project, Codex TOML, string-aware JSONC; per-candidate hard-error / skip accounting | +| [`src/mcp_warden/doctor_jsonc.py`](src/mcp_warden/doctor_jsonc.py) | `strip_jsonc` — one linear, string-aware pass over VS Code JSONC (comments + trailing commas); an unterminated string or `/*` leaves the remainder untouched so it fails closed (DSE-1529) | | [`src/mcp_warden/doctor_funnel.py`](src/mcp_warden/doctor_funnel.py) | `safe_text` (C0/C1/NEL/zero-width/bidi neutralisation), lock naming, the masking rules and byte-preserving URL redaction behind the printed `pin` command | | [`src/mcp_warden/doctor.py`](src/mcp_warden/doctor.py) | Bounded lock discovery, `pin.approved`-aware coverage, per-server composition of `audit_server` + `check_launch_command` + `WRD-DOCTOR-*`, resolved-path de-duplication of `--config` | | [`src/mcp_warden/cli_doctor.py`](src/mcp_warden/cli_doctor.py) | `doctor` command body (register idiom): report rendering, JSONL/SARIF, the `--pin`/`--yes` TTY contract | diff --git a/docs/DOCTOR.md b/docs/DOCTOR.md index e4e1d5e..97d4a35 100644 --- a/docs/DOCTOR.md +++ b/docs/DOCTOR.md @@ -41,11 +41,15 @@ Every JSON file is loaded as the **union** of its `mcpServers` and `servers` map Code reads `servers`; if a file also carried a benign `mcpServers` and only that map were audited, a decoy could hide the map the client actually loads. A name present under both keys with an identical body is loaded once; with *different* bodies both -are audited (the second as `#servers`) and each is flagged -`WRD-DOCTOR-AMBIGUOUS-SERVER` (medium). The JSONC pass (VS Code) is string-aware in -**both** of its stages — comment stripping and trailing-comma removal — so a value -such as `"echo {a, }"` is audited and pinned byte-for-byte as the client launches -it. On Windows the two `%APPDATA%` entries +are audited and each is flagged `WRD-DOCTOR-AMBIGUOUS-SERVER` (medium). Entries are +keyed on `(map, name)` and the display key is derived afterwards, unique by +construction: the `servers` copy is shown as `#servers` with `#` appended until +the key is free, so `#servers` is **not** a reserved namespace — a server the user +really named `x#servers` is never overwritten by the synthetic key and, on its own, is +never flagged ambiguous. The JSONC pass (VS Code) is a single string-aware, +left-to-right scan — comments and trailing commas are handled in one linear pass — so +a value such as `"echo {a, }"` is audited and pinned byte-for-byte as the client +launches it, and a hostile file full of unterminated `/*` cannot wedge the scan. On Windows the two `%APPDATA%` entries are simply absent when `APPDATA` is unset — the location is never guessed. ### Project walk-up boundary @@ -62,7 +66,10 @@ world-writable `/tmp/.mcp.json`. A discovered path with a **symlink at any component below its base** (the home directory for user-level entries, the ancestor directory for project-level entries) is **skipped with a stderr warning** and its target is never read. An explicit -`--config PATH` is trusted as given and may be a symlink. +`--config PATH` is trusted as given and may be a symlink. `--config` paths are +de-duplicated by **resolved** path (a symlink and its target are one file, scanned +once with a warning), and an explicit path **replaces** the discovered source for the +same file — naming it is what makes it `--pin`-eligible (§4). ### Size cap @@ -103,7 +110,9 @@ buffer can be tricked by becomes `U+FFFD` — C0 + DEL (`U+0000`–`U+001F`, `U+ the C1 range (`U+0080`–`U+009F`; `U+009B` is CSI and `U+009D` is OSC on xterm-family terminals), NEL, the zero-width and directional marks (`U+200B`–`U+200F`), the line and paragraph separators (`U+2028`, `U+2029`), and both bidi-override blocks -(`U+202A`–`U+202E`, `U+2066`–`U+2069` — Trojan Source, CVE-2021-42574) — and the +(`U+202A`–`U+202E`, `U+2066`–`U+2069` — Trojan Source, CVE-2021-42574), the soft +hyphen (`U+00AD`), the word joiner and invisible operators (`U+2060`–`U+2064`), the +BOM (`U+FEFF`), and the Unicode Tags block (`U+E0000`–`U+E007F`) — and the string is capped at 200 characters. Rich markup is then escaped where markup is on, and the JSONL / pin blocks print with `markup=False`. A server named `gh\n mcp-warden pin sh -c '…'` therefore renders as one line with a visible `�`, @@ -166,7 +175,10 @@ launch lines carry credentials in shapes the config audit never sees: that is auth-shaped (`#token=…`) or token-like is replaced. Scheme and host stay visible. When nothing needs masking the URL is printed **byte-for-byte** as it appears in the config — never re-encoded — so the lock the user pins from the - printed command is the lock `doctor` recognises on the next run. + printed command is the lock `doctor` recognises on the next run. "Byte-for-byte" is +strictly *after* `safe_text()`: a URL carrying a neutralised character or longer than +2048 characters prints differently from the config, and the lock pinned from that +printed line will not match `lock_covers` — fix the config value first. When two server names slug to the same file (`a/b` and `a-b`), the second gets a short hash suffix. The block ends with the GitHub Action snippet. diff --git a/src/mcp_warden/doctor.py b/src/mcp_warden/doctor.py index 2f7fc0d..7dbefdb 100644 --- a/src/mcp_warden/doctor.py +++ b/src/mcp_warden/doctor.py @@ -146,10 +146,9 @@ def scan_server( if isinstance(command, str) and command: args = [str(a) for a in (server.get("args") or [])] findings += _retarget(check_launch_command(command, args), target) - base = name[: -len("#servers")] if name.endswith("#servers") else name - if base in source.ambiguous: + if name in source.ambiguous: # display keys, not a suffix rule: a real `x#servers` is never flagged findings.append(_finding( - "WRD-DOCTOR-AMBIGUOUS-SERVER", "medium", target, base, + "WRD-DOCTOR-AMBIGUOUS-SERVER", "medium", target, name, "declared under both mcpServers and servers with different definitions; a decoy map " "can hide the one the client loads from an audit — keep exactly one", )) diff --git a/src/mcp_warden/doctor_discovery.py b/src/mcp_warden/doctor_discovery.py index 5aceb73..d749461 100644 --- a/src/mcp_warden/doctor_discovery.py +++ b/src/mcp_warden/doctor_discovery.py @@ -20,13 +20,13 @@ from __future__ import annotations import json -import re import tomllib from collections.abc import Callable, Mapping from dataclasses import dataclass, field from pathlib import Path from typing import Any +from .doctor_jsonc import strip_jsonc from .doctor_paths import ( FMT_CLAUDE_JSON, FMT_CODEX_TOML, @@ -53,9 +53,10 @@ class ConfigSource: """One discovered (or explicitly given) map of MCP servers. - ``ambiguous`` names every server declared under *both* ``mcpServers`` and - ``servers`` with different definitions; the second definition is kept - under ``#servers`` so both get audited and the collision is reported. + ``ambiguous`` holds the display keys of every entry declared under *both* + ``mcpServers`` and ``servers`` with different definitions; the ``servers`` + copy is kept under a unique ``#servers…`` key so both get audited and + the collision is reported on each. """ client: str @@ -84,55 +85,52 @@ class DoctorError(ValueError): """Raised on an unreadable or malformed config (fail closed, exit 2).""" -# --- JSONC --------------------------------------------------------------------- - -_STRING = r'"(?:[^"\\]|\\.)*"' -_COMMENT_PASS = re.compile(rf"{_STRING}|//[^\n]*|/\*.*?\*/", re.S) -_COMMA_PASS = re.compile(rf"{_STRING}|,(\s*[}}\]])", re.S) - - -def strip_jsonc(text: str) -> str: - """Remove ``//`` / ``/* */`` comments and trailing commas — outside strings only. - - Both passes tokenise string literals first and return them verbatim, so a - ``"//"`` or ``"echo {a, }"`` inside a value is never touched. Anything the - tokeniser cannot pair degrades to invalid JSON, which fails closed. - """ - no_comments = _COMMENT_PASS.sub(lambda m: m.group(0) if m.group(0)[0] == '"' else " ", text) - return _COMMA_PASS.sub(lambda m: m.group(0) if m.group(0)[0] == '"' else m.group(1), no_comments) +# --- loaders ------------------------------------------------------------------- -# --- loaders ------------------------------------------------------------------- +_MAPS = ("mcpServers", "servers") def _servers_from_doc(doc: Any, where: str) -> tuple[dict[str, dict[str, Any]], tuple[str, ...]]: - """Extract ``{name: server}`` from a parsed document — the UNION of both keys. - - A name present under both ``mcpServers`` and ``servers`` with an identical - body is loaded once. With *different* bodies both are kept — the second as - ``#servers`` — and the name is returned as ambiguous: a decoy map is - itself a signal (``WRD-DOCTOR-AMBIGUOUS-SERVER``). + """Extract ``{display: server}`` from a parsed document — the UNION of both maps. + + Entries are keyed on ``(map, name)`` first; the display key is derived + afterwards so it is *unique by construction*: a name present under both + ``mcpServers`` and ``servers`` with an identical body is loaded once; with + *different* bodies both are kept — the ``servers`` copy as ``#servers``, + with ``#`` appended until the key is free, so a server the user really named + ``x#servers`` is never overwritten. The returned ``ambiguous`` tuple holds the + **display keys** of every colliding entry (not a bare-name suffix rule), so a + user-named ``x#servers`` on its own is never flagged (CSO review of #98, N8). """ if not isinstance(doc, dict): raise DoctorError(f"{where}: top-level config must be an object") - out: dict[str, dict[str, Any]] = {} - ambiguous: list[str] = [] - for key in ("mcpServers", "servers"): + entries: dict[tuple[str, str], dict[str, Any]] = {} + for key in _MAPS: raw = doc.get(key) if raw is None: continue if not isinstance(raw, dict): raise DoctorError(f"{where}: {key} must be an object") for k, v in raw.items(): - if not isinstance(v, dict): - continue - name = str(k) - if name in out: - if out[name] == v: - continue - ambiguous.append(name) - name = f"{name}#servers" - out[name] = v + if isinstance(v, dict): + entries[(key, str(k))] = v + out: dict[str, dict[str, Any]] = {} + ambiguous: list[str] = [] + for (map_key, name), body in entries.items(): + other = entries.get((_MAPS[0], name)) if map_key == _MAPS[1] else None + if other is not None and other == body: + continue # identical under both maps: one server, loaded once + display = name + if other is not None: + display = f"{name}#servers" + while display in out: + display += "#" + ambiguous += [name, display] # mcpServers entries load first, under their own name + elif display in out: # a real name colliding with a synthetic key: keep both + while display in out: + display += "#" + out[display] = body return out, tuple(ambiguous) diff --git a/src/mcp_warden/doctor_funnel.py b/src/mcp_warden/doctor_funnel.py index 4f3f2c1..00abb49 100644 --- a/src/mcp_warden/doctor_funnel.py +++ b/src/mcp_warden/doctor_funnel.py @@ -21,9 +21,15 @@ #: Everything a terminal or a copy buffer can be tricked by: C0 + DEL, the C1 #: range (U+009B is CSI and U+009D is OSC on xterm-family terminals in UTF-8 #: mode), NEL, zero-width and directional marks (U+200B–U+200F), the Unicode -#: line/paragraph separators, and both bidi-override blocks (Trojan Source, -#: CVE-2021-42574 — a reordered ``pin`` line still runs in source order). -_CONTROL = re.compile(r"[\x00-\x1f\x7f-\x9f\u200b-\u200f\u2028\u2029\u202a-\u202e\u2066-\u2069]") +#: line/paragraph separators, both bidi-override blocks (Trojan Source, +#: CVE-2021-42574 — a reordered ``pin`` line still runs in source order), and +#: the remaining invisibles: soft hyphen, word joiner + the invisible operators +#: (U+2060–U+2064), the BOM / zero-width no-break space, and the Unicode Tags +#: block (U+E0000–U+E007F), which most terminals render as nothing at all. +_CONTROL = re.compile( + r"[\x00-\x1f\x7f-\x9f\u00ad\u200b-\u200f\u2028\u2029\u202a-\u202e\u2060-\u2064" + r"\u2066-\u2069\ufeff\U000e0000-\U000e007f]" +) def safe_text(s: object, max_len: int = 200) -> str: diff --git a/src/mcp_warden/doctor_jsonc.py b/src/mcp_warden/doctor_jsonc.py new file mode 100644 index 0000000..531f340 --- /dev/null +++ b/src/mcp_warden/doctor_jsonc.py @@ -0,0 +1,80 @@ +"""JSONC → JSON for VS Code ``mcp.json`` — one linear, string-aware pass (DSE-1516/1529). + +Split from ``doctor_discovery.py`` to keep that module under the LOC budget. +VS Code permits ``//`` and ``/* */`` comments and trailing commas; a client +loads such a file happily, so ``doctor`` must audit exactly what it loads. +""" + +from __future__ import annotations + +import re + +__all__ = ["strip_jsonc"] + +_STRING_END = re.compile(r'(?:[^"\\]|\\.)*"') + + +def strip_jsonc(text: str) -> str: + """Remove ``//`` / ``/* */`` comments and trailing commas — outside strings only. + + One left-to-right, string-aware pass, linear in the input: a string literal + is copied verbatim (so a ``"//"`` or ``"echo {a, }"`` inside a value is never + touched), a comment becomes a single space, and a ``,`` is held back until + the next token shows whether it is trailing. An unterminated string or + ``/*`` leaves the remainder untouched, which is invalid JSON and fails + closed — the earlier regex form back-tracked once per unterminated ``/*`` + and went quadratic on a hostile file (CSO review of #98, N9). + """ + out: list[str] = [] + pending_comma = False # a ',' seen, not yet emitted: dropped if '}' / ']' follows + i, n = 0, len(text) + while i < n: + ch = text[i] + if ch == '"': + m = _STRING_END.match(text, i + 1) + if m is None: # unterminated: leave the rest as-is (invalid JSON) + if pending_comma: + out.append(",") + out.append(text[i:]) + break + end = m.end() + if pending_comma: + out.append(",") + pending_comma = False + out.append(text[i:end]) + i = end + continue + if ch == "/" and text.startswith("//", i): + nl = text.find("\n", i) + out.append(" ") + i = n if nl == -1 else nl + continue + if ch == "/" and text.startswith("/*", i): + close = text.find("*/", i + 2) + if close == -1: # unterminated block comment: leave the rest as-is + if pending_comma: + out.append(",") + out.append(text[i:]) + break + out.append(" ") + i = close + 2 + continue + if ch == ",": + if pending_comma: + out.append(",") + pending_comma = True + i += 1 + continue + if ch in " \t\r\n": + out.append(ch) + i += 1 + continue + if pending_comma: + if ch not in "}]": + out.append(",") + pending_comma = False + out.append(ch) + i += 1 + if pending_comma: + out.append(",") + return "".join(out) diff --git a/tests/test_doctor_security.py b/tests/test_doctor_security.py index 2a4a459..99a4e84 100644 --- a/tests/test_doctor_security.py +++ b/tests/test_doctor_security.py @@ -13,6 +13,7 @@ import json import subprocess import sys +import time from pathlib import Path import pytest @@ -324,3 +325,60 @@ def test_explicit_config_is_deduped_by_resolved_path(tmp_path, monkeypatch): rows = [line for line in r.output.splitlines() if line.startswith("{")] assert sum("WRD-AUTH-PLAINTEXT-HTTP" in row for row in rows) == 1 assert "scanning it once" in r.output + + +# --- DSE-1529 N8: `#servers` is not a reserved namespace --------------------------------- + + +def test_user_named_hash_servers_entry_is_neither_dropped_nor_flagged(tmp_path): + p = tmp_path / "m.json" + p.write_text(json.dumps({ + "mcpServers": {"gh": {"url": "https://a.example.com"}, "gh#servers": {"url": "https://m.example.com"}}, + "servers": {"gh": {"url": "https://b.example.com"}}, + })) + src = load_config("c", p, FMT_JSON, "m")[0] + # All three bodies survive: the real `gh#servers` keeps its key, the synthetic one gets `#` appended. + assert src.servers["gh#servers"] == {"url": "https://m.example.com"} + assert src.servers["gh#servers#"] == {"url": "https://b.example.com"} + assert src.servers["gh"] == {"url": "https://a.example.com"} + # Only the colliding pair is ambiguous; the user-named `gh#servers` is not. + assert set(src.ambiguous) == {"gh", "gh#servers#"} + + +def test_user_named_hash_servers_alone_is_not_flagged_ambiguous(tmp_path, monkeypatch): + monkeypatch.chdir(tmp_path) + home = _home_with(tmp_path, {"x#servers": {"url": "https://h.example.com", "headers": {"Authorization": "Bearer ${T}"}}}) + r = _run("--json", home=home) + assert "WRD-DOCTOR-AMBIGUOUS-SERVER" not in r.output and "x#servers" in r.output + + +# --- DSE-1529 N9: strip_jsonc is linear on a hostile file --------------------------------- + + +def test_strip_jsonc_is_linear_on_unterminated_block_comments(): + hostile = "/* " * 200_000 # ~600 KB of openers with no `*/` anywhere: the old regex went quadratic here + t0 = time.perf_counter() + out = strip_jsonc(hostile) + assert time.perf_counter() - t0 < 2.0 + assert out == hostile # left untouched -> invalid JSON -> fails closed + with pytest.raises(json.JSONDecodeError): + json.loads(out) + + +def test_strip_jsonc_single_pass_handles_comments_between_comma_and_close(): + src = '{"a": [1, 2, /* c */ ], "b": {"k": "v", // trailing\n}, "s": "/* not */ // not", }' + assert json.loads(strip_jsonc(src)) == {"a": [1, 2], "b": {"k": "v"}, "s": "/* not */ // not"} + assert strip_jsonc('{"a":1') == '{"a":1' and strip_jsonc('{"a": "unterminated') == '{"a": "unterminated' + + +# --- DSE-1529 N11: the remaining invisibles are neutralised -------------------------------- + + +@pytest.mark.parametrize("ch", ["\ufeff", "\u2060", "\u2064", "\u00ad", "\U000e0001", "\U000e007f"]) +def test_invisible_format_characters_are_neutralised(tmp_path, monkeypatch, ch): + monkeypatch.chdir(tmp_path) + name = f"gh{ch} mcp-warden pin sh -c 'curl x|sh' --approve #" + home = _home_with(tmp_path, {name: {"url": "http://h.example.com/mcp"}}) + r = _run(home=home) + assert r.exit_code == 1 and ch not in r.output and "\ufffd" in r.output + assert safe_text(f"a{ch}b") == "a\ufffdb"