fix: add missing ownership checks to tag, report-log, and dataset-schema endpoints - #43390
fix: add missing ownership checks to tag, report-log, and dataset-schema endpoints#43390rusackas wants to merge 12 commits into
Conversation
TagDAO.create_tag_relationship deleted every tag association absent from the submitted objects_to_tag set (or all associations when the field was empty/omitted) with no per-object authorization check, while the addition path already filtered by current_user_can_modify_object. Apply the same check to the deletion branch, looking up each about-to-be-removed object with skip_base_filter=True so associations on objects the caller cannot modify are left alone instead of being stripped.
DeleteTagsCommand.validate only checked that each named tag existed before TagDAO.delete_tags removed it, cascading every org-wide association with no ownership check. Require the caller to be an admin or the tag's creator, and refuse to delete system-generated tags (type:*, editor:*, favorited_by:*) through this route at all.
The REST counterpart of the "User Registrations" FAB view was never added to ADMIN_ONLY_VIEW_MENUS, so role sync granted its list/get/ delete permissions to stock Gamma and Alpha, exposing pending registrants' PII and letting non-admins cancel registrations. Add "UserRegistrationsRestAPI" to the admin-only allowlist and restrict the API to its GET/GET_LIST/INFO routes so write handlers are never registered at all, even if the allowlist entry regresses later.
ReportExecutionLogRestApi declared no base_filters, so the only scoping on its list/item routes was the caller-chosen schedule pk folded into the rison filters -- any role with generic ReportSchedule read could iterate every schedule's logs, including alert result values and database error messages for schedules it doesn't own. Add ReportExecutionLogFilter, scoping directly on ReportExecutionLog.report_schedule_id (mirroring ReportScheduleFilter on the sibling schedule API), and apply it as a base filter on both routes.
The drill_info-local UserSchema declared an email field, and DatasetDrillInfoSchema nests it unfiltered for both created_by and changed_by regardless of the endpoint's select_columns contract. Any user with dataset-read access -- Gamma is sufficient -- received maintainer email addresses. Drop email from the schema so it is never serialized, matching the dashboard/RLS user schemas which expose names only.
Code Review Agent Run #e6ba68Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #43390 +/- ##
==========================================
- Coverage 66.84% 66.79% -0.05%
==========================================
Files 2876 2876
Lines 164066 164445 +379
Branches 37863 37940 +77
==========================================
+ Hits 109666 109849 +183
- Misses 52226 52403 +177
- Partials 2174 2193 +19
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
|
The flagged issue is correct. The To resolve this, you should normalize the for tag_name in self._tags:
normalized_name = tag_name.strip()
tag = TagDAO.find_by_name(normalized_name)
# Validate tag exists
if not tag:
exceptions.append(TagNotFoundError(normalized_name))
continue
# ... (rest of the logic using normalized_name)I have checked the other comments on this PR. Would you like me to fetch all comments, validate them, and implement fixes for the rest as well? superset/commands/tag/delete.py |
test_delete_tags_command_requires_authorization logged in as GAMMA without first logging out of the preceding ADMIN session. Superset's login view is a no-op when a session is already authenticated, so the test kept running as ADMIN throughout, and the ownership check it was meant to exercise never actually saw a non-owner. Add the same self.logout() + self.login() pairing already used elsewhere in this test suite for mid-test user switches. Also normalize tag_name in DeleteTagsCommand.validate() before the TagDAO.find_by_name() lookup, matching the strip() TagDAO.delete_tags() already applies, so a name with surrounding whitespace is validated and deleted consistently (per automated review feedback on the PR). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Drop an over-specific rationale phrase from a regression-test docstring.
rusackas
added
the
merge-if-green
|
Bito Automatic Review Failed - Technical Failure |
TagRestApi exposed the FAB-generated DELETE /api/v1/tag/<pk> route unmodified, which deletes the row via the datamodel directly and never runs DeleteTagsCommand's admin-or-creator and system-tag checks that gate the bulk_delete route. Override the pk route to look the tag up and run it through DeleteTagsCommand instead, so both delete paths share the same validation rather than duplicating it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…queries ReportExecutionLogFilter bypassed its ownership scoping for anyone with can_access_all_datasources, which is also granted to stock Alpha (ALPHA_ONLY_PERMISSIONS includes all_datasource_access), letting a non-editor Alpha user read every schedule's evaluated alert values and database errors regardless of ownership. Key the bypass off can_access_all_queries instead, the admin-only permission this codebase already uses to gate the equivalent per-execution data on SQL Lab query history (superset.queries.filters.QueryFilter). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Restricting UserRegistrationsRestAPI to read-only routes also removed DELETE, but the User Registrations admin page still renders a delete action calling DELETE /api/v1/security/user_registrations/<id>, breaking that existing Admin workflow with a 405. Add RouteMethod.DELETE back to include_route_methods; POST/PUT stay excluded. The class remains fully gated Admin-only via ADMIN_ONLY_VIEW_MENUS, which keys off the view-menu name and therefore covers every permission on the class, not just specific ones, so restoring DELETE does not grant non-Admin roles anything. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
DatasetDrillInfoSchema.editors nested the shared SubjectResponseSchema, which includes secondary_label. For a user-backed Subject, user-subject synchronization (superset.subjects.sync.sync_user_subject) stores that user's email in secondary_label, so any dataset reader still received every editor's email through this field even after UserSchema dropped its own email field for created_by/changed_by. Add a drill_info-local editor schema that excludes secondary_label, mirroring the UserSchema precedent. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…rror subclasses DeleteTagsCommand.validate() composited TagDeleteFailedError instances (plain CommandException subclasses, no normalized_messages()) into the TagInvalidError it raises. That crashed with AttributeError whenever a caller invoked normalized_messages() on the result, as the new single-object DELETE /api/v1/tag/<pk> route does. Switch the composited "tag not found" and "cannot delete" cases to ValidationError-derived exceptions (reusing TagNotFoundValidationError and adding TagDeleteForbiddenValidationError) so the aggregation works for both the new route and bulk_delete. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Code Review Agent Run #528b83Actionable Suggestions - 0Filtered by Review RulesBito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
| # System-generated tags (type:*, editor:*, favorited_by:*) are | ||
| # maintained by Superset itself and must not be deletable through | ||
| # the bulk route. | ||
| if tag.type is not None and tag.type != TagType.custom: | ||
| exceptions.append( | ||
| TagDeleteForbiddenValidationError( | ||
| f"Tag {tag_name} is a system tag and cannot be deleted" | ||
| ) | ||
| ) | ||
| continue |
There was a problem hiding this comment.
Btw, while I was working on the Subject feature, I noticed these system generated tags are apparently not used anywhere(!). So while we're at it, I suggest we consider removing them all together if they're not used at all.
sha174n
left a comment
sha174n
left a comment
There was a problem hiding this comment.
Reviewed the ownership and field-exposure boundaries and it looks complete. The report-execution-log base filter is applied on both read routes (the API is GET/GET_LIST only), so a foreign id 404s, and the admin path is keyed on the admin-only permission. Tag mutations are all covered: additions validated in the create/update commands, the relationship-removal path now runs the modify check with skip_base_filter so an inaccessible object reaches the check rather than resolving to None, and both bulk and single-pk delete route through DeleteTagsCommand (admin-or-creator plus a system-tag guard). drill_info no longer includes email/secondary_label across created_by/changed_by/editors. One behavior note for reviewers: bulk/single tag delete now requires admin-or-creator, a deliberate tightening; per-object association delete still works for anyone with object access. LGTM.
4 participants
SUMMARY
TESTING INSTRUCTIONS
pytest tests/unit_tests/tags/ tests/unit_tests/security/ tests/unit_tests/reports/ tests/unit_tests/datasets/— new/extended tests per change, each verified to fail pre-fix and pass post-fix.ADDITIONAL INFORMATION