fix(security): clear remaining CodeQL path-injection + response-splitting alerts

Structural changes so the taint tracker can see the sanitisers:

1. git_diff._match_in_whitelist now returns the element drawn from the
   ``tracked`` set (``git ls-files`` output) — never the user's input
   string, even when equality holds. This explicitly breaks the taint
   flow so downstream uses of ``safe_path`` are sanitised.

2. git_diff._safe_join switched from pathlib.resolve() + relative_to()
   to os.path.realpath + startswith(root + os.sep) — the canonical
   pattern CodeQL's py/path-injection query recognises as a sanitiser.
   Also handles the ``/foo`` vs ``/foobar`` prefix-collision edge case.

3. http_file_diff._git_root_for_name now constrains the ancestor walk
   to paths under a fixed allowlist of real-path probe roots
   (``$HOME``, server CWD, ``/tmp``, ``/var/folders``). Attackers
   cannot probe ``/etc``, ``/root``, or other system directories —
   any path outside the allowlist falls straight back to the CWD git
   root. Walk depth still capped at 64.

4. http_security.resolve_allowed_origin rejects request-Origin values
   containing control chars (< 0x20 and 0x7F) before reflecting them
   into Access-Control-Allow-Origin, closing a py/http-response-
   splitting vector (CWE-113). Request headers with CRLF are no
   longer echoed into response headers.

Smoke-tested: valid repo paths, deleted files in user-space, prefix-
collision escapes, absolute escapes, null bytes, traversal strings,
and cross-repo user-home paths all resolve correctly.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: cdeust

mcp_server/infrastructure/git_diff.py

@@ -9,15 +9,24 @@
 helpers live in ``git_diff_format``; this module owns the cascade and
 the whitelist-matching policy.
 
-Security: All file paths are validated against git's own tracked file
-list. User-controlled paths are NEVER used directly in filesystem
-operations — they are matched against ``git ls-files`` output (the
-whitelist) first, and for genuinely-new untracked files we resolve and
-then ``is_relative_to(git_root)`` to prevent traversal.
+Security (CWE-22 / path-injection):
+
+  * ``_match_in_whitelist`` always returns a string drawn from the
+    ``tracked`` set (``git ls-files`` output). It never returns the
+    user-supplied name, even when they are ``==``. This explicitly
+    breaks the taint flow so downstream uses of the returned path are
+    sanitised — static analysers (CodeQL ``py/path-injection``) see
+    that the returned object is not derived from user input.
+  * ``_safe_join`` uses ``os.path.realpath`` + ``startswith(root +
+    sep)`` containment — the canonical pattern CodeQL recognises as a
+    sanitiser for path-injection.
+  * Every direct filesystem probe (``is_file``, ``read_text``,
+    ``exists``) goes through ``_safe_join``.
 """
 
 from __future__ import annotations
 
+import os
 import subprocess
 from pathlib import Path
 
@@ -55,15 +64,17 @@ def find_git_root(start: Path | None = None) -> Path | None:
 def _match_in_whitelist(name: str, tracked: set[str]) -> str | None:
     """Match a user-provided name against the git-tracked whitelist.
 
-    Returns the canonical tracked path or ``None`` if no match. This
-    ensures we NEVER use the user's raw input as a path.
+    Returns the canonical tracked path or ``None`` if no match. To
+    break the taint flow from the caller's user-controlled ``name``,
+    every return path yields a string drawn from ``tracked`` (the
+    output of ``git ls-files``) — never ``name`` itself, even when
+    equality holds. This is what allows static analysers to see the
+    whitelist as a proper sanitiser.
     """
-    if name in tracked:
-        return name
     basename = name.rsplit("/", 1)[-1] if "/" in name else name
-    for f in tracked:
-        if f == basename or f.endswith("/" + basename):
-            return f
+    for t in tracked:
+        if t == name or t == basename or t.endswith("/" + basename):
+            return t
     return None
 
 
@@ -91,18 +102,20 @@ def resolve_file(name: str, git_root: Path) -> str | None:
 def _safe_join(root: Path, relative: str) -> Path | None:
     """Join ``relative`` onto ``root`` and confirm the result stays inside.
 
-    Returns the resolved absolute path or ``None`` if the join would
-    escape ``root``. Used to prove path-containment to static analyzers
-    (CodeQL ``py/path-injection``) even when ``relative`` originated
-    from git's own tracked-file list — the check is defensive: a
-    successful return means the path is provably inside the repo.
+    Uses ``os.path.realpath`` on both arguments and checks the
+    canonical containment ``startswith(root + os.sep)``. This is the
+    pattern CodeQL's ``py/path-injection`` query recognises as a
+    sanitiser. Returns the resolved ``Path`` or ``None`` when the join
+    would escape ``root`` (or raises on unresolvable input).
     """
     try:
-        joined = (root / relative).resolve()
-        joined.relative_to(root.resolve())
-        return joined
+        root_real = os.path.realpath(str(root))
+        joined_real = os.path.realpath(os.path.join(root_real, relative))
     except (OSError, ValueError):
         return None
+    if joined_real != root_real and not joined_real.startswith(root_real + os.sep):
+        return None
+    return Path(joined_real)
 
 
 def _read_safe(git_root: Path, relative: str) -> str | None:

mcp_server/server/http_file_diff.py

@@ -70,6 +70,41 @@ def _to_repo_rel(name: str, git_root) -> str:
     return clean
 
 
+def _allowed_probe_roots() -> "list[str]":
+    """Real-path roots under which ancestor-walking probes are allowed.
+
+    CWE-22 containment: we only probe directories that the user could
+    legitimately own (home, temp, current workdir). Anything outside
+    falls back to the server's CWD git root. This gives CodeQL an
+    explicit boundary on ``name``-derived path operations without
+    breaking the "repo on this laptop" use-case.
+    """
+    import os
+    from pathlib import Path
+
+    roots: list[str] = []
+    for candidate in (str(Path.home()), os.getcwd(), "/tmp", "/var/folders"):
+        try:
+            roots.append(os.path.realpath(candidate))
+        except (OSError, ValueError):
+            continue
+    return roots
+
+
+def _under_allowed_root(p: "Path") -> bool:  # noqa: F821
+    """True iff ``p`` realpath-resolves inside any allowed probe root."""
+    import os
+
+    try:
+        target = os.path.realpath(str(p))
+    except (OSError, ValueError):
+        return False
+    for root in _allowed_probe_roots():
+        if target == root or target.startswith(root + os.sep):
+            return True
+    return False
+
+
 def _git_root_for_name(name: str, find_git_root) -> "Path | None":  # noqa: F821
     """Resolve git root from the file's own path, then fall back to CWD.
 
@@ -79,13 +114,18 @@ def _git_root_for_name(name: str, find_git_root) -> "Path | None":  # noqa: F821
     along the ancestry exists, falls back to the server's cwd repo so
     that a tracked-then-deleted file can still be recovered from history.
 
-    Security: ``name`` is user-controlled (via ``?name=`` query param).
-    We normalize it with ``os.path.normpath`` (collapses ``..`` and
-    ``//``), reject null bytes and empty input, and cap the ancestor
-    walk at 64 levels to block pathological inputs. The walk only
-    performs ``is_dir()`` / ``rev-parse`` on the normalized ancestry —
-    never reads file content — and any read against the resulting root
-    goes through ``git_diff._safe_join`` (CWE-22 mitigation).
+    Security (CWE-22): ``name`` is user-controlled (via ``?name=``
+    query parameter). Defences:
+
+      * Strip surrounding quotes, reject empty/null-byte inputs.
+      * ``os.path.normpath`` collapses ``..`` and ``//`` segments.
+      * Require absolute paths — relative inputs go straight to CWD.
+      * ``_under_allowed_root`` constrains the probe surface to the
+        user's ``$HOME``, server CWD, and system temp directories —
+        attackers cannot probe ``/etc``, ``/root``, etc.
+      * Ancestor walk capped at 64 levels.
+      * Only ``is_dir()`` / ``git rev-parse`` run against the
+        ancestry — no file content is read in this function.
     """
     import os
     from pathlib import Path
@@ -94,27 +134,26 @@ def _git_root_for_name(name: str, find_git_root) -> "Path | None":  # noqa: F821
         clean = name.strip().strip("\"'`")
         if not clean or "\x00" in clean:
             return find_git_root()
-        # Normalize to collapse ``..`` / ``//`` segments before use.
         p = Path(os.path.normpath(clean))
     except (ValueError, OSError):
         return find_git_root()
 
-    if p.is_absolute():
-        # Walk up the ancestry until we hit an existing directory.
-        # Cap depth to 64 — far deeper than any real filesystem.
-        cursor = p.parent
-        for _ in range(64):
-            if cursor == cursor.parent:
-                break
-            try:
-                if cursor.is_dir():
-                    root = find_git_root(cursor)
-                    if root is not None:
-                        return root
-                    break
-            except OSError:
+    if not p.is_absolute() or not _under_allowed_root(p):
+        return find_git_root()
+
+    cursor = p.parent
+    for _ in range(64):
+        if cursor == cursor.parent or not _under_allowed_root(cursor):
+            break
+        try:
+            if cursor.is_dir():
+                root = find_git_root(cursor)
+                if root is not None:
+                    return root
                 break
-            cursor = cursor.parent
+        except OSError:
+            break
+        cursor = cursor.parent
     return find_git_root()
 
 

mcp_server/server/http_security.py

@@ -45,16 +45,31 @@ def validate_host_header(handler: BaseHTTPRequestHandler) -> bool:
     return _host_only(host.strip()) in _LOOPBACK_HOSTS
 
 
+def _is_safe_header_value(value: str) -> bool:
+    """Reject control chars that would enable response-header splitting.
+
+    CWE-113. A request-derived header value must never contain CR, LF,
+    NUL, or any other control character below 0x20 — they let an
+    attacker inject fabricated headers into the response by stuffing
+    ``\\r\\n`` into the ``Origin`` request header. Python's
+    ``BaseHTTPRequestHandler.send_header`` does NOT filter these, so
+    we filter them here before any reflect-to-header use.
+    """
+    return all(ord(c) >= 0x20 and c != "\x7f" for c in value)
+
+
 def resolve_allowed_origin(handler: BaseHTTPRequestHandler) -> str | None:
     """Return the exact Origin if it's a loopback origin, else None.
 
     We reflect the value rather than responding with ``*`` so that
     credentials-bearing requests (if ever introduced) are also rejected
     by browsers, and so cross-site pages can never read responses from
-    this server.
+    this server. Before reflecting we reject any control characters so
+    that an ``Origin: http://127.0.0.1\\r\\nX-Injected: …`` request
+    cannot splice extra response headers (CWE-113).
     """
     origin = (handler.headers.get("Origin") or "").strip()
-    if not origin:
+    if not origin or not _is_safe_header_value(origin):
         return None
     for scheme in ("http://", "https://"):
         if origin.startswith(scheme):