gabotorresruiz commented on code in PR #42087:
URL: https://github.com/apache/superset/pull/42087#discussion_r3730858946
##########
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:
Hi @sha174n, thanks for this. Enforcing at the query sink instead of
trusting save time is the right call.
This block worries me a bit, mostly because of what the docstring right
above it promises. `SupersetParseError` covers two different situations: benign
dialect syntax sqlglot cannot handle, and SQL that fails to parse precisely
because something was appended to it. Swallowing both means "A genuine
sub-query still parses and is caught" does not hold whenever the sub-query sits
next to unparseable syntax.
I checked this on the branch with the real code, no mocks, going through
`TableColumn.get_sqla_col` with a MySQL backend:
- `(SELECT 1)` raises `SupersetSecurityException`. The gate works.
- `DATE_ADD(ds, 1)` falls back to the raw expression. Intended, and it is
your own example from the docstring.
- `DATE_ADD(ds, 1) + (SELECT 1)` also falls back, so the sub-query reaches
`literal_column` unvalidated.
- `1; DROP TABLE t` on sqlite falls back too, so the multi-statement rule
that `test_validate_stored_expression_rejects_multi_statement` locks in at save
time has no query-time counterpart.
To be clear about scope: master has no query-time gate at all, so this is
not a regression, it just means the hardening is narrower than the docstring
claims. It is reachable through the same door this PR is closing, since save
time skips validation when Jinja is present and the skeleton does not parse
(`models.py:898`), and the template processor strips the Jinja before we get
here.
One cheap way to narrow the gap is to re-check with the permissive dialect
purely as a detector before giving up, and log when we do give up so a skipped
gate is visible:
```python
try:
return validate_adhoc_subquery(expression, database, catalog, schema,
engine)
except SupersetParseError:
# A parse failure is ambiguous: benign dialect syntax sqlglot cannot
# handle, or SQL that is unparseable because something was appended to
# it. Re-check with the permissive dialect as a detector before falling
# back to the raw expression.
try:
validate_adhoc_subquery(expression, database, catalog, schema,
"base")
except SupersetParseError:
logger.warning(
"Skipping query-time validation of unparseable stored expression"
)
return expression
```
I confirmed this actually buys something: `SQLStatement("DATE_ADD(ds, 1) +
(SELECT 1)", "base")` parses and reports `has_subquery()` as True, while
`SQLStatement("DATE_ADD(ds, 1)", "base")` parses clean and reports False, so
the benign case you documented still falls through. It will not catch
deliberately malformed SQL, and that is fine. At minimum I think the docstring
should stop promising that a genuine sub-query is always caught. A test with
`DATE_ADD(ds, 1) + (SELECT 1)` on MySQL would lock in whichever behavior you
settle on.
--
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]
