codeant-ai-for-open-source[bot] commented on code in PR #21769:
URL: https://github.com/apache/superset/pull/21769#discussion_r3460796309
##########
superset/queries/saved_queries/filters.py:
##########
@@ -82,10 +83,13 @@ class SavedQueryTagIdFilter(BaseTagIdFilter): # pylint:
disable=too-few-public-
class SavedQueryFilter(BaseFilter): # pylint: disable=too-few-public-methods
def apply(self, query: BaseQuery, value: Any) -> BaseQuery:
"""
- Filter saved queries to only those created by current user.
+ Filter saved queries to current user's queries unless the user can
+ access all queries.
:returns: flask-sqlalchemy query
"""
- return query.filter(
- SavedQuery.created_by == g.user # pylint:
disable=comparison-with-callable
- )
+ if not security_manager.can_access_all_queries():
+ query = query.filter(
+ SavedQuery.created_by == g.user # pylint:
disable=comparison-with-callable
+ )
+ return query
Review Comment:
**Suggestion:** This permission check is applied in the shared base filter,
so granting `all_query_access` now bypasses ownership checks for all
saved-query operations (read, update, delete, and export), not just list
visibility. That broadens write/export access beyond the PR intent ("see saved
queries") and can let users with this permission modify or remove other users'
saved queries. Scope this relaxation to list/read behavior only (or gate
write/export paths with stricter ownership/admin checks). [security]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ /api/v1/saved_query bulk delete removes others' queries.
- ❌ /api/v1/saved_query/export exposes non-owned saved SQL definitions.
- ⚠️ Permission all_query_access grants unintended write access to saved
queries.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Grant the `all_query_access` permission to a non-admin role as shown in
`superset/tests/integration_tests/sqllab_tests.py:10-23`, and assign that
role the
SavedQuery delete/export permissions used by `SavedQueryRestApi`
(`class_permission_name =
\"SavedQuery\"` and `method_permission_name` in
`superset/queries/saved_queries/api.py:69-81`), then attach this role to
user B.
2. As a different user A without `all_query_access`, create a saved query
via `POST
/api/v1/saved_query/` handled by `SavedQueryRestApi`
(`superset/queries/saved_queries/api.py:69-78`), which persists a
`SavedQuery` row with
`created_by` pointing to user A.
3. Log in as user B (with `all_query_access`) and invoke the bulk delete
endpoint `DELETE
/api/v1/saved_query/?q=...` implemented by `SavedQueryRestApi.bulk_delete` in
`superset/queries/saved_queries/api.py:134-184`, passing the ID of user A's
saved query in
the Rison-encoded `ids` list.
4. `bulk_delete` calls `DeleteSavedQueryCommand.run`
(`superset/commands/query/delete.py:9-13`), which uses
`SavedQueryDAO.find_by_ids`
(`superset/daos/query.py:86-87`); `BaseDAO.find_by_ids`
(`superset/daos/base.py:354`)
applies `_apply_base_filter` (`superset/daos/base.py:234`), instantiating
`SavedQueryFilter.apply`
(`superset/queries/saved_queries/filters.py:83-95`); because
`security_manager.can_access_all_queries()`
(`superset/security/manager.py:7-14`) returns
True for user B, the `created_by == g.user` filter is skipped, so user B's
delete request
succeeds against user A's saved query, showing ownership checks are bypassed
for delete
and likewise for export via `ExportSavedQueriesCommand`
(`superset/commands/query/export.py:3-5`,
`superset/commands/export/models.py:75-77`).
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d9b160787454463b8141540ebd3683b6&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=d9b160787454463b8141540ebd3683b6&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/queries/saved_queries/filters.py
**Line:** 91:95
**Comment:**
*Security: This permission check is applied in the shared base filter,
so granting `all_query_access` now bypasses ownership checks for all
saved-query operations (read, update, delete, and export), not just list
visibility. That broadens write/export access beyond the PR intent ("see saved
queries") and can let users with this permission modify or remove other users'
saved queries. Scope this relaxation to list/read behavior only (or gate
write/export paths with stricter ownership/admin checks).
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%2F21769&comment_hash=5c70f494e8addcd6e0f076ca877d81ed3fa164d22412db3074648c479a0223ff&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F21769&comment_hash=5c70f494e8addcd6e0f076ca877d81ed3fa164d22412db3074648c479a0223ff&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]
