codeant-ai-for-open-source[bot] commented on code in PR #41842: URL: https://github.com/apache/superset/pull/41842#discussion_r3533181047
########## superset/mcp_service/dashboard/tool/restore_dashboard.py: ########## @@ -0,0 +1,155 @@ +# 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: restore_dashboard +""" + +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.commands.dashboard.exceptions import ( + DashboardForbiddenError, + DashboardNotFoundError, +) +from superset.commands.exceptions import CommandException +from superset.extensions import event_logger +from superset.mcp_service.dashboard.schemas import ( + RestoreDashboardRequest, + RestoreDashboardResponse, +) +from superset.mcp_service.utils import escape_llm_context_delimiters + +logger = logging.getLogger(__name__) + + +def _find_dashboard_for_restore(identifier: int | str) -> Any | None: + """Resolve a dashboard by numeric ID or UUID, including soft-deleted rows. + + ``skip_visibility_filter`` is the only bypass β the DAO ``base_filter`` + stays in effect, so rows the user cannot see in the live UI stay hidden. + Ownership is then enforced by ``RestoreDashboardCommand``. + """ + from superset.daos.dashboard import DashboardDAO + + return DashboardDAO.find_by_id_or_uuid(str(identifier), skip_visibility_filter=True) + + +def _rollback() -> None: + from superset import db + + try: + db.session.rollback() # pylint: disable=consider-using-transaction + except SQLAlchemyError: + logger.warning( + "Database rollback failed during restore_dashboard error handling" + ) + + +@tool( + tags=["mutate"], + class_permission_name="Dashboard", + annotations=ToolAnnotations( + title="Restore dashboard", + readOnlyHint=False, + destructiveHint=False, + ), +) +async def restore_dashboard( + request: RestoreDashboardRequest, ctx: Context +) -> RestoreDashboardResponse: + """Restore a soft-deleted dashboard from trash. + + Identify the dashboard by numeric ID or UUID string (slug lookup does not + cover trashed dashboards). Only dashboards that were soft-deleted (moved + to trash while the ``SOFT_DELETE`` feature flag was enabled) can be + restored; permanently deleted dashboards are unrecoverable. The caller + must own the dashboard (or be an Admin). + + Example: + ```json + {"identifier": 42} + ``` + + Returns success with the restored dashboard's id/title, or an error. When + the caller lacks permission, ``permission_denied`` is true β do not retry; + ask the user. + """ + await ctx.info("Restoring dashboard: identifier=%s" % (request.identifier,)) + + dashboard = _find_dashboard_for_restore(request.identifier) + if not dashboard: + safe_id = escape_llm_context_delimiters(str(request.identifier)[:200]) + msg = f"No dashboard found with identifier: {safe_id}." + return RestoreDashboardResponse(success=False, error=msg, error_type="NotFound") + + dashboard_id = dashboard.id + dashboard_name = dashboard.dashboard_title + + if dashboard.deleted_at is None: + return RestoreDashboardResponse( + success=False, + error=( + f"Dashboard '{dashboard_name}' (id={dashboard_id}) is not in " + "trash; nothing to restore." + ), + error_type="NotDeleted", + ) + + try: + from superset.commands.dashboard.restore import RestoreDashboardCommand + + with event_logger.log_context(action="mcp.restore_dashboard"): + RestoreDashboardCommand(str(dashboard.uuid)).run() + + return RestoreDashboardResponse( + success=True, + restored_id=dashboard_id, + restored_name=dashboard_name, + message=( + f"Restored dashboard '{dashboard_name}' (id={dashboard_id}) from trash." + ), + ) Review Comment: **Suggestion:** `dashboard_name` is inserted directly into response text without sanitization, which lets dashboard titles carry untrusted prompt content into MCP responses. Wrap or sanitize dashboard titles before embedding them in `message`/`error` fields to prevent prompt-injection-through-output. [security] <details> <summary><b>Severity Level:</b> Critical π¨</summary> ```mdx β οΈ Dashboard titles can inject delimiters into MCP output. β οΈ Prompt-injection via restore_dashboard response affects LLM tools. β οΈ Restore_dashboard inconsistent with dashboard info sanitization rules. ``` </details> <details> <summary><b>Steps of Reproduction β </b></summary> ```mdx 1. Create or modify a dashboard whose title (dashboard.dashboard_title) contains LLM-control delimiters or prompt-like content; DashboardDAO treats dashboard_title as a mutable field in its serializer, and DashboardInfo sanitization uses this field name (superset/mcp_service/dashboard/schemas.py:18-28). 2. Soft-delete this dashboard so deleted_at on the Dashboard (SoftDeleteMixin in superset/models/helpers.py:71-85 and Dashboard restore command comments in superset/commands/dashboard/restore.py:30-40) becomes non-null via existing delete flows. 3. As a user who owns the dashboard (so restore is authorized), call the restore_dashboard MCP tool with its id or uuid using the documented schema in superset/mcp_service/dashboard/tool/restore_dashboard.py:79-90. 4. Inside restore_dashboard, the dashboard is loaded via _find_dashboard_for_restore (restore_dashboard.py:44-53) and dashboard_name is set from dashboard.dashboard_title at restore_dashboard.py:104-105; this title is user-controlled and not sanitized in this function. 5. Because dashboard.deleted_at is not None, RestoreDashboardCommand(str(dashboard.uuid)).run() executes (restore_dashboard.py:118-121), and the success response at restore_dashboard.py:123-130 is returned, embedding dashboard_name directly in the message string without calling escape_llm_context_delimiters or sanitize_for_llm_context. 6. Elsewhere, read-path dashboard responses are explicitly sanitized for LLM exposure: _sanitize_dashboard_info_for_llm_context in superset/mcp_service/dashboard/schemas.py:12-61 wraps dashboard_title, description, native_filters, chart slice_name, and datasource_name via sanitize_for_llm_context and escape_llm_context_delimiters, but restore_dashboardβs success message bypasses this sanitization, allowing untrusted dashboard titles to carry unescaped delimiters into MCP tool output. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=2f1abb4983b34ae7837c7ce014978cfb&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=2f1abb4983b34ae7837c7ce014978cfb&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent π€ </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/mcp_service/dashboard/tool/restore_dashboard.py **Line:** 123:130 **Comment:** *Security: `dashboard_name` is inserted directly into response text without sanitization, which lets dashboard titles carry untrusted prompt content into MCP responses. Wrap or sanitize dashboard titles before embedding them in `message`/`error` fields to prevent prompt-injection-through-output. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41842&comment_hash=385c4165ee169cc584a3532f7f6fcd351e0f2c2fca0df0e2188989cb05c9be30&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41842&comment_hash=385c4165ee169cc584a3532f7f6fcd351e0f2c2fca0df0e2188989cb05c9be30&reaction=dislike'>π</a> ########## superset/mcp_service/chart/tool/restore_chart.py: ########## @@ -0,0 +1,148 @@ +# 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: restore_chart +""" + +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.commands.chart.exceptions import ( + ChartForbiddenError, + ChartNotFoundError, +) +from superset.commands.exceptions import CommandException +from superset.extensions import event_logger +from superset.mcp_service.chart.schemas import ( + RestoreChartRequest, + RestoreChartResponse, +) +from superset.mcp_service.utils import escape_llm_context_delimiters + +logger = logging.getLogger(__name__) + + +def _find_chart_for_restore(identifier: int | str) -> Any | None: + """Resolve a chart by numeric ID or UUID, including soft-deleted rows. + + ``skip_visibility_filter`` is the only bypass β the DAO ``base_filter`` + stays in effect, so rows the user cannot see in the live UI stay hidden. + Ownership is then enforced by ``RestoreChartCommand``. + """ + from superset.daos.chart import ChartDAO + + return ChartDAO.find_by_id_or_uuid(str(identifier), skip_visibility_filter=True) + + +def _rollback() -> None: + from superset import db + + try: + db.session.rollback() # pylint: disable=consider-using-transaction + except SQLAlchemyError: + logger.warning("Database rollback failed during restore_chart error handling") + + +@tool( + tags=["mutate"], + class_permission_name="Chart", + annotations=ToolAnnotations( + title="Restore chart", + readOnlyHint=False, + destructiveHint=False, + ), +) +async def restore_chart( + request: RestoreChartRequest, ctx: Context +) -> RestoreChartResponse: + """Restore a soft-deleted chart from trash. + + Identify the chart by numeric ID or UUID string (NOT chart name). Only + charts that were soft-deleted (moved to trash while the ``SOFT_DELETE`` + feature flag was enabled) can be restored; permanently deleted charts are + unrecoverable. The caller must own the chart (or be an Admin). + + Example: + ```json + {"identifier": 123} + ``` + + Returns success with the restored chart's id/name, or an error. When the + caller lacks permission, ``permission_denied`` is true β do not retry; ask + the user. + """ + await ctx.info("Restoring chart: identifier=%s" % (request.identifier,)) + + chart = _find_chart_for_restore(request.identifier) + if not chart: + safe_id = escape_llm_context_delimiters(str(request.identifier)[:200]) + msg = f"No chart found with identifier: {safe_id}." + return RestoreChartResponse(success=False, error=msg, error_type="NotFound") + + chart_id = chart.id + chart_name = chart.slice_name + + if chart.deleted_at is None: + return RestoreChartResponse( + success=False, + error=( + f"Chart '{chart_name}' (id={chart_id}) is not in trash; " + "nothing to restore." + ), + error_type="NotDeleted", + ) Review Comment: **Suggestion:** The early `NotDeleted` return bypasses the command-layer ownership check, so a user who can see (but not own) a chart can still invoke this mutating tool and get a definitive restore-state response. Enforce ownership before returning this branch (or delegate to `RestoreChartCommand` for this path) so unauthorized callers consistently receive a permission-denied outcome. [security] <details> <summary><b>Severity Level:</b> Major β οΈ</summary> ```mdx β Restore tool reveals chart trash status to non-owners. β οΈ Ownership requirement in docs not enforced consistently. β οΈ Mutating tool leaks chart state without permission_denied. ``` </details> <details> <summary><b>Steps of Reproduction β </b></summary> ```mdx 1. Start the MCP service so chart tools are registered; restore_chart is exported in superset/mcp_service/chart/tool/__init__.py:18-26 and listed as a write tool in get_default_instructions at superset/mcp_service/app.py:175-186. 2. Ensure a chart exists that the current user can view via ChartFilter (superset/charts/filters.py:104-129) but does not own via the Slice.owners relationship (superset/models/slice.py:114-118); ownership is tracked separately from view access. 3. From an MCP client, call the restore_chart tool with that chart's numeric id or uuid using the documented schema (superset/mcp_service/chart/tool/restore_chart.py:77-88), so request.identifier resolves to this chart. 4. restore_chart calls _find_chart_for_restore (restore_chart.py:44-53), which uses ChartDAO.find_by_id_or_uuid with skip_visibility_filter=True; ChartDAO.base_filter is ChartFilter (superset/daos/chart.py:45-47), so a non-owner with dataset access still receives the Slice model. 5. Because chart.deleted_at is None for an active chart, the branch at restore_chart.py:104-112 executes and returns a RestoreChartResponse with error_type="NotDeleted" and a detailed message, without ever invoking RestoreChartCommand.run(). 6. In the command path, BaseRestoreCommand.validate (superset/commands/restore.py:76-97) would call security_manager.raise_for_ownership and raise ChartForbiddenError for non-owners via RestoreChartCommand (superset/commands/chart/restore.py:29-38), but this ownership check is bypassed for the NotDeleted branch, so unauthorized callers get stateful feedback instead of permission_denied. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=e56f8d84077c44cdb7f5758bb88234e2&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=e56f8d84077c44cdb7f5758bb88234e2&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent π€ </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/mcp_service/chart/tool/restore_chart.py **Line:** 104:112 **Comment:** *Security: The early `NotDeleted` return bypasses the command-layer ownership check, so a user who can see (but not own) a chart can still invoke this mutating tool and get a definitive restore-state response. Enforce ownership before returning this branch (or delegate to `RestoreChartCommand` for this path) so unauthorized callers consistently receive a permission-denied outcome. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41842&comment_hash=45e3e5126be6ee494f12b0b48e9c97bfdaaee1b144b5fdb397885b162b623496&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41842&comment_hash=45e3e5126be6ee494f12b0b48e9c97bfdaaee1b144b5fdb397885b162b623496&reaction=dislike'>π</a> ########## superset/mcp_service/chart/tool/restore_chart.py: ########## @@ -0,0 +1,148 @@ +# 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: restore_chart +""" + +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.commands.chart.exceptions import ( + ChartForbiddenError, + ChartNotFoundError, +) +from superset.commands.exceptions import CommandException +from superset.extensions import event_logger +from superset.mcp_service.chart.schemas import ( + RestoreChartRequest, + RestoreChartResponse, +) +from superset.mcp_service.utils import escape_llm_context_delimiters + +logger = logging.getLogger(__name__) + + +def _find_chart_for_restore(identifier: int | str) -> Any | None: + """Resolve a chart by numeric ID or UUID, including soft-deleted rows. + + ``skip_visibility_filter`` is the only bypass β the DAO ``base_filter`` + stays in effect, so rows the user cannot see in the live UI stay hidden. + Ownership is then enforced by ``RestoreChartCommand``. + """ + from superset.daos.chart import ChartDAO + + return ChartDAO.find_by_id_or_uuid(str(identifier), skip_visibility_filter=True) + + +def _rollback() -> None: + from superset import db + + try: + db.session.rollback() # pylint: disable=consider-using-transaction + except SQLAlchemyError: + logger.warning("Database rollback failed during restore_chart error handling") + + +@tool( + tags=["mutate"], + class_permission_name="Chart", + annotations=ToolAnnotations( + title="Restore chart", + readOnlyHint=False, + destructiveHint=False, + ), +) +async def restore_chart( + request: RestoreChartRequest, ctx: Context +) -> RestoreChartResponse: + """Restore a soft-deleted chart from trash. + + Identify the chart by numeric ID or UUID string (NOT chart name). Only + charts that were soft-deleted (moved to trash while the ``SOFT_DELETE`` + feature flag was enabled) can be restored; permanently deleted charts are + unrecoverable. The caller must own the chart (or be an Admin). + + Example: + ```json + {"identifier": 123} + ``` + + Returns success with the restored chart's id/name, or an error. When the + caller lacks permission, ``permission_denied`` is true β do not retry; ask + the user. + """ + await ctx.info("Restoring chart: identifier=%s" % (request.identifier,)) + + chart = _find_chart_for_restore(request.identifier) + if not chart: + safe_id = escape_llm_context_delimiters(str(request.identifier)[:200]) + msg = f"No chart found with identifier: {safe_id}." + return RestoreChartResponse(success=False, error=msg, error_type="NotFound") + + chart_id = chart.id + chart_name = chart.slice_name + + if chart.deleted_at is None: + return RestoreChartResponse( + success=False, + error=( + f"Chart '{chart_name}' (id={chart_id}) is not in trash; " + "nothing to restore." + ), + error_type="NotDeleted", + ) + + try: + from superset.commands.chart.restore import RestoreChartCommand + + with event_logger.log_context(action="mcp.restore_chart"): + RestoreChartCommand(str(chart.uuid)).run() + + return RestoreChartResponse( + success=True, + restored_id=chart_id, + restored_name=chart_name, + message=f"Restored chart '{chart_name}' (id={chart_id}) from trash.", + ) Review Comment: **Suggestion:** The success/error text interpolates `chart_name` directly into MCP output without LLM-context sanitization. Because chart names are user-controlled content, this can inject prompt-control delimiters into tool responses; sanitize or escape the name before composing response strings. [security] <details> <summary><b>Severity Level:</b> Critical π¨</summary> ```mdx β οΈ Chart titles can inject delimiters into MCP responses. β οΈ Prompt-injection via restore_chart output degrades LLM reliability. β οΈ Inconsistent sanitization compared to dashboard info serializers. ``` </details> <details> <summary><b>Steps of Reproduction β </b></summary> ```mdx 1. Create or modify a chart whose title (Slice.slice_name) contains LLM-control delimiters or prompt-like content; Slice defines slice_name as a user-controlled string column in superset/models/slice.py:90-92. 2. Soft-delete this chart so deleted_at on the Slice (SoftDeleteMixin in superset/models/helpers.py:71-85 and Slice base class in superset/models/slice.py:82-83) becomes non-null via existing delete flows, making it restorable. 3. As a user who owns the chart (so restore is allowed), call the restore_chart MCP tool using its documented request schema (superset/mcp_service/chart/tool/restore_chart.py:77-88) from an MCP client, with request.identifier targeting this chart id or uuid. 4. Inside restore_chart, the chart is loaded via _find_chart_for_restore (restore_chart.py:44-53) and chart_name is set from chart.slice_name at restore_chart.py:101-102; no sanitization is applied to this name. 5. Because chart.deleted_at is not None, RestoreChartCommand(str(chart.uuid)).run() executes (restore_chart.py:115-118), then the success response at restore_chart.py:120-125 is returned, interpolating chart_name directly into the message field without calling escape_llm_context_delimiters or sanitize_for_llm_context. 6. Other MCP responses deliberately sanitize user-controlled strings before LLM exposure, e.g. _sanitize_dashboard_info_for_llm_context in superset/mcp_service/dashboard/schemas.py:12-61 wraps dashboard_title, slice_name, and datasource_name via sanitize_for_llm_context and escape_llm_context_delimiters, but restore_chartβs success message omits this, allowing user-controlled chart titles to carry unescaped delimiters into MCP tool output. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=892256604b334abba12751678045c3fe&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=892256604b334abba12751678045c3fe&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent π€ </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/mcp_service/chart/tool/restore_chart.py **Line:** 120:125 **Comment:** *Security: The success/error text interpolates `chart_name` directly into MCP output without LLM-context sanitization. Because chart names are user-controlled content, this can inject prompt-control delimiters into tool responses; sanitize or escape the name before composing response strings. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41842&comment_hash=a3fdb2ce1974a8f29990b474e869519757479f04661efe040e27357a5f06b971&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41842&comment_hash=a3fdb2ce1974a8f29990b474e869519757479f04661efe040e27357a5f06b971&reaction=dislike'>π</a> ########## superset/mcp_service/dashboard/tool/restore_dashboard.py: ########## @@ -0,0 +1,155 @@ +# 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: restore_dashboard +""" + +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.commands.dashboard.exceptions import ( + DashboardForbiddenError, + DashboardNotFoundError, +) +from superset.commands.exceptions import CommandException +from superset.extensions import event_logger +from superset.mcp_service.dashboard.schemas import ( + RestoreDashboardRequest, + RestoreDashboardResponse, +) +from superset.mcp_service.utils import escape_llm_context_delimiters + +logger = logging.getLogger(__name__) + + +def _find_dashboard_for_restore(identifier: int | str) -> Any | None: + """Resolve a dashboard by numeric ID or UUID, including soft-deleted rows. + + ``skip_visibility_filter`` is the only bypass β the DAO ``base_filter`` + stays in effect, so rows the user cannot see in the live UI stay hidden. + Ownership is then enforced by ``RestoreDashboardCommand``. + """ + from superset.daos.dashboard import DashboardDAO + + return DashboardDAO.find_by_id_or_uuid(str(identifier), skip_visibility_filter=True) + + +def _rollback() -> None: + from superset import db + + try: + db.session.rollback() # pylint: disable=consider-using-transaction + except SQLAlchemyError: + logger.warning( + "Database rollback failed during restore_dashboard error handling" + ) + + +@tool( + tags=["mutate"], + class_permission_name="Dashboard", + annotations=ToolAnnotations( + title="Restore dashboard", + readOnlyHint=False, + destructiveHint=False, + ), +) +async def restore_dashboard( + request: RestoreDashboardRequest, ctx: Context +) -> RestoreDashboardResponse: + """Restore a soft-deleted dashboard from trash. + + Identify the dashboard by numeric ID or UUID string (slug lookup does not + cover trashed dashboards). Only dashboards that were soft-deleted (moved + to trash while the ``SOFT_DELETE`` feature flag was enabled) can be + restored; permanently deleted dashboards are unrecoverable. The caller + must own the dashboard (or be an Admin). + + Example: + ```json + {"identifier": 42} + ``` + + Returns success with the restored dashboard's id/title, or an error. When + the caller lacks permission, ``permission_denied`` is true β do not retry; + ask the user. + """ + await ctx.info("Restoring dashboard: identifier=%s" % (request.identifier,)) + + dashboard = _find_dashboard_for_restore(request.identifier) + if not dashboard: + safe_id = escape_llm_context_delimiters(str(request.identifier)[:200]) + msg = f"No dashboard found with identifier: {safe_id}." + return RestoreDashboardResponse(success=False, error=msg, error_type="NotFound") + + dashboard_id = dashboard.id + dashboard_name = dashboard.dashboard_title + + if dashboard.deleted_at is None: + return RestoreDashboardResponse( + success=False, + error=( + f"Dashboard '{dashboard_name}' (id={dashboard_id}) is not in " + "trash; nothing to restore." + ), + error_type="NotDeleted", + ) Review Comment: **Suggestion:** The early `NotDeleted` response is returned before any ownership enforcement, allowing non-owners with visibility to learn restore state through a write-scoped tool. Apply the same ownership gate here as in the command path so unauthorized callers do not receive object-state responses from restore operations. [security] <details> <summary><b>Severity Level:</b> Major β οΈ</summary> ```mdx β Restore_dashboard exposes trash status to non-owner dashboard viewers. β οΈ Dashboard restore tool diverges from ownership enforcement contract. β οΈ Unauthorized callers learn dashboard state via write tool. ``` </details> <details> <summary><b>Steps of Reproduction β </b></summary> ```mdx 1. Start the MCP service so dashboard tools are registered; restore_dashboard is exported in superset/mcp_service/dashboard/tool/__init__.py:18-27 and advertised as a write tool in get_default_instructions at superset/mcp_service/app.py:127-139. 2. Ensure a dashboard exists that the current user can view via DashboardAccessFilter (DashboardDAO.base_filter in superset/daos/dashboard.py:61-62) but does not own (owners relationship on the Dashboard model, referenced by dashboard_user in superset/daos/dashboard.py:99-101), giving view rights without ownership. 3. From an MCP client, call the restore_dashboard tool with that dashboardβs id or uuid using the documented request schema (superset/mcp_service/dashboard/tool/restore_dashboard.py:79-90), so request.identifier matches this dashboard. 4. restore_dashboard resolves the dashboard via _find_dashboard_for_restore (restore_dashboard.py:44-53), which calls DashboardDAO.find_by_id_or_uuid with skip_visibility_filter=True; because base_filter remains applied per BaseDAO.find_by_id_or_uuid (superset/daos/base.py:30-59), a non-owner with view access receives the Dashboard instance. 5. With an active dashboard (dashboard.deleted_at is None), the branch at restore_dashboard.py:107-115 executes and returns a RestoreDashboardResponse with error_type="NotDeleted" and a detailed message, without permission_denied=True and without invoking RestoreDashboardCommand.run(). 6. In the normal restore path, RestoreDashboardCommand extends BaseRestoreCommand (superset/commands/dashboard/restore.py:30-44), and BaseRestoreCommand.validate (superset/commands/restore.py:76-97) would enforce ownership via security_manager.raise_for_ownership and raise DashboardForbiddenError for non-owners, but this pre-command NotDeleted branch bypasses that check, so unauthorized callers use a write-scoped tool to learn restore state instead of receiving a forbidden response. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=cebfe7b938ab46658de0fd61446b41e7&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) [](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=cebfe7b938ab46658de0fd61446b41e7&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset) *(Use Cmd/Ctrl + Click for best experience)* <details> <summary><b>Prompt for AI Agent π€ </b></summary> ```mdx This is a comment left during a code review. **Path:** superset/mcp_service/dashboard/tool/restore_dashboard.py **Line:** 107:115 **Comment:** *Security: The early `NotDeleted` response is returned before any ownership enforcement, allowing non-owners with visibility to learn restore state through a write-scoped tool. Apply the same ownership gate here as in the command path so unauthorized callers do not receive object-state responses from restore operations. Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise. Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix ``` </details> <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41842&comment_hash=79a6c205b02538891cbc97659b4cb955065bcb8ff9b364f7b6974ecf01f1f807&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41842&comment_hash=79a6c205b02538891cbc97659b4cb955065bcb8ff9b364f7b6974ecf01f1f807&reaction=dislike'>π</a> -- 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]
