codeant-ai-for-open-source[bot] commented on code in PR #43066:
URL: https://github.com/apache/superset/pull/43066#discussion_r3768469718
##########
superset/mcp_service/chart/tool/get_chart_data.py:
##########
@@ -433,30 +440,30 @@ async def get_chart_data( # noqa: C901
)
logger.info("Getting data for chart %s: %s", chart.id,
chart.slice_name)
- # Skip the dataset RBAC pre-check for guests (see
guest_scope.is_guest_read).
- if not guest_scope.is_guest_read():
- validation_result = validate_chart_dataset(
- chart.datasource_id, check_access=True
+ # Validate the dataset for everyone. Guests skip the RBAC access check
+ # (governed by authorize_query below) but keep the existence check, so
a
+ # deleted dataset still returns the clean DatasetNotAccessible
contract.
+ validation_result = validate_chart_dataset(
+ chart.datasource_id, check_access=not guest_scope.is_guest_read()
+ )
Review Comment:
**Suggestion:** For guest requests this validates only the saved chart
datasource with `check_access=False`, while a supplied `form_data_key` can
replace the query context and its datasource before execution.
`guest_scope.authorize_query` overwrites `dashboardId` and `slice_id` but does
not pin the datasource to the resolved chart, so a cached form-data payload for
another datasource can be executed whenever the guest principal has any
matching datasource-level permission. Validate or reject the cached datasource
and ensure it matches the resolved chart before bypassing RBAC. [security]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Guest chart reads can execute cached queries against unrelated
datasources.
- ⚠️ Dataset validation and guest dashboard scoping become inconsistent.
- ❌ Sensitive data may be exposed through an unsaved form-data key.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=29ae3374cd754ae6a935ff247fda0f1d&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=29ae3374cd754ae6a935ff247fda0f1d&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<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_data.py
**Line:** 446:448
**Comment:**
*Security: For guest requests this validates only the saved chart
datasource with `check_access=False`, while a supplied `form_data_key` can
replace the query context and its datasource before execution.
`guest_scope.authorize_query` overwrites `dashboardId` and `slice_id` but does
not pin the datasource to the resolved chart, so a cached form-data payload for
another datasource can be executed whenever the guest principal has any
matching datasource-level permission. Validate or reject the cached datasource
and ensure it matches the resolved chart before bypassing RBAC.
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%2F43066&comment_hash=78282a39d31a05bdf43326dc2178c0023835398b5960fda35ca42f46d67fb43c&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43066&comment_hash=78282a39d31a05bdf43326dc2178c0023835398b5960fda35ca42f46d67fb43c&reaction=dislike'>👎</a>
##########
superset/utils/filters.py:
##########
@@ -78,7 +78,12 @@ def guest_embedded_dashboard_filter() ->
Optional[ColumnElement[bool]]:
# 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_)]
+ # A non-uuid id is a numeric dashboard id or a slug; route slugs to the
slug
+ # column instead of the int id column (which would raise a cast error). The
+ # data path (has_guest_access) stays the authorization gate.
+ non_uuid_ids = [id_ for id_ in ids if not is_uuid(id_)]
+ int_ids = [id_ for id_ in non_uuid_ids if str(id_).isdigit()]
+ slug_ids = [id_ for id_ in non_uuid_ids if not str(id_).isdigit()]
Review Comment:
**Suggestion:** Using `str(id_).isdigit()` does not match the identifier
contract used by `Dashboard.get`, whose `is_int` accepts any value convertible
by `int`, including whitespace-padded and explicitly signed numeric strings.
The token schema accepts these values as strings, and validation therefore
accepts a resource such as `+123` or ` 123`, but this code routes it to
`Dashboard.slug` instead of `Dashboard.id`, so the guest loses access to the
dashboard referenced by its otherwise valid token. Reuse the model's integer
classification helper rather than `isdigit()`. [type error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Formatted numeric resource ids fail dashboard scoping.
- ⚠️ Guest token validation and filtering classify ids differently.
- ⚠️ Embedded chart reads return not-found or access errors.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=078ce764466c4a9ebc005d448964c3a0&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=078ce764466c4a9ebc005d448964c3a0&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<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:** 84:86
**Comment:**
*Type Error: Using `str(id_).isdigit()` does not match the identifier
contract used by `Dashboard.get`, whose `is_int` accepts any value convertible
by `int`, including whitespace-padded and explicitly signed numeric strings.
The token schema accepts these values as strings, and validation therefore
accepts a resource such as `+123` or ` 123`, but this code routes it to
`Dashboard.slug` instead of `Dashboard.id`, so the guest loses access to the
dashboard referenced by its otherwise valid token. Reuse the model's integer
classification helper rather than `isdigit()`.
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%2F43066&comment_hash=62a134938e9bb70e4059967f7c4b7093ec5b32395f55b06452bbd3792f2671a9&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F43066&comment_hash=62a134938e9bb70e4059967f7c4b7093ec5b32395f55b06452bbd3792f2671a9&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]
