codeant-ai-for-open-source[bot] commented on code in PR #41842: URL: https://github.com/apache/superset/pull/41842#discussion_r3565416330
########## superset/mcp_service/chart/tool/restore_chart.py: ########## @@ -0,0 +1,154 @@ +# 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, + sanitize_for_llm_context, +) + +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) Review Comment: **Suggestion:** The pre-lookup keeps the DAO base filter enabled, which can hide a soft-deleted chart from users who are still valid editors; `RestoreChartCommand` intentionally bypasses the base filter to allow exactly that restore path. This causes false `NotFound` responses before authorization/restore logic runs. Align this lookup with command semantics by bypassing the base filter for restore resolution. [api mismatch] <details> <summary><b>Severity Level:</b> Critical π¨</summary> ```mdx β MCP restore_chart wrongly reports NotFound for editable charts. β οΈ Prevents soft-deleted chart recovery via MCP clients. ``` </details> <details> <summary><b>Steps of Reproduction β </b></summary> ```mdx 1. Start the MCP FastMCP server via `init_fastmcp_server()` in `superset/mcp_service/app.py:940`, which registers the `restore_chart()` tool defined in `superset/mcp_service/chart/tool/restore_chart.py:77-96`. 2. As a user who is an editor of a chart but whose dataset access has been revoked by RBAC, soft-delete the chart using the `delete_chart()` MCP tool in `superset/mcp_service/chart/tool/delete_chart.py:78-147`, which routes to `BaseDAO.delete` and sets `deleted_at` on the chart row. 3. From a FastMCP client, call the `restore_chart` MCP tool with that chartβs numeric ID or UUID; this invokes `_find_chart_for_restore()` in `superset/mcp_service/chart/tool/restore_chart.py:47-56`, which calls `ChartDAO.find_by_id_or_uuid(str(identifier), skip_visibility_filter=True)` while still applying the `ChartDAO.base_filter = ChartFilter` declared at `superset/daos/chart.py:45-47`. 4. In contrast, `BaseRestoreCommand.validate()` in `superset/commands/restore.py:76-91` uses `self.dao.find_by_id(..., id_column="uuid", skip_base_filter=True, skip_visibility_filter=True)` specifically to bypass the base filter for editorsβ trash; because `_find_chart_for_restore` does not set `skip_base_filter=True`, the chart row is hidden by `ChartFilter` and `restore_chart()` returns a `RestoreChartResponse` with `success=False` and `error_type="NotFound"` before `RestoreChartCommand` or ownership checks run, incorrectly blocking a legitimate restore. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=18b9c221cc4344a6b308eb73df10f051&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=18b9c221cc4344a6b308eb73df10f051&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:** 56:56 **Comment:** *Api Mismatch: The pre-lookup keeps the DAO base filter enabled, which can hide a soft-deleted chart from users who are still valid editors; `RestoreChartCommand` intentionally bypasses the base filter to allow exactly that restore path. This causes false `NotFound` responses before authorization/restore logic runs. Align this lookup with command semantics by bypassing the base filter for restore resolution. 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=66462f0f2b73d58275481d728ac9c2ee59d9162c8d17978506fe547080aa347f&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41842&comment_hash=66462f0f2b73d58275481d728ac9c2ee59d9162c8d17978506fe547080aa347f&reaction=dislike'>π</a> ########## superset/mcp_service/chart/tool/restore_chart.py: ########## @@ -0,0 +1,154 @@ +# 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, + sanitize_for_llm_context, +) + +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 be an editor of the chart (owners and + Admins qualify). + + 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 names are user-controlled; wrap before composing response text so + # a hostile name cannot inject prompt content into the tool output. + chart_name = sanitize_for_llm_context(chart.slice_name, field_path=("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.", + ) + except ChartForbiddenError: + await ctx.warning("Permission denied restoring chart id=%s" % (chart_id,)) + return RestoreChartResponse( + success=False, + permission_denied=True, + error=( + f"You do not have permission to restore chart '{chart_name}' " + f"(id={chart_id}). Ask the user to restore it or grant access; " + "do not retry." + ), + error_type="Forbidden", + ) + except ChartNotFoundError: + msg = f"Chart id={chart_id} is no longer restorable." + return RestoreChartResponse(success=False, error=msg, error_type="NotFound") + except (CommandException, SQLAlchemyError, ValueError) as ex: + _rollback() + await ctx.error("Chart restore failed: %s: %s" % (type(ex).__name__, ex)) + return RestoreChartResponse( + success=False, + error=f"Chart restore failed: {ex}", + error_type=type(ex).__name__, + ) Review Comment: **Suggestion:** This returns raw `SQLAlchemyError` text to clients, which can leak SQL/query/connection internals. Other mutate tools in this code path already mask DB exceptions with a generic message, and restore should do the same for consistency and security. [security] <details> <summary><b>Severity Level:</b> Major β οΈ</summary> ```mdx β οΈ MCP restore_chart leaks SQLAlchemyError details to clients. β οΈ Exposes database internals in LLM tool responses. ``` </details> <details> <summary><b>Steps of Reproduction β </b></summary> ```mdx 1. Invoke the `restore_chart` MCP tool from a FastMCP client; this calls `restore_chart()` in `superset/mcp_service/chart/tool/restore_chart.py:77-154` after the server is created by `create_mcp_app()` in `superset/mcp_service/app.py:602`. 2. During the `try` block at `restore_chart.py:120-125`, where `with event_logger.log_context(action="mcp.restore_chart"):` wraps `RestoreChartCommand(str(chart.uuid)).run()`, induce a database failure so that the command or transaction raises `SQLAlchemyError` (e.g., misconfigured DB, connection issue). 3. The exception is caught by the `except (CommandException, SQLAlchemyError, ValueError) as ex:` handler at `restore_chart.py:147-154`, which calls `_rollback()` at `restore_chart.py:59-66` and logs `await ctx.error("Chart restore failed: %s: %s" % (type(ex).__name__, ex))`. 4. The handler then returns `RestoreChartResponse(success=False, error=f"Chart restore failed: {ex}", error_type=type(ex).__name__)`, exposing the raw `SQLAlchemyError` string to the MCP client, whereas the delete path in `superset/mcp_service/chart/tool/delete_chart.py:173-185` explicitly masks `SQLAlchemyError` as `"Chart delete failed due to a database error."`, demonstrating that restore currently leaks more internal DB detail than other mutate tools. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=c42bcc964d9445fca593358ef534dab0&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=c42bcc964d9445fca593358ef534dab0&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:** 147:154 **Comment:** *Security: This returns raw `SQLAlchemyError` text to clients, which can leak SQL/query/connection internals. Other mutate tools in this code path already mask DB exceptions with a generic message, and restore should do the same for consistency and security. 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=fcbeb0c539e703d2a47677a51de91b70f1944f36161bfece3f69da793fee059c&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41842&comment_hash=fcbeb0c539e703d2a47677a51de91b70f1944f36161bfece3f69da793fee059c&reaction=dislike'>π</a> ########## superset/mcp_service/dashboard/tool/restore_dashboard.py: ########## @@ -0,0 +1,162 @@ +# 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, + sanitize_for_llm_context, +) + +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) Review Comment: **Suggestion:** The pre-lookup keeps the DAO base filter enabled, but the restore command is designed to bypass that filter so editors can restore their own soft-deleted dashboards even when visibility filters would hide them. Returning `NotFound` from this pre-check can therefore reject legitimate restores. Use the same base-filter bypass semantics as the restore command. [api mismatch] <details> <summary><b>Severity Level:</b> Critical π¨</summary> ```mdx β MCP restore_dashboard misreports NotFound for editable dashboards. β οΈ Blocks soft-deleted dashboard recovery via MCP clients. ``` </details> <details> <summary><b>Steps of Reproduction β </b></summary> ```mdx 1. Run the MCP FastMCP server so that dashboard tools, including `restore_dashboard()` in `superset/mcp_service/dashboard/tool/restore_dashboard.py:79-162`, are registered by `create_mcp_app()` in `superset/mcp_service/app.py:602`. 2. Soft-delete a dashboard (e.g., via the `delete_dashboard()` MCP tool in `superset/mcp_service/dashboard/tool/delete_dashboard.py:102-192`, which routes to `BaseDAO.delete` and sets `deleted_at`) while the user remains an editor but later loses dashboard visibility through `DashboardAccessFilter` (`DashboardDAO.base_filter = DashboardAccessFilter` at `superset/daos/dashboard.py:60-62`). 3. From a FastMCP client, call the `restore_dashboard` tool with the dashboardβs identifier; this invokes `_find_dashboard_for_restore()` at `superset/mcp_service/dashboard/tool/restore_dashboard.py:47-56`, which calls `DashboardDAO.find_by_id_or_uuid(str(identifier), skip_visibility_filter=True)` and still applies the base filter. 4. Meanwhile, `BaseRestoreCommand.validate()` used by `RestoreDashboardCommand` in `superset/commands/restore.py:76-91` deliberately performs a lookup with `skip_base_filter=True, skip_visibility_filter=True` so editors can see their own trash; because `_find_dashboard_for_restore` leaves `skip_base_filter` as False, the soft-deleted dashboard can be hidden by `DashboardAccessFilter`, causing `restore_dashboard()` to return `RestoreDashboardResponse(success=False, error_type="NotFound")` without ever invoking `RestoreDashboardCommand` or ownership checks, denying legitimate restores for editors. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ea01d32f597d490ba79e2340520bdb80&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=ea01d32f597d490ba79e2340520bdb80&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:** 56:56 **Comment:** *Api Mismatch: The pre-lookup keeps the DAO base filter enabled, but the restore command is designed to bypass that filter so editors can restore their own soft-deleted dashboards even when visibility filters would hide them. Returning `NotFound` from this pre-check can therefore reject legitimate restores. Use the same base-filter bypass semantics as the restore command. 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=fea709783f0cbdc6c5b2a3ed1dbb80d55f4bbd2ae2cb00b027a42849b18b3ee4&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41842&comment_hash=fea709783f0cbdc6c5b2a3ed1dbb80d55f4bbd2ae2cb00b027a42849b18b3ee4&reaction=dislike'>π</a> ########## superset/mcp_service/dashboard/tool/restore_dashboard.py: ########## @@ -0,0 +1,162 @@ +# 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, + sanitize_for_llm_context, +) + +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 be an editor of the dashboard (owners and Admins qualify). + + 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 titles are user-controlled; wrap before composing response + # text so a hostile title cannot inject prompt content into the output. + dashboard_name = sanitize_for_llm_context( + dashboard.dashboard_title, field_path=("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." + ), + ) + except DashboardForbiddenError: + await ctx.warning( + "Permission denied restoring dashboard id=%s" % (dashboard_id,) + ) + return RestoreDashboardResponse( + success=False, + permission_denied=True, + error=( + f"You do not have permission to restore dashboard " + f"'{dashboard_name}' (id={dashboard_id}). Ask the user to " + "restore it or grant access; do not retry." + ), + error_type="Forbidden", + ) + except DashboardNotFoundError: + msg = f"Dashboard id={dashboard_id} is no longer restorable." + return RestoreDashboardResponse(success=False, error=msg, error_type="NotFound") + except (CommandException, SQLAlchemyError, ValueError) as ex: + _rollback() + await ctx.error("Dashboard restore failed: %s: %s" % (type(ex).__name__, ex)) + return RestoreDashboardResponse( + success=False, + error=f"Dashboard restore failed: {ex}", + error_type=type(ex).__name__, Review Comment: **Suggestion:** This response includes raw database exception details, which can disclose internal SQL/DB information. Mirror the delete-tool pattern by returning a generic message for SQLAlchemy failures while keeping detailed logs server-side. [security] <details> <summary><b>Severity Level:</b> Major β οΈ</summary> ```mdx β οΈ MCP restore_dashboard returns raw SQLAlchemy errors to callers. β οΈ Reveals database/internal state in tool error messages. ``` </details> <details> <summary><b>Steps of Reproduction β </b></summary> ```mdx 1. Call the `restore_dashboard` MCP tool from a FastMCP client; this executes `restore_dashboard()` in `superset/mcp_service/dashboard/tool/restore_dashboard.py:79-162` after tool registration in `superset/mcp_service/app.py:602`. 2. Cause a database error during `RestoreDashboardCommand(str(dashboard.uuid)).run()` inside the `with event_logger.log_context(action="mcp.restore_dashboard"):` block at `restore_dashboard.py:124-128` (for example, a failing transaction or invalid SQL that surfaces as `SQLAlchemyError`). 3. The error is caught by the `except (CommandException, SQLAlchemyError, ValueError) as ex:` handler at `restore_dashboard.py:155-161`, which calls `_rollback()` (`restore_dashboard.py:59-67`) and logs `await ctx.error("Dashboard restore failed: %s: %s" % (type(ex).__name__, ex))`. 4. The handler then returns `RestoreDashboardResponse(success=False, error=f"Dashboard restore failed: {ex}", error_type=type(ex).__name__)`, exposing the raw `SQLAlchemyError` text to the MCP client, unlike `delete_dashboard()` in `superset/mcp_service/dashboard/tool/delete_dashboard.py:222-235`, where SQLAlchemy errors are mapped to the generic message `"Dashboard delete failed due to a database error."` to avoid leaking DB internals. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a038318e6c9a485383d33033f7e2b11f&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=a038318e6c9a485383d33033f7e2b11f&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:** 155:161 **Comment:** *Security: This response includes raw database exception details, which can disclose internal SQL/DB information. Mirror the delete-tool pattern by returning a generic message for SQLAlchemy failures while keeping detailed logs server-side. 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=fdecc2b3e02257100bcae99ebc853b3e6aa5461dcc5f0620de48f344b12811ff&reaction=like'>π</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41842&comment_hash=fdecc2b3e02257100bcae99ebc853b3e6aa5461dcc5f0620de48f344b12811ff&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]
