You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by mi...@apache.org on 2026/08/26 18:28:33 UTC
(superset) branch master updated: fix(mcp): reject unsafe dashboard layout replacements (#43476)
This is an automated email from the ASF dual-hosted git repository.
aminghadersohi pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new 434511cb376 fix(mcp): reject unsafe dashboard layout replacements (#43476)
434511cb376 is described below
commit 434511cb3768419a06e68370c6fa0e823a38a76e
Author: Amin Ghadersohi <am...@gmail.com>
AuthorDate: Wed Aug 26 14:28:26 2026 -0400
fix(mcp): reject unsafe dashboard layout replacements (#43476)
---
.../mcp_service/dashboard/layout_validation.py | 312 ++++++++++++++++
superset/mcp_service/dashboard/schemas.py | 8 +-
.../dashboard/tool/remove_chart_from_dashboard.py | 5 +-
.../mcp_service/dashboard/tool/update_dashboard.py | 14 +-
.../dashboard/test_layout_validation.py | 401 +++++++++++++++++++++
.../dashboard/tool/test_update_dashboard.py | 96 ++++-
6 files changed, 827 insertions(+), 9 deletions(-)
diff --git a/superset/mcp_service/dashboard/layout_validation.py b/superset/mcp_service/dashboard/layout_validation.py
new file mode 100644
index 00000000000..ef4bcc4661d
--- /dev/null
+++ b/superset/mcp_service/dashboard/layout_validation.py
@@ -0,0 +1,312 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+"""Validation for dashboard layouts supplied through MCP tools."""
+
+from __future__ import annotations
+
+from collections.abc import Collection
+from typing import Any
+
+_ROOT_ID = "ROOT_ID"
+_GRID_ID = "GRID_ID"
+_HEADER_ID = "HEADER_ID"
+_VERSION_KEY = "DASHBOARD_VERSION_KEY"
+_CHART_TYPE = "CHART"
+# Chart IDs are database integers; a decimal string longer than a 64-bit value
+# is malformed input rather than an ID that could ever resolve.
+_MAX_CHART_ID_DIGITS = 19
+
+# Keep in sync with the frontend's parent/child contract in
+# superset-frontend/src/dashboard/util/isValidChild.ts, which admits a child
+# only when both its type and its parent's nesting depth are allowed. The
+# values below are that file's ``parentMaxDepthLookup``; traversal is iterative
+# so deeply nested input cannot overflow Python's call stack.
+_ROOT_DEPTH = 0
+_DEPTH_ONE = _ROOT_DEPTH + 1
+_DEPTH_THREE = _ROOT_DEPTH + 3
+_DEPTH_FOUR = _ROOT_DEPTH + 4
+_DEPTH_FIVE = _ROOT_DEPTH + 5
+
+_PARENT_MAX_DEPTH: dict[str, dict[str, int]] = {
+ "ROOT": {"GRID": _ROOT_DEPTH, "TABS": _ROOT_DEPTH},
+ "GRID": {
+ "CHART": _DEPTH_ONE,
+ "COLUMN": _DEPTH_ONE,
+ "DIVIDER": _DEPTH_ONE,
+ "DYNAMIC": _DEPTH_ONE,
+ "HEADER": _DEPTH_ONE,
+ "MARKDOWN": _DEPTH_ONE,
+ "ROW": _DEPTH_ONE,
+ "TABS": _DEPTH_ONE,
+ },
+ "ROW": {
+ "CHART": _DEPTH_FOUR,
+ "COLUMN": _DEPTH_FOUR,
+ "DYNAMIC": _DEPTH_FOUR,
+ "MARKDOWN": _DEPTH_FOUR,
+ },
+ "TABS": {"TAB": _DEPTH_THREE},
+ "TAB": {
+ "CHART": _DEPTH_FIVE,
+ "COLUMN": _DEPTH_THREE,
+ "DIVIDER": _DEPTH_FIVE,
+ "DYNAMIC": _DEPTH_FIVE,
+ "HEADER": _DEPTH_FIVE,
+ "MARKDOWN": _DEPTH_FIVE,
+ "ROW": _DEPTH_THREE,
+ "TABS": _DEPTH_THREE,
+ },
+ "COLUMN": {
+ "CHART": _DEPTH_FIVE,
+ "DIVIDER": _DEPTH_THREE,
+ "HEADER": _DEPTH_FIVE,
+ "MARKDOWN": _DEPTH_FIVE,
+ "ROW": _DEPTH_THREE,
+ "TABS": _DEPTH_THREE,
+ },
+ "CHART": {},
+ "DIVIDER": {},
+ "DYNAMIC": {},
+ "HEADER": {},
+ "MARKDOWN": {},
+}
+_ALLOWED_CHILD_TYPES: dict[str, frozenset[str]] = {
+ parent_type: frozenset(child_depths)
+ for parent_type, child_depths in _PARENT_MAX_DEPTH.items()
+}
+# TABS and TAB deliberately render their children at their own depth; every
+# other container increments it. See the worked examples in isValidChild.ts.
+_DEPTH_TRANSPARENT_TYPES = frozenset({"TABS", "TAB"})
+_CONTAINER_TYPES = frozenset(
+ component_type
+ for component_type, child_types in _ALLOWED_CHILD_TYPES.items()
+ if child_types
+)
+_META_REQUIRED_TYPES = frozenset(_ALLOWED_CHILD_TYPES) - {"ROOT", "GRID"}
+
+
+def normalize_chart_id(value: Any) -> int | None:
+ """Normalize an integer or canonical decimal-string chart ID.
+
+ Only canonical decimal strings are accepted. Leading-zero forms such as
+ ``"007"`` are rejected so that layout lookups and the ``json_metadata``
+ cleanup in ``remove_chart_from_dashboard`` — which keys off
+ ``str(chart_id)`` — cannot disagree about whether a reference matches and
+ leave stale references behind. The digit bound keeps ``int()`` away from
+ CPython's integer string conversion limit, which would otherwise raise
+ ``ValueError`` out of the validator instead of returning a structured
+ error; no real chart ID approaches it.
+ """
+ if isinstance(value, bool):
+ return None
+ if isinstance(value, int):
+ return value if value > 0 else None
+ if (
+ isinstance(value, str)
+ and value.isascii()
+ and value.isdecimal()
+ and len(value) <= _MAX_CHART_ID_DIGITS
+ and not value.startswith("0")
+ ):
+ return int(value)
+ return None
+
+
+def _validate_component_shapes( # noqa: C901
+ layout: dict[str, Any],
+) -> tuple[dict[str, dict[str, Any]], str | None]:
+ """Validate and return every component object in a raw layout mapping."""
+ if layout.get(_VERSION_KEY) != "v2":
+ return {}, f"{_VERSION_KEY} must be the string 'v2'."
+
+ components: dict[str, dict[str, Any]] = {}
+ for component_id, component in layout.items():
+ if component_id == _VERSION_KEY:
+ continue
+ if not isinstance(component, dict):
+ return {}, f"Layout value {component_id} must be a component object."
+ if component.get("id") != component_id:
+ return {}, f"Layout component {component_id} must have the same id value."
+
+ component_type = component.get("type")
+ if not isinstance(component_type, str) or component_type not in (
+ _ALLOWED_CHILD_TYPES
+ ):
+ return {}, f"Layout component {component_id} has unsupported type."
+ if component_type == "DYNAMIC":
+ return {}, (
+ f"Layout component {component_id} uses DYNAMIC, which cannot be "
+ "safely validated by the server."
+ )
+
+ children = component.get("children")
+ if component_type in _CONTAINER_TYPES and children is None:
+ return {}, f"Layout component {component_id} must define children."
+ if children is not None and (
+ not isinstance(children, list)
+ or not all(isinstance(child_id, str) for child_id in children)
+ ):
+ return {}, f"Layout component {component_id}.children must be a list."
+ if component_type not in _CONTAINER_TYPES and children not in (None, []):
+ return {}, f"Layout component {component_id} cannot have children."
+ if component_type == "TABS" and not children:
+ return {}, f"Tabs component {component_id} must contain at least one tab."
+ if component_type in _META_REQUIRED_TYPES and not isinstance(
+ component.get("meta"), dict
+ ):
+ return {}, f"Layout component {component_id}.meta must be an object."
+
+ components[component_id] = component
+
+ return components, None
+
+
+def _validate_edges(
+ components: dict[str, dict[str, Any]],
+) -> tuple[dict[str, str], str | None]:
+ """Validate graph edges and return each component's actual parent."""
+ parent_by_child: dict[str, str] = {}
+ for parent_id, parent in components.items():
+ parent_type = parent["type"]
+ for child_id in parent.get("children") or []:
+ child = components.get(child_id)
+ if child is None:
+ return {}, f"Layout references missing component {child_id}."
+ if child["type"] not in _ALLOWED_CHILD_TYPES[parent_type]:
+ return {}, (
+ f"Layout component {child_id} cannot be a child of {parent_id}."
+ )
+ if child_id in parent_by_child:
+ return {}, f"Layout component {child_id} has more than one parent."
+ parent_by_child[child_id] = parent_id
+
+ if _ROOT_ID in parent_by_child:
+ return {}, "ROOT_ID must not have a parent."
+ return parent_by_child, None
+
+
+def _find_cycle(components: dict[str, dict[str, Any]]) -> str | None:
+ """Return a component ID in a cycle using an iterative depth-first walk."""
+ state: dict[str, int] = {}
+ for start_id in components:
+ if state.get(start_id) == 2:
+ continue
+ stack: list[tuple[str, bool]] = [(start_id, False)]
+ while stack:
+ component_id, exiting = stack.pop()
+ if exiting:
+ state[component_id] = 2
+ continue
+ if state.get(component_id) == 1:
+ return component_id
+ if state.get(component_id) == 2:
+ continue
+ state[component_id] = 1
+ stack.append((component_id, True))
+ for child_id in reversed(components[component_id].get("children") or []):
+ stack.append((child_id, False))
+ return None
+
+
+def validate_dashboard_layout( # noqa: C901
+ layout: dict[str, Any], expected_chart_ids: Collection[int]
+) -> str | None:
+ """Return an error when an MCP layout replacement is unsafe to persist.
+
+ Superset renders only components reachable from ``ROOT_ID`` but indexes all
+ chart nodes during hydration. This validates renderer-required component
+ shape and graph topology, then requires the reachable charts to match the
+ dashboard's associated charts before allowing a full replacement.
+
+ ``HEADER_ID`` is dashboard metadata rather than a rendered tree child.
+ Superset also retains an empty, detached ``GRID_ID`` when top-level tabs are
+ used; both are allowed as explicit reserved-node exceptions.
+ """
+ components, error = _validate_component_shapes(layout)
+ if error:
+ return error
+
+ root = components.get(_ROOT_ID)
+ if root is None or root.get("type") != "ROOT":
+ return "Layout must contain a ROOT_ID component with type ROOT."
+ root_children = root.get("children") or []
+ if len(root_children) != 1:
+ return "ROOT_ID must contain exactly one GRID or TABS component."
+
+ parent_by_child, error = _validate_edges(components)
+ if error:
+ return error
+ if cycle_id := _find_cycle(components):
+ return f"Layout contains a cycle at {cycle_id}."
+
+ visited: set[str] = set()
+ reachable_chart_ids: set[int] = set()
+ # The frontend treats ``parents`` as derived metadata and recomputes it
+ # during hydration. Saved layouts can therefore omit it or retain stale
+ # values after drag-and-drop; the validated child edges are authoritative.
+ # Depth is likewise derived here rather than trusted, and is only defined
+ # for reachable nodes, so the nesting limits are checked on this walk.
+ stack: list[tuple[str, int]] = [(_ROOT_ID, _ROOT_DEPTH)]
+ while stack:
+ component_id, depth = stack.pop()
+ component = components[component_id]
+ component_type = component["type"]
+ visited.add(component_id)
+
+ if component_type == _CHART_TYPE:
+ chart_id = normalize_chart_id(component["meta"].get("chartId"))
+ if chart_id is None:
+ return (
+ f"Chart component {component_id} must have a positive integer "
+ "or decimal-string chartId."
+ )
+ reachable_chart_ids.add(chart_id)
+
+ child_depth = depth if component_type in _DEPTH_TRANSPARENT_TYPES else depth + 1
+ for child_id in reversed(component.get("children") or []):
+ # ``_validate_edges`` already accepted this parent/child type pair,
+ # so a missing entry here is impossible.
+ if depth > _PARENT_MAX_DEPTH[component_type][components[child_id]["type"]]:
+ return (
+ f"Layout component {child_id} is nested too deeply under "
+ f"{component_id}."
+ )
+ stack.append((child_id, child_depth))
+
+ top_level_type = components[root_children[0]]["type"]
+ for component_id, component in components.items():
+ if component_id in visited:
+ continue
+ if component_id == _HEADER_ID and component["type"] == "HEADER":
+ continue
+ if (
+ component_id == _GRID_ID
+ and top_level_type == "TABS"
+ and component["type"] == "GRID"
+ and component.get("children") == []
+ ):
+ continue
+ return f"Layout component {component_id} is unreachable from ROOT_ID."
+
+ expected = set(expected_chart_ids)
+ if missing := sorted(expected - reachable_chart_ids):
+ return f"Layout would hide dashboard charts: {missing}."
+ if unknown := sorted(reachable_chart_ids - expected):
+ return f"Layout references charts not associated with the dashboard: {unknown}."
+
+ return None
diff --git a/superset/mcp_service/dashboard/schemas.py b/superset/mcp_service/dashboard/schemas.py
index 66bf5b6b56a..fa1da6762bc 100644
--- a/superset/mcp_service/dashboard/schemas.py
+++ b/superset/mcp_service/dashboard/schemas.py
@@ -762,9 +762,11 @@ class UpdateDashboardRequest(BaseModel):
None,
description=(
"Optional replacement layout (Superset's position_json dict). "
- "When set, fully replaces the existing layout. Get the current "
- "layout via ``get_dashboard_info`` first if you want to make "
- "incremental changes."
+ "When set, fully replaces the existing layout and must keep every "
+ "dashboard chart reachable from ROOT_ID, with consistent children "
+ "and parents. Do not use this field for incremental edits: MCP does "
+ "not currently expose the complete raw layout tree needed to safely "
+ "round-trip a replacement. Prefer purpose-built dashboard tools."
),
)
json_metadata_overrides: Dict[str, Any] | None = Field(
diff --git a/superset/mcp_service/dashboard/tool/remove_chart_from_dashboard.py b/superset/mcp_service/dashboard/tool/remove_chart_from_dashboard.py
index a0db952daee..1ad1f5a2c60 100644
--- a/superset/mcp_service/dashboard/tool/remove_chart_from_dashboard.py
+++ b/superset/mcp_service/dashboard/tool/remove_chart_from_dashboard.py
@@ -35,6 +35,7 @@ from superset_core.mcp.decorators import tool, ToolAnnotations
from superset.commands.exceptions import CommandException, ForbiddenError
from superset.extensions import event_logger
+from superset.mcp_service.dashboard.layout_validation import normalize_chart_id
from superset.mcp_service.dashboard.schemas import (
DashboardInfo,
RemoveChartFromDashboardRequest,
@@ -59,14 +60,12 @@ def _find_chart_keys(layout: Dict[str, Any], chart_id: int) -> list[str]:
A chart can legitimately appear more than once in a layout (e.g. under
multiple tabs), so all occurrences are returned.
"""
- # Accept both int and string chartId — position_json is user/frontend-authored
- # and imported or hand-edited layouts may store chartId as a string.
return [
key
for key, node in layout.items()
if isinstance(node, dict)
and node.get("type") == "CHART"
- and (node.get("meta") or {}).get("chartId") in (chart_id, str(chart_id))
+ and normalize_chart_id((node.get("meta") or {}).get("chartId")) == chart_id
]
diff --git a/superset/mcp_service/dashboard/tool/update_dashboard.py b/superset/mcp_service/dashboard/tool/update_dashboard.py
index 295f2f61045..b468573dc83 100644
--- a/superset/mcp_service/dashboard/tool/update_dashboard.py
+++ b/superset/mcp_service/dashboard/tool/update_dashboard.py
@@ -33,6 +33,7 @@ from superset_core.mcp.decorators import tool, ToolAnnotations
from superset.commands.dashboard.exceptions import DashboardNotFoundError
from superset.exceptions import SupersetSecurityException
from superset.extensions import db, event_logger
+from superset.mcp_service.dashboard.layout_validation import validate_dashboard_layout
from superset.mcp_service.dashboard.schemas import (
dashboard_serializer,
DashboardError,
@@ -237,6 +238,14 @@ def _validate_update_request(
from superset.dashboards.schemas import validate_css
from superset.tags.models import ObjectType
+ if request.position_json is not None:
+ chart_ids = [chart.id for chart in dashboard.slices]
+ if error := validate_dashboard_layout(request.position_json, chart_ids):
+ return DashboardError(
+ error=f"Dashboard layout is invalid: {error}",
+ error_type="InvalidDashboardLayout",
+ )
+
# Empty string clears CSS (no validation needed); only validate real content.
if request.css:
try:
@@ -289,9 +298,10 @@ async def update_dashboard(
) -> UpdateDashboardResponse | DashboardError:
"""Patch an existing dashboard's layout, theme, styling, or metadata.
- Companion to ``generate_dashboard`` for incremental edits. An LLM can:
+ Companion to ``generate_dashboard`` for incremental metadata and styling
+ edits. An LLM can:
- - Set or replace ``position_json`` after auto-generation
+ - Replace ``position_json`` only when it already has the complete raw tree
- Apply brand ``label_colors`` and ``color_scheme`` via
``json_metadata_overrides``
- Inject ``css`` to hide chrome on print-ready dashboards
diff --git a/tests/unit_tests/mcp_service/dashboard/test_layout_validation.py b/tests/unit_tests/mcp_service/dashboard/test_layout_validation.py
new file mode 100644
index 00000000000..287b29716eb
--- /dev/null
+++ b/tests/unit_tests/mcp_service/dashboard/test_layout_validation.py
@@ -0,0 +1,401 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+"""Tests for MCP dashboard layout validation."""
+
+from copy import deepcopy
+from pathlib import Path
+from typing import Any
+
+import yaml
+
+from superset.mcp_service.dashboard.layout_validation import (
+ validate_dashboard_layout,
+)
+
+
+def _grid_layout() -> dict[str, Any]:
+ return {
+ "DASHBOARD_VERSION_KEY": "v2",
+ "ROOT_ID": {
+ "children": ["GRID_ID"],
+ "id": "ROOT_ID",
+ "type": "ROOT",
+ },
+ "GRID_ID": {
+ "children": ["ROW-1"],
+ "id": "GRID_ID",
+ "parents": ["ROOT_ID"],
+ "type": "GRID",
+ },
+ "ROW-1": {
+ "children": ["CHART-1"],
+ "id": "ROW-1",
+ "meta": {},
+ "parents": ["ROOT_ID", "GRID_ID"],
+ "type": "ROW",
+ },
+ "CHART-1": {
+ "children": [],
+ "id": "CHART-1",
+ "meta": {"chartId": 1},
+ "parents": ["ROOT_ID", "GRID_ID", "ROW-1"],
+ "type": "CHART",
+ },
+ }
+
+
+def test_valid_layout() -> None:
+ assert validate_dashboard_layout(_grid_layout(), {1}) is None
+
+
+def test_valid_empty_grid() -> None:
+ layout = _grid_layout()
+ layout["GRID_ID"]["children"] = []
+ del layout["ROW-1"]
+ del layout["CHART-1"]
+
+ assert validate_dashboard_layout(layout, set()) is None
+
+
+def test_accepts_decimal_string_chart_id() -> None:
+ layout = _grid_layout()
+ layout["CHART-1"]["meta"]["chartId"] = "1"
+
+ assert validate_dashboard_layout(layout, {1}) is None
+
+
+def test_valid_top_level_tabs_with_reserved_nodes() -> None:
+ layout = {
+ "DASHBOARD_VERSION_KEY": "v2",
+ "ROOT_ID": {
+ "children": ["TABS-1"],
+ "id": "ROOT_ID",
+ "type": "ROOT",
+ },
+ "GRID_ID": {
+ "children": [],
+ "id": "GRID_ID",
+ "parents": ["ROOT_ID"],
+ "type": "GRID",
+ },
+ "HEADER_ID": {
+ "id": "HEADER_ID",
+ "meta": {"text": "Tabbed dashboard"},
+ "type": "HEADER",
+ },
+ "TABS-1": {
+ "children": ["TAB-1"],
+ "id": "TABS-1",
+ "meta": {},
+ "parents": ["ROOT_ID"],
+ "type": "TABS",
+ },
+ "TAB-1": {
+ "children": ["CHART-1"],
+ "id": "TAB-1",
+ "meta": {"text": "Overview"},
+ "parents": ["ROOT_ID", "TABS-1"],
+ "type": "TAB",
+ },
+ "CHART-1": {
+ "id": "CHART-1",
+ "meta": {"chartId": 1},
+ "parents": ["ROOT_ID", "TABS-1", "TAB-1"],
+ "type": "CHART",
+ },
+ }
+
+ assert validate_dashboard_layout(layout, {1}) is None
+
+
+def test_rejects_unreachable_chart() -> None:
+ layout = _grid_layout()
+ layout["GRID_ID"]["children"] = []
+
+ error = validate_dashboard_layout(layout, {1})
+
+ assert error == "Layout component ROW-1 is unreachable from ROOT_ID."
+
+
+def test_rejects_missing_child_reference() -> None:
+ layout = _grid_layout()
+ del layout["ROW-1"]
+
+ error = validate_dashboard_layout(layout, {1})
+
+ assert error == "Layout references missing component ROW-1."
+
+
+def test_rejects_cycle() -> None:
+ layout = _grid_layout()
+ layout["GRID_ID"]["children"] = []
+ layout["ROW-1"]["children"] = ["COLUMN-1"]
+ layout["ROW-1"]["parents"] = ["COLUMN-1"]
+ layout["COLUMN-1"] = {
+ "children": ["ROW-1"],
+ "id": "COLUMN-1",
+ "meta": {},
+ "parents": ["ROW-1"],
+ "type": "COLUMN",
+ }
+
+ error = validate_dashboard_layout(layout, {1})
+
+ assert error == "Layout contains a cycle at ROW-1."
+
+
+def test_accepts_stale_parents_metadata() -> None:
+ layout = _grid_layout()
+ layout["CHART-1"]["parents"] = ["ROOT_ID", "GRID_ID"]
+
+ assert validate_dashboard_layout(layout, {1}) is None
+
+
+def test_accepts_missing_parents_metadata() -> None:
+ layout = _grid_layout()
+ del layout["GRID_ID"]["parents"]
+ del layout["CHART-1"]["parents"]
+
+ assert validate_dashboard_layout(layout, {1}) is None
+
+
+def test_accepts_saved_example_layout_with_stale_parents() -> None:
+ fixture_path = (
+ Path(__file__).parents[4]
+ / "superset"
+ / "examples"
+ / "video_game_sales"
+ / "dashboard.yaml"
+ )
+ with fixture_path.open(encoding="utf-8") as fixture:
+ layout = yaml.safe_load(fixture)["position"]
+ chart_ids = {
+ component["meta"]["chartId"]
+ for component in layout.values()
+ if isinstance(component, dict) and component.get("type") == "CHART"
+ }
+
+ assert validate_dashboard_layout(layout, chart_ids) is None
+
+
+def test_rejects_component_in_invalid_parent() -> None:
+ layout = _grid_layout()
+ layout["ROW-1"]["type"] = "TABS"
+
+ error = validate_dashboard_layout(layout, {1})
+
+ assert error == "Layout component CHART-1 cannot be a child of ROW-1."
+
+
+def test_rejects_unsupported_component_type() -> None:
+ layout = _grid_layout()
+ layout["ROW-1"]["type"] = "UNKNOWN"
+
+ error = validate_dashboard_layout(layout, {1})
+
+ assert error == "Layout component ROW-1 has unsupported type."
+
+
+def test_rejects_layout_that_hides_associated_chart() -> None:
+ layout = deepcopy(_grid_layout())
+ layout["CHART-1"]["meta"]["chartId"] = 2
+
+ error = validate_dashboard_layout(layout, {1, 2})
+
+ assert error == "Layout would hide dashboard charts: [1]."
+
+
+def test_rejects_chart_not_associated_with_dashboard() -> None:
+ error = validate_dashboard_layout(_grid_layout(), set())
+
+ assert error == "Layout references charts not associated with the dashboard: [1]."
+
+
+def test_rejects_empty_root() -> None:
+ layout = _grid_layout()
+ layout["ROOT_ID"]["children"] = []
+
+ assert validate_dashboard_layout(layout, {1}) == (
+ "ROOT_ID must contain exactly one GRID or TABS component."
+ )
+
+
+def test_rejects_empty_tabs() -> None:
+ layout = _grid_layout()
+ layout["ROOT_ID"]["children"] = ["TABS-1"]
+ layout["TABS-1"] = {
+ "children": [],
+ "id": "TABS-1",
+ "meta": {},
+ "parents": ["ROOT_ID"],
+ "type": "TABS",
+ }
+
+ assert validate_dashboard_layout(layout, {1}) == (
+ "Tabs component TABS-1 must contain at least one tab."
+ )
+
+
+def test_rejects_non_component_top_level_value() -> None:
+ layout = _grid_layout()
+ layout["BROKEN"] = None
+
+ assert validate_dashboard_layout(layout, {1}) == (
+ "Layout value BROKEN must be a component object."
+ )
+
+
+def test_rejects_invalid_version() -> None:
+ layout = _grid_layout()
+ layout["DASHBOARD_VERSION_KEY"] = None
+
+ assert validate_dashboard_layout(layout, {1}) == (
+ "DASHBOARD_VERSION_KEY must be the string 'v2'."
+ )
+
+
+def test_rejects_missing_renderer_metadata() -> None:
+ layout = _grid_layout()
+ del layout["ROW-1"]["meta"]
+
+ assert validate_dashboard_layout(layout, {1}) == (
+ "Layout component ROW-1.meta must be an object."
+ )
+
+
+def test_rejects_dynamic_component() -> None:
+ layout = _grid_layout()
+ layout["CHART-1"]["type"] = "DYNAMIC"
+ layout["CHART-1"]["meta"] = {"componentKey": "unknown"}
+
+ assert validate_dashboard_layout(layout, set()) == (
+ "Layout component CHART-1 uses DYNAMIC, which cannot be safely "
+ "validated by the server."
+ )
+
+
+def test_rejects_malformed_string_chart_id() -> None:
+ layout = _grid_layout()
+ layout["CHART-1"]["meta"]["chartId"] = "1.0"
+
+ assert validate_dashboard_layout(layout, {1}) == (
+ "Chart component CHART-1 must have a positive integer or "
+ "decimal-string chartId."
+ )
+
+
+def test_rejects_leading_zero_string_chart_id() -> None:
+ # ``remove_chart_from_dashboard`` cleans json_metadata by ``str(chart_id)``,
+ # so accepting "001" here would let a chart be detached while stale "001"
+ # references survive in expanded_slices and timed_refresh_immune_slices.
+ layout = _grid_layout()
+ layout["CHART-1"]["meta"]["chartId"] = "001"
+
+ assert validate_dashboard_layout(layout, {1}) == (
+ "Chart component CHART-1 must have a positive integer or "
+ "decimal-string chartId."
+ )
+
+
+def test_rejects_oversized_string_chart_id() -> None:
+ # Guards CPython's integer string conversion limit: an unbounded int()
+ # would raise ValueError out of the tool instead of returning an error.
+ layout = _grid_layout()
+ layout["CHART-1"]["meta"]["chartId"] = "9" * 10_000
+
+ assert validate_dashboard_layout(layout, {1}) == (
+ "Chart component CHART-1 must have a positive integer or "
+ "decimal-string chartId."
+ )
+
+
+def test_rejects_nesting_beyond_frontend_depth_limit() -> None:
+ # isValidChild.ts caps COLUMN > ROW at a parent depth of three. Nesting
+ # ROW > COLUMN > ROW > COLUMN > ROW pushes the innermost COLUMN past it.
+ layout = _grid_layout()
+ layout["ROW-1"]["children"] = ["COLUMN-1"]
+ layout["COLUMN-1"] = {
+ "children": ["ROW-2"],
+ "id": "COLUMN-1",
+ "meta": {},
+ "type": "COLUMN",
+ }
+ layout["ROW-2"] = {
+ "children": ["COLUMN-2"],
+ "id": "ROW-2",
+ "meta": {},
+ "type": "ROW",
+ }
+ layout["COLUMN-2"] = {
+ "children": ["ROW-3"],
+ "id": "COLUMN-2",
+ "meta": {},
+ "type": "COLUMN",
+ }
+ layout["ROW-3"] = {
+ "children": ["CHART-1"],
+ "id": "ROW-3",
+ "meta": {},
+ "type": "ROW",
+ }
+
+ assert validate_dashboard_layout(layout, {1}) == (
+ "Layout component ROW-3 is nested too deeply under COLUMN-2."
+ )
+
+
+def test_accepts_maximum_supported_nesting_depth() -> None:
+ # The deepest arrangement isValidChild.ts documents as valid:
+ # root > grid > row > column > row > chart.
+ layout = _grid_layout()
+ layout["ROW-1"]["children"] = ["COLUMN-1"]
+ layout["COLUMN-1"] = {
+ "children": ["ROW-2"],
+ "id": "COLUMN-1",
+ "meta": {},
+ "type": "COLUMN",
+ }
+ layout["ROW-2"] = {
+ "children": ["CHART-1"],
+ "id": "ROW-2",
+ "meta": {},
+ "type": "ROW",
+ }
+
+ assert validate_dashboard_layout(layout, {1}) is None
+
+
+def test_accepts_tabs_without_consuming_depth() -> None:
+ # TABS and TAB render children at their own depth, so a tab-wrapped row
+ # must remain valid at the depth its enclosing container already had.
+ layout = _grid_layout()
+ layout["GRID_ID"]["children"] = ["TABS-1"]
+ layout["TABS-1"] = {
+ "children": ["TAB-1"],
+ "id": "TABS-1",
+ "meta": {},
+ "type": "TABS",
+ }
+ layout["TAB-1"] = {
+ "children": ["ROW-1"],
+ "id": "TAB-1",
+ "meta": {},
+ "type": "TAB",
+ }
+
+ assert validate_dashboard_layout(layout, {1}) is None
diff --git a/tests/unit_tests/mcp_service/dashboard/tool/test_update_dashboard.py b/tests/unit_tests/mcp_service/dashboard/tool/test_update_dashboard.py
index 07c62fd132f..1b217191851 100644
--- a/tests/unit_tests/mcp_service/dashboard/tool/test_update_dashboard.py
+++ b/tests/unit_tests/mcp_service/dashboard/tool/test_update_dashboard.py
@@ -104,7 +104,20 @@ class TestUpdateDashboard:
)
mock_get.return_value = dash
- position = {"ROOT_ID": {"type": "ROOT", "children": ["GRID_ID"]}}
+ position = {
+ "DASHBOARD_VERSION_KEY": "v2",
+ "ROOT_ID": {
+ "id": "ROOT_ID",
+ "type": "ROOT",
+ "children": ["GRID_ID"],
+ },
+ "GRID_ID": {
+ "id": "GRID_ID",
+ "type": "GRID",
+ "parents": ["ROOT_ID"],
+ "children": [],
+ },
+ }
overrides = {
"label_colors": {"Electronics": "#4C78A8"},
"cross_filters_enabled": False,
@@ -176,6 +189,87 @@ class TestUpdateDashboard:
assert payload["dashboard"]["dashboard_title"] == modified_title
assert payload["dashboard"]["description"] == modified_description
+ @patch("superset.daos.dashboard.DashboardDAO.get_by_id_or_slug")
+ @patch("superset.extensions.db.session")
+ @pytest.mark.asyncio
+ async def test_invalid_layout_does_not_replace_existing_content(
+ self, mock_session: Mock, mock_get: Mock, mcp_server: object
+ ) -> None:
+ original_position = json.dumps(
+ {
+ "DASHBOARD_VERSION_KEY": "v2",
+ "ROOT_ID": {
+ "id": "ROOT_ID",
+ "type": "ROOT",
+ "children": ["GRID_ID"],
+ },
+ "GRID_ID": {
+ "id": "GRID_ID",
+ "type": "GRID",
+ "parents": ["ROOT_ID"],
+ "children": ["CHART-old"],
+ },
+ "CHART-old": {
+ "id": "CHART-old",
+ "type": "CHART",
+ "parents": ["ROOT_ID", "GRID_ID"],
+ "meta": {"chartId": 10},
+ },
+ }
+ )
+ dash = _mock_dashboard(id=42, position_json=original_position)
+ dash.slices = [Mock(id=10)]
+ mock_get.return_value = dash
+ unreachable_layout = {
+ "DASHBOARD_VERSION_KEY": "v2",
+ "ROOT_ID": {
+ "id": "ROOT_ID",
+ "type": "ROOT",
+ "children": ["TABS-1"],
+ },
+ "TABS-1": {
+ "id": "TABS-1",
+ "type": "TABS",
+ "meta": {},
+ "parents": ["ROOT_ID"],
+ "children": ["TAB-1"],
+ },
+ "TAB-1": {
+ "id": "TAB-1",
+ "type": "TAB",
+ "meta": {"text": "Overview"},
+ "parents": ["ROOT_ID", "TABS-1"],
+ "children": [],
+ },
+ "CHART-10": {
+ "id": "CHART-10",
+ "type": "CHART",
+ "parents": ["ROOT_ID", "TABS-1", "TAB-1"],
+ "meta": {"chartId": 10},
+ },
+ }
+
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "update_dashboard",
+ {
+ "request": {
+ "identifier": 42,
+ "dashboard_title": "Must not be applied",
+ "css": ".must-not-be-applied { color: red; }",
+ "position_json": unreachable_layout,
+ }
+ },
+ )
+
+ payload = json.loads(result.content[0].text)
+ assert payload["error_type"] == "InvalidDashboardLayout"
+ assert "unreachable" in payload["error"]
+ assert dash.position_json == original_position
+ assert dash.dashboard_title == "Test Dashboard"
+ assert dash.css is None
+ mock_session.commit.assert_not_called()
+
@patch("superset.daos.dashboard.DashboardDAO.get_by_id_or_slug")
@patch("superset.extensions.db.session")
@pytest.mark.asyncio