codeant-ai-for-open-source[bot] commented on code in PR #40399:
URL: https://github.com/apache/superset/pull/40399#discussion_r3293916656
##########
superset/mcp_service/dashboard/schemas.py:
##########
@@ -607,6 +649,73 @@ def sanitize_dashboard_title(cls, v: str | None) -> str |
None:
)
+class UpdateDashboardRequest(BaseModel):
+ """Request schema for updating an existing dashboard's layout/theme/style.
+
+ All fields are optional; only the fields explicitly passed are applied.
+ Use to retroactively set a custom layout, brand palette, or CSS on a
+ dashboard that was created via ``generate_dashboard`` (or earlier via
+ the REST API) without a full re-create.
+ """
+
+ model_config = ConfigDict(populate_by_name=True)
+
+ identifier: int | str = Field(
+ ...,
+ description=(
+ "Dashboard ID (integer), UUID, or slug. Same identifier shape "
+ "accepted by ``get_dashboard_info``."
+ ),
+ )
+ dashboard_title: str | None = Field(
+ None,
+ max_length=500,
+ description="Optional new dashboard title.",
+ validation_alias=AliasChoices("dashboard_title", "title"),
+ )
Review Comment:
**Suggestion:** `UpdateDashboardRequest` accepts `dashboard_title` without
any sanitization validator, while `GenerateDashboardRequest` explicitly
sanitizes titles to prevent XSS. This creates a bypass where unsafe HTML/script
content can be written through `update_dashboard` even though create-path
validation blocks it. Add the same title sanitization (and empty-after-sanitize
rejection behavior) to `UpdateDashboardRequest` to keep update/create security
behavior consistent. [security]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ MCP `update_dashboard` persists unsanitized dashboard_title values.
- ⚠️ Create and update paths diverge in XSS protections.
- ⚠️ Stored unsafe titles may surface in downstream consumers.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Observe that `GenerateDashboardRequest` in
`superset/mcp_service/dashboard/schemas.py`
lines 518–649 applies XSS-focused sanitization to `dashboard_title`: the
`@model_validator` `_detect_dashboard_title_sanitization` (lines 590–633)
uses
`sanitize_user_input_with_changes` and raises when the sanitized result is
empty, and the
`@field_validator("dashboard_title")` `sanitize_dashboard_title` (lines
635–649) calls
`sanitize_user_input` before the title is stored.
2. Confirm via tests that script-only titles are rejected on the create path:
`tests/unit_tests/mcp_service/dashboard/test_dashboard_schemas.py` lines
581–593
(`TestGenerateDashboardRequestTitleSanitization.test_title_image_onerror_only_is_rejected`
and `test_title_script_only_is_rejected`) construct
`GenerateDashboardRequest(chart_ids=[1], dashboard_title='<img src=x
onerror="alert(1)">'`
and `"<script>alert(1)</script>"` and assert `ValidationError` with the
"removed entirely
by sanitization" message.
3. Compare this with `UpdateDashboardRequest` in
`superset/mcp_service/dashboard/schemas.py` lines 652–716: the
`dashboard_title` field
(lines 670–675) is defined with
`validation_alias=AliasChoices("dashboard_title",
"title")` but there is no `@model_validator` or `@field_validator` on
`UpdateDashboardRequest` that calls `sanitize_user_input` or
`sanitize_user_input_with_changes`, so any string (including
`<script>alert(1)</script>`)
passes through unmodified.
4. Follow the execution path of the FastMCP tool `update_dashboard` in
`superset/mcp_service/dashboard/tool/update_dashboard.py`: the tool is
exported from
`superset/mcp_service/dashboard/tool/__init__.py` (lines 19–29) and imported
into the MCP
app in `superset/mcp_service/app.py` lines 11–17, making it callable by
clients. When a
caller sends `{"identifier": 42, "dashboard_title":
"<script>alert(1)</script>"}` (or via
the alias `title`) the request is parsed as `UpdateDashboardRequest`
(schemas file lines
652–675). The tool then sets `dashboard.dashboard_title =
request.dashboard_title` at
`update_dashboard.py` lines 118–120 and commits the change at line 166,
without any
sanitization step. This allows storing a script-only title that the
`GenerateDashboardRequest` path explicitly rejects, creating a concrete
bypass of the
create-time XSS protection for titles.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a1cff26ec874485e9efc399e283b1466&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=a1cff26ec874485e9efc399e283b1466&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:** 670:675
**Comment:**
*Security: `UpdateDashboardRequest` accepts `dashboard_title` without
any sanitization validator, while `GenerateDashboardRequest` explicitly
sanitizes titles to prevent XSS. This creates a bypass where unsafe HTML/script
content can be written through `update_dashboard` even though create-path
validation blocks it. Add the same title sanitization (and empty-after-sanitize
rejection behavior) to `UpdateDashboardRequest` to keep update/create security
behavior consistent.
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%2F40399&comment_hash=3b5cd9feb37095ec366abc2af74167e697570d8c3ec1d79e793589bc4590e933&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40399&comment_hash=3b5cd9feb37095ec366abc2af74167e697570d8c3ec1d79e793589bc4590e933&reaction=dislike'>👎</a>
##########
superset/mcp_service/dashboard/tool/generate_dashboard.py:
##########
@@ -253,9 +253,14 @@ def generate_dashboard( # noqa: C901
),
)
- # Create dashboard layout with chart objects
+ # Create dashboard layout with chart objects.
+ # If the caller provided an explicit position_json, use it verbatim;
+ # otherwise auto-generate a packed-grid layout from the chart ids.
with event_logger.log_context(action="mcp.generate_dashboard.layout"):
- layout = _create_dashboard_layout(chart_objects)
+ if request.position_json:
+ layout = request.position_json
+ else:
+ layout = _create_dashboard_layout(chart_objects)
Review Comment:
**Suggestion:** The explicit layout branch uses a truthy check, so a
provided but empty `position_json` is ignored and auto-layout is generated
instead. This breaks the documented "use caller-supplied layout when provided"
behavior. Switch the condition to an explicit `is not None` check so provided
payloads are honored consistently. [incorrect condition logic]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ MCP dashboard generation ignores explicit empty layout payloads.
- ⚠️ LLM callers see auto-layout instead of requested layout.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In `superset/mcp_service/dashboard/tool/generate_dashboard.py:190-205`,
the
`generate_dashboard()` MCP tool is defined and at lines 256-263 it decides
the layout
using `with
event_logger.log_context(action="mcp.generate_dashboard.layout")` followed by
`if request.position_json: layout = request.position_json else: layout =
_create_dashboard_layout(chart_objects)`.
2. The Pydantic schema `GenerateDashboardRequest` in
`superset/mcp_service/dashboard/schemas.py` (see tests at
`tests/unit_tests/mcp_service/dashboard/test_dashboard_schemas.py:22-38`)
allows callers
to pass a `position_json` dict explicitly, including an empty dict.
3. When a FastMCP client calls `generate_dashboard` with a request like
`GenerateDashboardRequest(chart_ids=[1], dashboard_title="Test",
position_json={})`, at
runtime `request.position_json` is `{}` which is falsy, so the condition at
`generate_dashboard.py:260` evaluates to False and the code falls into the
`else` branch,
calling `_create_dashboard_layout(chart_objects)` instead of using the
caller-supplied
layout.
4. The resulting dashboard is persisted with `dashboard.position_json =
json.dumps(layout)` at `generate_dashboard.py:315`, so the stored layout
reflects the
auto-generated grid, contradicting the documented intent in the preceding
comment at lines
256-258 ("If the caller provided an explicit position_json, use it verbatim;
otherwise
auto-generate") whenever the caller provides an empty-but-intentional layout
structure.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=1c0424a2722f41019a74c1b17224c8fc&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=1c0424a2722f41019a74c1b17224c8fc&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/generate_dashboard.py
**Line:** 260:263
**Comment:**
*Incorrect Condition Logic: The explicit layout branch uses a truthy
check, so a provided but empty `position_json` is ignored and auto-layout is
generated instead. This breaks the documented "use caller-supplied layout when
provided" behavior. Switch the condition to an explicit `is not None` check so
provided payloads are honored 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%2F40399&comment_hash=18d66865138834267aaf8711548b355c68e2b5062611c788f645ddba3fb8206e&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40399&comment_hash=18d66865138834267aaf8711548b355c68e2b5062611c788f645ddba3fb8206e&reaction=dislike'>👎</a>
##########
superset/mcp_service/dashboard/tool/update_dashboard.py:
##########
@@ -0,0 +1,205 @@
+# 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.
+
+"""
+Update dashboard FastMCP tool
+
+This module contains the FastMCP tool for updating an existing dashboard's
+layout, theme, and styling. Companion to ``generate_dashboard`` for
+incremental edits without re-creating the dashboard.
+"""
+
+import logging
+
+from fastmcp import Context
+from sqlalchemy.exc import SQLAlchemyError
+from superset_core.mcp.decorators import tool, ToolAnnotations
+
+from superset.extensions import db, event_logger
+from superset.mcp_service.dashboard.schemas import (
+ DashboardError,
+ GenerateDashboardResponse,
+ UpdateDashboardRequest,
+ dashboard_serializer,
+)
+from superset.mcp_service.utils.url_utils import get_superset_base_url
+from superset.utils import json
+
+logger = logging.getLogger(__name__)
+
+
+def _resolve_dashboard(identifier):
+ """Look up a dashboard by id, uuid, or slug. Returns the model or None."""
+ # Deferred import — DashboardDAO transitively pulls models whose
+ # Column definitions need the Flask app to be initialized first.
+ # Importing inside the function lets tool registration succeed at
+ # module-load time without triggering "App not initialized yet".
+ from superset.daos.dashboard import DashboardDAO
+
+ try:
+ return DashboardDAO.get_by_id_or_slug(identifier)
+ except Exception: # pylint: disable=broad-except
+ # get_by_id_or_slug raises DashboardNotFoundError; treat all
+ # lookup-time errors as "not found" so the tool can return a
+ # structured DashboardError response instead of bubbling up.
+ return None
Review Comment:
**Suggestion:** Catching all exceptions in dashboard lookup and converting
them to "not found" masks real failures (including database/authorization
errors) and produces incorrect error semantics. Restrict this to expected
not-found exceptions and let unexpected errors surface as database/system
errors so callers can distinguish missing resources from operational failures.
[logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Permission denials reported misleadingly as "Dashboard not found".
- ⚠️ Operational failures indistinguishable from legitimately missing
dashboards.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. The helper `_resolve_dashboard(identifier)` in
`superset/mcp_service/dashboard/tool/update_dashboard.py:45-59` imports
`DashboardDAO` and
wraps `DashboardDAO.get_by_id_or_slug(identifier)` in a broad `try/except
Exception` at
lines 53-59, returning `None` on any exception so that callers treat it as
"not found".
2. `DashboardDAO.get_by_id_or_slug` in `superset/daos/dashboard.py:135-161`
first locates
the dashboard (raising `DashboardNotFoundError` if absent at line 153), then
performs
access checks by calling `dashboard.raise_for_access()` and catching
`SupersetSecurityException` to raise `DashboardAccessDeniedError` at lines
155-159. Both
of these domain-specific exceptions, as well as any unexpected runtime
errors (for
example, database connectivity issues), are subclasses of `Exception`.
3. Because `_resolve_dashboard` catches all `Exception` types, a permission
failure
(`DashboardAccessDeniedError`) or operational failure (e.g. a database error
propagated
through SQLAlchemy) will be swallowed and converted into `None`,
indistinguishable from
the legitimate "dashboard does not exist" case.
4. The `update_dashboard` tool then checks `if dashboard is None` at
`update_dashboard.py:107-112` and returns a
`DashboardError(error_type="DashboardNotFound")` to the caller,
misclassifying
access-denied or system failures as resource-not-found and preventing
clients from
distinguishing missing dashboards from authorization or operational problems.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=ede5862080454cd8983812f952e89d4e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=ede5862080454cd8983812f952e89d4e&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/update_dashboard.py
**Line:** 53:59
**Comment:**
*Logic Error: Catching all exceptions in dashboard lookup and
converting them to "not found" masks real failures (including
database/authorization errors) and produces incorrect error semantics. Restrict
this to expected not-found exceptions and let unexpected errors surface as
database/system errors so callers can distinguish missing resources from
operational failures.
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%2F40399&comment_hash=dc5dd6b8a95dcdacef8229900da726f8b63cf1b61601a2586e8222a19150c860&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40399&comment_hash=dc5dd6b8a95dcdacef8229900da726f8b63cf1b61601a2586e8222a19150c860&reaction=dislike'>👎</a>
##########
superset/mcp_service/dashboard/tool/update_dashboard.py:
##########
@@ -0,0 +1,205 @@
+# 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.
+
+"""
+Update dashboard FastMCP tool
+
+This module contains the FastMCP tool for updating an existing dashboard's
+layout, theme, and styling. Companion to ``generate_dashboard`` for
+incremental edits without re-creating the dashboard.
+"""
+
+import logging
+
+from fastmcp import Context
+from sqlalchemy.exc import SQLAlchemyError
+from superset_core.mcp.decorators import tool, ToolAnnotations
+
+from superset.extensions import db, event_logger
+from superset.mcp_service.dashboard.schemas import (
+ DashboardError,
+ GenerateDashboardResponse,
+ UpdateDashboardRequest,
+ dashboard_serializer,
+)
+from superset.mcp_service.utils.url_utils import get_superset_base_url
+from superset.utils import json
+
+logger = logging.getLogger(__name__)
+
+
+def _resolve_dashboard(identifier):
+ """Look up a dashboard by id, uuid, or slug. Returns the model or None."""
+ # Deferred import — DashboardDAO transitively pulls models whose
+ # Column definitions need the Flask app to be initialized first.
+ # Importing inside the function lets tool registration succeed at
+ # module-load time without triggering "App not initialized yet".
+ from superset.daos.dashboard import DashboardDAO
+
+ try:
+ return DashboardDAO.get_by_id_or_slug(identifier)
+ except Exception: # pylint: disable=broad-except
+ # get_by_id_or_slug raises DashboardNotFoundError; treat all
+ # lookup-time errors as "not found" so the tool can return a
+ # structured DashboardError response instead of bubbling up.
+ return None
+
+
+@tool(
+ tags=["mutate"],
+ class_permission_name="Dashboard",
+ method_permission_name="write",
+ annotations=ToolAnnotations(
+ title="Update dashboard layout/theme/CSS",
+ readOnlyHint=False,
+ destructiveHint=False,
+ ),
+)
+async def update_dashboard(
+ request: UpdateDashboardRequest, ctx: Context = None
+) -> GenerateDashboardResponse | DashboardError:
+ """Patch an existing dashboard's layout, theme, or styling.
+
+ Companion to ``generate_dashboard`` for incremental edits. Accepts
+ the same layout/theme/CSS fields that ``generate_dashboard`` does, so
+ an LLM can:
+
+ - Set or replace ``position_json`` after auto-generation
+ - Apply brand ``label_colors`` and ``color_scheme`` via
+ ``json_metadata_overrides``
+ - Toggle ``cross_filters_enabled`` via ``json_metadata_overrides``
+ - Inject ``css`` to hide chrome on print-ready dashboards
+ - Update ``dashboard_title``, ``description``, ``slug``, ``published``
+
+ Only the fields explicitly passed are applied; other fields are left
+ unchanged. ``json_metadata_overrides`` is merged shallowly with the
+ existing json_metadata — pass only the keys you want to change.
+
+ Example::
+
+ update_dashboard(request={
+ "identifier": 42,
+ "json_metadata_overrides": {
+ "label_colors": {"Electronics": "#4C78A8"},
+ "cross_filters_enabled": False,
+ },
+ "css": ".header-controls {display: none;}",
+ })
+ """
+ await ctx.info(
+ "Updating dashboard: identifier=%s" % (request.identifier,)
+ )
+
+ dashboard = _resolve_dashboard(request.identifier)
+ if dashboard is None:
+ return DashboardError(
+ error=f"Dashboard not found: {request.identifier!r}",
+ error_type="DashboardNotFound",
+ )
+
+ changed_fields: list[str] = []
+
+ try:
+ with event_logger.log_context(action="mcp.update_dashboard.apply"):
+ if request.dashboard_title is not None:
+ dashboard.dashboard_title = request.dashboard_title
+ changed_fields.append("dashboard_title")
+
+ if request.description is not None:
+ dashboard.description = request.description
+ changed_fields.append("description")
+
+ if request.slug is not None:
+ # Empty string clears the slug; non-empty sets it.
+ dashboard.slug = request.slug or None
+ changed_fields.append("slug")
+
+ if request.published is not None:
+ dashboard.published = request.published
+ changed_fields.append("published")
+
+ if request.position_json is not None:
+ dashboard.position_json = json.dumps(request.position_json)
+ changed_fields.append("position_json")
+
+ if request.json_metadata_overrides is not None:
+ # Shallow merge over the existing metadata.
+ existing = (
+ json.loads(dashboard.json_metadata or "{}")
+ if dashboard.json_metadata
+ else {}
+ )
+ existing.update(request.json_metadata_overrides)
Review Comment:
**Suggestion:** Parsing existing dashboard metadata can raise
`ValueError`/`TypeError` when stored JSON is malformed, but only
`SQLAlchemyError` is handled, so this path can crash the tool instead of
returning a structured error. Guard JSON decoding failures and treat invalid
persisted metadata as `{}` (or return a controlled validation error) before
merging overrides. [type error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ update_dashboard crashes on dashboards with corrupted json_metadata.
- ⚠️ Operators cannot remediate broken dashboards via MCP updates.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. In the `update_dashboard` tool's apply block
(`superset/mcp_service/dashboard/tool/update_dashboard.py:139-148`), when
`request.json_metadata_overrides` is not None, the code shallow-merges
overrides into the
current metadata by computing `existing =
(json.loads(dashboard.json_metadata or \"{}\")
if dashboard.json_metadata else {})` at lines 141-145, then calling
`existing.update(request.json_metadata_overrides)` at line 146.
2. The `json` module imported at `update_dashboard.py:40` is
`superset.utils.json`, whose
`loads()` delegates to a JSON parser that raises a `json.JSONDecodeError`
(or related
`ValueError`/`TypeError`) if `dashboard.json_metadata` contains malformed
JSON (for
example, from manual database edits, previous buggy writes, or data
corruption).
3. The `try`/`except` around the main update logic at
`update_dashboard.py:116-190` only
catches `SQLAlchemyError` (lines 177-190). Any exception raised by
`json.loads()` at lines
141-145 is not a `SQLAlchemyError` and will therefore propagate out of
`update_dashboard()` as an unhandled exception rather than being converted
into a
structured `DashboardError`.
4. When a caller invokes `update_dashboard` against a dashboard whose
`json_metadata` is
malformed, the tool will crash at the `json.loads()` call instead of
returning a
controlled error or treating the metadata as `{}`, causing the FastMCP
client to see a
transport-level failure rather than a usable `DashboardError` response.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=0cbffcfc737847d18a77144c159c161d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=0cbffcfc737847d18a77144c159c161d&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/update_dashboard.py
**Line:** 141:146
**Comment:**
*Type Error: Parsing existing dashboard metadata can raise
`ValueError`/`TypeError` when stored JSON is malformed, but only
`SQLAlchemyError` is handled, so this path can crash the tool instead of
returning a structured error. Guard JSON decoding failures and treat invalid
persisted metadata as `{}` (or return a controlled validation error) before
merging overrides.
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%2F40399&comment_hash=a651d9c59c7dd8539c11ed8d30cab9fa08425fc9c920a0dd34b581373e56cb1b&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40399&comment_hash=a651d9c59c7dd8539c11ed8d30cab9fa08425fc9c920a0dd34b581373e56cb1b&reaction=dislike'>👎</a>
##########
superset/mcp_service/dashboard/tool/update_dashboard.py:
##########
@@ -0,0 +1,205 @@
+# 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.
+
+"""
+Update dashboard FastMCP tool
+
+This module contains the FastMCP tool for updating an existing dashboard's
+layout, theme, and styling. Companion to ``generate_dashboard`` for
+incremental edits without re-creating the dashboard.
+"""
+
+import logging
+
+from fastmcp import Context
+from sqlalchemy.exc import SQLAlchemyError
+from superset_core.mcp.decorators import tool, ToolAnnotations
+
+from superset.extensions import db, event_logger
+from superset.mcp_service.dashboard.schemas import (
+ DashboardError,
+ GenerateDashboardResponse,
+ UpdateDashboardRequest,
+ dashboard_serializer,
+)
+from superset.mcp_service.utils.url_utils import get_superset_base_url
+from superset.utils import json
+
+logger = logging.getLogger(__name__)
+
+
+def _resolve_dashboard(identifier):
+ """Look up a dashboard by id, uuid, or slug. Returns the model or None."""
+ # Deferred import — DashboardDAO transitively pulls models whose
+ # Column definitions need the Flask app to be initialized first.
+ # Importing inside the function lets tool registration succeed at
+ # module-load time without triggering "App not initialized yet".
+ from superset.daos.dashboard import DashboardDAO
+
+ try:
+ return DashboardDAO.get_by_id_or_slug(identifier)
+ except Exception: # pylint: disable=broad-except
+ # get_by_id_or_slug raises DashboardNotFoundError; treat all
+ # lookup-time errors as "not found" so the tool can return a
+ # structured DashboardError response instead of bubbling up.
+ return None
+
+
+@tool(
+ tags=["mutate"],
+ class_permission_name="Dashboard",
+ method_permission_name="write",
+ annotations=ToolAnnotations(
+ title="Update dashboard layout/theme/CSS",
+ readOnlyHint=False,
+ destructiveHint=False,
+ ),
+)
+async def update_dashboard(
+ request: UpdateDashboardRequest, ctx: Context = None
+) -> GenerateDashboardResponse | DashboardError:
+ """Patch an existing dashboard's layout, theme, or styling.
+
+ Companion to ``generate_dashboard`` for incremental edits. Accepts
+ the same layout/theme/CSS fields that ``generate_dashboard`` does, so
+ an LLM can:
+
+ - Set or replace ``position_json`` after auto-generation
+ - Apply brand ``label_colors`` and ``color_scheme`` via
+ ``json_metadata_overrides``
+ - Toggle ``cross_filters_enabled`` via ``json_metadata_overrides``
+ - Inject ``css`` to hide chrome on print-ready dashboards
+ - Update ``dashboard_title``, ``description``, ``slug``, ``published``
+
+ Only the fields explicitly passed are applied; other fields are left
+ unchanged. ``json_metadata_overrides`` is merged shallowly with the
+ existing json_metadata — pass only the keys you want to change.
+
+ Example::
+
+ update_dashboard(request={
+ "identifier": 42,
+ "json_metadata_overrides": {
+ "label_colors": {"Electronics": "#4C78A8"},
+ "cross_filters_enabled": False,
+ },
+ "css": ".header-controls {display: none;}",
+ })
+ """
+ await ctx.info(
+ "Updating dashboard: identifier=%s" % (request.identifier,)
+ )
+
+ dashboard = _resolve_dashboard(request.identifier)
+ if dashboard is None:
+ return DashboardError(
+ error=f"Dashboard not found: {request.identifier!r}",
+ error_type="DashboardNotFound",
+ )
+
+ changed_fields: list[str] = []
+
+ try:
+ with event_logger.log_context(action="mcp.update_dashboard.apply"):
Review Comment:
**Suggestion:** This update path never enforces object-level ownership
before mutating a dashboard. `DashboardDAO.get_by_id_or_slug` only checks read
access, so a user who can resolve the dashboard can still change
title/layout/CSS without the ownership guard that other dashboard mutation
flows use. Add an explicit ownership check (same pattern as other dashboard
write tools/commands) before applying any field updates. [security]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ update_dashboard allows non-owners to mutate existing dashboards.
- ⚠️ Violates REST API ownership enforcement for dashboard edits.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. The `update_dashboard` MCP tool in
`superset/mcp_service/dashboard/tool/update_dashboard.py:62-75` is annotated
with
`@tool(class_permission_name="Dashboard", method_permission_name="write")`.
RBAC
enforcement for MCP tools is implemented in
`superset/mcp_service/auth.py:91-138`, where
`check_tool_permission()` constructs a permission string `can_write` and
calls
`security_manager.can_access(permission_str, class_permission_name)`,
enforcing only
global `can_write` RBAC, not per-object ownership.
2. When a caller with `can_write` on `Dashboard` (passes
`check_tool_permission`) invokes
`update_dashboard(request=UpdateDashboardRequest(identifier=some_id,
dashboard_title="New
Title"))`, the tool resolves the dashboard via
`_resolve_dashboard(request.identifier)` at
`update_dashboard.py:107`, which calls `DashboardDAO.get_by_id_or_slug()`
(`superset/daos/dashboard.py:135-161`). That DAO method applies
`DashboardAccessFilter`
and calls `dashboard.raise_for_access()`, checking basic access but not
ownership.
3. After resolution, `update_dashboard()` proceeds into the `with
event_logger.log_context(action="mcp.update_dashboard.apply")` block at
`update_dashboard.py:116-153`, where it conditionally mutates
`dashboard.dashboard_title`,
`description`, `slug`, `published`, `position_json`, `json_metadata`, and
`css`, and then
commits via `db.session.commit()` at line 166, without any call to
`security_manager.raise_for_ownership` or similar object-level check.
4. By contrast, the canonical REST update path uses `UpdateDashboardCommand`
(`superset/commands/dashboard/update.py:52-79`), whose `validate()` method
calls
`security_manager.raise_for_ownership(self._model)` at lines 93-96, and the
MCP tool
`add_chart_to_existing_dashboard` also enforces ownership via
`_find_and_authorize_dashboard()` in
`superset/mcp_service/dashboard/tool/add_chart_to_existing_dashboard.py:60-101`.
This
discrepancy means any user who has RBAC `can_write Dashboard` but is not an
owner of a
specific dashboard can still change its title/layout/CSS through
`update_dashboard`, even
though REST-based updates would be denied for the same user and dashboard.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=2582a90528434c7e94f3807516653bdf&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
| [Fix in VSCode
Claude](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=2582a90528434c7e94f3807516653bdf&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/update_dashboard.py
**Line:** 107:117
**Comment:**
*Security: This update path never enforces object-level ownership
before mutating a dashboard. `DashboardDAO.get_by_id_or_slug` only checks read
access, so a user who can resolve the dashboard can still change
title/layout/CSS without the ownership guard that other dashboard mutation
flows use. Add an explicit ownership check (same pattern as other dashboard
write tools/commands) before applying any field updates.
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%2F40399&comment_hash=b9622e786200b3e94b18329541e1c4a8651492fe8fe86f3dfc155becdd6ee55e&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40399&comment_hash=b9622e786200b3e94b18329541e1c4a8651492fe8fe86f3dfc155becdd6ee55e&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]
