Fix: Dark Lab tool override silently drops parameters/inputSchema

finbot/apps/darklab/routes/api.py

@@ -5,7 +5,7 @@
 import logging
 
 from fastapi import APIRouter, Depends, HTTPException, Query
-from pydantic import BaseModel
+from pydantic import BaseModel, field_validator
 from sqlalchemy.orm import Session
 
 from finbot.core.auth.middleware import get_session_context
@@ -36,6 +36,41 @@ def _get_mcp_defaults() -> dict:
 class ToolOverridesUpdate(BaseModel):
     tool_overrides: dict
 
+    @field_validator("tool_overrides")
+    @classmethod
+    def _validate_override_shape(cls, value: dict) -> dict:
+        """A malformed override -- a non-object entry, or a non-dict
+        "parameters"/"inputSchema", or a non-string "description" -- none
+        of these fail here or when applied to the live tool (fastmcp's
+        Tool doesn't validate on plain attribute assignment) -- they fail
+        later, in unrelated code that lists this server's tools, breaking
+        tool discovery (or, for a non-object entry, breaking server
+        creation entirely) until the override is reset. Reject all of
+        these at write time instead, with a clear error, rather than
+        letting a malformed override silently break a future, unrelated
+        request.
+        """
+        for tool_name, override in value.items():
+            if not isinstance(override, dict):
+                raise ValueError(
+                    f"Tool override for '{tool_name}' must be an object, "
+                    f"got {type(override).__name__}"
+                )
+            if "description" in override and not isinstance(
+                override["description"], str
+            ):
+                raise ValueError(
+                    f"Tool override for '{tool_name}': 'description' must "
+                    f"be a string, got {type(override['description']).__name__}"
+                )
+            for key in ("parameters", "inputSchema"):
+                if key in override and not isinstance(override[key], dict):
+                    raise ValueError(
+                        f"Tool override for '{tool_name}': '{key}' must be "
+                        f"an object, got {type(override[key]).__name__}"
+                    )
+        return value
+
 
 @router.get("/supply-chain/servers")
 async def list_servers_with_tools(

finbot/mcp/factory.py

@@ -36,10 +36,19 @@ def _import_factory(dotted_path: str) -> Any:
 
 
 async def _apply_tool_overrides(server: FastMCP, overrides: dict) -> None:
-    """Apply user-supplied tool description overrides to a FastMCP server.
-
-    Modifies tool descriptions (the text the LLM sees) via the provider's
-    get_tool() API. This is the primary CTF attack surface for tool poisoning.
+    """Apply user-supplied tool overrides to a FastMCP server.
+
+    Modifies tool descriptions and/or parameter schemas (the text and
+    schema the LLM sees) via the provider's get_tool() API. This is the
+    primary CTF attack surface for tool poisoning -- both description
+    poisoning and parameter-schema poisoning.
+
+    Accepts "parameters" or "inputSchema" as the override key for the
+    schema (both are used in the wild -- "inputSchema" is the MCP
+    wire-protocol field name, "parameters" is fastmcp's internal Tool
+    attribute name that actually needs to be set for the override to take
+    effect; setting an "inputSchema" attribute directly on a Tool object
+    does nothing, since that name isn't a declared field there).
     """
     if not overrides:
         return
@@ -49,17 +58,76 @@ async def _apply_tool_overrides(server: FastMCP, overrides: dict) -> None:
         return
 
     for tool_name, override in overrides.items():
+        if not isinstance(override, dict):
+            # Malformed top-level structure (e.g. a tool's override is a
+            # bare string, not an object). The API-level validator rejects
+            # this at write time, but nothing guarantees every row in the
+            # DB went through that path (a future direct write, a seed
+            # script, a migration). override.get(...) below would raise
+            # AttributeError on a non-dict, uncaught -- crashing server
+            # creation entirely, not just tool listing. Skip it instead.
+            logger.warning(
+                "Ignoring malformed override for '%s': expected an object, got %r",
+                tool_name,
+                type(override).__name__,
+            )
+            continue
+
         new_description = override.get("description")
-        if new_description:
-            try:
-                tool = await provider.get_tool(tool_name)
-                if tool:
-                    tool.description = new_description
-                    logger.debug(
-                        "Applied tool override for '%s': description updated", tool_name
-                    )
-            except Exception:
-                logger.debug("Tool '%s' not found for override", tool_name)
+        if new_description is not None and not isinstance(new_description, str):
+            # Same failure shape as the parameters case below: succeeds
+            # silently on plain attribute assignment, then fails later in
+            # to_mcp_tool() when this server's tools are next listed.
+            logger.warning(
+                "Ignoring non-string description override for '%s': %r",
+                tool_name,
+                type(new_description).__name__,
+            )
+            new_description = None
+
+        # Presence-check, not truthiness: a deliberate "parameters": {}
+        # override (stripping every param off a tool) is falsy and must
+        # not be silently dropped the same way the original bug dropped
+        # parameters entirely.
+        if "parameters" in override:
+            new_parameters = override["parameters"]
+        elif "inputSchema" in override:
+            new_parameters = override["inputSchema"]
+        else:
+            new_parameters = None
+
+        if new_parameters is not None and not isinstance(new_parameters, dict):
+            # A non-dict schema doesn't fail here (pydantic doesn't
+            # validate on plain attribute assignment) -- it fails later,
+            # in unrelated code that lists this server's tools, breaking
+            # tool discovery entirely until the override is reset. Reject
+            # it at the one place that actually writes to the live tool.
+            logger.warning(
+                "Ignoring non-dict parameters override for '%s': %r",
+                tool_name,
+                type(new_parameters).__name__,
+            )
+            new_parameters = None
+
+        if not new_description and new_parameters is None:
+            continue
+
+        try:
+            tool = await provider.get_tool(tool_name)
+            if not tool:
+                continue
+            if new_description:
+                tool.description = new_description
+            if new_parameters is not None:
+                tool.parameters = new_parameters
+            logger.debug(
+                "Applied tool override for '%s': description=%s parameters=%s",
+                tool_name,
+                bool(new_description),
+                new_parameters is not None,
+            )
+        except Exception:
+            logger.debug("Tool '%s' not found for override", tool_name)
 
 
 async def create_mcp_server(

tests/unit/apps/test_darklab_tool_overrides.py

@@ -0,0 +1,80 @@
+# Tests for ToolOverridesUpdate's parameters-shape validation (issue #547).
+#
+# Complements tests/unit/mcp/test_factory.py, which covers the runtime
+# application side (_apply_tool_overrides silently rejecting a bad schema
+# rather than letting it later crash tool listing). This covers the write
+# boundary: reject a malformed override at request time with a clear 422,
+# rather than accepting it into the DB and deferring the failure to an
+# unrelated future request.
+
+import pytest
+from pydantic import ValidationError
+
+from finbot.apps.darklab.routes.api import ToolOverridesUpdate
+
+
+class TestToolOverridesUpdateValidation:
+
+    @pytest.mark.unit
+    def test_accepts_valid_description_only_override(self):
+        ToolOverridesUpdate(tool_overrides={"send_email": {"description": "x"}})
+
+    @pytest.mark.unit
+    def test_accepts_valid_parameters_override(self):
+        ToolOverridesUpdate(
+            tool_overrides={
+                "send_email": {"parameters": {"type": "object", "properties": {}}}
+            }
+        )
+
+    @pytest.mark.unit
+    def test_accepts_valid_inputschema_override(self):
+        ToolOverridesUpdate(
+            tool_overrides={"send_email": {"inputSchema": {"type": "object"}}}
+        )
+
+    @pytest.mark.unit
+    def test_accepts_empty_dict_parameters(self):
+        """A deliberate {} override (stripping every param) is a valid,
+        meaningful override -- must not be rejected."""
+        ToolOverridesUpdate(tool_overrides={"send_email": {"parameters": {}}})
+
+    @pytest.mark.unit
+    def test_rejects_non_dict_parameters(self):
+        with pytest.raises(ValidationError, match="parameters"):
+            ToolOverridesUpdate(
+                tool_overrides={"send_email": {"parameters": "not a dict"}}
+            )
+
+    @pytest.mark.unit
+    def test_rejects_non_dict_inputschema(self):
+        with pytest.raises(ValidationError, match="inputSchema"):
+            ToolOverridesUpdate(
+                tool_overrides={"send_email": {"inputSchema": ["not", "a", "dict"]}}
+            )
+
+    @pytest.mark.unit
+    def test_rejects_non_dict_override_entry(self):
+        """A whole tool's override being a non-object (e.g. a bare
+        string) must be rejected at write time -- not silently skipped,
+        since that would let malformed structure into the DB that later
+        crashes _apply_tool_overrides when applied."""
+        with pytest.raises(ValidationError, match="object"):
+            ToolOverridesUpdate(tool_overrides={"send_email": "not even a dict"})
+
+    @pytest.mark.unit
+    def test_rejects_non_string_description(self):
+        with pytest.raises(ValidationError, match="description"):
+            ToolOverridesUpdate(tool_overrides={"send_email": {"description": 12345}})
+
+    @pytest.mark.unit
+    def test_rejects_non_dict_parameters_among_multiple_tools(self):
+        """A bad override anywhere in the batch is rejected -- not just
+        silently skipped while the rest apply."""
+        with pytest.raises(ValidationError):
+            ToolOverridesUpdate(
+                tool_overrides={
+                    "send_email": {"description": "fine"},
+                    "delete_file": {"parameters": 42},
+                }
+            )