aminghadersohi commented on code in PR #45575:
URL: https://github.com/apache/superset/pull/45575#discussion_r4343902099
##########
tests/unit_tests/mcp_service/utils/test_sanitization.py:
##########
@@ -903,3 +903,69 @@ 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(">", ">")
+
+
[email protected](
+ "value",
+ [
+ "<</b>script>alert(1)",
+ "<​script>alert(1)",
+ "<</b>img src=x onerror=alert(1)",
+ ],
+)
+def test_strip_html_tags_does_not_reconstruct_tag_openers(value: str) -> None:
+ """Removed tags or invisible characters must not reveal raw markup."""
+ result = _strip_html_tags(value)
+ assert "<script" not in result
+ assert "<img" not in result
+
+
[email protected](
+ "value",
+ [
+ "<<b></b>script>alert(1)",
+ "<​script>alert(1)",
+ "&lt;</script>click= <b>📈",
+ "<<</b>x",
+ ],
+)
+def test_sanitize_user_input_rejects_reconstructed_tag_text(value: str) ->
None:
+ """Tag-shaped text left after stripping is rejected, never stored."""
+ with pytest.raises(ValueError, match="malicious HTML"):
+ sanitize_user_input(value, "Title")
+
+
[email protected](
+ "value",
+ [
+ "Users aged <30",
+ "Revenue <$1M",
+ "A <> B",
+ "a << b",
+ "Weekly <img src=x>KPIs",
+ "Q3 <b>revenue</b> & costs",
+ "<3 charts",
Review Comment:
Fixed in 445b90b267. I extended the existing rejection policy: text that
rebuilds entities and would change on a second HTML-stripping pass is rejected
as unsafe_text rather than stored.
`tests/unit_tests/mcp_service/utils/test_sanitization.py::test_reject_entities_reconstructed_by_removal`
includes the exact example, hex or named entity fragments, and Unicode removal
across the text sanitizer, warning wrapper, and filter sanitizer. The new tests
reproduced 35 failures on the previous head across unit and wire cases; the
updated sanitizer/wire modules pass 765 tests. The shared title and
sibling-field wire tests also cover the rebuilt-entity input.
Siblings checked:
- superset/mcp_service/utils/sanitization.py:326 — fixed in 445b90b267
- superset/mcp_service/utils/sanitization.py:387 — fixed in 445b90b267
- superset/mcp_service/utils/sanitization.py:237 — fixed in 445b90b267
- superset/mcp_service/dashboard/schemas.py:884 — fixed in 445b90b267
- superset/mcp_service/dashboard/schemas.py:1050 — fixed in 445b90b267
- superset/mcp_service/dashboard/schemas.py:1531 — fixed in 445b90b267
- superset/mcp_service/dashboard/schemas.py:1393 — fixed in 445b90b267
- superset/mcp_service/dashboard/schemas.py:1414 — fixed in 445b90b267
- superset/mcp_service/chart/schemas.py:879 — fixed in 445b90b267
- superset/mcp_service/chart/schemas.py:1003 — fixed in 445b90b267
- superset/mcp_service/chart/schemas.py:1011 — fixed in 445b90b267
- superset/mcp_service/chart/schemas.py:1132 — fixed in 445b90b267
- superset/mcp_service/chart/schemas.py:1143 — fixed in 445b90b267
- superset/mcp_service/chart/schemas.py:1211 — fixed in 445b90b267
- superset/mcp_service/chart/schemas.py:2761 — fixed in 445b90b267
- superset/mcp_service/chart/schemas.py:3155 — fixed in 445b90b267
- superset/mcp_service/chart/schemas.py:4628 — fixed in 445b90b267
- superset/mcp_service/chart/schemas.py:4637 — fixed in 445b90b267
- superset/mcp_service/chart/schemas.py:6369 — fixed in 445b90b267
- superset/mcp_service/utils/sanitization.py:443 — not affected: SQL
expressions reject HTML constructs instead of stripping tags; they do not use
the changed HTML-removal path.
- superset/mcp_service/utils/error_builder.py:54 — not affected: independent
error-context sanitizer, not a persisted input or a caller of _strip_html_tags.
##########
docs/admin_docs/configuration/mcp-server.mdx:
##########
@@ -2424,3 +2424,18 @@ Context names are returned verbatim per the Tool Result
Value Contract.
"Did you mean" column candidates also preserve dataset spelling, including
quotes and ampersands, with a 200-character bound per candidate. Use
`get_dataset_info` for the complete schema.
+
+### Title validation errors
+
+Rejected MCP titles report a field-specific reason without echoing the input.
+Titles exceeding 500 characters for dashboards or 250 characters for charts
+report "String exceeds the allowed maximum length". `duplicate_dashboard`
+requires a non-blank title and reports "Text must not be empty" when no text
Review Comment:
Documentation corrected in 445b90b267: blank/whitespace-only input reports
empty_text, while stripped-only script content reports unsafe_text.
`tests/unit_tests/mcp_service/test_sanitization_warning_inputs.py::test_duplicate_dashboard_removed_or_unstable_title_wire_reason`
pins the full wire message for all three aliases. The script-only cases
preserve existing behavior (they also pass before this documentation
correction); its rebuilt-entity cases fail on the previous head and pass with
this commit.
Siblings checked:
- docs/admin_docs/configuration/mcp-server.mdx:2433 — fixed in 445b90b267
- docs/developer_docs/extensions/mcp.md:603 — not affected: the blank-name
paragraph describes input blankness, not content stripped by HTML sanitization.
- superset/mcp_service/dashboard/schemas.py:1531 — not affected:
duplicate_dashboard already distinguishes originally blank input from
stripped-only content; no runtime reclassification needed.
- superset/mcp_service/dashboard/schemas.py:884 — not affected:
generate_dashboard rejects stripped-only content as unsafe_text; its optional
blank-input behavior is intentionally unchanged.
- superset/mcp_service/dashboard/schemas.py:1050 — not affected:
update_dashboard also rejects stripped-only content as unsafe_text.
- superset/mcp_service/chart/schemas.py:6369 — not affected: shared
chart-name validation rejects stripped-only content as unsafe_text for both
creation and updates.
--
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]
