codeant-ai-for-open-source[bot] commented on code in PR #40853:
URL: https://github.com/apache/superset/pull/40853#discussion_r3389872187
##########
superset/mcp_service/chart/tool/update_chart.py:
##########
@@ -116,13 +124,24 @@ def _build_update_payload(
else chart.slice_name or generate_chart_name(parsed_config)
)
- return {
+ payload: dict[str, Any] = {
"slice_name": chart_name,
"viz_type": new_form_data["viz_type"],
"params": json.dumps(new_form_data),
# Clear stale query_context so get_chart_data uses the updated
params.
"query_context": None,
}
+ if request.dataset_id is not None:
+ payload["datasource_id"] = request.dataset_id
+ payload["datasource_type"] = "table"
+ return payload
+
+ # Dataset-only update: rebind chart to a different dataset without
changing viz
+ if request.dataset_id is not None:
+ return {
+ "datasource_id": request.dataset_id,
+ "datasource_type": "table",
+ }
Review Comment:
**Suggestion:** In the non-config path, when both `dataset_id` and
`chart_name` are provided, the function returns early with only datasource
fields and silently drops the rename. This creates inconsistent behavior
(preview can show the renamed title while save-without-preview won't persist
it). Include `slice_name` in the dataset-only payload when `chart_name` is
provided instead of discarding it. [incomplete implementation]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ Save path silently ignores requested chart title rename.
- ⚠️ Preview and saved chart titles become inconsistent for users.
- ⚠️ Dataset rebind plus rename workflow behaves unpredictably.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Call the MCP `update_chart` tool in
`superset/mcp_service/chart/tool/update_chart.py:339-400` with
`generate_preview=True`,
omitting `config` so `parsed_config` is `None`, but providing both
`dataset_id` (to
rebind) and `chart_name` (to rename), matching the documented dataset+name
update use case
in the PR description.
2. In this preview path, `_build_preview_form_data` at
`superset/mcp_service/chart/tool/update_chart.py:152-200` loads
`existing_form_data`,
computes `effective_dataset_id`, and because `parsed_config is None` but
`request.dataset_id` is not `None`, it skips `_missing_config_or_name_error`
at lines
187-188, copies `existing_form_data` into `merged`, then sets
`merged["slice_name"] =
request.chart_name` at lines 191-192 and `merged["datasource"] =
f"{effective_dataset_id}__table"` at lines 196-198, so the preview URL
reflects both the
new dataset and the new title.
3. Next, call `update_chart` again with the same `identifier`, `dataset_id`,
and
`chart_name` but with `generate_preview=False` (the "persist immediately"
mode described
in the docstring around lines 374-381 of `update_chart.py`), which routes
execution into
the non-preview save path at lines 72-119.
4. In this save path, `_build_update_payload` at
`superset/mcp_service/chart/tool/update_chart.py:98-149` sees `parsed_config
is None` and
skips the config branch, then hits the "Dataset-only update" block at lines
139-144: `if
request.dataset_id is not None: return {"datasource_id": request.dataset_id,
"datasource_type": "table"}`. This early return ignores
`request.chart_name`, so
`UpdateChartCommand` (invoked at lines 112-114) updates only
`datasource_id`/`datasource_type` and leaves `slice_name` unchanged, causing
the rename to
be silently dropped even though the earlier preview showed the new title.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=57595bfc047b4a97a7b66800ff4f7d78&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=57595bfc047b4a97a7b66800ff4f7d78&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/update_chart.py
**Line:** 139:144
**Comment:**
*Incomplete Implementation: In the non-config path, when both
`dataset_id` and `chart_name` are provided, the function returns early with
only datasource fields and silently drops the rename. This creates inconsistent
behavior (preview can show the renamed title while save-without-preview won't
persist it). Include `slice_name` in the dataset-only payload when `chart_name`
is provided instead of discarding it.
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%2F40853&comment_hash=acc9afb712abf74028f1329bfdc6a67c3418766e16b483acac53cdd2e29ff1fd&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40853&comment_hash=acc9afb712abf74028f1329bfdc6a67c3418766e16b483acac53cdd2e29ff1fd&reaction=dislike'>👎</a>
##########
superset/mcp_service/chart/tool/update_chart.py:
##########
@@ -178,26 +204,38 @@ def _validate_update_against_dataset(
parsed_config: Any,
form_data: dict[str, Any],
chart: Any,
+ dataset_id: int | None = None,
+ run_compile_check: bool = True,
) -> GenerateChartResponse | None:
"""Run Tier 1 (schema) + Tier 2 (compile) validation against the chart's
dataset. Returns ``None`` on success, or a :class:`GenerateChartResponse`
error envelope on failure that callers should return as-is.
+
+ When ``dataset_id`` is provided, validates against that dataset instead of
+ the chart's existing datasource (used when rebinding to a new dataset).
+ Pass ``run_compile_check=False`` to skip the Tier 2 live-query check (used
+ for dataset-only rebinds where no new chart config is provided).
"""
from superset.daos.dataset import DatasetDAO
- dataset = getattr(chart, "datasource", None)
- if dataset is None and getattr(chart, "datasource_id", None) is not None:
- dataset = DatasetDAO.find_by_id(chart.datasource_id)
+ if dataset_id is not None:
+ dataset = DatasetDAO.find_by_id(dataset_id)
Review Comment:
**Suggestion:** The new dataset lookup only checks existence and skips
dataset-level authorization, so a user can pass a `dataset_id` they cannot
access and still run validation against it. This can leak whether the dataset
exists and potentially schema-derived validation details before the write path
permission checks run. Add an explicit access check (same pattern used in other
chart MCP tools) immediately after resolving `dataset_id` and treat
unauthorized datasets as inaccessible. [security]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ update_chart tool can probe unauthorized dataset existence.
- ⚠️ Validation errors may leak schema or compile details.
- ⚠️ Live compile runs against datasets before access checks.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Invoke the MCP `update_chart` tool defined in
`superset/mcp_service/chart/tool/update_chart.py:339-400` with a request
containing an
accessible `identifier` (existing chart) plus a `dataset_id` pointing to a
different
dataset the caller cannot access, and a valid `config` payload so
`parsed_config` is
non-null.
2. Inside `update_chart`, the chart is loaded and the existing datasource is
checked via
`check_chart_data_access(chart)` at
`superset/mcp_service/chart/tool/update_chart.py:39-60`, which validates
only the chart's
current dataset, not the requested `dataset_id`, so the request continues
even if the
target dataset is unauthorized.
3. In the non-preview path (`generate_preview=False`), `update_chart` builds
`payload_or_error` and then calls
`_validate_update_against_dataset(parsed_config,
new_form_data, chart, dataset_id=request.dataset_id)` at
`superset/mcp_service/chart/tool/update_chart.py:87-94`, passing the
untrusted
`dataset_id` straight into validation.
4. `_validate_update_against_dataset` in
`superset/mcp_service/chart/tool/update_chart.py:203-249` executes `dataset =
DatasetDAO.find_by_id(dataset_id)` at lines 221-222 with no permission
check, then calls
`validate_and_compile(parsed_config, form_data, dataset,
run_compile_check=run_compile_check)` at lines 247-249 and returns either
success or
detailed validation errors in a `GenerateChartResponse`. Because the write
path's access
control (in `superset/commands/chart/update.py:144-151` using
`get_datasource_by_id` and
`security_manager.raise_for_access`) runs later, callers can probe which
`dataset_id`
values exist and observe schema-derived validation behavior on datasets they
are not
authorized to access.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=90731e726436427f886cf2c432cbb42e&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=90731e726436427f886cf2c432cbb42e&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/update_chart.py
**Line:** 221:222
**Comment:**
*Security: The new dataset lookup only checks existence and skips
dataset-level authorization, so a user can pass a `dataset_id` they cannot
access and still run validation against it. This can leak whether the dataset
exists and potentially schema-derived validation details before the write path
permission checks run. Add an explicit access check (same pattern used in other
chart MCP tools) immediately after resolving `dataset_id` and treat
unauthorized datasets as inaccessible.
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%2F40853&comment_hash=10813f3e70461892ed7a32732cada341492379934ff8d554ce448b0e9745540d&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40853&comment_hash=10813f3e70461892ed7a32732cada341492379934ff8d554ce448b0e9745540d&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]
