rebenitez1802 commented on code in PR #45575:
URL: https://github.com/apache/superset/pull/45575#discussion_r4350189772
##########
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:
**π’ Low β Consider an allowlist for the `<` restore (hardening)**
This denylist matches the HTML tokenizer's tag-open rule for the value as
stored, but it emits a raw `<` before non-ASCII letters that case-fold to ASCII:
- `Q3 <ΕΏcript src=//e.example/x.js` (U+017F) is stored as-is and becomes
`<SCRIPT SRC=β¦` under `.toUpperCase()`.
- js-xss lowercases a KELVIN SIGN (U+212A) tag name, so `x <\u212Abd
title="y` in a `tooltipHtml` cell becomes a real `<kbd>` that swallows the rest
of the row.
Base kept both escaped. I didn't find a sink that executes script this way,
and REST already stores raw titles, so this is defense in depth only. An
allowlist keeps every literal-comparison case (`<30`, `<$1M`, `<3`, `< 10`,
`<=`, `<-`, `A <> B`) and passes the same tests:
```python
re.sub(r"<(?=[\d\s$β¬Β£Β₯%+=.,:;)\]&-]|$)", "<", cleaned)
```
(`&` covers a following escaped character, such as the `>` in `A <> B`.)
##########
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:
**π’ Low β Literal `<` now truncates Bullet/Rose tooltips (follow-up)**
These literals are now stored raw, but two ECharts tooltips run plain text
through `sanitizeHtml` (js-xss with `stripIgnoreTag`) instead of escaping it:
the Bullet range-label tooltip
(`plugin-chart-echarts/src/Bullet/transformProps.ts:231`) and the Rose series
tooltips (`Rose/transformProps.ts:136,183`). A `range_labels` value of `Users
<30` renders as `Range: Users ` and a series named `Love <3 charts` as `Love `
(checked in jsdom with Superset's js-xss config); base's escaped `<30`
happened to display correctly there. Labels typed in Explore already hit this,
so not a blocker here, but worth a follow-up to HTML-escape those text
fragments instead of filtering them.
##########
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:
**π’ Low β Add an UPDATING.md entry for the MCP contract changes**
Rejecting blank renames is a behavior change: base let `""` through and
`update_dashboard` persisted it (it writes any non-null title). Together with
`chart_name`'s published `maxLength` dropping from 255 to 250 and the new 250
cap on `theme_name`, MCP clients see a stricter contract. UPDATING.md already
logs comparable MCP changes under `## Next`, e.g.:
```md
- MCP `update_dashboard` rejects blank or whitespace-only `dashboard_title`
values with "Text must not be empty"; omit the field to keep the stored
title.
MCP chart names and theme names are capped at 250 characters, matching
their
model columns.
```
##########
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:
**π’ Low β Built-in tool docs land on the extension-authoring page**
This section sits after "Next Steps", and together with "Chart and dashboard
titles" (line 31) it documents built-in tool behavior on the page for extension
authors. Most of it is already in the new admin section
(`docs/admin_docs/configuration/mcp-server.mdx`, "Title validation errors").
Consider moving the remaining details (theme, saved-query, SQL Lab and slug
caps; trim-before-length) there, or at least placing this section above "Next
Steps".
##########
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:
Companion to the `_strip_html_tags` suggestion: this assertion pins the
`<`/`>` asymmetry. With both applied, the touched MCP test files pass locally
(1,742 passed).
```suggestion
"A <> B",
"Users aged >30",
"Revenue\u00a0>\u00a0$1M",
],
)
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
```
##########
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:
**π‘ Medium β `>`/NBSP still escaped, and length caps now measure the escaped
value**
This restores `<` but still emits `>` as `>` and U+00A0 as ` `.
Since the before-validators now write the sanitized value back
(`chart/schemas.py:6381`, `dashboard/schemas.py:1061`), `Field(max_length)`
measures the escaped string:
- `"x" * 247 + ">"` (248 chars) fails `generate_chart` (even with
`save_chart=False`) and `update_chart` with `string_too_long`; base accepted it
(and stored 251 chars against `String(250)`).
- `update_dashboard` rejects a 500-char title containing `>` the same way,
while `generate_dashboard`/`duplicate_dashboard` (no Field cap) store it as 503
chars against `String(500)`.
- Literal comparisons are one-sided: `Users aged >30` is stored as `Users
aged >30` with a "modified during sanitization" warning, and filter value
`>5` is rejected (the `;` in `>` trips the shell-character check) while `<5`
passes.
Neither character can open a tag, and restoring them means sanitized output
is never longer than the input:
```suggestion
# ">" and NBSP can never open a tag; restore them so plain text such as
# "Revenue > $1M" is stored as typed and never outgrows the length caps.
return (
re.sub(r"<(?![A-Za-z/!?])", "<", cleaned)
.replace(">", ">")
.replace(" ", "\u00a0")
.replace("&", "&")
)
```
Paired test update on `test_strip_html_tags_preserves_non_tag_less_than` in
a separate comment.
--
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]
