diff --git a/backend/packages/harness/deerflow/agents/middlewares/AGENTS.md b/backend/packages/harness/deerflow/agents/middlewares/AGENTS.md index 426f9f3be..650eac8ae 100644 --- a/backend/packages/harness/deerflow/agents/middlewares/AGENTS.md +++ b/backend/packages/harness/deerflow/agents/middlewares/AGENTS.md @@ -38,7 +38,7 @@ it to that middleware's declaration in the same change. 1. **InputSanitizationMiddleware** - First, so it is the outermost `wrap_model_call` wrapper; every inner middleware (including LLM retries) sees sanitized messages. `additional_kwargs.original_user_content` is server-owned provenance: Gateway strips caller-supplied values for non-internal run requests, trusted IM calls may carry the string they captured before adding transport/file context, and the middleware replaces any non-string value before wrapping. Uploads and sanitization retain first-writer-wins only for validated strings. 2. **ToolOutputBudgetMiddleware** - Caps tool output size (per app config) before it re-enters the model context. Oversized results are externalized to `tool_output.storage_subdir` (default `.tool-results`, shared constant `TOOL_RESULTS_DIRNAME`) under the thread outputs dir with a typed synopsis + `read_file` reference left in context; those files are process feedback, so the workspace-changes scanner excludes that directory and run delivery verification never counts them as produced artifacts -3. **ToolResultSanitizationMiddleware** - Neutralizes framework/injection tags (e.g. ``) and boundary markers in *remote-content* tool results (`web_fetch`/`web_search`/`image_search`/`web_capture`) so attacker-controlled fetched pages cannot forge trusted framework context. Mirrors `InputSanitizationMiddleware`'s user-input guardrail for the other untrusted-content entry point; sits inner of `ToolOutputBudgetMiddleware` (neutralizes the raw output, then the budget truncates). Local tool output (bash/read_file) is left untouched. Scope is a name-based allowlist, so MCP remote-content tools registered under other names (e.g. `fetch_url`) are not yet covered — a metadata-tagging follow-up is tracked in the middleware source +3. **ToolResultSanitizationMiddleware** - Neutralizes framework/injection tags (e.g. ``) and boundary markers in *remote-content* tool results (`web_fetch`/`web_search`/`image_search`/`web_capture`) so attacker-controlled fetched pages cannot forge trusted framework context. Mirrors `InputSanitizationMiddleware`'s user-input guardrail for the other untrusted-content entry point; sits inner of `ToolOutputBudgetMiddleware` (neutralizes the raw output, then the budget truncates). Local tool output (bash/read_file) is left untouched. Scope is a name-based allowlist for the first-party web tools, plus every MCP-sourced tool via its `deerflow_mcp` metadata tag, so an MCP server naming its fetcher `fetch_url` is still covered Result-rewriting middlewares between the raw callable boundary and the model-visible result append a declared entry to diff --git a/backend/packages/harness/deerflow/agents/middlewares/tool_result_sanitization_middleware.py b/backend/packages/harness/deerflow/agents/middlewares/tool_result_sanitization_middleware.py index 5c437aecf..89825f74d 100644 --- a/backend/packages/harness/deerflow/agents/middlewares/tool_result_sanitization_middleware.py +++ b/backend/packages/harness/deerflow/agents/middlewares/tool_result_sanitization_middleware.py @@ -16,9 +16,11 @@ network tools, so a fetched ```` is escaped to deliberately targets only the remote-content tools: local tool output (bash, file reads) is left untouched so legitimate code/log content is never mangled. -Scope note: matching is a name-based allowlist, so MCP-provided remote-content -tools registered under other names are not yet covered — see -``_REMOTE_CONTENT_TOOL_NAMES``. +Scope: the built-in network tools are matched by name +(``_REMOTE_CONTENT_TOOL_NAMES``), and MCP-sourced tools are matched by their +``deerflow_mcp`` metadata tag (third-party remote code, untrusted by default). +Local tool output (bash, file reads) is left untouched so legitimate code/log +content is never mangled. """ from __future__ import annotations @@ -35,6 +37,7 @@ from langgraph.prebuilt.tool_node import ToolCallRequest from langgraph.types import Command from deerflow.agents.middlewares.tool_transform_meta import append_tool_transform +from deerflow.tools.mcp_metadata import is_mcp_tool logger = logging.getLogger(__name__) @@ -45,15 +48,12 @@ logger = logging.getLogger(__name__) # surfaces the target site's response-status text (``X-Response-Status``, a # free-form reason phrase controlled by whatever server is being captured) into # its result message, so it is untrusted remote content too and belongs here. -# -# Known limitation: the gate is name-based. An MCP server may expose a -# remote-content tool under an arbitrary name (e.g. ``fetch_url`` / -# ``scrape_page``); its results are equally untrusted but are NOT matched here, -# so they reach the model unneutralized. A name heuristic (matching -# fetch/search/crawl substrings) is intentionally avoided because it would also -# mangle legitimate *local* tool output (e.g. a ``file_search`` result). Robust -# MCP coverage should tag remote-content tools via metadata at registration -# rather than by name; tracked as a follow-up. +# The gate is name-based for the first-party web tools; MCP-sourced tools are +# covered by their ``deerflow_mcp`` metadata tag instead (every MCP server is +# third-party remote code, so its results are untrusted regardless of what the +# tool is named). A name heuristic for MCP tools (matching fetch/search/crawl +# substrings) is intentionally avoided because it would also mangle legitimate +# *local* tool output (e.g. a ``file_search`` result). _REMOTE_CONTENT_TOOL_NAMES: frozenset[str] = frozenset( { "web_fetch", @@ -127,15 +127,17 @@ class ToolResultSanitizationMiddleware(AgentMiddleware[AgentState]): is returned unchanged. Mirrors the user-input guardrail so untrusted remote content and untrusted user input receive the same structural neutralization. - Scope is a name-based allowlist (``_REMOTE_CONTENT_TOOL_NAMES``): it reliably - covers the built-in web tools without false positives on local tools. It does - NOT cover MCP-provided remote-content tools registered under other names — - see the note on ``_REMOTE_CONTENT_TOOL_NAMES`` for why a name heuristic is - avoided and the metadata-tagging follow-up. + Scope: the built-in web tools are covered by name (``_REMOTE_CONTENT_TOOL_NAMES``), + and every MCP-sourced tool is covered via its ``deerflow_mcp`` metadata tag — + an MCP server is third-party remote code, so its results are untrusted by + default. Neutralization only touches structural control tokens, so benign MCP + content passes through unchanged. """ def _should_sanitize(self, request: ToolCallRequest) -> bool: - return request.tool_call.get("name") in _REMOTE_CONTENT_TOOL_NAMES + if request.tool_call.get("name") in _REMOTE_CONTENT_TOOL_NAMES: + return True + return is_mcp_tool(getattr(request, "tool", None)) @override def wrap_tool_call( diff --git a/backend/tests/test_tool_result_sanitization_middleware.py b/backend/tests/test_tool_result_sanitization_middleware.py index 304bf45ec..a6245f02e 100644 --- a/backend/tests/test_tool_result_sanitization_middleware.py +++ b/backend/tests/test_tool_result_sanitization_middleware.py @@ -188,19 +188,55 @@ class TestCommandAndContentShapes: class TestKnownScopeBoundary: - """Pin the documented name-based scope so any coverage change is deliberate.""" + """Pin the documented coverage scope so any change is deliberate.""" - def test_mcp_named_remote_tool_is_not_sanitized(self): - # KNOWN LIMITATION: an MCP tool registered under an arbitrary name - # (e.g. `fetch_url`) is remote content but is NOT matched by the - # name allowlist, so it is passed through unchanged today. This test - # documents that boundary; broadening coverage (metadata tagging) is a - # tracked follow-up and should update this test intentionally. + def test_untagged_mcp_named_tool_is_not_sanitized(self): + # An MCP-registered tool that never got the deerflow_mcp metadata tag + # (e.g. loaded through a path that does not tag) is still passed through + # unchanged. Coverage follows the tag, not the name. The tool object is + # present here with a non-empty metadata dict, so the untagged branch of + # (metadata or {}).get(key) is what this test pins. + mw = ToolResultSanitizationMiddleware() + msg = _msg(_MALICIOUS_PAGE, name="fetch_url") + request = SimpleNamespace( + tool_call={"name": "fetch_url", "id": "tc-1"}, + tool=SimpleNamespace(metadata={"other_marker": True}), + ) + result = mw.wrap_tool_call(request, lambda _: msg) + assert result is msg + assert "" in result.content + + +class TestMcpTaggedToolResults: + """MCP-sourced tools (third-party remote code) are untrusted by default.""" + + @staticmethod + def _mcp_request(tool_name: str) -> SimpleNamespace: + return SimpleNamespace( + tool_call={"name": tool_name, "id": "tc-1"}, + tool=SimpleNamespace(metadata={"deerflow_mcp": True}), + ) + + def test_mcp_tagged_tool_result_sanitized(self): + mw = ToolResultSanitizationMiddleware() + msg = _msg(_MALICIOUS_PAGE, name="fetch_url") + result = mw.wrap_tool_call(self._mcp_request("fetch_url"), lambda _: msg) + assert "<system-reminder>" in result.content + assert "" not in result.content + + def test_mcp_tagged_clean_result_returns_same_object(self): + mw = ToolResultSanitizationMiddleware() + msg = _msg("# Title\n\nJust clean gardening content.", name="fetch_url") + result = mw.wrap_tool_call(self._mcp_request("fetch_url"), lambda _: msg) + assert result is msg + + def test_missing_tool_attr_stays_untouched(self): + # ToolCallRequest.tool is optional; requests without it (and untagged + # tools) keep the old behavior. mw = ToolResultSanitizationMiddleware() msg = _msg(_MALICIOUS_PAGE, name="fetch_url") result = mw.wrap_tool_call(_request("fetch_url"), lambda _: msg) assert result is msg - assert "" in result.content class TestAsyncPath: