codeant-ai-for-open-source[bot] commented on code in PR #41120:
URL: https://github.com/apache/superset/pull/41120#discussion_r3507283243
##########
superset/commands/sql_lab/estimate.py:
##########
@@ -102,57 +101,43 @@ def _apply_sql_security(self, sql: str) -> str:
db_engine_spec.engine,
set(),
)
- if disallowed_tables and
parsed_script.check_tables_present(disallowed_tables):
- found_tables = set()
- for statement in parsed_script.statements:
- present = {table.table.lower() for table in statement.tables}
- for table in disallowed_tables:
- if table.lower() in present:
- found_tables.add(table)
- raise SupersetDisallowedSQLTableException(found_tables or
disallowed_tables)
+ rls_enabled = is_feature_enabled("RLS_IN_SQLLAB")
+
+ # Resolve the effective per-query schema once, the same way the
execution
+ # path does (``sql_lab.execute_sql_statements``), but only when a
control
+ # below actually needs it. Going through
``get_default_schema_for_query``
+ # rather than the static ``get_default_schema`` runs engine-specific
+ # per-query security gates too — e.g. ``PostgresEngineSpec`` rejects a
+ # query that sets ``search_path`` — and resolves unqualified
references to
+ # the schema the engine uses at runtime, so both the denylist check and
+ # RLS injection match the execution path exactly.
+ catalog: str | None = None
+ effective_schema = ""
+ if disallowed_tables or rls_enabled:
+ catalog = self._catalog or self._database.get_default_catalog()
+ resolved_schema = self._database.resolve_query_default_schema(
+ self._sql, self._schema, catalog, self._template_params
+ )
Review Comment:
**Suggestion:** This path resolves the effective schema from `self._sql`
even though `_apply_sql_security` is validating the `sql` argument. When the
argument has already been rendered/transformed, the schema/security gate may
inspect different SQL than the denylist parser, producing inconsistent
allow/deny behavior. Resolve the default schema from the same `sql` value being
checked in this method. [security]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ SQL Lab cost estimate denylists may misfire under templating.
- ⚠️ RLS predicates may target wrong schema during estimation.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Trigger SQL Lab cost estimation, which calls QueryEstimationCommand.run()
in
superset/commands/sql_lab/estimate.py:76-100 (see BulkRead output where
run() starts
around file line 145).
2. In run(), observe that `sql` is first set from `self._sql` and then, when
`self._template_params` is present, rendered via
`template_processor.process_template(sql,
**self._template_params)` before being passed to
`self._apply_sql_security(sql)`
(estimate.py:81-90).
3. Inside `_apply_sql_security` (superset/commands/sql_lab/estimate.py:80-74
relative to
file, shown at lines 11-74 in BulkRead), note that the denylist and RLS
checks parse the
*rendered* SQL argument via `parsed_script = SQLScript(sql,
engine=db_engine_spec.engine)`
(line ~20 in the excerpt).
4. In the same method, the effective schema used for denylist matching and
RLS injection
is resolved via `resolved_schema =
self._database.resolve_query_default_schema(self._sql,
self._schema, catalog, self._template_params)` (estimate.py:118-120), which
builds a probe
Query using the original `self._sql` and re-renders templates inside
`get_default_schema_for_query` (superset/models/core.py:701-707). Because
`_apply_sql_security` validates the already-rendered `sql` argument while
schema
resolution runs against a separate render of `self._sql`, any
non-deterministic or
context-dependent templating (e.g. macros that vary per render) can make the
schema/security gate inspect different SQL than the denylist parser, causing
inconsistent
allow/deny behavior for table denylists and RLS in the cost-estimation path.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=407d8cb52af04bf88970ec2f676bff42&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=407d8cb52af04bf88970ec2f676bff42&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:** 118:120
**Comment:**
*Security: This path resolves the effective schema from `self._sql`
even though `_apply_sql_security` is validating the `sql` argument. When the
argument has already been rendered/transformed, the schema/security gate may
inspect different SQL than the denylist parser, producing inconsistent
allow/deny behavior. Resolve the default schema from the same `sql` value being
checked in this method.
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=b0b3431cacfee81b615954af53c11cc5c59594def5943bff75aa39f583dcd298&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41120&comment_hash=b0b3431cacfee81b615954af53c11cc5c59594def5943bff75aa39f583dcd298&reaction=dislike'>👎</a>
##########
superset/sql/parse.py:
##########
@@ -1060,15 +1057,135 @@ def check_functions_present(self, functions: set[str])
-> bool:
return any(function.upper() in present for function in functions)
- def check_tables_present(self, tables: set[str]) -> bool:
+ def check_tables_present(
+ self, tables: set[str], default_schema: str | None = None
+ ) -> bool:
"""
Check if any of the given tables are present in the statement.
+ Denylist entries may be bare (``pg_stat_activity``) or
+ schema-qualified (``information_schema.tables``). Bare entries
+ match by table name regardless of schema; qualified entries
+ require the schema to match too. This lets us block all access
+ to ``information_schema`` without also blocking any
+ user-authored table that happens to be named ``tables``.
+
:param tables: Set of table names to check for (case-insensitive)
- :return: True if any of the tables are present
+ :param default_schema: Schema unqualified references resolve to at
+ runtime (e.g. the session ``search_path`` / selected schema)
+ :return: True if any of the given tables is referenced
+ """
+ return bool(self.get_disallowed_tables(tables, default_schema))
+
+ def changes_search_path(self) -> bool:
+ """
+ Return True if the statement changes the session ``search_path``.
+
+ A ``SET search_path = ...`` makes unqualified references in later
+ statements resolve to a schema other than the caller's
+ ``default_schema``, so denylist matching against ``default_schema``
+ alone becomes unreliable once such a statement is present.
+ """
+ # `SET search_path = schema` (and the `TO`/`SESSION`/`LOCAL` variants)
+ # parse as a structured exp.Set, surfaced by get_settings(). Strip any
+ # identifier quoting so `SET "search_path" = ...` (equivalent to the
+ # unquoted form in Postgres) is still recognized.
+ if any(key.strip('"').lower() == "search_path" for key in
self.get_settings()):
+ return True
+ # `set_config('search_path', ...)` rebinds the search path through a
+ # function call rather than a SET statement, so it never reaches
+ # get_settings() and must be detected on the parsed tree.
+ for func in self._parsed.find_all(exp.Anonymous):
+ if (
+ func.name.lower() == "set_config"
+ and func.expressions
+ and isinstance(func.expressions[0], exp.Literal)
+ and func.expressions[0].name.lower() == "search_path"
+ ):
+ return True
+ # Exotic forms (e.g. `SET search_path TO "$user", public`) fall back to
+ # an opaque exp.Command. Match the leading setting name rather than
+ # scanning the whole expression, so `SET ROLE my_search_path_role`
+ # (whose value merely contains the substring) is not misclassified.
+ parsed = self._parsed
+ if isinstance(parsed, exp.Command) and parsed.name.upper() == "SET":
+ tokens = str(parsed.expression).replace("=", " ").split()
+ while tokens and tokens[0].upper() in {"SESSION", "LOCAL"}:
+ tokens.pop(0)
+ return bool(tokens) and tokens[0].strip('"').lower() ==
"search_path"
+ return False
Review Comment:
**Suggestion:** The search-path change detector only handles
`SET`/`set_config` forms and misses `RESET search_path`, which also changes how
later unqualified tables resolve. In multi-statement scripts this can leave
`schema_indeterminate` false and allow a schema-qualified denylist entry to be
bypassed after a reset. Extend `changes_search_path()` to treat `RESET
search_path` as a search-path mutation. [security]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ RESET search_path lets queries evade information_schema table denylists.
- ⚠️ Multi-statement SQLLab queries may bypass schema-qualified protections.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure DISALLOWED_SQL_TABLES for a PostgreSQL engine to include a
schema-qualified
entry such as `information_schema.tables`, as used by SQL Lab and the
executor paths (see
usage in superset/sql_lab.py:452-456 and
superset/sql/execution/executor.py:736-758 where
`parsed_script.get_disallowed_tables(...)` is called).
2. Submit a multi-statement script against that database via SQL Lab or
Database.execute(), for example `RESET search_path; SELECT * FROM tables;`,
where
PostgreSQL resolves unqualified `tables` to `information_schema.tables`
after the `RESET
search_path` statement based on the server’s default search_path.
3. Observe that the script is parsed into a SQLScript and checked for
denylists using
`SQLScript.get_disallowed_tables(disallowed_tables, default_schema)`
(superset/sql/parse.py:1833-1859). This method iterates statements, passing a
`schema_indeterminate` flag that is flipped when
`statement.changes_search_path()` returns
True, so later statements will conservatively match unqualified references
against
schema-qualified denylist entries.
4. Inspect `SQLStatement.changes_search_path()`
(superset/sql/parse.py:81-117). It returns
True for explicit `SET search_path` forms (via `get_settings()` and exp.Set)
and for
`set_config('search_path', ...)`, and finally for opaque `SET ...` commands
by checking
`parsed = self._parsed` and `isinstance(parsed, exp.Command) and
parsed.name.upper() ==
"SET"` with a tokenized setting name. There is no handling for `RESET
search_path`, which
SQLGlot parses as `exp.Command` named `"RESET"`, so `changes_search_path()`
returns False
for that statement. As a result, `schema_indeterminate` never becomes True,
and the
following `SELECT * FROM tables` is matched in
`SQLStatement.get_disallowed_tables()` only
against `default_schema.tables`, not against the schema-qualified
`information_schema.tables` denylist entry. The script therefore passes
denylist checks
while at runtime the unqualified `tables` actually resolves to the blocked
`information_schema.tables`, allowing `RESET search_path` to bypass
schema-qualified table
denylists in multi-statement scripts.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=6e1340d7a05f47c2aba51576603ed9e9&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=6e1340d7a05f47c2aba51576603ed9e9&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:** 1110:1116
**Comment:**
*Security: The search-path change detector only handles
`SET`/`set_config` forms and misses `RESET search_path`, which also changes how
later unqualified tables resolve. In multi-statement scripts this can leave
`schema_indeterminate` false and allow a schema-qualified denylist entry to be
bypassed after a reset. Extend `changes_search_path()` to treat `RESET
search_path` as a search-path mutation.
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=da543ff774f66013a34bba7bcda6b7db245818fc4e4c4309aef44306fa8454a8&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41120&comment_hash=da543ff774f66013a34bba7bcda6b7db245818fc4e4c4309aef44306fa8454a8&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]
