This is an automated email from the ASF dual-hosted git repository.
sha174n 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 954dbc9ff61 fix(dataset): validate SQL table access on virtual dataset
import (#43999)
954dbc9ff61 is described below
commit 954dbc9ff61675fd61efb6b78b7f3303dd240664
Author: Shaitan <[email protected]>
AuthorDate: Wed Sep 9 14:02:52 2026 +0100
fix(dataset): validate SQL table access on virtual dataset import (#43999)
Co-authored-by: sha174n <[email protected]>
Co-authored-by: Claude Opus 4.8 <[email protected]>
---
superset/commands/dataset/importers/v1/utils.py | 46 +++++++-
.../datasets/commands/importers/v1/import_test.py | 118 +++++++++++++++++++++
2 files changed, 163 insertions(+), 1 deletion(-)
diff --git a/superset/commands/dataset/importers/v1/utils.py
b/superset/commands/dataset/importers/v1/utils.py
index cd41d3ec812..8f984b98eef 100644
--- a/superset/commands/dataset/importers/v1/utils.py
+++ b/superset/commands/dataset/importers/v1/utils.py
@@ -29,6 +29,7 @@ from urllib.request import HTTPRedirectHandler
import pandas as pd
from flask import current_app as app
+from jinja2.exceptions import TemplateError
from pandas.errors import OutOfBoundsDatetime
from sqlalchemy import BigInteger, Boolean, Date, DateTime, Float, String, Text
from sqlalchemy.exc import MultipleResultsFound
@@ -41,6 +42,7 @@ from superset.commands.dataset.exceptions import (
MultiCatalogDisabledValidationError,
)
from superset.commands.exceptions import ImportFailedError
+from superset.commands.importers.exceptions import IncorrectFormatError
from superset.commands.importers.v1.utils import (
find_existing_by_import_identity,
find_existing_for_import,
@@ -48,7 +50,7 @@ from superset.commands.importers.v1.utils import (
from superset.connectors.sqla.models import SqlaTable
from superset.constants import SKIP_VISIBILITY_FILTER_CLASSES
from superset.daos.dataset import DatasetDAO
-from superset.exceptions import SupersetSecurityException
+from superset.exceptions import SupersetParseError, SupersetSecurityException
from superset.models.core import Database
from superset.models.helpers import ChildMultipleResultsFound
from superset.sql.parse import Table
@@ -246,6 +248,25 @@ def validate_catalog(config: dict[str, Any]) -> None:
raise MultiCatalogDisabledValidationError()
+def _get_template_params(dataset: SqlaTable) -> dict[str, Any]:
+ """
+ Return a dataset's template params as a dict for the access check.
+
+ Delegates to ``template_params_dict`` so the check reads the params the
+ same way rendering does, but tolerates the two inputs that property does
+ not guard: it raises on malformed JSON and passes non-dict JSON through.
+ Either yields an empty dict so the check still runs against the raw SQL.
+ """
+ try:
+ params = dataset.template_params_dict
+ except json.JSONDecodeError:
+ logger.warning(
+ "Unable to decode template_params for dataset %s",
dataset.table_name
+ )
+ return {}
+ return params if isinstance(params, dict) else {}
+
+
def import_dataset( # noqa: C901
config: dict[str, Any],
overwrite: bool = False,
@@ -564,8 +585,31 @@ def import_dataset( # noqa: C901
if not ignore_permissions:
try:
security_manager.raise_for_access(datasource=dataset)
+ # For virtual datasets, also validate access to the tables the
+ # SQL references, matching the create and update commands.
+ if dataset.sql:
+ security_manager.raise_for_access(
+ database=dataset.database,
+ sql=dataset.sql,
+ # Empty strings would qualify table names against a
+ # nonexistent catalog/schema, so they are normalized to
+ # None to let the check fall back to the defaults.
+ catalog=dataset.catalog or None,
+ schema=dataset.schema or None,
+ # Jinja-templated SQL only reveals the tables it really
+ # references once the params are applied, so they are
+ # forwarded rather than left to default to empty.
+ template_params=_get_template_params(dataset),
+ )
except SupersetSecurityException as ex:
raise DatasetAccessDeniedError() from ex
+ # SQL that can't be parsed or rendered can't be access-checked, so fail
+ # closed, but as an invalid payload (422) rather than an access denial
+ # or a server error.
+ except SupersetParseError as ex:
+ raise IncorrectFormatError(f"Invalid SQL: {ex.error.message}")
from ex
+ except TemplateError as ex:
+ raise IncorrectFormatError(f"Invalid SQL: {ex}") from ex
# `has_table` opens a live connection to the target database to run a
# schema-introspection query. Its result is only ever consulted below to
diff --git a/tests/unit_tests/datasets/commands/importers/v1/import_test.py
b/tests/unit_tests/datasets/commands/importers/v1/import_test.py
index d4943ea4df6..8aa16ce09f8 100644
--- a/tests/unit_tests/datasets/commands/importers/v1/import_test.py
+++ b/tests/unit_tests/datasets/commands/importers/v1/import_test.py
@@ -29,6 +29,7 @@ import pytest
import yaml
from flask import current_app
from flask_appbuilder.security.sqla.models import Role, User
+from jinja2.exceptions import TemplateError
from marshmallow import ValidationError
from pytest_mock import MockerFixture
from sqlalchemy.orm.session import Session
@@ -44,8 +45,11 @@ from superset.commands.dataset.importers.v1.utils import (
validate_data_uri,
)
from superset.commands.exceptions import ImportFailedError
+from superset.commands.importers.exceptions import IncorrectFormatError
from superset.connectors.sqla.models import SqlaTable, TableColumn
from superset.datasets.schemas import ImportV1DatasetSchema
+from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
+from superset.exceptions import SupersetParseError, SupersetSecurityException
from superset.models.core import Database
from superset.utils import json
from superset.utils.core import override_user
@@ -630,6 +634,120 @@ def _dataset_config_with_children(
}
+def test_import_dataset_virtual_checks_sql_table_access(
+ mocker: MockerFixture, session: Session
+) -> None:
+ """
+ A virtual dataset import validates access to the tables its SQL
+ references, not only access to the dataset object itself.
+ """
+ engine = db.session.get_bind()
+ SqlaTable.metadata.create_all(engine) # pylint: disable=no-member
+
+ database = Database(database_name="my_database",
sqlalchemy_uri="sqlite://")
+ db.session.add(database)
+ db.session.flush()
+
+ mocker.patch.object(security_manager, "can_access", return_value=True)
+
+ def raise_for_access(**kwargs: object) -> None:
+ # Deny only the SQL/table-level check; the datasource-level check
passes.
+ if kwargs.get("sql"):
+ raise SupersetSecurityException(
+ SupersetError(
+ error_type=SupersetErrorType.TABLE_SECURITY_ACCESS_ERROR,
+ message="You need access to the following tables",
+ level=ErrorLevel.ERROR,
+ )
+ )
+
+ mocker.patch.object(
+ security_manager, "raise_for_access", side_effect=raise_for_access
+ )
+
+ config = copy.deepcopy(dataset_fixture)
+ config["database_id"] = database.id
+ config["sql"] = "SELECT * FROM secret_table"
+
+ with pytest.raises(DatasetAccessDeniedError):
+ import_dataset(config)
+
+
+def test_import_dataset_virtual_sql_check_receives_template_params(
+ mocker: MockerFixture, session: Session
+) -> None:
+ """
+ Jinja-templated SQL only resolves its real table references once the
+ template params are applied, so they must reach the access check.
+ """
+ engine = db.session.get_bind()
+ SqlaTable.metadata.create_all(engine) # pylint: disable=no-member
+
+ database = Database(database_name="my_database",
sqlalchemy_uri="sqlite://")
+ db.session.add(database)
+ db.session.flush()
+
+ mocker.patch.object(security_manager, "can_access", return_value=True)
+ raise_for_access = mocker.patch.object(security_manager,
"raise_for_access")
+
+ config = copy.deepcopy(dataset_fixture)
+ config["database_id"] = database.id
+ config["sql"] = (
+ "{% if flag %}SELECT * FROM secret_table{% else %}SELECT 1{% endif %}"
# noqa: E501
+ )
+ config["template_params"] = {"flag": True}
+
+ import_dataset(config)
+
+ sql_call = next(
+ call for call in raise_for_access.call_args_list if
call.kwargs.get("sql")
+ )
+ assert sql_call.kwargs["template_params"] == {"flag": True}
+ # Empty strings in the export payload must not be forwarded verbatim.
+ assert sql_call.kwargs["schema"] is None
+
+
[email protected](
+ "error",
+ [
+ SupersetParseError("SELECT", message="Could not parse SQL"),
+ TemplateError("Could not render SQL"),
+ ],
+)
+def test_import_dataset_virtual_invalid_sql_is_rejected_as_bad_payload(
+ error: Exception, mocker: MockerFixture, session: Session
+) -> None:
+ """
+ SQL that cannot be parsed or rendered can't be access-checked, so the
+ import fails closed as an invalid payload rather than an access denial.
+ """
+ engine = db.session.get_bind()
+ SqlaTable.metadata.create_all(engine) # pylint: disable=no-member
+
+ database = Database(database_name="my_database",
sqlalchemy_uri="sqlite://")
+ db.session.add(database)
+ db.session.flush()
+
+ mocker.patch.object(security_manager, "can_access", return_value=True)
+
+ def raise_for_access(**kwargs: object) -> None:
+ # Only the SQL/table-level check fails, so the test still fails if the
+ # datasource-level call is the one that raises.
+ if kwargs.get("sql"):
+ raise error
+
+ mocker.patch.object(
+ security_manager, "raise_for_access", side_effect=raise_for_access
+ )
+
+ config = copy.deepcopy(dataset_fixture)
+ config["database_id"] = database.id
+ config["sql"] = "SELECT +/"
+
+ with pytest.raises(IncorrectFormatError, match="Invalid SQL"):
+ import_dataset(config)
+
+
def test_import_dataset_schema_rejects_duplicate_metric_uuids() -> None:
"""
Two metrics sharing a UUID must be rejected by the import schema.
