codeant-ai-for-open-source[bot] commented on code in PR #41120:
URL: https://github.com/apache/superset/pull/41120#discussion_r3507283253
##########
superset/sql/execution/executor.py:
##########
@@ -436,9 +436,21 @@ def _prepare_sql(
rendered_sql, self.database.db_engine_spec.engine
)
- # 4. Get catalog and schema
+ # 4. Get catalog and the effective per-query schema. Resolve the schema
+ # through the query-aware ``get_default_schema_for_query`` rather than
the
+ # static ``get_default_schema``, the same way
``execute_sql_statements``
+ # and the estimate path do: it resolves an unqualified reference to the
+ # schema the engine actually uses at runtime (engines without
+ # dynamic-schema support ignore the request's selected schema) and runs
+ # engine-specific per-query security gates (e.g. ``PostgresEngineSpec``
+ # rejects a query that sets ``search_path``), so the denylist check and
+ # RLS injection match execution instead of a schema that may never
apply.
catalog = opts.catalog or self.database.get_default_catalog()
- schema = opts.schema or self.database.get_default_schema(catalog)
+ # Resolve unconditionally, even when an explicit schema is supplied, so
+ # the engine's per-query security gate always runs (parity with the
+ # estimate path); the explicit schema still wins as the effective
target.
+ resolved_schema = self._resolve_query_schema(sql, opts, catalog)
+ schema = opts.schema or resolved_schema
Review Comment:
**Suggestion:** The schema resolver is invoked with the pre-render SQL
instead of the already rendered SQL that is actually parsed/executed. If
templating is non-deterministic or context-dependent, the schema gate can
evaluate a different statement than the one being checked for denylisted
tables, causing security checks to run against the wrong schema context.
Resolve the schema from the same rendered SQL string used to build the scripts
so security checks and execution stay consistent. [security]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Core SQL execution security may use mismatched schema context.
- ⚠️ Table denylists could be bypassed with non-deterministic templates.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Execute a query through the core Database API, which uses SQLExecutor
(superset/sql/execution/executor.py:188-221) from
superset/models/core.py:1438 to run
`Database.execute()` or `execute_async()`.
2. In SQLExecutor.execute() (executor.py:221-241), observe that
`_prepare_sql(sql, opts)`
is called to render templates and build SQLScript objects, and the returned
`schema` is
later passed into `self._check_security(transformed_script, schema)`
(executor.py:233-235).
3. Inside `_prepare_sql` (executor.py:426-443, BulkRead lines 9-43), note
that
`rendered_sql = self._render_sql_template(sql, opts.template_params)`
produces the SQL
text actually parsed and executed, and both `original_script` and
`transformed_script` are
constructed from this `rendered_sql` via `SQLScript(rendered_sql, ...)`.
4. Still in `_prepare_sql`, see that `resolved_schema =
self._resolve_query_schema(sql,
opts, catalog)` (executor.py:452) is invoked with the *original* pre-render
SQL, not
`rendered_sql`. `_resolve_query_schema` delegates to
`self.database.resolve_query_default_schema(sql, opts.schema, catalog,
opts.template_params)` (executor.py:732-734), which builds a probe Query and
calls
`get_default_schema_for_query` (superset/models/core.py:701-707),
re-rendering templates
to determine the runtime default schema. Because security checks in
`_check_security`
(executor.py:45-83) use `transformed_script` built from `rendered_sql` but
the schema
context comes from a separate render of the original SQL, any
non-deterministic or
context-dependent templating can make the denylisted table/function checks
and DML guard
run against a schema derived from different SQL than the one actually being
executed,
desynchronizing core execution security from the real query text.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=4b220bd99fa34ffe8b9267ae267d9a9f&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=4b220bd99fa34ffe8b9267ae267d9a9f&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/execution/executor.py
**Line:** 452:453
**Comment:**
*Security: The schema resolver is invoked with the pre-render SQL
instead of the already rendered SQL that is actually parsed/executed. If
templating is non-deterministic or context-dependent, the schema gate can
evaluate a different statement than the one being checked for denylisted
tables, causing security checks to run against the wrong schema context.
Resolve the schema from the same rendered SQL string used to build the scripts
so security checks and execution stay consistent.
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%2F41120&comment_hash=eec6d3e6f7fd0e3f04a11625177aa1f5c82ce4ea381f18b6061b3350b0d3eb4c&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41120&comment_hash=eec6d3e6f7fd0e3f04a11625177aa1f5c82ce4ea381f18b6061b3350b0d3eb4c&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]
