codeant-ai-for-open-source[bot] commented on code in PR #41842: URL: https://github.com/apache/superset/pull/41842#discussion_r3575310139
########## superset/mcp_service/chart/tool/restore_chart.py: ########## @@ -0,0 +1,177 @@ +# 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. + + Both bypasses mirror ``BaseRestoreCommand.validate``'s own lookup: + ``skip_visibility_filter`` unhides the soft-deleted row, and + ``skip_base_filter`` keeps an editor's own trash reachable even when the + entity's RBAC base_filter has no editorship leg (a lost grant must not + hide a row from the one audience that can restore it). The restore + audience is enforced by ``RestoreChartCommand`` via + ``raise_for_editorship``. + """ + from superset.daos.chart import ChartDAO + + return ChartDAO.find_by_id_or_uuid( + str(identifier), + skip_base_filter=True, + 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,)) + + try: + chart = _find_chart_for_restore(request.identifier) + except SQLAlchemyError: + _rollback() + logger.exception("Chart lookup failed during restore_chart") + return RestoreChartResponse( + success=False, + error="Chart lookup failed due to a database error.", + error_type="LookupFailed", + ) + 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() Review Comment: **Suggestion:** The command execution is inside `event_logger.log_context`, but the `try/except` is outside it, so failed restore attempts exit the context by exception and are not reliably audited. Move exception handling inside the log context block so both success and failure paths are captured consistently. [incomplete implementation] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Failed chart restores missing action in central audit log. - ⚠️ Weakens ability to investigate MCP restore misuse. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Note the established audit pattern in the delete tool: in `delete_chart()` (superset/mcp_service/chart/tool/delete_chart.py:124-127) the `try/except` block is placed inside `with event_logger.log_context(action="mcp.delete_chart")`, and the comment explicitly states “the context manager does not log when an exception propagates through it”, so failures are handled inside the context to ensure logging. 2. Start the Superset MCP service and call `restore_chart()` (superset/mcp_service/chart/tool/restore_chart.py:85-104) as a user who is not an editor of the target soft-deleted chart, so `RestoreChartCommand.run()` will raise `ChartForbiddenError` after `security_manager.raise_for_editorship()` in `BaseRestoreCommand.validate()` (superset/commands/restore.py:76-103). 3. Observe the restore implementation at `superset/mcp_service/chart/tool/restore_chart.py:137-149`: the `try:` wraps the `with event_logger.log_context(action="mcp.restore_chart")` block and the subsequent success return, while the `except ChartForbiddenError` and other `except` clauses (lines 150-167) live outside the `with` block. 4. When `RestoreChartCommand(str(chart.uuid)).run()` raises `ChartForbiddenError`, the exception propagates out of `event_logger.log_context(...)` before being caught by the outer `except`, matching the delete-tool comment that the context manager does not log on exceptions; as a result, forbidden/failed restore attempts are handled and surfaced to the client but the central `event_logger` audit trail does not record a `mcp.restore_chart` failure event. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d8ba9b5b2539498db6b44c71d2e9a989&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=d8ba9b5b2539498db6b44c71d2e9a989&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:** 137:141 **Comment:** *Incomplete Implementation: The command execution is inside `event_logger.log_context`, but the `try/except` is outside it, so failed restore attempts exit the context by exception and are not reliably audited. Move exception handling inside the log context block so both success and failure paths are captured consistently. 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=c22c3b06d78457afc33b1174dd8dcbc94b7853a69c494d2aaea93f38725a6280&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41842&comment_hash=c22c3b06d78457afc33b1174dd8dcbc94b7853a69c494d2aaea93f38725a6280&reaction=dislike'>👎</a> ########## superset/mcp_service/chart/tool/restore_chart.py: ########## @@ -0,0 +1,177 @@ +# 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. + + Both bypasses mirror ``BaseRestoreCommand.validate``'s own lookup: + ``skip_visibility_filter`` unhides the soft-deleted row, and + ``skip_base_filter`` keeps an editor's own trash reachable even when the + entity's RBAC base_filter has no editorship leg (a lost grant must not + hide a row from the one audience that can restore it). The restore + audience is enforced by ``RestoreChartCommand`` via + ``raise_for_editorship``. + """ + from superset.daos.chart import ChartDAO + + return ChartDAO.find_by_id_or_uuid( + str(identifier), + skip_base_filter=True, + skip_visibility_filter=True, + ) Review Comment: **Suggestion:** The pre-lookup disables the DAO `base_filter`, which lets the tool resolve charts that should remain hidden by RBAC visibility rules and can leak their existence/title via `NotDeleted` or `Forbidden` responses before the command layer runs. Keep `base_filter` enabled in this pre-lookup and only bypass visibility filtering for soft-deleted rows. [security] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ MCP restore_chart leaks names of RBAC-hidden charts. - ⚠️ Undermines dashboard/chart RBAC visibility guarantees for MCP clients. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Start the Superset MCP service so chart tools are available; tools are wired via `create_mcp_app()` in `superset/mcp_service/app.py:602`, which includes `restore_chart()` from `superset/mcp_service/chart/tool/restore_chart.py:85`. 2. Create a chart that is hidden from the caller by RBAC base filtering, e.g. via `ChartDAO.base_filter = ChartFilter` in `superset/daos/chart.py:45-47` so `ChartDAO.find_by_id_or_uuid()` normally scopes results to permitted datasources. 3. As an MCP client user who has Chart class permission but lacks editorship/datasource access to that chart, call the `restore_chart()` tool (superset/mcp_service/chart/tool/restore_chart.py:85-104) with `request.identifier` set to the chart’s numeric ID or UUID. 4. Inside `restore_chart()`, helper `_find_chart_for_restore()` (superset/mcp_service/chart/tool/restore_chart.py:47-64) calls `ChartDAO.find_by_id_or_uuid(str(identifier), skip_base_filter=True, skip_visibility_filter=True)` (BaseDAO implementation at `superset/daos/base.py:229-245`), resolving the RBAC-hidden chart; subsequent checks at `restore_chart.py:128-135` (“NotDeleted” path) and error handling at `restore_chart.py:150-159` (“Forbidden” path) include `chart.slice_name` and `chart.id` in tool responses, disclosing the existence/name of charts that base_filter would normally hide from this user. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4673099de8d64724a9cf9aec5ec73ad9&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=4673099de8d64724a9cf9aec5ec73ad9&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:** 60:64 **Comment:** *Security: The pre-lookup disables the DAO `base_filter`, which lets the tool resolve charts that should remain hidden by RBAC visibility rules and can leak their existence/title via `NotDeleted` or `Forbidden` responses before the command layer runs. Keep `base_filter` enabled in this pre-lookup and only bypass visibility filtering for soft-deleted rows. 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=4b52a08930ff3d3bc4d88b3d40af2f66d0db0ef33b650f00ca6d539806905168&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41842&comment_hash=4b52a08930ff3d3bc4d88b3d40af2f66d0db0ef33b650f00ca6d539806905168&reaction=dislike'>👎</a> ########## superset/mcp_service/dashboard/tool/restore_dashboard.py: ########## @@ -0,0 +1,185 @@ +# 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. + + Both bypasses mirror ``BaseRestoreCommand.validate``'s own lookup: + ``skip_visibility_filter`` unhides the soft-deleted row, and + ``skip_base_filter`` keeps an editor's own trash reachable even when the + entity's RBAC base_filter has no editorship leg (a lost grant must not + hide a row from the one audience that can restore it). The restore + audience is enforced by ``RestoreDashboardCommand`` via + ``raise_for_editorship``. + """ + from superset.daos.dashboard import DashboardDAO + + return DashboardDAO.find_by_id_or_uuid( + str(identifier), + skip_base_filter=True, + 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,)) + + try: + dashboard = _find_dashboard_for_restore(request.identifier) + except SQLAlchemyError: + _rollback() + logger.exception("Dashboard lookup failed during restore_dashboard") + return RestoreDashboardResponse( + success=False, + error="Dashboard lookup failed due to a database error.", + error_type="LookupFailed", + ) + 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() Review Comment: **Suggestion:** The `try/except` wraps the `with event_logger.log_context(...)` block, which means exceptions from restore can propagate through the context manager before being handled and may skip failure audit logging. Mirror the established delete-tool pattern by placing exception handling inside the logging context. [incomplete implementation] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ Failed dashboard restores missing entries in audit trail. - ⚠️ Hampers forensic analysis of MCP dashboard restore misuse. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Compare the established delete-tool pattern: in `delete_dashboard()` (superset/mcp_service/dashboard/tool/delete_dashboard.py:165-169) the `try/except` block is placed inside `with event_logger.log_context(action="mcp.delete_dashboard")`, with a comment noting that the context manager does not log when an exception propagates through it, so failures are handled inside the context to guarantee audit logging. 2. Start the Superset MCP service and call `restore_dashboard()` (superset/mcp_service/dashboard/tool/restore_dashboard.py:87-106) as a user who is not an editor of the soft-deleted dashboard, so `RestoreDashboardCommand.run()` ultimately raises `DashboardForbiddenError` via `security_manager.raise_for_editorship()` in `BaseRestoreCommand.validate()` (superset/commands/restore.py:76-103). 3. Inspect the restore implementation at `superset/mcp_service/dashboard/tool/restore_dashboard.py:141-171`: the outer `try:` wraps the `with event_logger.log_context(action="mcp.restore_dashboard")` block and the success return, while the `except DashboardForbiddenError`, `except DashboardNotFoundError`, and generic `except (CommandException, SQLAlchemyError, ValueError)` handlers (lines 155-172) reside outside the `with` block. 4. When `RestoreDashboardCommand(str(dashboard.uuid)).run()` raises `DashboardForbiddenError` or another error, the exception leaves `event_logger.log_context(...)` before being caught by the outer `except`, matching the delete-tool comment about missing logs on propagated exceptions; as a result, failed or forbidden dashboard restore attempts are surfaced to the client but do not produce a corresponding `mcp.restore_dashboard` event in the central audit log, reducing visibility into restore failures compared to delete operations. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=19a1f4c3a5e247da9c1844fd372593cf&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=19a1f4c3a5e247da9c1844fd372593cf&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:** 141:145 **Comment:** *Incomplete Implementation: The `try/except` wraps the `with event_logger.log_context(...)` block, which means exceptions from restore can propagate through the context manager before being handled and may skip failure audit logging. Mirror the established delete-tool pattern by placing exception handling inside the logging context. 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=ffd102e5339d94861ac667ee1b99d94566980f2f13d829040c01c89e69b425ff&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41842&comment_hash=ffd102e5339d94861ac667ee1b99d94566980f2f13d829040c01c89e69b425ff&reaction=dislike'>👎</a> ########## superset/mcp_service/dashboard/tool/restore_dashboard.py: ########## @@ -0,0 +1,185 @@ +# 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. + + Both bypasses mirror ``BaseRestoreCommand.validate``'s own lookup: + ``skip_visibility_filter`` unhides the soft-deleted row, and + ``skip_base_filter`` keeps an editor's own trash reachable even when the + entity's RBAC base_filter has no editorship leg (a lost grant must not + hide a row from the one audience that can restore it). The restore + audience is enforced by ``RestoreDashboardCommand`` via + ``raise_for_editorship``. + """ + from superset.daos.dashboard import DashboardDAO + + return DashboardDAO.find_by_id_or_uuid( + str(identifier), + skip_base_filter=True, + skip_visibility_filter=True, + ) Review Comment: **Suggestion:** The restore pre-lookup bypasses `base_filter`, so dashboards hidden by normal DAO scoping can still be resolved and partially disclosed in tool errors/messages. Keep `base_filter` active and only skip visibility filtering needed to see soft-deleted rows. [security] <details> <summary><b>Severity Level:</b> Critical 🚨</summary> ```mdx - ❌ MCP restore_dashboard leaks titles of RBAC-hidden dashboards. - ⚠️ Bypasses dashboard base_filter visibility gate for MCP callers. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Start the Superset MCP service so dashboard tools are available; tools are wired via `create_mcp_app()` in `superset/mcp_service/app.py:602`, which includes `restore_dashboard()` from `superset/mcp_service/dashboard/tool/restore_dashboard.py:87`. 2. Ensure a dashboard exists that is hidden from the caller by `DashboardDAO.base_filter = DashboardAccessFilter` (superset/daos/dashboard.py:60-61), so standard DAO lookups respect RBAC scoping and the user cannot see it via normal list/get APIs. 3. As an MCP client user with Dashboard class permission but without editorship or view access to that dashboard, call the `restore_dashboard()` tool (superset/mcp_service/dashboard/tool/restore_dashboard.py:87-106) with `request.identifier` set to that dashboard’s numeric ID or UUID. 4. Inside `restore_dashboard()`, helper `_find_dashboard_for_restore()` (superset/mcp_service/dashboard/tool/restore_dashboard.py:48-64) invokes `DashboardDAO.find_by_id_or_uuid(str(identifier), skip_base_filter=True, skip_visibility_filter=True)` (BaseDAO implementation at `superset/daos/base.py:229-245`), bypassing the RBAC base_filter and resolving the hidden dashboard; subsequent logic at `restore_dashboard.py:131-138` (“NotDeleted” error) and forbidden handling at `restore_dashboard.py:156-166` includes the sanitized title and id in responses, disclosing the existence/title of dashboards that the base_filter is designed to keep invisible to this caller. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a287a79e619c45259c8770ef695671f1&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=a287a79e619c45259c8770ef695671f1&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:** 60:64 **Comment:** *Security: The restore pre-lookup bypasses `base_filter`, so dashboards hidden by normal DAO scoping can still be resolved and partially disclosed in tool errors/messages. Keep `base_filter` active and only skip visibility filtering needed to see soft-deleted rows. 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=7b5a0f9c00c37b848994b02662c2801bcac1729d5f491621ff0533ab0fb6cd71&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41842&comment_hash=7b5a0f9c00c37b848994b02662c2801bcac1729d5f491621ff0533ab0fb6cd71&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]
