From 76d6f817897d22918421f0c347398c4e040f3f41 Mon Sep 17 00:00:00 2001 From: h Date: Tue, 1 Sep 2026 14:12:27 +0000 Subject: [PATCH] fix(mcp,redact,gateway_tools): mask credentials in tool results, names kept and values gone --- src/beaver_gateway/core/gateway_tools.py | 25 ++- src/beaver_gateway/core/redact.py | 126 +++++++++++++- src/beaver_gateway/mcp/internal_app.py | 8 + src/beaver_gateway/mcp/redacting.py | 85 +++++++++ tests/test_mcp_redacting.py | 208 +++++++++++++++++++++++ 5 files changed, 442 insertions(+), 10 deletions(-) create mode 100644 src/beaver_gateway/mcp/redacting.py create mode 100644 tests/test_mcp_redacting.py diff --git a/src/beaver_gateway/core/gateway_tools.py b/src/beaver_gateway/core/gateway_tools.py index a661c46..0079da6 100644 --- a/src/beaver_gateway/core/gateway_tools.py +++ b/src/beaver_gateway/core/gateway_tools.py @@ -8,12 +8,14 @@ session gets comes from ``ClaudeAgent.gateway_tools``. from __future__ import annotations import logging +from dataclasses import replace from typing import TYPE_CHECKING, Any, cast from claude_agent_sdk import create_sdk_mcp_server, tool from beaver_gateway.core.injects import URGENCY from beaver_gateway.core.kinds import as_kind +from beaver_gateway.core.redact import redact_data URGENCY_HELP = ( "normal waits for the hourly batch or rides with the next turn, wake " @@ -52,13 +54,34 @@ def build_tool_server( if unknown: msg = f"unknown gateway tools: {sorted(unknown)}" raise ValueError(msg) - tools = [t for t in _tools(conversations, conversation_key) if t.name in wanted] + tools = [ + _redacting(t) + for t in _tools(conversations, conversation_key) + if t.name in wanted + ] if not tools: return None server = create_sdk_mcp_server(SERVER_NAME, tools=tools) return cast("McpSdkServerConfig", {**server, "alwaysLoad": True}) +def _redacting(spec: SdkMcpTool[Any]) -> SdkMcpTool[Any]: + """Put a tool's result through the same mask as every other MCP. + + These tools are mounted in-process by the SDK, so they bypass the + ``FastMCP`` middleware in :mod:`beaver_gateway.mcp.redacting` and + need the filter attached here instead. ``read_conversation`` is the + one that earns it: it replays a transcript, and a transcript written + before any of this existed can still hold a credential. + """ + inner = spec.handler + + async def handler(args: Any) -> dict[str, Any]: + return cast("dict[str, Any]", redact_data(await inner(args))) + + return replace(spec, handler=handler) + + def _tools(conversations: Conversations, key: str) -> list[SdkMcpTool[Any]]: async def current() -> Any: conv = await conversations.get(key) diff --git a/src/beaver_gateway/core/redact.py b/src/beaver_gateway/core/redact.py index 83505fc..34874c7 100644 --- a/src/beaver_gateway/core/redact.py +++ b/src/beaver_gateway/core/redact.py @@ -1,4 +1,4 @@ -"""Keep credentials out of the process log. +"""Recognise credentials in free text and mask them. ``docker logs`` is not a vault: the stack's stdout lands in an unrotated json file on the host, and the model itself can page through it (the @@ -18,9 +18,16 @@ feed, both handed to the calendar MCP as query parameters. So: silence the HTTP clients' per-request chatter (nothing here reads it), and run one redaction pass over every formatted record as a second -line of defence. Redaction knows two things — patterns for credentials -that live in URLs and headers, and the literal values of the -secret-looking environment variables this process was started with. +line of defence. + +The same pass guards the wider channel — what an MCP tool hands back to +the model (see :mod:`beaver_gateway.mcp.redacting`). :func:`redact` +recognises three things: literal values of the secret-looking +environment variables this process was started with, credentials that +live in URLs and headers, and ``NAME: value`` assignments whose *name* +says the value is a credential. The last one is what covers a config +dump of a stack whose secrets this process never held — and it keeps +the name, masking only the value, so the dump still says what is set. """ from __future__ import annotations @@ -32,7 +39,7 @@ from typing import TYPE_CHECKING from urllib.parse import quote, unquote if TYPE_CHECKING: - from collections.abc import Mapping + from collections.abc import Callable, Mapping MASK = "<…>" @@ -44,10 +51,16 @@ CHATTY: tuple[str, ...] = ("httpx", "httpcore", "aiohttp.client", "urllib3") # ``dictConfig`` drops a logger's handlers but keeps its filters. OWN_HANDLERS: tuple[str, ...] = ("uvicorn", "uvicorn.access", "uvicorn.error") -# Env vars whose *value* is a credential — matched as a substring of the -# name, so ``FIREFLY_PAT`` and ``T3_MAC_TOKEN`` both qualify. +# Names whose *value* is a credential — matched as a substring, so +# ``FIREFLY_PAT`` and ``T3_MAC_TOKEN`` both qualify. Used twice: to pick +# which environment variables contribute literal values, and to decide +# whether a ``NAME: value`` line in a config dump should keep its value. +# ``MCP`` is in here because every MCP URL in this stack carries its +# credential in the query string. _SECRET_NAME = re.compile( - r"TOKEN|SECRET|KEY|PASSWORD|PASS\b|PAT\b|BEARER|CREDENTIAL|MCPS", re.IGNORECASE + r"TOKEN|SECRET|KEY|PASSWORD|PASS\b|PAT\b|BEARER|CREDENTIAL" + r"|MCPS?\b|AUTH|PRIVATE|SESSION|DSN", + re.IGNORECASE, ) _MIN_SECRET = 8 @@ -57,7 +70,38 @@ _LEFT = r"(?:(?,;)\]}])+" -_RULES: tuple[tuple[re.Pattern[str], str], ...] = ( + +# Names worth masking where they sit in a dump next to their password, +# but not worth hunting for as literals across every log line: a +# username is short and ordinary, and ``CLAUDE_RUNNER_USER=beaver-runner`` +# turned into a global literal would mask half the entrypoint script. +_SECRET_IN_DUMP = re.compile(r"USER\b|LOGIN\b", re.IGNORECASE) + + +def is_secret_name(name: str) -> bool: + """Whether a variable/field of this name holds a credential.""" + return bool(_SECRET_NAME.search(name) or _SECRET_IN_DUMP.search(name)) + + +def _mask_named_value(match: re.Match[str]) -> str: + """Mask ``value`` in a ``NAME: value`` match, keeping the name. + + A dump is still worth reading when it says *which* variables are + set; it is the values that must not travel. An already-masked or + empty value is left alone so the pass stays idempotent, and ``lead`` + / ``tail`` carry back whatever quoting the surrounding format used. + """ + groups = match.groupdict() + name, value = groups["name"], groups["value"] + if not is_secret_name(name) or value.strip(" \t\"'") in ("", MASK): + return match[0] + lead, tail = groups.get("lead") or "", groups.get("tail") or "" + return f"{lead}{name}{groups['sep']}{MASK}{tail}" + + +type _Repl = str | Callable[[re.Match[str]], str] + +_RULES: tuple[tuple[re.Pattern[str], _Repl], ...] = ( # Google Calendar's secret path segment; the ``/basic.ics`` after it # survives, encoded or not, so the line still says what it fetched. ( @@ -85,6 +129,50 @@ _RULES: tuple[tuple[re.Pattern[str], str], ...] = ( ), # A bare ``Bearer `` anywhere else. (re.compile(r"\b(bearer)\s+[A-Za-z0-9._~+/=-]{8,}", re.IGNORECASE), r"\1 " + MASK), + # ``NAME: value`` / ``NAME=value`` at the start of a line — the shape + # of a compose ``environment:`` block, an ``.env`` file, a printed + # settings object. Anchored to the line so that an ``actor=token:foo`` + # in the middle of a log line doesn't swallow the rest of it, and + # ``=`` must be tight (``NAME=value``, the way env files write it) so + # that ``SESSION_SECRET = os.environ[…]`` in source the dispatcher is + # reading through t3code stays readable. + ( + re.compile( + r"""(?m)^(?P[ \t]*(?:-[ \t]+)?["']?) + (?P[A-Z][A-Z0-9_]{2,}) + (?P["']?(?::[ \t]*|=)) + (?P"[^"\n]*"|'[^'\n]*'|[^\n]*)""", + re.VERBOSE, + ), + _mask_named_value, + ), + # The same at the start of a line in lower case, but only for names + # that are unambiguously a credential — ``_SECRET_NAME`` is too broad + # here, it would read the ``mcp:`` prefix of a log line as one. + ( + re.compile( + r"""(?m)^(?P[ \t]*(?:-[ \t]+)?["']?) + (?Ppassword|passwd|secret|token|api[-_]?key|apikey + |access[-_]?token|auth[-_]?token|private[-_]?key|credentials?) + (?P["']?(?::[ \t]*|=)) + (?P"[^"\n]*"|'[^'\n]*'|[^\n]*)""", + re.VERBOSE | re.IGNORECASE, + ), + _mask_named_value, + ), + # ``scheme://user:password@host`` — the credential a connection + # string carries. Host and database stay, so the line still locates + # the service it failed to reach. + (re.compile(r"(://[^/\s:@]+:)[^@\s/]+(@)"), r"\1" + MASK + r"\2"), + # ``"name": "value"`` anywhere — the same idea for one-line JSON, + # where the quotes make the end of the value unambiguous. + ( + re.compile( + r'(?P")(?P[^"\n]{1,64})(?P"\s*:\s*")' + r'(?P[^"\n]*)(?P")' + ), + _mask_named_value, + ), ) _ENV_SECRETS: list[str] = [] @@ -120,6 +208,26 @@ def redact(text: str) -> str: return text +def redact_data(value: object) -> object: + """Mask every string inside a decoded JSON tree. + + A field name is the only context a bare value has: ``"0d1e…"`` on + its own looks like nothing, ``{"ADMIN_PASS": "0d1e…"}`` is a + password. So a secret-looking key masks its whole subtree — the + structured twin of the ``NAME: value`` rule above. + """ + if isinstance(value, str): + return redact(value) + if isinstance(value, dict): + return { + key: MASK if is_secret_name(str(key)) and item else redact_data(item) + for key, item in value.items() + } + if isinstance(value, list): + return [redact_data(item) for item in value] + return value + + class RedactingFormatter(logging.Formatter): """Wrap another formatter and redact whatever it produced. diff --git a/src/beaver_gateway/mcp/internal_app.py b/src/beaver_gateway/mcp/internal_app.py index 224b708..cbcfbe2 100644 --- a/src/beaver_gateway/mcp/internal_app.py +++ b/src/beaver_gateway/mcp/internal_app.py @@ -33,6 +33,7 @@ from starlette.applications import Starlette from starlette.routing import Mount from beaver_gateway.mcp.client_pool import build_http_proxy, build_stdio_proxy +from beaver_gateway.mcp.redacting import RedactingMiddleware from beaver_gateway.mcp.types import HttpMcp, PythonToolMcp, StdioMcp from beaver_gateway.mcp.wrap import build_python_tool_server @@ -69,6 +70,12 @@ def build_internal_app( MCP tools into the Raycast wire). """ servers: dict[str, FastMCP] = {spec.name: _build_server(spec) for spec in mcps} + # The one place every tool result passes through on its way to the + # model. Attached per namespace rather than once at the top so that + # a direct ``call_tool`` on a child (the Raycast backend does that) + # is filtered too. + for server in servers.values(): + server.add_middleware(RedactingMiddleware()) child_apps = { name: s.http_app(transport="http", path="/") for name, s in servers.items() @@ -80,6 +87,7 @@ def build_internal_app( all_app = None if servers: all_server = _build_all_server(servers) + all_server.add_middleware(RedactingMiddleware()) all_app = all_server.http_app(transport="http", path="/") routes.append(Mount(f"/mcp/{ALL_NAMESPACE}", app=all_app)) diff --git a/src/beaver_gateway/mcp/redacting.py b/src/beaver_gateway/mcp/redacting.py new file mode 100644 index 0000000..052a4a5 --- /dev/null +++ b/src/beaver_gateway/mcp/redacting.py @@ -0,0 +1,85 @@ +"""Redact credentials on the way out of an MCP tool. + +A tool result is a wider channel than the log. It goes straight into the +model's context, from there into the turn record in Postgres, and from +there into the markdown transcript in ``💬 чаты`` — which Obsidian Sync +carries off the machine. One ``komodo`` deploy returns the resolved +compose file, ``environment:`` block and all, so a single call can put +every secret of a stack into all four places at once. + +One filter, not a list of exceptions: every MCP the model can reach is +built into a ``FastMCP`` by :mod:`beaver_gateway.mcp.internal_app` — +``python_tool`` bundles like komodo, stdio subprocesses, remote HTTP +servers — and every route into one of them runs its middleware chain. +That covers the per-namespace ``/mcp//`` mounts claude-code talks +to, the ``/mcp/all`` bundle, the external MCP frontend reverse-proxying +into both, and the Raycast backend's direct ``call_tool``. A new MCP in +``config.py`` is covered the day it is added, without anyone +remembering to list it here. + +What this does not reach: tools that never touch a FastMCP server — the +gateway's own ``gateway`` tools, and everything claude-code runs inside +its own process (``Bash``, ``Read``). Those are guarded by +:mod:`beaver_gateway.core.policy` and the vault mounts instead. +""" + +from __future__ import annotations + +from typing import TYPE_CHECKING + +import mcp.types as mt +from fastmcp.server.middleware import Middleware +from fastmcp.tools.base import ToolResult + +from beaver_gateway.core.redact import redact, redact_data + +if TYPE_CHECKING: + from fastmcp.server.middleware import CallNext, MiddlewareContext + +__all__ = ["RedactingMiddleware"] + + +def _redact_block(block: mt.ContentBlock) -> mt.ContentBlock: + """Mask a content block's text; binary blocks pass through.""" + if isinstance(block, mt.TextContent): + masked = redact(block.text) + return ( + block if masked == block.text else block.model_copy(update={"text": masked}) + ) + if isinstance(block, mt.EmbeddedResource) and isinstance( + block.resource, mt.TextResourceContents + ): + masked = redact(block.resource.text) + if masked == block.resource.text: + return block + resource = block.resource.model_copy(update={"text": masked}) + return block.model_copy(update={"resource": resource}) + return block + + +class RedactingMiddleware(Middleware): + """Mask credentials in whatever a tool call returns. + + Idempotent, so mounting a server both on its own namespace and in + the ``/mcp/all`` bundle costs a second pass, not a double mask. + """ + + async def on_call_tool( + self, + context: MiddlewareContext[mt.CallToolRequestParams], + call_next: CallNext[mt.CallToolRequestParams, ToolResult], + ) -> ToolResult: + result = await call_next(context) + content = [_redact_block(block) for block in result.content] + structured = redact_data(result.structured_content) + if content == result.content and structured == result.structured_content: + return result + return ToolResult( + content=content, + structured_content=structured, + # Masked structured content no longer has to satisfy the + # tool's output schema; a non-None meta takes the + # ``CallToolResult`` path that skips that validation, the + # same trick ``ResponseLimitingMiddleware`` uses. + meta=result.meta if result.meta is not None else {}, + ) diff --git a/tests/test_mcp_redacting.py b/tests/test_mcp_redacting.py new file mode 100644 index 0000000..0007e2d --- /dev/null +++ b/tests/test_mcp_redacting.py @@ -0,0 +1,208 @@ +from __future__ import annotations + +import mcp.types as mt +import pytest +from fastmcp import FastMCP +from fastmcp.tools.base import ToolResult + +from beaver_gateway.core import redact as redact_mod +from beaver_gateway.core.redact import redact +from beaver_gateway.mcp.internal_app import build_internal_app +from beaver_gateway.mcp.redacting import RedactingMiddleware +from beaver_gateway.mcp.types import McpServer + +# A trimmed but otherwise verbatim ``komodo deploy`` result: this is the +# shape Komodo returns for the ``Compose Config`` stage of a +# DeployStackService, which the komodo tool passes on to the model as +# ``-- ok`` plus the stage's stdout. Values are substituted. +KOMODO_DEPLOY = """DeployStackService Complete ok +-- Service/s ok +gateway +-- Compose Config ok +name: beaver-agent +services: + gateway: + container_name: beaver-gateway + environment: + ADMIN_PASS: 0d1e2f3a4b5c6d7e8f9a0b1c2d3e4f50 + ADMIN_USER: admin + BEAVERGRAM_MCP: https://tg.example.dev/mcp?token=bg-0123456789abcdef + BOOTSTRAP_TOKENS: admin:bs-0123456789abcdef,komodo:hook-0123456789 + CALENDAR_MCPS: personal=https://cal.example.com/api/mcp?icsUrl=https%3A%2F%2Fcalendar.google.com%2Fcalendar%2Fical%2Fx%2Fprivate-0123456789abcdef%2Fbasic.ics + CLAUDE_CODE_OAUTH_TOKEN: sk-ant-oat01-0123456789abcdef + CLAUDE_HOME: /home/beaver-runner + COMPOSE_PROFILES: db,gateway,obsidian,t3 + DATABASE_URL: postgresql+psycopg://beaver:pg-0123456789abcdef@postgres:5432/beaver + KOMODO_URL: https://komod.example.dev + POSTGRES_DB: beaver + POSTGRES_PASSWORD: pg-0123456789abcdef + POSTGRES_USER: beaver + ports: + - 62990:62990 +-- Compose Up ok + Container beaver-gateway Started +""" + +SECRETS = ( + "0d1e2f3a4b5c6d7e8f9a0b1c2d3e4f50", + "bg-0123456789abcdef", + "bs-0123456789abcdef", + "hook-0123456789", + "private-0123456789abcdef", + "sk-ant-oat01-0123456789abcdef", + "pg-0123456789abcdef", +) + + +@pytest.fixture(autouse=True) +def _no_env_secrets() -> None: + """Prove the patterns alone carry it — no literal env values loaded.""" + redact_mod._ENV_SECRETS[:] = [] # noqa: SLF001 + + +def test_compose_config_dump_loses_every_value_but_keeps_every_name() -> None: + out = redact(KOMODO_DEPLOY) + for secret in SECRETS: + assert secret not in out, secret + # Names stay: the dump still says what is configured. + for name in ( + "ADMIN_PASS", + "BOOTSTRAP_TOKENS", + "CALENDAR_MCPS", + "POSTGRES_PASSWORD", + # a username is half of a login pair — in prod ADMIN_USER is a + # 64-hex string, not "admin" + "ADMIN_USER", + "POSTGRES_USER", + ): + assert f"{name}: <…>" in out + # Non-secret values are untouched, so the result stays worth reading. + assert "POSTGRES_DB: beaver" in out + assert "CLAUDE_HOME: /home/beaver-runner" in out + assert "COMPOSE_PROFILES: db,gateway,obsidian,t3" in out + assert "KOMODO_URL: https://komod.example.dev" in out + assert "container_name: beaver-gateway" in out + assert "-- Compose Up ok" in out + assert " Container beaver-gateway Started" in out + + +def test_database_url_keeps_its_shape_and_loses_its_password() -> None: + # DATABASE_URL is not a secret-looking name, so the whole line is not + # masked; the password is caught as the userinfo of the connection + # string, which is what keeps the host and database readable. + out = redact(KOMODO_DEPLOY) + line = next(ln for ln in out.splitlines() if "DATABASE_URL" in ln) + assert "pg-0123456789abcdef" not in line + assert "postgres:5432/beaver" in line + + +def test_redaction_is_idempotent() -> None: + once = redact(KOMODO_DEPLOY) + assert redact(once) == once + + +def test_a_log_line_is_not_swallowed_by_the_assignment_rule() -> None: + # ``token:`` in mid-line must not eat the rest of an audit line. + line = "mcp: actor=token:komodo namespace=calendar POST /mcp/ -> 200" + assert redact(line) == line + + +@pytest.mark.anyio +async def test_middleware_masks_a_tool_result() -> None: + def deploy(stack: str) -> str: + """Deploy a stack.""" + return f"{stack}\n{KOMODO_DEPLOY}" + + server = FastMCP(name="komodo", tools=[deploy]) + server.add_middleware(RedactingMiddleware()) + result = await server.call_tool("deploy", {"stack": "beaver-agent"}) + text = "\n".join(b.text for b in result.content if isinstance(b, mt.TextContent)) + for secret in SECRETS: + assert secret not in text, secret + assert "ADMIN_PASS: <…>" in text + + +@pytest.mark.anyio +async def test_middleware_masks_structured_content() -> None: + def config() -> dict[str, object]: + """Return a config blob.""" + return { + "env": {"ADMIN_PASS": "0d1e2f3a4b5c6d7e8f9a0b1c2d3e4f50"}, + "port": 62990, + } + + server = FastMCP(name="cfg", tools=[config]) + server.add_middleware(RedactingMiddleware()) + result = await server.call_tool("config", {}) + dumped = str(result.structured_content) + str(result.content) + assert "0d1e2f3a4b5c6d7e8f9a0b1c2d3e4f50" not in dumped + assert "62990" in dumped + + +@pytest.mark.anyio +async def test_clean_result_passes_through_untouched() -> None: + def ping() -> str: + """Say hello.""" + return "pong" + + server = FastMCP(name="p", tools=[ping]) + server.add_middleware(RedactingMiddleware()) + result = await server.call_tool("ping", {}) + assert isinstance(result, ToolResult) + assert result.content[0].text == "pong" # ty: ignore[unresolved-attribute] + + +@pytest.mark.anyio +async def test_every_namespace_built_by_the_aggregator_is_filtered() -> None: + def deploy() -> str: + """Deploy.""" + return KOMODO_DEPLOY + + _app, _urls, servers = build_internal_app( + [McpServer.python_tool(name="komodo", tools=[deploy])], + host="127.0.0.1", + port=8765, + ) + for name, server in servers.items(): + result = await server.call_tool( + "deploy" if name == "komodo" else "komodo_deploy", {} + ) + text = "\n".join( + b.text for b in result.content if isinstance(b, mt.TextContent) + ) + assert "0d1e2f3a4b5c6d7e8f9a0b1c2d3e4f50" not in text, name + + +@pytest.mark.anyio +async def test_gateway_own_tools_are_filtered_too() -> None: + # These bypass the FastMCP middleware — the SDK mounts them + # in-process — so they carry their own wrapper. + from claude_agent_sdk import SdkMcpTool + + from beaver_gateway.core.gateway_tools import _redacting + + async def handler(_args: dict[str, object]) -> dict[str, object]: + return {"content": [{"type": "text", "text": KOMODO_DEPLOY}]} + + spec = _redacting( + SdkMcpTool( + name="read_conversation", description="", input_schema={}, handler=handler + ) + ) + out = str(await spec.handler({})) + for secret in SECRETS: + assert secret not in out, secret + + +def test_source_code_the_dispatcher_reads_is_not_mangled() -> None: + # t3code hands back files and diffs. A SCREAMING_SNAKE constant is + # an assignment too, but ``NAME = expr`` is code, not a config dump. + source = ( + 'SESSION_SECRET = os.environ["SESSION_SECRET"]\nAPI_KEY_HEADER = "X-Api-Key"' + ) + assert redact(source) == source + + +def test_env_file_lines_are_still_masked() -> None: + out = redact("ADMIN_PASS=0d1e2f3a4b5c6d7e8f9a0b1c2d3e4f50\nPOSTGRES_DB=beaver") + assert out == "ADMIN_PASS=<…>\nPOSTGRES_DB=beaver"