aminghadersohi commented on code in PR #45575:
URL: https://github.com/apache/superset/pull/45575#discussion_r4354259962
##########
superset/mcp_service/utils/sanitization.py:
##########
@@ -70,11 +70,36 @@ def _strip_html_tags(value: str) -> str:
# url_schemes=set() blocks all URL schemes in any remaining attributes
cleaned = nh3.clean(decoded, tags=set(), url_schemes=set())
- # Only restore & → & to preserve ampersands in display text (e.g. "A &
B").
- # Do NOT use html.unescape() here: nh3 may pass through HTML entities from
- # the input (e.g. <script>), and a full unescape would re-introduce
- # raw angle brackets, creating an XSS vector.
- return cleaned.replace("&", "&")
+ # Restore brackets only before comparison punctuation, numbers or
whitespace.
+ # Non-ASCII letters can case-fold into tag names in downstream renderers;
+ # leave those and reconstructed tag openers escaped.
+ # Normalize removable characters before deciding whether a bracket is safe;
+ # otherwise a later Unicode strip could turn <\u200bscript into markup.
+ cleaned = _remove_dangerous_unicode(cleaned)
+ # Greater-than signs and NBSP cannot open tags. Restore them so plain text
+ # neither changes on display nor outgrows the model-column length limits.
+ return (
+ re.sub(r"<(?=[\d\s$€£¥%+=.,:;)\]&-]|$)", "<", cleaned)
Review Comment:
Fixed in 0c05b084da93e460e38157f057f171f40479d721. I verified the suggested
allowlist against the report-title normalization path: control whitespace stays
escaped before non-whitespace content; trailing whitespace and literal
comparisons retain their existing behavior.
Regression tests:
`tests/unit_tests/mcp_service/test_sanitization_warning_inputs.py::test_title_control_whitespace_cannot_form_slack_markup`
covers chart create (including unsaved), chart update, and all three dashboard
title schemas, four Slack control forms, six whitespace characters, and
resubmission.
`tests/unit_tests/mcp_service/utils/test_sanitization.py::test_control_whitespace_warning_wrapper_keeps_slack_markup_inert`
pins the exact normalized escaped output and warning for literal and
entity-encoded TAB/LF/CR. The selected new tests produced 87 failures against
the reviewed sanitizer and pass with this commit (nh3 0.3.7, pydantic 2.14.1,
fastmcp 4.1.2); the two touched files pass 1,019 tests. Filter rejection and
safe trailing whitespace are separately pinned.
Siblings checked:
- superset/mcp_service/utils/sanitization.py:39 — fixed in
0c05b084da93e460e38157f057f171f40479d721
- superset/mcp_service/utils/sanitization.py:233 — fixed in
0c05b084da93e460e38157f057f171f40479d721
- superset/mcp_service/utils/sanitization.py:261 — fixed in
0c05b084da93e460e38157f057f171f40479d721
- superset/mcp_service/utils/sanitization.py:363 — fixed in
0c05b084da93e460e38157f057f171f40479d721
- superset/mcp_service/chart/schemas.py:879 — fixed in
0c05b084da93e460e38157f057f171f40479d721
- superset/mcp_service/chart/schemas.py:1003 — fixed in
0c05b084da93e460e38157f057f171f40479d721
- superset/mcp_service/chart/schemas.py:1011 — fixed in
0c05b084da93e460e38157f057f171f40479d721
- superset/mcp_service/chart/schemas.py:1132 — fixed in
0c05b084da93e460e38157f057f171f40479d721
- superset/mcp_service/chart/schemas.py:1143 — fixed in
0c05b084da93e460e38157f057f171f40479d721
- superset/mcp_service/chart/schemas.py:1144 — fixed in
0c05b084da93e460e38157f057f171f40479d721
- superset/mcp_service/chart/schemas.py:1211 — fixed in
0c05b084da93e460e38157f057f171f40479d721
- superset/mcp_service/chart/schemas.py:2761 — fixed in
0c05b084da93e460e38157f057f171f40479d721
- superset/mcp_service/chart/schemas.py:3155 — fixed in
0c05b084da93e460e38157f057f171f40479d721
- superset/mcp_service/chart/schemas.py:4628 — fixed in
0c05b084da93e460e38157f057f171f40479d721
- superset/mcp_service/chart/schemas.py:4637 — fixed in
0c05b084da93e460e38157f057f171f40479d721
- superset/mcp_service/chart/schemas.py:6369 — fixed in
0c05b084da93e460e38157f057f171f40479d721
- superset/mcp_service/dashboard/schemas.py:884 — fixed in
0c05b084da93e460e38157f057f171f40479d721
- superset/mcp_service/dashboard/schemas.py:1050 — fixed in
0c05b084da93e460e38157f057f171f40479d721
- superset/mcp_service/dashboard/schemas.py:1393 — fixed in
0c05b084da93e460e38157f057f171f40479d721
- superset/mcp_service/dashboard/schemas.py:1414 — fixed in
0c05b084da93e460e38157f057f171f40479d721
- superset/mcp_service/dashboard/schemas.py:1531 — fixed in
0c05b084da93e460e38157f057f171f40479d721
- superset/mcp_service/utils/sanitization.py:105 — not affected: the
stability check delegates to the same stripping helper; the new model
resubmission assertions pass.
- superset/utils/file.py:47 — not affected: control stripping is intentional
and unchanged; regression assertions apply this function to the stored MCP
title.
- superset/commands/report/execute.py:1656 — not affected: chart and
dashboard report names use sanitize_title; MCP titles reach it with the bracket
escaped.
- superset/reports/notifications/slack_mixin.py:50 — not affected: both
Slack notification transports share this template; the MCP title stays escaped
before interpolation.
- superset/mcp_service/utils/error_builder.py:54 — not affected: independent
error-context sanitizer, not a caller of the changed helper or a persisted
title.
- superset/mcp_service/utils/sanitization.py:465 — not affected:
SQL-expression validation does not call the HTML-stripping helper.
Search: `rg -n
'_strip_html_tags\(|sanitize_user_input\(|sanitize_user_input_with_changes\(|sanitize_filter_value\('
superset/mcp_service`; `rg -n 'sanitize_title' superset`; `rg -n
'SlackMixin|content.name' superset/reports/notifications`.
##########
tests/unit_tests/mcp_service/test_sanitization_warning_inputs.py:
##########
@@ -524,3 +579,683 @@ def validate(request: BaseModel) -> str:
)
assert f"request.{field}: Contains disallowed content" in message
assert "private-input" not in message
+
+
[email protected]
[email protected](
+ ("tool", "payload", "field", "limit"),
+ [
+ ("generate_dashboard", {"chart_ids": [1]}, "dashboard_title", 500),
+ ("update_dashboard", {"identifier": 1}, "dashboard_title", 500),
+ ("duplicate_dashboard", {"dashboard_id": 1}, "dashboard_title", 500),
+ (
+ "generate_chart",
+ {
+ "dataset_id": 1,
+ "config": {"chart_type": "table", "columns": [{"name": "a"}]},
+ },
+ "chart_name",
+ 250,
+ ),
+ ("update_chart", {"identifier": 1}, "chart_name", 250),
+ ],
+)
[email protected]("alias", [None, "title", "name"])
[email protected]("length", [None, 600])
+async def test_title_length_wire_reason(
+ tool: str,
+ payload: dict[str, Any],
+ field: str,
+ limit: int,
+ alias: str | None,
+ length: int | None,
+) -> None:
+ """Length errors retain their reason through warning detection and MCP."""
+ rejected = "x" * (length or limit + 1)
+ async with Client(make_title_server()) as client:
+ result = await client.call_tool_mcp(
+ tool, {"request": {**payload, alias or field: rejected}}
+ )
+ assert result.isError is True
+ assert result.content == [
+ TextContent(
+ type="text",
+ text=f"Error: Validation error in {tool}: request.{field}: "
+ "String exceeds the allowed maximum length",
+ )
+ ]
+
+
[email protected]
[email protected]("field", ["dashboard_title", "title", "name"])
[email protected]("rejected", ["", " \t\n"])
+async def test_duplicate_dashboard_empty_title_wire_reason(
+ field: str,
+ rejected: str,
+) -> None:
+ """Blank required titles have a static, actionable reason on the wire."""
+ async with Client(make_title_server()) as client:
+ result = await client.call_tool_mcp(
+ "duplicate_dashboard", {"request": {"dashboard_id": 1, field:
rejected}}
+ )
+ assert result.isError is True
+ assert result.content == [
+ TextContent(
+ type="text",
+ text="Error: Validation error in duplicate_dashboard: "
+ "request.dashboard_title: Text must not be empty",
+ )
+ ]
+
+
+_REQUIRED_NAME_CASES: list[tuple[str, dict[str, Any], str, list[str | None]]]
= [
+ (
+ "create_virtual_dataset",
+ {"database_id": 1, "sql": "select 1"},
+ "dataset_name",
+ ["", " \t\n"],
+ ),
+ ("create_theme", {"json_data": {}}, "theme_name", ["", " \t\n"]),
+ (
+ "create_dataset_metric",
+ {"dataset_id": 1, "expression": "count(*)"},
+ "metric_name",
+ ["", " \t\n"],
+ ),
+ (
+ "update_dataset_metric",
+ {"dataset_id": 1, "metric": 1},
+ "metric_name",
+ ["", " \t\n", None],
+ ),
+ ("update_dataset", {"dataset_id": 1}, "table_name", ["", " \t\n", None]),
+ (
+ "save_sql_query",
+ {"database_id": 1, "sql": "select 1"},
+ "label",
+ ["", " \t\n"],
+ ),
+]
+
+
[email protected]
[email protected](
+ ("tool", "payload", "field", "rejected"),
+ [
+ (tool, payload, field, rejected)
+ for tool, payload, field, blanks in _REQUIRED_NAME_CASES
+ for rejected in blanks
+ ],
+)
+async def test_required_name_empty_wire_reason(
+ tool: str, payload: dict[str, Any], field: str, rejected: str | None
+) -> None:
+ """Blank object names report a safe reason at the declared field."""
+ async with Client(make_name_server()) as client:
+ result = await client.call_tool_mcp(
+ tool, {"request": {**payload, field: rejected}}
+ )
+ assert result.isError is True
+ assert result.content == [
+ TextContent(
+ type="text",
+ text=f"Error: Validation error in {tool}: request.{field}: "
+ "Text must not be empty",
+ )
+ ]
+
+
[email protected](
+ "model,payload",
+ [
+ (
+ GenerateChartRequest,
+ {
+ "dataset_id": 1,
+ "config": {"chart_type": "table", "columns": [{"name": "a"}]},
+ },
+ ),
+ (UpdateChartRequest, {"identifier": 1}),
+ ],
+)
[email protected]("length", [250, 251, 252, 253, 254, 255])
+def test_chart_name_model_column_boundary(
+ model: type[BaseModel], payload: dict[str, Any], length: int
+) -> None:
+ """Chart names must fit the persisted Slice name column."""
+ from superset.models.slice import Slice
+
+ limit = Slice.__table__.c.slice_name.type.length
+ assert limit == 250
+ if length <= limit:
+ assert (
+ model.model_validate({**payload, "chart_name": "x" *
length}).chart_name
+ == "x" * length
+ )
+ else:
+ with pytest.raises(ValidationError) as exc:
+ model.model_validate({**payload, "chart_name": "x" * length})
+ assert exc.value.errors()[0]["type"] == "string_too_long"
+ assert exc.value.errors()[0]["loc"] == ("chart_name",)
+
+
[email protected](
+ "model,payload",
+ [
+ (
+ GenerateChartRequest,
+ {
+ "dataset_id": 1,
+ "config": {"chart_type": "table", "columns": [{"name": "a"}]},
+ },
+ ),
+ (UpdateChartRequest, {"identifier": 1}),
+ ],
+)
[email protected]("alias", ["chart_name", "name", "title",
"slice_name"])
[email protected]("length", [250, 251])
+def test_padded_chart_name_boundary(
+ model: type[BaseModel], payload: dict[str, Any], alias: str, length: int
+) -> None:
+ """Validate the trimmed chart name against the model column boundary."""
+ value = " " + "x" * length + " "
+ if length == 250:
+ assert (
+ model.model_validate({**payload, alias: value}).chart_name == "x"
* length
+ )
+ else:
+ with pytest.raises(ValidationError) as exc:
+ model.model_validate({**payload, alias: value})
+ assert exc.value.errors()[0]["type"] == "string_too_long"
+
+
[email protected]("length", [250, 251, 310])
+def test_theme_name_model_column_boundary(length: int) -> None:
+ """Theme creation must respect its persisted name column."""
+ from superset.models.core import Theme
+
+ assert Theme.__table__.c.theme_name.type.length == 250
+ if length == 250:
+ assert (
+ CreateThemeRequest(theme_name="x" * length,
json_data={}).theme_name
+ == "x" * length
+ )
+ else:
+ with pytest.raises(ValidationError) as exc:
+ CreateThemeRequest(theme_name="x" * length, json_data={})
+ assert exc.value.errors()[0]["type"] == "string_too_long"
+
+
[email protected]
[email protected]("alias", ["dashboard_title", "title", "name"])
[email protected]("value", ["", " \t\n"])
+async def test_update_dashboard_blank_title_wire_reason(alias: str, value:
str) -> None:
+ """Reject blank renames rather than reporting a title that was not
saved."""
+ async with Client(make_title_server()) as client:
+ result = await client.call_tool_mcp(
+ "update_dashboard", {"request": {"identifier": 1, alias: value}}
+ )
+ assert result.isError is True
+ assert "request.dashboard_title: Text must not be empty" in
result.content[0].text
+
+
[email protected]("alias", ["chart_name", "name", "title",
"slice_name"])
[email protected]("script_only", [True, False])
+def test_update_chart_sanitization_matches_generate(
+ alias: str, script_only: bool
+) -> None:
+ """Update rejects stripped-only names and warns on partially stripped
names."""
+ if script_only:
+ with pytest.raises(ValidationError) as exc:
+ UpdateChartRequest.model_validate(
+ {"identifier": 1, alias: "<script>alert(1)</script>"}
+ )
+ assert exc.value.errors()[0]["type"] == "unsafe_text"
+ assert exc.value.errors()[0]["loc"] == ("chart_name",)
+ return
+ request = UpdateChartRequest.model_validate(
+ {
+ "identifier": 1,
+ alias: "Weekly KPIs <img src=x onerror=alert(1)>",
+ "sanitization_warnings": ["untrusted"],
+ }
+ )
+ assert request.chart_name == "Weekly KPIs"
+ assert len(request.sanitization_warnings) == 1
+ assert "untrusted" not in request.sanitization_warnings
+ assert "sanitization_warnings" not in json.dumps(
+ UpdateChartRequest.model_json_schema()
+ )
+
+
+def test_duplicate_dataset_error_is_readable() -> None:
+ """Duplicate errors use a plain string and preserve display-name
characters."""
+ from superset.commands.dataset.exceptions import
DatasetExistsValidationError
+ from superset.sql.parse import Table
+
+ name = "Sales “Q3” & Growth"
+ error = DatasetExistsValidationError(
+ Table(name, schema="my schema", catalog="my catalog")
+ )
+ assert error.normalized_messages() == {
+ "table": [f"Dataset my catalog.my schema.{name} already exists"]
+ }
+ assert type(error.messages[0]) is str
+
+
[email protected](
+ "kind,limit", [("dataset", 250), ("saved_query", 256), ("tab", 256)]
+)
[email protected]("extra", [0, 1])
+def test_padded_sibling_name_boundary(kind: str, limit: int, extra: int) ->
None:
+ """Persisted names that trim must check the length after trimming."""
+ from superset.mcp_service.sql_lab.schemas import OpenSqlLabRequest
+
+ cases: dict[str, tuple[type[BaseModel], dict[str, Any], str]] = {
+ "dataset": (
+ CreateVirtualDatasetRequest,
+ {"database_id": 1, "sql": "select 1"},
+ "dataset_name",
+ ),
+ "saved_query": (
+ SaveSqlQueryRequest,
+ {"database_id": 1, "sql": "select 1"},
+ "label",
+ ),
+ "tab": (OpenSqlLabRequest, {"database_id": 1}, "title"),
+ }
+ model, payload, field = cases[kind]
+ value = "x" * (limit + extra)
+ if not extra:
+ assert (
+ getattr(model.model_validate({**payload, field: f" {value} "}),
field)
+ == value
+ )
+ else:
+ with pytest.raises(ValidationError) as exc:
+ model.model_validate({**payload, field: f" {value} "})
+ assert exc.value.errors()[0]["type"] == "string_too_long"
+
+
[email protected](
+ "model,payload",
+ [
+ (GenerateDashboardRequest, {"chart_ids": [1]}),
+ (UpdateDashboardRequest, {"identifier": 1}),
+ (DuplicateDashboardRequest, {"dashboard_id": 1}),
+ ],
+)
[email protected]("alias", ["dashboard_title", "title", "name"])
[email protected]("length", [500, 501])
+def test_padded_dashboard_title_boundary(
+ model: type[BaseModel], payload: dict[str, Any], alias: str, length: int
+) -> None:
+ """Dashboard aliases check the trimmed title against the column cap."""
+ if length == 500:
+ assert (
+ model.model_validate(
+ {**payload, alias: " " + "x" * length + " "}
+ ).dashboard_title
+ == "x" * length
+ )
+ else:
+ with pytest.raises(ValidationError) as exc:
+ model.model_validate({**payload, alias: " " + "x" * length + "
"})
+ assert exc.value.errors()[0]["type"] == "string_too_long"
+
+
+def test_padded_dashboard_slug_boundary() -> None:
+ """Update slug normalization precedes its persisted column limit."""
+ request = UpdateDashboardRequest(identifier=1, slug=" " + "x" * 255 + "
")
+ assert request.slug == "x" * 255
+ with pytest.raises(ValidationError):
+ UpdateDashboardRequest(identifier=1, slug=" " + "x" * 256 + " ")
+
+
+_TITLE_SANITIZATION_CASES = [
+ (
+ GenerateChartRequest,
+ {
+ "dataset_id": 1,
+ "config": {"chart_type": "table", "columns": [{"name": "a"}]},
+ "save_chart": False,
+ },
+ "chart_name",
+ ),
+ (
+ GenerateChartRequest,
+ {
+ "dataset_id": 1,
+ "config": {"chart_type": "table", "columns": [{"name": "a"}]},
+ },
+ "chart_name",
+ ),
+ (UpdateChartRequest, {"identifier": 1}, "chart_name"),
+ (GenerateDashboardRequest, {"chart_ids": [1]}, "dashboard_title"),
+ (UpdateDashboardRequest, {"identifier": 1}, "dashboard_title"),
+ (DuplicateDashboardRequest, {"dashboard_id": 1}, "dashboard_title"),
+]
+
+
[email protected]("model,payload,field", _TITLE_SANITIZATION_CASES)
[email protected](
+ "value",
+ [
+ "Users aged <30",
+ "Revenue <$1M",
+ "Love <3 charts",
+ "Users aged <30 & “VIP” 📈",
+ "Users aged >30",
+ "Revenue\u00a0>\u00a0$1M",
+ ],
+)
+def test_literal_less_than_title_round_trip(
+ model: type[BaseModel], payload: dict[str, Any], field: str, value: str
+) -> None:
+ """Non-tag less-than signs survive validation and JSON readback
unchanged."""
+ request = model.model_validate({**payload, field: value})
+ assert getattr(request, field) == value
+ assert request.sanitization_warnings == []
+ assert json.loads(request.model_dump_json())[field] == value
+
+
[email protected]("model,payload,field", _TITLE_SANITIZATION_CASES)
[email protected](
+ "value", ["Weekly <img src=x>KPIs", "Q3 <b>revenue</b> & costs"]
+)
+def test_title_sanitized_once(
+ model: type[BaseModel], payload: dict[str, Any], field: str, value: str
+) -> None:
+ """Persist the single sanitizer result, never a second-pass empty
fallback."""
+ from unittest.mock import patch
+
+ from superset.mcp_service.utils.sanitization import sanitize_user_input
+
+ expected = sanitize_user_input(value, "Title", max_length=500,
allow_empty=True)
+ assert expected
+ with (
+ patch(
+ "superset.mcp_service.utils.sanitization.sanitize_user_input",
+ wraps=sanitize_user_input,
+ ) as sanitize,
+ patch("superset.mcp_service.chart.schemas.sanitize_user_input",
sanitize),
+ patch("superset.mcp_service.dashboard.schemas.sanitize_user_input",
sanitize),
+ ):
+ request = model.model_validate({**payload, field: value})
+ calls = [
+ c
+ for c in sanitize.call_args_list
+ if c.args[1] in ("Chart name", "Dashboard title")
+ ]
+ assert len(calls) == 1
+ assert getattr(request, field) == expected
+ assert len(request.sanitization_warnings) == 1
+
+
[email protected]("model,payload,field", _TITLE_SANITIZATION_CASES)
[email protected](
+ "value", ["<<</b>x", "&lt;</script>click= <b>📈"]
+)
+def test_reconstructed_tag_title_rejected(
+ model: type[BaseModel], payload: dict[str, Any], field: str, value: str
+) -> None:
+ """Encoded tag text is rejected instead of stored or auto-named."""
+ with pytest.raises(ValidationError) as exc:
+ model.model_validate({**payload, field: value})
+ assert exc.value.errors()[0]["loc"] == (field,)
+ assert exc.value.errors()[0]["type"] == "unsafe_text"
+
+
[email protected]("model,payload,field", _TITLE_SANITIZATION_CASES)
+def test_empty_sanitized_title_never_uses_fallback(
+ model: type[BaseModel], payload: dict[str, Any], field: str
+) -> None:
+ """Encoded script-only names retain the existing disallowed-content
reason."""
+ with pytest.raises(ValidationError) as exc:
+ model.model_validate({**payload, field:
"&lt;script>alert(1)</script>"})
+ assert exc.value.errors()[0]["loc"] == (field,)
+ assert exc.value.errors()[0]["type"] == "unsafe_text"
+
+
+_SIBLING_SANITIZATION_CASES = [
+ (ManageDashboardCertificationRequest, {"identifier": 1}, "certified_by"),
+ (
+ ManageDashboardCertificationRequest,
+ {"identifier": 1},
+ "certification_details",
+ ),
+ (BaseChartConfig, {}, "temporal_column"),
+ (ColumnRef, {}, "name"),
+ (ColumnRef, {"name": "amount"}, "label"),
+ (FilterConfig, {"op": "=", "value": "safe"}, "column"),
+ (FilterConfig, {"column": "region", "op": "="}, "value"),
+ (GanttSortByConfig, {}, "column"),
+ (
+ SunburstChartConfig,
+ {
+ "hierarchy": [{"name": "region"}],
+ "metric": {"name": "amount", "aggregate": "SUM"},
+ },
+ "time_range",
+ ),
+ (
+ InteractivePivotChartConfig,
+ {
+ "rows": [{"name": "region"}],
+ "metrics": [{"name": "amount", "aggregate": "SUM"}],
+ },
+ "comparison_period",
+ ),
+ (
+ BulletChartConfig,
+ {"metric": {"name": "amount", "aggregate": "SUM"}},
+ "time_range",
+ ),
+ (
+ BulletChartConfig,
+ {"metric": {"name": "amount", "aggregate": "SUM"}},
+ "range_labels",
+ ),
+ (
+ BulletChartConfig,
+ {"metric": {"name": "amount", "aggregate": "SUM"}},
+ "marker_labels",
+ ),
+ (
+ BulletChartConfig,
+ {"metric": {"name": "amount", "aggregate": "SUM"}},
+ "marker_line_labels",
+ ),
+]
+
+
[email protected]("model,payload,field", _SIBLING_SANITIZATION_CASES)
[email protected](
+ "value", ["Users aged <30", "Users aged >30", "Users\u00a0>30"]
+)
+def test_sibling_fields_preserve_literal_comparison(
+ model: type[BaseModel], payload: dict[str, Any], field: str, value: str
+) -> None:
+ """Every sibling accepts a non-tag comparison and preserves its literal
text."""
+ if model is InteractivePivotChartConfig:
+ payload = {**payload, "comparison_type": "values"}
+ expected = [value] if field.endswith("_labels") else value
+ request = model.model_validate({**payload, field: expected})
+ assert getattr(request, field) == expected
+
+
[email protected]("model,payload,field", _SIBLING_SANITIZATION_CASES)
[email protected](
+ "value", ["<<</b>x", "&lt;</script>click= <b>📈"]
+)
+def test_sibling_fields_reject_reconstructed_tag(
+ model: type[BaseModel], payload: dict[str, Any], field: str, value: str
+) -> None:
+ """Reconstructed tag openers fail with a canonical field and typed
reason."""
+ if model is InteractivePivotChartConfig:
+ payload = {**payload, "comparison_type": "values"}
+ with pytest.raises(ValidationError) as exc:
+ model.model_validate(
+ {**payload, field: [value] if field.endswith("_labels") else value}
+ )
+ assert [(error["type"], error["loc"]) for error in exc.value.errors()] == [
+ ("unsafe_text", (field,))
+ ]
+
+
[email protected](
+ "model,payload,field",
+ [
+ (
+ CreateVirtualDatasetRequest,
+ {"database_id": 1, "sql": "select 1"},
+ "dataset_name",
+ ),
+ (UpdateDatasetRequest, {"dataset_id": 1}, "table_name"),
+ (
+ CreateDatasetMetricRequest,
+ {"dataset_id": 1, "expression": "SUM(x)"},
+ "metric_name",
+ ),
+ (UpdateDatasetMetricRequest, {"dataset_id": 1, "metric": 1},
"metric_name"),
+ (CreateThemeRequest, {"json_data": {}}, "theme_name"),
+ (SaveSqlQueryRequest, {"database_id": 1, "sql": "select 1"}, "label"),
+ (
+ CreateVirtualDatasetRequest,
+ {"database_id": 1, "sql": "select 1", "dataset_name": "ds"},
+ "description",
+ ),
+ (UpdateDatasetRequest, {"dataset_id": 1}, "description"),
+ (
+ SaveSqlQueryRequest,
+ {"database_id": 1, "sql": "select 1", "label": "query"},
+ "description",
+ ),
+ (UpdateDashboardRequest, {"identifier": 1}, "description"),
+ ],
+)
[email protected](
+ "value",
+ [
+ "Users aged <30",
+ "Revenue <$1M",
+ "Love <3 charts",
+ "<<</b>x",
+ "&lt;</script>click= <b>📈",
+ ],
+)
+def test_unsanitized_sibling_names_remain_literal(
+ model: type[BaseModel], payload: dict[str, Any], field: str, value: str
+) -> None:
+ """Non-sanitizer paths retain their existing literal identifier/text
contract."""
+ request = model.model_validate({**payload, field: value})
+ assert getattr(request, field) == value
+
+
[email protected](
+ "value",
+ [
+ "Users aged <30",
+ "Revenue <$1M",
+ "Love <3 charts",
+ "<<</b>x",
+ "&lt;</script>click= <b>📈",
+ ],
+)
+def test_sql_lab_title_remains_literal(value: str) -> None:
+ """SQL Lab tab titles trim whitespace without HTML sanitization."""
+ from superset.mcp_service.sql_lab.schemas import OpenSqlLabRequest
+
+ assert OpenSqlLabRequest(database_id=1, title=value).title == value
+
+
[email protected]
[email protected]("field", ["dashboard_title", "title", "name"])
[email protected](
+ "rejected", ["<script>alert(1)</script>", "Weekly
&am<b></b>p;lt;b&gt;KPIs"]
+)
+async def test_duplicate_dashboard_removed_or_unstable_title_wire_reason(
+ field: str, rejected: str
+) -> None:
+ """Removed or unstable markup is unsafe content, not an empty input."""
+ async with Client(make_title_server()) as client:
+ result = await client.call_tool_mcp(
+ "duplicate_dashboard", {"request": {"dashboard_id": 1, field:
rejected}}
+ )
+ assert result.isError is True
+ assert result.content == [
+ TextContent(
+ type="text",
+ text="Error: Validation error in duplicate_dashboard: "
+ "request.dashboard_title: Contains disallowed content "
+ "(HTML, script or data: URL, event handler, or SQL keyword); "
+ "a 'data:' label must not be followed by a comma",
+ )
+ ]
+
+
[email protected]("model,payload,field", _TITLE_SANITIZATION_CASES)
[email protected]("character", [">", "\u00a0"])
[email protected]("at_limit", [True, False])
+def test_title_length_counts_plain_text(
+ model: type[BaseModel],
+ payload: dict[str, Any],
+ field: str,
+ character: str,
+ at_limit: bool,
+) -> None:
+ """Entity serialization must not inflate persisted titles or reject valid
ones."""
+ limit = 250 if field == "chart_name" else 500
+ value = "x" * (limit - 2 + (not at_limit)) + character + "y"
+ if not at_limit:
+ with pytest.raises(ValidationError) as exc:
+ model.model_validate({**payload, field: value})
+ assert exc.value.errors()[0]["type"] == "string_too_long"
+ return
+ request = model.model_validate({**payload, field: value})
+ assert getattr(request, field) == value
+ assert len(getattr(request, field)) == limit
+ assert request.sanitization_warnings == []
+ assert getattr(model.model_validate(request.model_dump()), field) == value
+
+
+def test_builtin_name_validation_documented_for_administrators() -> None:
+ """Built-in validation guidance belongs with server administration."""
+ from pathlib import Path
+
+ root = Path(__file__).resolve().parents[3]
+ admin = (root / "docs/admin_docs/configuration/mcp-server.mdx").read_text()
+ extensions = (root / "docs/developer_docs/extensions/mcp.md").read_text()
+ for heading in ("Chart and dashboard titles", "Object-name validation"):
+ assert heading in admin
+ assert heading not in extensions
+ assert "Theme names retain their whitespace" in admin
+ assert "length validation applies after trimming" in admin
+
+
+def test_name_contract_changes_have_upgrade_guidance() -> None:
+ """Clients need migration guidance for blank renames and tighter name
caps."""
+ from pathlib import Path
+
+ root = Path(__file__).resolve().parents[3]
+ next_release = (
+ (root / "UPDATING.md").read_text().split("## Next", 1)[1].split("\n##
", 1)[0]
+ )
+ assert (
+ "`update_dashboard` rejects blank or whitespace-only `dashboard_title`"
+ in next_release
+ )
+ assert "chart names and theme names are capped at 250 characters" in
next_release
+ assert "omit the field to keep the stored title" in next_release
+
+
Review Comment:
Fixed in 0c05b084da93e460e38157f057f171f40479d721. I verified the
release-cut example and removed
`test_name_contract_changes_have_upgrade_guidance` as suggested. The upgrade
note remains in UPDATING.md; unit tests no longer require it to stay under `##
Next` forever.
Regression reproduction: I inserted a release heading immediately after `##
Next`, restoring the reviewed test for the before run. The selected suite
failed only `test_name_contract_changes_have_upgrade_guidance` (1 failed, 21
passed). With that test removed and the same release heading retained, it
passed (21 passed). I then restored UPDATING.md unchanged. The PR-added
`tests/unit_tests/mcp_service/test_sanitization_warning_inputs.py::test_update_dashboard_blank_title_wire_reason`,
`test_chart_name_model_column_boundary`, and
`test_theme_name_model_column_boundary` keep pinning the runtime contract;
those 21 cases intentionally pass in both runs. The removed documentation
assertion, not runtime validation, is the release-cut regression. The complete
two touched files pass 1,019 tests.
Siblings checked:
- tests/unit_tests/mcp_service/test_sanitization_warning_inputs.py — fixed
in 0c05b084da93e460e38157f057f171f40479d721
- UPDATING.md:25 — not affected: the migration guidance is retained and may
move under a version heading at release.
- docs/admin_docs/configuration/mcp-server.mdx:2433 — not affected:
permanent administrator guidance does not depend on release headings.
- tests/unit_tests/mcp_service/test_sanitization_warning_inputs.py:792 — not
affected: blank-rename wire tests validate behavior, not documentation
placement.
- tests/unit_tests/mcp_service/test_sanitization_warning_inputs.py:722 — not
affected: chart-name limits are asserted against the persisted model column.
- tests/unit_tests/mcp_service/test_sanitization_warning_inputs.py:773 — not
affected: theme-name limits are asserted against the persisted model column.
Search: `rg -n 'UPDATING.md|## Next' tests/unit_tests` found no other unit
test reading the Next section (other hits are comments/docstrings); `git show
d76b896c9c -- UPDATING.md` confirms the release-heading insertion pattern.
--
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]
