fix(security): sanitize MCP-sourced tool results through the same trust boundary (#4839)

* fix(security): sanitize MCP-sourced tool results through the same trust boundary

Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com>

* fix(security): sync the trust-boundary docs with tag coverage and pin the untagged branch

Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com>

---------

Signed-off-by: Yufeng He <40085740+he-yufeng@users.noreply.github.com>

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. `<system-reminder>`) 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. `<system-reminder>`) 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

backend/packages/harness/deerflow/agents/middlewares/tool_result_sanitization_middleware.py

@@ -16,9 +16,11 @@ network tools, so a fetched ``<system-reminder>`` 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(

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 "<system-reminder>" 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 "&lt;system-reminder&gt;" in result.content
+        assert "<system-reminder>" 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 "<system-reminder>" in result.content
 
 
 class TestAsyncPath: