aminghadersohi commented on code in PR #45485:
URL: https://github.com/apache/superset/pull/45485#discussion_r4337260804
##########
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:
Fixed in e8c421c3c2f319f6ec2d86102fa6fb925bddb972. The formatter maps
unsafe_text to an input-free, actionable reason, including the comma
restriction; update_chart uses the same error type.
Regression test:
test_title_sanitization_wire_error_names_field_with_actionable_reason — 60
failed before, 60 passed after (five tools, canonical fields and title aliases).
Siblings checked:
- superset/mcp_service/chart/schemas.py — fixed in
e8c421c3c2f319f6ec2d86102fa6fb925bddb972
- superset/mcp_service/dashboard/schemas.py — not affected: all three title
paths use the shared helper and are covered by the wire test
- superset/mcp_service/utils/validation.py — fixed in
e8c421c3c2f319f6ec2d86102fa6fb925bddb972
##########
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:
Fixed in e8c421c3c2f319f6ec2d86102fa6fb925bddb972. The PydanticCustomError
import supports the shared unsafe_text error type.
Regression test:
test_title_sanitization_wire_error_names_field_with_actionable_reason — 60
failed before, 60 passed after (five tools, canonical fields and title aliases).
Siblings checked:
- superset/mcp_service/chart/schemas.py — fixed in
e8c421c3c2f319f6ec2d86102fa6fb925bddb972
- superset/mcp_service/dashboard/schemas.py — not affected: all three title
paths use the shared helper and are covered by the wire test
- superset/mcp_service/utils/validation.py — fixed in
e8c421c3c2f319f6ec2d86102fa6fb925bddb972
##########
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:
Fixed in e8c421c3c2f319f6ec2d86102fa6fb925bddb972. The wire assertion
requires the actionable reason and comma guidance without echoing the input.
Regression test:
test_title_sanitization_wire_error_names_field_with_actionable_reason — 60
failed before, 60 passed after (five tools, canonical fields and title aliases).
Siblings checked:
- superset/mcp_service/chart/schemas.py — fixed in
e8c421c3c2f319f6ec2d86102fa6fb925bddb972
- superset/mcp_service/dashboard/schemas.py — not affected: all three title
paths use the shared helper and are covered by the wire test
- superset/mcp_service/utils/validation.py — fixed in
e8c421c3c2f319f6ec2d86102fa6fb925bddb972
--
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]
