From 57ff94c44e51ececacea9e45b998fe594a3ebbec Mon Sep 17 00:00:00 2001 From: Bill Thornton Date: Thu, 2 Jul 2026 11:39:48 -0700 Subject: [PATCH] fix(server): guard project_root writes against mock/relative coercion (TAP-4573) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two production write paths mkdir/write under Path(settings.project_root) with no guard: agent_identity._write_uuid (via get_stable_agent_id) and server_helpers.write_session_start_marker. ~70 test sites pass a bare MagicMock(); os.fspath(MagicMock().project_root) coerces to the relative path 'MagicMock/mock.project_root/', so full-suite runs created real agent.id / session-start-marker trees in the pytest CWD (the repo root), invisible to git status because .tapps-mcp/ is gitignored at any depth. Add a shared guard is_real_writable_root() in tapps_core.agent_identity that accepts only absolute filesystem paths. Both write paths short-circuit when it fails: get_stable_agent_id returns a valid in-memory id (no persistence), write_session_start_marker returns early. No-op for real deployments (roots are always absolute); no edits to the 70 test sites. Verified: full package unit run reports 'no MagicMock leak' and 7368 passed (the single ordering-flake failure in test_cli_memory is pre-existing and unrelated — passes in isolation and has zero linkage to the changed code). Co-Authored-By: Claude Opus 4.8 (1M context) --- .../src/tapps_core/agent_identity.py | 32 ++++++++++++- .../tests/unit/test_agent_identity.py | 46 ++++++++++++++++++- .../tapps-mcp/src/tapps_mcp/server_helpers.py | 7 +++ .../unit/test_user_prompt_submit_hook.py | 11 +++++ 4 files changed, 93 insertions(+), 3 deletions(-) diff --git a/packages/tapps-core/src/tapps_core/agent_identity.py b/packages/tapps-core/src/tapps_core/agent_identity.py index 8e5ae02e..84ff5cdb 100644 --- a/packages/tapps-core/src/tapps_core/agent_identity.py +++ b/packages/tapps-core/src/tapps_core/agent_identity.py @@ -35,6 +35,25 @@ _SLUG_INVALID_RE = re.compile(r"[^a-zA-Z0-9_-]+") +def is_real_writable_root(project_root: object) -> bool: + """Return True only when *project_root* is a real, absolute filesystem path. + + TAP-4573 guard. Production callers always resolve ``project_root`` to an + absolute path (explicit arg, ``TAPPS_MCP_PROJECT_ROOT`` env, or ``cwd``). + A bare ``MagicMock()`` coerces via ``os.fspath`` to the *relative* string + ``MagicMock/mock.project_root/`` (verified empirically), so ~70 test + call sites that pass unspec'd mocks were causing real ``mkdir`` trees under + the pytest CWD (the repo root). Rejecting non-absolute / non-coercible + roots blocks that leak at the single production write path without touching + the tests, and is a no-op for every real deployment (roots are absolute). + """ + try: + raw = os.fspath(project_root) # type: ignore[arg-type] + except (TypeError, ValueError): + return False + return Path(raw).is_absolute() + + def _slugify(value: str) -> str: """Reduce a string to a safe agent-id prefix (alnum/dash/underscore).""" cleaned = _SLUG_INVALID_RE.sub("-", value).strip("-_") @@ -86,7 +105,16 @@ def get_stable_agent_id(settings: TappsMCPSettings) -> str: if override: return override - project_root = Path(getattr(settings, "project_root", Path.cwd())) + raw_root = getattr(settings, "project_root", Path.cwd()) + + # TAP-4573: never mkdir/write under a non-real (mock- or relative-coerced) + # project_root. A bare MagicMock() coerces to a relative "MagicMock/..." + # path, so writing it would create a real tree in the pytest CWD. Skip the + # persistence attempt but still return a valid in-memory agent id. + if not is_real_writable_root(raw_root): + return f"{_project_slug(settings)}-{uuid.uuid4().hex[:_UUID_SHORT_LEN]}" + + project_root = Path(raw_root) id_path = project_root / _AGENT_ID_RELATIVE_PATH uuid_hex = _read_uuid(id_path) @@ -113,4 +141,4 @@ def get_stable_agent_id(settings: TappsMCPSettings) -> str: return f"{_project_slug(settings)}-{short}" -__all__ = ["get_stable_agent_id"] +__all__ = ["get_stable_agent_id", "is_real_writable_root"] diff --git a/packages/tapps-core/tests/unit/test_agent_identity.py b/packages/tapps-core/tests/unit/test_agent_identity.py index 8331c304..8785b416 100644 --- a/packages/tapps-core/tests/unit/test_agent_identity.py +++ b/packages/tapps-core/tests/unit/test_agent_identity.py @@ -5,8 +5,12 @@ import re from pathlib import Path from typing import TYPE_CHECKING +from unittest.mock import MagicMock -from tapps_core.agent_identity import get_stable_agent_id +from tapps_core.agent_identity import ( + get_stable_agent_id, + is_real_writable_root, +) from tapps_core.config.settings import TappsMCPSettings if TYPE_CHECKING: @@ -115,3 +119,43 @@ def test_agent_id_slugifies_unsafe_project_id( assert agent_id.startswith("my-project-v2-") assert _AGENT_ID_RE.match(agent_id) + + +# --------------------------------------------------------------------------- # +# TAP-4573: mock/relative-root write guard +# --------------------------------------------------------------------------- # + + +def test_is_real_writable_root_rejects_mock_and_relative() -> None: + """The guard accepts only absolute filesystem paths.""" + assert is_real_writable_root(Path("/abs/real/path")) is True + assert is_real_writable_root("/abs/real/path") is True + # A bare MagicMock coerces (via os.fspath) to a *relative* "MagicMock/..." + # path — the exact leak vector from TAP-4573. + assert is_real_writable_root(MagicMock().project_root) is False + assert is_real_writable_root(Path("relative/dir")) is False + assert is_real_writable_root(object()) is False + + +def test_mock_settings_creates_no_files( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """A bare MagicMock() settings must not mkdir/write under the CWD. + + Regression guard for TAP-4573: ~70 call sites passed unspec'd mocks whose + ``project_root`` coerced to a relative ``MagicMock/...`` path, leaking real + directory trees into the repo root on every full-suite run. + """ + monkeypatch.delenv("CLAUDE_AGENT_ID", raising=False) + monkeypatch.chdir(tmp_path) + + settings = MagicMock() + settings.memory.project_id = "" + + agent_id = get_stable_agent_id(settings) + + # Still returns a well-formed id (in-memory UUID, no persistence). + assert _AGENT_ID_RE.match(agent_id), f"unexpected format: {agent_id!r}" + # And critically: no MagicMock/ tree was created under the CWD. + assert not (tmp_path / "MagicMock").exists() + assert list(tmp_path.iterdir()) == [] diff --git a/packages/tapps-mcp/src/tapps_mcp/server_helpers.py b/packages/tapps-mcp/src/tapps_mcp/server_helpers.py index d9ef361a..976733a8 100644 --- a/packages/tapps-mcp/src/tapps_mcp/server_helpers.py +++ b/packages/tapps-mcp/src/tapps_mcp/server_helpers.py @@ -654,6 +654,13 @@ def write_session_start_marker(project_root: Path | str) -> None: user prompt yields a reminder. """ try: + # TAP-4573: refuse to mkdir/write under a non-real project_root. A bare + # MagicMock() coerces to a relative "MagicMock/..." path, which would + # otherwise create a real tree in the pytest CWD (the repo root). + from tapps_core.agent_identity import is_real_writable_root + + if not is_real_writable_root(project_root): + return root = project_root if isinstance(project_root, Path) else Path(project_root) sidecar_dir = root / ".tapps-mcp" sidecar_dir.mkdir(parents=True, exist_ok=True) diff --git a/packages/tapps-mcp/tests/unit/test_user_prompt_submit_hook.py b/packages/tapps-mcp/tests/unit/test_user_prompt_submit_hook.py index 3066c6ba..56a47ce8 100644 --- a/packages/tapps-mcp/tests/unit/test_user_prompt_submit_hook.py +++ b/packages/tapps-mcp/tests/unit/test_user_prompt_submit_hook.py @@ -59,6 +59,17 @@ def test_session_start_marker_swallows_errors(self) -> None: # Writer must not raise. write_session_start_marker("/dev/null/cannot-mkdir-here") + def test_session_start_marker_ignores_mock_root( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + # TAP-4573: a bare MagicMock().project_root coerces to a relative + # "MagicMock/..." path. The writer must refuse it rather than create a + # real tree under the pytest CWD. + monkeypatch.chdir(tmp_path) + write_session_start_marker(MagicMock().project_root) + assert not (tmp_path / "MagicMock").exists() + assert list(tmp_path.iterdir()) == [] + def test_checklist_state_marker_emits_brain_event(self, tmp_path: Path) -> None: mock_bridge = MagicMock() mock_bridge.record_kg_event = AsyncMock(return_value={"recorded": True})