-
Notifications
You must be signed in to change notification settings - Fork 17.9k
feat(sql): schema-qualified table denylist + information_schema/lo_* defaults #41120
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
6e97e9f
0f033f0
22ad47d
9d9856f
02c97ac
4905211
f1259b6
4617bdc
102f2ec
de1485c
21a7362
2f15332
c95c452
e75c66c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1958,6 +1958,24 @@ def engine_context_manager( # pylint: disable=unused-argument | |
| "pg_read_file", | ||
| "pg_ls_dir", | ||
| "pg_read_binary_file", | ||
| # PostgreSQL large-object functions: writers can plant arbitrary | ||
| # bytes on the server filesystem (lo_export, lo_from_bytea, lowrite, | ||
| # lo_put, lo_create, lo_creat, lo_import), readers can pull bytes back | ||
| # out (lo_get, loread), lo_truncate/lo_truncate64 shrink existing | ||
| # large objects, and lo_unlink deletes them outright. Defense-in-depth | ||
| # on top of is_mutating()'s function-name check. | ||
| "lo_from_bytea", | ||
| "lo_export", | ||
| "lo_import", | ||
| "lo_put", | ||
| "lo_create", | ||
| "lo_creat", | ||
| "lowrite", | ||
| "lo_get", | ||
| "loread", | ||
| "lo_truncate", | ||
| "lo_truncate64", | ||
| "lo_unlink", | ||
|
sha174n marked this conversation as resolved.
sha174n marked this conversation as resolved.
|
||
| # XML functions that can execute SQL | ||
| "database_to_xml", | ||
| "database_to_xmlschema", | ||
|
|
@@ -2048,6 +2066,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", | ||
|
Comment on lines
+2069
to
+2072
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: Adding very common object names as schema-qualified denylist entries (for example Severity Level: Major
|
||
| "information_schema.routines", | ||
| "information_schema.role_table_grants", | ||
| "information_schema.role_column_grants", | ||
| "information_schema.role_routine_grants", | ||
| "information_schema.table_privileges", | ||
| "information_schema.column_privileges", | ||
| "information_schema.usage_privileges", | ||
| "information_schema.key_column_usage", | ||
| "information_schema.table_constraints", | ||
| "information_schema.referential_constraints", | ||
| "information_schema.view_table_usage", | ||
| "information_schema.applicable_roles", | ||
| "information_schema.enabled_roles", | ||
| }, | ||
| "mysql": { | ||
| "mysql.user", | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -698,6 +698,46 @@ def get_default_schema_for_query( | |
| self, query, template_params | ||
| ) | ||
|
|
||
| def resolve_query_default_schema( | ||
| self, | ||
| sql: str, | ||
| schema: str | None, | ||
| catalog: str | None, | ||
| template_params: Optional[dict[str, Any]] = None, | ||
| ) -> str | None: | ||
| """ | ||
| Resolve the effective per-query default schema for a SQL string through | ||
| the query-aware :meth:`get_default_schema_for_query`. | ||
|
|
||
| Builds a transient (unsaved) ``Query`` probe so the engine spec resolves | ||
| the schema exactly as execution does -- running engine-specific per-query | ||
| security gates too -- then expunges it so the ``database`` backref's | ||
| ``cascade="all, delete-orphan"`` cannot autoflush this incomplete row | ||
| into the session. Centralizes the probe construction shared by the SQL | ||
| Lab executor, the cost-estimate command, and datasource denylist checks | ||
| so the schema-resolution behavior stays in sync across these | ||
| security-sensitive paths. | ||
|
|
||
| :param sql: Original (pre-render) SQL the query will execute | ||
| :param schema: Explicit per-query schema, if any | ||
| :param catalog: Resolved catalog | ||
| :param template_params: Jinja template parameters, if any | ||
| :returns: The runtime-resolved default schema, or None | ||
| """ | ||
| from superset.models.sql_lab import Query | ||
|
|
||
| probe_query = Query( | ||
| database=self, | ||
| sql=sql, | ||
| schema=schema or None, | ||
| catalog=catalog, | ||
| client_id=utils.shortid()[:10], | ||
| user_id=utils.get_user_id(), | ||
| ) | ||
|
Comment on lines
+729
to
+736
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: Add an explicit type annotation for the local query probe variable to satisfy the requirement for annotating relevant variables. [custom_rule] Severity Level: Minor Why it matters? 🤔This is new Python code introducing a local variable that can be explicitly annotated. The custom rule requires type hints for relevant variables where possible, so omitting an annotation here is a real violation. (Use Cmd/Ctrl + Click for best experience) Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** superset/models/core.py
**Line:** 729:736
**Comment:**
*Custom Rule: Add an explicit type annotation for the local query probe variable to satisfy the requirement for annotating relevant variables.
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 |
||
| if probe_query in db.session: | ||
| db.session.expunge(probe_query) | ||
| return self.get_default_schema_for_query(probe_query, template_params) | ||
|
|
||
| @staticmethod | ||
| def post_process_df(df: pd.DataFrame) -> pd.DataFrame: | ||
| def column_needs_conversion(df_series: pd.Series) -> bool: | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
Comment on lines
+1206
to
+1207
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: The early return for an explicit schema bypasses Severity Level: Critical 🚨- ❌ Datasource queries bypass Postgres search_path security gate.
- ⚠️ Denylist behavior diverges between SQL Lab and datasource.Steps of Reproduction ✅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.(Use Cmd/Ctrl + Click for best experience) Prompt for AI Agent 🤖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 |
||
| if not hasattr(self, "_denylist_default_schema"): | ||
| catalog = self.catalog or self.database.get_default_catalog() | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: Add an explicit type annotation for the Severity Level: Minor Why it matters? 🤔This is a newly introduced Python local variable that can be annotated, but it is assigned without a type hint. That matches the custom rule requiring type hints for relevant new or modified variables. (Use Cmd/Ctrl + Click for best experience) Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** superset/models/helpers.py
**Line:** 1209:1209
**Comment:**
*Custom Rule: Add an explicit type annotation for the `catalog` local variable to satisfy the type-hint requirement for newly introduced variables.
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 |
||
| self._denylist_default_schema = self.database.resolve_query_default_schema( | ||
| sql, None, catalog | ||
| ) | ||
|
Comment on lines
+1210
to
+1212
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: Provide a type annotation for the cached Severity Level: Minor Why it matters? 🤔This is a new instance attribute assignment in Python and it is not explicitly typed. Since the value is a schema string cache that can be annotated, the suggestion correctly identifies a type-hint omission. (Use Cmd/Ctrl + Click for best experience) Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** superset/models/helpers.py
**Line:** 1210:1212
**Comment:**
*Custom Rule: Provide a type annotation for the cached `_denylist_default_schema` assignment so this new instance-level value is not introduced as implicitly typed.
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 |
||
| return self._denylist_default_schema | ||
|
Comment on lines
+1208
to
+1213
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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] Severity Level: Critical 🚨- ❌ Postgres search_path gate skipped for later datasource queries.
- ⚠️ Denylist table matching can use stale runtime schema.Steps of Reproduction ✅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.(Use Cmd/Ctrl + Click for best experience) Prompt for AI Agent 🤖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 |
||
|
|
||
| def _process_sql_expression( # pylint: disable=too-many-arguments | ||
| self, | ||
| expression: Optional[str], | ||
|
|
@@ -1236,24 +1261,23 @@ def _process_sql_expression( # pylint: disable=too-many-arguments | |
| disallowed_functions | ||
| ): | ||
| raise SupersetDisallowedSQLFunctionException(disallowed_functions) | ||
| if disallowed_tables and parsed.check_tables_present(disallowed_tables): | ||
| if disallowed_tables: | ||
| # Report only the tables actually found in the expression, | ||
| # mirroring the canonical execution-time gate in | ||
| # `superset.sql_lab._validate_query` so the user-facing | ||
| # error doesn't echo the operator's full denylist. | ||
| present_tables = { | ||
| table.table.lower() | ||
| for statement in parsed.statements | ||
| for table in statement.tables | ||
| } | ||
| found_tables = { | ||
| table | ||
| for table in disallowed_tables | ||
| if table.lower() in present_tables | ||
| } | ||
| raise SupersetDisallowedSQLTableException( | ||
| found_tables or disallowed_tables | ||
| # error doesn't echo the operator's full denylist. Honors | ||
| # schema-qualified denylist entries (e.g. | ||
| # ``information_schema.tables``) and resolves unqualified | ||
| # references against the same runtime schema the SQL Lab / | ||
| # executor gate resolves them to (via the query-aware | ||
| # resolver, not the static inspector default), so both | ||
| # surfaces enforce the denylist consistently. | ||
| effective_schema = self._resolve_denylist_schema(sql_to_check) | ||
| found_tables = parsed.get_disallowed_tables( | ||
| disallowed_tables, effective_schema | ||
| ) | ||
|
sha174n marked this conversation as resolved.
Comment on lines
+1275
to
1278
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: Add explicit type annotations for the new Severity Level: Minor Why it matters? 🤔Both locals are newly introduced and can be statically annotated, but they are not. This is a valid instance of the project rule requiring type hints on relevant new variables. (Use Cmd/Ctrl + Click for best experience) Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** superset/models/helpers.py
**Line:** 1275:1278
**Comment:**
*Custom Rule: Add explicit type annotations for the new `effective_schema` and `found_tables` locals in the disallowed-table validation 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 |
||
| if found_tables: | ||
| raise SupersetDisallowedSQLTableException(found_tables) | ||
| return expression | ||
|
|
||
| def _process_select_expression( | ||
|
|
@@ -1480,19 +1504,21 @@ def _raise_for_disallowed_sql(self, sql: str) -> None: | |
| disallowed_functions | ||
| ): | ||
| raise SupersetDisallowedSQLFunctionException(disallowed_functions) | ||
| if disallowed_tables and parsed_script.check_tables_present(disallowed_tables): | ||
| if disallowed_tables: | ||
| # Report only the tables actually found in the query, mirroring the | ||
| # canonical execution-time gate so the user-facing error doesn't | ||
| # echo the operator's full denylist. | ||
| present_tables = { | ||
| table.table.lower() | ||
| for statement in parsed_script.statements | ||
| for table in statement.tables | ||
| } | ||
| found_tables = { | ||
| table for table in disallowed_tables if table.lower() in present_tables | ||
| } | ||
| raise SupersetDisallowedSQLTableException(found_tables or disallowed_tables) | ||
| # echo the operator's full denylist. Honors schema-qualified | ||
| # denylist entries (e.g. ``information_schema.tables``) and resolves | ||
| # unqualified references against the same runtime schema the SQL Lab | ||
| # / executor gate resolves them to (via the query-aware resolver, | ||
| # not the static inspector default), so both surfaces enforce the | ||
| # denylist consistently. | ||
| effective_schema = self._resolve_denylist_schema(sql) | ||
| found_tables = parsed_script.get_disallowed_tables( | ||
| disallowed_tables, effective_schema | ||
| ) | ||
|
sha174n marked this conversation as resolved.
Comment on lines
+1516
to
+1519
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: Add explicit type annotations for the new Severity Level: Minor Why it matters? 🤔These are new local variables in Python code and they are left implicitly typed. They are suitable for annotation, so this is a real violation of the type-hint rule. (Use Cmd/Ctrl + Click for best experience) Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** superset/models/helpers.py
**Line:** 1516:1519
**Comment:**
*Custom Rule: Add explicit type annotations for the new `effective_schema` and `found_tables` locals in the query-level disallowed-table check.
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 |
||
| if found_tables: | ||
| raise SupersetDisallowedSQLTableException(found_tables) | ||
|
|
||
| def query(self, query_obj: QueryObjectDict) -> QueryResult: | ||
| """ | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: This path resolves the effective schema from
self._sqleven though_apply_sql_securityis validating thesqlargument. 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 samesqlvalue being checked in this method. [security]Severity Level: Major⚠️
Steps of Reproduction ✅
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖