You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@superset.apache.org by ru...@apache.org on 2026/08/21 16:09:55 UTC
(superset) branch fix/semantic-layer-theme-access-checks created (now 3f645b444b9)
This is an automated email from the ASF dual-hosted git repository.
rusackas pushed a change to branch fix/semantic-layer-theme-access-checks
in repository https://gitbox.apache.org/repos/asf/superset.git
at 3f645b444b9 fix(themes): block import overwrite of system-slotted themes
This branch includes the following new commits:
new 176257e42a1 fix(semantic_layers): check access before returning a layer's configuration
new 6430db9517d fix(semantic_layers): require editorship before updating or deleting a layer
new fae62663cba fix(themes): require Alpha role for theme writes, protect the active system theme, and harden script-tag stripping
new 3f645b444b9 fix(themes): block import overwrite of system-slotted themes
The 4 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
(superset) 02/04: fix(semantic_layers): require editorship before updating or deleting a layer
Posted by ru...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
rusackas pushed a commit to branch fix/semantic-layer-theme-access-checks
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 6430db9517da8a5165ae6e3411cbd4b9d3393f1c
Author: Superset Dev <de...@superset.apache.org>
AuthorDate: Fri Aug 21 00:47:52 2026 -0700
fix(semantic_layers): require editorship before updating or deleting a layer
Add security_manager.raise_for_editorship() checks to
UpdateSemanticLayerCommand.validate, DeleteSemanticLayerCommand.validate,
and CreateSemanticViewCommand.validate, mirroring the pattern already used
by the sibling Update/Delete/BulkDeleteSemanticViewCommand classes.
Previously these commands only checked existence/uniqueness, so anyone with
generic write access to semantic layers could modify or delete any layer.
Wire the new SemanticLayerForbiddenError into 403 responses on the layer
PUT/DELETE routes, and handle SemanticViewForbiddenError per-item in the
bulk semantic-view create loop so one forbidden item doesn't abort the
whole batch.
Co-Authored-By: Claude Sonnet 5 <no...@anthropic.com>
---
superset/commands/semantic_layer/create.py | 11 +++-
superset/commands/semantic_layer/delete.py | 6 ++
superset/commands/semantic_layer/update.py | 6 ++
superset/semantic_layers/api.py | 7 ++
.../commands/semantic_layer/create_test.py | 44 +++++++++++++
.../commands/semantic_layer/delete_test.py | 33 +++++++++-
.../commands/semantic_layer/update_test.py | 44 +++++++++++++
tests/unit_tests/semantic_layers/api_test.py | 74 ++++++++++++++++++++++
8 files changed, 223 insertions(+), 2 deletions(-)
diff --git a/superset/commands/semantic_layer/create.py b/superset/commands/semantic_layer/create.py
index 8d88578aa6e..513e40d8aea 100644
--- a/superset/commands/semantic_layer/create.py
+++ b/superset/commands/semantic_layer/create.py
@@ -23,14 +23,17 @@ from typing import Any
from flask_appbuilder.models.sqla import Model
from sqlalchemy.exc import SQLAlchemyError
+from superset import security_manager
from superset.commands.base import BaseCommand
from superset.commands.semantic_layer.exceptions import (
SemanticLayerCreateFailedError,
SemanticLayerInvalidError,
SemanticLayerNotFoundError,
SemanticViewCreateFailedError,
+ SemanticViewForbiddenError,
)
from superset.daos.semantic_layer import SemanticLayerDAO, SemanticViewDAO
+from superset.exceptions import SupersetSecurityException
from superset.semantic_layers.registry import registry
from superset.utils import json
from superset.utils.decorators import on_error, transaction
@@ -92,9 +95,15 @@ class CreateSemanticViewCommand(BaseCommand):
def validate(self) -> None:
layer_uuid: str = self._properties.get("semantic_layer_uuid", "")
- if not SemanticLayerDAO.find_by_uuid(layer_uuid):
+ layer = SemanticLayerDAO.find_by_uuid(layer_uuid)
+ if not layer:
raise SemanticLayerNotFoundError()
+ try:
+ security_manager.raise_for_editorship(layer)
+ except SupersetSecurityException as ex:
+ raise SemanticViewForbiddenError() from ex
+
name: str = self._properties.get("name", "")
configuration: dict[str, Any] = self._properties.get("configuration") or {}
if not SemanticViewDAO.validate_uniqueness(name, layer_uuid, configuration):
diff --git a/superset/commands/semantic_layer/delete.py b/superset/commands/semantic_layer/delete.py
index 8c39672d188..426543fe69a 100644
--- a/superset/commands/semantic_layer/delete.py
+++ b/superset/commands/semantic_layer/delete.py
@@ -25,6 +25,7 @@ from superset import security_manager
from superset.commands.base import BaseCommand
from superset.commands.semantic_layer.exceptions import (
SemanticLayerDeleteFailedError,
+ SemanticLayerForbiddenError,
SemanticLayerNotFoundError,
SemanticViewDeleteFailedError,
SemanticViewForbiddenError,
@@ -60,6 +61,11 @@ class DeleteSemanticLayerCommand(BaseCommand):
if not self._model:
raise SemanticLayerNotFoundError()
+ try:
+ security_manager.raise_for_editorship(self._model)
+ except SupersetSecurityException as ex:
+ raise SemanticLayerForbiddenError() from ex
+
class DeleteSemanticViewCommand(BaseCommand):
def __init__(self, pk: int):
diff --git a/superset/commands/semantic_layer/update.py b/superset/commands/semantic_layer/update.py
index 4df9ee9e216..7ae4e16d8e0 100644
--- a/superset/commands/semantic_layer/update.py
+++ b/superset/commands/semantic_layer/update.py
@@ -26,6 +26,7 @@ from sqlalchemy.exc import SQLAlchemyError
from superset import security_manager
from superset.commands.base import BaseCommand
from superset.commands.semantic_layer.exceptions import (
+ SemanticLayerForbiddenError,
SemanticLayerInvalidError,
SemanticLayerNotFoundError,
SemanticLayerUpdateFailedError,
@@ -116,6 +117,11 @@ class UpdateSemanticLayerCommand(BaseCommand):
if not self._model:
raise SemanticLayerNotFoundError()
+ try:
+ security_manager.raise_for_editorship(self._model)
+ except SupersetSecurityException as ex:
+ raise SemanticLayerForbiddenError() from ex
+
name = self._properties.get("name")
if name and not SemanticLayerDAO.validate_update_uniqueness(self._uuid, name):
raise SemanticLayerInvalidError(f"Name already exists: {name}")
diff --git a/superset/semantic_layers/api.py b/superset/semantic_layers/api.py
index ff83a3a1f81..64348e72603 100644
--- a/superset/semantic_layers/api.py
+++ b/superset/semantic_layers/api.py
@@ -41,6 +41,7 @@ from superset.commands.semantic_layer.delete import (
from superset.commands.semantic_layer.exceptions import (
SemanticLayerCreateFailedError,
SemanticLayerDeleteFailedError,
+ SemanticLayerForbiddenError,
SemanticLayerInvalidError,
SemanticLayerNotFoundError,
SemanticLayerUpdateFailedError,
@@ -338,6 +339,8 @@ class SemanticViewRestApi(BaseSupersetModelRestApi):
errors.append(
{"name": view_data.get("name"), "error": "Semantic layer not found"}
)
+ except SemanticViewForbiddenError as ex:
+ errors.append({"name": view_data.get("name"), "error": str(ex)})
except SemanticViewCreateFailedError as ex:
logger.error(
"Error creating semantic view: %s",
@@ -880,6 +883,8 @@ class SemanticLayerRestApi(BaseSupersetApi):
return self.response(200, result={"uuid": str(changed_model.uuid)})
except SemanticLayerNotFoundError:
return self.response_404()
+ except SemanticLayerForbiddenError as ex:
+ return self.response(403, message=str(ex))
except SemanticLayerInvalidError as ex:
return self.response_422(message=str(ex))
except SemanticLayerUpdateFailedError as ex:
@@ -919,6 +924,8 @@ class SemanticLayerRestApi(BaseSupersetApi):
return self.response(200, message="OK")
except SemanticLayerNotFoundError:
return self.response_404()
+ except SemanticLayerForbiddenError as ex:
+ return self.response(403, message=str(ex))
except SemanticLayerDeleteFailedError as ex:
logger.error(
"Error deleting semantic layer: %s",
diff --git a/tests/unit_tests/commands/semantic_layer/create_test.py b/tests/unit_tests/commands/semantic_layer/create_test.py
index e531a1cec0d..23d79a80638 100644
--- a/tests/unit_tests/commands/semantic_layer/create_test.py
+++ b/tests/unit_tests/commands/semantic_layer/create_test.py
@@ -166,6 +166,10 @@ def test_create_semantic_view_success(mocker: MockerFixture) -> None:
mock_model.name = "orders"
dao_view.create.return_value = mock_model
+ mocker.patch(
+ "superset.commands.semantic_layer.create.security_manager"
+ ).raise_for_editorship.return_value = None
+
from superset.commands.semantic_layer.create import CreateSemanticViewCommand
result = CreateSemanticViewCommand(
@@ -215,6 +219,10 @@ def test_create_semantic_view_duplicate(mocker: MockerFixture) -> None:
)
dao_view.validate_uniqueness.return_value = False
+ mocker.patch(
+ "superset.commands.semantic_layer.create.security_manager"
+ ).raise_for_editorship.return_value = None
+
from superset.commands.semantic_layer.create import CreateSemanticViewCommand
from superset.commands.semantic_layer.exceptions import (
SemanticViewCreateFailedError,
@@ -228,3 +236,39 @@ def test_create_semantic_view_duplicate(mocker: MockerFixture) -> None:
"configuration": {"db": "prod"},
}
).run()
+
+
+def test_create_semantic_view_forbidden(mocker: MockerFixture) -> None:
+ """Test CreateSemanticViewCommand raises when caller lacks layer editorship."""
+ from superset.commands.semantic_layer.create import CreateSemanticViewCommand
+ from superset.commands.semantic_layer.exceptions import SemanticViewForbiddenError
+ from superset.exceptions import SupersetSecurityException
+
+ mock_layer = MagicMock()
+ dao_layer = mocker.patch(
+ "superset.commands.semantic_layer.create.SemanticLayerDAO",
+ )
+ dao_layer.find_by_uuid.return_value = mock_layer
+
+ dao_view = mocker.patch(
+ "superset.commands.semantic_layer.create.SemanticViewDAO",
+ )
+
+ sm = mocker.patch(
+ "superset.commands.semantic_layer.create.security_manager",
+ )
+ # Use a regular MagicMock for raise_for_editorship to avoid AsyncMock issues
+ sm.raise_for_editorship = MagicMock(
+ side_effect=SupersetSecurityException(MagicMock()),
+ )
+
+ with pytest.raises(SemanticViewForbiddenError):
+ CreateSemanticViewCommand(
+ {
+ "name": "orders",
+ "semantic_layer_uuid": "layer-uuid",
+ "configuration": {"db": "prod"},
+ }
+ ).run()
+
+ dao_view.create.assert_not_called()
diff --git a/tests/unit_tests/commands/semantic_layer/delete_test.py b/tests/unit_tests/commands/semantic_layer/delete_test.py
index 78c559ccdb7..f9aac001f61 100644
--- a/tests/unit_tests/commands/semantic_layer/delete_test.py
+++ b/tests/unit_tests/commands/semantic_layer/delete_test.py
@@ -21,7 +21,11 @@ import pytest
from pytest_mock import MockerFixture
from superset.commands.semantic_layer.delete import DeleteSemanticLayerCommand
-from superset.commands.semantic_layer.exceptions import SemanticLayerNotFoundError
+from superset.commands.semantic_layer.exceptions import (
+ SemanticLayerForbiddenError,
+ SemanticLayerNotFoundError,
+)
+from superset.exceptions import SupersetSecurityException
def test_delete_semantic_layer_success(mocker: MockerFixture) -> None:
@@ -33,6 +37,10 @@ def test_delete_semantic_layer_success(mocker: MockerFixture) -> None:
)
dao.find_by_uuid.return_value = mock_model
+ mocker.patch(
+ "superset.commands.semantic_layer.delete.security_manager"
+ ).raise_for_editorship.return_value = None
+
DeleteSemanticLayerCommand("some-uuid").run()
dao.find_by_uuid.assert_called_once_with("some-uuid")
@@ -50,6 +58,29 @@ def test_delete_semantic_layer_not_found(mocker: MockerFixture) -> None:
DeleteSemanticLayerCommand("missing-uuid").run()
+def test_delete_semantic_layer_forbidden(mocker: MockerFixture) -> None:
+ """Test that SemanticLayerForbiddenError is raised for non-editors."""
+ mock_model = MagicMock()
+
+ dao = mocker.patch(
+ "superset.commands.semantic_layer.delete.SemanticLayerDAO",
+ )
+ dao.find_by_uuid.return_value = mock_model
+
+ sm = mocker.patch(
+ "superset.commands.semantic_layer.delete.security_manager",
+ )
+ # Use a regular MagicMock for raise_for_editorship to avoid AsyncMock issues
+ sm.raise_for_editorship = MagicMock(
+ side_effect=SupersetSecurityException(MagicMock()),
+ )
+
+ with pytest.raises(SemanticLayerForbiddenError):
+ DeleteSemanticLayerCommand("some-uuid").run()
+
+ dao.delete.assert_not_called()
+
+
def test_delete_semantic_view_success(mocker: MockerFixture) -> None:
"""Test successful deletion of a semantic view."""
mock_model = MagicMock()
diff --git a/tests/unit_tests/commands/semantic_layer/update_test.py b/tests/unit_tests/commands/semantic_layer/update_test.py
index 8903256de4a..172e5efbc65 100644
--- a/tests/unit_tests/commands/semantic_layer/update_test.py
+++ b/tests/unit_tests/commands/semantic_layer/update_test.py
@@ -21,6 +21,7 @@ import pytest
from pytest_mock import MockerFixture
from superset.commands.semantic_layer.exceptions import (
+ SemanticLayerForbiddenError,
SemanticLayerInvalidError,
SemanticLayerNotFoundError,
SemanticViewForbiddenError,
@@ -127,6 +128,10 @@ def test_update_semantic_layer_success(mocker: MockerFixture) -> None:
dao.find_by_uuid.return_value = mock_model
dao.update.return_value = mock_model
+ mocker.patch(
+ "superset.commands.semantic_layer.update.security_manager",
+ )
+
data = {"name": "Updated", "description": "New desc"}
result = UpdateSemanticLayerCommand("some-uuid", data).run()
@@ -146,6 +151,29 @@ def test_update_semantic_layer_not_found(mocker: MockerFixture) -> None:
UpdateSemanticLayerCommand("missing-uuid", {"name": "test"}).run()
+def test_update_semantic_layer_forbidden(mocker: MockerFixture) -> None:
+ """Test that SemanticLayerForbiddenError is raised on ownership failure."""
+ mock_model = MagicMock()
+ mock_model.type = "snowflake"
+
+ dao = mocker.patch(
+ "superset.commands.semantic_layer.update.SemanticLayerDAO",
+ )
+ dao.find_by_uuid.return_value = mock_model
+
+ sm = mocker.patch(
+ "superset.commands.semantic_layer.update.security_manager",
+ )
+ sm.raise_for_editorship = MagicMock(
+ side_effect=SupersetSecurityException(MagicMock()),
+ )
+
+ with pytest.raises(SemanticLayerForbiddenError):
+ UpdateSemanticLayerCommand("some-uuid", {"name": "test"}).run()
+
+ dao.update.assert_not_called()
+
+
def test_update_semantic_layer_duplicate_name(mocker: MockerFixture) -> None:
"""Test that SemanticLayerInvalidError is raised for duplicate names."""
mock_model = MagicMock()
@@ -157,6 +185,10 @@ def test_update_semantic_layer_duplicate_name(mocker: MockerFixture) -> None:
dao.find_by_uuid.return_value = mock_model
dao.validate_update_uniqueness.return_value = False
+ mocker.patch(
+ "superset.commands.semantic_layer.update.security_manager",
+ )
+
with pytest.raises(SemanticLayerInvalidError):
UpdateSemanticLayerCommand("some-uuid", {"name": "Duplicate"}).run()
@@ -174,6 +206,10 @@ def test_update_semantic_layer_validates_configuration(
dao.find_by_uuid.return_value = mock_model
dao.update.return_value = mock_model
+ mocker.patch(
+ "superset.commands.semantic_layer.update.security_manager",
+ )
+
mock_cls = MagicMock()
mocker.patch.dict(
"superset.commands.semantic_layer.update.registry",
@@ -199,6 +235,10 @@ def test_update_semantic_layer_skips_name_check_when_no_name(
dao.find_by_uuid.return_value = mock_model
dao.update.return_value = mock_model
+ mocker.patch(
+ "superset.commands.semantic_layer.update.security_manager",
+ )
+
UpdateSemanticLayerCommand("some-uuid", {"description": "Updated"}).run()
dao.validate_update_uniqueness.assert_not_called()
@@ -215,6 +255,10 @@ def test_update_semantic_layer_copies_data(mocker: MockerFixture) -> None:
dao.find_by_uuid.return_value = mock_model
dao.update.return_value = mock_model
+ mocker.patch(
+ "superset.commands.semantic_layer.update.security_manager",
+ )
+
original_data = {"description": "Original"}
UpdateSemanticLayerCommand("some-uuid", original_data).run()
diff --git a/tests/unit_tests/semantic_layers/api_test.py b/tests/unit_tests/semantic_layers/api_test.py
index bab98cb6ae6..f2dea7cce30 100644
--- a/tests/unit_tests/semantic_layers/api_test.py
+++ b/tests/unit_tests/semantic_layers/api_test.py
@@ -758,6 +758,30 @@ def test_put_semantic_layer_not_found(
assert response.status_code == 404
+@SEMANTIC_LAYERS_APP
+def test_put_semantic_layer_forbidden(
+ client: Any,
+ full_api_access: None,
+ mocker: MockerFixture,
+) -> None:
+ """Test PUT /<uuid> returns 403 when the caller is not an editor."""
+ from superset.commands.semantic_layer.exceptions import (
+ SemanticLayerForbiddenError,
+ )
+
+ mock_command = mocker.patch(
+ "superset.semantic_layers.api.UpdateSemanticLayerCommand",
+ )
+ mock_command.return_value.run.side_effect = SemanticLayerForbiddenError()
+
+ response = client.put(
+ f"/api/v1/semantic_layer/{uuid_lib.uuid4()}",
+ json={"name": "New"},
+ )
+
+ assert response.status_code == 403
+
+
@SEMANTIC_LAYERS_APP
def test_put_semantic_layer_invalid(
client: Any,
@@ -850,6 +874,27 @@ def test_delete_semantic_layer_not_found(
assert response.status_code == 404
+@SEMANTIC_LAYERS_APP
+def test_delete_semantic_layer_forbidden(
+ client: Any,
+ full_api_access: None,
+ mocker: MockerFixture,
+) -> None:
+ """Test DELETE /<uuid> returns 403 when the caller is not an editor."""
+ from superset.commands.semantic_layer.exceptions import (
+ SemanticLayerForbiddenError,
+ )
+
+ mock_command = mocker.patch(
+ "superset.semantic_layers.api.DeleteSemanticLayerCommand",
+ )
+ mock_command.return_value.run.side_effect = SemanticLayerForbiddenError()
+
+ response = client.delete(f"/api/v1/semantic_layer/{uuid_lib.uuid4()}")
+
+ assert response.status_code == 403
+
+
@SEMANTIC_LAYERS_APP
def test_delete_semantic_layer_failed(
client: Any,
@@ -1701,6 +1746,35 @@ def test_post_semantic_view_layer_not_found(
assert result["errors"][0]["error"] == "Semantic layer not found"
+@SEMANTIC_LAYERS_APP
+def test_post_semantic_view_forbidden(
+ client: Any,
+ full_api_access: None,
+ mocker: MockerFixture,
+) -> None:
+ """Test POST / collects forbidden errors instead of aborting the batch."""
+ mock_command = mocker.patch(
+ "superset.semantic_layers.api.CreateSemanticViewCommand",
+ )
+ mock_command.return_value.run.side_effect = SemanticViewForbiddenError()
+
+ payload = {
+ "views": [
+ {
+ "name": "View 1",
+ "semantic_layer_uuid": str(uuid_lib.uuid4()),
+ "configuration": {},
+ },
+ ],
+ }
+ response = client.post("/api/v1/semantic_view/", json=payload)
+
+ assert response.status_code == 422
+ result = response.json["result"]
+ assert len(result["errors"]) == 1
+ assert not result["created"]
+
+
@SEMANTIC_LAYERS_APP
def test_post_semantic_view_create_failed(
client: Any,
(superset) 04/04: fix(themes): block import overwrite of system-slotted themes
Posted by ru...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
rusackas pushed a commit to branch fix/semantic-layer-theme-access-checks
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 3f645b444b975f3f8e1de5eb82fef21c78a76339
Author: Superset Dev <de...@superset.apache.org>
AuthorDate: Fri Aug 21 01:00:52 2026 -0700
fix(themes): block import overwrite of system-slotted themes
import_theme's overwrite branch only checked can_write('Theme') before
replacing an existing theme matched by UUID, unlike UpdateThemeCommand and
DeleteThemeCommand which both refuse to touch system-protected themes.
Since seeded themes have stable, publicly-listable UUIDs, an import
bundle with overwrite=true could replace a system theme, or the theme
currently in the system-default/dark slot, bypassing the protection the
direct update/delete routes already enforce. Raise ThemeImportError
before binding the existing row's id when is_system, is_system_default,
or is_system_dark is set.
Co-Authored-By: Claude Sonnet 5 <no...@anthropic.com>
---
superset/commands/theme/import_themes.py | 5 +
.../commands/theme/test_import_themes.py | 129 +++++++++++++++++++++
2 files changed, 134 insertions(+)
diff --git a/superset/commands/theme/import_themes.py b/superset/commands/theme/import_themes.py
index dd37f12247f..e8912de6bd0 100644
--- a/superset/commands/theme/import_themes.py
+++ b/superset/commands/theme/import_themes.py
@@ -44,6 +44,11 @@ def import_theme(config: dict[str, Any], overwrite: bool = False) -> "Theme | No
if existing:
if not overwrite or not can_write:
return existing
+ if existing.is_system or existing.is_system_default or existing.is_system_dark:
+ raise ThemeImportError(
+ "Cannot overwrite a system theme or the active "
+ "system-default/dark theme via import"
+ )
config["id"] = existing.id
elif not can_write:
raise ThemeImportError(
diff --git a/tests/unit_tests/commands/theme/test_import_themes.py b/tests/unit_tests/commands/theme/test_import_themes.py
new file mode 100644
index 00000000000..32d0072791b
--- /dev/null
+++ b/tests/unit_tests/commands/theme/test_import_themes.py
@@ -0,0 +1,129 @@
+# 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 MagicMock, patch
+
+import pytest
+
+from superset.commands.theme.exceptions import ThemeImportError
+from superset.commands.theme.import_themes import import_theme
+from superset.models.core import Theme
+
+
+def _mock_existing(
+ is_system: bool = False,
+ is_system_default: bool = False,
+ is_system_dark: bool = False,
+) -> MagicMock:
+ theme = MagicMock(spec=Theme)
+ theme.id = 1
+ theme.is_system = is_system
+ theme.is_system_default = is_system_default
+ theme.is_system_dark = is_system_dark
+ return theme
+
+
+@patch("superset.security_manager")
+@patch("superset.db")
+def test_import_theme_refuses_system_theme_overwrite(mock_db, mock_security_manager):
+ """overwrite=True must not be able to replace a seeded system theme."""
+ mock_security_manager.can_access.return_value = True
+ existing = _mock_existing(is_system=True)
+ mock_db.session.query.return_value.filter_by.return_value.first.return_value = (
+ existing
+ )
+
+ config = {"uuid": "some-uuid", "theme_name": "hostile", "json_data": "{}"}
+
+ with pytest.raises(ThemeImportError):
+ import_theme(config, overwrite=True)
+
+
+@patch("superset.security_manager")
+@patch("superset.db")
+def test_import_theme_refuses_system_default_overwrite(mock_db, mock_security_manager):
+ """overwrite=True must not be able to replace the active default theme."""
+ mock_security_manager.can_access.return_value = True
+ existing = _mock_existing(is_system_default=True)
+ mock_db.session.query.return_value.filter_by.return_value.first.return_value = (
+ existing
+ )
+
+ config = {"uuid": "some-uuid", "theme_name": "hostile", "json_data": "{}"}
+
+ with pytest.raises(ThemeImportError):
+ import_theme(config, overwrite=True)
+
+
+@patch("superset.security_manager")
+@patch("superset.db")
+def test_import_theme_refuses_system_dark_overwrite(mock_db, mock_security_manager):
+ """overwrite=True must not be able to replace the active dark theme."""
+ mock_security_manager.can_access.return_value = True
+ existing = _mock_existing(is_system_dark=True)
+ mock_db.session.query.return_value.filter_by.return_value.first.return_value = (
+ existing
+ )
+
+ config = {"uuid": "some-uuid", "theme_name": "hostile", "json_data": "{}"}
+
+ with pytest.raises(ThemeImportError):
+ import_theme(config, overwrite=True)
+
+
+@patch("superset.utils.core.get_user")
+@patch("superset.security_manager")
+@patch("superset.db")
+def test_import_theme_allows_regular_theme_overwrite(
+ mock_db, mock_security_manager, mock_get_user
+):
+ """A regular (non-system) theme can still be overwritten as before."""
+ mock_security_manager.can_access.return_value = True
+ mock_get_user.return_value = None
+ existing = _mock_existing()
+ mock_db.session.query.return_value.filter_by.return_value.first.return_value = (
+ existing
+ )
+
+ config = {"uuid": "some-uuid", "theme_name": "updated", "json_data": "{}"}
+
+ with patch("superset.models.core.Theme.import_from_dict") as mock_import_from_dict:
+ mock_theme = MagicMock(spec=Theme)
+ mock_theme.id = 1
+ mock_import_from_dict.return_value = mock_theme
+
+ result = import_theme(config, overwrite=True)
+
+ assert result is mock_theme
+ assert config["id"] == existing.id
+
+
+@patch("superset.security_manager")
+@patch("superset.db")
+def test_import_theme_no_overwrite_returns_existing(mock_db, mock_security_manager):
+ """Without overwrite=True, the existing theme is returned untouched."""
+ mock_security_manager.can_access.return_value = True
+ existing = _mock_existing(is_system_default=True)
+ mock_db.session.query.return_value.filter_by.return_value.first.return_value = (
+ existing
+ )
+
+ config = {"uuid": "some-uuid", "theme_name": "hostile", "json_data": "{}"}
+
+ result = import_theme(config, overwrite=False)
+
+ assert result is existing
(superset) 01/04: fix(semantic_layers): check access before returning a layer's configuration
Posted by ru...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
rusackas pushed a commit to branch fix/semantic-layer-theme-access-checks
in repository https://gitbox.apache.org/repos/asf/superset.git
commit 176257e42a1d2b5f2decfaa16802da59cdc2b126
Author: Superset Dev <de...@superset.apache.org>
AuthorDate: Fri Aug 21 00:38:48 2026 -0700
fix(semantic_layers): check access before returning a layer's configuration
Add SemanticLayer.raise_for_access(), mirroring the existing
SemanticView.raise_for_access(), and call it from the layer GET, runtime
schema, and views endpoints before returning or exercising a layer's
decrypted configuration. Previously these endpoints resolved the layer by
UUID with no per-object permission check.
Co-Authored-By: Claude Sonnet 5 <no...@anthropic.com>
---
superset/semantic_layers/api.py | 16 +++++
superset/semantic_layers/models.py | 20 +++++++
tests/unit_tests/semantic_layers/api_test.py | 87 ++++++++++++++++++++++++++++
3 files changed, 123 insertions(+)
diff --git a/superset/semantic_layers/api.py b/superset/semantic_layers/api.py
index 19773ce73c1..ff83a3a1f81 100644
--- a/superset/semantic_layers/api.py
+++ b/superset/semantic_layers/api.py
@@ -672,6 +672,11 @@ class SemanticLayerRestApi(BaseSupersetApi):
if not layer:
return self.response_404()
+ try:
+ layer.raise_for_access()
+ except SupersetSecurityException as ex:
+ return self.response(403, message=ex.message)
+
body = request.get_json(silent=True) or {}
runtime_data = body.get("runtime_data")
@@ -726,6 +731,11 @@ class SemanticLayerRestApi(BaseSupersetApi):
if not layer:
return self.response_404()
+ try:
+ layer.raise_for_access()
+ except SupersetSecurityException as ex:
+ return self.response(403, message=ex.message)
+
body = request.get_json(silent=True) or {}
runtime_data = body.get("runtime_data", {})
@@ -1158,4 +1168,10 @@ class SemanticLayerRestApi(BaseSupersetApi):
layer = SemanticLayerDAO.find_by_uuid(uuid)
if not layer:
return self.response_404()
+
+ try:
+ layer.raise_for_access()
+ except SupersetSecurityException as ex:
+ return self.response(403, message=ex.message)
+
return self.response(200, result=_serialize_layer(layer))
diff --git a/superset/semantic_layers/models.py b/superset/semantic_layers/models.py
index 08677f2c731..e639d9b23d6 100644
--- a/superset/semantic_layers/models.py
+++ b/superset/semantic_layers/models.py
@@ -148,6 +148,26 @@ class SemanticLayer(AuditMixinNullable, Model):
"""Compute the permission string for this semantic layer."""
return f"[{self.name}](id:{self.uuid.hex})"
+ def raise_for_access(self) -> None:
+ """Check that the user has access to this semantic layer."""
+ from superset import security_manager
+ from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
+ from superset.exceptions import SupersetSecurityException
+
+ if security_manager.can_access_all_datasources():
+ return
+
+ if self.perm and security_manager.can_access("datasource_access", self.perm):
+ return
+
+ raise SupersetSecurityException(
+ SupersetError(
+ error_type=SupersetErrorType.DATASOURCE_SECURITY_ACCESS_ERROR,
+ message=str(_("You don't have access to this semantic layer.")),
+ level=ErrorLevel.ERROR,
+ )
+ )
+
@staticmethod
def after_insert(
mapper: Mapper,
diff --git a/tests/unit_tests/semantic_layers/api_test.py b/tests/unit_tests/semantic_layers/api_test.py
index 5dd2a480a64..bab98cb6ae6 100644
--- a/tests/unit_tests/semantic_layers/api_test.py
+++ b/tests/unit_tests/semantic_layers/api_test.py
@@ -36,6 +36,7 @@ from superset.commands.semantic_layer.exceptions import (
SemanticViewNotFoundError,
SemanticViewUpdateFailedError,
)
+from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
from superset.exceptions import SupersetSecurityException
from superset.semantic_layers.api import SemanticLayerRestApi, SemanticViewRestApi
@@ -529,6 +530,34 @@ def test_runtime_schema_not_found(
assert response.status_code == 404
+@SEMANTIC_LAYERS_APP
+def test_runtime_schema_forbidden(
+ client: Any,
+ full_api_access: None,
+ mocker: MockerFixture,
+) -> None:
+ """Test POST /<uuid>/schema/runtime returns 403 when access is denied."""
+ test_uuid = str(uuid_lib.uuid4())
+ mock_layer = MagicMock()
+ mock_layer.raise_for_access.side_effect = SupersetSecurityException(
+ SupersetError(
+ error_type=SupersetErrorType.DATASOURCE_SECURITY_ACCESS_ERROR,
+ message="You don't have access to this semantic layer.",
+ level=ErrorLevel.ERROR,
+ )
+ )
+
+ mock_dao = mocker.patch("superset.semantic_layers.api.SemanticLayerDAO")
+ mock_dao.find_by_uuid.return_value = mock_layer
+
+ response = client.post(
+ f"/api/v1/semantic_layer/{test_uuid}/schema/runtime",
+ )
+
+ assert response.status_code == 403
+ mock_layer.raise_for_access.assert_called_once()
+
+
@SEMANTIC_LAYERS_APP
def test_runtime_schema_unknown_type(
client: Any,
@@ -939,6 +968,33 @@ def test_get_semantic_layer_not_found(
assert response.status_code == 404
+@SEMANTIC_LAYERS_APP
+def test_get_semantic_layer_forbidden(
+ client: Any,
+ full_api_access: None,
+ mocker: MockerFixture,
+) -> None:
+ """Test GET /<uuid> returns 403 when user lacks access to the layer."""
+ test_uuid = uuid_lib.uuid4()
+ layer = MagicMock()
+ layer.uuid = test_uuid
+ layer.raise_for_access.side_effect = SupersetSecurityException(
+ SupersetError(
+ error_type=SupersetErrorType.DATASOURCE_SECURITY_ACCESS_ERROR,
+ message="You don't have access to this semantic layer.",
+ level=ErrorLevel.ERROR,
+ )
+ )
+
+ mock_dao = mocker.patch("superset.semantic_layers.api.SemanticLayerDAO")
+ mock_dao.find_by_uuid.return_value = layer
+
+ response = client.get(f"/api/v1/semantic_layer/{test_uuid}")
+
+ assert response.status_code == 403
+ layer.raise_for_access.assert_called_once()
+
+
@SEMANTIC_LAYERS_APP
def test_serialize_layer_string_config(
client: Any,
@@ -1956,6 +2012,37 @@ def test_get_views(
assert result[1]["name"] == "View B"
+@SEMANTIC_LAYERS_APP
+def test_get_views_forbidden(
+ client: Any,
+ full_api_access: None,
+ mocker: MockerFixture,
+) -> None:
+ """Test POST /<uuid>/views returns 403 when access is denied."""
+ test_uuid = str(uuid_lib.uuid4())
+ mock_layer = MagicMock()
+ mock_layer.uuid = uuid_lib.uuid4()
+ mock_layer.raise_for_access.side_effect = SupersetSecurityException(
+ SupersetError(
+ error_type=SupersetErrorType.DATASOURCE_SECURITY_ACCESS_ERROR,
+ message="You don't have access to this semantic layer.",
+ level=ErrorLevel.ERROR,
+ )
+ )
+
+ mock_dao = mocker.patch("superset.semantic_layers.api.SemanticLayerDAO")
+ mock_dao.find_by_uuid.return_value = mock_layer
+
+ response = client.post(
+ f"/api/v1/semantic_layer/{test_uuid}/views",
+ json={"runtime_data": {"database": "mydb"}},
+ )
+
+ assert response.status_code == 403
+ mock_layer.raise_for_access.assert_called_once()
+ mock_layer.implementation.get_semantic_views.assert_not_called()
+
+
@SEMANTIC_LAYERS_APP
def test_get_views_with_existing(
client: Any,
(superset) 03/04: fix(themes): require Alpha role for theme writes, protect the active system theme, and harden script-tag stripping
Posted by ru...@apache.org.
This is an automated email from the ASF dual-hosted git repository.
rusackas pushed a commit to branch fix/semantic-layer-theme-access-checks
in repository https://gitbox.apache.org/repos/asf/superset.git
commit fae62663cba4048f3a20add29ced30ea38c9cf69
Author: Superset Dev <de...@superset.apache.org>
AuthorDate: Fri Aug 21 00:59:49 2026 -0700
fix(themes): require Alpha role for theme writes, protect the active system theme, and harden script-tag stripping
Theme was missing from GAMMA_READ_ONLY_MODEL_VIEWS, so default Gamma role
sync granted full Theme CRUD instead of read-only access like the sibling
CssTemplate resource; move Theme writes to Alpha+ to match.
UpdateThemeCommand.validate only rejected edits to is_system themes, not
the theme currently promoted to the live system-default/dark slot
(is_system_default/is_system_dark), which SetSystemDefaultThemeCommand
never marks is_system. Add a non-admin guard on those flags so only an
admin can edit the theme actively rendered for every user; map the new
SystemThemeInUseError to a 403 in the PUT route.
sanitize_svg_content's script-tag regex only matched a bare literal
</script> closer, so a closing tag with trailing attributes/whitespace
(e.g. </script foo>), which browsers still treat as a valid closer,
survived removal. Make the closing-tag pattern tolerant of that, and add
a second pass that strips any leftover <script>/</script> fragment,
including an unterminated opener with no matching closer.
Co-Authored-By: Claude Sonnet 5 <no...@anthropic.com>
---
superset/commands/theme/update.py | 10 +++++++
superset/security/manager.py | 1 +
superset/themes/api.py | 2 ++
superset/utils/core.py | 10 ++++++-
tests/unit_tests/themes/commands_test.py | 51 ++++++++++++++++++++++++++++++++
tests/unit_tests/utils/test_core.py | 17 ++++++++++-
6 files changed, 89 insertions(+), 2 deletions(-)
diff --git a/superset/commands/theme/update.py b/superset/commands/theme/update.py
index 2f2151edba3..a4c91217c5f 100644
--- a/superset/commands/theme/update.py
+++ b/superset/commands/theme/update.py
@@ -18,8 +18,10 @@ import logging
from functools import partial
from typing import Any, Optional
+from superset import security_manager
from superset.commands.base import UpdateMixin
from superset.commands.theme.exceptions import (
+ SystemThemeInUseError,
SystemThemeProtectedError,
ThemeNotFoundError,
)
@@ -52,3 +54,11 @@ class UpdateThemeCommand(UpdateMixin):
# Check if it's a system theme
if self._model.is_system:
raise SystemThemeProtectedError()
+
+ # The active system-default/dark theme slot may be edited by admins
+ # only; a non-admin editing it would change the theme rendered for
+ # every user, including the login page and other admins.
+ if (
+ self._model.is_system_default or self._model.is_system_dark
+ ) and not security_manager.is_admin():
+ raise SystemThemeInUseError()
diff --git a/superset/security/manager.py b/superset/security/manager.py
index 79c262dfddc..edb3a037e6b 100644
--- a/superset/security/manager.py
+++ b/superset/security/manager.py
@@ -1408,6 +1408,7 @@ class SupersetSecurityManager( # pylint: disable=too-many-public-methods
"CssTemplate",
"Dataset",
"Datasource",
+ "Theme",
} | READ_ONLY_MODEL_VIEWS
GAMMA_EXCLUDED_PVMS = {
diff --git a/superset/themes/api.py b/superset/themes/api.py
index 527ec29b969..4e26971f081 100644
--- a/superset/themes/api.py
+++ b/superset/themes/api.py
@@ -350,6 +350,8 @@ class ThemeRestApi(BaseSupersetModelRestApi):
return self.response_404()
except SystemThemeProtectedError:
return self.response_403()
+ except SystemThemeInUseError:
+ return self.response_403()
except Exception as ex:
logger.exception("Unexpected error in PUT /theme/%s", pk)
return self.response_422(message=str(ex))
diff --git a/superset/utils/core.py b/superset/utils/core.py
index abfe9b221b9..3b9e46baf13 100644
--- a/superset/utils/core.py
+++ b/superset/utils/core.py
@@ -615,9 +615,17 @@ def sanitize_svg_content(svg_content: str) -> str:
return ""
# Minimal protection: remove obvious malicious content, preserve all SVG features
+ # The closing tag pattern tolerates attributes/whitespace after "script"
+ # (e.g. "</script foo>"), which browsers still parse as a valid closer.
content = re.sub(
- r"<script[^>]*>.*?</script>", "", svg_content, flags=re.IGNORECASE | re.DOTALL
+ r"<script\b[^>]*>.*?</script\b[^>]*>",
+ "",
+ svg_content,
+ flags=re.IGNORECASE | re.DOTALL,
)
+ # Second pass: strip any leftover <script ...> / </script ...> fragments
+ # that didn't have a matching pair above (e.g. an unterminated opener).
+ content = re.sub(r"</?script\b[^>]*>?", "", content, flags=re.IGNORECASE)
content = re.sub(r"javascript:", "", content, flags=re.IGNORECASE)
content = re.sub(r"data:[^;]*;[^,]*,.*javascript", "", content, flags=re.IGNORECASE)
diff --git a/tests/unit_tests/themes/commands_test.py b/tests/unit_tests/themes/commands_test.py
index b81a6c22f7b..04eab55174e 100644
--- a/tests/unit_tests/themes/commands_test.py
+++ b/tests/unit_tests/themes/commands_test.py
@@ -20,6 +20,7 @@ from unittest.mock import Mock, patch
import pytest
from superset.commands.theme.exceptions import (
+ SystemThemeInUseError,
SystemThemeProtectedError,
ThemeNotFoundError,
)
@@ -62,6 +63,8 @@ class TestUpdateThemeCommand:
# Arrange
mock_theme = Mock(spec=Theme)
mock_theme.is_system = False
+ mock_theme.is_system_default = False
+ mock_theme.is_system_dark = False
mock_theme_dao.find_by_id.return_value = mock_theme
command = UpdateThemeCommand(123, {"theme_name": "test"})
@@ -77,6 +80,8 @@ class TestUpdateThemeCommand:
# Arrange
mock_theme = Mock(spec=Theme)
mock_theme.is_system = False
+ mock_theme.is_system_default = False
+ mock_theme.is_system_dark = False
mock_updated_theme = Mock(spec=Theme)
mock_theme_dao.find_by_id.return_value = mock_theme
mock_theme_dao.update.return_value = mock_updated_theme
@@ -92,6 +97,52 @@ class TestUpdateThemeCommand:
mock_theme, {"theme_name": "updated_name"}
)
+ @patch("superset.commands.theme.update.security_manager")
+ @patch("superset.commands.theme.update.ThemeDAO")
+ def test_validate_system_default_theme_blocks_non_admin(
+ self, mock_theme_dao, mock_security_manager
+ ):
+ """Non-admins cannot update the active system-default/dark theme slot."""
+ mock_theme = Mock(spec=Theme)
+ mock_theme.is_system = False
+ mock_theme.is_system_default = True
+ mock_theme.is_system_dark = False
+ mock_theme_dao.find_by_id.return_value = mock_theme
+ # Use a regular Mock for is_admin to avoid AsyncMock auto-detection
+ mock_security_manager.is_admin = Mock(return_value=False)
+
+ command = UpdateThemeCommand(123, {"json_data": "{}"})
+
+ with pytest.raises(SystemThemeInUseError):
+ command.validate()
+
+ @patch("superset.commands.theme.update.security_manager")
+ @patch("superset.commands.theme.update.ThemeDAO")
+ def test_validate_system_default_theme_allows_admin(
+ self, mock_theme_dao, mock_security_manager
+ ):
+ """Admins can still update the active system-default/dark theme slot."""
+ mock_theme = Mock(spec=Theme)
+ mock_theme.is_system = False
+ mock_theme.is_system_default = True
+ mock_theme.is_system_dark = False
+ mock_theme_dao.find_by_id.return_value = mock_theme
+ # Use a regular Mock for is_admin to avoid AsyncMock auto-detection
+ mock_security_manager.is_admin = Mock(return_value=True)
+
+ command = UpdateThemeCommand(123, {"json_data": "{}"})
+
+ command.validate() # Should not raise any exception
+
+ assert command._model == mock_theme
+
+
+def test_theme_is_gamma_read_only():
+ """Theme writes must require at least Alpha; Gamma only gets read access."""
+ from superset import security_manager
+
+ assert "Theme" in security_manager.GAMMA_READ_ONLY_MODEL_VIEWS
+
class TestSeedSystemThemesCommand:
"""Unit tests for SeedSystemThemesCommand"""
diff --git a/tests/unit_tests/utils/test_core.py b/tests/unit_tests/utils/test_core.py
index d0b8a510d7a..1fd871e16c6 100644
--- a/tests/unit_tests/utils/test_core.py
+++ b/tests/unit_tests/utils/test_core.py
@@ -1942,13 +1942,28 @@ def test_sanitize_svg_content_safe():
def test_sanitize_svg_content_removes_scripts():
- """Test that nh3 removes dangerous script content."""
+ """Test that dangerous script content is removed."""
malicious_svg = '<svg><script>alert("xss")</script><rect/></svg>'
result = sanitize_svg_content(malicious_svg)
assert "script" not in result.lower()
assert "alert" not in result
+def test_sanitize_svg_content_removes_script_with_attributes_on_closer():
+ """A closing </script foo> tag is still a valid closer to browsers."""
+ malicious_svg = "<svg><script>fetch('/api/v1/me/')</script foo></svg>"
+ result = sanitize_svg_content(malicious_svg)
+ assert "script" not in result.lower()
+ assert "fetch" not in result
+
+
+def test_sanitize_svg_content_removes_unterminated_script():
+ """An unterminated <script> opener with no closing tag is still stripped."""
+ malicious_svg = "<svg><script>alert('xss')"
+ result = sanitize_svg_content(malicious_svg)
+ assert "script" not in result.lower()
+
+
def test_sanitize_url_relative():
"""Test that relative URLs are allowed."""
assert sanitize_url("/static/spinner.gif") == "/static/spinner.gif"