codeant-ai-for-open-source[bot] commented on code in PR #41120:
URL: https://github.com/apache/superset/pull/41120#discussion_r3507274208
##########
superset/models/helpers.py:
##########
@@ -1187,6 +1187,31 @@ def get_sqla_row_level_filters(
# for datasources of type query
return []
+ def _resolve_denylist_schema(self, sql: str) -> Optional[str]:
+ """
+ Resolve the effective default schema for ``DISALLOWED_SQL_TABLES``
+ checks, memoized per datasource instance.
+
+ An explicit datasource schema always wins. Otherwise the schema is
+ resolved through the query-aware ``get_default_schema_for_query`` (not
+ the static ``get_default_schema``) so the value matches what the engine
+ uses at runtime -- honoring dynamic-schema engines and URI/connect-arg
+ schema rules -- exactly like the SQL Lab / executor denylist gate.
+
+ The result is cached on the instance so adhoc-expression validation,
+ which runs once per selected column/metric/order-by, resolves the
schema
+ at most once instead of issuing a fallback inspector round-trip per
+ expression.
+ """
+ if self.schema:
+ return self.schema
Review Comment:
**Suggestion:** The early return for an explicit schema bypasses
`resolve_query_default_schema`, so engine-level per-query schema security
checks (like rejecting `SET search_path` patterns) are never executed on this
path. Keep the explicit schema as the final effective schema, but still invoke
the query-aware resolver so security-gate side effects run consistently with
SQL Lab/executor behavior. [security]
<details>
<summary><b>Severity Level:</b> Critical π¨</summary>
```mdx
- β Datasource queries bypass Postgres search_path security gate.
- β οΈ Denylist behavior diverges between SQL Lab and datasource.
```
</details>
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. Configure a PostgreSQL Database in Superset (engine `postgresql`) and set
a
DISALLOWED_SQL_TABLES entry for that engine in `app.config`, e.g.
`"DISALLOWED_SQL_TABLES": {"postgresql": {"information_schema.tables"}}`, as
read by
`_raise_for_disallowed_sql` in `superset/models/helpers.py:1497-1498`.
2. Create a SQLA dataset (SqlaTable) bound to this Database with an explicit
`schema` (for
example `"analytics"`); SqlaTable inherits the helpers mixin so its
`query()` method at
`superset/models/helpers.py:1491-1539` will call
`self._raise_for_disallowed_sql(sql)`
before executing.
3. Execute a chart on this dataset whose underlying SQL contains a `SET
search_path =
information_schema; SELECT * FROM tables` pattern (or any query that would
trigger the
Postgres search_path gate); this SQL reaches
`_raise_for_disallowed_sql(sql)` at
`superset/models/helpers.py:1491-1501`, which then calls `effective_schema =
self._resolve_denylist_schema(sql)` at line 1516.
4. Because the dataset has an explicit schema, `_resolve_denylist_schema` at
`superset/models/helpers.py:1190-1213` immediately returns `self.schema` via
the `if
self.schema: return self.schema` branch (lines 1206-1207) and never invokes
`self.database.resolve_query_default_schema`. As a result,
`PostgresEngineSpec.get_default_schema_for_query` at
`superset/db_engine_specs/postgres.py:605-630` (which parses the query SQL
and raises
`SupersetSecurityException` if `"search_path"` is present) is not executed
on this
datasource path, even though the same gate does run unconditionally for SQL
Lab
execution/estimate via `executor._resolve_query_schema`
(`superset/sql/execution/executor.py:440-452`) and
`commands/sql_lab/estimate.py:106-113`.
The query therefore bypasses the per-query search_path security gate in the
datasource
flow while being rejected in the SQL Lab/estimate flow.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=0eb93242da834c44a776914482f03c99&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=0eb93242da834c44a776914482f03c99&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/models/helpers.py
**Line:** 1206:1207
**Comment:**
*Security: The early return for an explicit schema bypasses
`resolve_query_default_schema`, so engine-level per-query schema security
checks (like rejecting `SET search_path` patterns) are never executed on this
path. Keep the explicit schema as the final effective schema, but still invoke
the query-aware resolver so security-gate side effects run consistently with
SQL Lab/executor behavior.
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=a8b3ebae3e83005645b80fd106500a0d4a7b575afc02b132df0889bafb80b701&reaction=like'>π</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41120&comment_hash=a8b3ebae3e83005645b80fd106500a0d4a7b575afc02b132df0889bafb80b701&reaction=dislike'>π</a>
##########
superset/models/helpers.py:
##########
@@ -1187,6 +1187,31 @@ def get_sqla_row_level_filters(
# for datasources of type query
return []
+ def _resolve_denylist_schema(self, sql: str) -> Optional[str]:
+ """
+ Resolve the effective default schema for ``DISALLOWED_SQL_TABLES``
+ checks, memoized per datasource instance.
+
+ An explicit datasource schema always wins. Otherwise the schema is
+ resolved through the query-aware ``get_default_schema_for_query`` (not
+ the static ``get_default_schema``) so the value matches what the engine
+ uses at runtime -- honoring dynamic-schema engines and URI/connect-arg
+ schema rules -- exactly like the SQL Lab / executor denylist gate.
+
+ The result is cached on the instance so adhoc-expression validation,
+ which runs once per selected column/metric/order-by, resolves the
schema
+ at most once instead of issuing a fallback inspector round-trip per
+ expression.
+ """
+ if self.schema:
+ return self.schema
+ if not hasattr(self, "_denylist_default_schema"):
+ catalog = self.catalog or self.database.get_default_catalog()
+ self._denylist_default_schema =
self.database.resolve_query_default_schema(
+ sql, None, catalog
+ )
+ return self._denylist_default_schema
Review Comment:
**Suggestion:** This cache is stored once per datasource instance without
keying by SQL/catalog, but schema resolution is query-dependent; after one
call, later checks can reuse a stale schema for different SQL and skip a fresh
resolver run. This can produce incorrect denylist matching and miss
query-specific schema-gate exceptions, so cache by the relevant inputs (at
minimum SQL and catalog) or avoid caching in this security-sensitive path.
[cache]
<details>
<summary><b>Severity Level:</b> Critical π¨</summary>
```mdx
- β Postgres search_path gate skipped for later datasource queries.
- β οΈ Denylist table matching can use stale runtime schema.
```
</details>
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. Use a PostgreSQL Database with engine `postgresql` and configured
DISALLOWED_SQL_TABLES
for that engine (read by `_process_sql_expression` and
`_raise_for_disallowed_sql` in
`superset/models/helpers.py:1269-1273` and `1497-1498`). Ensure this
Database is one where
per-query schema behavior matters (dynamic search_path / URI-based schema,
as described by
`Database.resolve_query_default_schema` in
`superset/models/core.py:701-734`).
2. Create a SqlaTable datasource against this Database with no explicit
`schema` (so
`self.schema` is `None`), matching the test setup in
`tests/unit_tests/models/helpers_test.py:21-23`. Add multiple adhoc fields
(columns/metrics/order-bys) so `_process_sql_expression` at
`superset/models/helpers.py:1215-1281` is called repeatedly: the first call
to
`_resolve_denylist_schema(sql_to_check)` will see no
`_denylist_default_schema` attribute,
compute `catalog = self.catalog or self.database.get_default_catalog()` and
invoke
`self.database.resolve_query_default_schema(sql, None, catalog)` (lines
1208-1212), which
builds a transient `Query` and calls
`PostgresEngineSpec.get_default_schema_for_query` at
`superset/db_engine_specs/postgres.py:605-630`, running the search_path
security gate and
resolving a schema. That schema is cached on the instance as
`_denylist_default_schema`.
3. On the same datasource instance, issue a different query whose SQL now
includes a
prohibited search_path change (e.g. `SET search_path = information_schema;
SELECT * FROM
tables`) as part of the datasourceβs base query or a saved query; when
`query()` runs at
`superset/models/helpers.py:1491-1539`, it calls
`_raise_for_disallowed_sql(sql)` (line
1534), which again computes `effective_schema =
self._resolve_denylist_schema(sql)` at
line 1516.
4. Because `_denylist_default_schema` is already set from the earlier
adhoc-expression
validation, `_resolve_denylist_schema` takes the cached branch at
`superset/models/helpers.py:1208-1213` and returns
`self._denylist_default_schema` without
invoking `self.database.resolve_query_default_schema` for this new SQL.
Consequently,
`PostgresEngineSpec.get_default_schema_for_query` (which parses the current
query SQL and
raises `SupersetSecurityException` if `"search_path"` is present) is not run
for the
executed query: the per-query gate and schema resolution are based solely on
the first SQL
string that populated the cache (often a simple `SELECT <expression>`), not
on the actual
query being validated/executed. This allows later queries on the same
datasource instance
that change `search_path` or otherwise depend on per-query schema semantics
to bypass the
engine-level security gate and can also cause denylist table matching to use
a stale
runtime schema instead of the current one.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=02442e14f0514d5ea8d0ef26909e7c9a&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=02442e14f0514d5ea8d0ef26909e7c9a&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/models/helpers.py
**Line:** 1208:1213
**Comment:**
*Cache: This cache is stored once per datasource instance without
keying by SQL/catalog, but schema resolution is query-dependent; after one
call, later checks can reuse a stale schema for different SQL and skip a fresh
resolver run. This can produce incorrect denylist matching and miss
query-specific schema-gate exceptions, so cache by the relevant inputs (at
minimum SQL and catalog) or avoid caching in this security-sensitive path.
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=95a51e5bc02640cea37b5bada9484a07ae26d56af65467f68929ac353d2e5862&reaction=like'>π</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41120&comment_hash=95a51e5bc02640cea37b5bada9484a07ae26d56af65467f68929ac353d2e5862&reaction=dislike'>π</a>
##########
superset/config.py:
##########
@@ -2048,6 +2059,30 @@ def engine_context_manager( # pylint:
disable=unused-argument
"pg_stat_replication",
"pg_stat_wal_receiver",
"pg_user",
+ # The SQL-standard `information_schema` views expose table /
+ # column / privilege / view-definition metadata across the entire
+ # database role the connection user can see. Entries are
+ # schema-qualified so `check_tables_present` only matches when the
+ # reference resolves to `information_schema.<view>` -- either written
+ # explicitly or as an unqualified name under an `information_schema`
+ # search_path -- not any user table that happens to share a name.
+ "information_schema.tables",
+ "information_schema.columns",
+ "information_schema.schemata",
+ "information_schema.views",
Review Comment:
**Suggestion:** Adding very common object names as schema-qualified denylist
entries (for example `information_schema.tables` / `information_schema.columns`
/ `information_schema.views`) causes false positives when a query script
changes `search_path`: the parser intentionally enters an indeterminate mode
and matches unqualified names against the bare part of qualified denylist
entries, so legitimate user tables named `tables`/`columns`/`views` can be
blocked even outside `information_schema`. Either avoid shipping these generic
entries by default, or tighten the matcher to only widen when the runtime
`search_path` can actually resolve to `information_schema`. [logic error]
<details>
<summary><b>Severity Level:</b> Major β οΈ</summary>
```mdx
- β SQL Lab blocks queries on user tables named tables.
- β οΈ Postgres denylist misclassifies benign tables after search-path change.
- β οΈ Behavior contradicts comments promising no overblocking of user tables.
```
</details>
<details>
<summary><b>Steps of Reproduction β
</b></summary>
```mdx
1. Configure a PostgreSQL database in Superset and ensure
`DISALLOWED_SQL_TABLES["postgresql"]` includes `information_schema.tables`
(and related
views) as defined in `superset/config.py:2035-2114`, particularly lines
2069-2072 where
`"information_schema.tables"`, `"information_schema.columns"`, and
`"information_schema.views"` are added.
2. In that PostgreSQL database, create a user schema (for example `public`)
containing a
normal user table named `tables` (or `columns`/`views`) and connect it to
Superset; then,
from SQL Lab, run a multi-statement query like `SET search_path = public;
SELECT * FROM
tables;` which is parsed into a `SQLScript` in `superset/sql_lab.py:420-459`
(lines 2-3)
and passed to `parsed_script.get_disallowed_tables(disallowed_tables,
effective_schema)`
at lines 33-35.
3. During parsing, `SQLScript.get_disallowed_tables` in
`superset/sql/parse.py:1833-1912`
iterates statements; the first `SET search_path` causes
`SQLStatement.changes_search_path()` (implemented at
`superset/sql/parse.py:1055-1115`,
lines 25-46) to return True, setting `schema_indeterminate = True` for
subsequent
statements, after which the second `SELECT * FROM tables` is processed with
`schema_indeterminate` enabled.
4. For the second statement, `SQLStatement.get_disallowed_tables()` in
`superset/sql/parse.py:1144-1223` (lines 6-45) records the unqualified table
`tables` in
`present_unqualified`, and because `schema_indeterminate` is True, it
matches the bare
name `tables` against the qualified denylist entry
`information_schema.tables` (lines
38-42), causing `parsed_script.get_disallowed_tables()` to return
`{"information_schema.tables"}`; `superset/sql_lab.py:420-459` then raises
`SupersetDisallowedSQLTableException` at lines 36-37, blocking the query
even though it
only references the user table `public.tables`, demonstrating the
overblocking described
in the suggestion.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=31d49f19cedb427fbf7797864c7df204&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=31d49f19cedb427fbf7797864c7df204&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/config.py
**Line:** 2069:2072
**Comment:**
*Logic Error: Adding very common object names as schema-qualified
denylist entries (for example `information_schema.tables` /
`information_schema.columns` / `information_schema.views`) causes false
positives when a query script changes `search_path`: the parser intentionally
enters an indeterminate mode and matches unqualified names against the bare
part of qualified denylist entries, so legitimate user tables named
`tables`/`columns`/`views` can be blocked even outside `information_schema`.
Either avoid shipping these generic entries by default, or tighten the matcher
to only widen when the runtime `search_path` can actually resolve to
`information_schema`.
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=67a0078bd8a034181f4aae7be59343860a908535641df5a2f7041b73d67c3c45&reaction=like'>π</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F41120&comment_hash=67a0078bd8a034181f4aae7be59343860a908535641df5a2f7041b73d67c3c45&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]
