From 599697b383125615172bcd34f9c9f220ee0ae027 Mon Sep 17 00:00:00 2001 From: Superset Dev Date: Fri, 21 Aug 2026 00:37:17 -0700 Subject: [PATCH 01/12] fix(tags): check per-object access before deleting tag associations 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. --- superset/daos/tag.py | 18 ++- tests/unit_tests/tags/commands/update_test.py | 152 ++++++++++++++++++ 2 files changed, 169 insertions(+), 1 deletion(-) diff --git a/superset/daos/tag.py b/superset/daos/tag.py index 943132071b3b..139be7633475 100644 --- a/superset/daos/tag.py +++ b/superset/daos/tag.py @@ -21,7 +21,11 @@ from sqlalchemy.exc import NoResultFound from superset.commands.tag.exceptions import TagNotFoundError -from superset.commands.tag.utils import to_object_type +from superset.commands.tag.utils import ( + current_user_can_modify_object, + to_object_model, + to_object_type, +) from superset.daos.base import BaseDAO from superset.daos.chart import ChartDAO from superset.daos.dashboard import DashboardDAO @@ -372,6 +376,18 @@ def create_tag_relationship( if not bulk_create: # delete relationships that aren't retained from single tag create for object_type, object_id in tagged_objects_to_delete: + # Only remove associations from objects the current user may + # modify, mirroring the per-object check applied to additions. + # Look the object up bypassing the access base filter so an + # inaccessible object reaches the check instead of resolving + # to None and having its association deleted unchecked. + model = to_object_model( + object_type, # type: ignore + object_id, + skip_base_filter=True, + ) + if model and not current_user_can_modify_object(model): + continue # delete objects that were removed TagDAO.delete_tagged_object( object_type, # type: ignore diff --git a/tests/unit_tests/tags/commands/update_test.py b/tests/unit_tests/tags/commands/update_test.py index edd41991fce1..52323edc99a4 100644 --- a/tests/unit_tests/tags/commands/update_test.py +++ b/tests/unit_tests/tags/commands/update_test.py @@ -264,3 +264,155 @@ def test_update_command_remove_all_tagged_objects( assert ( len(db.session.query(TaggedObject).filter_by(tag_id=updated_tag.id).all()) == 0 ) + + +def test_update_command_skips_removal_of_inaccessible_objects( + session_with_data: Session, mocker: MockerFixture +): + """Associations on objects the user cannot modify must survive an update. + + Regression test: the deletion branch of ``create_tag_relationship`` + removed every association absent from the submitted set with no + per-object check, so a low-privilege user could strip a tag from + objects they are not allowed to modify (or from every object, by + sending an empty ``objects_to_tag``). + """ + from superset.commands.tag.create import CreateCustomTagWithRelationshipsCommand + from superset.commands.tag.update import UpdateTagCommand + from superset.daos.tag import TagDAO + from superset.models.dashboard import Dashboard + from superset.models.slice import Slice + from superset.tags.models import ObjectType, TaggedObject + + dashboard = db.session.query(Dashboard).first() + chart = db.session.query(Slice).first() + + mocker.patch( + "superset.security.SupersetSecurityManager.is_admin", return_value=True + ) + mocker.patch("superset.daos.chart.ChartDAO.find_by_id", return_value=chart) + mocker.patch( + "superset.daos.dashboard.DashboardDAO.find_by_id", return_value=dashboard + ) + + # An admin tags both a dashboard and a chart + CreateCustomTagWithRelationshipsCommand( + data={ + "name": "test_tag", + "objects_to_tag": [ + (ObjectType.dashboard, dashboard.id), + (ObjectType.chart, chart.id), + ], + } + ).run() + + tag = TagDAO.find_by_name("test_tag") + assert len(tag.objects) == 2 + + # A non-admin who may modify the chart but not the dashboard submits + # only the chart: the dashboard association must not be deleted. + mocker.patch( + "superset.security.SupersetSecurityManager.is_admin", return_value=False + ) + + def can_modify(model): + return isinstance(model, Slice) + + mocker.patch( + "superset.commands.tag.update.current_user_can_modify_object", + side_effect=can_modify, + ) + mocker.patch( + "superset.daos.tag.current_user_can_modify_object", + side_effect=can_modify, + ) + + UpdateTagCommand( + tag.id, + { + "name": "test_tag", + "description": "test_description", + "objects_to_tag": [(ObjectType.chart, chart.id)], + }, + ).run() + + remaining = { + (obj.object_type, obj.object_id) + for obj in db.session.query(TaggedObject).filter_by(tag_id=tag.id).all() + } + assert (ObjectType.dashboard, dashboard.id) in remaining + assert (ObjectType.chart, chart.id) in remaining + + +def test_update_command_empty_objects_to_tag_only_removes_accessible( + session_with_data: Session, mocker: MockerFixture +): + """An empty/omitted objects_to_tag must not mass-delete every association. + + Regression test for the finding's exact exploit payload: PUT with no + objects_to_tag used to be treated as "delete every current association" + with zero per-object check. Now each deletion is access-checked, so + objects the caller cannot modify keep their association. + """ + from superset.commands.tag.create import CreateCustomTagWithRelationshipsCommand + from superset.commands.tag.update import UpdateTagCommand + from superset.daos.tag import TagDAO + from superset.models.dashboard import Dashboard + from superset.models.slice import Slice + from superset.tags.models import ObjectType, TaggedObject + + dashboard = db.session.query(Dashboard).first() + chart = db.session.query(Slice).first() + + mocker.patch( + "superset.security.SupersetSecurityManager.is_admin", return_value=True + ) + mocker.patch("superset.daos.chart.ChartDAO.find_by_id", return_value=chart) + mocker.patch( + "superset.daos.dashboard.DashboardDAO.find_by_id", return_value=dashboard + ) + + CreateCustomTagWithRelationshipsCommand( + data={ + "name": "test_tag", + "objects_to_tag": [ + (ObjectType.dashboard, dashboard.id), + (ObjectType.chart, chart.id), + ], + } + ).run() + + tag = TagDAO.find_by_name("test_tag") + assert len(tag.objects) == 2 + + mocker.patch( + "superset.security.SupersetSecurityManager.is_admin", return_value=False + ) + + def can_modify(model): + return isinstance(model, Slice) + + mocker.patch( + "superset.commands.tag.update.current_user_can_modify_object", + side_effect=can_modify, + ) + mocker.patch( + "superset.daos.tag.current_user_can_modify_object", + side_effect=can_modify, + ) + + UpdateTagCommand( + tag.id, + { + "name": "test_tag", + "description": "test_description", + "objects_to_tag": [], + }, + ).run() + + remaining = { + (obj.object_type, obj.object_id) + for obj in db.session.query(TaggedObject).filter_by(tag_id=tag.id).all() + } + assert (ObjectType.dashboard, dashboard.id) in remaining + assert (ObjectType.chart, chart.id) not in remaining From 6ee48204a5b0e9d9b434574ae609656a654b2f9a Mon Sep 17 00:00:00 2001 From: Superset Dev Date: Fri, 21 Aug 2026 00:42:26 -0700 Subject: [PATCH 02/12] fix(tags): require ownership before bulk-deleting tags by name 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. --- superset/commands/tag/delete.py | 35 +++- .../integration_tests/tags/commands_tests.py | 47 +++++- tests/unit_tests/tags/commands/delete_test.py | 151 ++++++++++++++++++ 3 files changed, 226 insertions(+), 7 deletions(-) create mode 100644 tests/unit_tests/tags/commands/delete_test.py diff --git a/superset/commands/tag/delete.py b/superset/commands/tag/delete.py index dfc5686d497d..0a37bfcee501 100644 --- a/superset/commands/tag/delete.py +++ b/superset/commands/tag/delete.py @@ -30,7 +30,7 @@ from superset.commands.tag.utils import to_object_model, to_object_type from superset.daos.tag import TagDAO from superset.exceptions import SupersetSecurityException -from superset.tags.models import ObjectType +from superset.tags.models import ObjectType, TagType from superset.utils.decorators import on_error, transaction from superset.views.base import DeleteMixin @@ -134,10 +134,33 @@ def run(self) -> None: TagDAO.delete_tags(self._tags) def validate(self) -> None: - exceptions = [] - # Validate tag exists - for tag in self._tags: - if not TagDAO.find_by_name(tag): - exceptions.append(TagNotFoundError(tag)) + exceptions: list[TagNotFoundError | TagDeleteFailedError] = [] + for tag_name in self._tags: + tag = TagDAO.find_by_name(tag_name) + # Validate tag exists + if not tag: + exceptions.append(TagNotFoundError(tag_name)) + continue + # 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( + TagDeleteFailedError( + f"Tag {tag_name} is a system tag and cannot be deleted" + ) + ) + continue + # Deleting a tag cascades removal of all of its associations + # org-wide, so existence is not enough: require the user to be an + # admin or the tag's creator (the single-association route + # enforces per-object access in DeleteTaggedObjectCommand). + if not ( + security_manager.is_admin() + or (tag.created_by and tag.created_by == security_manager.current_user) + ): + exceptions.append( + TagDeleteFailedError(f"Access denied to tag {tag_name}") + ) if exceptions: raise TagInvalidError(exceptions=exceptions) diff --git a/tests/integration_tests/tags/commands_tests.py b/tests/integration_tests/tags/commands_tests.py index 055c51f09adc..a35c756364a2 100644 --- a/tests/integration_tests/tags/commands_tests.py +++ b/tests/integration_tests/tags/commands_tests.py @@ -33,13 +33,14 @@ from superset.commands.importers.exceptions import IncorrectVersionError # noqa: F401 from superset.commands.tag.create import CreateCustomTagCommand from superset.commands.tag.delete import DeleteTaggedObjectCommand, DeleteTagsCommand +from superset.commands.tag.exceptions import TagInvalidError from superset.connectors.sqla.models import SqlaTable # noqa: F401 from superset.models.core import Database # noqa: F401 from superset.models.dashboard import Dashboard from superset.models.slice import Slice # noqa: F401 from superset.tags.models import ObjectType, Tag, TaggedObject, TagType from tests.integration_tests.base_tests import SupersetTestCase -from tests.integration_tests.constants import ADMIN_USERNAME +from tests.integration_tests.constants import ADMIN_USERNAME, GAMMA_USERNAME from tests.integration_tests.fixtures.importexport import ( chart_config, # noqa: F401 dashboard_config, # noqa: F401 @@ -127,6 +128,50 @@ def test_delete_tags_command(self): tags = db.session.query(Tag).filter(Tag.name.in_(example_tags)) assert tags.count() == 0 + @pytest.mark.usefixtures("load_world_bank_dashboard_with_slices") + @pytest.mark.usefixtures("with_tagging_system_feature") + def test_delete_tags_command_requires_authorization(self): + """ + Regression test: DeleteTagsCommand used to check only that each named + tag existed, letting any Gamma user bulk-delete tags (and every + association they carry) they neither created nor own. It must now + require admin-or-creator, and must refuse system-generated tags + outright for everyone. + """ + example_dashboard = ( + db.session.query(Dashboard) + .filter_by(dashboard_title="World Bank's Data") + .one() + ) + self.login(ADMIN_USERNAME) + example_tags = {"delete tag authz example"} + CreateCustomTagCommand( + ObjectType.dashboard.value, example_dashboard.id, example_tags + ).run() + + system_tag = Tag(name="type:delete_tag_authz", type=TagType.type) + db.session.add(system_tag) + db.session.commit() + + try: + # a non-admin who is not the tag's creator may not delete it + self.login(GAMMA_USERNAME) + with pytest.raises(TagInvalidError): + DeleteTagsCommand(example_tags).run() + assert db.session.query(Tag).filter(Tag.name.in_(example_tags)).count() == 1 + + # system-generated tags are refused outright, even for an admin + self.login(ADMIN_USERNAME) + with pytest.raises(TagInvalidError): + DeleteTagsCommand([system_tag.name]).run() + assert db.session.query(Tag).filter_by(name=system_tag.name).count() == 1 + finally: + # cleanup + self.login(ADMIN_USERNAME) + DeleteTagsCommand(example_tags).run() + db.session.query(Tag).filter_by(name=system_tag.name).delete() + db.session.commit() + # test delete tagged objects command class TestDeleteTaggedObjectCommand(SupersetTestCase): diff --git a/tests/unit_tests/tags/commands/delete_test.py b/tests/unit_tests/tags/commands/delete_test.py new file mode 100644 index 000000000000..4e9803f2c296 --- /dev/null +++ b/tests/unit_tests/tags/commands/delete_test.py @@ -0,0 +1,151 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from unittest.mock import PropertyMock + +import pytest +from pytest_mock import MockerFixture +from sqlalchemy.orm.session import Session + +from superset import db + + +@pytest.fixture +def session_with_tags(session: Session): + from flask_appbuilder.security.sqla.models import User + + from superset.tags.models import Tag, TagType + + engine = session.get_bind() + Tag.metadata.create_all(engine) # pylint: disable=no-member + User.metadata.create_all(engine) # pylint: disable=no-member + + owner = User( + first_name="owner", last_name="owner", username="owner", email="owner@x.com" + ) + other = User( + first_name="other", last_name="other", username="other", email="other@x.com" + ) + session.add(owner) + session.add(other) + session.flush() + + owned_tag = Tag( + name="owned_tag", + type=TagType.custom, + created_by_fk=owner.id, + created_by=owner, + ) + system_tag = Tag(name="type:some_type", type=TagType.type) + + session.add(owned_tag) + session.add(system_tag) + session.commit() + return session + + +def test_delete_tags_command_admin_can_delete_any_custom_tag( + session_with_tags: Session, mocker: MockerFixture +): + from superset.commands.tag.delete import DeleteTagsCommand + from superset.tags.models import Tag + + mocker.patch( + "superset.security.SupersetSecurityManager.is_admin", return_value=True + ) + + DeleteTagsCommand(["owned_tag"]).run() + + assert db.session.query(Tag).filter_by(name="owned_tag").one_or_none() is None + + +def test_delete_tags_command_creator_can_delete_own_tag( + session_with_tags: Session, mocker: MockerFixture +): + from flask_appbuilder.security.sqla.models import User + + from superset.commands.tag.delete import DeleteTagsCommand + from superset.tags.models import Tag + + owner = db.session.query(User).filter_by(username="owner").one() + + mocker.patch( + "superset.security.SupersetSecurityManager.is_admin", return_value=False + ) + mocker.patch( + "superset.security.SupersetSecurityManager.current_user", + new_callable=PropertyMock, + return_value=owner, + ) + + DeleteTagsCommand(["owned_tag"]).run() + + assert db.session.query(Tag).filter_by(name="owned_tag").one_or_none() is None + + +def test_delete_tags_command_non_creator_non_admin_denied( + session_with_tags: Session, mocker: MockerFixture +): + """Regression test: DeleteTagsCommand.validate previously checked only + that each named tag existed, letting any user with can_delete on Tag + (default Gamma) bulk-delete tags -- and every association they carry -- + they neither created nor own. + """ + from flask_appbuilder.security.sqla.models import User + + from superset.commands.tag.delete import DeleteTagsCommand + from superset.commands.tag.exceptions import TagInvalidError + from superset.tags.models import Tag + + other = db.session.query(User).filter_by(username="other").one() + + mocker.patch( + "superset.security.SupersetSecurityManager.is_admin", return_value=False + ) + mocker.patch( + "superset.security.SupersetSecurityManager.current_user", + new_callable=PropertyMock, + return_value=other, + ) + + with pytest.raises(TagInvalidError): + DeleteTagsCommand(["owned_tag"]).run() + + # the tag must survive the denied deletion + assert db.session.query(Tag).filter_by(name="owned_tag").one_or_none() is not None + + +def test_delete_tags_command_refuses_system_tag_even_for_admin( + session_with_tags: Session, mocker: MockerFixture +): + """System-generated tags (type:*, editor:*, favorited_by:*) are + maintained by Superset itself and must not be deletable through the + bulk route, regardless of the caller's role. + """ + from superset.commands.tag.delete import DeleteTagsCommand + from superset.commands.tag.exceptions import TagInvalidError + from superset.tags.models import Tag + + mocker.patch( + "superset.security.SupersetSecurityManager.is_admin", return_value=True + ) + + with pytest.raises(TagInvalidError): + DeleteTagsCommand(["type:some_type"]).run() + + assert ( + db.session.query(Tag).filter_by(name="type:some_type").one_or_none() is not None + ) From 13af5cc86559e8b6573d6bce007ee1e2096d718e Mon Sep 17 00:00:00 2001 From: Superset Dev Date: Fri, 21 Aug 2026 00:44:50 -0700 Subject: [PATCH 03/12] fix(security): make user-registrations API admin-only and read-only 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. --- superset/security/api.py | 6 ++++++ superset/security/manager.py | 4 ++++ tests/unit_tests/security/api_test.py | 27 ++++++++++++++++++++++++++- 3 files changed, 36 insertions(+), 1 deletion(-) diff --git a/superset/security/api.py b/superset/security/api.py index bb8995d46657..92084a25ced8 100644 --- a/superset/security/api.py +++ b/superset/security/api.py @@ -40,6 +40,7 @@ EmbeddedDashboardNotFoundError, ) from superset.commands.exceptions import ForbiddenError +from superset.constants import RouteMethod from superset.exceptions import SupersetGenericErrorException from superset.extensions import db, event_logger from superset.security.guest_token import ( @@ -423,6 +424,11 @@ class UserRegistrationsRestAPI(BaseSupersetModelRestApi): resource_name = "security/user_registrations" datamodel = SQLAInterface(RegisterUser) allow_browser_login = True + # This API is read-only by design: restricting the exposed routes keeps + # the FAB default POST/PUT/DELETE handlers from ever being registered, + # so a mis-granted role cannot create, alter, or silently cancel a + # pending registration. + include_route_methods = {RouteMethod.GET, RouteMethod.GET_LIST, RouteMethod.INFO} # NOTE: registration_hash is intentionally excluded from both list_columns # and search_columns. It is a bearer token for the # /register/activation/ flow; exposing it in API responses (and thus diff --git a/superset/security/manager.py b/superset/security/manager.py index 79c262dfddca..21f0729d4101 100644 --- a/superset/security/manager.py +++ b/superset/security/manager.py @@ -1432,6 +1432,10 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods "Security", "SQL Lab", "User Registrations", + # REST counterpart of the FAB "User Registrations" views. FAB derives + # the view-menu name from the class name; without this entry + # _is_gamma_pvm grants its permissions to stock Gamma and Alpha. + "UserRegistrationsRestAPI", "User's Statistics", # Guarding all AB_ADD_SECURITY_API = True REST APIs "RoleRestAPI", diff --git a/tests/unit_tests/security/api_test.py b/tests/unit_tests/security/api_test.py index f2962a82a66c..ad3c3c1e2ebf 100644 --- a/tests/unit_tests/security/api_test.py +++ b/tests/unit_tests/security/api_test.py @@ -19,8 +19,10 @@ import pytest from marshmallow import ValidationError +from superset.constants import RouteMethod from superset.extensions import csrf -from superset.security.api import RlsRuleSchema +from superset.security.api import RlsRuleSchema, UserRegistrationsRestAPI +from superset.security.manager import SupersetSecurityManager @pytest.mark.parametrize( @@ -172,3 +174,26 @@ def test_rls_rule_schema_rejects_falsy_dataset(dataset: Any) -> None: with pytest.raises(ValidationError) as exc_info: RlsRuleSchema().load({"dataset": dataset, "clause": "tenant_id = 1"}) assert "dataset" in exc_info.value.messages + + +def test_user_registrations_rest_api_is_admin_only() -> None: + """ + The API is documented Admin-only, but the admin gate is membership in + ADMIN_ONLY_VIEW_MENUS keyed by the FAB-derived view-menu name (the class + name). If the entry is missing, ``superset init`` grants the API's + permissions to stock Gamma and Alpha via ``_is_gamma_pvm``, exposing + pending registrants' PII and registration deletion. + """ + assert "UserRegistrationsRestAPI" in SupersetSecurityManager.ADMIN_ONLY_VIEW_MENUS + + +def test_user_registrations_rest_api_routes_are_read_only() -> None: + """ + Only the read routes should be registered; the FAB default POST/PUT/ + DELETE handlers must not exist on this API at all. + """ + assert UserRegistrationsRestAPI.include_route_methods == { + RouteMethod.GET, + RouteMethod.GET_LIST, + RouteMethod.INFO, + } From 816b98f876455b9eb083a5a888ac21b9eac9e26d Mon Sep 17 00:00:00 2001 From: Superset Dev Date: Fri, 21 Aug 2026 00:48:26 -0700 Subject: [PATCH 04/12] fix(reports): scope execution-log API reads to editable schedules 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. --- superset/reports/filters.py | 22 +++++++++++- superset/reports/logs/api.py | 8 +++++ tests/unit_tests/reports/filters_test.py | 42 +++++++++++++++++++++++ tests/unit_tests/reports/logs_api_test.py | 37 ++++++++++++++++++++ 4 files changed, 108 insertions(+), 1 deletion(-) create mode 100644 tests/unit_tests/reports/logs_api_test.py diff --git a/superset/reports/filters.py b/superset/reports/filters.py index ad08a4a234a1..21209312df6e 100644 --- a/superset/reports/filters.py +++ b/superset/reports/filters.py @@ -22,7 +22,7 @@ from superset import db, security_manager from superset.daos.base import _escape_like -from superset.reports.models import ReportSchedule +from superset.reports.models import ReportExecutionLog, ReportSchedule from superset.subjects.filters import subject_relation_exists_for_current_user from superset.views.base import BaseFilter @@ -43,6 +43,26 @@ def _apply_editors(self, query: Query) -> Query: return query.filter(ReportSchedule.id.in_(editor_ids_query)) +class ReportExecutionLogFilter(BaseFilter): # pylint: disable=too-few-public-methods + """ + Scope execution logs to report schedules the user can edit, mirroring + ``ReportScheduleFilter`` on the schedule API. Logs carry evaluated alert + values and database error messages, so they must not be readable across + ownership boundaries via an attacker-chosen schedule id. + """ + + def apply(self, query: Query, value: Any) -> Query: + if security_manager.can_access_all_datasources(): + return query + + from superset.subjects.models import report_schedule_editors + + editor_ids_query = db.session.query( + report_schedule_editors.c.report_schedule_id + ).filter(subject_relation_exists_for_current_user(report_schedule_editors)) + return query.filter(ReportExecutionLog.report_schedule_id.in_(editor_ids_query)) + + class ReportScheduleAllTextFilter(BaseFilter): # pylint: disable=too-few-public-methods name = _("All Text") arg_name = "report_all_text" diff --git a/superset/reports/logs/api.py b/superset/reports/logs/api.py index 9a2cea964e3b..96673d1ef3db 100644 --- a/superset/reports/logs/api.py +++ b/superset/reports/logs/api.py @@ -31,6 +31,7 @@ from superset import is_feature_enabled from superset.constants import MODEL_API_RW_METHOD_PERMISSION_MAP, RouteMethod +from superset.reports.filters import ReportExecutionLogFilter from superset.reports.logs.schemas import openapi_spec_methods_override from superset.reports.models import ReportExecutionLog from superset.views.base_api import BaseSupersetModelRestApi @@ -53,6 +54,13 @@ def ensure_alert_reports_enabled(self) -> Optional[Response]: class_permission_name = "ReportSchedule" resource_name = "report" allow_browser_login = True + # Ownership scoping. Without a base filter the only scoping on these + # routes is the caller-controlled ``pk`` path parameter (folded into the + # rison filters below), so any role with ReportSchedule read could + # iterate every schedule's logs deployment-wide. Mirrors the + # ReportScheduleFilter applied by the schedule API + # (superset/reports/api.py). + base_filters = [["id", ReportExecutionLogFilter, lambda: []]] show_columns = [ "id", diff --git a/tests/unit_tests/reports/filters_test.py b/tests/unit_tests/reports/filters_test.py index bdcb2c181fb1..defc5fd11cee 100644 --- a/tests/unit_tests/reports/filters_test.py +++ b/tests/unit_tests/reports/filters_test.py @@ -106,3 +106,45 @@ def test_report_schedule_all_text_filter_coerces_non_string( mock_report_schedule.sql, ): column.ilike.assert_called_once_with(expected, escape="\\") + + +@patch("superset.reports.filters.security_manager", new_callable=MagicMock) +def test_report_execution_log_filter_admin_sees_all(mock_sm: MagicMock) -> None: + """ + Regression test: ``ReportExecutionLogRestApi`` had no base filter at all, + so any role with generic ReportSchedule read could iterate every + schedule's logs by pk. An admin (can_access_all_datasources) must still + see everything unfiltered. + """ + from superset.reports.filters import ReportExecutionLogFilter + + mock_sm.can_access_all_datasources.return_value = True + query = MagicMock() + f = ReportExecutionLogFilter("id", MagicMock()) + result = f.apply(query, None) + assert result is query + query.filter.assert_not_called() + + +@patch("superset.reports.filters.security_manager", new_callable=MagicMock) +@patch("superset.reports.filters.db") +def test_report_execution_log_filter_non_admin_scoped_to_log_fk( + mock_db: MagicMock, mock_sm: MagicMock +) -> None: + """ + A non-admin must be scoped by ``ReportExecutionLog.report_schedule_id`` + directly (not by an unjoined filter on ``ReportSchedule.id``, which would + pass for any log row as long as the caller edits at least one schedule). + """ + from superset.reports.filters import ReportExecutionLogFilter + from superset.reports.models import ReportExecutionLog + + mock_sm.can_access_all_datasources.return_value = False + query = MagicMock() + f = ReportExecutionLogFilter("id", MagicMock()) + f.apply(query, None) + + query.filter.assert_called_once() + (filter_expr,) = query.filter.call_args[0] + assert filter_expr.left.table is ReportExecutionLog.__table__ + assert filter_expr.left.name == "report_schedule_id" diff --git a/tests/unit_tests/reports/logs_api_test.py b/tests/unit_tests/reports/logs_api_test.py new file mode 100644 index 000000000000..e081f54e58ff --- /dev/null +++ b/tests/unit_tests/reports/logs_api_test.py @@ -0,0 +1,37 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from superset.reports.filters import ReportExecutionLogFilter +from superset.reports.logs.api import ReportExecutionLogRestApi + + +def test_execution_log_api_has_ownership_base_filter() -> None: + """ + Regression test: the schedule id in the ``/log/`` and ``/log/`` + routes is caller-controlled (folded into the rison filters), so an + editor-scoped base filter must be applied to both the list and item + routes -- otherwise any role with generic ReportSchedule read can iterate + every schedule's logs. + """ + assert ReportExecutionLogRestApi.base_filters, ( + "ReportExecutionLogRestApi must apply an ownership base filter; " + "without one, logs are readable across schedules regardless of " + "ownership" + ) + assert any( + filter_class is ReportExecutionLogFilter + for _, filter_class, _ in ReportExecutionLogRestApi.base_filters + ) From c91505e86467ed38ffff838e47220c241cd59a73 Mon Sep 17 00:00:00 2001 From: Superset Dev Date: Fri, 21 Aug 2026 00:50:05 -0700 Subject: [PATCH 05/12] fix(datasets): drop email from drill_info's nested user schema 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. --- superset/datasets/schemas.py | 6 +++- tests/integration_tests/datasets/api_tests.py | 31 +++++++++++++++++++ tests/unit_tests/datasets/schema_tests.py | 21 +++++++++++++ 3 files changed, 57 insertions(+), 1 deletion(-) diff --git a/superset/datasets/schemas.py b/superset/datasets/schemas.py index 59aa2185c78c..cd88acff9c63 100644 --- a/superset/datasets/schemas.py +++ b/superset/datasets/schemas.py @@ -480,9 +480,13 @@ class DatasetColumnDrillInfoSchema(Schema): class UserSchema(Schema): + # Deliberately excludes ``email``: drill_info is reachable by any user + # with read access to the dataset (and, via the dashboard fallback, by + # embedded guests), so exposing maintainer emails here would leak user + # PII across an access boundary. Mirrors the dashboard/RLS user schemas, + # which expose names only. first_name = fields.String() last_name = fields.String() - email = fields.String() class DatasetDrillInfoSchema(Schema): diff --git a/tests/integration_tests/datasets/api_tests.py b/tests/integration_tests/datasets/api_tests.py index 56f3e38efd78..f70d5f00945d 100644 --- a/tests/integration_tests/datasets/api_tests.py +++ b/tests/integration_tests/datasets/api_tests.py @@ -3402,6 +3402,37 @@ def test_get_drill_info_admin_user(self): self.items_to_delete = [dataset] + def test_get_drill_info_does_not_expose_user_emails(self): + """ + Dataset API: drill_info must not leak creator/modifier email addresses. + + The nested user schema exposes first/last name only; email is PII and + is not part of the endpoint's select_columns contract. + """ + self.login(ADMIN_USERNAME) + dataset = self.insert_dataset( + table_name="test_drill_dataset_no_email", + editor_user_ids=[], + columns=[ + TableColumn( + column_name="category", + type="VARCHAR(255)", + groupby=True, + ), + ], + fetch_metadata=False, + ) + + uri = f"api/v1/dataset/{dataset.id}/drill_info/" + rv = self.get_assert_metric(uri, "get_drill_info") + assert rv.status_code == 200 + + result = json.loads(rv.data.decode("utf-8"))["result"] + for user_field in ("created_by", "changed_by"): + assert "email" not in (result.get(user_field) or {}) + + self.items_to_delete = [dataset] + def test_get_drill_info_admin_user_dataset_not_found(self): """ Dataset API: Test drill_info endpoint returns 404 for non-existent dataset. diff --git a/tests/unit_tests/datasets/schema_tests.py b/tests/unit_tests/datasets/schema_tests.py index dcea6e795a63..f868888e02af 100644 --- a/tests/unit_tests/datasets/schema_tests.py +++ b/tests/unit_tests/datasets/schema_tests.py @@ -48,6 +48,27 @@ def test_validate_python_date_format_raises(payload) -> None: validate_python_date_format(payload) +def test_drill_info_user_schema_does_not_expose_email() -> None: + """ + Regression test: the drill_info-local ``UserSchema`` used to declare an + ``email`` field. ``DatasetDrillInfoSchema`` nests it (unfiltered by + ``select_columns``) for both ``created_by`` and ``changed_by``, so any + user with dataset-read access -- or, via the dashboard fallback, + embedded guests -- received maintainer email addresses. Dataset-read + access does not imply entitlement to other users' PII. + """ + from superset.datasets.schemas import UserSchema + + class _FakeUser: + first_name = "Jane" + last_name = "Doe" + email = "jane.doe@example.com" + + dumped = UserSchema().dump(_FakeUser()) + assert "email" not in dumped + assert dumped == {"first_name": "Jane", "last_name": "Doe"} + + def test_dataset_post_schema_has_all_put_scalar_fields() -> None: """ Every scalar model field accepted by DatasetPutSchema should also be accepted From 345d7c3fc98e1042ce5c33e8a028c45aa24b1d78 Mon Sep 17 00:00:00 2001 From: Superset Dev Date: Fri, 21 Aug 2026 09:55:52 -0700 Subject: [PATCH 06/12] fix(tags): call self.logout() between user switches in delete-tags test 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 --- superset/commands/tag/delete.py | 1 + tests/integration_tests/tags/commands_tests.py | 3 +++ 2 files changed, 4 insertions(+) diff --git a/superset/commands/tag/delete.py b/superset/commands/tag/delete.py index 0a37bfcee501..8ae04852ece1 100644 --- a/superset/commands/tag/delete.py +++ b/superset/commands/tag/delete.py @@ -136,6 +136,7 @@ def run(self) -> None: def validate(self) -> None: exceptions: list[TagNotFoundError | TagDeleteFailedError] = [] for tag_name in self._tags: + tag_name = tag_name.strip() tag = TagDAO.find_by_name(tag_name) # Validate tag exists if not tag: diff --git a/tests/integration_tests/tags/commands_tests.py b/tests/integration_tests/tags/commands_tests.py index a35c756364a2..f83e4f06c395 100644 --- a/tests/integration_tests/tags/commands_tests.py +++ b/tests/integration_tests/tags/commands_tests.py @@ -155,18 +155,21 @@ def test_delete_tags_command_requires_authorization(self): try: # a non-admin who is not the tag's creator may not delete it + self.logout() self.login(GAMMA_USERNAME) with pytest.raises(TagInvalidError): DeleteTagsCommand(example_tags).run() assert db.session.query(Tag).filter(Tag.name.in_(example_tags)).count() == 1 # system-generated tags are refused outright, even for an admin + self.logout() self.login(ADMIN_USERNAME) with pytest.raises(TagInvalidError): DeleteTagsCommand([system_tag.name]).run() assert db.session.query(Tag).filter_by(name=system_tag.name).count() == 1 finally: # cleanup + self.logout() self.login(ADMIN_USERNAME) DeleteTagsCommand(example_tags).run() db.session.query(Tag).filter_by(name=system_tag.name).delete() From 0dbd0dfd74814d9fbc90aad52dbf31ebcc813108 Mon Sep 17 00:00:00 2001 From: Superset Dev Date: Fri, 21 Aug 2026 10:00:14 -0700 Subject: [PATCH 07/12] docs(tags): trim a test docstring Drop an over-specific rationale phrase from a regression-test docstring. --- tests/unit_tests/tags/commands/update_test.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/unit_tests/tags/commands/update_test.py b/tests/unit_tests/tags/commands/update_test.py index 52323edc99a4..24a8bd43369c 100644 --- a/tests/unit_tests/tags/commands/update_test.py +++ b/tests/unit_tests/tags/commands/update_test.py @@ -349,10 +349,10 @@ def test_update_command_empty_objects_to_tag_only_removes_accessible( ): """An empty/omitted objects_to_tag must not mass-delete every association. - Regression test for the finding's exact exploit payload: PUT with no - objects_to_tag used to be treated as "delete every current association" - with zero per-object check. Now each deletion is access-checked, so - objects the caller cannot modify keep their association. + A PUT with no objects_to_tag used to be treated as "delete every current + association" with zero per-object check. Now each deletion is + access-checked, so objects the caller cannot modify keep their + association. """ from superset.commands.tag.create import CreateCustomTagWithRelationshipsCommand from superset.commands.tag.update import UpdateTagCommand From d0861c387e92dd934a0b15a3ee1609bad089a9b4 Mon Sep 17 00:00:00 2001 From: Superset Dev Date: Fri, 21 Aug 2026 13:16:46 -0700 Subject: [PATCH 08/12] fix(tags): route single-tag DELETE through DeleteTagsCommand TagRestApi exposed the FAB-generated DELETE /api/v1/tag/ 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 --- superset/tags/api.py | 58 ++++++++++ tests/integration_tests/tags/api_tests.py | 43 ++++++++ tests/unit_tests/tags/api_test.py | 124 ++++++++++++++++++++++ 3 files changed, 225 insertions(+) create mode 100644 tests/unit_tests/tags/api_test.py diff --git a/superset/tags/api.py b/superset/tags/api.py index 38e76f4fedbc..09a640d52e47 100644 --- a/superset/tags/api.py +++ b/superset/tags/api.py @@ -486,6 +486,64 @@ def delete_object( ) return self.response_422(message=str(ex)) + @expose("/", methods=("DELETE",)) + @protect() + @safe + @statsd_metrics + @event_logger.log_this_with_context( + action=lambda self, *args, **kwargs: f"{self.__class__.__name__}.delete", + log_to_statsd=False, + ) + def delete(self, pk: int) -> Response: + """Deletes a Tag + --- + delete: + description: >- + Delete a Tag by id. This will remove all tagged objects with + this tag. + parameters: + - in: path + schema: + type: integer + name: pk + responses: + 200: + description: Tag deleted + content: + application/json: + schema: + type: object + properties: + message: + type: string + 401: + $ref: '#/components/responses/401' + 403: + $ref: '#/components/responses/403' + 404: + $ref: '#/components/responses/404' + 422: + $ref: '#/components/responses/422' + 500: + $ref: '#/components/responses/500' + """ + # Overrides the FAB-generated single-object delete route (which would + # otherwise call self.datamodel.delete directly, bypassing + # DeleteTagsCommand's ownership and system-tag checks) so both the + # single-object and bulk-delete routes share the same validation. + tag = TagDAO.find_by_id(pk) + if not tag: + return self.response_404() + try: + DeleteTagsCommand([tag.name]).run() + return self.response(200, message="OK") + except TagNotFoundError: + return self.response_404() + except TagInvalidError as ex: + return self.response_422(message=ex.normalized_messages()) + except TagDeleteFailedError as ex: + return self.response_422(message=str(ex)) + @expose("/", methods=("DELETE",)) @protect() @safe diff --git a/tests/integration_tests/tags/api_tests.py b/tests/integration_tests/tags/api_tests.py index e34fb4e622bb..15d0ec2ec0bc 100644 --- a/tests/integration_tests/tags/api_tests.py +++ b/tests/integration_tests/tags/api_tests.py @@ -570,6 +570,49 @@ def test_delete_tags(self): tags = db.session.query(Tag).filter(Tag.name.in_(example_tag_names)) assert tags.count() == 0 + @pytest.mark.usefixtures("create_tags") + def test_delete_tag_by_pk(self): + """ + Tag API: the single-object ``DELETE /api/v1/tag/`` route must + share the same ownership/system-tag validation as bulk_delete + (DeleteTagsCommand), not the FAB-generated model delete. + """ + tag = db.session.query(Tag).filter(Tag.name == "example_tag_1").one() + system_tag = Tag(name="system:pk_delete_example", type=TagType.type) + db.session.add(system_tag) + db.session.commit() + + try: + # a non-admin, non-creator user may not delete via the pk route + self.login(GAMMA_USERNAME) + rv = self.client.delete(f"api/v1/tag/{tag.id}", follow_redirects=True) + assert rv.status_code == 422 + assert db.session.query(Tag).filter(Tag.id == tag.id).count() == 1 + + # system-generated tags are refused outright, even for an admin + self.logout() + self.login(ADMIN_USERNAME) + rv = self.client.delete( + f"api/v1/tag/{system_tag.id}", follow_redirects=True + ) + assert rv.status_code == 422 + assert db.session.query(Tag).filter(Tag.id == system_tag.id).count() == 1 + + # an admin may delete a custom tag via the pk route + rv = self.client.delete(f"api/v1/tag/{tag.id}", follow_redirects=True) + assert rv.status_code == 200 + assert db.session.query(Tag).filter(Tag.id == tag.id).count() == 0 + finally: + db.session.query(Tag).filter(Tag.id == system_tag.id).delete() + db.session.commit() + + def test_delete_tag_by_pk_not_found(self): + self.login(ADMIN_USERNAME) + existing_ids = [tag_id for (tag_id,) in db.session.query(Tag.id).all()] + non_existent_id = max(existing_ids, default=0) + 1 + rv = self.client.delete(f"api/v1/tag/{non_existent_id}", follow_redirects=True) + assert rv.status_code == 404 + @pytest.mark.usefixtures("create_tags") def test_delete_favorite_tag(self): self.login(ADMIN_USERNAME) diff --git a/tests/unit_tests/tags/api_test.py b/tests/unit_tests/tags/api_test.py new file mode 100644 index 000000000000..53bea2f5d899 --- /dev/null +++ b/tests/unit_tests/tags/api_test.py @@ -0,0 +1,124 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from typing import Any +from unittest.mock import MagicMock + +from pytest_mock import MockerFixture + + +def test_delete_tag_by_pk_routes_through_delete_tags_command( + client: Any, + full_api_access: None, + mocker: MockerFixture, +) -> None: + """ + Regression test: ``DELETE /api/v1/tag/`` used to fall through to the + FAB-generated single-object delete route, which deletes the row directly + via the datamodel and never runs ``DeleteTagsCommand.validate`` (the + admin-or-creator check, and the system-tag refusal). The pk route must + be overridden to share that same validation instead of duplicating it. + """ + mock_tag = MagicMock(id=1) + mock_tag.name = "example_tag" + mocker.patch("superset.tags.api.TagDAO.find_by_id", return_value=mock_tag) + mock_command = mocker.patch("superset.tags.api.DeleteTagsCommand") + mock_command.return_value.run.return_value = None + + response = client.delete("/api/v1/tag/1") + + assert response.status_code == 200 + mock_command.assert_called_once_with(["example_tag"]) + + +def test_delete_tag_by_pk_not_found( + client: Any, + full_api_access: None, + mocker: MockerFixture, +) -> None: + """DELETE /api/v1/tag/ returns 404 when the tag does not exist.""" + mocker.patch("superset.tags.api.TagDAO.find_by_id", return_value=None) + mock_command = mocker.patch("superset.tags.api.DeleteTagsCommand") + + response = client.delete("/api/v1/tag/999") + + assert response.status_code == 404 + mock_command.assert_not_called() + + +def test_delete_tag_by_pk_denied_surfaces_as_422( + client: Any, + full_api_access: None, + mocker: MockerFixture, +) -> None: + """ + A non-admin, non-creator caller (or an attempt to delete a system tag) + is rejected by DeleteTagsCommand.validate with TagInvalidError; the pk + route must surface that as 422, not silently succeed. + """ + from superset.commands.tag.exceptions import TagInvalidError + + mock_tag = MagicMock(id=1) + mock_tag.name = "someone_elses_tag" + mocker.patch("superset.tags.api.TagDAO.find_by_id", return_value=mock_tag) + mock_command = mocker.patch("superset.tags.api.DeleteTagsCommand") + mock_command.return_value.run.side_effect = TagInvalidError() + + response = client.delete("/api/v1/tag/1") + + assert response.status_code == 422 + + +def test_delete_tag_by_pk_race_with_bulk_delete_surfaces_as_404( + client: Any, + full_api_access: None, + mocker: MockerFixture, +) -> None: + """ + If the tag is deleted concurrently between the ``find_by_id`` lookup and + ``DeleteTagsCommand.validate``'s own lookup, the command raises + TagNotFoundError; the pk route must surface that as 404. + """ + from superset.commands.tag.exceptions import TagNotFoundError + + mock_tag = MagicMock(id=1) + mock_tag.name = "example_tag" + mocker.patch("superset.tags.api.TagDAO.find_by_id", return_value=mock_tag) + mock_command = mocker.patch("superset.tags.api.DeleteTagsCommand") + mock_command.return_value.run.side_effect = TagNotFoundError("example_tag") + + response = client.delete("/api/v1/tag/1") + + assert response.status_code == 404 + + +def test_delete_tag_by_pk_delete_failed_surfaces_as_422( + client: Any, + full_api_access: None, + mocker: MockerFixture, +) -> None: + """DeleteTagsCommand raising TagDeleteFailedError surfaces as 422.""" + from superset.commands.tag.exceptions import TagDeleteFailedError + + mock_tag = MagicMock(id=1) + mock_tag.name = "example_tag" + mocker.patch("superset.tags.api.TagDAO.find_by_id", return_value=mock_tag) + mock_command = mocker.patch("superset.tags.api.DeleteTagsCommand") + mock_command.return_value.run.side_effect = TagDeleteFailedError() + + response = client.delete("/api/v1/tag/1") + + assert response.status_code == 422 From 0ceacdedf45f8eaa161d06aa787256056c4004a2 Mon Sep 17 00:00:00 2001 From: Superset Dev Date: Fri, 21 Aug 2026 13:17:28 -0700 Subject: [PATCH 09/12] fix(reports): gate execution-log unrestricted read on can_access_all_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 --- superset/reports/filters.py | 10 +++++++- tests/unit_tests/reports/filters_test.py | 30 +++++++++++++++++++++--- 2 files changed, 36 insertions(+), 4 deletions(-) diff --git a/superset/reports/filters.py b/superset/reports/filters.py index 21209312df6e..af4781a4e482 100644 --- a/superset/reports/filters.py +++ b/superset/reports/filters.py @@ -49,10 +49,18 @@ class ReportExecutionLogFilter(BaseFilter): # pylint: disable=too-few-public-me ``ReportScheduleFilter`` on the schedule API. Logs carry evaluated alert values and database error messages, so they must not be readable across ownership boundaries via an attacker-chosen schedule id. + + The unrestricted bypass is gated by ``can_access_all_queries`` rather + than ``can_access_all_datasources``: the latter is also granted to + stock Alpha, which would let a non-editor Alpha user read every other + schedule's evaluated values and database errors. ``can_access_all_queries`` + is the admin-only permission this codebase already uses to gate the + equivalent per-execution data on SQL Lab query history + (see ``superset.queries.filters.QueryFilter``). """ def apply(self, query: Query, value: Any) -> Query: - if security_manager.can_access_all_datasources(): + if security_manager.can_access_all_queries(): return query from superset.subjects.models import report_schedule_editors diff --git a/tests/unit_tests/reports/filters_test.py b/tests/unit_tests/reports/filters_test.py index defc5fd11cee..3f3367b69598 100644 --- a/tests/unit_tests/reports/filters_test.py +++ b/tests/unit_tests/reports/filters_test.py @@ -113,12 +113,12 @@ def test_report_execution_log_filter_admin_sees_all(mock_sm: MagicMock) -> None: """ Regression test: ``ReportExecutionLogRestApi`` had no base filter at all, so any role with generic ReportSchedule read could iterate every - schedule's logs by pk. An admin (can_access_all_datasources) must still + schedule's logs by pk. An admin (can_access_all_queries) must still see everything unfiltered. """ from superset.reports.filters import ReportExecutionLogFilter - mock_sm.can_access_all_datasources.return_value = True + mock_sm.can_access_all_queries.return_value = True query = MagicMock() f = ReportExecutionLogFilter("id", MagicMock()) result = f.apply(query, None) @@ -126,6 +126,30 @@ def test_report_execution_log_filter_admin_sees_all(mock_sm: MagicMock) -> None: query.filter.assert_not_called() +@patch("superset.reports.filters.security_manager", new_callable=MagicMock) +def test_report_execution_log_filter_stock_alpha_is_scoped( + mock_sm: MagicMock, +) -> None: + """ + Regression test: the unrestricted bypass used to key off + ``can_access_all_datasources``, which is also granted to stock Alpha + (see ``SupersetSecurityManager.ALPHA_ONLY_PERMISSIONS``), letting a + non-editor Alpha user read every other schedule's evaluated alert + values and database errors. The bypass must require + ``can_access_all_queries`` (admin-only) instead, matching the + equivalent per-execution SQL Lab query history filter + (``superset.queries.filters.QueryFilter``). + """ + from superset.reports.filters import ReportExecutionLogFilter + + mock_sm.can_access_all_datasources.return_value = True + mock_sm.can_access_all_queries.return_value = False + query = MagicMock() + f = ReportExecutionLogFilter("id", MagicMock()) + f.apply(query, None) + query.filter.assert_called_once() + + @patch("superset.reports.filters.security_manager", new_callable=MagicMock) @patch("superset.reports.filters.db") def test_report_execution_log_filter_non_admin_scoped_to_log_fk( @@ -139,7 +163,7 @@ def test_report_execution_log_filter_non_admin_scoped_to_log_fk( from superset.reports.filters import ReportExecutionLogFilter from superset.reports.models import ReportExecutionLog - mock_sm.can_access_all_datasources.return_value = False + mock_sm.can_access_all_queries.return_value = False query = MagicMock() f = ReportExecutionLogFilter("id", MagicMock()) f.apply(query, None) From bf4a7817ebd786c8b5974dc56f3641f14bf508c6 Mon Sep 17 00:00:00 2001 From: Superset Dev Date: Fri, 21 Aug 2026 13:17:47 -0700 Subject: [PATCH 10/12] fix(security): restore DELETE on the user-registrations API 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/, 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 --- superset/security/api.py | 19 ++++++++++++++----- tests/unit_tests/security/api_test.py | 12 +++++++++--- 2 files changed, 23 insertions(+), 8 deletions(-) diff --git a/superset/security/api.py b/superset/security/api.py index 92084a25ced8..63291e14d540 100644 --- a/superset/security/api.py +++ b/superset/security/api.py @@ -424,11 +424,20 @@ class UserRegistrationsRestAPI(BaseSupersetModelRestApi): resource_name = "security/user_registrations" datamodel = SQLAInterface(RegisterUser) allow_browser_login = True - # This API is read-only by design: restricting the exposed routes keeps - # the FAB default POST/PUT/DELETE handlers from ever being registered, - # so a mis-granted role cannot create, alter, or silently cancel a - # pending registration. - include_route_methods = {RouteMethod.GET, RouteMethod.GET_LIST, RouteMethod.INFO} + # POST/PUT are intentionally excluded: restricting the exposed routes + # keeps the FAB default create/update handlers from ever being + # registered, so a mis-granted role cannot silently alter a pending + # registration. DELETE is kept: the User Registrations admin page + # deletes pending registrations through this route, and the class is + # gated Admin-only via ADMIN_ONLY_VIEW_MENUS (keyed on the view-menu + # name, i.e. every permission on this class, not just specific ones), + # so exposing it does not grant non-Admin roles anything. + include_route_methods = { + RouteMethod.GET, + RouteMethod.GET_LIST, + RouteMethod.INFO, + RouteMethod.DELETE, + } # NOTE: registration_hash is intentionally excluded from both list_columns # and search_columns. It is a bearer token for the # /register/activation/ flow; exposing it in API responses (and thus diff --git a/tests/unit_tests/security/api_test.py b/tests/unit_tests/security/api_test.py index ad3c3c1e2ebf..6d87a8fbf75f 100644 --- a/tests/unit_tests/security/api_test.py +++ b/tests/unit_tests/security/api_test.py @@ -187,13 +187,19 @@ def test_user_registrations_rest_api_is_admin_only() -> None: assert "UserRegistrationsRestAPI" in SupersetSecurityManager.ADMIN_ONLY_VIEW_MENUS -def test_user_registrations_rest_api_routes_are_read_only() -> None: +def test_user_registrations_rest_api_excludes_create_and_update() -> None: """ - Only the read routes should be registered; the FAB default POST/PUT/ - DELETE handlers must not exist on this API at all. + The FAB default POST/PUT handlers must not exist on this API, so a + mis-granted role cannot create or silently alter a pending registration. + DELETE stays registered: the User Registrations admin page deletes + pending registrations through it, and the route is still fully gated + Admin-only (see test_user_registrations_rest_api_is_admin_only). """ assert UserRegistrationsRestAPI.include_route_methods == { RouteMethod.GET, RouteMethod.GET_LIST, RouteMethod.INFO, + RouteMethod.DELETE, } + assert "post" not in UserRegistrationsRestAPI.include_route_methods + assert "put" not in UserRegistrationsRestAPI.include_route_methods From b8ffd8db78f566f641583ca2aeae635644fdc800 Mon Sep 17 00:00:00 2001 From: Superset Dev Date: Fri, 21 Aug 2026 13:18:07 -0700 Subject: [PATCH 11/12] fix(datasets): drop secondary_label from drill_info's editors field 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 --- superset/datasets/schemas.py | 15 ++++++-- tests/integration_tests/datasets/api_tests.py | 36 +++++++++++++++++++ tests/unit_tests/datasets/schema_tests.py | 24 +++++++++++++ 3 files changed, 73 insertions(+), 2 deletions(-) diff --git a/superset/datasets/schemas.py b/superset/datasets/schemas.py index cd88acff9c63..17425bee014c 100644 --- a/superset/datasets/schemas.py +++ b/superset/datasets/schemas.py @@ -34,7 +34,6 @@ from superset.connectors.sqla.models import SqlaTable from superset.exceptions import SupersetMarshmallowValidationError from superset.models.sql_types import parse_currency_string -from superset.subjects.schemas import SubjectResponseSchema from superset.utils import json get_delete_ids_schema = { @@ -489,11 +488,23 @@ class UserSchema(Schema): last_name = fields.String() +class DrillInfoEditorSchema(Schema): + # Deliberately excludes ``secondary_label``: for a user-backed Subject, + # user-subject synchronization (superset.subjects.sync.sync_user_subject) + # stores that user's email in this field, so including it here would + # leak the same maintainer PII that ``UserSchema`` above excludes + # ``email`` to avoid, just through a different field name. + id = fields.Int() + label = fields.String() + img = fields.String() + type = fields.Integer() + + class DatasetDrillInfoSchema(Schema): id = fields.Integer() columns = fields.List(fields.Nested(DatasetColumnDrillInfoSchema)) table_name = fields.String() - editors = fields.List(fields.Nested(SubjectResponseSchema)) + editors = fields.List(fields.Nested(DrillInfoEditorSchema)) created_by = fields.Nested(UserSchema) created_on_humanized = fields.String() changed_by = fields.Nested(UserSchema) diff --git a/tests/integration_tests/datasets/api_tests.py b/tests/integration_tests/datasets/api_tests.py index f70d5f00945d..cfd32fe9deda 100644 --- a/tests/integration_tests/datasets/api_tests.py +++ b/tests/integration_tests/datasets/api_tests.py @@ -3433,6 +3433,42 @@ def test_get_drill_info_does_not_expose_user_emails(self): self.items_to_delete = [dataset] + def test_get_drill_info_does_not_expose_editor_emails(self): + """ + Dataset API: drill_info must not leak an editor's email address + through the ``editors`` list. + + User-subject synchronization stores a user's email in the Subject's + ``secondary_label`` field, and ``editors`` nests Subjects directly, + so email must be excluded the same way it is for created_by/changed_by. + """ + self.login(ADMIN_USERNAME) + gamma_user = self.get_user(GAMMA_USERNAME) + dataset = self.insert_dataset( + table_name="test_drill_dataset_no_editor_email", + editor_user_ids=[gamma_user.id], + columns=[ + TableColumn( + column_name="category", + type="VARCHAR(255)", + groupby=True, + ), + ], + fetch_metadata=False, + ) + + uri = f"api/v1/dataset/{dataset.id}/drill_info/" + rv = self.get_assert_metric(uri, "get_drill_info") + assert rv.status_code == 200 + + result = json.loads(rv.data.decode("utf-8"))["result"] + editors = result.get("editors") or [] + assert len(editors) == 1 + assert "secondary_label" not in editors[0] + assert gamma_user.email not in json.dumps(editors) + + self.items_to_delete = [dataset] + def test_get_drill_info_admin_user_dataset_not_found(self): """ Dataset API: Test drill_info endpoint returns 404 for non-existent dataset. diff --git a/tests/unit_tests/datasets/schema_tests.py b/tests/unit_tests/datasets/schema_tests.py index f868888e02af..5ca7e99dd29b 100644 --- a/tests/unit_tests/datasets/schema_tests.py +++ b/tests/unit_tests/datasets/schema_tests.py @@ -69,6 +69,30 @@ class _FakeUser: assert dumped == {"first_name": "Jane", "last_name": "Doe"} +def test_drill_info_editor_schema_does_not_expose_secondary_label() -> None: + """ + Regression test: ``DatasetDrillInfoSchema.editors`` used to nest 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 nesting it unfiltered leaked the same + maintainer PII that dropping ``email`` from ``UserSchema`` was meant to + close, just under a different field name. + """ + from superset.datasets.schemas import DrillInfoEditorSchema + + class _FakeEditorSubject: + id = 1 + label = "Jane Doe" + secondary_label = "jane.doe@example.com" + img = "avatar.png" + type = 1 + + dumped = DrillInfoEditorSchema().dump(_FakeEditorSubject()) + assert "secondary_label" not in dumped + assert dumped == {"id": 1, "label": "Jane Doe", "img": "avatar.png", "type": 1} + + def test_dataset_post_schema_has_all_put_scalar_fields() -> None: """ Every scalar model field accepted by DatasetPutSchema should also be accepted From 9da55539a3ae10cd949438fa1e8f0478edf67808 Mon Sep 17 00:00:00 2001 From: Superset Dev Date: Fri, 21 Aug 2026 14:12:43 -0700 Subject: [PATCH 12/12] fix(tags): make DeleteTagsCommand's composited exceptions ValidationError 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/ 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 --- superset/commands/tag/delete.py | 21 +++++++-- superset/commands/tag/exceptions.py | 13 +++++ tests/unit_tests/tags/api_test.py | 40 ++++++++++++++++ tests/unit_tests/tags/commands/delete_test.py | 47 +++++++++++++++++++ 4 files changed, 116 insertions(+), 5 deletions(-) diff --git a/superset/commands/tag/delete.py b/superset/commands/tag/delete.py index 8ae04852ece1..6b8693a3dd3f 100644 --- a/superset/commands/tag/delete.py +++ b/superset/commands/tag/delete.py @@ -18,14 +18,17 @@ from functools import partial from typing import Any +from marshmallow import ValidationError + from superset import security_manager from superset.commands.base import BaseCommand +from superset.commands.exceptions import TagNotFoundValidationError from superset.commands.tag.exceptions import ( TagDeleteFailedError, + TagDeleteForbiddenValidationError, TaggedObjectDeleteFailedError, TaggedObjectNotFoundError, TagInvalidError, - TagNotFoundError, ) from superset.commands.tag.utils import to_object_model, to_object_type from superset.daos.tag import TagDAO @@ -134,20 +137,26 @@ def run(self) -> None: TagDAO.delete_tags(self._tags) def validate(self) -> None: - exceptions: list[TagNotFoundError | TagDeleteFailedError] = [] + # Every item appended here must be a ValidationError (or subclass), + # since TagInvalidError.normalized_messages() calls + # .normalized_messages() on each one to build the aggregated 422 + # response. + exceptions: list[ValidationError] = [] for tag_name in self._tags: tag_name = tag_name.strip() tag = TagDAO.find_by_name(tag_name) # Validate tag exists if not tag: - exceptions.append(TagNotFoundError(tag_name)) + exceptions.append( + TagNotFoundValidationError(f"Tag with name {tag_name} not found.") + ) continue # 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( - TagDeleteFailedError( + TagDeleteForbiddenValidationError( f"Tag {tag_name} is a system tag and cannot be deleted" ) ) @@ -161,7 +170,9 @@ def validate(self) -> None: or (tag.created_by and tag.created_by == security_manager.current_user) ): exceptions.append( - TagDeleteFailedError(f"Access denied to tag {tag_name}") + TagDeleteForbiddenValidationError( + f"Access denied to tag {tag_name}" + ) ) if exceptions: raise TagInvalidError(exceptions=exceptions) diff --git a/superset/commands/tag/exceptions.py b/superset/commands/tag/exceptions.py index 6778c8e221a1..6fd838c32a3b 100644 --- a/superset/commands/tag/exceptions.py +++ b/superset/commands/tag/exceptions.py @@ -17,6 +17,7 @@ from typing import Optional from flask_babel import lazy_gettext as _ +from marshmallow import ValidationError from superset.commands.exceptions import ( CommandException, @@ -44,6 +45,18 @@ class TagDeleteFailedError(DeleteFailedError): message = _("Tag could not be deleted.") +class TagDeleteForbiddenValidationError(ValidationError): + """A tag exists but may not be deleted (a system-generated tag, or the + caller lacks ownership/admin rights). Unlike ``TagDeleteFailedError``, + this is a ``ValidationError`` so it can be composited into a + ``TagInvalidError`` alongside other validation failures and still + support ``CommandInvalidError.normalized_messages()``. + """ + + def __init__(self, message: str) -> None: + super().__init__(message, field_name="tags") + + class TaggedObjectDeleteFailedError(DeleteFailedError): message = _("Tagged Object could not be deleted.") diff --git a/tests/unit_tests/tags/api_test.py b/tests/unit_tests/tags/api_test.py index 53bea2f5d899..9cd4f77a4006 100644 --- a/tests/unit_tests/tags/api_test.py +++ b/tests/unit_tests/tags/api_test.py @@ -82,6 +82,46 @@ def test_delete_tag_by_pk_denied_surfaces_as_422( assert response.status_code == 422 +def test_delete_tag_by_pk_denied_with_populated_exceptions_surfaces_as_422( + client: Any, + full_api_access: None, + mocker: MockerFixture, +) -> None: + """ + Regression test for an AttributeError that only surfaced once + DeleteTagsCommand.validate's TagInvalidError actually carried the + exceptions it composites at runtime (a plain TagDeleteFailedError, + not a ValidationError). The pk route calls + ``ex.normalized_messages()`` on the TagInvalidError it catches, which + previously crashed with a 500 instead of returning 422 because + TagDeleteFailedError has no ``normalized_messages()`` method. A + TagInvalidError() with no exceptions (as in the test above) does not + exercise that aggregation loop, so this test populates it the way the + real command does. + """ + from superset.commands.tag.exceptions import ( + TagDeleteForbiddenValidationError, + TagInvalidError, + ) + + mock_tag = MagicMock(id=1) + mock_tag.name = "system:some_type" + mocker.patch("superset.tags.api.TagDAO.find_by_id", return_value=mock_tag) + mock_command = mocker.patch("superset.tags.api.DeleteTagsCommand") + mock_command.return_value.run.side_effect = TagInvalidError( + exceptions=[ + TagDeleteForbiddenValidationError( + "Tag system:some_type is a system tag and cannot be deleted" + ) + ] + ) + + response = client.delete("/api/v1/tag/1") + + assert response.status_code == 422 + assert "tags" in response.json["message"] + + def test_delete_tag_by_pk_race_with_bulk_delete_surfaces_as_404( client: Any, full_api_access: None, diff --git a/tests/unit_tests/tags/commands/delete_test.py b/tests/unit_tests/tags/commands/delete_test.py index 4e9803f2c296..7bd8888457aa 100644 --- a/tests/unit_tests/tags/commands/delete_test.py +++ b/tests/unit_tests/tags/commands/delete_test.py @@ -149,3 +149,50 @@ def test_delete_tags_command_refuses_system_tag_even_for_admin( assert ( db.session.query(Tag).filter_by(name="type:some_type").one_or_none() is not None ) + + +def test_delete_tags_command_refused_tag_reports_normalized_messages( + session_with_tags: Session, mocker: MockerFixture +): + """Regression test: DeleteTagsCommand.validate previously appended a + plain CommandException (TagDeleteFailedError) into the TagInvalidError + it raises, which crashed with AttributeError as soon as anything called + .normalized_messages() on that TagInvalidError (as the single-object + DELETE /api/v1/tag/ route does). Every exception composited into + TagInvalidError must be a ValidationError so normalized_messages() can + aggregate it. + """ + from superset.commands.tag.delete import DeleteTagsCommand + from superset.commands.tag.exceptions import TagInvalidError + + mocker.patch( + "superset.security.SupersetSecurityManager.is_admin", return_value=True + ) + + with pytest.raises(TagInvalidError) as excinfo: + DeleteTagsCommand(["type:some_type"]).run() + + messages = excinfo.value.normalized_messages() + assert "tags" in messages + assert "system tag" in messages["tags"][0] + + +def test_delete_tags_command_not_found_reports_normalized_messages( + session_with_tags: Session, mocker: MockerFixture +): + """A nonexistent tag name is also composited into TagInvalidError; it + must likewise support normalized_messages() without raising. + """ + from superset.commands.tag.delete import DeleteTagsCommand + from superset.commands.tag.exceptions import TagInvalidError + + mocker.patch( + "superset.security.SupersetSecurityManager.is_admin", return_value=True + ) + + with pytest.raises(TagInvalidError) as excinfo: + DeleteTagsCommand(["does_not_exist"]).run() + + messages = excinfo.value.normalized_messages() + assert "tags" in messages + assert "not found" in messages["tags"][0]