bito-code-review[bot] commented on code in PR #40333:
URL: https://github.com/apache/superset/pull/40333#discussion_r3293202535
##########
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:
<!-- Bito Reply -->
The suggestion is valid and should be applied. It addresses a critical
security issue where the current implementation silently skips authorization
checks for chart objects that do not implement `raise_for_access`. This allows
unauthorized users to add or delete tags on charts they shouldn't have access
to. By replacing the dynamic `hasattr` check with explicit permission checks
using `security_manager.raise_for_access(chart=...)`, the code will properly
enforce access controls for all object types, including charts. This change
aligns with the principle of least privilege and ensures that unsupported or
missing objects are treated as validation failures, improving the overall
security posture of the application.
**superset/commands/tag/create.py**
```
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()
```
##########
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:
<!-- Bito Reply -->
The suggestion to add dataset handling in `to_object_model` addresses a
security vulnerability (CWE-285) by ensuring proper access checks for datasets.
This change aligns with the pattern already used for dashboards, saved queries,
and charts. The implementation includes a lazy import to avoid circular
dependencies, which is a valid approach for this scenario.
**superset/commands/tag/utils.py**
```
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
```
--
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]
