aminghadersohi commented on code in PR #45575:
URL: https://github.com/apache/superset/pull/45575#discussion_r4350925836
##########
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
+ # mark. Preserve literal comparisons like <30 and <$1M, but never decode
+ # a tag opener reconstructed by stripping intervening markup.
+ # 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)
+ return re.sub(r"<(?![A-Za-z/!?])", "<", cleaned).replace("&", "&")
Review Comment:
Fixed in 6143776c9a761201f4633bcf804243f5901094ce. I restore `>` and NBSP
after tag stripping, preserving literal text and length caps without
entity-semicolon filter rejections. The allowlist can intentionally leave other
brackets escaped, so I also enforce the sanitized storage limit for
generate/duplicate dashboard paths without Field caps.
Regressions:
`tests/unit_tests/mcp_service/test_sanitization_warning_inputs.py::test_title_length_counts_plain_text`,
`tests/unit_tests/mcp_service/utils/test_sanitization.py::test_filter_value_preserves_greater_than_and_nbsp`,
and
`tests/unit_tests/mcp_service/test_sanitization_warning_inputs.py::test_sibling_fields_preserve_literal_comparison`
fail with the reviewed sanitizer and pass with this change. The reviewed-head
sanitizer run produced 55 failed / 776 passed. The four
`test_*_literal_less_than_storage_round_trip` tool tests gained `>`/NBSP cases:
8 failed / 12 passed against reviewed source, all pass after. The seven touched
Python files pass in Docker: 1,232 passed (fastmcp 3.4.7 / nh3 0.3.7 / pydantic
2.14.1; manual/llm_compliance deselected).
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.
##########
tests/unit_tests/mcp_service/utils/test_sanitization.py:
##########
@@ -903,3 +904,95 @@ def
test_data_label_followed_by_control_whitespace_is_preserved(
"""Whitespace after a comma-free data: label may be a tab or line break."""
value = f"Substitute data:{separator}Jira support tickets"
assert sanitize(value) == value.replace("\r\n", "\n")
+
+
[email protected](
+ "value",
+ [
+ "Users aged <30",
+ "Revenue <$1M",
+ "Love <3 charts",
+ "5 < 10",
+ "A < B",
+ "A <",
+ "A <> B",
+ ],
+)
+def test_strip_html_tags_preserves_non_tag_less_than(value: str) -> None:
+ """Preserve display text without decoding tag-opening brackets."""
+ assert _strip_html_tags(value) == value.replace(">", ">")
Review Comment:
Fixed in 6143776c9a761201f4633bcf804243f5901094ce.
`tests/unit_tests/mcp_service/utils/test_sanitization.py::test_strip_html_tags_preserves_non_tag_less_than`
asserts exact equality, including `A <> B`, `Users aged >30`, and NBSP
comparisons. Those assertions fail with the reviewed sanitizer and pass after
restoring the safe characters. The full touched Python selection passes in
Docker (1,232 passed); filter, schema boundary, warning, resubmission and
persistence assertions cover the same behavior rather than preserving the old
asymmetry.
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.
##########
docs/developer_docs/extensions/mcp.md:
##########
@@ -28,6 +28,17 @@ under the License.
Model Context Protocol (MCP) integration allows extensions to register custom
AI agent capabilities that integrate seamlessly with Superset's MCP service.
Extensions can provide both **tools** (executable functions) and **prompts**
(interactive guidance) that AI agents can discover and use.
+## Chart and dashboard titles
+
+MCP chart names and dashboard titles preserve literal comparisons such as
+`Users aged <30`, `Revenue <$1M`, and `Love <3 charts`. HTML tags are stripped;
Review Comment:
Fixed in 6143776c9a761201f4633bcf804243f5901094ce, including the follow-up
in this PR. I HTML-escape plain-text fragments using the existing ECharts
helper instead of treating labels as rich HTML. I also found the same
core-filter use in Rose's drilled pie and changed it.
Regression tests:
`superset-frontend/plugins/plugin-chart-echarts/test/Bullet/transformProps.test.ts::escapes
literal comparison text in every Bullet tooltip`,
`superset-frontend/plugins/plugin-chart-echarts/test/Rose/transformProps.test.ts::escapes
literal series text in Rose tooltips (rich=%s)`, and
`superset-frontend/plugins/plugin-chart-echarts/test/Rose/EchartsRose.test.tsx::drilled
Rose tooltips preserve literal series and period text`. The three suites
produce 5 failed / 27 passed with reviewed source, then 32 passed after the
change. They inspect rendered DOM text and ensure label markup stays inert
while intended tooltip formatting survives. I used the checkout's core source
through a Jest mapper with cached dependencies. The rendering fix covers all
input entry points, including 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.
--
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]
