This is an automated email from the ASF dual-hosted git repository.
EnxDev pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new 424c34c695d fix(security): stop dataset editorship from authorizing
new SQL (#45342)
424c34c695d is described below
commit 424c34c695d6c9eb103141b33b04c390216bde4f
Author: Enzo Martellucci <[email protected]>
AuthorDate: Fri Oct 23 09:02:54 2026 +0200
fix(security): stop dataset editorship from authorizing new SQL (#45342)
Co-authored-by: Diego Pucci <[email protected]>
Co-authored-by: Claude Opus 4.8 <[email protected]>
---
superset/commands/dataset/create.py | 4 +-
superset/commands/explore/get.py | 14 ++-
superset/connectors/sqla/utils.py | 6 +-
superset/explore/utils.py | 6 +-
superset/security/manager.py | 11 +--
tests/unit_tests/commands/dataset/test_create.py | 11 +++
.../explore/test_get_query_authorship_bypass.py | 100 ++++++++++++++++++++-
tests/unit_tests/connectors/sqla/utils_test.py | 24 +++++
tests/unit_tests/explore/utils_test.py | 39 ++++++++
tests/unit_tests/subjects/test_raise_for_access.py | 4 +
10 files changed, 207 insertions(+), 12 deletions(-)
diff --git a/superset/commands/dataset/create.py
b/superset/commands/dataset/create.py
index cd366f5bf56..054f5901df7 100644
--- a/superset/commands/dataset/create.py
+++ b/superset/commands/dataset/create.py
@@ -140,10 +140,12 @@ class CreateDatasetCommand(CreateMixin, BaseCommand):
if sql:
try:
+ # Render with the params chart execution exposes, so the
+ # derived schema comes from the SQL the access check sees.
parse_result = process_jinja_sql(
sql,
database,
- template_params,
+ template_params_for_access_check(template_params),
)
except Exception: # pylint: disable=broad-exception-caught
logger.debug(
diff --git a/superset/commands/explore/get.py b/superset/commands/explore/get.py
index c5e2c650e1a..b7f22876cc9 100644
--- a/superset/commands/explore/get.py
+++ b/superset/commands/explore/get.py
@@ -64,7 +64,15 @@ def _authorize_datasource(
"""
if slc:
security_manager.raise_for_access(chart=slc)
- elif isinstance(datasource, Query):
+ # Chart access covers only the chart's own datasource. The request's
+ # form_data can name a different one, which then needs its own check.
+ if (slc.datasource_id, slc.datasource_type) == (
+ datasource.id,
+ datasource.type,
+ ):
+ return
+
+ if isinstance(datasource, Query):
# A "query" datasource_type resolves to the actual SQL Lab
# Query row, not a registered dataset -- its .perm is a
# synthetic string no role is ever granted, so the generic
@@ -83,10 +91,14 @@ def _authorize_datasource(
# the query at all. The author bypass returns before the generic
# ``datasource`` perm check at the tail of ``raise_for_access``,
# so this does not weaken anything for anyone else.
+ #
+ # Editorship stays off for everyone else: a dataset named after a
+ # table the query reads is a decoy any dataset creator can make.
security_manager.raise_for_access(
query=datasource,
datasource=datasource,
allow_query_authorship_bypass=True,
+ allow_editorship=False,
)
else:
security_manager.raise_for_access(datasource=datasource)
diff --git a/superset/connectors/sqla/utils.py
b/superset/connectors/sqla/utils.py
index e9d0f210cf2..3ce157cdce1 100644
--- a/superset/connectors/sqla/utils.py
+++ b/superset/connectors/sqla/utils.py
@@ -43,6 +43,7 @@ from superset.exceptions import (
SupersetVirtualTableParseException,
)
from superset.models.core import Database
+from superset.models.helpers import template_params_for_access_check
from superset.result_set import SupersetResultSet
from superset.sql.parse import SQLScript, Table
from superset.superset_typing import ResultSetColumnType
@@ -132,8 +133,11 @@ def get_virtual_table_metadata(dataset: SqlaTable) ->
list[ResultSetColumnType]:
db_engine_spec = dataset.database.db_engine_spec
original_sql = dataset.sql
try:
+ # Render with the params chart execution exposes, so the columns
+ # come from the same SQL the access check and chart-data render.
sql = dataset.get_template_processor().process_template(
- original_sql, **dataset.template_params_dict
+ original_sql,
+ **template_params_for_access_check(dataset.template_params_dict),
)
except UndefinedError as ex:
# ``process_template``'s Jinja *render* step re-raises a raw
diff --git a/superset/explore/utils.py b/superset/explore/utils.py
index 67f0d916a8a..147e1d118c1 100644
--- a/superset/explore/utils.py
+++ b/superset/explore/utils.py
@@ -66,8 +66,12 @@ def check_query_access(query_id: int) -> Optional[bool]:
# This is the one-time explore/create-chart transition the
# query-authorship bypass in raise_for_access is meant to
# cover; see allow_query_authorship_bypass's docstring there.
+ # Editorship of a dataset named after a table the query reads
+ # does not authorize it, as on the chart-data path.
security_manager.raise_for_access(
- query=query, allow_query_authorship_bypass=True
+ query=query,
+ allow_query_authorship_bypass=True,
+ allow_editorship=False,
)
except TemplateError as ex:
# raise_for_access() Jinja-renders the query's SQL to resolve
diff --git a/superset/security/manager.py b/superset/security/manager.py
index e45ba89fe24..2f537cd51ad 100644
--- a/superset/security/manager.py
+++ b/superset/security/manager.py
@@ -5031,9 +5031,10 @@ class SupersetSecurityManager( # pylint:
disable=too-many-public-methods
``schema_access`` / ``datasource_access`` checks still apply).
The paths that define or repoint a dataset over a table -- dataset
create, update, v1 import, and the legacy ``/datasource/save/``
- repoint -- and query-backed chart-data set this to False so that
- binding a dataset to (or reading a stored query against) a table
- requires real data access to that table, not editorship of a
+ repoint -- and the SQL Lab query paths (query-backed chart-data,
+ the Explore GET and the explore form-data step) set this to False
+ so that binding a dataset to (or reading a stored query against) a
+ table requires real data access to that table, not editorship of a
same-named dataset -- otherwise a no-table decoy dataset named
after the table would let its editor define a second dataset that
reads it. Unlike ``force_dataset_match`` this keeps the
@@ -5411,8 +5412,8 @@ class SupersetSecurityManager( # pylint:
disable=too-many-public-methods
# * ``allow_editorship=False`` -- the paths that define
# or repoint a dataset over a table (create, update, v1
# import, and the legacy ``/datasource/save/`` repoint)
- # and query-backed chart-data (which runs a stored
- # query's raw SQL) -- which keep the catalog/schema
+ # and the SQL Lab query paths (query-backed
+ # chart-data, Explore) -- which keep the catalog/schema
# fallthroughs but refuse editorship.
# Both must follow the user's live data-access grants, not
# editorship of a same-named dataset: any principal able to
diff --git a/tests/unit_tests/commands/dataset/test_create.py
b/tests/unit_tests/commands/dataset/test_create.py
index ddc2856cc88..16f135a2689 100644
--- a/tests/unit_tests/commands/dataset/test_create.py
+++ b/tests/unit_tests/commands/dataset/test_create.py
@@ -199,6 +199,17 @@ def test_create_dataset_schema_uses_template_params() ->
None:
assert command._properties["schema"] == "secret"
+def test_create_dataset_schema_ignores_reserved_template_params() -> None:
+ """Reserved processor names are not variables when deriving the schema."""
+ command = _validate_virtual_dataset(
+ "{% if removed_filters is defined %}SELECT * FROM decoy.t"
+ "{% else %}SELECT * FROM actual.t{% endif %}",
+ template_params='{"removed_filters": []}',
+ )
+
+ assert command._properties["schema"] == "actual"
+
+
def test_create_dataset_keeps_dropdown_schema_for_show_statement() -> None:
"""A metadata statement has no meaningful virtual-dataset schema."""
command = _validate_virtual_dataset(
diff --git
a/tests/unit_tests/commands/explore/test_get_query_authorship_bypass.py
b/tests/unit_tests/commands/explore/test_get_query_authorship_bypass.py
index 777bee73687..91b30f3dfdc 100644
--- a/tests/unit_tests/commands/explore/test_get_query_authorship_bypass.py
+++ b/tests/unit_tests/commands/explore/test_get_query_authorship_bypass.py
@@ -43,8 +43,13 @@ def test_query_datasource_uses_authorship_bypass(mocker:
MockerFixture) -> None:
_authorize_datasource(query, None)
+ # Editorship of a dataset named after a table the query reads is a decoy
+ # any dataset creator can make, so it must not authorize the query.
mock_sm.raise_for_access.assert_called_once_with(
- query=query, datasource=query, allow_query_authorship_bypass=True
+ query=query,
+ datasource=query,
+ allow_query_authorship_bypass=True,
+ allow_editorship=False,
)
@@ -75,14 +80,103 @@ def test_query_datasource_still_reaches_extra_bypass_hook(
def test_saved_chart_path_unaffected(mocker: MockerFixture) -> None:
"""A saved chart keeps going through the chart-based access check."""
- slc = Slice()
+ slc = Slice(datasource_id=1, datasource_type="table")
mock_sm = mocker.patch("superset.commands.explore.get.security_manager")
- _authorize_datasource(SqlaTable(), slc)
+ _authorize_datasource(SqlaTable(id=1), slc)
mock_sm.raise_for_access.assert_called_once_with(chart=slc)
+def test_saved_chart_with_other_query_checks_the_query(
+ mocker: MockerFixture,
+) -> None:
+ """
+ ``form_data`` in the request overrides the chart's saved datasource, so
+ ``?slice_id=<chart>&form_data={"datasource": "<id>__query"}`` resolves to
+ a SQL Lab query the chart doesn't use. Access to the chart must not
+ authorize it: the query goes through the same check as without a chart.
+ """
+ slc = Slice(datasource_id=1, datasource_type="table")
+ query = Query(id=1)
+ mock_sm = mocker.patch("superset.commands.explore.get.security_manager")
+
+ _authorize_datasource(query, slc)
+
+ assert mock_sm.raise_for_access.call_args_list == [
+ mocker.call(chart=slc),
+ mocker.call(
+ query=query,
+ datasource=query,
+ allow_query_authorship_bypass=True,
+ allow_editorship=False,
+ ),
+ ]
+
+
+def test_saved_chart_with_other_dataset_checks_the_dataset(
+ mocker: MockerFixture,
+) -> None:
+ """A dataset the chart doesn't use needs its own datasource check."""
+ slc = Slice(datasource_id=1, datasource_type="table")
+ dataset = SqlaTable(id=2)
+ mock_sm = mocker.patch("superset.commands.explore.get.security_manager")
+
+ _authorize_datasource(dataset, slc)
+
+ assert mock_sm.raise_for_access.call_args_list == [
+ mocker.call(chart=slc),
+ mocker.call(datasource=dataset),
+ ]
+
+
+def test_explore_with_slice_denies_query_from_form_data(
+ mocker: MockerFixture,
+) -> None:
+ """
+ End to end through ``GetExploreCommand.run()``: a readable chart plus a
+ ``form_data`` that points at another user's query is denied instead of
+ returning the query's SQL.
+ """
+ from superset.commands.explore.get import GetExploreCommand
+ from superset.commands.explore.parameters import CommandParameters
+ from superset.exceptions import SupersetSecurityException
+
+ slc = Slice(id=10, datasource_id=1, datasource_type="table")
+ query = Query(id=7, sql="SELECT secret FROM public.customer_pii")
+ mocker.patch(
+ "superset.commands.explore.get.get_form_data",
+ return_value=({"slice_id": 10, "datasource": "7__query"}, slc),
+ )
+ mocker.patch(
+ "superset.commands.explore.get.DatasourceDAO.get_datasource",
+ return_value=query,
+ )
+ mock_sm = mocker.patch(
+ "superset.commands.explore.get.security_manager", new_callable=Mock
+ )
+
+ def raise_for_access(**kwargs: object) -> None:
+ if "query" in kwargs:
+ raise SupersetSecurityException(mocker.MagicMock())
+
+ mock_sm.raise_for_access.side_effect = raise_for_access
+
+ command = GetExploreCommand(
+ CommandParameters(
+ permalink_key=None,
+ form_data_key=None,
+ datasource_id=None,
+ datasource_type=None,
+ slice_id=10,
+ )
+ )
+ with pytest.raises(SupersetSecurityException):
+ command.run()
+
+ mock_sm.raise_for_access.assert_any_call(chart=slc)
+
+
def test_table_datasource_path_unaffected(mocker: MockerFixture) -> None:
"""A registered dataset keeps going through the generic datasource
check."""
dataset = SqlaTable()
diff --git a/tests/unit_tests/connectors/sqla/utils_test.py
b/tests/unit_tests/connectors/sqla/utils_test.py
index ca46374cf7a..4a2a925e234 100644
--- a/tests/unit_tests/connectors/sqla/utils_test.py
+++ b/tests/unit_tests/connectors/sqla/utils_test.py
@@ -446,6 +446,30 @@ def test_get_virtual_table_metadata(mocker: MockerFixture)
-> None:
assert get_virtual_table_metadata(dataset) == [{"name": "one", "type":
"INTEGER"}]
+def test_get_virtual_table_metadata_drops_reserved_template_params(
+ mocker: MockerFixture,
+) -> None:
+ """
+ Chart execution never exposes ``removed_filters`` and the other names the
+ template processor reserves as template variables, and neither does the
+ access check. Column sync must render the same SQL, or a template that
+ branches on one of them would run a branch the check never saw.
+ """
+ mocker.patch(
+ "superset.connectors.sqla.utils.get_columns_description",
+ return_value=[{"name": "one", "type": "INTEGER"}],
+ )
+ dataset = mocker.MagicMock(sql="SELECT 1 AS one")
+ dataset.template_params_dict = {"removed_filters": [], "table": 1, "t":
"x"}
+ dataset.database.db_engine_spec.engine = "postgresql"
+ process_template = dataset.get_template_processor().process_template
+ process_template.return_value = dataset.sql
+
+ get_virtual_table_metadata(dataset)
+
+ process_template.assert_called_once_with(dataset.sql, t="x")
+
+
def test_get_virtual_table_metadata_mutating(mocker: MockerFixture) -> None:
"""
Test the `get_virtual_table_metadata` function with mutating SQL.
diff --git a/tests/unit_tests/explore/utils_test.py
b/tests/unit_tests/explore/utils_test.py
index d000fc210ae..723601fd1d8 100644
--- a/tests/unit_tests/explore/utils_test.py
+++ b/tests/unit_tests/explore/utils_test.py
@@ -456,6 +456,45 @@ def test_query_no_access(mocker: MockerFixture, client) ->
None:
)
+def test_query_access_ignores_editorship_of_same_named_dataset(
+ mocker: MockerFixture, client
+) -> None:
+ """
+ Editorship of a dataset named after a table the stored query reads must
+ not authorize the query for a user who is not its author. Any principal
+ able to create datasets can make a ``SELECT 1`` decoy under that name,
+ so accepting editorship would let them open another user's query (and
+ its SQL) in Explore.
+ """
+ from superset.connectors.sqla.models import SqlaTable
+ from superset.explore.utils import check_datasource_access
+ from superset.models.sql_lab import Query
+
+ database = mocker.MagicMock()
+ database.get_default_catalog.return_value = None
+ database.get_default_schema_for_query.return_value = "public"
+ mocker.patch(
+ query_find_by_id,
+ return_value=Query(
+ database=database,
+ sql="select * from customer_pii",
+ user_id=2,
+ status=QueryStatus.SUCCESS,
+ ),
+ )
+ mocker.patch(query_datasources_by_name, return_value=[SqlaTable()])
+ mocker.patch(is_admin, return_value=False)
+ mocker.patch(is_editor, return_value=True)
+ mocker.patch(can_access, return_value=False)
+
+ with override_user(User(id=1)):
+ with raises(SupersetSecurityException):
+ check_datasource_access(
+ datasource_id=1,
+ datasource_type=DatasourceType.QUERY,
+ )
+
+
def test_unsaved_query_explore_allows_the_query_author(
mocker: MockerFixture, client
) -> None:
diff --git a/tests/unit_tests/subjects/test_raise_for_access.py
b/tests/unit_tests/subjects/test_raise_for_access.py
index da6bb335c00..cd70cd79d39 100644
--- a/tests/unit_tests/subjects/test_raise_for_access.py
+++ b/tests/unit_tests/subjects/test_raise_for_access.py
@@ -590,8 +590,12 @@ def
test_explore_command_uses_chart_access_when_slice_exists(app_context):
mock_slc.dashboards = []
mock_slc.created_by = None
mock_slc.changed_by = None
+ mock_slc.datasource_id = 1
+ mock_slc.datasource_type = "table"
mock_datasource = MagicMock()
+ mock_datasource.id = 1
+ mock_datasource.type = "table"
mock_datasource.name = "test"
mock_datasource.default_endpoint = None
