codeant-ai-for-open-source[bot] commented on code in PR #41472:
URL: https://github.com/apache/superset/pull/41472#discussion_r3556083229
##########
superset/mcp_service/chart/schemas.py:
##########
@@ -2725,3 +2725,49 @@ class ChartFiltersInfo(BaseModel):
# Rebuild ChartInfo so Pydantic can resolve the ChartFiltersInfo forward
reference.
ChartInfo.model_rebuild()
+
+
+class DeleteChartRequest(BaseModel):
+ """Request schema for delete_chart."""
+
+ identifier: int | str = Field(
+ ...,
+ description=(
+ "Chart identifier - numeric ID or UUID string (charts have no
slug)."
+ ),
+ )
+
+ @field_validator("identifier", mode="before")
+ @classmethod
+ def reject_bool_identifier(cls, value: object) -> object:
+ """bool is a subclass of int, so identifier=true would coerce to
+ chart ID 1 and delete the wrong object; reject it outright."""
+ if isinstance(value, bool):
+ raise ValueError("identifier must be an integer ID or UUID string")
+ return value
+
+
+class DeleteChartResponse(BaseModel):
+ """Result of a delete_chart operation."""
+
+ success: bool = Field(description="Whether the chart was deleted")
+ deleted_id: int | None = Field(None, description="ID of the deleted chart")
+ deleted_name: str | None = Field(None, description="Name of the deleted
chart")
+ soft_deleted: bool = Field(
+ False,
+ description=(
+ "True when the chart was soft-deleted (moved to trash, because the
"
+ "SOFT_DELETE feature flag is enabled) and can be restored by an "
+ "owner or Admin. False means the delete was permanent."
+ ),
+ )
+ message: str | None = Field(None, description="Human-readable outcome
message")
+ error: str | None = Field(None, description="Error message if the delete
failed")
Review Comment:
**Suggestion:** The delete response exposes an `error` field without any
LLM-context sanitization, so exception text containing user-controlled content
(for example report names propagated from command errors) can be returned as
raw model content and trigger prompt-injection behavior in MCP clients. Add a
field validator that wraps this field with the same `sanitize_for_llm_context`
pattern used in other MCP response schemas. [security]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
⚠️ delete_chart MCP tool may expose unsanitized error strings.
⚠️ LLM agents risk prompt-injection via unwrapped error content.
⚠️ Error sanitization inconsistent with DatasetError schema pattern.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Invoke the MCP `delete_chart` tool, implemented at
`superset/mcp_service/chart/tool/delete_chart.py:69-99`, which returns a
`DeleteChartResponse` Pydantic model imported at lines 37-40.
2. Trigger a delete failure inside `DeleteChartCommand([chart_id]).run()`
(called at
`delete_chart.py:129-132`), for example when associated alerts/reports exist
(`ChartDeleteFailedReportsExistError` from
`superset/commands/chart/exceptions.py:136-137`) or when another
`CommandException`/`ValueError` is raised.
3. Observe the exception-handling blocks in `delete_chart.py`: at lines
160-169 and
173-186 they build `DeleteChartResponse(success=False, error=...)`, where
`error` is
composed directly from the exception string (`f"... {ex}. Remove..."` or
`client_error =
f"Chart delete failed: {ex}"`) without calling `sanitize_for_llm_context` on
this field.
4. Inspect the `DeleteChartResponse` schema in
`superset/mcp_service/chart/schemas.py:2750-2767` where `error: str | None =
Field(...)`
is declared at line 2765 with no `@field_validator("error")` to wrap it for
LLM context,
in contrast to `DatasetError` in
`superset/mcp_service/dataset/schemas.py:26-37`, which
uses a `sanitize_error_for_llm_context` validator to call
`sanitize_for_llm_context(value,
field_path=("error",))`. As a result, delete-chart error strings (including
any future
exceptions that embed user-controlled text) are emitted raw into MCP tool
JSON and then
into LLM context.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=9ab586368f63438a9367046a8b5914ea&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=9ab586368f63438a9367046a8b5914ea&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/schemas.py
**Line:** 2765:2765
**Comment:**
*Security: The delete response exposes an `error` field without any
LLM-context sanitization, so exception text containing user-controlled content
(for example report names propagated from command errors) can be returned as
raw model content and trigger prompt-injection behavior in MCP clients. Add a
field validator that wraps this field with the same `sanitize_for_llm_context`
pattern used in other MCP response schemas.
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=ca7c38bbf68fb48076c41087d3549b94f319addd090ac4985da27f8985ff918b&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41472&comment_hash=ca7c38bbf68fb48076c41087d3549b94f319addd090ac4985da27f8985ff918b&reaction=dislike'>👎</a>
##########
superset/mcp_service/dashboard/schemas.py:
##########
@@ -1793,6 +1793,50 @@ def dashboard_layout_serializer(dashboard: "Dashboard")
-> DashboardLayout:
)
+class DeleteDashboardRequest(BaseModel):
+ """Request schema for delete_dashboard."""
+
+ identifier: int | str = Field(
+ ...,
+ description="Dashboard identifier - numeric ID, UUID string, or slug.",
+ )
+
+ @field_validator("identifier", mode="before")
+ @classmethod
+ def reject_bool_identifier(cls, value: object) -> object:
+ """bool is a subclass of int, so identifier=true would coerce to
+ dashboard ID 1 and delete the wrong object; reject it outright."""
+ if isinstance(value, bool):
+ raise ValueError("identifier must be an integer ID, UUID, or slug
string")
+ return value
+
+
+class DeleteDashboardResponse(BaseModel):
+ """Result of a delete_dashboard operation."""
+
+ success: bool = Field(description="Whether the dashboard was deleted")
+ deleted_id: int | None = Field(None, description="ID of the deleted
dashboard")
+ deleted_name: str | None = Field(None, description="Title of the deleted
dashboard")
+ soft_deleted: bool = Field(
+ False,
+ description=(
+ "True when the dashboard was soft-deleted (moved to trash, because
"
+ "the SOFT_DELETE feature flag is enabled) and can be restored by
an "
+ "owner or Admin. False means the delete was permanent."
+ ),
+ )
+ message: str | None = Field(None, description="Human-readable outcome
message")
+ error: str | None = Field(None, description="Error message if the delete
failed")
Review Comment:
**Suggestion:** The dashboard delete response also returns `error` text
without schema-level sanitization, allowing command/DB error strings that
include dashboard-controlled data to be emitted raw to MCP consumers. Add a
validator to sanitize this field before serialization, consistent with other
dashboard response models that already sanitize `error`. [security]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
⚠️ delete_dashboard tool outputs unsanitized exception error strings.
⚠️ Unsanitized errors weaken MCP LLM prompt defenses.
⚠️ Error handling inconsistent with sanitized DatasetError pattern.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Invoke the MCP `delete_dashboard` tool defined at
`superset/mcp_service/dashboard/tool/delete_dashboard.py:93-124`, which
returns a
`DeleteDashboardResponse` Pydantic model imported at lines 38-40.
2. Trigger a delete failure when
`DeleteDashboardCommand([dashboard_id]).run()` (called at
`delete_dashboard.py:170-172`) raises
`DashboardDeleteFailedReportsExistError`
(`superset/commands/dashboard/exceptions.py:99-101`) or another
`CommandException`/`ValueError`, causing the handler’s except blocks at
`delete_dashboard.py:193-206` and `207-215` (plus the generic catch at
173-186) to
construct `DeleteDashboardResponse(success=False, error=...)` using the
exception’s string
directly.
3. Inspect the `DeleteDashboardResponse` schema in
`superset/mcp_service/dashboard/schemas.py:1814-1831`, where `error: str |
None =
Field(...)` is declared at line 1829 with no `@field_validator("error")`
that applies
`sanitize_for_llm_context`, so the exception text is serialized unchanged
into the MCP
tool response.
4. Compare this with `DatasetError` in
`superset/mcp_service/dataset/schemas.py:26-37`,
which defines a `@field_validator("error")` named
`sanitize_error_for_llm_context` that
wraps error text via `sanitize_for_llm_context(value,
field_path=("error",))`. The absence
of a similar validator on `DeleteDashboardResponse.error` means
delete-dashboard failures
can surface raw exception strings into LLM context, creating a
prompt-injection surface
even though other MCP schemas already follow a sanitized-error pattern.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=988c5d5756a44fdb93eea421262c0820&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=988c5d5756a44fdb93eea421262c0820&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/schemas.py
**Line:** 1829:1829
**Comment:**
*Security: The dashboard delete response also returns `error` text
without schema-level sanitization, allowing command/DB error strings that
include dashboard-controlled data to be emitted raw to MCP consumers. Add a
validator to sanitize this field before serialization, consistent with other
dashboard response models that already sanitize `error`.
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=81be8fd9f7a2b5d513215ad364a6bf0574688677763cf35a5094f1b6926c47ce&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41472&comment_hash=81be8fd9f7a2b5d513215ad364a6bf0574688677763cf35a5094f1b6926c47ce&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]
