mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-10-26 13:16:24 +00:00
fix(mcp): avoid leaking credentials in discovery error logs (#6794)
* fix(mcp): keep resolved credentials out of discovery failure logs A failed per-server MCP discovery logged the raw exception message and traceback. ExtensionsConfig resolves $VAR values before validation, so a discovery error can embed a resolved-credential request URL (for example an httpx error carrying a URL with userinfo). Log only the server name and the exception type instead, at both the per-server and whole-phase handlers, and pin it with a caplog regression test using a simulated credential. * test(mcp): assert the exact retained server name when redacting logs The redaction regression test asserted a bare "A" in the captured log, which any unrelated A would satisfy. Assert the full "server 'A'" fragment so the test actually proves the offending server name is preserved while the credential is not. * test(mcp): assert structured discovery warning arguments The redaction change made the per-server discovery warning a parameterized log call, so asserting the server name inside the raw format string no longer matches. Assert the structured arguments instead: the server name, the exception type, the fully formatted message, and the absence of exc_info.
This commit is contained in:
parent
c5bbaea02f
commit
9fd88c5ffc
@ -1086,9 +1086,14 @@ async def _discover_server_tools(
|
|||||||
except (asyncio.CancelledError, StaleMCPBindingError, McpTaskConfigurationError):
|
except (asyncio.CancelledError, StaleMCPBindingError, McpTaskConfigurationError):
|
||||||
raise
|
raise
|
||||||
except Exception as e:
|
except Exception as e:
|
||||||
|
# Never interpolate ``e`` nor attach its traceback: ``ExtensionsConfig``
|
||||||
|
# resolves ``$VAR`` before validation, so a discovery error message can
|
||||||
|
# embed a resolved-credential request URL. Keep the server name and the
|
||||||
|
# exception type for diagnosis.
|
||||||
logger.warning(
|
logger.warning(
|
||||||
f"Skipping MCP server '{server_name}' after tool discovery failed: {e}",
|
"Skipping MCP server '%s' after tool discovery failed (%s)",
|
||||||
exc_info=True,
|
server_name,
|
||||||
|
type(e).__name__,
|
||||||
)
|
)
|
||||||
return server_name, None
|
return server_name, None
|
||||||
|
|
||||||
@ -1432,5 +1437,7 @@ async def get_mcp_tools(
|
|||||||
except (McpTaskConfigurationError, StaleMCPBindingError):
|
except (McpTaskConfigurationError, StaleMCPBindingError):
|
||||||
raise
|
raise
|
||||||
except Exception as e:
|
except Exception as e:
|
||||||
logger.error(f"Failed to load MCP tools: {e}", exc_info=True)
|
# Only the exception type is logged: the message/traceback can carry a
|
||||||
|
# resolved-credential request URL (see ``_discover_server_tools``).
|
||||||
|
logger.error("Failed to load MCP tools (%s)", type(e).__name__)
|
||||||
return []
|
return []
|
||||||
|
|||||||
@ -7,6 +7,7 @@ import builtins
|
|||||||
import logging
|
import logging
|
||||||
from unittest.mock import AsyncMock, MagicMock
|
from unittest.mock import AsyncMock, MagicMock
|
||||||
|
|
||||||
|
import httpx
|
||||||
import pytest
|
import pytest
|
||||||
from langchain_core.tools import StructuredTool
|
from langchain_core.tools import StructuredTool
|
||||||
|
|
||||||
@ -711,3 +712,37 @@ async def test_discovery_and_wrapper_use_the_same_session_init_timeout(monkeypat
|
|||||||
assert len(groups["A"].tools) == 1
|
assert len(groups["A"].tools) == 1
|
||||||
assert observed["discovery"] == 5.0
|
assert observed["discovery"] == 5.0
|
||||||
assert observed["wrapper"] == observed["discovery"]
|
assert observed["wrapper"] == observed["discovery"]
|
||||||
|
|
||||||
|
|
||||||
|
@pytest.mark.asyncio
|
||||||
|
async def test_per_server_discovery_failure_log_redacts_resolved_credentials(monkeypatch, caplog):
|
||||||
|
"""A failed per-server discovery must not log a credential-bearing URL."""
|
||||||
|
from langchain_mcp_adapters import client as adapter_client
|
||||||
|
|
||||||
|
secret = "s3cr3t-pass-123"
|
||||||
|
config = _config({"A": {"enabled": True, "type": "http", "url": "https://alice@remote.test/mcp"}})
|
||||||
|
_patch_common(monkeypatch)
|
||||||
|
|
||||||
|
class FakeClient:
|
||||||
|
def __init__(self, connections, **kwargs) -> None:
|
||||||
|
self.callbacks = kwargs.get("callbacks") or []
|
||||||
|
self.tool_interceptors = kwargs.get("tool_interceptors") or []
|
||||||
|
self.tool_name_prefix = kwargs.get("tool_name_prefix", True)
|
||||||
|
|
||||||
|
async def get_tools(self, *, server_name=None):
|
||||||
|
request = httpx.Request("GET", f"https://alice:{secret}@remote.test/mcp")
|
||||||
|
raise httpx.HTTPStatusError(
|
||||||
|
f"Client error for url '{request.url}'",
|
||||||
|
request=request,
|
||||||
|
response=httpx.Response(401, request=request),
|
||||||
|
)
|
||||||
|
|
||||||
|
monkeypatch.setattr(adapter_client, "MultiServerMCPClient", FakeClient)
|
||||||
|
|
||||||
|
with caplog.at_level("WARNING", logger="deerflow.mcp.tools"):
|
||||||
|
result = await get_mcp_tools_by_server(config, server_names={"A"})
|
||||||
|
|
||||||
|
assert result == {}
|
||||||
|
assert secret not in caplog.text # the resolved credential never reaches the log
|
||||||
|
assert "server 'A'" in caplog.text # the offending server name is retained
|
||||||
|
assert "HTTPStatusError" in caplog.text # exception type retained
|
||||||
|
|||||||
@ -130,7 +130,11 @@ def test_mcp_tool_loading_skips_failed_server():
|
|||||||
assert [tool.name for tool in tools] == ["good-server_search"]
|
assert [tool.name for tool in tools] == ["good-server_search"]
|
||||||
assert tools[0].func is not None
|
assert tools[0].func is not None
|
||||||
mock_warning.assert_called_once()
|
mock_warning.assert_called_once()
|
||||||
assert "bad-server" in mock_warning.call_args[0][0]
|
args, kwargs = mock_warning.call_args
|
||||||
|
assert args[1] == "bad-server"
|
||||||
|
assert args[2] == "RuntimeError"
|
||||||
|
assert "server 'bad-server'" in args[0] % args[1:]
|
||||||
|
assert kwargs.get("exc_info") is None
|
||||||
|
|
||||||
|
|
||||||
def test_mcp_tool_sync_wrapper_in_running_loop():
|
def test_mcp_tool_sync_wrapper_in_running_loop():
|
||||||
|
|||||||
Loading…
x
Reference in New Issue
Block a user