rusackas commented on code in PR #42929:
URL: https://github.com/apache/superset/pull/42929#discussion_r3742516981
##########
superset/commands/report/alert.py:
##########
@@ -220,6 +235,17 @@ def _execute_query(self) -> pd.DataFrame:
raise ReportScheduleExecutorNotFoundError(username)
with override_user(user):
+ # Run table-level authorization as the executing user against
+ # the rendered SQL.
+ try:
+ security_manager.raise_for_access(
+ database=self._report_schedule.database,
+ sql=rendered_sql,
+ )
Review Comment:
Agreed. Added force_dataset_match=True to both the save-time and
execution-time checks so alert SQL is scoped the same way SQL Lab is.
##########
superset/commands/report/alert.py:
##########
@@ -181,6 +183,18 @@ def _get_alert_metadata_from_object(self) -> dict[str,
Any]:
"execution_id": self._execution_id,
}
+ def _validate_rendered_sql(self, rendered_sql: str) -> None:
+ """
+ Enforce SQL-level constraints on the rendered alert query: a single
+ statement, and no mutations unless the database allows DML.
+ """
+ database = self._report_schedule.database
+ script = SQLScript(rendered_sql, engine=database.backend)
+ if len(script.statements) > 1:
+ raise AlertQueryError(message=_("Alert query must be a single
statement"))
+ if script.has_mutation() and not database.allow_dml:
Review Comment:
Good point. Changed it to require exactly one statement, so an empty or
comment-only query gets a proper validation error instead of failing later at
run time.
##########
superset/commands/report/alert.py:
##########
@@ -196,6 +210,7 @@ def _execute_query(self) -> pd.DataFrame:
try:
rendered_sql =
sql_template.process_template(self._report_schedule.sql)
Review Comment:
The SQL mutator and the limit wrapper are operator-controlled, so they sit
inside the trust boundary described in SECURITY.md, and apply_limit only wraps
the query with a LIMIT. Keeping the check on the rendered SQL rather than
re-authorizing after trusted config runs.
##########
superset/models/helpers.py:
##########
@@ -3020,9 +3080,40 @@ def get_from_clause(
if rls_applied:
from_sql = parsed_script.format()
- except Exception as ex:
- # Log the error but don't fail - RLS application is best-effort
- logger.warning("Failed to apply RLS to virtual dataset SQL:
%s", ex)
+ except Exception as ex: # pylint: disable=broad-except
+ # RLS injection failures fail closed: only continue when it is
+ # positively confirmed that no RLS predicates apply to the
+ # referenced tables; any other outcome aborts the query.
+ try:
+ rls_required = any(
+ get_predicates_for_table(
+ table.qualify(
+ catalog=self.catalog,
+ schema=self.schema or default_schema or "",
+ ),
+ self.database,
+ self.database.get_default_catalog(),
+ exclude_dataset_id=self_id,
+ )
+ for statement in parsed_script.statements
+ for table in statement.tables
+ )
+ except Exception: # pylint: disable=broad-except
Review Comment:
This one is intentional. It's a fail-closed path, so we want to catch
anything and fall back to the safe behavior rather than narrow it to specific
exception types.
##########
superset/commands/report/exceptions.py:
##########
@@ -40,6 +40,38 @@ def __init__(self) -> None:
super().__init__(_("Database does not exist"), field_name="database")
+class AlertQueryMultipleStatementsValidationError(ValidationError):
+ """
+ Marshmallow validation error for alert SQL containing multiple statements
+ """
+
+ def __init__(self) -> None:
+ super().__init__(
+ _("Alert query must be a single statement"),
+ field_name="sql",
+ )
+
+
+class AlertQueryDMLNotAllowedValidationError(ValidationError):
+ """
+ Marshmallow validation error for alert SQL that mutates state on a
+ database that does not allow DML
+ """
+
+ def __init__(self) -> None:
+ super().__init__(_("Alert query must be read-only"), field_name="sql")
+
+
+class AlertQueryDataAccessValidationError(ValidationError):
+ """
+ Marshmallow validation error for alert SQL referencing tables the user
+ is not authorized to query
+ """
+
+ def __init__(self, message: str) -> None:
+ super().__init__(message, field_name="sql")
+
Review Comment:
Added unit tests for the DML-not-allowed and data-access cases in
base_test.py.
##########
tests/unit_tests/connectors/sqla/models_test.py:
##########
@@ -1486,6 +1486,33 @@ def
test_validate_stored_expression_rejects_subquery_around_jinja(
)
+def test_get_sqla_col_revalidates_rendered_jinja_expression(
+ mocker: MockerFixture,
+) -> None:
+ """
+ A Jinja block that renders into a sub-query must be rejected at query
+ time: save-time validation only sees the block as a placeholder, so the
+ rendered expression is re-validated before it is embedded via
+ ``literal_column``.
+ """
+ # A real Database (not a MagicMock) so the ORM relationship assignment on
+ # SqlaTable has a valid instance state; sqlite gives a concrete backend.
+ database = Database(database_name="t", sqlalchemy_uri="sqlite://")
+ mocker.patch("superset.models.helpers.is_feature_enabled",
return_value=False)
Review Comment:
The validator resolves is_feature_enabled from the helpers module namespace,
so patching it there is correct. The test passes and exercises the intended
path.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
