sadpandajoe commented on code in PR #42785:
URL: https://github.com/apache/superset/pull/42785#discussion_r4128846215
##########
superset/commands/sql_lab/estimate.py:
##########
@@ -161,27 +162,51 @@ def run(
) -> list[dict[str, Any]]:
self.validate()
- sql = self._sql
- if self._template_params:
- # Access is already checked in validate() before any rendering.
- template_processor = get_template_processor(self._database)
- try:
- sql = template_processor.process_template(sql,
**self._template_params)
- except TemplateError as ex:
- raise SupersetErrorException(
- SupersetError(
- message=str(ex),
- error_type=SupersetErrorType.GENERIC_COMMAND_ERROR,
- level=ErrorLevel.ERROR,
- ),
- status=400,
- ) from ex
+ # Access is already checked in validate() before any rendering.
+ #
+ # Rendered whether or not `template_params` was supplied, the way
+ # `validate()` above already jinja-processes for authorization and the
+ # execution path does in `SqlQueryRenderImpl.render`. A query needs no
+ # declared parameter to need rendering -- `get_time_filter()`,
+ # `current_username()`, `url_param()` take none -- and SQL Lab posts an
+ # empty `template_params` for an estimate, so those never rendered.
+ template_processor = get_template_processor(self._database)
+ try:
+ sql = template_processor.process_template(
Review Comment:
This still authorizes a different table set than what gets estimated.
`raise_for_access`'s internal `process_jinja_sql` (`superset/sql/parse.py`)
calls `template.render(context)` unconditionally, whether or not
`template_params` is passed and whether or not `executed_sql` is pinned — so
pinning `executed_sql` and passing `sql=` are equally exposed, not equally
safe. A construct that survives into the first-rendered text (for example,
inside a SQL comment) can be interpreted as live Jinja on this second pass and
dropped from what gets authorized, while a SQL comment only hides it from the
SQL parser, not from the estimate. Could authorization extract tables from the
literal already-rendered text without routing it through another Jinja render?
--
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]
