rusackas commented on code in PR #42929:
URL: https://github.com/apache/superset/pull/42929#discussion_r3763263328
##########
superset/commands/report/base.py:
##########
@@ -58,6 +68,52 @@ def run(self) -> Any:
def validate(self) -> None:
pass
+ def validate_alert_query(
+ self,
+ database: Database,
+ sql: str,
+ exceptions: list[ValidationError],
+ ) -> None:
+ """
+ Validate alert SQL at save time: it must parse as a single statement,
+ must not mutate state unless the database allows DML, and the saving
+ user must be authorized for the tables it reads. Templated SQL that
+ only parses after rendering is validated at execution time on the
+ rendered query.
+ """
+ contains_jinja = bool(_JINJA_BLOCK_RE.search(sql))
+ try:
+ script = SQLScript(sql, engine=database.backend)
+ except SupersetParseError as ex:
+ if not contains_jinja:
+ exceptions.append(
+ ValidationError(
+ _("Invalid SQL: %(error)s", error=ex.error.message),
+ field_name="sql",
+ )
+ )
+ return
+ if len(script.statements) != 1:
+ exceptions.append(AlertQueryMultipleStatementsValidationError())
Review Comment:
This only skips the save-time check on a parse failure, which is the case
that actually matters: Jinja confined to a string literal (e.g. a templated
date bound) still parses as valid SQL, and the statement-count/DML/access
checks on that literal text are still correct and worth running early.
Blanket-skipping every query that contains `{{ }}` would waive save-time
validation for that common case too, which is a step back, not forward -- and
the rendered SQL is re-validated at execution time regardless
(_validate_rendered_sql in alert.py).
##########
superset/connectors/sqla/models.py:
##########
@@ -1215,6 +1217,14 @@ def get_sqla_col(
msg=msg,
)
) from ex
+ if expression != self.expression:
+ # Re-check the rendered expression before embedding it.
+ expression = validate_rendered_expression(
+ expression,
+ self.database,
+ self.table.catalog if self.table else None,
+ self.table.schema if self.table else None,
+ )
Review Comment:
Fixed. `validate_rendered_expression` now converts every failure mode
(parse, set operation, disallowed sub-query, sanitization) to
`QueryObjectValidationError`, matching
`validate_stored_expression_at_query_time`, instead of letting
`SupersetSecurityException` escape uncaught from the call sites.
##########
superset/models/helpers.py:
##########
@@ -329,6 +329,65 @@ def validate_stored_expression_at_query_time(
return expression
+def validate_rendered_expression(
+ expression: str,
+ database: Database,
+ catalog: str | None,
+ schema: str | None,
+) -> str:
+ """
+ Apply the stored-expression validation policy to a rendered expression.
+
+ Query-time counterpart to ``validate_stored_expression``: it runs on the
+ already-rendered expression that is embedded via ``literal_column`` and
+ applies the same policy, failing closed on unparseable results.
+
+ :param expression: the rendered expression
+ :returns: the expression to embed, possibly rewritten with RLS predicates
+ :raises SupersetSecurityException: on multi-statement, set-operation, or
+ disallowed sub-query expressions
+ :raises QueryObjectValidationError: if the clause fails sanitization
+ """
+ engine = database.backend
+ wrapped = f"SELECT {expression}"
+
+ try:
+ parsed = SQLStatement(wrapped, engine)
+ except SupersetParseError as ex:
+ raise SupersetSecurityException(
+ SupersetError(
+ error_type=SupersetErrorType.ADHOC_SUBQUERY_NOT_ALLOWED_ERROR,
+ message=_(
+ "Custom SQL fields cannot be parsed as a single SQL
statement."
+ ),
+ level=ErrorLevel.ERROR,
+ )
+ ) from ex
+
+ if parsed.is_set_operation():
+ raise SupersetSecurityException(
+ SupersetError(
+ error_type=SupersetErrorType.ADHOC_SUBQUERY_NOT_ALLOWED_ERROR,
+ message=_("Custom SQL fields cannot contain set operations."),
+ level=ErrorLevel.ERROR,
+ )
+ )
+
Review Comment:
Same fix as the models.py thread: `validate_rendered_expression` now always
converts to `QueryObjectValidationError`, so this call site gets consistent
semantics without needing its own conversion handler.
##########
superset/utils/rls.py:
##########
@@ -204,6 +204,10 @@ def collect_rls_predicates_for_sql(
}
)
except Exception:
- # If we can't parse the SQL, return empty list
- # This ensures RLS application failure doesn't break caching
- return []
+ # The applicable predicates could not be determined (e.g. the SQL
+ # could not be parsed); scope the cache key to the current user.
+ from superset.utils.core import (
+ get_user_id, # pylint: disable=import-outside-toplevel
+ )
+
+ return [f"rls-predicates-unresolved:{get_user_id()}"]
Review Comment:
Fixed. The unresolved-predicates cache-key fallback now keys guest users by
a hash of their token's RLS rules and resources instead of `get_user_id()`,
which is `None` for every guest and was collapsing all of them onto the same
cache entry.
--
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]
