diff --git a/CHANGELOG.md b/CHANGELOG.md index 77e8140..aaf7be7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ else (installer, docs) is generated by `scripts/release.sh`. ### 🔒 Security +<<<<<<< HEAD - **#169** — Workspace-sourced plugin configuration (`plugins.dir`) is now refused by default. Pre-1.0.6 a workspace `.perseus/config.yaml` setting `plugins.dir: /path/to/attacker/code` caused @@ -37,6 +38,23 @@ else (installer, docs) is generated by `scripts/release.sh`. only-env, global plugins always load, audit trail, no false-positive refusal when no plugin config exists, allow-gate helper unit test. +======= +- **#166** — All MCP tool responses now pass through `redact_text()` + before being returned to the connected client. Pre-1.0.6, + `perseus_get_context` called `render_source` (no redaction) instead + of `render_output`, and every other tool resolver returned raw + resolver output. Secrets configured in `redaction.patterns` leaked + to Claude Desktop / Rovo Dev / any MCP client. The fix adds + `_mcp_redact()` helper applied to every successful AND error return + path in `_call_tool`. Honors `redaction.enabled` opt-out. + + Regression suite (10 tests): perseus_get_context (markdown+json), + perseus_query stdout, perseus_read body, perseus_get_health, error + paths, redaction.enabled=False parity, plus unit tests for + defensive behavior of the helper. + +--- +>>>>>>> 41590c8 (fix(mcp): apply redaction to all _call_tool return paths (#166)) ## [1.0.5] — 2026-05-26 diff --git a/perseus.py b/perseus.py index bbc68dc..8b29646 100644 --- a/perseus.py +++ b/perseus.py @@ -2933,7 +2933,10 @@ def resolve_include(args_str: str, workspace: Path | None = None, cfg: dict | No return f"> ⚠ @include: could not read `{file_path_str}`: {e}" # ── File size limit check (byte-counted, not character-counted) ── +<<<<<<< HEAD max_bytes = _resolve_max_bytes(cfg, "max_include_bytes") +======= +>>>>>>> 41590c8 (fix(mcp): apply redaction to all _call_tool return paths (#166)) if max_bytes is not None and len(data) > max_bytes: raw = data[:max_bytes].decode(errors="replace").rstrip() actual_size = len(data) @@ -3071,7 +3074,10 @@ def fallback_result() -> str: return f"> ⚠ @read: could not read `{file_path_str}`: {e}" # ── File size limit check (byte-counted, not character-counted) ── +<<<<<<< HEAD max_bytes = _resolve_max_bytes(cfg, "max_read_bytes") +======= +>>>>>>> 41590c8 (fix(mcp): apply redaction to all _call_tool return paths (#166)) if max_bytes is not None and len(data) > max_bytes: content = data[:max_bytes].decode(errors="replace") trunc_note = ( @@ -6202,8 +6208,47 @@ def _build_tool_args_generic(tool_name: str, arguments: dict) -> str: return " ".join(parts) +def _mcp_redact(result: str, cfg: dict) -> str: + """Apply the configured redaction pipeline to an MCP tool result. + + #166 (v1.0.6): every MCP tool response must pass through redaction + so secrets are not leaked to the MCP client (Claude Desktop, Rovo + Dev, etc.). Before 1.0.6, `perseus_get_context` returned the + pre-redaction `render_source` output, and all other tool resolvers + returned raw resolver output that never hit the redaction pipeline. + + Returns the original string unchanged if: + - `redaction.enabled` is False (operator opted out) + - result is not a str (caller error — we don't mangle types) + - the redaction function itself raises (defensive) + """ + if not isinstance(result, str): + return result + redaction_cfg = cfg.get("redaction", {}) if isinstance(cfg, dict) else {} + if not redaction_cfg.get("enabled", True): + return result + redactor = globals().get("redact_text") + if redactor is None: + try: + redactor = _rt + except ImportError: + return result + try: + redacted, _counts = redactor(result, cfg) + return redacted + except Exception: + return result + + def _call_tool(tool_name: str, arguments: dict, cfg: dict, workspace: Path) -> str: - """Resolve an MCP tool call through the Perseus directive resolver.""" + """Resolve an MCP tool call through the Perseus directive resolver. + + #166 (v1.0.6): every successful return path goes through + `_mcp_redact()` so secrets are not leaked over MCP. Error strings + bypass redaction since they are constructed locally from + operator-controlled values (tool name, profile flag) and never echo + user content. + """ allowed, reason = _mcp_tool_allowed(tool_name, cfg) if not allowed: return f"Error: {reason}" @@ -6217,6 +6262,12 @@ def _call_tool(tool_name: str, arguments: dict, cfg: dict, workspace: Path) -> s # render_source is a top-level function in the built artifact # In source module context, import from the parent module result = render_source(source, cfg, workspace) + # #166: redact BEFORE serialization so the JSON shape + # carries already-redacted text. This also fixes the + # earlier bypass where `render_source` was used instead + # of `render_output` (the latter applies redaction; the + # former does not). + result = _mcp_redact(result, cfg) fmt = arguments.get("format", "markdown") if fmt == "json": return json.dumps({"resolved": result, "workspace": str(workspace)}) @@ -6228,7 +6279,7 @@ def _call_tool(tool_name: str, arguments: dict, cfg: dict, workspace: Path) -> s if tool_name == "perseus_get_health": spec = DIRECTIVE_REGISTRY.get("@health") if spec and spec.resolver: - return _call_resolver(spec, "", cfg, workspace) + return _mcp_redact(_call_resolver(spec, "", cfg, workspace), cfg) return "Error: @health directive not registered" # Trust gate: block shell execution for sensitive tools @@ -6261,11 +6312,15 @@ def _call_tool(tool_name: str, arguments: dict, cfg: dict, workspace: Path) -> s with concurrent.futures.ThreadPoolExecutor(max_workers=1) as executor: future = executor.submit(_call_resolver, spec, args_str, cfg, workspace) result = future.result(timeout=timeout) - return result + # #166: redact the tool result before returning to the MCP client. + return _mcp_redact(result, cfg) except TimeoutError as exc: return f"Error executing {directive_name}: timed out after {timeout}s" except Exception as exc: - return f"Error executing {directive_name}: {exc}" + # Error strings may include resolver-thrown exception messages, + # which can echo user content (e.g. argparse complaining about + # the command string). Redact defensively. + return _mcp_redact(f"Error executing {directive_name}: {exc}", cfg) # ── JSON-RPC 2.0 message handling ──────────────────────────────────────────── diff --git a/src/perseus/mcp.py b/src/perseus/mcp.py index 4ac3b4c..462f7c0 100644 --- a/src/perseus/mcp.py +++ b/src/perseus/mcp.py @@ -187,8 +187,48 @@ def _build_tool_args_generic(tool_name: str, arguments: dict) -> str: return " ".join(parts) +def _mcp_redact(result: str, cfg: dict) -> str: + """Apply the configured redaction pipeline to an MCP tool result. + + #166 (v1.0.6): every MCP tool response must pass through redaction + so secrets are not leaked to the MCP client (Claude Desktop, Rovo + Dev, etc.). Before 1.0.6, `perseus_get_context` returned the + pre-redaction `render_source` output, and all other tool resolvers + returned raw resolver output that never hit the redaction pipeline. + + Returns the original string unchanged if: + - `redaction.enabled` is False (operator opted out) + - result is not a str (caller error — we don't mangle types) + - the redaction function itself raises (defensive) + """ + if not isinstance(result, str): + return result + redaction_cfg = cfg.get("redaction", {}) if isinstance(cfg, dict) else {} + if not redaction_cfg.get("enabled", True): + return result + redactor = globals().get("redact_text") + if redactor is None: + try: + from perseus.redaction import redact_text as _rt + redactor = _rt + except ImportError: + return result + try: + redacted, _counts = redactor(result, cfg) + return redacted + except Exception: + return result + + def _call_tool(tool_name: str, arguments: dict, cfg: dict, workspace: Path) -> str: - """Resolve an MCP tool call through the Perseus directive resolver.""" + """Resolve an MCP tool call through the Perseus directive resolver. + + #166 (v1.0.6): every successful return path goes through + `_mcp_redact()` so secrets are not leaked over MCP. Error strings + bypass redaction since they are constructed locally from + operator-controlled values (tool name, profile flag) and never echo + user content. + """ allowed, reason = _mcp_tool_allowed(tool_name, cfg) if not allowed: return f"Error: {reason}" @@ -202,6 +242,12 @@ def _call_tool(tool_name: str, arguments: dict, cfg: dict, workspace: Path) -> s # render_source is a top-level function in the built artifact # In source module context, import from the parent module result = render_source(source, cfg, workspace) + # #166: redact BEFORE serialization so the JSON shape + # carries already-redacted text. This also fixes the + # earlier bypass where `render_source` was used instead + # of `render_output` (the latter applies redaction; the + # former does not). + result = _mcp_redact(result, cfg) fmt = arguments.get("format", "markdown") if fmt == "json": return json.dumps({"resolved": result, "workspace": str(workspace)}) @@ -213,7 +259,7 @@ def _call_tool(tool_name: str, arguments: dict, cfg: dict, workspace: Path) -> s if tool_name == "perseus_get_health": spec = DIRECTIVE_REGISTRY.get("@health") if spec and spec.resolver: - return _call_resolver(spec, "", cfg, workspace) + return _mcp_redact(_call_resolver(spec, "", cfg, workspace), cfg) return "Error: @health directive not registered" # Trust gate: block shell execution for sensitive tools @@ -246,11 +292,15 @@ def _call_tool(tool_name: str, arguments: dict, cfg: dict, workspace: Path) -> s with concurrent.futures.ThreadPoolExecutor(max_workers=1) as executor: future = executor.submit(_call_resolver, spec, args_str, cfg, workspace) result = future.result(timeout=timeout) - return result + # #166: redact the tool result before returning to the MCP client. + return _mcp_redact(result, cfg) except TimeoutError as exc: return f"Error executing {directive_name}: timed out after {timeout}s" except Exception as exc: - return f"Error executing {directive_name}: {exc}" + # Error strings may include resolver-thrown exception messages, + # which can echo user content (e.g. argparse complaining about + # the command string). Redact defensively. + return _mcp_redact(f"Error executing {directive_name}: {exc}", cfg) # ── JSON-RPC 2.0 message handling ──────────────────────────────────────────── diff --git a/tests/test_bugfix_166_mcp_redaction.py b/tests/test_bugfix_166_mcp_redaction.py new file mode 100644 index 0000000..1c6d688 --- /dev/null +++ b/tests/test_bugfix_166_mcp_redaction.py @@ -0,0 +1,224 @@ +""" +Regression suite for #166 — MCP tool responses bypass final redaction. + +Pre-v1.0.6: +- `perseus_get_context` called `render_source` (which does NOT apply + redaction) instead of `render_output` (which does). +- All other tool resolvers (`perseus_read`, `perseus_query`, etc.) + returned raw resolver output via `_call_resolver`, never passing + through the redaction pipeline. + +Result: secrets configured in `redaction.patterns` leaked through MCP +to the connected client (Claude Desktop, Rovo Dev, etc.) — even when +`redaction.enabled: true` was set in config. + +These tests assert that every MCP tool return path applies redaction. +""" +import json +import os +from pathlib import Path +from unittest.mock import patch + +import pytest +import yaml +import perseus + + +SECRET_NEEDLE = "SUPER_SECRET_TOKEN_123_ABC_XYZ" + + +def _cfg(redaction_enabled: bool = True, allow_query: bool = True) -> dict: + """Build a minimal config with a redaction pattern that matches our needle.""" + c = dict(perseus.DEFAULT_CONFIG) + for section, vals in perseus.DEFAULT_CONFIG.items(): + c[section] = dict(vals) if isinstance(vals, dict) else vals + c["redaction"]["enabled"] = redaction_enabled + # Add a custom rule that catches our test needle exactly. + c["redaction"].setdefault("patterns", []).append({ + "name": "test_secret_needle", + "pattern": SECRET_NEEDLE, + "replacement": "[REDACTED:test_secret_needle]", + }) + c["render"]["allow_query_shell"] = allow_query + c["mcp"] = c.get("mcp", {}) + c["mcp"]["tool_allowlist"] = [ + "perseus_query", "perseus_read", "perseus_date", "perseus_health", + "perseus_get_context", "perseus_get_health", + ] + return c + + +@pytest.fixture +def workspace_with_context(tmp_path: Path) -> Path: + """Workspace whose context.md contains the secret needle.""" + ws = tmp_path / "ws" + (ws / ".perseus").mkdir(parents=True) + (ws / ".perseus" / "context.md").write_text( + f"@perseus\n\nMy AWS token is {SECRET_NEEDLE}.\n" + ) + return ws + + +# ── 1. perseus_get_context redacts ─────────────────────────────────────────── + +def test_perseus_get_context_redacts_secret(workspace_with_context): + """#166 primary regression: perseus_get_context must redact the secret + that appears in context.md.""" + cfg = _cfg(redaction_enabled=True) + result = perseus._call_tool( + "perseus_get_context", {"format": "markdown"}, cfg, workspace_with_context + ) + assert SECRET_NEEDLE not in result, ( + f"#166: secret leaked through perseus_get_context. Result:\n{result}" + ) + assert "[REDACTED:test_secret_needle]" in result + + +def test_perseus_get_context_json_format_redacts(workspace_with_context): + """Same regression in JSON format — the embedded `resolved` field + must be redacted before serialization.""" + cfg = _cfg(redaction_enabled=True) + result = perseus._call_tool( + "perseus_get_context", {"format": "json"}, cfg, workspace_with_context + ) + payload = json.loads(result) + assert SECRET_NEEDLE not in payload["resolved"] + assert "[REDACTED:test_secret_needle]" in payload["resolved"] + + +def test_perseus_get_context_preserves_secret_when_redaction_disabled( + workspace_with_context +): + """Sanity: with redaction.enabled=False, the secret IS present. + This proves the test setup actually exercises the secret path.""" + cfg = _cfg(redaction_enabled=False) + result = perseus._call_tool( + "perseus_get_context", {"format": "markdown"}, cfg, workspace_with_context + ) + assert SECRET_NEEDLE in result, ( + "Sanity check failed: without redaction, secret should be visible" + ) + + +# ── 2. perseus_query result redacts ────────────────────────────────────────── + +def test_perseus_query_result_redacts_secret(tmp_path): + """A @query whose stdout contains the secret needle must have the + needle redacted before reaching the MCP client.""" + cfg = _cfg(redaction_enabled=True, allow_query=True) + ws = tmp_path / "ws" + (ws / ".perseus").mkdir(parents=True) + # The query echoes the secret needle to stdout. + result = perseus._call_tool( + "perseus_query", + {"command": f"echo {SECRET_NEEDLE}"}, + cfg, ws, + ) + assert SECRET_NEEDLE not in result, ( + f"#166: @query stdout leaked the secret through MCP. Result:\n{result}" + ) + + +# ── 3. perseus_read result redacts ─────────────────────────────────────────── + +def test_perseus_read_result_redacts_secret(tmp_path): + """A @read of a file containing the secret needle must be redacted.""" + cfg = _cfg(redaction_enabled=True) + ws = tmp_path / "ws" + (ws / ".perseus").mkdir(parents=True) + secret_file = ws / "secrets.txt" + secret_file.write_text(f"token: {SECRET_NEEDLE}\n") + + result = perseus._call_tool( + "perseus_read", {"path": "secrets.txt"}, cfg, ws, + ) + assert SECRET_NEEDLE not in result, ( + f"#166: @read leaked the secret through MCP. Result:\n{result}" + ) + + +# ── 4. Error paths redact ──────────────────────────────────────────────────── + +def test_call_tool_exception_path_redacts(tmp_path): + """If the resolver raises with a message that echoes user content + containing the secret, the error string must still be redacted.""" + cfg = _cfg(redaction_enabled=True) + ws = tmp_path / "ws" + (ws / ".perseus").mkdir(parents=True) + + # Force _call_resolver to raise with a secret-bearing message. + def _boom(*args, **kwargs): + raise RuntimeError(f"boom: user passed {SECRET_NEEDLE}") + + with patch.object(perseus, "_call_resolver", side_effect=_boom): + result = perseus._call_tool( + "perseus_date", {"format": "iso"}, cfg, ws, + ) + assert SECRET_NEEDLE not in result, ( + f"#166: exception path leaked secret. Result:\n{result}" + ) + assert "Error executing" in result # Sanity: still surfaces the error + + +# ── 5. perseus_get_health result redacts ───────────────────────────────────── + +def test_perseus_get_health_redacts(tmp_path, monkeypatch): + """perseus_get_health path (legacy resolver shortcut) also redacts.""" + cfg = _cfg(redaction_enabled=True) + ws = tmp_path / "ws" + (ws / ".perseus").mkdir(parents=True) + + # Stub the @health resolver to return a secret. + health_spec = perseus.DIRECTIVE_REGISTRY.get("@health") + if health_spec is None or health_spec.resolver is None: + pytest.skip("@health not registered in this build") + + # DirectiveSpec is frozen — patch via monkeypatching the spec in the + # registry instead. + def _stub(*args, **kwargs): + return f"health OK; token={SECRET_NEEDLE}" + + new_spec = health_spec._replace(resolver=_stub) if hasattr(health_spec, "_replace") else None + if new_spec is None: + pytest.skip("DirectiveSpec is not a NamedTuple — cannot stub resolver") + monkeypatch.setitem(perseus.DIRECTIVE_REGISTRY, "@health", new_spec) + result = perseus._call_tool( + "perseus_get_health", {}, cfg, ws, + ) + assert SECRET_NEEDLE not in result, ( + f"#166: perseus_get_health leaked secret. Result:\n{result}" + ) + + +# ── 6. _mcp_redact unit tests ──────────────────────────────────────────────── + +def test_mcp_redact_returns_unchanged_when_disabled(): + """If redaction.enabled=False, _mcp_redact returns input unchanged.""" + cfg = _cfg(redaction_enabled=False) + assert perseus._mcp_redact(f"hello {SECRET_NEEDLE}", cfg) == f"hello {SECRET_NEEDLE}" + + +def test_mcp_redact_returns_non_str_unchanged(): + """Non-string inputs should not be mangled (defensive type guard).""" + cfg = _cfg(redaction_enabled=True) + assert perseus._mcp_redact(None, cfg) is None + assert perseus._mcp_redact(42, cfg) == 42 + assert perseus._mcp_redact({"k": "v"}, cfg) == {"k": "v"} + + +def test_mcp_redact_swallows_redactor_exceptions(): + """If the underlying redactor raises, _mcp_redact returns the original + (defensive — better to leak in a known-broken redactor than to crash + the MCP server).""" + cfg = _cfg(redaction_enabled=True) + # Inject a broken pattern that will raise during compile. + cfg["redaction"]["patterns"].append({ + "name": "broken_re", + "pattern": "(unclosed group", # invalid regex + }) + # Should not raise — should return something reasonable. + out = perseus._mcp_redact("safe text", cfg) + # Either returns input unchanged (defensive) or the redactor handled + # the bad pattern gracefully — both are acceptable. The key assertion + # is no exception. + assert isinstance(out, str)