sha174n commented on code in PR #42087:
URL: https://github.com/apache/superset/pull/42087#discussion_r3731717517
##########
tests/unit_tests/connectors/sqla/models_test.py:
##########
@@ -1177,3 +1179,86 @@ def
test_validate_stored_expression_rejects_subquery_around_jinja(
None,
"(SELECT password FROM ab_user LIMIT 1) {# x #}",
)
+
+
+def test_get_sqla_col_validates_stored_expression_at_query_time(
+ mocker: MockerFixture,
+) -> None:
+ """
+ A stored calculated-column expression must be validated at the query sink,
+ not only at save time. ``get_sqla_col`` routes the expression through
+ ``validate_adhoc_subquery`` so a disallowed sub-query is rejected even when
+ it reaches the query with the save-time check bypassed (templating, the
+ create path, or older data). Locks in the query-time gate.
+ """
+ tc = TableColumn(
+ column_name="leak",
+ expression="(SELECT password FROM ab_user LIMIT 1)",
+ )
+ tc.table = mocker.MagicMock()
+ tc.table.database.backend = "sqlite"
+ spy = mocker.patch(
+ "superset.models.helpers.validate_adhoc_subquery",
+ side_effect=SupersetSecurityException(
+ SupersetError(
+ message="Sub-queries are not allowed in stored expressions.",
+ error_type=SupersetErrorType.ADHOC_SUBQUERY_NOT_ALLOWED_ERROR,
+ level=ErrorLevel.ERROR,
+ )
+ ),
+ )
+ with pytest.raises(SupersetSecurityException):
+ tc.get_sqla_col()
+ spy.assert_called_once()
+
+
+def test_get_timestamp_expression_validates_stored_expression_at_query_time(
+ mocker: MockerFixture,
+) -> None:
+ """
+ The timestamp-expression sink must enforce the same query-time gate as
+ ``get_sqla_col``: a stored datetime column expression is routed through
+ ``validate_adhoc_subquery`` before it reaches ``literal_column``, so a
+ disallowed sub-query is rejected on the time-grained query path too.
+ """
+ tc = TableColumn(
+ column_name="ds",
+ expression="(SELECT ts FROM ab_user LIMIT 1)",
+ )
+ tc.table = mocker.MagicMock()
+ tc.table.database.backend = "sqlite"
+ spy = mocker.patch(
+ "superset.models.helpers.validate_adhoc_subquery",
+ side_effect=SupersetSecurityException(
+ SupersetError(
+ message="Sub-queries are not allowed in stored expressions.",
+ error_type=SupersetErrorType.ADHOC_SUBQUERY_NOT_ALLOWED_ERROR,
+ level=ErrorLevel.ERROR,
+ )
+ ),
+ )
+ with pytest.raises(SupersetSecurityException):
+ tc.get_timestamp_expression(time_grain=None)
+ spy.assert_called_once()
+
+
+def test_get_sqla_col_falls_back_when_stored_expression_unparseable(
+ mocker: MockerFixture,
+) -> None:
+ """
+ A stored expression using dialect-specific syntax that sqlglot cannot parse
+ (e.g. ``DATE_ADD(ds, 1)`` on MySQL) pre-dates the query-time gate and went
+ to the query unparsed. A parse failure must fall back to the raw expression
+ rather than break the query; a genuine sub-query still parses and is
caught.
+ """
+ tc = TableColumn(column_name="ds", expression="DATE_ADD(ds, 1)")
+ tc.table = mocker.MagicMock()
+ tc.table.database.backend = "mysql"
+ mocker.patch(
+ "superset.models.helpers.validate_adhoc_subquery",
+ side_effect=SupersetParseError("DATE_ADD(ds, 1)", "mysql"),
+ )
+ literal = mocker.patch("superset.connectors.sqla.models.literal_column")
+ tc.get_sqla_col()
+ # The raw expression reaches ``literal_column`` unchanged; no exception.
+ assert literal.call_args.args[0] == "DATE_ADD(ds, 1)"
Review Comment:
Reworked this into a real-parse test: it now drives the actual validator
through get_sqla_col with a MySQL backend, so it fails if the query-time gate
is removed rather than relying on a mocked validator. 271c5f5612
##########
superset/models/helpers.py:
##########
@@ -259,6 +260,31 @@ def validate_adhoc_subquery(
return parsed_statement.format() if rls_applied else sql
+def validate_stored_expression_at_query_time(
+ expression: str,
+ database: Database,
+ catalog: str | None,
+ schema: str,
+ engine: str,
+) -> str:
+ """
+ Validate a stored column/metric expression at the point of use, applying
the
+ same sub-query policy and RLS injection as adhoc expressions. The save-time
+ check can be deferred past (Jinja templating, the create path, older data),
+ so the query sink is the reliable place to enforce it.
+
+ Stored expressions can contain dialect-specific syntax sqlglot cannot parse
+ (e.g. ``DATE_ADD(ds, 1)`` on MySQL); such expressions pre-date this gate
and
+ went straight to the query unparsed, so a parse failure falls back to the
raw
+ expression rather than breaking the query. A genuine sub-query still parses
+ and is caught.
+ """
+ try:
+ return validate_adhoc_subquery(expression, database, catalog, schema,
engine)
+ except SupersetParseError:
+ return expression
Review Comment:
Added a permissive-dialect re-check as a detector before the raw fallback
(and a warning log when validation is skipped), so a sub-query next to
unparseable dialect syntax is still caught; updated the docstring accordingly
and added a DATE_ADD(ds, 1) + (SELECT 1) test on MySQL. 271c5f5612
##########
tests/unit_tests/connectors/sqla/models_test.py:
##########
@@ -1177,3 +1179,86 @@ def
test_validate_stored_expression_rejects_subquery_around_jinja(
None,
"(SELECT password FROM ab_user LIMIT 1) {# x #}",
)
+
+
+def test_get_sqla_col_validates_stored_expression_at_query_time(
+ mocker: MockerFixture,
+) -> None:
+ """
+ A stored calculated-column expression must be validated at the query sink,
+ not only at save time. ``get_sqla_col`` routes the expression through
+ ``validate_adhoc_subquery`` so a disallowed sub-query is rejected even when
+ it reaches the query with the save-time check bypassed (templating, the
+ create path, or older data). Locks in the query-time gate.
+ """
+ tc = TableColumn(
+ column_name="leak",
+ expression="(SELECT password FROM ab_user LIMIT 1)",
+ )
+ tc.table = mocker.MagicMock()
+ tc.table.database.backend = "sqlite"
+ spy = mocker.patch(
+ "superset.models.helpers.validate_adhoc_subquery",
+ side_effect=SupersetSecurityException(
+ SupersetError(
+ message="Sub-queries are not allowed in stored expressions.",
+ error_type=SupersetErrorType.ADHOC_SUBQUERY_NOT_ALLOWED_ERROR,
+ level=ErrorLevel.ERROR,
+ )
+ ),
+ )
+ with pytest.raises(SupersetSecurityException):
+ tc.get_sqla_col()
+ spy.assert_called_once()
+
+
+def test_get_timestamp_expression_validates_stored_expression_at_query_time(
+ mocker: MockerFixture,
+) -> None:
+ """
+ The timestamp-expression sink must enforce the same query-time gate as
+ ``get_sqla_col``: a stored datetime column expression is routed through
+ ``validate_adhoc_subquery`` before it reaches ``literal_column``, so a
+ disallowed sub-query is rejected on the time-grained query path too.
+ """
+ tc = TableColumn(
+ column_name="ds",
+ expression="(SELECT ts FROM ab_user LIMIT 1)",
+ )
+ tc.table = mocker.MagicMock()
+ tc.table.database.backend = "sqlite"
+ spy = mocker.patch(
+ "superset.models.helpers.validate_adhoc_subquery",
+ side_effect=SupersetSecurityException(
+ SupersetError(
+ message="Sub-queries are not allowed in stored expressions.",
+ error_type=SupersetErrorType.ADHOC_SUBQUERY_NOT_ALLOWED_ERROR,
+ level=ErrorLevel.ERROR,
+ )
+ ),
+ )
+ with pytest.raises(SupersetSecurityException):
+ tc.get_timestamp_expression(time_grain=None)
+ spy.assert_called_once()
+
+
+def test_get_sqla_col_falls_back_when_stored_expression_unparseable(
+ mocker: MockerFixture,
+) -> None:
+ """
+ A stored expression using dialect-specific syntax that sqlglot cannot parse
+ (e.g. ``DATE_ADD(ds, 1)`` on MySQL) pre-dates the query-time gate and went
+ to the query unparsed. A parse failure must fall back to the raw expression
+ rather than break the query; a genuine sub-query still parses and is
caught.
+ """
+ tc = TableColumn(column_name="ds", expression="DATE_ADD(ds, 1)")
+ tc.table = mocker.MagicMock()
+ tc.table.database.backend = "mysql"
+ mocker.patch(
+ "superset.models.helpers.validate_adhoc_subquery",
+ side_effect=SupersetParseError("DATE_ADD(ds, 1)", "mysql"),
+ )
+ literal = mocker.patch("superset.connectors.sqla.models.literal_column")
+ tc.get_sqla_col()
+ # The raw expression reaches ``literal_column`` unchanged; no exception.
+ assert literal.call_args.args[0] == "DATE_ADD(ds, 1)"
Review Comment:
Reworked all sink tests to run the real validator via a _stored_col helper
(real MySQL parse), and added coverage for the metric sink and
convert_tbl_column_to_sqla_col. 271c5f5612
##########
superset/connectors/sqla/models.py:
##########
@@ -1103,6 +1113,7 @@ def get_sqla_col(
msg=msg,
)
) from ex
+ expression = self._validate_stored_expression(expression)
Review Comment:
Good call, the shared helper now converts SupersetSecurityException to
QueryObjectValidationError, so all five sinks surface it as a chart-level 400
like adhoc_metric_to_sqla/adhoc_column_to_sqla instead of a raw 403. 271c5f5612
--
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]
