aminghadersohi commented on code in PR #40959: URL: https://github.com/apache/superset/pull/40959#discussion_r3413991602
########## superset/mcp_service/dashboard/tool/duplicate_dashboard.py: ########## @@ -0,0 +1,277 @@ +# 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. + +""" +MCP tool: duplicate_dashboard + +Duplicates an existing dashboard, optionally deep-copying its charts. +Canonical workflow: clone a template dashboard, then edit the copy +(e.g. to create a regional or staging variant). +""" + +import logging +from typing import Any + +from fastmcp import Context +from sqlalchemy.exc import SQLAlchemyError +from superset_core.mcp.decorators import tool, ToolAnnotations + +from superset.extensions import event_logger +from superset.mcp_service.dashboard.schemas import ( + DashboardInfo, + DuplicateDashboardRequest, + DuplicateDashboardResponse, + serialize_chart_summary, +) +from superset.mcp_service.privacy import user_can_view_data_model_metadata +from superset.mcp_service.utils.url_utils import get_superset_base_url +from superset.utils import json + +logger = logging.getLogger(__name__) + + +def _build_copy_payload( + source: Any, dashboard_title: str, duplicate_slices: bool +) -> dict[str, Any]: + """Build the data payload expected by ``CopyDashboardCommand``. + + Mirrors what the frontend "Save as" flow sends to the + ``/api/v1/dashboard/<id>/copy/`` endpoint: the source dashboard's + current ``json_metadata`` with a ``positions`` key holding the current + layout (``position_json``). ``DashboardCopySchema`` requires + ``json_metadata``, and ``DashboardDAO.copy_dashboard`` reads + ``positions`` from it to remap chart IDs when ``duplicate_slices`` + is enabled. + """ + try: + metadata = json.loads(source.json_metadata or "{}") + except (json.JSONDecodeError, TypeError): + metadata = {} + if not isinstance(metadata, dict): + metadata = {} + + try: + positions = json.loads(source.position_json or "{}") + except (json.JSONDecodeError, TypeError): + positions = {} + if not isinstance(positions, dict): + positions = {} + + metadata["positions"] = positions Review Comment: Good catch — fixed in 5e9923e7b3. The tool now detects when the source has charts but its layout maps none (missing/invalid `position_json`) and fails fast with a clear error, instead of letting `set_dash_metadata` rebuild the copy with zero slices and silently produce an empty dashboard. Added a regression test. ########## tests/unit_tests/mcp_service/dashboard/tool/test_duplicate_dashboard.py: ########## @@ -0,0 +1,361 @@ +# 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. + +""" +Unit tests for the duplicate_dashboard MCP tool. + +Follows the same pattern used in test_add_chart_to_existing_dashboard.py: +- Tests run through the async MCP Client (not direct function calls) +- Patches applied at source locations (superset.daos.dashboard.*, + superset.commands.dashboard.copy.*) +- auth is mocked via the autouse mock_auth fixture + +Covers: +- Duplicate referencing the same charts (duplicate_slices=False, default) +- Duplicate with deep-copied charts (duplicate_slices=True) +- Source dashboard not found +- Source dashboard access denied / copy forbidden +- Title sanitization (XSS stripped, XSS-only title rejected) +""" + +import logging +from unittest.mock import Mock, patch + +import pytest +from fastmcp import Client + +from superset.mcp_service.app import mcp +from superset.utils import json + +logging.basicConfig(level=logging.DEBUG) +logger = logging.getLogger(__name__) + + +# --------------------------------------------------------------------------- +# Fixtures +# --------------------------------------------------------------------------- + + [email protected] +def mcp_server() -> object: + """Return the FastMCP app instance for use in MCP client tests.""" + return mcp + + [email protected](autouse=True) +def mock_auth(): Review Comment: Addressed in 5e9923e7b3 — added `-> Iterator[MagicMock]` return type to the fixture. ########## tests/unit_tests/mcp_service/dashboard/test_dashboard_schemas.py: ########## @@ -625,3 +627,63 @@ def test_client_warnings_discarded_even_when_server_also_warns(self) -> None: assert len(req.sanitization_warnings) == 1 assert "dashboard_title" in req.sanitization_warnings[0] assert "injected" not in req.sanitization_warnings[0] + + +class TestDuplicateDashboardRequestTitleSanitization: + """XSS / sanitization behavior for DuplicateDashboardRequest.dashboard_title.""" + + def test_plain_title_passes_without_warning(self) -> None: Review Comment: Addressed in 5e9923e7b3 — added a docstring to this test method. ########## tests/unit_tests/mcp_service/dashboard/test_dashboard_schemas.py: ########## @@ -625,3 +627,63 @@ def test_client_warnings_discarded_even_when_server_also_warns(self) -> None: assert len(req.sanitization_warnings) == 1 assert "dashboard_title" in req.sanitization_warnings[0] assert "injected" not in req.sanitization_warnings[0] + + +class TestDuplicateDashboardRequestTitleSanitization: + """XSS / sanitization behavior for DuplicateDashboardRequest.dashboard_title.""" + + def test_plain_title_passes_without_warning(self) -> None: + req = DuplicateDashboardRequest(dashboard_id=1, dashboard_title="Regional Copy") + assert req.dashboard_title == "Regional Copy" + assert req.sanitization_warnings == [] + + def test_title_accepts_aliases(self) -> None: Review Comment: Addressed in 5e9923e7b3 — added a docstring to this test method. ########## tests/unit_tests/mcp_service/dashboard/test_dashboard_schemas.py: ########## @@ -625,3 +627,63 @@ def test_client_warnings_discarded_even_when_server_also_warns(self) -> None: assert len(req.sanitization_warnings) == 1 assert "dashboard_title" in req.sanitization_warnings[0] assert "injected" not in req.sanitization_warnings[0] + + +class TestDuplicateDashboardRequestTitleSanitization: + """XSS / sanitization behavior for DuplicateDashboardRequest.dashboard_title.""" + + def test_plain_title_passes_without_warning(self) -> None: + req = DuplicateDashboardRequest(dashboard_id=1, dashboard_title="Regional Copy") + assert req.dashboard_title == "Regional Copy" + assert req.sanitization_warnings == [] + + def test_title_accepts_aliases(self) -> None: + req = DuplicateDashboardRequest(dashboard_id="my-slug", name="From Name") + assert req.dashboard_title == "From Name" + + def test_script_only_title_is_rejected(self) -> None: Review Comment: Addressed in 5e9923e7b3 — added a docstring to this test method. ########## tests/unit_tests/mcp_service/dashboard/test_dashboard_schemas.py: ########## @@ -625,3 +627,63 @@ def test_client_warnings_discarded_even_when_server_also_warns(self) -> None: assert len(req.sanitization_warnings) == 1 assert "dashboard_title" in req.sanitization_warnings[0] assert "injected" not in req.sanitization_warnings[0] + + +class TestDuplicateDashboardRequestTitleSanitization: + """XSS / sanitization behavior for DuplicateDashboardRequest.dashboard_title.""" + + def test_plain_title_passes_without_warning(self) -> None: + req = DuplicateDashboardRequest(dashboard_id=1, dashboard_title="Regional Copy") + assert req.dashboard_title == "Regional Copy" + assert req.sanitization_warnings == [] + + def test_title_accepts_aliases(self) -> None: + req = DuplicateDashboardRequest(dashboard_id="my-slug", name="From Name") + assert req.dashboard_title == "From Name" + + def test_script_only_title_is_rejected(self) -> None: + with pytest.raises(ValidationError, match="removed entirely by sanitization"): + DuplicateDashboardRequest( + dashboard_id=1, dashboard_title="<script>alert(1)</script>" + ) + + def test_empty_title_is_rejected(self) -> None: + with pytest.raises(ValidationError): + DuplicateDashboardRequest(dashboard_id=1, dashboard_title="") + + def test_partial_strip_emits_warning(self) -> None: + req = DuplicateDashboardRequest( + dashboard_id=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_client_supplied_warnings_are_discarded(self) -> None: + """``sanitization_warnings`` is server-only; client input is dropped.""" + req = DuplicateDashboardRequest( + dashboard_id=1, + dashboard_title="Plain Title", + sanitization_warnings=["<script>fake notice</script>"], + ) + assert req.sanitization_warnings == [] + + +class TestDuplicateDashboardResponse: + """Serialization and error sanitization for DuplicateDashboardResponse.""" + + def test_defaults(self) -> None: Review Comment: Addressed in 5e9923e7b3 — added a docstring to this test method. ########## tests/unit_tests/mcp_service/dashboard/test_dashboard_schemas.py: ########## @@ -625,3 +627,63 @@ def test_client_warnings_discarded_even_when_server_also_warns(self) -> None: assert len(req.sanitization_warnings) == 1 assert "dashboard_title" in req.sanitization_warnings[0] assert "injected" not in req.sanitization_warnings[0] + + +class TestDuplicateDashboardRequestTitleSanitization: + """XSS / sanitization behavior for DuplicateDashboardRequest.dashboard_title.""" + + def test_plain_title_passes_without_warning(self) -> None: + req = DuplicateDashboardRequest(dashboard_id=1, dashboard_title="Regional Copy") + assert req.dashboard_title == "Regional Copy" + assert req.sanitization_warnings == [] + + def test_title_accepts_aliases(self) -> None: + req = DuplicateDashboardRequest(dashboard_id="my-slug", name="From Name") + assert req.dashboard_title == "From Name" + + def test_script_only_title_is_rejected(self) -> None: + with pytest.raises(ValidationError, match="removed entirely by sanitization"): + DuplicateDashboardRequest( + dashboard_id=1, dashboard_title="<script>alert(1)</script>" + ) + + def test_empty_title_is_rejected(self) -> None: + with pytest.raises(ValidationError): + DuplicateDashboardRequest(dashboard_id=1, dashboard_title="") + + def test_partial_strip_emits_warning(self) -> None: + req = DuplicateDashboardRequest( + dashboard_id=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_client_supplied_warnings_are_discarded(self) -> None: + """``sanitization_warnings`` is server-only; client input is dropped.""" + req = DuplicateDashboardRequest( + dashboard_id=1, + dashboard_title="Plain Title", + sanitization_warnings=["<script>fake notice</script>"], + ) + assert req.sanitization_warnings == [] + + +class TestDuplicateDashboardResponse: + """Serialization and error sanitization for DuplicateDashboardResponse.""" + + def test_defaults(self) -> None: + resp = DuplicateDashboardResponse() + assert resp.dashboard is None + assert resp.dashboard_url is None + assert resp.duplicated_slices is False + assert resp.error is None + assert resp.warnings == [] + + def test_error_is_wrapped_for_llm_context(self) -> None: Review Comment: Addressed in 5e9923e7b3 — added a docstring to this test method. -- 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]
