codeant-ai-for-open-source[bot] commented on code in PR #41472: URL: https://github.com/apache/superset/pull/41472#discussion_r3485216754
########## superset/mcp_service/dashboard/tool/delete_dashboard.py: ########## @@ -0,0 +1,166 @@ +# 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: delete_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 ( + DashboardDeleteFailedReportsExistError, + DashboardForbiddenError, + DashboardNotFoundError, +) +from superset.commands.exceptions import CommandException +from superset.extensions import event_logger +from superset.mcp_service.dashboard.schemas import ( + DeleteDashboardRequest, + DeleteDashboardResponse, +) +from superset.mcp_service.utils import escape_llm_context_delimiters + +logger = logging.getLogger(__name__) + + +def _find_dashboard_by_identifier(identifier: int | str) -> Any | None: + """Resolve a dashboard by numeric ID, UUID string, or slug. Returns None.""" + from superset.daos.dashboard import DashboardDAO + + if isinstance(identifier, int) or ( + isinstance(identifier, str) and identifier.isdigit() + ): + return DashboardDAO.find_by_id(int(identifier)) + # Try UUID, then fall back to slug. + dashboard = DashboardDAO.find_by_id(identifier, id_column="uuid") + if dashboard: + return dashboard + try: + return DashboardDAO.get_by_id_or_slug(identifier) + except DashboardNotFoundError: + return None Review Comment: **Suggestion:** This DAO call can raise access-denied exceptions, but the handler only catches not-found, so unauthorized lookups can escape as unhandled errors before the command layer maps them to a structured `permission_denied` response. Catch the access-denied exception (or avoid this access-checking DAO path here) so the tool returns a consistent forbidden result instead of failing unexpectedly. [api mismatch] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ⚠️ delete_dashboard MCP tool leaks uncaught access-denied errors. - ⚠️ Permission failures lack structured permission_denied response for agents. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. In `_find_dashboard_by_identifier` at `superset/mcp_service/dashboard/tool/delete_dashboard.py:45-60`, note that the DAO call `DashboardDAO.get_by_id_or_slug(identifier)` is wrapped in a `try`/`except` that only catches `DashboardNotFoundError` (lines 57-60); other exceptions propagate out of this helper. 2. Inspect `DashboardDAO.get_by_id_or_slug` in `superset/daos/dashboard.py:96-122`: after resolving the dashboard (lines 97-113), it calls `dashboard.raise_for_access()` at line 118, and on `SupersetSecurityException` it raises `DashboardAccessDeniedError` (lines 117-120), which is a subclass of `ForbiddenError` defined in `superset/commands/dashboard/exceptions.py:93-95`. 3. Observe that `delete_dashboard` (defined in `superset/mcp_service/dashboard/tool/delete_dashboard.py:83-166`) calls `_find_dashboard_by_identifier(request.identifier)` at lines 105-112, before entering the `try`/`except` block that handles `DashboardForbiddenError` and other command-layer errors (lines 117-166), so any `DashboardAccessDeniedError` from the DAO is not caught and will escape the tool. 4. When an MCP client (as in `tests/unit_tests/mcp_service/dashboard/tool/test_delete_dashboard.py:37-40` using `Client(mcp_server)`) calls `delete_dashboard` with an identifier that resolves via `get_by_id_or_slug` to a dashboard the current user is not allowed to access, `DashboardDAO.get_by_id_or_slug` raises `DashboardAccessDeniedError`; `_find_dashboard_by_identifier` does not handle it, and `delete_dashboard` only catches `DashboardForbiddenError` from `DeleteDashboardCommand.run`, so the tool fails with an uncaught error instead of returning a structured `DeleteDashboardResponse` with `permission_denied=True` and `error_type="Forbidden"`. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=def91ed6ff054029aeade51f563e3440&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=def91ed6ff054029aeade51f563e3440&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/delete_dashboard.py **Line:** 57:60 **Comment:** *Api Mismatch: This DAO call can raise access-denied exceptions, but the handler only catches not-found, so unauthorized lookups can escape as unhandled errors before the command layer maps them to a structured `permission_denied` response. Catch the access-denied exception (or avoid this access-checking DAO path here) so the tool returns a consistent forbidden result instead of failing unexpectedly. 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%2F41472&comment_hash=2dcd251e2886e88b634e4bf5751e86fd0dd50af9151a031f5583bb29a2bffa27&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41472&comment_hash=2dcd251e2886e88b634e4bf5751e86fd0dd50af9151a031f5583bb29a2bffa27&reaction=dislike'>👎</a> ########## superset/mcp_service/dashboard/tool/delete_dashboard.py: ########## @@ -0,0 +1,166 @@ +# 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: delete_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 ( + DashboardDeleteFailedReportsExistError, + DashboardForbiddenError, + DashboardNotFoundError, +) +from superset.commands.exceptions import CommandException +from superset.extensions import event_logger +from superset.mcp_service.dashboard.schemas import ( + DeleteDashboardRequest, + DeleteDashboardResponse, +) +from superset.mcp_service.utils import escape_llm_context_delimiters + +logger = logging.getLogger(__name__) + + +def _find_dashboard_by_identifier(identifier: int | str) -> Any | None: + """Resolve a dashboard by numeric ID, UUID string, or slug. Returns None.""" + from superset.daos.dashboard import DashboardDAO + + if isinstance(identifier, int) or ( + isinstance(identifier, str) and identifier.isdigit() + ): + return DashboardDAO.find_by_id(int(identifier)) Review Comment: **Suggestion:** The identifier resolution treats any digit-only string as a numeric ID and returns immediately, so dashboards whose slug is numeric (for example `"2024"`) can never be found by slug if no dashboard with ID 2024 exists. Resolve numeric strings through the same ID/UUID/slug fallback path (or attempt slug lookup when ID lookup misses) instead of short-circuiting. [incorrect condition logic] <details> <summary><b>Severity Level:</b> Major ⚠️</summary> ```mdx - ❌ delete_dashboard MCP tool fails for numeric slug identifiers. - ⚠️ Agents relying on slug identifiers mis-handle existing dashboards. ``` </details> <details> <summary><b>Steps of Reproduction ✅ </b></summary> ```mdx 1. Observe `_find_dashboard_by_identifier` implementation in `superset/mcp_service/dashboard/tool/delete_dashboard.py:45-60`, where digit-only strings satisfy the condition at lines 49-51 and immediately return `DashboardDAO.find_by_id(int(identifier))` at line 52. 2. Note that the UUID and slug resolution path (`DashboardDAO.find_by_id(..., id_column="uuid")` and `DashboardDAO.get_by_id_or_slug(identifier)`) is only executed for identifiers that do NOT satisfy the digit-only condition (lines 54-60 in the same file). 3. Inspect `DashboardDAO.get_by_id_or_slug` in `superset/daos/dashboard.py:96-122`, which for non-UUID values uses `id_or_slug_filter(id_or_slug)` (lines 102-105) to resolve either by ID or slug, meaning it correctly supports slugs that are purely numeric strings. 4. Call the MCP tool `delete_dashboard` (defined at `superset/mcp_service/dashboard/tool/delete_dashboard.py:83-102`) via the MCP server (`mcp` fixture in `tests/unit_tests/mcp_service/dashboard/tool/test_delete_dashboard.py:37-40`) with `{"request": {"identifier": "2024"}}` for a dashboard whose `Dashboard.slug == "2024"` but whose numeric ID is not 2024; `_find_dashboard_by_identifier` returns `None` after `DashboardDAO.find_by_id(2024)` fails, so `delete_dashboard` hits the not-found branch at lines 105-112 and returns a `DeleteDashboardResponse` with `success=False` and `error_type="NotFound"` even though a matching slug exists. ``` </details> [](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4ad723ef752742829d9da2a675ea94be&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=4ad723ef752742829d9da2a675ea94be&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/delete_dashboard.py **Line:** 49:52 **Comment:** *Incorrect Condition Logic: The identifier resolution treats any digit-only string as a numeric ID and returns immediately, so dashboards whose slug is numeric (for example `"2024"`) can never be found by slug if no dashboard with ID 2024 exists. Resolve numeric strings through the same ID/UUID/slug fallback path (or attempt slug lookup when ID lookup misses) instead of short-circuiting. 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%2F41472&comment_hash=c3d40c4626b1afe7bcdd9a84a6d45e5c189a83d0dd73d92700bcb7aa19efb608&reaction=like'>👍</a> | <a href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41472&comment_hash=c3d40c4626b1afe7bcdd9a84a6d45e5c189a83d0dd73d92700bcb7aa19efb608&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]
