rebenitez1802 commented on code in PR #45485:
URL: https://github.com/apache/superset/pull/45485#discussion_r4337141306
##########
superset/mcp_service/utils/schema_utils.py:
##########
@@ -25,6 +25,8 @@
from __future__ import annotations
import logging
+from collections.abc import Iterator
+from contextlib import contextmanager
from typing import Any, Callable, List, Type, TypeVar
from pydantic import BaseModel, GetJsonSchemaHandler, ValidationError
Review Comment:
Part of the `unsafe_text` suggestion on lines 49–52.
```suggestion
from pydantic import BaseModel, GetJsonSchemaHandler, ValidationError
from pydantic_core import PydanticCustomError
```
##########
tests/unit_tests/mcp_service/test_sanitization_warning_inputs.py:
##########
@@ -78,3 +82,164 @@ def test_sanitization_warnings_are_server_only(
assert "caller-controlled" not in warnings[0]
else:
assert warnings == []
+
+
[email protected](
+ ("model", "payload", "field"),
+ [
+ (GenerateDashboardRequest, {"chart_ids": [1]}, "dashboard_title"),
+ (UpdateDashboardRequest, {"identifier": 1}, "dashboard_title"),
+ (DuplicateDashboardRequest, {"dashboard_id": 1}, "dashboard_title"),
+ (
+ GenerateChartRequest,
+ {
+ "dataset_id": 1,
+ "config": {"chart_type": "table", "columns": [{"name": "a"}]},
+ },
+ "chart_name",
+ ),
+ ],
+)
[email protected](
+ "title",
+ [
+ "[DEMO] Service Desk - Executive Summary "
+ "(substitute data: Jira support tickets)",
+ "Service Desk (source data: issue records and support tickets)",
+ ],
+)
+def test_title_accepts_data_in_prose(
+ model: type[
+ GenerateChartRequest
+ | GenerateDashboardRequest
+ | UpdateDashboardRequest
+ | DuplicateDashboardRequest
+ ],
+ payload: dict[str, Any],
+ field: str,
+ title: str,
+) -> None:
+ """A colon in prose is not a data URL scheme."""
+ request = model.model_validate({**payload, field: title})
+ assert getattr(request, field) == title
+ assert request.sanitization_warnings == []
+
+
[email protected](
+ ("model", "payload", "field"),
+ [
+ (GenerateDashboardRequest, {"chart_ids": [1]}, "dashboard_title"),
+ (UpdateDashboardRequest, {"identifier": 1}, "dashboard_title"),
+ (DuplicateDashboardRequest, {"dashboard_id": 1}, "dashboard_title"),
+ (
+ GenerateChartRequest,
+ {
+ "dataset_id": 1,
+ "config": {"chart_type": "table", "columns": [{"name": "a"}]},
+ },
+ "chart_name",
+ ),
+ ],
+)
[email protected](
+ "value",
+ [
+ "javascript:alert(1)",
+ "data:text/html,payload",
+ "<script>alert(1)</script>",
+ "Inspect data: text,URL_PROBE",
+ ],
+)
+def test_title_sanitization_error_names_field(
+ model: type[
+ GenerateChartRequest
+ | GenerateDashboardRequest
+ | UpdateDashboardRequest
+ | DuplicateDashboardRequest
+ ],
+ payload: dict[str, Any],
+ field: str,
+ value: str,
+) -> None:
+ """Pre-field warning detection must preserve the rejected field's
location."""
+ with pytest.raises(ValidationError) as exc:
+ model.model_validate({**payload, field: value})
+ assert exc.value.errors()[0]["loc"] == (field,)
+
+
[email protected]
[email protected](
+ ("tool", "payload", "field"),
+ [
+ ("generate_dashboard", {"chart_ids": [1]}, "dashboard_title"),
+ ("update_dashboard", {"identifier": 1}, "dashboard_title"),
+ ("duplicate_dashboard", {"dashboard_id": 1}, "dashboard_title"),
+ (
+ "generate_chart",
+ {
+ "dataset_id": 1,
+ "config": {"chart_type": "table", "columns": [{"name": "a"}]},
+ },
+ "chart_name",
+ ),
+ ("update_chart", {"identifier": 1}, "chart_name"),
+ ],
+)
[email protected]("alias", [False, True])
[email protected](
+ "rejected",
+ [
+ "javascript:alert('private-input')",
+ "Inspect data: text,private-input",
+ "java\tscript:private-input",
+ "vb\nscript:private-input",
+ "dat\ra:text/html,private-input",
+ ],
+)
+async def test_title_sanitization_wire_error_names_field(
+ tool: str,
+ payload: dict[str, Any],
+ field: str,
+ alias: bool,
+ rejected: str,
+) -> None:
+ """Real MCP argument validation names fields without exposing rejected
input."""
+ server = FastMCP("title-validation")
+ server.add_middleware(ToolResultCompatibilityMiddleware())
+
+ @server.tool
+ def generate_dashboard(request: GenerateDashboardRequest) -> str:
+ """Validate dashboard creation."""
+ return "ok"
+
+ @server.tool
+ def update_dashboard(request: UpdateDashboardRequest) -> str:
+ """Validate dashboard updates."""
+ return "ok"
+
+ @server.tool
+ def duplicate_dashboard(request: DuplicateDashboardRequest) -> str:
+ """Validate dashboard duplication."""
+ return "ok"
+
+ @server.tool
+ def generate_chart(request: GenerateChartRequest) -> str:
+ """Validate chart creation."""
+ return "ok"
+
+ @server.tool
+ def update_chart(request: UpdateChartRequest) -> str:
+ """Validate chart updates."""
+ return "ok"
+
+ async with Client(server) as client:
+ result = await client.call_tool_mcp(
+ tool, {"request": {**payload, "title" if alias else field:
rejected}}
+ )
+ assert result.isError is True
+ message = " ".join(
+ block.text for block in result.content if isinstance(block,
TextContent)
+ )
+ assert f"request.{field}: Invalid value" in message
Review Comment:
Part of the `unsafe_text` suggestion in `schema_utils.py`: this test pins
the generic reason.
```suggestion
assert f"request.{field}: Contains disallowed content" in message
```
##########
superset/mcp_service/utils/schema_utils.py:
##########
@@ -34,6 +36,22 @@
T = TypeVar("T")
+@contextmanager
+def field_validation_error(field_name: str) -> Iterator[None]:
+ """Attach a declared field location to checks in a model-level validator.
+
+ Keep the original ValueError for Pydantic consumers; the MCP diagnostic
+ formatter exposes only the schema-backed location and a static reason.
+ """
+ try:
+ yield
+ except ValueError as ex:
+ raise ValidationError.from_exception_data(
+ "Field validation",
+ [{"type": "value_error", "loc": (field_name,), "ctx": {"error":
ex}}],
+ ) from ex
Review Comment:
🟡 Medium — Rejected titles still get an opaque reason
A title like `Exec (source data: Jira, Zendesk)` is still rejected, and the
MCP error reads `request.dashboard_title: Invalid value; check the field
schema`. That's because `value_error` is intentionally kept out of `_REASONS`.
The LLM caller can't tell what to change, and comma lists are common in
generated titles. A custom error type lets the formatter return a fixed,
input-free reason:
```suggestion
error = PydanticCustomError("unsafe_text", "{error}", {"error":
str(ex)})
raise ValidationError.from_exception_data(
"Field validation", [{"type": error, "loc": (field_name,)}]
) from ex
```
Apply this together with the import and test-assertion suggestions in this
review, and add the reason in `superset/mcp_service/utils/validation.py`, which
isn't in this diff:
```python
_REASONS = {
"missing": "Field required",
"unsafe_text": (
"Contains disallowed content (HTML, script or data: URL, event
handler, "
"or SQL keyword); a 'data:' label must not be followed by a comma"
),
...
```
I tried this locally on `c37b155`. `generate_dashboard`, `update_dashboard`,
`duplicate_dashboard` and `generate_chart` then return `request.<field>:
Contains disallowed content …`, still without echoing the input, and the MCP
schema/sanitization suites pass. `update_chart` is the exception: it rejects
through `UpdateChartRequest.sanitize_chart_name`, a plain `@field_validator`,
so it still reports `Invalid value` (10 of the 40
`test_title_sanitization_wire_error_names_field` cases). To cover it, raise
`PydanticCustomError("unsafe_text", …)` from that validator too, or move the
chart-name check into a `field_validation_error` block like
`GenerateChartRequest` does.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
