gabotorresruiz commented on code in PR #42087:
URL: https://github.com/apache/superset/pull/42087#discussion_r3730861964
##########
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:
Not a blocker, more a consistency question: should this raise the way the
adhoc siblings do?
`validate_adhoc_subquery` raises `SupersetSecurityException`, and that
carries `status = 403` (`superset/exceptions.py:168`). The adhoc paths
deliberately do not let it escape: `adhoc_metric_to_sqla` at `models.py:1760`
and `adhoc_column_to_sqla` at `models.py:1855` both convert it with `raise
QueryObjectValidationError(ex.message) from ex`, which
`query_context_processor.py:142` catches, records as the query's error message,
and surfaces as a 400.
Here it propagates raw, and I traced that nothing in between converts it
(`ChartDataCommand.run` only maps `CacheLoadError`). So a dashboard with a
pre-existing calculated column containing a sub-query would get a 403 rather
than the chart-level "Custom SQL fields cannot contain sub-queries" message,
and the failure never gets recorded on the query result. It also makes those
failures look like access-control errors in logs and alerting, which sends
whoever debugs it to the wrong place.
Wrapping it inside the shared helper would cover all five sinks at once. Or
is there a reason to keep the security typing on this 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]
