diff --git a/backend/docs/MCP_SERVER.md b/backend/docs/MCP_SERVER.md index 61f18e5ef..d4de6df15 100644 --- a/backend/docs/MCP_SERVER.md +++ b/backend/docs/MCP_SERVER.md @@ -409,6 +409,14 @@ The caller supplies the values on each run request: the discovery credential — which in a multi-tenant deployment would send one tenant's request under another tenant's authority. Set `"passthrough"` to opt out and forward the static headers instead. +- A value that cannot be sent as an HTTP header — a stray newline picked up + when reading a token from a file, leading/trailing whitespace, characters + outside ASCII — is always denied, regardless of `on_missing`. The error + names the offending key but never repeats the value; without this check a + newline or stray whitespace would reach h11, whose rejection echoes the full + credential into a model-visible tool error. The same check covers every other + way a value reaches these headers: `user_auth`, the OAuth token returned by + the token endpoint, and the static `headers` in the config file. - Precedence for a server declaring several sources: static `headers` < `oauth` < `user_auth` < `headers_from_context`. The value chosen for this one request is the most specific, so it wins. diff --git a/backend/packages/harness/deerflow/mcp/AGENTS.md b/backend/packages/harness/deerflow/mcp/AGENTS.md index f65b975e4..d0e316606 100644 --- a/backend/packages/harness/deerflow/mcp/AGENTS.md +++ b/backend/packages/harness/deerflow/mcp/AGENTS.md @@ -9,9 +9,9 @@ - **Cache invalidation**: Detects extensions-config changes by comparing the resolved config path and a `(mtime, size, sha256)` content signature against the values recorded at initialization, not a strict mtime `>` comparison. This catches same-second edits, mtime that stays put or moves backward (`git checkout`, `cp -p` / backup restore, `tar` / `rsync`, object-store / network mounts), and a switch to a different config file with an equal-or-older mtime. The signature helper (`config/file_signature.py::get_config_signature`) is shared with `config/app_config.py::get_app_config()` for the sibling runtime-editable config file, rather than each maintaining its own copy. `ExtensionsConfig.resolve_config_path()` raises `FileNotFoundError` for an explicit `config_path`/`DEER_FLOW_EXTENSIONS_CONFIG_PATH` that points at a missing file — an operator-asserted path going missing is a real misconfiguration, so this is intentionally loud for callers that load the config for actual use (e.g. `from_file()` via `get_mcp_tools()`); only the fallback search mode returns `None`. The MCP cache's own path resolution (`mcp/cache.py::_resolve_config_path`) is narrower: it catches that specific `FileNotFoundError` locally and treats it the same as "unconfigured", so this staleness check degrades to "not stale" instead of propagating an exception when a previously-valid explicit/env-var config disappears mid-run - **Transports**: stdio (command-based), SSE, HTTP - **Per-server tool-name prefixing**: `mcpServers..tool_name_prefix` defaults to `true`, preserving the collision-safe `_` prefix. Servers whose tools already carry a stable namespace may set it to `false`; discovery then calls `langchain_mcp_adapters.tools.load_mcp_tools` with that server's flag. Source routing and stdio session-pool wrapping are based on the producing server and transport, never on whether the visible tool name starts with the server prefix. -- **OAuth (HTTP/SSE)**: Supports token endpoint flows (`client_credentials`, `refresh_token`) with automatic token refresh + Authorization header injection -- **Per-user credentials (HTTP/SSE)**: `mcpServers..user_auth` maps DeerFlow user ids to credential header values (`$ENV_VAR` references supported). `mcp/user_scoped_auth.py::build_user_scoped_auth_interceptor` rewrites the configured header on every tool call from the authenticated runtime user (registered after OAuth in `mcp/interceptors.py`, so its per-user value wins the header for servers declaring both). Fail-closed: an unmapped user or an empty resolved credential raises a `ToolException` unless `on_missing: "passthrough"` is set. The server's static `headers` serve startup tool discovery only. Gateway GET masks `user_auth.users` values; PUT round-trips masked values by preserving stored credentials. -- **Per-request credentials (HTTP/SSE)**: `mcpServers..headers_from_context` maps HTTP header names to keys of the run request's `config.context.secrets` carrier, for credentials the caller chooses per request (multi-tenant gateways, per-run API keys) rather than per configured user. `mcp/context_headers.py::build_context_headers_interceptor` resolves the mapping on every tool call and rewrites those headers. Registered **after** `user_auth` in `mcp/interceptors.py`, so for a server declaring several sources the per-request value wins the final header: precedence is static `headers` < `oauth` < `user_auth` < `headers_from_context`. Fail-closed: a mapped key absent from the request secrets (or resolved empty) raises a `ToolException` naming only the missing key, unless `on_missing: "passthrough"` is set — a silent fallback would send one tenant's call under the discovery credential's authority. `sse`/`http` only; a stdio server warns and is skipped, as with `user_auth`. The block stores names, never a credential, so the Gateway returns it unmasked and a `PUT` replaces the declared mapping verbatim; only `extra="allow"` keys inside it get sensitive-key masking, and those are restored from the stored block on a round-trip like every other masked extra. Durable `task_toolsets` calls split: submit is awaited inside the Agent run and carries the mapped headers (`McpTaskToolCaller.call_tool(request_scoped_headers=True)`, set only by `OrdinaryMcpTaskDriver.submit`), while status and cancel run after that run ended and keep the server-level credentials — so `on_missing: "deny"` covers submit but not those polls, which is what the startup warning is about. +- **OAuth (HTTP/SSE)**: Supports token endpoint flows (`client_credentials`, `refresh_token`) with automatic token refresh + Authorization header injection. The rendered ` ` passes `mcp/headers.py::illegal_header_value_reason` inside `OAuthTokenManager.get_authorization_header` — the one boundary the tool interceptor, the initial discovery headers and the durable task path all read their value from — so a token endpoint returning something the transport would refuse fails closed instead of letting h11 echo the token into a model-visible tool error. The rendered value is what gets checked, not the two fields separately, because that is what the transport sees. The operator's static `headers` get the same check in `mcp/client.py::build_server_params`, where `build_servers_config` already drops just that server and logs the reason. +- **Per-user credentials (HTTP/SSE)**: `mcpServers..user_auth` maps DeerFlow user ids to credential header values (`$ENV_VAR` references supported). `mcp/user_scoped_auth.py::build_user_scoped_auth_interceptor` rewrites the configured header on every tool call from the authenticated runtime user (registered after OAuth in `mcp/interceptors.py`, so its per-user value wins the header for servers declaring both). Fail-closed: an unmapped user or an empty resolved credential raises a `ToolException` unless `on_missing: "passthrough"` is set; a resolved credential the transport would refuse as a header value (line break, surrounding whitespace, non-ASCII — `mcp/headers.py::illegal_header_value_reason`) is always denied without echoing the value, since h11 renders the full value into its exception message on the line break and whitespace cases and tool errors are model-visible (httpx catches the non-ASCII case earlier, naming only the offending character). The server's static `headers` serve startup tool discovery only. Gateway GET masks `user_auth.users` values; PUT round-trips masked values by preserving stored credentials. +- **Per-request credentials (HTTP/SSE)**: `mcpServers..headers_from_context` maps HTTP header names to keys of the run request's `config.context.secrets` carrier, for credentials the caller chooses per request (multi-tenant gateways, per-run API keys) rather than per configured user. `mcp/context_headers.py::build_context_headers_interceptor` resolves the mapping on every tool call and rewrites those headers. Registered **after** `user_auth` in `mcp/interceptors.py`, so for a server declaring several sources the per-request value wins the final header: precedence is static `headers` < `oauth` < `user_auth` < `headers_from_context`. Fail-closed: a mapped key absent from the request secrets (or resolved empty) raises a `ToolException` naming only the missing key, unless `on_missing: "passthrough"` is set — a silent fallback would send one tenant's call under the discovery credential's authority. A resolved value the transport would refuse as a header value (line break, surrounding whitespace, non-ASCII) is always denied regardless of `on_missing`, without echoing the value: h11 renders the full value into its exception message on a line break or surrounding whitespace, and `ToolErrorHandlingMiddleware` copies tool errors into model-visible messages, so an unchecked bad credential would land the secret in the prompt, the checkpoint, and traces. `sse`/`http` only; a stdio server warns and is skipped, as with `user_auth`. The block stores names, never a credential, so the Gateway returns it unmasked and a `PUT` replaces the declared mapping verbatim; only `extra="allow"` keys inside it get sensitive-key masking, and those are restored from the stored block on a round-trip like every other masked extra. Durable `task_toolsets` calls split: submit is awaited inside the Agent run and carries the mapped headers (`McpTaskToolCaller.call_tool(request_scoped_headers=True)`, set only by `OrdinaryMcpTaskDriver.submit`), while status and cancel run after that run ended and keep the server-level credentials — so `on_missing: "deny"` covers submit but not those polls, which is what the startup warning is about. - **Header names are case-insensitive** (`mcp/headers.py`): every credential interceptor writes through `apply_header_overrides`, which drops a key differing only in case and emits the spelling the connection already uses. Without it a static `authorization` and an injected `Authorization` both reach httpx — the adapter merges connection and override headers with a plain `{**static, **override}` splat — and a server reading the field with a single-value accessor gets the static entry, silently inverting the precedence above. `headers_from_context.headers` also rejects two spellings of one header at config load. - **Reading the run context from an interceptor**: use `request.runtime` (LangGraph's tool node injects a `ToolRuntime` into any tool parameter named `runtime`, which covers both the pooled stdio wrapper and `langchain-mcp-adapters`' own HTTP/SSE tool), falling back to ambient `langgraph.runtime.get_runtime()`. Do **not** use `langgraph.config.get_config()["context"]`: the run context rides the runtime, not the `RunnableConfig` propagated to child runnables, so that key is `None` inside a tool call. `tests/test_mcp_context_headers.py::test_adapter_tool_receives_the_runtime_langgraph_injects` pins the injection rule against an upstream rename by disabling the ambient fallback and driving a real adapter tool through a real graph. - **Routing hints**: `extensions_config.json -> mcpServers..routing` and diff --git a/backend/packages/harness/deerflow/mcp/client.py b/backend/packages/harness/deerflow/mcp/client.py index 62bda9d94..924395bc5 100644 --- a/backend/packages/harness/deerflow/mcp/client.py +++ b/backend/packages/harness/deerflow/mcp/client.py @@ -4,6 +4,7 @@ import logging from typing import Any from deerflow.config.extensions_config import ExtensionsConfig, McpServerConfig +from deerflow.mcp.headers import illegal_header_value_reason logger = logging.getLogger(__name__) @@ -35,6 +36,17 @@ def build_server_params(server_name: str, config: McpServerConfig) -> dict[str, params["url"] = config.url # Add headers if present if config.headers: + # A statically configured value the transport would refuse gets the + # same treatment as a request-scoped one: h11 renders the full + # value into its exception on a line break or surrounding + # whitespace, which reaches the model through + # ToolErrorHandlingMiddleware. These values are API keys often + # enough to be worth refusing here, where build_servers_config + # already drops just this server and logs the reason. + for header_name, header_value in config.headers.items(): + reason = illegal_header_value_reason(header_value) + if reason is not None: + raise ValueError(f"MCP server '{server_name}' has a header '{header_name}' that cannot be sent as an HTTP header value: it {reason}") params["headers"] = config.headers else: raise ValueError(f"MCP server '{server_name}' has unsupported transport type: {transport_type}") diff --git a/backend/packages/harness/deerflow/mcp/context_headers.py b/backend/packages/harness/deerflow/mcp/context_headers.py index 7777e5edd..b985c8dfc 100644 --- a/backend/packages/harness/deerflow/mcp/context_headers.py +++ b/backend/packages/harness/deerflow/mcp/context_headers.py @@ -27,6 +27,16 @@ Fail-closed by default: a mapped key that is absent from the request secrets falling back to the server's static discovery credential, which in a multi-tenant deployment would send one tenant's request under another tenant's authority. ``on_missing: "passthrough"`` is the explicit opt-out. + +A resolved value the transport would refuse (line break, surrounding +whitespace, non-ASCII — see ``mcp/headers.py``) is always denied, regardless +of ``on_missing``. h11 renders the full value into its exception message when +it refuses a line break or surrounding whitespace, and that message would +otherwise travel into a model-visible tool error and the trace; the non-ASCII +case fails inside httpx instead, which names only the offending character, so +denying it here buys an actionable error rather than secrecy. Either way a +passthrough fallback would run the call under the shared discovery credential +even though the caller did supply a key. """ from __future__ import annotations @@ -37,7 +47,11 @@ from typing import Any from langchain_core.tools import ToolException from deerflow.config.extensions_config import ExtensionsConfig, McpContextHeadersConfig -from deerflow.mcp.headers import apply_header_overrides, header_spellings +from deerflow.mcp.headers import ( + apply_header_overrides, + header_spellings, + illegal_header_value_reason, +) from deerflow.runtime.secret_context import extract_request_secrets logger = logging.getLogger(__name__) @@ -128,14 +142,44 @@ def build_context_headers_interceptor(extensions_config: ExtensionsConfig) -> An secrets = _request_secrets(request) resolved: dict[str, str] = {} missing: list[str] = [] + illegal: dict[str, str] = {} for header_name, secret_key in context_headers.headers.items(): # Empty string covers a caller-side `$ENV_VAR` that was unset: an # empty credential must fail closed rather than send an empty header. value = secrets.get(secret_key, "") - if value: - resolved[header_name] = value - else: + if not value: missing.append(secret_key) + continue + # A value the transport would refuse (trailing newline from reading + # a token file, CR/LF, non-ASCII) must be rejected *here*. h11 + # renders the full value into its exception message on the line + # break and whitespace cases, and ToolErrorHandlingMiddleware + # copies that message into a model-visible ToolMessage — putting + # the secret in the prompt, the checkpoint, and traces, everywhere + # this module promises it never goes. Always denied, regardless of + # on_missing: the key is present, so falling back to the discovery + # credential would silently run this tenant's call under the shared + # authority. + reason = illegal_header_value_reason(value) + if reason is not None: + illegal[secret_key] = reason + continue + resolved[header_name] = value + + if illegal: + illegal_keys = ", ".join(sorted(illegal)) + logger.warning( + "Denied MCP tool call to server '%s': request-scoped secret(s) %s cannot be sent as an HTTP header value", + request.server_name, + illegal_keys, + ) + details = "; ".join(f"'{key}' {reason}" for key, reason in sorted(illegal.items())) + # Like the missing-key denial below, only the configured key names + # (plus the reason) are surfaced — never the value. + raise ToolException( + f"MCP server '{request.server_name}' cannot send request-scoped credential(s) as HTTP header values: {details}. " + "Fix the value passed in config.context.secrets; a stray newline picked up when reading a token from a file is the usual cause." + ) if missing and context_headers.on_missing == "deny": missing_keys = ", ".join(sorted(missing)) diff --git a/backend/packages/harness/deerflow/mcp/headers.py b/backend/packages/harness/deerflow/mcp/headers.py index 67b6089fc..eeb37b087 100644 --- a/backend/packages/harness/deerflow/mcp/headers.py +++ b/backend/packages/harness/deerflow/mcp/headers.py @@ -14,12 +14,54 @@ The credential interceptors therefore write header names through :func:`apply_header_overrides`, which drops any key differing only in case and emits the spelling the connection already uses, so the adapter's merge replaces the static entry instead of duplicating it. + +Every path that writes a value into an MCP request header checks it with +:func:`illegal_header_value_reason` first — both credential interceptors, the +OAuth token manager, and ``build_server_params`` for the operator's static +headers. + +The two halves of that boundary fail differently, and only one of them leaks. +h11 rejects line breaks and surrounding whitespace with the *full value* in its +message, ``ToolErrorHandlingMiddleware`` copies that into a model-visible +ToolMessage, and the credential lands in the prompt, the checkpoint, and +traces. That is the leak this check exists to stop. httpx encodes ``str`` +values as ASCII and raises ``UnicodeEncodeError``, whose message names only the +offending character and its position, so at most one character escapes; +refusing that value up front buys an actionable error rather than an encode +failure raised from inside the client. """ from __future__ import annotations +import re from collections.abc import Iterable, Mapping +# What h11 refuses inside a field value (its field_vchar is ``[^\x00\s]``): +# NUL and the vertical-whitespace characters. SP/HTAB are legal separators +# *between* visible characters but not at either end. +_FORBIDDEN_HEADER_VALUE_CHARS = re.compile(r"[\x00\n\x0b\x0c\r]") + + +def illegal_header_value_reason(value: str) -> str | None: + """Explain why *value* cannot be sent as an HTTP header value, or ``None``. + + Mirrors what the transport enforces — the MCP clients hand ``dict[str, + str]`` headers to ``httpx``, which encodes ``str`` values as ASCII (raising + ``UnicodeEncodeError`` before h11 ever sees the value), and h11 rejects + NUL/vertical whitespace and leading or trailing SP/HTAB — without + repeating the value, so callers can fail closed with a message that names + the credential instead of leaking it. + """ + try: + value.encode("ascii") + except UnicodeEncodeError: + return "contains characters outside ASCII" + if _FORBIDDEN_HEADER_VALUE_CHARS.search(value): + return "contains a line break or another forbidden control character" + if value != value.strip(" \t"): + return "has leading or trailing whitespace" + return None + def header_spellings(names: Iterable[str] | None) -> dict[str, str]: """Index header names by their lowercased form. diff --git a/backend/packages/harness/deerflow/mcp/oauth.py b/backend/packages/harness/deerflow/mcp/oauth.py index 4c7672148..013c98d58 100644 --- a/backend/packages/harness/deerflow/mcp/oauth.py +++ b/backend/packages/harness/deerflow/mcp/oauth.py @@ -10,7 +10,7 @@ from datetime import UTC, datetime, timedelta from typing import Any from deerflow.config.extensions_config import ExtensionsConfig, McpOAuthConfig -from deerflow.mcp.headers import apply_header_overrides, header_spellings +from deerflow.mcp.headers import apply_header_overrides, header_spellings, illegal_header_value_reason logger = logging.getLogger(__name__) @@ -63,7 +63,7 @@ class OAuthTokenManager: token = self._tokens.get(server_name) if token and not self._is_expiring(token, oauth): - return f"{token.token_type} {token.access_token}" + return self._authorization_value(token, server_name) lock = self._locks[server_name] # Acquire the OS-level lock off-thread so a blocking wait never blocks this @@ -102,15 +102,45 @@ class OAuthTokenManager: try: token = self._tokens.get(server_name) if token and not self._is_expiring(token, oauth): - return f"{token.token_type} {token.access_token}" + return self._authorization_value(token, server_name) fresh = await self._fetch_token(oauth) self._tokens[server_name] = fresh logger.info(f"Refreshed OAuth access token for MCP server: {server_name}") - return f"{fresh.token_type} {fresh.access_token}" + return self._authorization_value(fresh, server_name) finally: lock.release() + @staticmethod + def _authorization_value(token: _OAuthToken, server_name: str) -> str: + """Render the Authorization value, refusing one the transport would echo. + + The token endpoint's response is not this process's to control: an + ``access_token`` or ``token_type`` carrying a newline reaches h11, which + raises with the full value in the message, and + ``ToolErrorHandlingMiddleware`` copies that message into a + model-visible ToolMessage. Failing closed here keeps the token out of + the prompt, the checkpoint, and traces, at the one boundary every caller + goes through -- the tool interceptor, the initial discovery headers, and + the durable task path all read their value from here. A token outside + ASCII fails earlier, inside httpx, with only the offending character in + the message; that one is refused for a deliverable error rather than for + secrecy. + + The rendered value is what gets checked, not the two fields separately, + because the rendered value is what the transport sees. An + ``access_token`` of ``" abc"`` is legal once it sits after ``Bearer `` + even though the field on its own carries leading whitespace, and + rejecting it would deny a token the server would have accepted. + """ + value = f"{token.token_type} {token.access_token}" + reason = illegal_header_value_reason(value) + if reason is not None: + # Names the server and the reason, never the token: this message + # travels to the model on the interceptor path. + raise ValueError(f"OAuth token for MCP server '{server_name}' cannot be sent as an HTTP header value: the Authorization value {reason}. Check what the token endpoint returned for this server.") + return value + @staticmethod def _is_expiring(token: _OAuthToken, oauth: McpOAuthConfig) -> bool: now = datetime.now(UTC) diff --git a/backend/packages/harness/deerflow/mcp/user_scoped_auth.py b/backend/packages/harness/deerflow/mcp/user_scoped_auth.py index f2f08e110..dee31853c 100644 --- a/backend/packages/harness/deerflow/mcp/user_scoped_auth.py +++ b/backend/packages/harness/deerflow/mcp/user_scoped_auth.py @@ -27,7 +27,11 @@ from typing import Any from langchain_core.tools import ToolException from deerflow.config.extensions_config import ExtensionsConfig, McpUserScopedAuthConfig -from deerflow.mcp.headers import apply_header_overrides, header_spellings +from deerflow.mcp.headers import ( + apply_header_overrides, + header_spellings, + illegal_header_value_reason, +) from deerflow.runtime.user_context import resolve_runtime_user_id logger = logging.getLogger(__name__) @@ -116,6 +120,26 @@ def build_user_scoped_auth_interceptor(extensions_config: ExtensionsConfig) -> A f"No credential is configured for your account (user id '{user_id}') on MCP server '{request.server_name}'. Ask the operator to add this exact id to that server's user_auth.users map (or set its environment variable)." ) + # A credential the transport would refuse (trailing newline from a + # token file or a CRLF env-file, non-ASCII) must be rejected here. On + # the line break and whitespace cases h11 renders the full value into + # its exception message, which ToolErrorHandlingMiddleware copies into + # a model-visible ToolMessage. Always denied, regardless of on_missing + # — the user *is* mapped, so falling back to the discovery credential + # would silently run the call under the shared authority. + reason = illegal_header_value_reason(credential) + if reason is not None: + logger.warning( + "Denied MCP tool call to server '%s': the user_auth credential for user '%s' cannot be sent as an HTTP header value (%s)", + request.server_name, + user_id, + reason, + ) + raise ToolException( + f"The credential configured for your account (user id '{user_id}') on MCP server '{request.server_name}' {reason}, so it cannot be sent as an HTTP header. " + "Ask the operator to fix that server's user_auth.users entry; a stray newline in the value or its environment variable is the usual cause." + ) + updated_headers = apply_header_overrides( request.headers, {user_auth.header: credential}, diff --git a/backend/tests/test_mcp_client_config.py b/backend/tests/test_mcp_client_config.py index 0396b2a5b..5d3d28650 100644 --- a/backend/tests/test_mcp_client_config.py +++ b/backend/tests/test_mcp_client_config.py @@ -1,5 +1,7 @@ """Core behavior tests for MCP client server config building.""" +import logging + import pytest from deerflow.config.extensions_config import ExtensionsConfig, McpServerConfig @@ -83,6 +85,63 @@ def test_build_server_params_rejects_unsupported_transport(): 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``. diff --git a/backend/tests/test_mcp_context_headers.py b/backend/tests/test_mcp_context_headers.py index ac447be56..04a4e6fb1 100644 --- a/backend/tests/test_mcp_context_headers.py +++ b/backend/tests/test_mcp_context_headers.py @@ -343,6 +343,89 @@ def test_passthrough_still_injects_the_secrets_that_are_present(): assert result.headers == {"X-Tenant-Id": "acme"} +# --------------------------------------------------------------------------- +# Illegal header values +# +# A secret that cannot travel as an HTTP header value (trailing newline from +# reading a token file, CR/LF, characters outside ASCII) must be rejected +# here, before it reaches the HTTP client. On the line break and whitespace +# cases h11 renders the full value into its LocalProtocolError message, +# ToolErrorHandlingMiddleware copies that message into a model-visible +# ToolMessage, and the secret lands in the prompt, the checkpoint, and traces — +# everywhere this module promises it never goes. Non-ASCII fails earlier, +# inside httpx, with only the offending character in the message. +# --------------------------------------------------------------------------- + + +def test_secret_with_trailing_newline_is_denied_without_calling_handler(): + interceptor = build_context_headers_interceptor(_config(headers={"X-Tenant-Token": "tenant_token"})) + handler = AsyncMock() + with pytest.raises(ToolException, match="tenant_token"): + asyncio.run(interceptor(_request(runtime=_runtime_with_secrets(tenant_token=TENANT_TOKEN + "\n")), handler)) + handler.assert_not_awaited() + + +def test_illegal_value_deny_message_does_not_contain_the_value(): + interceptor = build_context_headers_interceptor(_config(headers={"X-Tenant-Token": "tenant_token"})) + with pytest.raises(ToolException) as excinfo: + asyncio.run(interceptor(_request(runtime=_runtime_with_secrets(tenant_token="sk-secret-value\n")), AsyncMock())) + assert "sk-secret-value" not in str(excinfo.value) + + +def test_illegal_value_warning_log_does_not_contain_the_value(caplog): + interceptor = build_context_headers_interceptor(_config(headers={"X-Tenant-Token": "tenant_token"})) + with caplog.at_level(logging.WARNING, logger="deerflow.mcp.context_headers"), pytest.raises(ToolException): + asyncio.run(interceptor(_request(runtime=_runtime_with_secrets(tenant_token="sk-secret-value\n")), AsyncMock())) + assert "sk-secret-value" not in caplog.text + assert "tenant_token" in caplog.text + + +def test_embedded_crlf_is_denied(): + interceptor = build_context_headers_interceptor(_config(headers={"X-Tenant-Token": "tenant_token"})) + with pytest.raises(ToolException, match="tenant_token"): + asyncio.run(interceptor(_request(runtime=_runtime_with_secrets(tenant_token="a\r\nX-Injected: b")), AsyncMock())) + + +def test_non_ascii_value_is_denied(): + """httpx encodes str header values as ASCII and raises UnicodeEncodeError otherwise.""" + interceptor = build_context_headers_interceptor(_config(headers={"X-Tenant-Token": "tenant_token"})) + for value in ("пароль", "Bearer caf\xe9"): + with pytest.raises(ToolException, match="tenant_token"): + asyncio.run(interceptor(_request(runtime=_runtime_with_secrets(tenant_token=value)), AsyncMock())) + + +def test_leading_or_trailing_whitespace_is_denied(): + """h11 rejects field values with leading/trailing SP or HTAB.""" + interceptor = build_context_headers_interceptor(_config(headers={"X-Tenant-Token": "tenant_token"})) + for value in ("Bearer x ", " Bearer x", "Bearer x\t"): + with pytest.raises(ToolException, match="tenant_token"): + asyncio.run(interceptor(_request(runtime=_runtime_with_secrets(tenant_token=value)), AsyncMock())) + + +def test_illegal_value_is_denied_even_with_on_missing_passthrough(): + """passthrough covers an *absent* key; a present-but-broken value must not + silently fall back to the shared discovery credential.""" + interceptor = build_context_headers_interceptor(_config(headers={"X-Tenant-Token": "tenant_token"}, on_missing="passthrough")) + with pytest.raises(ToolException, match="tenant_token"): + asyncio.run(interceptor(_request(runtime=_runtime_with_secrets(tenant_token=TENANT_TOKEN + "\n")), AsyncMock())) + + +def test_values_with_embedded_spaces_and_tabs_are_not_rejected(): + """h11 allows SP/HTAB between visible characters — 'Bearer ' must pass.""" + interceptor = build_context_headers_interceptor(_config(headers={"X-Tenant-Token": "tenant_token"})) + result = asyncio.run(interceptor(_request(runtime=_runtime_with_secrets(tenant_token="Bearer abc\tdef ghi")), _echo_handler)) + assert result.headers["X-Tenant-Token"] == "Bearer abc\tdef ghi" + + +def test_one_illegal_mapping_denies_the_whole_call(): + """One broken credential denies the call; it must not partially inject.""" + interceptor = build_context_headers_interceptor(_config(headers={"X-Tenant-Id": "tenant_id", "X-Org": "org"})) + handler = AsyncMock() + with pytest.raises(ToolException, match="org"): + asyncio.run(interceptor(_request(runtime=_runtime_with_secrets(tenant_id="acme", org="bad\n")), handler)) + handler.assert_not_awaited() + + # --------------------------------------------------------------------------- # Config model # --------------------------------------------------------------------------- diff --git a/backend/tests/test_mcp_header_names.py b/backend/tests/test_mcp_header_names.py index ca7a5a788..dbd925a55 100644 --- a/backend/tests/test_mcp_header_names.py +++ b/backend/tests/test_mcp_header_names.py @@ -15,7 +15,11 @@ from langchain_mcp_adapters.interceptors import MCPToolCallRequest from pydantic import ValidationError from deerflow.config.extensions_config import ExtensionsConfig, McpServerConfig, McpUserScopedAuthConfig -from deerflow.mcp.headers import apply_header_overrides, header_spellings +from deerflow.mcp.headers import ( + apply_header_overrides, + header_spellings, + illegal_header_value_reason, +) from deerflow.mcp.oauth import build_oauth_tool_interceptor from deerflow.mcp.user_scoped_auth import build_user_scoped_auth_interceptor @@ -64,6 +68,81 @@ def test_override_does_not_mutate_the_base(): assert base == {"Authorization": DISCOVERY} +# --------------------------------------------------------------------------- +# illegal_header_value_reason +# +# The boundary mirrors what the transport enforces: the MCP clients hand +# ``dict[str, str]`` headers to httpx, which encodes ``str`` values as ASCII +# (raising UnicodeEncodeError before h11 ever sees the value); h11's +# field_vchar is ``[^\x00\s]`` with SP/HTAB allowed only between visible +# characters. Values the transport accepts must not be rejected here. +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize( + "value", + [ + "Bearer sk-token\n", + "Bearer sk-token\r", + "a\r\nX-Injected: b", + "a\x00b", + "a\x0bb", + "a\x0cb", + " leading-space", + "trailing-space ", + "trailing-tab\t", + "caf\xe9", # Latin-1 high byte: h11 would send it, but httpx encodes str values as ASCII first + "\u043f\u0430\u0440\u043e\u043b\u044c", + ], +) +def test_transport_rejected_values_are_flagged(value): + assert illegal_header_value_reason(value) is not None + + +@pytest.mark.parametrize( + "value", + [ + "Bearer sk-token", + "Bearer abc\tdef", + "two words", + "a\x7fb", # DEL: ASCII-encodable, and h11's field_vchar accepts it + ], +) +def test_transport_accepted_values_are_not_flagged(value): + assert illegal_header_value_reason(value) is None + + +def test_reason_never_repeats_the_value(): + reason = illegal_header_value_reason("sk-secret-value\n") + assert reason is not None + assert "sk-secret-value" not in reason + + +def test_validator_mirrors_the_mcp_http_client_encoding_boundary(): + """Pin the boundary against the real client the headers are handed to. + + ``build_server_params`` passes ``dict[str, str]`` headers through the MCP + SDK's ``create_mcp_http_client`` into ``httpx.AsyncClient``, which encodes + ``str`` header values as ASCII at construction time. A value the validator + accepts must construct that client; the canonical counter-example — a + Latin-1 high byte h11 itself would happily send — must be flagged by the + validator, because httpx raises ``UnicodeEncodeError`` before h11 runs. + That exception names the offending character rather than the credential, so + what the denial buys here is an actionable error instead of an encode + failure raised from inside the client. + """ + from mcp.shared._httpx_utils import create_mcp_http_client + + assert illegal_header_value_reason("Bearer caf\xe9") is not None + with pytest.raises(UnicodeEncodeError): + create_mcp_http_client(headers={"Authorization": "Bearer caf\xe9"}) + + for value in ("Bearer sk-token", "Bearer abc\tdef ghi", "a\x7fb"): + assert illegal_header_value_reason(value) is None + client = create_mcp_http_client(headers={"X-Tenant-Token": value}) + assert client.headers["X-Tenant-Token"] == value + + # --------------------------------------------------------------------------- # Static header spelling validation # --------------------------------------------------------------------------- diff --git a/backend/tests/test_mcp_oauth.py b/backend/tests/test_mcp_oauth.py index c339b9a58..d08824331 100644 --- a/backend/tests/test_mcp_oauth.py +++ b/backend/tests/test_mcp_oauth.py @@ -3,6 +3,7 @@ from __future__ import annotations import asyncio +import logging import threading from typing import Any @@ -613,3 +614,127 @@ def test_get_authorization_header_cancelled_while_waiting_does_not_leak_lock(mon # Exactly one real token fetch: the cancelled waiter must never reach # _fetch_token, so the third call is the only one that performs it. assert len(post_calls) == 1 + + +# --- Illegal header values --------------------------------------------------- +# +# What the token endpoint returns is not this process's to control. An +# access_token or token_type carrying a newline reaches h11, which raises with +# the full value in the message, and ToolErrorHandlingMiddleware copies that +# message into a model-visible ToolMessage. Every one of these asserts the +# token never appears in what the caller sees. + + +def _oauth_server_config() -> ExtensionsConfig: + return ExtensionsConfig.model_validate( + { + "mcpServers": { + "secure-http": { + "enabled": True, + "type": "http", + "url": "https://api.example.com/mcp", + "oauth": { + "enabled": True, + "token_url": "https://auth.example.com/oauth/token", + "grant_type": "client_credentials", + "client_id": "client-id", + "client_secret": "client-secret", + }, + } + } + } + ) + + +def _token_endpoint_returns(monkeypatch, payload: dict[str, Any]) -> list[dict[str, Any]]: + post_calls: list[dict[str, Any]] = [] + + def _client_factory(*args, **kwargs): + return _MockAsyncClient(payload=payload, post_calls=post_calls, **kwargs) + + monkeypatch.setattr("httpx.AsyncClient", _client_factory) + return post_calls + + +@pytest.mark.parametrize( + ("payload", "secret"), + [ + ( + {"access_token": "oauth-secret-123\n", "token_type": "Bearer", "expires_in": 3600}, + "oauth-secret-123", + ), + ( + {"access_token": "oauth-secret-456", "token_type": "Bearer\r", "expires_in": 3600}, + "oauth-secret-456", + ), + ( + {"access_token": "oauth-secret-caf\u00e9", "token_type": "Bearer", "expires_in": 3600}, + "oauth-secret-caf\u00e9", + ), + ( + {"access_token": "oauth-secret-789 ", "token_type": "Bearer", "expires_in": 3600}, + "oauth-secret-789", + ), + ], + ids=["trailing-newline", "cr-in-token-type", "non-ascii", "trailing-space"], +) +def test_illegal_oauth_token_is_denied_without_leaking(monkeypatch, payload, secret): + _token_endpoint_returns(monkeypatch, payload) + manager = OAuthTokenManager.from_extensions_config(_oauth_server_config()) + + with pytest.raises(ValueError) as excinfo: + asyncio.run(manager.get_authorization_header("secure-http")) + + message = str(excinfo.value) + assert secret not in message + assert "secure-http" in message + + +def test_oauth_token_kept_legal_by_the_space_after_token_type_is_accepted(monkeypatch): + # The rendered value is the boundary, not the two fields on their own: this + # access_token carries leading whitespace, which the transport tolerates + # once it follows "Bearer ". Denying it would refuse a token the server + # would have accepted. + _token_endpoint_returns(monkeypatch, {"access_token": " leading-space-token", "token_type": "Bearer", "expires_in": 3600}) + manager = OAuthTokenManager.from_extensions_config(_oauth_server_config()) + + assert asyncio.run(manager.get_authorization_header("secure-http")) == "Bearer leading-space-token" + + +def test_oauth_interceptor_denies_illegal_token_without_calling_the_handler(monkeypatch): + _token_endpoint_returns(monkeypatch, {"access_token": "oauth-secret-abc\n", "token_type": "Bearer", "expires_in": 3600}) + config = _oauth_server_config() + interceptor = build_oauth_tool_interceptor(config) + assert interceptor is not None + + class _Request: + server_name = "secure-http" + headers: dict[str, str] = {} + + def override(self, **kwargs): # pragma: no cover - denied before reached + raise AssertionError("the request must never be forwarded with an illegal token") + + handler_calls: list[Any] = [] + + async def _handler(request): # pragma: no cover - denied before reached + handler_calls.append(request) + return "ok" + + with pytest.raises(ValueError) as excinfo: + asyncio.run(interceptor(_Request(), _handler)) + + assert "oauth-secret-abc" not in str(excinfo.value) + assert handler_calls == [] + + +def test_initial_oauth_headers_skips_server_with_illegal_token(monkeypatch, caplog): + _token_endpoint_returns(monkeypatch, {"access_token": "oauth-secret-xyz\n", "token_type": "Bearer", "expires_in": 3600}) + config = _oauth_server_config() + + with caplog.at_level(logging.WARNING, logger="deerflow.mcp.oauth"): + headers = asyncio.run(get_initial_oauth_headers(config)) + + # No header at all rather than a broken one: the connection then fails + # authentication at the server, which says nothing about the token. + assert headers == {} + assert "oauth-secret-xyz" not in caplog.text diff --git a/backend/tests/test_mcp_user_scoped_auth.py b/backend/tests/test_mcp_user_scoped_auth.py index e42477f73..a0a95ebb8 100644 --- a/backend/tests/test_mcp_user_scoped_auth.py +++ b/backend/tests/test_mcp_user_scoped_auth.py @@ -118,6 +118,34 @@ def test_default_user_fallback_is_denied_when_unmapped(): asyncio.run(interceptor(_request(runtime=None), AsyncMock())) +def test_credential_with_trailing_newline_is_denied_without_leaking_it(): + """A credential that cannot travel as a header value (docker env-file with + CRLF line endings, `$ENV_VAR` set from a token file) must be rejected here: + h11 renders the full value into its LocalProtocolError message, which + ToolErrorHandlingMiddleware then copies into a model-visible ToolMessage.""" + interceptor = build_user_scoped_auth_interceptor(_config(users={"u1": "Bearer sk-secret-value\n"})) + handler = AsyncMock() + with pytest.raises(ToolException) as excinfo: + asyncio.run(interceptor(_request(runtime=_runtime_for_user("u1")), handler)) + handler.assert_not_awaited() + assert "sk-secret-value" not in str(excinfo.value) + assert "u1" in str(excinfo.value) + + +def test_credential_with_carriage_return_is_denied_even_with_passthrough(): + """passthrough covers an *unmapped user*; a mapped-but-broken credential + must not silently fall back to the shared discovery credential.""" + interceptor = build_user_scoped_auth_interceptor(_config(users={"u1": "Bearer t1\r"}, on_missing="passthrough")) + with pytest.raises(ToolException): + asyncio.run(interceptor(_request(runtime=_runtime_for_user("u1")), AsyncMock())) + + +def test_credential_with_embedded_space_is_not_rejected(): + interceptor = build_user_scoped_auth_interceptor(_config(users={"u1": "Bearer t1"})) + result = asyncio.run(interceptor(_request(runtime=_runtime_for_user("u1")), _echo_handler)) + assert result.headers["Authorization"] == "Bearer t1" + + def test_env_var_reference_resolution(tmp_path, monkeypatch): monkeypatch.setenv("TEST_USER_CRED", "Bearer from-env") config_file = tmp_path / "extensions_config.json" diff --git a/frontend/src/content/en/harness/mcp.mdx b/frontend/src/content/en/harness/mcp.mdx index a262047b2..30dfc6a6a 100644 --- a/frontend/src/content/en/harness/mcp.mdx +++ b/frontend/src/content/en/harness/mcp.mdx @@ -140,7 +140,9 @@ discovery. - **Fail-closed by default**: a user with no mapped credential — or a mapped `$ENV_VAR` that is unset — gets a clear error instead of another user's credential. Set `"on_missing": "passthrough"` to instead forward such calls - with the server's static headers. + with the server's static headers. A mapped credential that cannot travel as + an HTTP header (for example one with a trailing newline) is always rejected + with an error that never repeats the value. - Combined with per-`(user, thread)` MCP session scoping, users cannot reach each other's authenticated sessions or credentials. - The Gateway API masks `user_auth.users` values in GET responses and preserves @@ -208,6 +210,12 @@ conversation: call fails with an error naming the missing key rather than falling back to the discovery credential — which would send one tenant's request under another tenant's authority. Set `"on_missing": "passthrough"` to opt out. +- A value that cannot travel as an HTTP header — a stray newline picked up when + reading a token from a file, leading/trailing whitespace, characters outside + ASCII — is always rejected, regardless of `on_missing`. The error names the + offending key but never repeats the value; without this check a newline or + stray whitespace would reach the HTTP client, whose rejection echoes the full + credential into a model-visible tool error. - For a server declaring several sources, precedence is static `headers` < `oauth` < `user_auth` < `headers_from_context`: the value chosen for this one request is the most specific, so it wins.