codeant-ai-for-open-source[bot] commented on code in PR #42928:
URL: https://github.com/apache/superset/pull/42928#discussion_r3742439556
##########
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:
**Suggestion:** `validate()` already invokes `raise_for_access`, but `run()`
invokes it a second time whenever template parameters exist. This repeats
SQL/Jinja parsing and authorization hooks, and because template rendering can
execute macros or observe changing state, the second authorization pass can
produce different results or reject a request that passed the first pass.
Perform the access check once and reuse the validated/rendered result.
[performance]
<details>
<summary><b>Severity Level:</b> Minor ๐งน</summary>
```mdx
- โ ๏ธ Templated cost estimates perform authorization twice.
- โ ๏ธ Database-backed Jinja macros may execute repeatedly.
- โ ๏ธ SQL Lab estimation incurs unnecessary parsing overhead.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=2c8306869d1d49d5af574a06c15ecf0e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=2c8306869d1d49d5af574a06c15ecf0e&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/commands/sql_lab/estimate.py
**Line:** 165:175
**Comment:**
*Performance: `validate()` already invokes `raise_for_access`, but
`run()` invokes it a second time whenever template parameters exist. This
repeats SQL/Jinja parsing and authorization hooks, and because template
rendering can execute macros or observe changing state, the second
authorization pass can produce different results or reject a request that
passed the first pass. Perform the access check once and reuse the
validated/rendered result.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42928&comment_hash=e43c87b360bd92721c3eedd5411d4fe0d3aac7afd03a0ae34cf96cf87a130701&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42928&comment_hash=e43c87b360bd92721c3eedd5411d4fe0d3aac7afd03a0ae34cf96cf87a130701&reaction=dislike'>๐</a>
##########
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:
**Suggestion:** This renders and parses the Jinja query during PostgreSQL
schema resolution, but the security-manager flow then calls `process_jinja_sql`
again for table authorization and execution renders the template once more.
Database-backed macros such as partition helpers therefore run multiple times
during one request, potentially repeating database queries and producing
different validation and execution SQL for non-idempotent macros. Reuse a
single rendered result or avoid rendering again during schema resolution.
[performance]
<details>
<summary><b>Severity Level:</b> Major โ ๏ธ</summary>
```mdx
- โ ๏ธ SQL Lab Jinja macros may execute multiple times per query.
- โ ๏ธ Repeated macro calls can produce inconsistent validation and execution
SQL.
- โ ๏ธ Additional database-backed macro queries increase request latency.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=7dc825a8d1134bd9b4b0eecd1fc83dd0&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=7dc825a8d1134bd9b4b0eecd1fc83dd0&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/db_engine_specs/postgres.py
**Line:** 647:647
**Comment:**
*Performance: This renders and parses the Jinja query during PostgreSQL
schema resolution, but the security-manager flow then calls `process_jinja_sql`
again for table authorization and execution renders the template once more.
Database-backed macros such as partition helpers therefore run multiple times
during one request, potentially repeating database queries and producing
different validation and execution SQL for non-idempotent macros. Reuse a
single rendered result or avoid rendering again during schema resolution.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42928&comment_hash=aad94fdf8601adc6183451064697c7c12758b0a80902ab3ae43266363bed55af&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42928&comment_hash=aad94fdf8601adc6183451064697c7c12758b0a80902ab3ae43266363bed55af&reaction=dislike'>๐</a>
##########
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:
**Suggestion:** The validation path now renders the template directly, but
unlike `BaseTemplateProcessor.process_template`, this call does not translate
Jinja rendering failures into Superset's typed template exceptions. Because
`raise_for_access` invokes `process_jinja_sql` before the later `run()`
try/except, an undefined variable or other template error can escape as a raw
Jinja exception instead of the expected `SupersetErrorException`. Use the
processor's error-handling path or wrap this render consistently. [error
handling]
<details>
<summary><b>Severity Level:</b> Major โ ๏ธ</summary>
```mdx
- โ SQL validation can return raw template-rendering failures.
- โ ๏ธ Clients lose the expected structured validator error response.
- โ ๏ธ Authorization and validation endpoints handle identical templates
inconsistently.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=bfff5b10405f46219c8f6fcc6ab9f029&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=bfff5b10405f46219c8f6fcc6ab9f029&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/sql/parse.py
**Line:** 2335:2335
**Comment:**
*Error Handling: The validation path now renders the template directly,
but unlike `BaseTemplateProcessor.process_template`, this call does not
translate Jinja rendering failures into Superset's typed template exceptions.
Because `raise_for_access` invokes `process_jinja_sql` before the later `run()`
try/except, an undefined variable or other template error can escape as a raw
Jinja exception instead of the expected `SupersetErrorException`. Use the
processor's error-handling path or wrap this render consistently.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42928&comment_hash=7ca541c54a3572891b8e54e1f6c256c1625e1d62fdb66d386f1225e4e7ba8b0b&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42928&comment_hash=7ca541c54a3572891b8e54e1f6c256c1625e1d62fdb66d386f1225e4e7ba8b0b&reaction=dislike'>๐</a>
--
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]
