aminghadersohi commented on code in PR #45575:
URL: https://github.com/apache/superset/pull/45575#discussion_r4350928022
##########
superset/mcp_service/utils/sanitization.py:
##########
@@ -70,11 +70,29 @@ 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("&", "&")
+ # HTML tag-open states require an ASCII letter, slash, bang or question
Review Comment:
Fixed in 6143776c9a761201f4633bcf804243f5901094ce. I adopted the
comparison-character allowlist.
`tests/unit_tests/mcp_service/utils/test_sanitization.py::test_strip_html_tags_keeps_casefolding_tag_openers_escaped`
fails for both U+017F and U+212A with reviewed source and passes after the
change;
`tests/unit_tests/mcp_service/utils/test_sanitization.py::test_strip_html_tags_preserves_non_tag_less_than`
retains the literal-comparison contract.
`tests/unit_tests/mcp_service/test_sanitization_warning_inputs.py::test_escaped_title_still_fits_model_column`
additionally caught generate/duplicate dashboard overflow from intentionally
retained escaping (2 failed before the storage-cap guard, all pass after).
Docker: 1,232 touched Python tests passed. The tooltip sibling fix escapes the
corresponding display fragments.
Siblings checked:
- superset/mcp_service/chart/schemas.py:6369 — fixed in
6143776c9a761201f4633bcf804243f5901094ce (generate/save=false/update chart
names).
- superset/mcp_service/dashboard/schemas.py:884 — fixed in
6143776c9a761201f4633bcf804243f5901094ce (generate titles).
- superset/mcp_service/dashboard/schemas.py:1050 — fixed in
6143776c9a761201f4633bcf804243f5901094ce (update titles).
- superset/mcp_service/dashboard/schemas.py:1531 — fixed in
6143776c9a761201f4633bcf804243f5901094ce (duplicate titles).
- superset/mcp_service/dashboard/schemas.py:1393 — fixed in
6143776c9a761201f4633bcf804243f5901094ce (certified_by).
- superset/mcp_service/dashboard/schemas.py:1414 — fixed in
6143776c9a761201f4633bcf804243f5901094ce (certification_details).
- superset/mcp_service/chart/schemas.py:879 — fixed in
6143776c9a761201f4633bcf804243f5901094ce (temporal columns).
- superset/mcp_service/chart/schemas.py:1003 — fixed in
6143776c9a761201f4633bcf804243f5901094ce (column names).
- superset/mcp_service/chart/schemas.py:1011 — fixed in
6143776c9a761201f4633bcf804243f5901094ce (column labels).
- superset/mcp_service/chart/schemas.py:1132 — fixed in
6143776c9a761201f4633bcf804243f5901094ce (filter columns).
- superset/mcp_service/chart/schemas.py:1143 — fixed in
6143776c9a761201f4633bcf804243f5901094ce (scalar/list filter values).
- superset/mcp_service/chart/schemas.py:1211 — fixed in
6143776c9a761201f4633bcf804243f5901094ce (Gantt columns).
- superset/mcp_service/chart/schemas.py:2761 — fixed in
6143776c9a761201f4633bcf804243f5901094ce (Sunburst ranges).
- superset/mcp_service/chart/schemas.py:3155 — fixed in
6143776c9a761201f4633bcf804243f5901094ce (pivot comparison periods).
- superset/mcp_service/chart/schemas.py:4628 — fixed in
6143776c9a761201f4633bcf804243f5901094ce (Bullet range/marker/marker-line
labels).
- superset/mcp_service/chart/schemas.py:4637 — fixed in
6143776c9a761201f4633bcf804243f5901094ce (Bullet ranges).
- superset/mcp_service/chart/tool/generate_chart.py:255 — fixed in
6143776c9a761201f4633bcf804243f5901094ce (storage/readback regression; schema
revalidation carries the sanitized title).
- superset/mcp_service/chart/tool/update_chart.py — fixed in
6143776c9a761201f4633bcf804243f5901094ce (storage/readback regression).
- superset/mcp_service/dashboard/tool/generate_dashboard.py — fixed in
6143776c9a761201f4633bcf804243f5901094ce (storage/readback regression).
- superset/mcp_service/dashboard/tool/update_dashboard.py — fixed in
6143776c9a761201f4633bcf804243f5901094ce (storage/readback regression).
- superset/mcp_service/chart/schemas.py:6435 — not affected: Explore links
and update_chart_preview have no title field; their config uses the shared
field validators above.
- superset/mcp_service/dataset/schemas.py — not affected: object
names/descriptions do not use this HTML sanitizer; literal-text controls remain
covered.
- superset/mcp_service/theme/schemas.py — not affected: blank/length
validation without HTML stripping.
- superset/mcp_service/sql_lab/schemas.py — not affected: label/tab trimming
without HTML stripping.
- superset/mcp_service/utils/sanitization.py:sanitize_sql_expression — not
affected: separate SQL canonicalization, with existing comparison-operator
tests.
- superset/mcp_service/utils/error_builder.py:54 — not affected: differently
named diagnostic scrubber, not a caller of the shared sanitizer.
- superset/charts/schemas.py — not affected: REST does not call the MCP
sanitizer; the tooltip rendering changes also cover raw text originating in
REST/Explore.
Siblings checked:
-
superset-frontend/plugins/plugin-chart-echarts/src/Bullet/transformProps.ts:229
— fixed in 6143776c9a761201f4633bcf804243f5901094ce (group prefix, metric,
containing-range label and formatted value).
-
superset-frontend/plugins/plugin-chart-echarts/src/Bullet/transformProps.ts:261
— fixed in 6143776c9a761201f4633bcf804243f5901094ce (range threshold tooltip).
-
superset-frontend/plugins/plugin-chart-echarts/src/Bullet/transformProps.ts:278
— fixed in 6143776c9a761201f4633bcf804243f5901094ce (marker tooltip).
-
superset-frontend/plugins/plugin-chart-echarts/src/Rose/transformProps.ts:136 —
fixed in 6143776c9a761201f4633bcf804243f5901094ce (rich series tooltip, period
text and formatted value).
-
superset-frontend/plugins/plugin-chart-echarts/src/Rose/transformProps.ts:183 —
fixed in 6143776c9a761201f4633bcf804243f5901094ce (plain series tooltip).
- superset-frontend/plugins/plugin-chart-echarts/src/Rose/EchartsRose.tsx:81
— fixed in 6143776c9a761201f4633bcf804243f5901094ce (drill-down pie tooltip).
- superset-frontend/plugins/plugin-chart-echarts/src/utils/series.ts:1043 —
not affected: this similarly named sanitizeHtml already HTML-escapes with
ECharts encodeHTML; reused for these fixes.
-
superset-frontend/plugins/plugin-chart-echarts/src/BoxPlot/transformProps.ts:149
— not affected: both tooltip paths import the escaping helper from
utils/series, not the core HTML filter.
-
superset-frontend/plugins/plugin-chart-echarts/src/Graph/transformProps.ts:390
— not affected: source/target labels already use the escaping helper.
-
superset-frontend/plugins/plugin-chart-echarts/src/Pie/transformProps.ts:75 —
not affected: raw names already use the escaping helper before tooltipHtml.
-
superset-frontend/plugins/plugin-chart-echarts/src/Funnel/transformProps.ts:73
— not affected: raw names already use the escaping helper before tooltipHtml.
- superset-frontend/plugins/plugin-chart-echarts/src/Radar/utils.ts:67 — not
affected: label/value fragments import the escaping helper.
- superset-frontend/plugins/plugin-chart-echarts/src/utils/forecast.ts:124 —
not affected: series labels import the escaping helper.
-
superset-frontend/plugins/plugin-chart-echarts/src/utils/eChartOptionsSchema.ts:71
— not affected: sanitizes explicitly HTML-valued option strings, not the
plain-text label fragments changed here.
- superset-frontend/packages/superset-ui-core/src/utils/tooltip.ts:78 — not
affected: intentionally accepts rich HTML; the Pie/Funnel/Graph callers above
escape plain text before calling it.
-
superset-frontend/plugins/plugin-chart-echarts/src/Bullet/transformProps.ts —
not affected: canvas axis/legend/mark-line labels are not HTML sinks; the
common transformer covers MCP, REST and Explore inputs.
##########
superset/mcp_service/dashboard/schemas.py:
##########
@@ -1065,17 +1069,17 @@ def _detect_dashboard_title_sanitization(cls, data:
Any) -> Any:
@field_validator("dashboard_title")
@classmethod
- def sanitize_dashboard_title(cls, v: str | None) -> str | None:
- """Sanitize dashboard title to prevent XSS."""
- if v is None or v == "":
+ def normalize_blank_dashboard_title(cls, v: str | None) -> str | None:
+ """Reject blank updates; nonblank titles are sanitized above."""
+ if v is None:
return v
- return sanitize_user_input(
- v, "Dashboard title", max_length=500, allow_empty=True
- )
+ if not v.strip():
+ raise PydanticCustomError("empty_text", "dashboard_title cannot be
empty")
Review Comment:
Fixed in 6143776c9a761201f4633bcf804243f5901094ce. I added the `## Next`
migration note covering explicit blank dashboard renames, omission to retain
the stored title, chart names dropping from the published 255 limit to 250, and
the new 250-character theme-name cap.
Regression:
`tests/unit_tests/mcp_service/test_sanitization_warning_inputs.py::test_name_contract_changes_have_upgrade_guidance`
fails with the reviewed UPDATING.md and passes after this change (the two
documentation tests failed before, both pass in the 1,232-test Docker run).
Runtime contracts remain pinned by
`test_update_dashboard_blank_title_wire_reason`,
`test_chart_name_model_column_boundary`, and
`test_theme_name_model_column_boundary` in the same PR test file.
Siblings checked:
- UPDATING.md:28 — fixed in 6143776c9a761201f4633bcf804243f5901094ce.
- docs/admin_docs/configuration/mcp-server.mdx:2428 — fixed in
6143776c9a761201f4633bcf804243f5901094ce (blank rename and all name/length
guidance together).
- docs/developer_docs/extensions/mcp.md — fixed in
6143776c9a761201f4633bcf804243f5901094ce (built-in validation text moved to
administration).
- superset/mcp_service/chart/schemas.py:6330 — not affected: existing
runtime 250 cap is documented, not changed again.
- superset/mcp_service/dashboard/schemas.py:1072 — not affected: existing
explicit-blank rejection is documented.
- superset/mcp_service/theme/schemas.py:143 — not affected: existing 250 cap
is documented.
##########
docs/developer_docs/extensions/mcp.md:
##########
@@ -587,3 +598,17 @@ governance concern from layout/theme/metadata edits.
- **[Development](./development.md)** - Project structure, APIs, and dev
workflow
- **[Security](./security.md)** - Security best practices for extensions
+
+## Object-name validation
Review Comment:
Fixed in 6143776c9a761201f4633bcf804243f5901094ce. I moved both “Chart and
dashboard titles” and “Object-name validation” to the admin MCP server
documentation, retaining theme/saved-query/SQL Lab/slug caps,
trim-before-length and the theme whitespace exception. Neither section remains
after “Next Steps” on the extension page.
Regression:
`tests/unit_tests/mcp_service/test_sanitization_warning_inputs.py::test_builtin_name_validation_documented_for_administrators`
fails with reviewed documentation and passes after the move (both
documentation regressions pass in the 1,232-test Docker run).
Siblings checked:
- docs/developer_docs/extensions/mcp.md — fixed in
6143776c9a761201f4633bcf804243f5901094ce (both built-in validation sections
removed).
- docs/admin_docs/configuration/mcp-server.mdx:2428 — fixed in
6143776c9a761201f4633bcf804243f5901094ce (title-error guidance retained).
- docs/admin_docs/configuration/mcp-server.mdx:2447 — fixed in
6143776c9a761201f4633bcf804243f5901094ce (literal title handling moved here).
- docs/admin_docs/configuration/mcp-server.mdx:2458 — fixed in
6143776c9a761201f4633bcf804243f5901094ce (object caps and trimming moved here).
- docs/developer_docs/extensions/mcp.md:31 — not affected: pre-existing
chart-dataset rebinding guidance is outside these two validation sections and
unchanged.
--
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]
