sadpandajoe commented on code in PR #45575:
URL: https://github.com/apache/superset/pull/45575#discussion_r4342213504
##########
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:
The resubmission-stability cases don't cover entities rebuilt by tag
removal. With the head sanitizer, `Weekly &am<b></b>p;lt;b&gt;KPIs` is
stored as `Weekly &lt;b>KPIs`, and sending that stored title back
sanitizes it to `Weekly KPIs`, so a round-trip silently changes the title. Is
that intended, or should this input be added here and the single pass made
stable for it?
##########
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:
A `duplicate_dashboard` title such as `<script>alert(1)</script>` is
stripped to nothing and rejected through the plain `ValueError` path, so
clients get "Contains disallowed content" (`unsafe_text`), not "Text must not
be empty". Only originally blank/whitespace titles reach `empty_text`, so a
client following this paragraph would misclassify the stripped-only case.
Should this sentence say "blank or whitespace-only" and name the stripped-only
case separately?
--
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]
