From 985de39daaf25d917d07d92d145cc2c2d6bfe600 Mon Sep 17 00:00:00 2001 From: cdeust Date: Tue, 9 Jun 2026 10:23:19 +0200 Subject: [PATCH 1/6] fix(sec): resolve CodeQL CWE-22 path-injection (#90,#86) and weak-hash (#84) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Path injection — mcp_server/server/http_file_diff.py (#90 :108, #86 :179): the prior inline pathlib sanitiser (`.resolve()` + `is_relative_to`) was not recognised by CodeQL's dataflow — the `.resolve()` sink fired on raw tainted input and the `is_relative_to` barrier did not propagate across the `_contained_resolved` return. Replace with the canonical, CodeQL- recognised barrier: `os.path.realpath` + `os.path.commonpath(...) == root`. * `_within(real, root)` — segment-aware commonpath containment (fixes the naive-startswith sibling-prefix bug: /home/user-evil ⊄ /home/user). * `_contained_resolved` — realpath then `_within`; returns the validated Path (contract preserved for http_standalone_trace caller). * `_first_existing_dir_within` — ancestor walk that re-asserts `_within` before every `is_dir()` sink, so the barrier dominates locally even as the walk ascends; the probe can never climb out to / or /etc. * Remove dead `_under_allowed_root` (referenced only in docstrings). Weak hashing — workflow_graph_schema.py:126 (#84) + workflow_graph_source.py:47: SHA-1 → SHA-256 for the deterministic node-id factories. These IDs feed only `workflow_graph_layout`, a position cache keyed by (node_id, topology_fingerprint, layout_version) — when the id scheme changes the fingerprint changes and the layout recomputes, so there is no cross-build stability requirement to preserve. SHA-256 is a genuine fix (non-broken algo), not `usedforsecurity=` suppression. Add 14 CWE-22 containment regression tests (tests_py/server/ test_http_file_diff_path_containment.py): /etc blocked, .. rejected, null-byte rejected, sibling-prefix rejected, happy-path repo resolution preserved. All 49 server tests pass; 39 workflow-graph node-id tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) --- mcp_server/core/workflow_graph_schema.py | 20 +-- .../infrastructure/workflow_graph_source.py | 5 +- mcp_server/server/http_file_diff.py | 134 ++++++++++-------- .../test_http_file_diff_path_containment.py | 132 +++++++++++++++++ 4 files changed, 223 insertions(+), 68 deletions(-) create mode 100644 tests_py/server/test_http_file_diff_path_containment.py diff --git a/mcp_server/core/workflow_graph_schema.py b/mcp_server/core/workflow_graph_schema.py index 7f7f739d..977528ef 100644 --- a/mcp_server/core/workflow_graph_schema.py +++ b/mcp_server/core/workflow_graph_schema.py @@ -116,16 +116,18 @@ class WorkflowEdge(BaseModel): def _short_hash(value: str, width: int = 10) -> str: - """Stable, non-cryptographic short hash for ID stability across runs. - - ``usedforsecurity=False`` (Python 3.9+) documents intent — this hash mints - deterministic node IDs, it never protects sensitive data — and lets CodeQL's - weak-hashing query correctly skip it. SHA-1 is retained (not upgraded to - SHA-256) so existing node IDs stay stable across graphs and snapshots. + """Stable, non-cryptographic short hash for deterministic node IDs. + + Uses SHA-256 (a non-broken algorithm) rather than SHA-1 purely for + determinism: ``same input -> same id`` within a graph build. The only + consumer of these IDs is ``workflow_graph_layout``, a position cache + keyed by ``(node_id, topology_fingerprint, layout_version)`` — when the + ID scheme changes, the fingerprint changes and the layout recomputes, so + there is no cross-build stability requirement to preserve. SHA-256 keeps + CodeQL's weak-hashing query (CWE-327/328) clean without relying on the + ``usedforsecurity`` flag. """ - return hashlib.sha1(value.encode("utf-8"), usedforsecurity=False).hexdigest()[ - :width - ] + return hashlib.sha256(value.encode("utf-8")).hexdigest()[:width] class NodeIdFactory: diff --git a/mcp_server/infrastructure/workflow_graph_source.py b/mcp_server/infrastructure/workflow_graph_source.py index c8114892..02974dad 100644 --- a/mcp_server/infrastructure/workflow_graph_source.py +++ b/mcp_server/infrastructure/workflow_graph_source.py @@ -44,7 +44,10 @@ def _cmd_hash(cmd: str) -> str: - return hashlib.sha1(cmd.encode("utf-8")).hexdigest()[:12] + # SHA-256 (non-broken) for deterministic command node IDs — feeds the + # self-healing workflow_graph_layout cache, not a security boundary. + # Matches workflow_graph_schema._short_hash (CWE-327/328 clean). + return hashlib.sha256(cmd.encode("utf-8")).hexdigest()[:12] def _first_line(text: str) -> str: diff --git a/mcp_server/server/http_file_diff.py b/mcp_server/server/http_file_diff.py index 66d20382..157af1a2 100644 --- a/mcp_server/server/http_file_diff.py +++ b/mcp_server/server/http_file_diff.py @@ -91,36 +91,75 @@ def _allowed_probe_roots() -> "list[str]": return roots +def _within(real_path: str, root: str) -> bool: + """True iff ``real_path`` is ``root`` or nested beneath it. + + ``os.path.commonpath`` is the canonical CWE-22 containment barrier and + is recognised by CodeQL's path-injection dataflow as a sanitising guard. + It compares whole path *segments*, so ``/home/user`` does not "contain" + ``/home/user-evil`` the way a naive ``startswith`` prefix test would. + Both inputs are expected to be real-paths, so symlink escapes are already + collapsed before the comparison. + """ + import os + + try: + return os.path.commonpath([root, real_path]) == root + except (ValueError, OSError): + # ValueError: paths on different drives or mixed absolute/relative. + return False + + def _contained_resolved(p: "str | Path") -> "Path | None": # noqa: F821 - """Resolve ``p`` and return it ONLY if it lands inside an allowed probe + """Real-path ``p`` and return it ONLY if it lands inside an allowed probe root; otherwise ``None``. - Sanitise-and-return: the returned Path is the *validated* value, and - callers must use it (never the raw input) for any subsequent filesystem - op. This places the ``is_relative_to`` barrier (the canonical CWE-22 - path-traversal sanitiser) directly on the tainted→sink dataflow, which - CodeQL recognises — so ``?name=`` / ``?path=`` query data can never - reach a filesystem op that escapes ``$HOME`` / cwd / temp. + Sanitise-and-return: callers must use the returned Path (never the raw + input) for any subsequent filesystem op. ``os.path.realpath`` normalises + ``..`` and symlink segments, and ``_within`` (``os.path.commonpath``) is + the CodeQL-recognised barrier placed directly on the tainted→sink + dataflow — so ``?name=`` / ``?path=`` query data can never reach a + filesystem op that escapes ``$HOME`` / cwd / temp. """ + import os from pathlib import Path try: - target = Path(p).resolve(strict=False) + real = os.path.realpath(str(p)) except (OSError, ValueError): return None for root in _allowed_probe_roots(): - try: - base = Path(root).resolve(strict=False) - except (OSError, ValueError): - continue - if target == base or target.is_relative_to(base): - return target + if _within(real, root): + return Path(real) return None -def _under_allowed_root(p: "Path") -> bool: # noqa: F821 - """Back-compat boolean wrapper around :func:`_contained_resolved`.""" - return _contained_resolved(p) is not None +def _first_existing_dir_within(target: "Path") -> "Path | None": # noqa: F821 + """Walk UP from ``target`` to the first directory that exists on disk, + never leaving an allowed probe root. Capped at 64 levels. + + The ``_within`` guard is re-asserted on every iteration *before* the + ``is_dir()`` sink, so the CWE-22 containment barrier dominates the + filesystem access locally even as the walk ascends toward the root — + a crafted ``?name=`` cannot make the probe climb out to ``/`` or ``/etc``. + """ + import os + from pathlib import Path + + roots = _allowed_probe_roots() + cur = target + for _ in range(64): + real = os.path.realpath(str(cur)) + if not any(_within(real, root) for root in roots): + return None + contained = Path(real) + if contained.is_dir(): + return contained + parent = contained.parent + if parent == contained: + return None + cur = parent + return None def _git_root_for_name(name: str, find_git_root) -> "Path | None": # noqa: F821 @@ -136,14 +175,13 @@ def _git_root_for_name(name: str, find_git_root) -> "Path | None": # noqa: F821 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 — + * ``..`` segments are rejected outright — input falls back to CWD. + * Every probed path is real-pathed and gated by ``_within`` + (``os.path.commonpath``) against ``$HOME`` / cwd / temp, so 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. + * Ancestor walk capped at 64 levels (``_first_existing_dir_within``). + * Only ``is_dir()`` / ``git rev-parse`` run against the ancestry — + no file content is read in this function. """ from pathlib import Path @@ -158,49 +196,29 @@ def _git_root_for_name(name: str, find_git_root) -> "Path | None": # noqa: F821 except (ValueError, OSError): return find_git_root() - # Absolute inputs are the COMMON case, not an attack: graph file - # nodes carry the absolute ``file_path`` captured from the original - # tool call, on this same machine. The server is loopback-only and - # ``name`` comes from the user's own stored data, so we resolve the - # repo from the file's own location — constrained to - # ``_under_allowed_root`` (HOME / cwd / temp) so a crafted ``?name=`` - # still can't probe ``/etc`` / ``/root`` (CWE-22). + # Absolute inputs are the COMMON case, not an attack: graph file nodes + # carry the absolute ``file_path`` captured from the original tool call, + # on this same machine. ``_contained_resolved`` bounds the path to + # HOME / cwd / temp, then ``_first_existing_dir_within`` walks up to the + # first existing dir — both gated by the ``commonpath`` barrier (CWE-22). if clean.startswith(("/", "\\")): - # Sanitise-and-return: ``target`` is the validated resolved path - # (gated by is_relative_to), so the value reaching ``is_dir()`` / - # ``git rev-parse`` below carries the CWE-22 barrier inline. target = _contained_resolved(clean) if target is None: return find_git_root() - # Walk up to the first directory that exists (handles a file, or - # an intermediate dir, deleted after capture). Capped at 64. - start = target - for _ in range(64): - if start.is_dir(): - break - parent = start.parent - if parent == start: - break - start = parent + start = _first_existing_dir_within(target) + if start is None: + return find_git_root() root = find_git_root(start) return root if root is not None else find_git_root() - # Relative inputs: join under each allowed probe root and let git - # walk the ancestry. ``is_relative_to`` keeps the join inside base. + # Relative inputs: join under each allowed probe root, contain it, then + # walk to the first existing dir within that root. for base_root in _allowed_probe_roots(): - try: - base = Path(base_root).resolve(strict=False) - target = (base / Path(*parts)).resolve(strict=False) - except (OSError, ValueError): - continue - # Canonical CodeQL-recognised sanitiser. - if not (target == base or target.is_relative_to(base)): - continue - try: - start = target if target.is_dir() else target.parent - except OSError: + target = _contained_resolved(str(Path(base_root) / Path(*parts))) + if target is None: continue - if not (start == base or start.is_relative_to(base)): + start = _first_existing_dir_within(target) + if start is None: continue root = find_git_root(start) if root is not None: diff --git a/tests_py/server/test_http_file_diff_path_containment.py b/tests_py/server/test_http_file_diff_path_containment.py new file mode 100644 index 00000000..47df283c --- /dev/null +++ b/tests_py/server/test_http_file_diff_path_containment.py @@ -0,0 +1,132 @@ +"""CWE-22 (path traversal) regression tests for +``mcp_server.server.http_file_diff``. + +The ``/api/file-diff?name=`` endpoint takes a user-controlled path and +resolves a git root from it. These tests pin the containment barrier: +every probed path must real-path *inside* an allowed root (``$HOME`` / +cwd / system temp) — a crafted ``?name=`` can never make the server +probe ``/etc``, ``/root`` or climb out via ``..`` / symlink escapes. + +The sanitiser is ``os.path.realpath`` + ``os.path.commonpath`` (the +canonical, CodeQL-recognised CWE-22 barrier). Each test would FAIL if a +regression reverted to a naive ``startswith`` prefix test, dropped the +``..`` rejection, or let the ancestor walk leave the allowed root. +""" + +from __future__ import annotations + +import os +import tempfile +from pathlib import Path + +from mcp_server.server.http_file_diff import ( + _allowed_probe_roots, + _contained_resolved, + _first_existing_dir_within, + _git_root_for_name, + _within, +) + + +def _sentinel_root(): + """Return ``find_git_root`` and a marker it yields, so we can assert the + code fell back to the CWD repo instead of probing the tainted path.""" + sentinel = Path("/__cwd_repo_fallback__") + + def fake_find_git_root(start=None): + # No argument => the CWD-repo fallback branch. + return sentinel if start is None else None + + return fake_find_git_root, sentinel + + +# ── _within: commonpath is segment-aware, not a prefix test ────────────── + + +def test_within_true_for_nested_path(): + assert _within("/home/user/project", "/home/user") is True + + +def test_within_true_for_exact_root(): + assert _within("/home/user", "/home/user") is True + + +def test_within_false_for_sibling_prefix(): + # The naive ``startswith`` bug: "/home/user-evil".startswith("/home/user"). + assert _within("/home/user-evil", "/home/user") is False + + +def test_within_false_for_outside_path(): + assert _within("/etc/passwd", "/home/user") is False + + +# ── _contained_resolved: only returns paths inside an allowed root ─────── + + +def test_contained_resolved_blocks_etc(): + assert _contained_resolved("/etc/passwd") is None + + +def test_contained_resolved_allows_home(): + out = _contained_resolved(str(Path.home())) + assert out is not None + assert _within(os.path.realpath(str(out)), os.path.realpath(str(Path.home()))) + + +def test_contained_resolved_allows_temp(): + with tempfile.TemporaryDirectory() as td: + # /tmp and /var/folders are allowed roots; a real temp dir resolves + # under one of them on every supported platform. + real = os.path.realpath(td) + assert any(_within(real, r) for r in _allowed_probe_roots()) + assert _contained_resolved(td) is not None + + +# ── _first_existing_dir_within: walk never leaves the root ─────────────── + + +def test_first_existing_dir_returns_existing_ancestor(): + with tempfile.TemporaryDirectory() as td: + deep = Path(td) / "a" / "b" / "c.py" # never created + out = _first_existing_dir_within(deep) + assert out is not None + assert os.path.realpath(str(out)) == os.path.realpath(td) + + +def test_first_existing_dir_rejects_outside_root(): + # A path outside any allowed root yields None rather than probing it. + assert _first_existing_dir_within(Path("/etc/cron.d")) is None + + +# ── _git_root_for_name: end-to-end traversal blocking ──────────────────── + + +def test_git_root_dotdot_falls_back_to_cwd(): + find, sentinel = _sentinel_root() + assert _git_root_for_name("/etc/../etc/passwd", find) is sentinel + + +def test_git_root_etc_falls_back_to_cwd(): + find, sentinel = _sentinel_root() + assert _git_root_for_name("/etc/passwd", find) is sentinel + + +def test_git_root_nullbyte_falls_back_to_cwd(): + find, sentinel = _sentinel_root() + assert _git_root_for_name("/home/\x00/x", find) is sentinel + + +def test_git_root_empty_falls_back_to_cwd(): + find, sentinel = _sentinel_root() + assert _git_root_for_name(" ", find) is sentinel + + +def test_git_root_resolves_real_in_repo_path(): + # A legitimate absolute path inside this repo (cwd) must resolve to a + # real git root, proving the barrier does not break the happy path. + from mcp_server.infrastructure.git_diff import find_git_root + + here = os.path.realpath(__file__) + root = _git_root_for_name(here, find_git_root) + assert root is not None + assert Path(root).is_dir() From 6f109912c072d789eff50e889c24f7804cf0c0d9 Mon Sep 17 00:00:00 2001 From: cdeust Date: Tue, 9 Jun 2026 10:34:33 +0200 Subject: [PATCH 2/6] fix(sec): inline commonpath guard to dominate is_dir sink (CodeQL #57) The first refactor moved the two path-injection sinks to a single `contained.is_dir()` in `_first_existing_dir_within`, but CodeQL still flagged it (new alert, http_file_diff.py:156): the `commonpath` containment check was behind the `_within` helper + an `any(...)` generator, so CodeQL's barrier-guard dataflow could not connect the sanitiser to the sink. Inline the guard into the exact shape CodeQL recognises (and that the query's own Recommendation documents): the `is_dir()` filesystem access now sits directly inside `if os.path.commonpath([root, real]) == root:`, so the barrier dominates the sink on `real`. Behaviour and the 14 CWE-22 containment tests are unchanged; 49/49 server tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) --- mcp_server/server/http_file_diff.py | 26 +++++++++++++++++++------- 1 file changed, 19 insertions(+), 7 deletions(-) diff --git a/mcp_server/server/http_file_diff.py b/mcp_server/server/http_file_diff.py index 157af1a2..08922d75 100644 --- a/mcp_server/server/http_file_diff.py +++ b/mcp_server/server/http_file_diff.py @@ -150,15 +150,27 @@ def _first_existing_dir_within(target: "Path") -> "Path | None": # noqa: F821 cur = target for _ in range(64): real = os.path.realpath(str(cur)) - if not any(_within(real, root) for root in roots): + contained = False + for root in roots: + try: + # Inline CWE-22 barrier: the ``is_dir()`` sink lives directly + # inside the ``commonpath`` guard so it dominates the filesystem + # access on ``real`` — this is the exact shape CodeQL's + # path-injection dataflow recognises as a sanitiser (a guard + # behind a helper / ``any(...)`` generator is NOT recognised). + if os.path.commonpath([root, real]) == root: + contained = True + if Path(real).is_dir(): + return Path(real) + break + except (ValueError, OSError): + continue + if not contained: return None - contained = Path(real) - if contained.is_dir(): - return contained - parent = contained.parent - if parent == contained: + parent = os.path.dirname(real) + if parent == real: return None - cur = parent + cur = Path(parent) return None From 6383d461cc1faed8ab3cd936f16ec84733414e8b Mon Sep 17 00:00:00 2001 From: cdeust Date: Tue, 9 Jun 2026 10:55:24 +0200 Subject: [PATCH 3/6] fix(sec): break CWE-22 taint via trusted scandir descent (CodeQL #57) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The inline-commonpath guard (6f10991) still left CodeQL alert #92 at the is_dir() sink: the ancestor up-walk re-derived the probed path from realpath(user_input) on every iteration, so the loop-carried value stayed tainted and the per-root commonpath guard was not accepted as a sanitiser. Redesign: replace the up-walk with a DOWNWARD descent from a constant allowed probe root via os.scandir (_descend_trusted). User path components select among trusted enumerated entries by `entry.name == name` equality only — they never construct a probed path. The value reaching is_dir() / `git rev-parse --cwd` is composed entirely from the constant root plus scandir output, breaking the taint flow exactly as git_diff._match_in_whitelist does for the file whitelist. _contained_resolved / _within / _allowed_probe_roots unchanged (no sinks; external caller http_standalone_trace.py contract preserved). Tests: 14/14 CWE-22 containment, 49/49 server. ruff clean. Co-Authored-By: Claude Opus 4.8 (1M context) --- mcp_server/server/http_file_diff.py | 113 ++++++++++++++++++---------- 1 file changed, 73 insertions(+), 40 deletions(-) diff --git a/mcp_server/server/http_file_diff.py b/mcp_server/server/http_file_diff.py index 08922d75..f24c8e1f 100644 --- a/mcp_server/server/http_file_diff.py +++ b/mcp_server/server/http_file_diff.py @@ -134,43 +134,70 @@ def _contained_resolved(p: "str | Path") -> "Path | None": # noqa: F821 return None -def _first_existing_dir_within(target: "Path") -> "Path | None": # noqa: F821 - """Walk UP from ``target`` to the first directory that exists on disk, - never leaving an allowed probe root. Capped at 64 levels. - - The ``_within`` guard is re-asserted on every iteration *before* the - ``is_dir()`` sink, so the CWE-22 containment barrier dominates the - filesystem access locally even as the walk ascends toward the root — - a crafted ``?name=`` cannot make the probe climb out to ``/`` or ``/etc``. +def _descend_trusted(root: str, names: "list[str]") -> "Path | None": # noqa: F821 + """Descend from a TRUSTED ``root`` into child directories whose names + match the successive user-supplied ``names``, returning the deepest + existing directory reached. + + CWE-22 taint break: this is the ``git_diff._match_in_whitelist`` pattern + applied to directory traversal. At every level the candidate paths come + from ``os.scandir(cur)`` — a trusted enumeration of what is actually on + disk — and a user component selects among them ONLY via ``entry.name == + name`` equality. The path that reaches the ``is_dir`` / ``scandir`` sink + (``cur`` / ``entry.path``) is composed entirely from the constant + ``root`` plus scandir output; the user ``names`` never construct a probed + path. Static analysers (CodeQL ``py/path-injection``) therefore see the + sink operand as not derived from user input. Capped at 64 levels. """ import os from pathlib import Path - roots = _allowed_probe_roots() - cur = target - for _ in range(64): - real = os.path.realpath(str(cur)) - contained = False - for root in roots: - try: - # Inline CWE-22 barrier: the ``is_dir()`` sink lives directly - # inside the ``commonpath`` guard so it dominates the filesystem - # access on ``real`` — this is the exact shape CodeQL's - # path-injection dataflow recognises as a sanitiser (a guard - # behind a helper / ``any(...)`` generator is NOT recognised). - if os.path.commonpath([root, real]) == root: - contained = True - if Path(real).is_dir(): - return Path(real) - break - except (ValueError, OSError): - continue - if not contained: - return None - parent = os.path.dirname(real) - if parent == real: - return None - cur = Path(parent) + cur = os.path.realpath(root) # ``root`` is a constant probe root → trusted + if not os.path.isdir(cur): + return None + deepest = cur + for name in names[:64]: + nxt = None + try: + with os.scandir(cur) as entries: + for entry in entries: + # Equality match only — ``name`` selects a trusted entry, + # it never builds the path that gets probed. + if entry.name == name and entry.is_dir(): + nxt = entry.path + break + except (OSError, ValueError): + break + if nxt is None: + break + cur = nxt + deepest = cur + return Path(deepest) + + +def _first_existing_dir_within(target: "Path") -> "Path | None": # noqa: F821 + """Deepest existing directory on ``target``'s path chain, found by + DESCENDING from the allowed probe root that contains it — never by + probing a ``realpath(user_input)``-derived path. + + CWE-22 taint break (redesign): the up-walk variant fed ``is_dir()`` a + value derived from the user-controlled ``target`` on every iteration, + which CodeQL's loop-carried dataflow re-taints and refuses to treat as + sanitised. Instead we locate the constant allowed root that prefixes + ``target`` (a pure segment comparison — no filesystem op on user data), + then hand the remaining components to :func:`_descend_trusted`, where the + filesystem sinks only ever touch trusted enumerated paths. ``target`` is + used solely to *choose* a root and *compare* component names. + """ + import os + + real = os.path.realpath(str(target)) + target_parts = [p for p in real.split(os.sep) if p] + for root in _allowed_probe_roots(): + root_parts = [p for p in root.split(os.sep) if p] + if target_parts[: len(root_parts)] != root_parts: + continue + return _descend_trusted(root, target_parts[len(root_parts) :]) return None @@ -188,11 +215,16 @@ def _git_root_for_name(name: str, find_git_root) -> "Path | None": # noqa: F821 * Strip surrounding quotes, reject empty/null-byte inputs. * ``..`` segments are rejected outright — input falls back to CWD. - * Every probed path is real-pathed and gated by ``_within`` - (``os.path.commonpath``) against ``$HOME`` / cwd / temp, so - attackers cannot probe ``/etc``, ``/root``, etc. - * Ancestor walk capped at 64 levels (``_first_existing_dir_within``). - * Only ``is_dir()`` / ``git rev-parse`` run against the ancestry — + * ``_contained_resolved`` bounds the input to ``$HOME`` / cwd / temp + (``os.path.commonpath``), so anything outside falls back to CWD. + * The directory actually probed is reached by DESCENDING from a + constant allowed root via ``_first_existing_dir_within`` / + ``_descend_trusted`` (``os.scandir``): the value that reaches + ``is_dir`` / ``git rev-parse --cwd`` is composed from trusted + enumeration, not from ``name`` — the CWE-22 taint flow is broken + the same way ``git_diff._match_in_whitelist`` breaks it. + * Descent capped at 64 levels. + * Only directory probes / ``git rev-parse`` run against the path — no file content is read in this function. """ from pathlib import Path @@ -211,8 +243,9 @@ def _git_root_for_name(name: str, find_git_root) -> "Path | None": # noqa: F821 # Absolute inputs are the COMMON case, not an attack: graph file nodes # carry the absolute ``file_path`` captured from the original tool call, # on this same machine. ``_contained_resolved`` bounds the path to - # HOME / cwd / temp, then ``_first_existing_dir_within`` walks up to the - # first existing dir — both gated by the ``commonpath`` barrier (CWE-22). + # HOME / cwd / temp, then ``_first_existing_dir_within`` DESCENDS from the + # containing trusted root via ``os.scandir`` to the deepest existing dir — + # so the path reaching the git sink is trusted enumeration (CWE-22). if clean.startswith(("/", "\\")): target = _contained_resolved(clean) if target is None: From 07619e20e9d16e0827f43daf288a0801522ced13 Mon Sep 17 00:00:00 2001 From: cdeust Date: Tue, 9 Jun 2026 11:08:20 +0200 Subject: [PATCH 4/6] fix(setup): per-user DATABASE_URL config + three-way DB diagnostics MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Harden the database setup/config layer so per-user DB connection settings survive plugin updates and setup failures are diagnosed correctly. - plugin.json: declare userConfig.database_url (default 127.0.0.1) — persists across updates; user-settable in settings. - .mcp.json: replace hardcoded DATABASE_URL literal with ${user_config.database_url} (the env block is a hard override of shell env). - pg_store._get_database_url(): treat empty OR unexpanded ${...} token as unset -> fall back to settings default (MCP-server path only). - setup_db.py: three-way diagnostic — server-unreachable (with stale-lock hint), db-missing, auth-failure — replacing the misleading "createdb cortex". New auth_failed status surfaced in session_start cold-start message. - localhost -> 127.0.0.1 in runtime defaults only (avoids IPv6 ::1 / peer-auth). No ${VAR:-default} syntax exists in .mcp.json env, so fallback stays in Python. Server-layer loopback binding (GHSA-gvpp-v77h-5w8g) untouched — diff has no mcp_server/server/ files; localhost->127.0.0.1 is the DB connection target, not the HTTP host allowlist. Verified: live DB -> ready (536k memories); server-down sim -> stale-lock hint; token guard -> 127.0.0.1; test_doctor + test_doctor_mcp -> 44 passed. Co-Authored-By: Claude Opus 4.8 (1M context) --- .claude-plugin/plugin.json | 8 ++ .mcp.json | 2 +- mcp_server/hooks/session_start.py | 6 +- mcp_server/infrastructure/memory_config.py | 2 +- mcp_server/infrastructure/pg_store.py | 11 +- scripts/setup.sh | 4 +- scripts/setup_db.py | 159 ++++++++++++++------- tasks/db-config-hardening.md | 40 ++++++ 8 files changed, 176 insertions(+), 56 deletions(-) create mode 100644 tasks/db-config-hardening.md diff --git a/.claude-plugin/plugin.json b/.claude-plugin/plugin.json index 551540c6..d05295d0 100644 --- a/.claude-plugin/plugin.json +++ b/.claude-plugin/plugin.json @@ -17,6 +17,14 @@ "neuroscience", "agents" ], + "userConfig": { + "database_url": { + "type": "string", + "title": "Cortex database URL", + "description": "PostgreSQL connection string (postgresql://[user[:pass]@]host[:port]/dbname). Leave as the default for a local install; set this to point Cortex at a remote or credentialed database. Your value persists across plugin updates.", + "default": "postgresql://127.0.0.1:5432/cortex" + } + }, "mcpServers": "./.mcp.json", "postInstall": { "command": "bash ${CLAUDE_PLUGIN_ROOT}/scripts/install-plugin.sh", diff --git a/.mcp.json b/.mcp.json index 7d467f39..65baf46f 100644 --- a/.mcp.json +++ b/.mcp.json @@ -7,7 +7,7 @@ "mcp_server" ], "env": { - "DATABASE_URL": "postgresql://localhost:5432/cortex", + "DATABASE_URL": "${user_config.database_url}", "CORTEX_RUNTIME": "", "CORTEX_MEMORY_AP_ENABLED": "1" } diff --git a/mcp_server/hooks/session_start.py b/mcp_server/hooks/session_start.py index 7a3e9e5e..0bce24b1 100644 --- a/mcp_server/hooks/session_start.py +++ b/mcp_server/hooks/session_start.py @@ -26,7 +26,7 @@ # ── Config ──────────────────────────────────────────────────────────────── -_DATABASE_URL = os.environ.get("DATABASE_URL", "postgresql://localhost:5432/cortex") +_DATABASE_URL = os.environ.get("DATABASE_URL", "postgresql://127.0.0.1:5432/cortex") _HOT_LIMIT = int(os.environ.get("CORTEX_SESSION_START_LIMIT", "8")) _MIN_HEAT = float(os.environ.get("CORTEX_SESSION_START_MIN_HEAT", "0.4")) _ANCHOR_LIMIT = int(os.environ.get("CORTEX_SESSION_START_ANCHOR_LIMIT", "5")) @@ -523,6 +523,10 @@ def _build_cold_start_message(setup_result: dict | None) -> str: lines.append("Cortex will auto-create the database and schema on next start.") return "\n".join(lines) + if setup_result and setup_result.get("status") == "auth_failed": + msg = setup_result.get("message", "Authentication failed") + return "## Cortex — Database Authentication\n\n" + msg + if setup_result and setup_result.get("status") != "ready": msg = setup_result.get("message", "Unknown setup error") lines.append(f"Setup issue: {msg}\n") diff --git a/mcp_server/infrastructure/memory_config.py b/mcp_server/infrastructure/memory_config.py index 74081c3e..824ee5ea 100644 --- a/mcp_server/infrastructure/memory_config.py +++ b/mcp_server/infrastructure/memory_config.py @@ -44,7 +44,7 @@ class MemorySettings(BaseSettings): RUNTIME: str = "" # "cli" | "cowork" — set by validator from CORTEX_RUNTIME or CLAUDE_ENVIRONMENT # ── Storage ────────────────────────────────────────────────────────── - DATABASE_URL: str = "postgresql://localhost:5432/cortex" + DATABASE_URL: str = "postgresql://127.0.0.1:5432/cortex" # 127.0.0.1 not localhost: avoids IPv6 ::1 / peer-auth ambiguity DB_PATH: str = str(METHODOLOGY_DIR / "memory.db") # deprecated, kept for migration SQLITE_FALLBACK_PATH: str = str(METHODOLOGY_DIR / "memory.db") STORE_BACKEND: str = "auto" # "auto" | "postgresql" | "sqlite" diff --git a/mcp_server/infrastructure/pg_store.py b/mcp_server/infrastructure/pg_store.py index e14707e4..8c2f8356 100644 --- a/mcp_server/infrastructure/pg_store.py +++ b/mcp_server/infrastructure/pg_store.py @@ -42,9 +42,14 @@ def _get_database_url() -> str: - """Get DATABASE_URL from environment or MemorySettings default.""" - url = os.environ.get("DATABASE_URL", "") - if not url: + """Get DATABASE_URL from environment or MemorySettings default. + + An unexpanded ``${user_config.database_url}`` token (Claude Code passes the + literal through if the user_config option is unset and carries no default) + is treated as unset, so the settings default still applies. + """ + url = os.environ.get("DATABASE_URL", "").strip() + if not url or "${" in url: from mcp_server.infrastructure.memory_config import get_memory_settings url = get_memory_settings().DATABASE_URL diff --git a/scripts/setup.sh b/scripts/setup.sh index d6273121..5a7db026 100755 --- a/scripts/setup.sh +++ b/scripts/setup.sh @@ -210,7 +210,7 @@ ok "Python packages installed" step "Database & schema" export PYTHONPATH="${PROJECT_DIR}:${DEPS_DIR}" -export DATABASE_URL="${DATABASE_URL:-postgresql://localhost:5432/cortex}" +export DATABASE_URL="${DATABASE_URL:-postgresql://127.0.0.1:5432/cortex}" # Run the existing setup_db.py which handles DB creation, extensions, and schema SETUP_OUTPUT=$(python3 "$SCRIPT_DIR/setup_db.py" 2>/dev/null || true) @@ -403,5 +403,5 @@ echo " 1. Restart Claude Code to activate" echo " 2. Start a conversation — Cortex works automatically" echo " 3. Use /cortex-recall to search memories" echo "" -echo "Database: ${DATABASE_URL:-postgresql://localhost:5432/cortex}" +echo "Database: ${DATABASE_URL:-postgresql://127.0.0.1:5432/cortex}" echo "Deps: ${DEPS_DIR}" diff --git a/scripts/setup_db.py b/scripts/setup_db.py index 39a29c6b..9023df30 100644 --- a/scripts/setup_db.py +++ b/scripts/setup_db.py @@ -6,9 +6,10 @@ Exit codes: 0 — database ready (created or already existed) - 1 — PostgreSQL not running or not installed + 1 — PostgreSQL not running or not installed (server unreachable) 2 — could not create database or extensions 3 — schema initialization failed + 4 — authentication/authorization failure (server up, bad credentials/role) """ from __future__ import annotations @@ -34,15 +35,27 @@ def _log(msg: str) -> None: def _result(status: str, message: str, **extra: object) -> None: """Print JSON result to stdout and exit.""" - code = {"ready": 0, "needs_install": 1, "create_failed": 2, "schema_failed": 3} + code = { + "ready": 0, + "needs_install": 1, + "create_failed": 2, + "schema_failed": 3, + "auth_failed": 4, + } out = {"status": status, "message": message, **extra} print(json.dumps(out)) sys.exit(code.get(status, 1)) def _get_database_url() -> str: - """Resolve DATABASE_URL from environment or default.""" - return os.environ.get("DATABASE_URL", "postgresql://localhost:5432/cortex") + """Resolve DATABASE_URL from environment or default. + + Treats an empty value or an unexpanded ``${...}`` token as unset. + """ + url = os.environ.get("DATABASE_URL", "").strip() + if not url or "${" in url: + return "postgresql://127.0.0.1:5432/cortex" + return url def _parse_db_url(url: str) -> dict: @@ -76,38 +89,58 @@ def _pg_is_running(host: str, port: str) -> bool: return False -def _db_exists(host: str, port: str, dbname: str) -> bool: - """Check if the target database exists.""" +# Substrings that mark a PostgreSQL connection failure as authentication/ +# authorization (not a missing database or a down server). Source: libpq +# error messages — postgresql.org/docs/current/protocol-error-fields.html +_AUTH_SIGNATURES = ( + "password authentication failed", + "no password supplied", + "authentication failed", + "does not exist", # role "x" does not exist + "permission denied", + "must be superuser", + "must be member of", +) + + +def _is_auth_error(stderr: str) -> bool: + """True if stderr indicates an auth/authorization failure (not db-absent).""" + s = stderr.lower() + return any(sig in s for sig in _AUTH_SIGNATURES) + + +def _probe_database(host: str, port: str, dbname: str) -> tuple[str, str]: + """Probe for the target database. + + Returns (state, detail) where state is one of: + exists | absent | auth_failed | error + """ psql = shutil.which("psql") if not psql: - return False + return "error", "psql not found on PATH" try: r = subprocess.run( [ - psql, - "-h", - host, - "-p", - port, - "-d", - "postgres", - "-tAc", + psql, "-h", host, "-p", port, "-d", "postgres", "-tAc", f"SELECT 1 FROM pg_database WHERE datname = '{_safe_ident(dbname)}'", ], capture_output=True, timeout=5, text=True, ) - return "1" in r.stdout - except Exception: - return False + except Exception as e: + return "error", str(e) + if r.returncode != 0: + detail = r.stderr.strip() + return ("auth_failed" if _is_auth_error(detail) else "error"), detail + return ("exists" if "1" in r.stdout else "absent"), "" -def _create_db(host: str, port: str, dbname: str) -> bool: - """Create the database.""" +def _create_db(host: str, port: str, dbname: str) -> tuple[bool, str]: + """Create the database. Returns (ok, stderr).""" createdb = shutil.which("createdb") if not createdb: - return False + return False, "createdb not found on PATH" try: r = subprocess.run( [createdb, "-h", host, "-p", port, dbname], @@ -115,9 +148,9 @@ def _create_db(host: str, port: str, dbname: str) -> bool: timeout=10, text=True, ) - return r.returncode == 0 - except Exception: - return False + return r.returncode == 0, r.stderr.strip() + except Exception as e: + return False, str(e) def _create_extensions(host: str, port: str, dbname: str) -> tuple[bool, str]: @@ -208,6 +241,44 @@ def _count_session_files() -> int: return count +def _server_down_message(host: str, port: str) -> str: + """Message for an unreachable server — includes the stale-lock case.""" + return ( + f"PostgreSQL is not accepting connections at {host}:{port}.\n" + "\n" + "If it was working before, an unclean shutdown can leave a stale lock\n" + "that blocks restart (the PID in postmaster.pid gets reused by another\n" + "process, so PostgreSQL refuses to start):\n" + " brew services list # look for postgresql@NN in 'error'\n" + " rm -f $(brew --prefix)/var/postgresql@17/postmaster.pid\n" + " brew services restart postgresql@17\n" + "\n" + "If it is not installed yet:\n" + " # macOS\n" + " brew install postgresql@17 pgvector && brew services start postgresql@17\n" + " # Ubuntu/Debian\n" + " sudo apt install postgresql postgresql-server-dev-all\n" + " sudo systemctl start postgresql\n" + " # pgvector: https://github.com/pgvector/pgvector#installation\n" + "\n" + "Then restart Claude Code." + ) + + +def _auth_message(host: str, port: str, dbname: str, detail: str) -> str: + """Message for an auth/authorization failure — server up, creds wrong.""" + return ( + f"PostgreSQL at {host}:{port} is running, but the connection was " + f"refused for authentication/authorization reasons:\n" + f" {detail}\n" + "\n" + "The database may well exist — this is a credentials/role problem, not a\n" + "missing database. Check the user, password, and role in your DATABASE_URL\n" + "(set it via the plugin's database_url config), and that the role may " + f"connect to '{dbname}'." + ) + + def main() -> None: """Auto-detect and set up PostgreSQL for Cortex.""" url = _get_database_url() @@ -216,34 +287,26 @@ def main() -> None: _log(f"Checking PostgreSQL at {host}:{port}/{dbname}") - # Step 1: Is PostgreSQL running? + # Step 1: Is the server reachable at all? if not _pg_is_running(host, port): - _result( - "needs_install", - ( - "PostgreSQL is not running. To set up Cortex:\n" - "\n" - " # macOS\n" - " brew install postgresql@17 pgvector\n" - " brew services start postgresql@17\n" - "\n" - " # Ubuntu/Debian\n" - " sudo apt install postgresql postgresql-server-dev-all\n" - " sudo systemctl start postgresql\n" - " # Install pgvector: https://github.com/pgvector/pgvector#installation\n" - "\n" - "Then restart Claude Code." - ), - ) - - # Step 2: Does the database exist? - if not _db_exists(host, port, dbname): + _result("needs_install", _server_down_message(host, port)) + + # Step 2: Probe the database — distinguishes absent from auth failure. + state, detail = _probe_database(host, port, dbname) + if state == "auth_failed": + _result("auth_failed", _auth_message(host, port, dbname, detail)) + if state == "error": + _result("create_failed", f"Could not query PostgreSQL: {detail}") + if state == "absent": _log(f"Database '{dbname}' not found, creating...") - if not _create_db(host, port, dbname): + ok, err = _create_db(host, port, dbname) + if not ok: + if _is_auth_error(err): + _result("auth_failed", _auth_message(host, port, dbname, err)) _result( "create_failed", - f"Could not create database '{dbname}'. " - f"Try manually: createdb {dbname}", + f"Could not create database '{dbname}': {err or 'unknown error'}\n" + f"Try manually: createdb -h {host} -p {port} {dbname}", ) _log(f"Database '{dbname}' created") diff --git a/tasks/db-config-hardening.md b/tasks/db-config-hardening.md new file mode 100644 index 00000000..fbfcedb0 --- /dev/null +++ b/tasks/db-config-hardening.md @@ -0,0 +1,40 @@ +# DATABASE_URL config hardening (5 suggestions) + +Context: cortex MCP failed at session start (`-32000`). Root cause was a **stale +`postmaster.pid`** (PID 1255 reused by `mobilerepaird` after unclean shutdown), +not config. Fixed by removing the stale lock + `brew services restart`. These 5 +items harden the *setup/config* layer so future failures are diagnosed correctly +and per-user DB config survives plugin updates. + +Verified facts (via claude-code-guide, source: code.claude.com/docs plugins-reference): +- `userConfig` lives in **plugin.json** (NOT marketplace.json). Token: `${user_config.}`. +- `.mcp.json` `env` block is a **hard override** of shell env. +- No `${VAR:-default}` syntax — fallback must be in code or via userConfig `default`. + +## Tasks + +- [x] **#3 + #1** — `userConfig.database_url` in `plugin.json` with `default` + `postgresql://127.0.0.1:5432/cortex`. Non-sensitive (resolves silently to + default → preserves install-and-forget; survives updates via settings.json + pluginConfigs). Trade-off: a user-supplied password lands in settings.json + plaintext — same exposure class as the prior hardcoded/shell approach. +- [x] **#2** — `.mcp.json`: `DATABASE_URL` → `${user_config.database_url}`. +- [x] **#2 guard** — `pg_store._get_database_url()`: empty OR `${…}` → settings default. +- [x] **#5** — `localhost` → `127.0.0.1`: `memory_config.py:47`, `setup_db.py`, + `session_start.py:29`, `setup.sh:213,406`. Benchmarks/tests untouched. +- [x] **#4** — `setup_db.py`: three-way diagnostic (server-unreachable + stale-lock + hint / db-missing / auth-failure). `auth_failed` surfaced in + `session_start._build_cold_start_message`. + +## Verification +- [x] live DB → status "ready" (536,499 memories) via venv python. +- [x] server-down (bad port) → needs_install + stale-lock hint present. +- [x] unexpanded `${user_config.database_url}` token → resolves to 127.0.0.1 default. +- [x] plugin.json / .mcp.json / marketplace.json valid JSON. +- [x] pytest test_doctor.py + test_doctor_mcp.py → 44 passed, 0 regressions. +- [x] GHSA-gvpp-v77h-5w8g: server-layer binding untouched; diff has no server/ files. + +## Notes +- `setup_db.py` is now 349 lines (>300). Low-stakes setup script (§10 stakes- + calibration → size informal); single cohesive responsibility. Left as-is; a + later split would move the diagnostic helpers to `infrastructure/db_diagnostics.py`. From 6b0640790ef89c9e90a22c52a524593ac40163da Mon Sep 17 00:00:00 2001 From: cdeust Date: Tue, 9 Jun 2026 11:13:08 +0200 Subject: [PATCH 5/6] style(setup): ruff format setup_db.py (subprocess args one-per-line) Co-Authored-By: Claude Opus 4.8 (1M context) --- scripts/setup_db.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/scripts/setup_db.py b/scripts/setup_db.py index 9023df30..b413a779 100644 --- a/scripts/setup_db.py +++ b/scripts/setup_db.py @@ -121,7 +121,14 @@ def _probe_database(host: str, port: str, dbname: str) -> tuple[str, str]: try: r = subprocess.run( [ - psql, "-h", host, "-p", port, "-d", "postgres", "-tAc", + psql, + "-h", + host, + "-p", + port, + "-d", + "postgres", + "-tAc", f"SELECT 1 FROM pg_database WHERE datname = '{_safe_ident(dbname)}'", ], capture_output=True, From 46ed2bae150601b47fe35068efff285355e36383 Mon Sep 17 00:00:00 2001 From: cdeust Date: Tue, 9 Jun 2026 11:22:52 +0200 Subject: [PATCH 6/6] fix(ci): realign I2 writer line pins (548/608) + allow auth_failed status MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #58 added the pg_store token guard (5 lines above bump_heat_raw) and a new 'auth_failed' setup status. Two regression guards lagged the change: - test_I2_canonical_writer: heat_base UPDATE sites shifted 543→548, 603→608. - test_cold_start: setup_db.py now emits 'auth_failed'; add to valid set. Co-Authored-By: Claude Opus 4.8 (1M context) --- tests_py/integration/test_cold_start.py | 1 + tests_py/invariants/test_I2_canonical_writer.py | 10 +++++----- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/tests_py/integration/test_cold_start.py b/tests_py/integration/test_cold_start.py index 7d378c9c..ad9ae990 100644 --- a/tests_py/integration/test_cold_start.py +++ b/tests_py/integration/test_cold_start.py @@ -325,6 +325,7 @@ def test_setup_reports_ready_or_needs_install(self): "needs_setup", "create_failed", "schema_failed", + "auth_failed", "error", ) if parsed["status"] == "ready": diff --git a/tests_py/invariants/test_I2_canonical_writer.py b/tests_py/invariants/test_I2_canonical_writer.py index e0001545..9a01a6f3 100644 --- a/tests_py/invariants/test_I2_canonical_writer.py +++ b/tests_py/invariants/test_I2_canonical_writer.py @@ -33,12 +33,12 @@ # OR be added here with a source-commented ADR justification. _ALLOWED_WRITERS: set[tuple[str, int]] = { # Canonical single-row writer (all callers route through this). - # bump_heat_raw (def at line 526) — shifted 477→543 by code added above - # it in pg_store.py; still the one canonical single-row site. - ("infrastructure/pg_store.py", 543), + # bump_heat_raw — UPDATE at line 548 (shifted 543→548 by the pg_store + # token guard added above it); still the one canonical single-row site. + ("infrastructure/pg_store.py", 548), # A3 batched writer (homeostatic cohort branch + any other batch consumer). - # update_memories_heat_batch (def at line 587) — shifted 537→603 likewise. - ("infrastructure/pg_store.py", 603), + # update_memories_heat_batch — SET line at 608 (shifted 603→608 likewise). + ("infrastructure/pg_store.py", 608), # SQLite parity. ("infrastructure/sqlite_store.py", 284), ("infrastructure/sqlite_store.py", 328),