codeant-ai-for-open-source[bot] commented on code in PR #40329:
URL: https://github.com/apache/superset/pull/40329#discussion_r3325850087
##########
superset/commands/tag/delete.py:
##########
@@ -85,6 +94,23 @@ 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 is None:
+ # Object may have been deleted; allow tag cleanup
+ return
Review Comment:
**Suggestion:** This early return skips permission checks whenever object
lookup returns `None`; combined with unsupported object-type resolution (e.g.,
dataset), this creates an authorization bypass where users can still remove tag
relations without access to the underlying object. Only allow this cleanup
bypass when you can prove the object type is supported and the object was truly
deleted, otherwise treat it as an access failure. [security]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Dataset tag delete endpoint ignores dataset-level access control.
- ⚠️ Non-owners can remove metadata tags from restricted datasets.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Create a dataset so it receives implicit tags, as shown in
`tests/integration_tests/tagging_tests.py:46-76`, which inserts a
`SqlaTable` and verifies
a `TaggedObject` row with `object_type == ObjectType.dataset` and the
dataset ID exists.
2. Call the delete-tag endpoint `DELETE
/api/v1/tag/<object_type>/<object_id>/<tag>/`
implemented by `TagRestApi.delete_object` in `superset/tags/api.py:410-461`,
passing
`object_type=ObjectType.dataset.value` and the dataset's ID and tag name;
this constructs
`DeleteTaggedObjectCommand(object_type, object_id, tag)` and calls `.run()`.
3. Inside `DeleteTaggedObjectCommand.validate` in
`superset/commands/tag/delete.py:59-95`,
`to_object_type(self._object_type)` returns `ObjectType.dataset` (since the
argument is
already an `ObjectType`, see `to_object_type` in
`superset/commands/tag/utils.py:29-35`),
so `_validate_object_access(object_type, self._object_id, exceptions)` is
invoked (line
80-81).
4. `_validate_object_access` at `superset/commands/tag/delete.py:97-105`
calls
`to_object_model(object_type, object_id)`, which uses `model_map` in
`superset/commands/tag/utils.py:47-51` that lacks `ObjectType.dataset`, so
it returns
`None` (lines 52-55); the `if target_object is None:` branch at lines
103-105 executes and
returns early without calling `raise_for_object_access`, after which
`TagDAO.find_tagged_object` and `TagDAO.delete_tagged_object` in
`superset/daos/tag.py:29-52, 100-115` remove the tag, meaning dataset tag
deletions occur
with no dataset-level access check, unlike dashboards/charts/queries which
go through
`raise_for_object_access` (`superset/commands/tag/utils.py:69-85`).
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d024eeacf5234ddbaff96f3f1306ebfd&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=d024eeacf5234ddbaff96f3f1306ebfd&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:** 102:105
**Comment:**
*Security: This early return skips permission checks whenever object
lookup returns `None`; combined with unsupported object-type resolution (e.g.,
dataset), this creates an authorization bypass where users can still remove tag
relations without access to the underlying object. Only allow this cleanup
bypass when you can prove the object type is supported and the object was truly
deleted, otherwise treat it as an access failure.
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%2F40329&comment_hash=6979dbe5a034e503487d7ee2eeaede6afb294a106e6274e466c793239da00e01&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40329&comment_hash=6979dbe5a034e503487d7ee2eeaede6afb294a106e6274e466c793239da00e01&reaction=dislike'>👎</a>
##########
superset/commands/tag/utils.py:
##########
@@ -38,10 +38,49 @@ def to_object_type(object_type: Union[ObjectType, int,
str]) -> Optional[ObjectT
def to_object_model(
object_type: ObjectType, object_id: int
) -> Optional[Union[Dashboard, SavedQuery, Slice]]:
- if ObjectType.dashboard == object_type:
- return DashboardDAO.find_by_id(object_id)
- if ObjectType.query == object_type:
- return SavedQueryDAO.find_by_id(object_id)
- if ObjectType.chart == object_type:
- return ChartDAO.find_by_id(object_id)
- return None
+ """Load a model instance by type and id.
+
+ Uses db.session.get() instead of DAO.find_by_id() to avoid DAO base
+ filters that require request context. Authorization is enforced by the
+ caller via raise_for_object_access() on the returned object.
+ """
+ model_map: dict[ObjectType, type] = {
+ ObjectType.dashboard: Dashboard,
+ ObjectType.query: SavedQuery,
+ ObjectType.chart: Slice,
+ }
Review Comment:
**Suggestion:** `to_object_model` does not handle `ObjectType.dataset`, even
though `ObjectType` includes it and tag commands accept it. This makes dataset
lookups always return `None`, which breaks integration: create-tag validation
now treats valid dataset targets as inaccessible and rejects tagging. Add
dataset model resolution in this mapping so dataset tagging follows the same
access flow as other object types. [incomplete implementation]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Dataset TagRestApi.add_objects cannot create custom dataset tags.
- ⚠️ UI workflows for tagging datasets return 422 errors.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Use the add-tags endpoint `POST /api/v1/tag/<object_type>/<object_id>/`
implemented by
`TagRestApi.add_objects` in `superset/tags/api.py:346-401`, which calls
`CreateCustomTagCommand(object_type, object_id, tags).run()` with
`object_type` typed as
`ObjectType` and `object_id` as `int`.
2. Pass a dataset as target by using `object_type=ObjectType.dataset.value`
(enum defined
in `superset/tags/models.py:80-88`), so the Flask route converts the integer
to
`ObjectType.dataset` and hands that to `CreateCustomTagCommand.__init__`
(`superset/commands/tag/create.py:37-42`).
3. In `CreateCustomTagCommand.validate`
(`superset/commands/tag/create.py:56-73`),
`object_type = to_object_type(self._object_type)` immediately returns the
same enum (see
`to_object_type` in `superset/commands/tag/utils.py:29-35`), and since it's
truthy,
`_validate_object_access(object_type, self._object_id, exceptions)` is
invoked at lines
68-71.
4. `_validate_object_access` at `superset/commands/tag/create.py:75-85` calls
`to_object_model(object_type, object_id)`, which uses `model_map` at
`superset/commands/tag/utils.py:47-51` that only handles `dashboard`,
`query`, and
`chart`; for `ObjectType.dataset` `model_cls` is `None`, so
`to_object_model` returns
`None` (lines 52-55), causing the `if target_object is None:` branch at
lines 81-85 to
append `TagCreateFailedError(f"Access denied for {object_type}
{object_id}")` and return,
leading `validate()` to raise `TagInvalidError` (lines 72-73) and
`TagRestApi.add_objects`
to return HTTP 422 (`superset/tags/api.py:407-408`), so any attempt to add
custom tags to
datasets through this endpoint is rejected even when the dataset exists and
the caller is
authorized.
```
</details>
[Fix in
Cursor](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=5021abd2d6354333af22017a31d2da54&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=5021abd2d6354333af22017a31d2da54&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/utils.py
**Line:** 47:51
**Comment:**
*Incomplete Implementation: `to_object_model` does not handle
`ObjectType.dataset`, even though `ObjectType` includes it and tag commands
accept it. This makes dataset lookups always return `None`, which breaks
integration: create-tag validation now treats valid dataset targets as
inaccessible and rejects tagging. Add dataset model resolution in this mapping
so dataset tagging follows the same access flow as other object types.
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%2F40329&comment_hash=f4f3ef717645a9a2a56f0f58f557d9c857b4b3ccb9af3da0922631883243d626&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40329&comment_hash=f4f3ef717645a9a2a56f0f58f557d9c857b4b3ccb9af3da0922631883243d626&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]
