rusackas commented on code in PR #43394:
URL: https://github.com/apache/superset/pull/43394#discussion_r3832184274
##########
superset/jinja_context.py:
##########
@@ -118,6 +118,48 @@ class TimeFilter:
time_range: str | None
+class SQLSafeList(list[Any]): # noqa: FURB189
+ """
+ A list of dialect-escaped values whose *whole-container* string
+ rendering cannot re-introduce raw quote characters.
+
+ Rendering a plain Python list in a Jinja template goes through
+ ``str()``/``repr()``, which wraps every string element in fresh quote
+ delimiters (and switches to double-quote delimiters when the element
+ contains a single quote, emitting that single quote raw). Either way
+ the rendered text can contain quote characters that were never
+ escaped for SQL, so a template interpolating the list inside its own
+ quotes -- e.g. ``LIKE '{{ filter.get('escaped_val') }}'`` -- could be
+ broken out of even though every string leaf was individually escaped.
+ This subclass renders as its (already-escaped) elements joined with
+ ``", "``, with no additional delimiters, so every quote in the output
+ is one the dialect's literal processor already escaped.
+ """
+
+ def __str__(self) -> str:
+ return ", ".join(str(element) for element in self)
+
+ __repr__ = __str__
+
+
+class SQLSafeDict(dict[Any, Any]): # noqa: FURB189
+ """
+ A dict of dialect-escaped values whose *whole-container* string
+ rendering cannot re-introduce raw quote characters. Mirrors
+ :class:`SQLSafeList` for the mapping case.
+
+ Keys are rendered as-is: they are used for member lookups (for
+ example ``{{ get_guest_user_attribute('tenant').id }}``), not
+ interpolated into SQL, and are never passed through
+ ``ExtraCache._escape_value``.
+ """
+
+ def __str__(self) -> str:
Review Comment:
Good catch, keys are now escaped the same way values are in 4dce236.
Whole-dict rendering now carries the same guarantee as SQLSafeList already did
for values.
##########
superset/commands/sql_lab/execute.py:
##########
@@ -151,6 +151,15 @@ def _run_sql_json_exec_from_scratch(self) ->
SqlJsonExecutionStatus:
self._validate_access(query,
self._execution_context.template_params)
self._execution_context.set_query(query)
rendered_query =
self._sql_query_render.render(self._execution_context)
+ # The check above authorizes a render of query.sql +
template_params
+ # performed before rendering, so that macros with side effects are
+ # gated before they run. self._sql_query_render.render() above is
an
+ # independent second render of the same source; for a
+ # nondeterministic template (e.g. one using Jinja's `random` filter
+ # to pick a table) the two renders can diverge, letting a query
+ # read a table the first check never saw. Re-validate the literal
+ # rendered text that is about to execute.
+ self._validate_rendered_access(query, rendered_query)
Review Comment:
This one's out of scope: SQL_QUERY_MUTATOR is an operator-configured hook
(set in superset_config.py), not something an unprivileged caller can
influence. If an operator writes a mutator that rewrites table references,
that's their own code running with their own trust, same as any other custom
mutator behavior. The re-validation here targets the actual gap: Jinja template
rendering itself diverging between the pre-check and the executed SQL.
--
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]
