codeant-ai-for-open-source[bot] commented on code in PR #41753:
URL: https://github.com/apache/superset/pull/41753#discussion_r3537772365
##########
superset/mcp_service/chart/tool/get_chart_preview.py:
##########
@@ -1243,9 +1254,11 @@ def __init__(self, form_data: Dict[str, Any]):
logger.info("Generating preview for chart %s", getattr(chart, "id",
"NO_ID"))
logger.info("Chart datasource_id: %s", getattr(chart, "datasource_id",
"NONE"))
- # Validate the chart's dataset is accessible before generating preview
- # Skip validation for transient charts (no ID) - different data sources
- if getattr(chart, "id", None) is not None:
+ # Skip the dataset pre-check for transient charts (no ID) and for
guests
+ # (authorized via the dashboard context, not dataset RBAC).
+ from superset.mcp_service import guest_scope
+
+ if getattr(chart, "id", None) is not None and not
guest_scope.is_guest_read():
Review Comment:
**Suggestion:** The transient-chart skip check is incorrect:
unsaved/transient charts in this file are created with `id = 0`, so checking
`is not None` still runs dataset-access validation for them. That causes
transient previews to be treated as saved charts and can incorrectly return
dataset-access errors instead of proceeding through the intended transient
flow. Use a truthy/saved-ID check consistent with the rest of the MCP chart
tools so transient `id=0` charts are actually skipped. [incorrect condition
logic]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Unsaved chart previews via form_data_key fail dataset pre-check.
- ⚠️ Inconsistent access behavior between preview, info, data tools.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Call the MCP tool `get_chart_preview()` defined in
`superset/mcp_service/chart/tool/get_chart_preview.py:1433-1492` with a
request that has
`form_data_key` set and `identifier` unset or falsy, so
`_get_chart_preview_internal()`
(lines 1061-1076) is invoked.
2. Inside `_get_chart_preview_internal()`, hit the unsaved-chart branch at
lines
1084-1092, which constructs a transient chart class `TransientChartFromKey`
(lines
1107-1127) with `self.id = 0` (line 1109), or the later transient
`TransientChart` (lines
1190-1202) which also sets `self.id = 0` (line 1193).
3. After the chart is resolved, `_get_chart_preview_internal()` reaches the
dataset
pre-check at lines 1257-1262: it imports `guest_scope` (lines 1259-1260) and
evaluates `if
getattr(chart, "id", None) is not None and not guest_scope.is_guest_read():`
(line 1261).
For a normal authenticated (non-guest) user, `guest_scope.is_guest_read()`
returns `False`
(see `superset/mcp_service/guest_scope.py:35-41`), and `getattr(chart, "id",
None)`
returns `0`, so the condition is `True` even though the chart is transient.
4. Because the condition passes, `validate_chart_dataset(chart,
check_access=True)` is
called (line 1262) and, if the dataset access check fails, the function
returns a
`ChartError` with type `DatasetNotAccessible` (lines 1263-1272) instead of
proceeding to
the preview strategy. This is inconsistent with the intended “skip for
transient charts”
behavior and with `_validate_chart_dataset_access()` in
`superset/mcp_service/chart/tool/get_chart_info.py:100-115`, which
explicitly skips
dataset validation when `result.id` is falsy, meaning transient `id=0`
charts are treated
as saved only in this preview path.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=1679f790a181479d93f5ee48c99e64f5&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=1679f790a181479d93f5ee48c99e64f5&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/get_chart_preview.py
**Line:** 1261:1261
**Comment:**
*Incorrect Condition Logic: The transient-chart skip check is
incorrect: unsaved/transient charts in this file are created with `id = 0`, so
checking `is not None` still runs dataset-access validation for them. That
causes transient previews to be treated as saved charts and can incorrectly
return dataset-access errors instead of proceeding through the intended
transient flow. Use a truthy/saved-ID check consistent with the rest of the MCP
chart tools so transient `id=0` charts are actually skipped.
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%2F41753&comment_hash=d00507d6c787914c263a8192976b3367a97136c613cbdf834f4a3270f389187b&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41753&comment_hash=d00507d6c787914c263a8192976b3367a97136c613cbdf834f4a3270f389187b&reaction=dislike'>👎</a>
##########
superset/utils/filters.py:
##########
@@ -41,3 +41,43 @@ def get_dataset_access_filters(
base_model.schema_perm.in_(schema_perms),
*args,
)
+
+
+def guest_embedded_dashboard_filter() -> Optional[Any]:
+ """SQLAlchemy condition matching dashboards embedded via the current guest
+ token's resources, or None when the user is not an embedded guest.
+
+ Used by the chart access filter to scope an embedded guest to its token's
+ dashboards (and their charts), mirroring how DashboardAccessFilter already
+ scopes dashboards.
+ """
+ # pylint: disable=import-outside-toplevel
+ from superset import is_feature_enabled, security_manager
+ from superset.models.dashboard import Dashboard, is_uuid
+ from superset.models.embedded_dashboard import EmbeddedDashboard
+ from superset.security.guest_token import GuestTokenResourceType
+
+ if not is_feature_enabled("EMBEDDED_SUPERSET"):
+ return None
+ guest = security_manager.get_current_guest_user_if_guest()
+ if guest is None:
+ return None
+ ids: list[Any] = [
+ r["id"]
+ for r in guest.resources
+ if r["type"] == GuestTokenResourceType.DASHBOARD.value
+ ]
+ if not ids:
+ return None
+ # TODO (embedded): only use the uuid filter once uuids are rolled out
+ # A guest token may mix uuid and int dashboard ids during the uuid rollout.
+ # Route each id kind to its own column and OR them — a plain int sent to
the
+ # uuid-typed column would raise a bind/type error.
+ uuid_ids = [id_ for id_ in ids if is_uuid(id_)]
+ int_ids = [id_ for id_ in ids if not is_uuid(id_)]
+ conditions = []
+ if uuid_ids:
+
conditions.append(Dashboard.embedded.any(EmbeddedDashboard.uuid.in_(uuid_ids)))
+ if int_ids:
+ conditions.append(Dashboard.id.in_(int_ids))
Review Comment:
**Suggestion:** The legacy integer-ID branch allows any dashboard ID from
the guest token without requiring that the dashboard is actually
embedded-enabled. Since guest token resource validation accepts existing
dashboard IDs, this can scope guests to non-embedded dashboards/charts when
integer IDs are used. Add an embedded constraint for the integer-ID path (or
resolve integer IDs through embedded-dashboard mappings) so guest scoping
remains limited to embedded dashboards only. [security]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Guest can read charts on non-embedded dashboards in token.
- ⚠️ Embedded assistant scope broader than embedded dashboard configuration.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Issue a guest token via the `/security/guest_token` API using
`GuestTokenCreateSchema`
and `ResourceSchema` in `superset/security/api.py:60-77`, with `resources`
containing an
entry of type `dashboard` whose `id` is a legacy integer dashboard ID `D.id`
for a
dashboard `D` that has no embedded configuration (`D.embedded` is empty per
`superset/models/dashboard.py:203-207` and
`superset/models/embedded_dashboard.py:41-56`).
Token creation succeeds because
`security_manager.validate_guest_token_resources()` (lines
35-52 in `superset/security/manager.py`) only checks that
`Dashboard.get(str(resource["id"]))` or
`EmbeddedDashboardDAO.find_by_id(...)` exists, not
that `dashboard.embedded` is non-empty.
2. When a request authenticated with this guest token is processed,
`security_manager.get_guest_user_from_request()` (lines 97-130 in
`superset/security/manager.py`) builds a `GuestUser` whose `resources` list
mirrors the
token payload (see `GuestUser.__init__` in
`superset/security/guest_token.py:188-197`), so
`guest.resources` includes the integer dashboard ID `D.id` even though `D`
is not
embedded-enabled.
3. For this guest request, `guest_embedded_dashboard_filter()` in
`superset/utils/filters.py:46-83` is invoked from `ChartFilter.apply()`
(lines 18-25 in
`superset/charts/filters.py`). Because `EMBEDDED_SUPERSET` is enabled and
`security_manager.get_current_guest_user_if_guest()` returns the guest
(lines 60-63), the
helper collects `ids` from `guest.resources` where `type ==
GuestTokenResourceType.DASHBOARD.value` (lines 65-69), splits them into
`uuid_ids` and
`int_ids` (lines 76-77), and since only integer IDs are present, returns the
condition
`Dashboard.id.in_(int_ids)` (lines 78-82) without adding any
`Dashboard.embedded`
constraint.
4. When the MCP tool `get_chart_info()` is called for a chart `C` that is on
dashboard `D`
(so `C` appears in `D.slices`), `ModelGetInfoCore` in
`superset/mcp_service/chart/tool/get_chart_info.py:281-331` resolves the
chart via
`ChartDAO` with `ChartFilter` applied (`superset/daos/chart.py:20-25`).
`ChartFilter.apply()` sees a non-None `guest_dashboards` condition and
immediately returns
`query.filter(self.model.dashboards.any(guest_dashboards))` (lines 23-25 in
`superset/charts/filters.py`), which matches `C` because it is linked to
dashboard `D`
whose `id` is in `int_ids`, even though `D.embedded` is empty.
`_validate_chart_dataset_access()` then skips dataset-permission checks for
guests
(`guest_scope.is_guest_read()` early-return at
`superset/mcp_service/chart/tool/get_chart_info.py:111-115`), and
`has_guest_access()` is
never consulted here, so the guest receives `ChartInfo` for charts on a
non-embedded
dashboard, in contrast to `security_manager.has_guest_access()` (lines 4-24
in
`superset/security/manager.py`) which would reject access when `not
dashboard.embedded`.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=0e5539ab56b14b108440a86b27d1256c&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=0e5539ab56b14b108440a86b27d1256c&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/utils/filters.py
**Line:** 81:82
**Comment:**
*Security: The legacy integer-ID branch allows any dashboard ID from
the guest token without requiring that the dashboard is actually
embedded-enabled. Since guest token resource validation accepts existing
dashboard IDs, this can scope guests to non-embedded dashboards/charts when
integer IDs are used. Add an embedded constraint for the integer-ID path (or
resolve integer IDs through embedded-dashboard mappings) so guest scoping
remains limited to embedded dashboards only.
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%2F41753&comment_hash=876ec6f908d8977395bcb54f52c98fbeb49f8eeeb588438b8443cee52f7df07d&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41753&comment_hash=876ec6f908d8977395bcb54f52c98fbeb49f8eeeb588438b8443cee52f7df07d&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]
