bito-code-review[bot] commented on code in PR #40333:
URL: https://github.com/apache/superset/pull/40333#discussion_r3282053175
##########
superset/databases/api.py:
##########
@@ -1328,14 +1328,21 @@ def related_objects(self, pk: int) -> Response:
}
for chart in data["charts"]
]
+ # Filter dashboards to only include those the user can access
+ accessible_dashboards = [
+ dashboard
+ for dashboard in data["dashboards"]
+ if security_manager.can_access_dashboard(dashboard)
+ ]
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>CWE-200: Inconsistent Security Filtering</b></div>
<div id="fix">
The diff adds security filtering for dashboards but charts remain
unfiltered. Users can infer the existence of inaccessible charts through the
response count and metadata, creating an information disclosure vulnerability.
Apply the same `can_access_chart()` check to the charts list comprehension to
maintain consistency with the dashboard fix. (See also:
[CWE-200](https://cwe.mitre.org/data/definitions/200.html))
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
Add equivalent security filtering for charts using
`security_manager.can_access_chart(chart)` before including them in the
response, matching the dashboard filtering pattern.
```
</div>
</details>
</div>
<small><i>Code Review Run #a54e91</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
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)
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>CWE-285: Missing dataset access check</b></div>
<div id="fix">
When `object_type` is `ObjectType.dataset`, `to_object_model` returns `None`
because dataset handling is missing. This causes `_validate_object_access` to
skip the `raise_for_access()` check silently, allowing unauthorized tag
deletion from datasets. Add dataset case: `if ObjectType.dataset ==
object_type: return DatasetDAO.find_by_id(object_id)` (See also:
[CWE-285](https://cwe.mitre.org/data/definitions/285.html))
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
--- superset/commands/tag/utils.py (lines 38-47) ---
38: def to_object_model(
39: object_type: ObjectType, object_id: int
40: ) -> Optional[Union[Dashboard, SavedQuery, Slice]]:
41: if ObjectType.dashboard == object_type:
42: return DashboardDAO.find_by_id(object_id)
43: if ObjectType.query == object_type:
44: return SavedQueryDAO.find_by_id(object_id)
45: if ObjectType.chart == object_type:
46: return ChartDAO.find_by_id(object_id)
47: + if ObjectType.dataset == object_type:
48: + from superset.daos.dataset import DatasetDAO
49: + return DatasetDAO.find_by_id(object_id)
50: return None
```
</div>
</details>
</div>
<small><i>Code Review Run #a54e91</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
##########
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"):
Review Comment:
<div>
<div id="suggestion">
<div id="issue"><b>Missing dataset access check</b></div>
<div id="fix">
When tagging a dataset, `to_object_model` returns `None` because
`ObjectType.dataset` is not handled. This causes the access check on line 77 to
be skipped silently, allowing unauthorized tagging of datasets. The
`to_object_model` function in `utils.py` only handles `dashboard`, `query`, and
`chart`.
</div>
<details>
<summary>
<b>Code suggestion</b>
</summary>
<blockquote>Check the AI-generated fix before applying</blockquote>
<div id="code">
```
--- superset/commands/tag/utils.py ---
+++ superset/commands/tag/utils.py
@@ -17,6 +17,7 @@
from typing import Optional, Union
+from superset.daos.dataset import DatasetDAO
from superset.daos.chart import ChartDAO
from superset.daos.dashboard import DashboardDAO
from superset.daos.query import SavedQueryDAO
@@ -44,4 +45,6 @@ def to_object_model(
return ChartDAO.find_by_id(object_id)
if ObjectType.dataset == object_type:
return DatasetDAO.find_by_id(object_id)
return None
```
</div>
</details>
</div>
<small><i>Code Review Run #a54e91</i></small>
</div>
---
Should Bito avoid suggestions like this for future reviews? (<a
href=https://alpha.bito.ai/home/ai-agents/review-rules>Manage Rules</a>)
- [ ] Yes, avoid them
--
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]
