You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by ju...@apache.org on 2026/08/07 18:07:05 UTC
(superset) 02/04: fix(mcp): surface XSS sanitization in chart/dashboard names instead of silently stripping (#39491)
This is an automated email from the ASF dual-hosted git repository.
justinpark pushed a commit to branch 6.2
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 036edfc869cc8a4513385e7168e4f29c9ad6bb35
Author: Enzo Martellucci <52...@users.noreply.github.com>
AuthorDate: Fri Apr 24 14:59:20 2026 +0200
fix(mcp): surface XSS sanitization in chart/dashboard names instead of silently stripping (#39491)
(cherry picked from commit d7941ccfec22ff15fcd39b999c4c4768559f5f79)
---
superset-frontend/src/features/home/Menu.tsx | 7 +-
superset-frontend/src/views/CRUD/hooks.ts | 2 +-
superset/mcp_service/chart/schemas.py | 49 ++++++++++++++
superset/mcp_service/chart/tool/generate_chart.py | 9 ++-
superset/mcp_service/dashboard/schemas.py | 79 +++++++++++++++++++---
.../dashboard/tool/generate_dashboard.py | 11 ++-
superset/mcp_service/utils/sanitization.py | 28 ++++++++
.../mcp_service/chart/test_chart_schemas.py | 68 +++++++++++++++++++
.../dashboard/test_dashboard_schemas.py | 64 ++++++++++++++++++
9 files changed, 304 insertions(+), 13 deletions(-)
diff --git a/superset-frontend/src/features/home/Menu.tsx b/superset-frontend/src/features/home/Menu.tsx
index ca05bfec786..a945cab58d0 100644
--- a/superset-frontend/src/features/home/Menu.tsx
+++ b/superset-frontend/src/features/home/Menu.tsx
@@ -342,7 +342,12 @@ export function Menu({
return <>{link}</>;
};
return (
- <StyledHeader className="top" id="main-menu" role="navigation">
+ <StyledHeader
+ className="top"
+ id="main-menu"
+ role="navigation"
+ aria-label={t('Main navigation')}
+ >
<StyledRow>
<StyledCol md={16} xs={24}>
<Tooltip
diff --git a/superset-frontend/src/views/CRUD/hooks.ts b/superset-frontend/src/views/CRUD/hooks.ts
index dc705c1da72..fa69b584070 100644
--- a/superset-frontend/src/views/CRUD/hooks.ts
+++ b/superset-frontend/src/views/CRUD/hooks.ts
@@ -142,7 +142,7 @@ export function useListViewResource<D extends object = any>(
return false;
}
- return Boolean(state.permissions.find(p => p === perm));
+ return Boolean(state.permissions.some(p => p === perm));
},
[state.permissions],
);
diff --git a/superset/mcp_service/chart/schemas.py b/superset/mcp_service/chart/schemas.py
index 8e5e13d0c44..865fd74e2ac 100644
--- a/superset/mcp_service/chart/schemas.py
+++ b/superset/mcp_service/chart/schemas.py
@@ -59,6 +59,7 @@ from superset.mcp_service.system.schemas import (
from superset.mcp_service.utils.sanitization import (
sanitize_filter_value,
sanitize_user_input,
+ sanitize_user_input_with_changes,
)
@@ -1340,12 +1341,60 @@ class GenerateChartRequest(QueryCacheControl):
preview_formats: List[Literal["url", "ascii", "vega_lite", "table"]] = Field(
default_factory=lambda: ["url"],
)
+ sanitization_warnings: List[str] = Field(
+ default_factory=list,
+ description=(
+ "Internal: warnings emitted when user input was altered by "
+ "sanitization. Populated by the ``mode='before'`` validator "
+ "before chart_name is rewritten, so the tool can surface a "
+ "notice to the caller instead of silently dropping content."
+ ),
+ )
@field_validator("config", mode="before")
@classmethod
def coerce_config(cls, v: Any) -> Dict[str, Any]:
return _coerce_config_to_dict(v)
+ @model_validator(mode="before")
+ @classmethod
+ def _detect_chart_name_sanitization(cls, data: Any) -> Any:
+ """Record a warning when chart_name sanitization alters user input.
+
+ Runs before the ``chart_name`` field validator strips HTML, so we
+ can compare the caller's raw input against the sanitizer output
+ and tell the tool whether to notify the caller. Empty-after-
+ sanitization is rejected with a ValueError so the caller gets a
+ clear error instead of a silently auto-generated name.
+
+ ``sanitization_warnings`` is a server-only field — any value the
+ caller supplied is discarded here so the tool cannot be tricked
+ into echoing attacker-controlled text back through the response.
+ """
+ if not isinstance(data, dict):
+ return data
+ data["sanitization_warnings"] = []
+ raw = data.get("chart_name")
+ if not isinstance(raw, str) or not raw.strip():
+ return data
+ sanitized, was_modified = sanitize_user_input_with_changes(
+ raw, "Chart name", max_length=255, allow_empty=True
+ )
+ if was_modified and not sanitized:
+ raise ValueError(
+ "chart_name contained only disallowed content "
+ "(HTML/script/URL schemes) and was removed entirely by "
+ "sanitization. Provide a chart_name with plain text, or "
+ "omit it to auto-generate one."
+ )
+ if was_modified:
+ data["sanitization_warnings"].append(
+ "chart_name was modified during sanitization to remove "
+ "potentially unsafe content; the stored name differs "
+ "from the input."
+ )
+ return data
+
@field_validator("chart_name")
@classmethod
def sanitize_chart_name(cls, v: str | None) -> str | None:
diff --git a/superset/mcp_service/chart/tool/generate_chart.py b/superset/mcp_service/chart/tool/generate_chart.py
index 494af9b5614..b41ccfc5632 100644
--- a/superset/mcp_service/chart/tool/generate_chart.py
+++ b/superset/mcp_service/chart/tool/generate_chart.py
@@ -230,6 +230,11 @@ async def generate_chart( # noqa: C901
# Track runtime warnings to include in response
runtime_warnings: list[str] = []
+ # Surface warnings captured during pydantic validation (e.g. chart_name
+ # sanitization) so callers know when their input was altered.
+ sanitization_warnings: list[str] = list(
+ getattr(request, "sanitization_warnings", []) or []
+ )
try:
# Run comprehensive validation pipeline
@@ -815,8 +820,8 @@ async def generate_chart( # noqa: C901
else {},
"performance": performance.model_dump() if performance else None,
"accessibility": accessibility.model_dump() if accessibility else None,
- # Combined runtime and response warnings
- "warnings": runtime_warnings + response_warnings,
+ # Combined runtime, response, and sanitization warnings
+ "warnings": sanitization_warnings + runtime_warnings + response_warnings,
"success": True,
"schema_version": "2.0",
"api_version": "v1",
diff --git a/superset/mcp_service/dashboard/schemas.py b/superset/mcp_service/dashboard/schemas.py
index 6973245ad64..83087e66c4d 100644
--- a/superset/mcp_service/dashboard/schemas.py
+++ b/superset/mcp_service/dashboard/schemas.py
@@ -94,8 +94,8 @@ from superset.mcp_service.system.schemas import (
UserInfo,
)
from superset.mcp_service.utils.sanitization import (
- _remove_dangerous_unicode,
- _strip_html_tags,
+ sanitize_user_input,
+ sanitize_user_input_with_changes,
)
from superset.utils.json import loads as json_loads
@@ -521,16 +521,71 @@ class GenerateDashboardRequest(BaseModel):
published: bool = Field(
default=False, description="Whether to publish the dashboard"
)
+ sanitization_warnings: List[str] = Field(
+ default_factory=list,
+ description=(
+ "Internal: warnings emitted when user input was altered by "
+ "sanitization. Populated by the ``mode='before'`` validator "
+ "before dashboard_title is rewritten, so the tool can surface "
+ "a notice to the caller instead of silently dropping content."
+ ),
+ )
+
+ @model_validator(mode="before")
+ @classmethod
+ def _detect_dashboard_title_sanitization(cls, data: Any) -> Any:
+ """Reject empty-after-sanitization titles and warn on partial strip.
+
+ Runs before the ``dashboard_title`` field validator rewrites the
+ value. If the caller supplied a non-empty title and sanitization
+ would strip it entirely (XSS-only content), we raise so the caller
+ gets a clear error instead of a blank-titled dashboard. When the
+ sanitizer only trims part of the title, we record a warning the
+ tool can return alongside the successful result.
+
+ ``sanitization_warnings`` is a server-only field — any value the
+ caller supplied is discarded here so the tool cannot be tricked
+ into echoing attacker-controlled text back through the response.
+ """
+ if not isinstance(data, dict):
+ return data
+ data["sanitization_warnings"] = []
+ raw = data.get("dashboard_title")
+ if not isinstance(raw, str) or not raw.strip():
+ return data
+ sanitized, was_modified = sanitize_user_input_with_changes(
+ raw, "Dashboard title", max_length=500, allow_empty=True
+ )
+ if was_modified and not sanitized:
+ raise ValueError(
+ "dashboard_title contained only disallowed content "
+ "(HTML/script/URL schemes) and was removed entirely by "
+ "sanitization. Provide a dashboard_title with plain text, "
+ "or omit it to auto-generate one from chart names."
+ )
+ if was_modified:
+ data["sanitization_warnings"].append(
+ "dashboard_title was modified during sanitization to "
+ "remove potentially unsafe content; the stored title "
+ "differs from the input."
+ )
+ return data
@field_validator("dashboard_title")
@classmethod
def sanitize_dashboard_title(cls, v: str | None) -> str | None:
- """Strip HTML tags from dashboard title to prevent XSS."""
- if v is None:
- return None
- v = _strip_html_tags(v.strip())
- v = _remove_dangerous_unicode(v)
- return v
+ """Sanitize dashboard title to prevent XSS.
+
+ Preserves an explicit empty string (caller-provided ``""``) rather
+ than collapsing it to ``None``, since the tool treats ``None`` as
+ "auto-generate a title from charts" but an explicit empty string
+ as an intentional blank title.
+ """
+ if v is None or v == "":
+ return v
+ return sanitize_user_input(
+ v, "Dashboard title", max_length=500, allow_empty=True
+ )
class GenerateDashboardResponse(BaseModel):
@@ -541,6 +596,14 @@ class GenerateDashboardResponse(BaseModel):
)
dashboard_url: str | None = Field(None, description="URL to view the dashboard")
error: str | None = Field(None, description="Error message, if creation failed")
+ warnings: List[str] = Field(
+ default_factory=list,
+ description=(
+ "Non-fatal advisory messages about the created dashboard — "
+ "for example, that the supplied title was altered by "
+ "sanitization."
+ ),
+ )
def _parse_json_metadata(json_metadata_str: str | None) -> Dict[str, Any] | None:
diff --git a/superset/mcp_service/dashboard/tool/generate_dashboard.py b/superset/mcp_service/dashboard/tool/generate_dashboard.py
index aa1bb900a31..52056b3ca12 100644
--- a/superset/mcp_service/dashboard/tool/generate_dashboard.py
+++ b/superset/mcp_service/dashboard/tool/generate_dashboard.py
@@ -204,6 +204,11 @@ def generate_dashboard( # noqa: C901
from pydantic import ValidationError
from sqlalchemy.exc import SQLAlchemyError
+ # Advisory messages (e.g. title sanitization) surfaced to the caller
+ # alongside the created dashboard so they can tell when their input
+ # was altered.
+ sanitization_warnings = list(getattr(request, "sanitization_warnings", []) or [])
+
try:
# Get chart objects from IDs (required for SQLAlchemy relationships)
from superset import db
@@ -393,6 +398,7 @@ def generate_dashboard( # noqa: C901
),
dashboard_url=dashboard_url,
error=None,
+ warnings=sanitization_warnings,
)
# Convert to our response format
@@ -440,7 +446,10 @@ def generate_dashboard( # noqa: C901
)
return GenerateDashboardResponse(
- dashboard=dashboard_info, dashboard_url=dashboard_url, error=None
+ dashboard=dashboard_info,
+ dashboard_url=dashboard_url,
+ error=None,
+ warnings=sanitization_warnings,
)
except (SQLAlchemyError, ValueError, AttributeError, ValidationError) as e:
diff --git a/superset/mcp_service/utils/sanitization.py b/superset/mcp_service/utils/sanitization.py
index 22f81247128..48d32b43882 100644
--- a/superset/mcp_service/utils/sanitization.py
+++ b/superset/mcp_service/utils/sanitization.py
@@ -142,6 +142,34 @@ def _remove_dangerous_unicode(value: str) -> str:
)
+def sanitize_user_input_with_changes(
+ value: str | None,
+ field_name: str,
+ max_length: int = 255,
+ check_sql_keywords: bool = False,
+ allow_empty: bool = False,
+) -> tuple[str | None, bool]:
+ """
+ Sanitize and report whether the value was modified.
+
+ Same security guarantees as ``sanitize_user_input`` — returns both
+ the sanitized value and a boolean indicating whether any characters
+ were stripped or altered. Callers that need to surface a warning
+ when user-provided content is silently removed (e.g. XSS payloads)
+ should use this variant instead of ``sanitize_user_input``.
+ """
+ original_stripped = value.strip() if isinstance(value, str) else value
+ sanitized = sanitize_user_input(
+ value,
+ field_name,
+ max_length=max_length,
+ check_sql_keywords=check_sql_keywords,
+ allow_empty=allow_empty,
+ )
+ was_modified = original_stripped != (sanitized or "") and bool(original_stripped)
+ return sanitized, was_modified
+
+
def sanitize_user_input(
value: str | None,
field_name: str,
diff --git a/tests/unit_tests/mcp_service/chart/test_chart_schemas.py b/tests/unit_tests/mcp_service/chart/test_chart_schemas.py
index 3cee98f72b5..0c22ef7eea1 100644
--- a/tests/unit_tests/mcp_service/chart/test_chart_schemas.py
+++ b/tests/unit_tests/mcp_service/chart/test_chart_schemas.py
@@ -162,6 +162,7 @@ class TestXYChartConfig:
), # Label: "COUNT(product_line)"
],
)
+ assert config.x is not None
assert config.x.name == "product_line"
assert config.y[0].aggregate == "COUNT"
@@ -234,6 +235,7 @@ class TestXYChartConfig:
ColumnRef(name="sales", aggregate="SUM"),
],
)
+ assert config.x is not None
assert config.x.name == "product_line"
assert len(config.y) == 2
@@ -249,6 +251,7 @@ class TestXYChartConfig:
],
kind="line",
)
+ assert config.x is not None
assert config.x.name == "order_date"
assert config.kind == "line"
@@ -263,6 +266,7 @@ class TestXYChartConfig:
],
kind="line",
)
+ assert config.x is not None
assert config.x.label == "Order Date"
def test_area_chart_configuration(self) -> None:
@@ -730,6 +734,7 @@ class TestParseChartConfig:
{"chart_type": "xy", "x": {"name": "date"}, "y": [{"name": "v"}]}
)
assert config.chart_type == "xy"
+ assert config.x is not None
assert config.x.name == "date"
assert len(config.y) == 1
assert config.y[0].name == "v"
@@ -765,3 +770,66 @@ class TestParseChartConfig:
def test_coerce_invalid_json_string_raises(self) -> None:
with pytest.raises(ValidationError):
GenerateChartRequest(dataset_id=1, config="not valid json")
+
+
+class TestGenerateChartRequestChartNameSanitization:
+ """XSS / sanitization behavior for the chart_name field."""
+
+ def _config(self) -> dict[str, object]:
+ return {
+ "chart_type": "table",
+ "columns": [{"name": "a"}],
+ }
+
+ def test_plain_chart_name_passes_without_warning(self) -> None:
+ req = GenerateChartRequest(
+ dataset_id=1, config=self._config(), chart_name="Sales Report"
+ )
+ assert req.chart_name == "Sales Report"
+ assert req.sanitization_warnings == []
+
+ def test_chart_name_script_only_is_rejected(self) -> None:
+ with pytest.raises(ValidationError, match="removed entirely by sanitization"):
+ GenerateChartRequest(
+ dataset_id=1,
+ config=self._config(),
+ chart_name="<script>alert(1)</script>",
+ )
+
+ def test_chart_name_partial_strip_emits_warning(self) -> None:
+ req = GenerateChartRequest(
+ dataset_id=1,
+ config=self._config(),
+ chart_name="Q1 <b>Report</b>",
+ )
+ assert req.chart_name == "Q1 Report"
+ assert len(req.sanitization_warnings) == 1
+ assert "chart_name" in req.sanitization_warnings[0]
+
+ def test_chart_name_omitted_does_not_warn(self) -> None:
+ req = GenerateChartRequest(dataset_id=1, config=self._config())
+ assert req.chart_name is None
+ assert req.sanitization_warnings == []
+
+ def test_client_supplied_warnings_are_discarded(self) -> None:
+ """``sanitization_warnings`` is server-only; client input is dropped."""
+ req = GenerateChartRequest(
+ dataset_id=1,
+ config=self._config(),
+ chart_name="Plain Name",
+ sanitization_warnings=["<script>fake notice</script>"],
+ )
+ assert req.sanitization_warnings == []
+
+ def test_client_warnings_discarded_even_when_server_also_warns(self) -> None:
+ """Client-supplied warnings must not survive, even when the server
+ appends one of its own during the same request."""
+ req = GenerateChartRequest(
+ dataset_id=1,
+ config=self._config(),
+ chart_name="Q1 <b>Report</b>",
+ sanitization_warnings=["injected attacker text"],
+ )
+ assert len(req.sanitization_warnings) == 1
+ assert "chart_name" in req.sanitization_warnings[0]
+ assert "injected" not in req.sanitization_warnings[0]
diff --git a/tests/unit_tests/mcp_service/dashboard/test_dashboard_schemas.py b/tests/unit_tests/mcp_service/dashboard/test_dashboard_schemas.py
index 98fe464a24f..07f99004d45 100644
--- a/tests/unit_tests/mcp_service/dashboard/test_dashboard_schemas.py
+++ b/tests/unit_tests/mcp_service/dashboard/test_dashboard_schemas.py
@@ -24,9 +24,13 @@ Tests that serialize_dashboard_object correctly handles slug and other fields.
from typing import Any
from unittest.mock import MagicMock, patch
+import pytest
+from pydantic import ValidationError
+
from superset.mcp_service.dashboard.schemas import (
_extract_cross_filters_enabled,
_extract_native_filters,
+ GenerateDashboardRequest,
serialize_dashboard_object,
)
from superset.utils.json import dumps as json_dumps
@@ -339,3 +343,63 @@ class TestOmittedFieldsBuilder:
assert "json_metadata" in result.omitted_fields
assert "position_json" in result.omitted_fields
+
+
+class TestGenerateDashboardRequestTitleSanitization:
+ """XSS / sanitization behavior for dashboard_title."""
+
+ def test_plain_title_passes_without_warning(self) -> None:
+ req = GenerateDashboardRequest(
+ chart_ids=[1], dashboard_title="Analytics Dashboard"
+ )
+ assert req.dashboard_title == "Analytics Dashboard"
+ assert req.sanitization_warnings == []
+
+ def test_title_image_onerror_only_is_rejected(self) -> None:
+ with pytest.raises(ValidationError, match="removed entirely by sanitization"):
+ GenerateDashboardRequest(
+ chart_ids=[1],
+ dashboard_title='<img src=x onerror="alert(1)">',
+ )
+
+ def test_title_script_only_is_rejected(self) -> None:
+ with pytest.raises(ValidationError, match="removed entirely by sanitization"):
+ GenerateDashboardRequest(
+ chart_ids=[1],
+ dashboard_title="<script>alert(1)</script>",
+ )
+
+ def test_title_partial_strip_emits_warning(self) -> None:
+ req = GenerateDashboardRequest(
+ chart_ids=[1],
+ dashboard_title="Q1 <b>Review</b>",
+ )
+ assert req.dashboard_title == "Q1 Review"
+ assert len(req.sanitization_warnings) == 1
+ assert "dashboard_title" in req.sanitization_warnings[0]
+
+ def test_title_omitted_does_not_warn(self) -> None:
+ req = GenerateDashboardRequest(chart_ids=[1])
+ assert req.dashboard_title is None
+ assert req.sanitization_warnings == []
+
+ def test_client_supplied_warnings_are_discarded(self) -> None:
+ """``sanitization_warnings`` is server-only; client input is dropped."""
+ req = GenerateDashboardRequest(
+ chart_ids=[1],
+ dashboard_title="Plain Title",
+ sanitization_warnings=["<script>fake notice</script>"],
+ )
+ assert req.sanitization_warnings == []
+
+ def test_client_warnings_discarded_even_when_server_also_warns(self) -> None:
+ """Client-supplied warnings must not survive, even when the server
+ appends one of its own during the same request."""
+ req = GenerateDashboardRequest(
+ chart_ids=[1],
+ dashboard_title="Q1 <b>Review</b>",
+ sanitization_warnings=["injected attacker text"],
+ )
+ assert len(req.sanitization_warnings) == 1
+ assert "dashboard_title" in req.sanitization_warnings[0]
+ assert "injected" not in req.sanitization_warnings[0]