rusackas commented on code in PR #42928:
URL: https://github.com/apache/superset/pull/42928#discussion_r3742516858
##########
superset/commands/sql_lab/estimate.py:
##########
@@ -150,6 +163,16 @@ def run(
sql = self._sql
if self._template_params:
+ # Check access before rendering the Jinja template (mirrors the
+ # SQL Lab execute path).
+ security_manager.raise_for_access(
+ database=self._database,
+ sql=sql,
+ catalog=self._catalog,
+ schema=self._schema or None,
+ template_params=self._template_params,
+ force_dataset_match=True,
+ )
Review Comment:
Good catch. validate() already runs this same check with the SQL and strict
scoping before run() does any rendering, so the second call was redundant.
Removed it.
##########
superset/db_engine_specs/postgres.py:
##########
@@ -641,12 +641,11 @@ def get_default_schema_for_query(
"""
Return the default schema for a given query.
- This method simply uses the parent method after checking that there
are no
- malicious path setting in the query.
+ This method simply uses the parent method after checking that the query
+ cannot rebind the schema used to resolve unqualified table names.
"""
script = process_jinja_sql(query.sql, database, template_params).script
Review Comment:
This render is pre-existing on the schema-resolution path, and the security
manager parses the query for authorization regardless, so this isn't new
behavior. Consolidating the renders would be a nice cleanup but it's out of
scope here and doesn't change the result.
##########
superset/sql/parse.py:
##########
@@ -2074,48 +2254,88 @@ def process_jinja_sql(
:returns: JinjaSQLResult containing the processed script and table
references
:raises SupersetSecurityException: If SQLGlot is unable to parse the SQL
statement
:raises jinja2.exceptions.TemplateError: If the Jinjafied SQL could not be
rendered
+ :raises SupersetParseError: If a partition macro references a table that
+ cannot be determined statically
"""
from superset.jinja_context import ( # pylint:
disable=import-outside-toplevel
get_template_processor,
+ NoOpTemplateProcessor,
)
processor = get_template_processor(database)
ast = processor.env.parse(sql)
tables = set()
+ def raise_for_unresolvable_macro() -> Any:
+ raise SupersetParseError(
+ sql,
+ database.db_engine_spec.engine,
+ message=(
+ "Unable to determine the table referenced by a partition "
+ "macro; use a single constant table reference"
+ ),
+ )
+
for node in ast.find_all(nodes.Call):
- if isinstance(node.node, nodes.Getattr) and node.node.attr in (
- "latest_partition",
- "latest_sub_partition",
+ if (
+ isinstance(node.node, nodes.Getattr)
+ and node.node.attr in PARTITION_MACRO_NAMES
):
- # Try to extract the table referenced in the macro.
+ # Extract the table referenced in the macro. The reference must
+ # be statically evaluable; otherwise raise rather than render.
try:
+ if len(node.args) != 1:
+ raise nodes.Impossible()
tables.add(
Table(
*[
remove_quotes(part.strip())
for part in
node.args[0].as_const().split(".")[::-1]
- if len(node.args) == 1
]
)
)
except nodes.Impossible:
- pass
+ raise_for_unresolvable_macro()
# Replace the potentially problematic Jinja macro with some benign
SQL.
node.__class__ = nodes.TemplateData
node.fields = nodes.TemplateData.fields
node.data = "NULL"
- # re-render template back into a string
- code = processor.env.compile(ast)
- template = Template.from_code(processor.env, code,
globals=processor.env.globals)
- rendered_sql = template.render(processor.get_context(), **(template_params
or {}))
+ # Render the neutralized template once, using the same context
+ # ``process_template`` builds at execution time, so the validated SQL
+ # matches the executed SQL. A no-op processor runs the raw SQL at
+ # execution time, so validate that raw SQL directly.
+ if isinstance(processor, NoOpTemplateProcessor):
+ rendered_sql = processor.process_template(sql)
+ else:
+ code = processor.env.compile(ast)
+ template = Template.from_code(
+ processor.env,
+ code,
+ globals=processor.env.globals,
+ )
+ # Replace live partition macros with stubs so a call that survives
+ # neutralization (e.g. via a dynamic attribute lookup) does not
+ # execute during this render.
+ context = processor.get_template_context(**(template_params or {}))
+ if (engine := getattr(processor, "engine", None)) and isinstance(
+ context.get(engine), dict
+ ):
+ context[engine] = {
+ key: (
+ (lambda *args, **kwargs: raise_for_unresolvable_macro())
+ if key in PARTITION_MACRO_NAMES
+ else value
+ )
+ for key, value in context[engine].items()
+ }
+ rendered_sql = template.render(context)
Review Comment:
The render on this path could already surface a raw Jinja error on the prior
version of this code, so it isn't a new behavior, and the run() path still
wraps it. I'd rather tighten the typing in a follow-up than widen this change.
--
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]
