deer-flow/backend/tests/test_mcp_client_config.py
Terminator666666 c17aa8b98f
fix(mcp): reject credentials that cannot travel as HTTP header values (#5066)
* fix(mcp): reject credentials that cannot travel as HTTP header values

A request-scoped secret or user_auth credential with a trailing newline
(the usual result of reading a token from a file, or a CRLF env-file),
CR/LF, surrounding whitespace, or characters outside Latin-1 sailed
through the credential interceptors into the HTTP client, where httpx/h11
reject it with an exception that echoes the full value:

    LocalProtocolError: Illegal header value b'Bearer sk-...\n'

ToolErrorHandlingMiddleware copies that message into a model-visible
ToolMessage, so the secret landed in the prompt, the checkpoint, and
traces - everywhere headers_from_context promises it never goes.

Add illegal_header_value_reason to mcp/headers.py, mirroring the
transport's own rules (Latin-1 encodable; h11's field_vchar is [^\x00\s]
with SP/HTAB legal only between visible characters), and fail closed in
both interceptors before the value can reach the client. The denial names
only the secret key (plus the reason) and never repeats the value.

Illegal values are denied regardless of on_missing: the key is present,
so a passthrough fallback would silently run the call under the shared
discovery credential - the exact authority confusion the deny default
exists to prevent.

Values the transport accepts are not rejected: embedded SP/HTAB
('Bearer <token>'), Latin-1 high bytes, and DEL all still pass, pinned
by tests against h11's observed behaviour.

* fix(mcp): tighten header value validation to httpx's ASCII boundary

The validator mirrored h11's Latin-1 boundary, but the transport rejects
more than h11 does: build_server_params hands dict[str, str] headers
through the MCP SDK's create_mcp_http_client into httpx.AsyncClient, and
httpx (pinned 0.28.1) encodes str header values as ASCII - so a Latin-1
high byte like 'Bearer caf\xe9' passed validation here only to raise
UnicodeEncodeError inside httpx before h11 ever ran, with the exception
message repeating the offending value.

Validate str values against ASCII instead, flip the tests that pinned
Latin-1 high bytes as transportable, and pin the boundary against the
real client: create_mcp_http_client must reject what the validator
flags and construct cleanly for what it accepts (embedded SP/HTAB and
DEL still pass).

Addresses review feedback on the ASCII vs Latin-1 boundary.

* fix(mcp): validate OAuth and static header values at the same boundary

The validator added for headers_from_context and user_auth left two paths
uncovered. A token endpoint returning an access_token or token_type with a
newline reached httpx/h11, which raise with the full token in the message, and
ToolErrorHandlingMiddleware copies that message into a model-visible
ToolMessage -- the leak this PR set out to close. The operator's static headers
had the same hole.

OAuthTokenManager.get_authorization_header now renders the Authorization value
through one checked helper, so the tool interceptor, the initial discovery
headers and the durable task path are all covered by a single guard. The
rendered value is what gets checked rather than the two fields separately,
because that is what the transport sees: an access_token with leading
whitespace is legal once it follows "Bearer ".

build_server_params applies the same check to statically configured headers.
build_servers_config already isolates a per-server failure, so a bad value
drops that one server and logs the reason instead of the value.

* docs(mcp): correct which transport echoes the full header value

The rationale claimed httpx and h11 both render the full value into their
exception message. Only h11 does, on the line break and surrounding whitespace
cases. httpx's ASCII failure is a UnicodeEncodeError naming the offending
character and its position, not the credential, so at most one character
escapes there; refusing the value up front buys an actionable error rather than
an encode failure raised from inside the client.

Corrected in headers.py and in every copy of the claim: context_headers.py,
user_scoped_auth.py, oauth.py, client.py, mcp/AGENTS.md, docs/MCP_SERVER.md,
the frontend mcp.mdx, and the test comments carrying the same wording. No
behavior change.

---------

Co-authored-by: Terminator666666 <Terminator666666@users.noreply.github.com>
2026-08-31 15:07:30 +08:00

233 lines
7.7 KiB
Python

"""Core behavior tests for MCP client server config building."""
import logging
import pytest
from deerflow.config.extensions_config import ExtensionsConfig, McpServerConfig
from deerflow.mcp.client import build_server_params, build_servers_config
def test_build_server_params_stdio_success():
config = McpServerConfig(
type="stdio",
command="npx",
args=["-y", "my-mcp-server"],
env={"API_KEY": "secret"},
)
params = build_server_params("my-server", config)
assert params == {
"transport": "stdio",
"command": "npx",
"args": ["-y", "my-mcp-server"],
"env": {"API_KEY": "secret"},
}
def test_extensions_config_resolves_env_variables_inside_nested_collections(monkeypatch):
monkeypatch.setenv("MCP_TOKEN", "secret")
monkeypatch.delenv("MISSING_TOKEN", raising=False)
raw_config = {
"args": ["--token", "$MCP_TOKEN", {"nested": ["$MCP_TOKEN", "$MISSING_TOKEN"]}],
"tuple_args": ("$MCP_TOKEN", "$MISSING_TOKEN"),
"env": {"API_KEY": "$MCP_TOKEN"},
"enabled": True,
"timeout": 30,
}
resolved = ExtensionsConfig.resolve_env_variables(raw_config)
assert resolved["args"] == ["--token", "secret", {"nested": ["secret", ""]}]
assert resolved["tuple_args"] == ("secret", "")
assert resolved["env"] == {"API_KEY": "secret"}
assert resolved["enabled"] is True
assert resolved["timeout"] == 30
def test_build_server_params_stdio_requires_command():
config = McpServerConfig(type="stdio", command=None)
with pytest.raises(ValueError, match="requires 'command' field"):
build_server_params("broken-stdio", config)
@pytest.mark.parametrize("transport", ["sse", "http"])
def test_build_server_params_http_like_success(transport: str):
config = McpServerConfig(
type=transport,
url="https://example.com/mcp",
headers={"Authorization": "Bearer token"},
)
params = build_server_params("remote-server", config)
assert params == {
"transport": transport,
"url": "https://example.com/mcp",
"headers": {"Authorization": "Bearer token"},
}
@pytest.mark.parametrize("transport", ["sse", "http"])
def test_build_server_params_http_like_requires_url(transport: str):
config = McpServerConfig(type=transport, url=None)
with pytest.raises(ValueError, match="requires 'url' field"):
build_server_params("broken-remote", config)
def test_build_server_params_rejects_unsupported_transport():
config = McpServerConfig(type="websocket")
with pytest.raises(ValueError, match="unsupported transport type"):
build_server_params("bad-transport", config)
@pytest.mark.parametrize(
("value", "reason"),
[
("Bearer static-secret-123\n", "line break"),
("Bearer static-secret-caf\u00e9", "outside ASCII"),
("Bearer static-secret-456 ", "whitespace"),
],
ids=["trailing-newline", "non-ascii", "trailing-space"],
)
def test_build_server_params_rejects_illegal_header_value(value: str, reason: str):
"""A statically configured value the transport would refuse is denied here.
h11 renders the full value into its exception message on a line break or
surrounding whitespace, which ToolErrorHandlingMiddleware turns into a
model-visible ToolMessage. These values are API keys often enough that the
denial names the header and the reason instead.
"""
config = McpServerConfig(type="http", url="https://example.com/mcp", headers={"Authorization": value})
with pytest.raises(ValueError) as excinfo:
build_server_params("remote-server", config)
message = str(excinfo.value)
assert reason in message
assert "static-secret" not in message
assert "Authorization" in message
def test_build_servers_config_drops_only_the_server_with_an_illegal_header(caplog):
config = ExtensionsConfig.model_validate(
{
"mcpServers": {
"broken": {
"enabled": True,
"type": "http",
"url": "https://example.com/mcp",
"headers": {"Authorization": "Bearer static-secret-123\n"},
},
"healthy": {
"enabled": True,
"type": "http",
"url": "https://example.com/mcp",
"headers": {"Authorization": "Bearer fine"},
},
}
}
)
with caplog.at_level(logging.ERROR, logger="deerflow.mcp.client"):
servers_config = build_servers_config(config)
# One bad server does not take the others down with it, and the log that
# explains the drop does not carry the value either.
assert set(servers_config) == {"healthy"}
assert "static-secret" not in caplog.text
@pytest.mark.parametrize("transport", ["sse", "http"])
def test_mcp_server_config_accepts_transport_alias(transport: str):
"""The MCP-spec ``transport`` field should be accepted as an alias for ``type``.
Regression test for https://github.com/bytedance/deer-flow/issues/3238 — a
remote MCP server configured with only ``transport: sse`` was previously
misidentified as ``stdio`` (the default for ``type``).
"""
config = McpServerConfig.model_validate(
{
"transport": transport,
"url": "https://example.com/mcp",
}
)
assert config.type == transport
params = build_server_params("aliased-server", config)
assert params["transport"] == transport
assert params["url"] == "https://example.com/mcp"
def test_mcp_server_config_type_takes_precedence_over_transport():
"""When both ``type`` and ``transport`` are provided, ``type`` wins."""
config = McpServerConfig.model_validate(
{
"type": "http",
"transport": "sse",
"url": "https://example.com/mcp",
}
)
assert config.type == "http"
def test_build_servers_config_returns_empty_when_no_enabled_servers():
extensions = ExtensionsConfig(
mcp_servers={
"disabled-a": McpServerConfig(enabled=False, type="stdio", command="echo"),
"disabled-b": McpServerConfig(enabled=False, type="http", url="https://example.com"),
},
skills={},
)
assert build_servers_config(extensions) == {}
def test_build_servers_config_skips_invalid_server_and_keeps_valid_ones():
extensions = ExtensionsConfig(
mcp_servers={
"valid-stdio": McpServerConfig(enabled=True, type="stdio", command="npx", args=["server"]),
"invalid-stdio": McpServerConfig(enabled=True, type="stdio", command=None),
"disabled-http": McpServerConfig(enabled=False, type="http", url="https://disabled.example.com"),
},
skills={},
)
result = build_servers_config(extensions)
assert "valid-stdio" in result
assert result["valid-stdio"]["transport"] == "stdio"
assert "invalid-stdio" not in result
assert "disabled-http" not in result
def test_build_server_params_excludes_tool_call_timeout():
"""tool_call_timeout must NOT appear in the connection dict.
langchain-mcp-adapters passes the connection dict to create_session(),
which forwards unknown keys to _create_stdio_session(), causing TypeError.
The timeout is read from McpServerConfig at the tool wrapper call-site
instead. Regression for PR #3843 P1 bug.
"""
config = McpServerConfig(
type="stdio",
command="npx",
args=["-y", "my-mcp-server"],
tool_call_timeout=30.0,
)
params = build_server_params("my-server", config)
assert "tool_call_timeout" not in params
assert params == {
"transport": "stdio",
"command": "npx",
"args": ["-y", "my-mcp-server"],
}