codeant-ai-for-open-source[bot] commented on code in PR #40333:
URL: https://github.com/apache/superset/pull/40333#discussion_r3281947738
##########
superset/commands/tag/delete.py:
##########
@@ -85,6 +90,21 @@ def validate(self) -> None:
if exceptions:
raise TagInvalidError(exceptions=exceptions)
+ def _validate_object_access(
+ self, object_type: ObjectType, object_id: int, exceptions: list[Any]
+ ) -> None:
+ """Validate that the current user has access to the target object."""
+ try:
+ target_object = to_object_model(object_type, object_id)
+ if target_object and hasattr(target_object, "raise_for_access"):
+ target_object.raise_for_access()
Review Comment:
**Suggestion:** This delete-path access validation has the same bypass: if
the resolved object lacks `raise_for_access` (such as chart objects), no
permission check is executed and tag deletion can proceed for unauthorized
users. Use explicit object-type-aware access checks instead of `hasattr`, and
fail closed when the check cannot be performed. [security]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ TagRestApi.delete_object removes tags from unauthorized charts.
- ⚠️ Tag cleanup workflows bypass chart-level access controls.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Call the tag delete endpoint `TagRestApi.delete_object` at
`superset/tags/api.py:71-81`
via `DELETE /<int:object_type>/<int:object_id>/<tag>/` with `object_type`
corresponding to
`ObjectType.chart` (`superset/tags/models.py:80-88`) and a valid chart
`object_id`.
2. The view calls `DeleteTaggedObjectCommand(object_type, object_id,
tag).run()` at
`superset/tags/api.py:459-467`, which dispatches to
`DeleteTaggedObjectCommand.validate()`
at `superset/commands/tag/delete.py:45-55`.
3. In `validate`, once the tag and object type are validated
(`superset/commands/tag/delete.py:61-75`),
`_validate_object_access(object_type,
self._object_id, exceptions)` is invoked
(`superset/commands/tag/delete.py:75-78`), and
`_validate_object_access` resolves the target model via
`to_object_model(object_type,
object_id)` at `superset/commands/tag/delete.py:98`, which for charts
returns a `Slice`
via `ChartDAO.find_by_id` (`superset/commands/tag/utils.py:38-46`,
`superset/models/slice.py:68-76`).
4. `_validate_object_access` enforces authorization only when
`hasattr(target_object,
"raise_for_access")` (`superset/commands/tag/delete.py:99-100`), but `Slice`
lacks
`raise_for_access` (confirmed by `grep` and `superset/models/slice.py`
contents), so no
`SupersetSecurityException` is raised. `TagDAO.find_tagged_object(...)` and
ultimately
`TagDAO.delete_tagged_object(...)` (`superset/daos/tag.py:73-96`) execute,
allowing a user
who would be blocked by `security_manager.raise_for_access(chart=...)`
(`superset/security/manager.py:2854+`) to delete tags from an unauthorized
chart.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4230872655d044d3a482ef46b64f5f3f&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=4230872655d044d3a482ef46b64f5f3f&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/commands/tag/delete.py
**Line:** 99:100
**Comment:**
*Security: This delete-path access validation has the same bypass: if
the resolved object lacks `raise_for_access` (such as chart objects), no
permission check is executed and tag deletion can proceed for unauthorized
users. Use explicit object-type-aware access checks instead of `hasattr`, and
fail closed when the check cannot be performed.
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%2F40333&comment_hash=3bd64d50bfb735992a0f959f9433e49e6e962b551e7271c5e0ce0ccf0d7c3a32&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40333&comment_hash=3bd64d50bfb735992a0f959f9433e49e6e962b551e7271c5e0ce0ccf0d7c3a32&reaction=dislike'>👎</a>
##########
superset/commands/tag/create.py:
##########
@@ -60,9 +60,27 @@ def validate(self) -> None:
exceptions.append(
TagCreateFailedError(f"invalid object type
{self._object_type}")
)
+
+ # Validate user has access to the target object
+ if object_type:
+ self._validate_object_access(object_type, self._object_id,
exceptions)
+
if exceptions:
raise TagInvalidError(exceptions=exceptions)
+ def _validate_object_access(
+ self, object_type: ObjectType, object_id: int, exceptions: list[Any]
+ ) -> None:
+ """Validate that the current user has access to the target object."""
+ try:
+ target_object = to_object_model(object_type, object_id)
+ if target_object and hasattr(target_object, "raise_for_access"):
+ target_object.raise_for_access()
Review Comment:
**Suggestion:** The new access check silently skips authorization for object
models that do not implement `raise_for_access` (notably charts), so
unauthorized users can still add tags to those objects. Replace this dynamic
`hasattr` gate with explicit per-object permission checks (for example via
`security_manager.raise_for_access(chart=...)` / dashboard/query equivalents)
and treat unsupported or missing objects as validation failures. [security]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ TagRestApi.add_objects allows tagging unauthorized chart objects.
- ⚠️ Related_objects results may expose private chart associations.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Call the tag add endpoint `TagRestApi.add_objects` at
`superset/tags/api.py:15-61` via
`POST /<int:object_type>/<int:object_id>/` with `object_type` corresponding
to
`ObjectType.chart` (value 2 per `superset/tags/models.py:80-88`) and a valid
chart
`object_id`.
2. The view calls `CreateCustomTagCommand(object_type, object_id,
tags).run()` at
`superset/tags/api.py:58-62`, which in turn calls
`CreateCustomTagCommand.validate()` at
`superset/commands/tag/create.py:39-52`.
3. Inside `validate`, `_validate_object_access` is invoked when
`object_type` is truthy
(`superset/commands/tag/create.py:58-66`); `_validate_object_access`
resolves the target
model via `to_object_model(object_type, object_id)` at
`superset/commands/tag/create.py:76`, which for charts returns a `Slice`
instance
(`ObjectType.chart` maps to `ChartDAO.find_by_id` in
`superset/commands/tag/utils.py:38-46`, which returns
`superset/models/slice.py:68-76`).
4. `_validate_object_access` only enforces authorization if
`hasattr(target_object,
"raise_for_access")` is true (`superset/commands/tag/create.py:77-78`), but
`Slice` does
not define `raise_for_access` (verified by `superset/models/slice.py:1-200`
and a `grep`
for `def raise_for_access` showing only dashboard and SQL models). As a
result, no
`SupersetSecurityException` is raised and
`TagDAO.create_custom_tagged_objects(...)` at
`superset/daos/tag.py:46-71` proceeds to add tags to the chart even for a
user who would
be denied by `security_manager.raise_for_access(chart=...)`
(`superset/security/manager.py:2854+`), allowing unauthorized chart tagging.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=3f2d7fba1dd54f08b6fcc6f99d74d539&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=3f2d7fba1dd54f08b6fcc6f99d74d539&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/commands/tag/create.py
**Line:** 77:78
**Comment:**
*Security: The new access check silently skips authorization for object
models that do not implement `raise_for_access` (notably charts), so
unauthorized users can still add tags to those objects. Replace this dynamic
`hasattr` gate with explicit per-object permission checks (for example via
`security_manager.raise_for_access(chart=...)` / dashboard/query equivalents)
and treat unsupported or missing objects as validation failures.
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%2F40333&comment_hash=ef2f929dd61593713adc89921a23c8d0604c0d012880cd63634da9133b9a07b7&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40333&comment_hash=ef2f929dd61593713adc89921a23c8d0604c0d012880cd63634da9133b9a07b7&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]
