Merge pull request #277 from GitHubSecurityLab/anticomputer/redact-secrets-in-logs

Log tool-call environment variable names instead of values

Author: anticomputer

src/seclab_taskflow_agent/mcp_utils.py

@@ -172,6 +172,16 @@ async def call_tool(self, *args: Any, **kwargs: Any) -> Any:
 ClientParamsMap = dict[str, tuple[dict[str, Any], list[str], str | None, int | None]]
 
 
+def _env_names(env: dict[str, Any] | None) -> list[str] | None:
+    """Return only the environment variable names, for safe debug logging.
+
+    Tool-call environments routinely carry credentials (e.g. ``GH_TOKEN``), so
+    their values must never be written to logs. Callers log the names to keep
+    debug output useful without leaking secrets.
+    """
+    return sorted(env) if env is not None else None
+
+
 def mcp_client_params(
     available_tools: AvailableTools,
     requested_toolboxes: list[str],
@@ -202,7 +212,7 @@ def mcp_client_params(
             case "stdio":
                 env = dict(sp.env) if sp.env else None
                 args = list(sp.args) if sp.args else None
-                logging.debug("Initializing toolbox: %s\nargs:\n%s\nenv:\n%s\n", tb, args, env)
+                logging.debug("Initializing toolbox: %s\nargs:\n%s\nenv names:\n%s\n", tb, args, _env_names(env))
                 if env:
                     for k, v in list(env.items()):
                         try:
@@ -217,7 +227,7 @@ def mcp_client_params(
                                       "http_proxy", "https_proxy", "no_proxy"):
                         if proxy_var not in env and proxy_var in os.environ:
                             env[proxy_var] = os.environ[proxy_var]
-                logging.debug("Tool call environment: %s", env)
+                logging.debug("Tool call environment names: %s", _env_names(env))
                 if args:
                     for i, v in enumerate(args):
                         args[i] = swap_env(v)
@@ -241,7 +251,7 @@ def mcp_client_params(
                 if sp.command is not None:
                     env = dict(sp.env) if sp.env else None
                     args = list(sp.args) if sp.args else None
-                    logging.debug("Initializing streamable toolbox: %s\nargs:\n%s\nenv:\n%s\n", tb, args, env)
+                    logging.debug("Initializing streamable toolbox: %s\nargs:\n%s\nenv names:\n%s\n", tb, args, _env_names(env))
                     exe = shutil.which(sp.command)
                     if exe is None:
                         raise FileNotFoundError(f"Could not resolve path to {sp.command}")

tests/test_mcp_utils.py

@@ -6,12 +6,18 @@
 from __future__ import annotations
 
 import asyncio
+import logging
 from types import SimpleNamespace
 from unittest.mock import AsyncMock, MagicMock
 
 import pytest
 
-from seclab_taskflow_agent.mcp_utils import MCPNamespaceWrap, compress_name
+from seclab_taskflow_agent.mcp_utils import (
+    MCPNamespaceWrap,
+    _env_names,
+    compress_name,
+    mcp_client_params,
+)
 
 
 class _FakeTool:
@@ -145,3 +151,48 @@ def test_list_tools_existing_behaviour_unchanged():
     obj.list_tools.assert_awaited_once_with(run_context="ctx", agent="agent")
     ns = compress_name("RepoContext")
     assert result[0].name == f"{ns}read_file"
+
+
+# -- secret redaction in debug logs --
+
+
+def test_mcp_client_params_logs_env_names_not_secret_values(monkeypatch, caplog):
+    # A tool-call environment routinely resolves credentials like GH_TOKEN;
+    # debug logging must record only the variable names, never the values.
+    sentinel = "do-not-log-this-value"
+    monkeypatch.setenv("GH_TOKEN", sentinel)
+    server_params = SimpleNamespace(
+        kind="stdio",
+        reconnecting=False,
+        env={"GH_TOKEN": "{{ env('GH_TOKEN') }}", "LOG_DIR": "/tmp/logs"},
+        args=None,
+        command="echo",
+    )
+    toolbox = SimpleNamespace(
+        server_params=server_params,
+        confirm=[],
+        server_prompt=None,
+        client_session_timeout=None,
+    )
+    available_tools = MagicMock()
+    available_tools.get_toolbox.return_value = toolbox
+
+    with caplog.at_level(logging.DEBUG):
+        params = mcp_client_params(available_tools, ["pkg.tb"])
+
+    # The resolved secret value never reaches the logs ...
+    assert sentinel not in caplog.text
+    # ... but the variable names are still logged for debuggability.
+    assert "GH_TOKEN" in caplog.text
+    assert "LOG_DIR" in caplog.text
+    # Redaction is log-only: the real env (with the resolved secret) is still
+    # passed through to the MCP server.
+    assert params["pkg.tb"][0]["env"]["GH_TOKEN"] == sentinel
+
+
+def test_env_names_returns_sorted_names_or_none():
+    # Non-empty env -> sorted names; empty dict -> empty list; None -> None
+    # (honouring the list[str] | None contract, never leaking values).
+    assert _env_names({"B_VAR": "x", "A_VAR": "{{ env('A') }}"}) == ["A_VAR", "B_VAR"]
+    assert _env_names({}) == []
+    assert _env_names(None) is None