codeant-ai-for-open-source[bot] commented on code in PR #40497:
URL: https://github.com/apache/superset/pull/40497#discussion_r3319502758
##########
superset/views/api.py:
##########
@@ -89,6 +90,14 @@ def query_form_data(self) -> FlaskResponse:
if slice_id := request.args.get("slice_id"):
slc = db.session.query(Slice).filter_by(id=slice_id).one_or_none()
if slc:
+ # Normalise unauthorised access to the same 404 the modern
+ # ChartRestApi.get returns, so callers cannot distinguish
+ # "chart does not exist" from "chart exists but is denied"
+ # by status code.
+ try:
+ security_manager.raise_for_access(chart=slc)
Review Comment:
**Suggestion:** This access check is not equivalent to `ChartRestApi.get`:
`raise_for_access(chart=...)` allows chart owners even without dataset
permission, while `ChartRestApi.get` relies on `ChartFilter` (dataset-scoped
filtering). That mismatch can expose form data to users who cannot access the
chart through the main chart API. Use the same filtered lookup path (or enforce
datasource-level access only) to keep authorization behavior consistent.
[security]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Chart form_data accessible while main chart API denies.
- ⚠️ Bypasses dataset-level authorization for chart owners without
permissions.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. The main chart API `GET /api/v1/chart/<id_or_uuid>` is implemented in
`superset/charts/api.py:9-54` (`ChartRestApi.get`), which loads charts via
`ChartDAO.get_by_id_or_uuid(id_or_uuid)` (`superset/daos/chart.py:98-106`).
`ChartDAO.get_by_id_or_uuid` applies `ChartFilter` (line 102) before
returning a chart,
and `ChartFilter.apply` in `superset/charts/filters.py:102-113` joins the
dataset and
applies `get_dataset_access_filters(self.model)`, enforcing dataset-level
access
constraints without owner bypass.
2. The new legacy endpoint access check in `Api.query_form_data`
(superset/views/api.py:84-46) calls
`security_manager.raise_for_access(chart=slc)` at PR
hunk line 98 when a slice is found. The chart branch of `raise_for_access` in
`superset/security/manager.py:188-195` explicitly allows access if
`self.is_admin()` or
`self.is_owner(chart)` before checking `chart.datasource` and
`self.can_access_datasource(chart.datasource)`, meaning chart owners bypass
dataset
permission checks.
3. Existing tests `test_get_chart_no_data_access` at
`tests/integration_tests/charts/api_tests.py:1121-18` demonstrate that
`ChartRestApi.get`
returns 404 for a Gamma user hitting `api/v1/chart/<id>` for `"Girl Name
Cloud"` when they
lack `datasource_access`; this 404 comes from `ChartDAO.get_by_id_or_uuid`
raising
`ChartNotFoundError` after `ChartFilter` removes unauthorized charts.
4. If a Superset operator assigns such a user as an owner of that chart
(using the
`Slice.owners` relationship defined in `superset/models/slice.py:100-104`),
then for that
same user: (a) `GET /api/v1/chart/<id>` still goes through `ChartDAO` and
`ChartFilter`,
returning 404 due to missing dataset permissions; but (b) `GET
/api/v1/form_data/?slice_id=<id>` enters `query_form_data`, loads the
`Slice`, and
`security_manager.raise_for_access(chart=slc)` returns early because
`is_owner(chart)` is
true, so the method returns `slc.form_data` with HTTP 200. This demonstrates
a concrete
authorization mismatch where the legacy form_data endpoint exposes chart
configuration
that the main ChartRestApi intentionally hides.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=7f0808eeda5f441aaf77e9d6ac635fdf&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=7f0808eeda5f441aaf77e9d6ac635fdf&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/views/api.py
**Line:** 98:98
**Comment:**
*Security: This access check is not equivalent to `ChartRestApi.get`:
`raise_for_access(chart=...)` allows chart owners even without dataset
permission, while `ChartRestApi.get` relies on `ChartFilter` (dataset-scoped
filtering). That mismatch can expose form data to users who cannot access the
chart through the main chart API. Use the same filtered lookup path (or enforce
datasource-level access only) to keep authorization 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%2F40497&comment_hash=6dba763b7aefe001ddbd7148f4c35869859cbb8e8ed28dc5b0d71a43470724b8&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40497&comment_hash=6dba763b7aefe001ddbd7148f4c35869859cbb8e8ed28dc5b0d71a43470724b8&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]
