feat(sql): schema-qualified table denylist + information_schema/lo_* defaults#41120
Conversation
…defaults This is the breaking-default half split out of apache#40421. It makes the SQL table denylist schema-aware and rewires the call sites to report exactly which denylisted tables were hit. In superset/sql/parse.py: - check_tables_present now distinguishes schema-qualified denylist entries (e.g. information_schema.tables, mysql.user, sys.sql_logins) from bare-name entries (e.g. pg_stat_activity): qualified entries only match qualified references, bare entries match by table name. - new get_disallowed_tables returns the matched entries in their original denylist form, resolving unqualified references against the caller's default schema. - new changes_search_path detects SET search_path / set_config rebinds so a script that changes the search path cannot bypass a qualified entry via an unqualified reference. Call sites in models/helpers.py, sql/execution/executor.py, sql_lab.py and commands/sql_lab/estimate.py are updated to pass the default schema and surface the matched tables. New shipped defaults and the migration note land in config.py and UPDATING.md. Targeted at 7.0 because it changes default behavior. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
dosubot
Bot
added
risk:breaking-change
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #41120 +/- ##
=======================================
Coverage 64.41% 64.42%
=======================================
Files 2668 2668
Lines 147198 147258 +60
Branches 33949 33965 +16
=======================================
+ Hits 94824 94872 +48
Misses 50657 50657
- Partials 1717 1729 +12
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![bito-code-review[bot]](https://avatars.githubusercontent.com/in/1061978?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Agent Run #58aed7
Actionable Suggestions - 1
-
superset/config.py - 1
- CWE-276: lo_creat Missing from Blocklist · Line 1880-1888
Additional Suggestions - 1
-
superset/commands/sql_lab/estimate.py - 1
-
CWE-287: Schema Resolution Bypasses Security Gate · Line 105-119Using `get_default_schema` instead of `get_default_schema_for_query` bypasses the Postgres `search_path` injection security gate that the RLS path runs. The comment on line 109 already acknowledges Postgres, and `config.py` lines 1979–1986 document that `information_schema.*` denylist entries are specifically designed to require this runtime resolution — but `get_default_schema` skips the `process_jinja_sql` + `search_path` scan that `get_default_schema_for_query` performs (`postgres.py` lines 617–628), creating a gap where a malicious `SET search_path` query bypasses the denylist check on the estimate path while being blocked on the execution path.
Code suggestion
--- a/superset/commands/sql_lab/estimate.py +++ b/superset/commands/sql_lab/estimate.py @@ -109,9 +109,14 @@ class QueryEstimationCommand: # schema so an unqualified reference that the connection resolves to # it at runtime (e.g. Postgres ``public``) still matches a qualified # entry, even when the request omitted the schema. - effective_schema = self._schema or self._database.get_default_schema( - self._catalog or self._database.get_default_catalog() - ) + # NOTE: Using get_default_schema_for_query (not get_default_schema) + # to invoke engine-specific security gates, particularly the Postgres + # search_path injection check that get_default_schema skips. + _query = Query() + _query.schema = self._schema or None + _query.catalog = self._catalog + _query.sql = sql_statement # already available in scope + effective_schema = self._database.get_default_schema_for_query(_query) found_tables = parsed_script.get_disallowed_tables( disallowed_tables, effective_schema )
-
Filtered by Review Rules
Bito filtered these suggestions based on rules created automatically for your feedback. Manage rules.
-
superset/sql/parse.py - 1
- Function signature mismatch causes TypeError · Line 1459-1464
Review Details
-
Files reviewed - 7 · Commit Range:
6e97e9f..6e97e9f- superset/commands/sql_lab/estimate.py
- superset/config.py
- superset/models/helpers.py
- superset/sql/execution/executor.py
- superset/sql/parse.py
- superset/sql_lab.py
- tests/unit_tests/sql/parse_tests.py
-
Files skipped - 1
- UPDATING.md - Reason: Filter setting
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
get_disallowed_tables only indexed the schema.table form, so a catalog-qualified denylist entry would never match. Index the fully-qualified form too, and document that an unqualified reference under an unknown default schema can only be matched via the schema-indeterminate bare-name fallback (an inherent limit of static analysis without the live search_path). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- sql_lab.py: resolve the denylist effective schema through the query-aware get_default_schema_for_query, matching the RLS gate, so unqualified references match against the schema the engine actually resolves them to at runtime. - models/helpers.py: fall back to the database default schema in both adhoc-expression and datasource denylist checks so a schema-qualified entry cannot be bypassed by an unqualified reference when the datasource has no schema (e.g. virtual datasets). - config.py / UPDATING.md: add the lo_creat large-object alias to the PostgreSQL defaults and keep the migration note in sync (loread, lo_creat). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@bito-code-review Thanks. The On the estimate-path schema suggestion: the script-level |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Resolve the effective schema for the cost-estimate denylist check through get_default_schema_for_query (the query-aware resolver used by the execution path) instead of the static get_default_schema, so engine-specific per-query gates run for the denylist check too. Share a single resolution between the denylist check and RLS injection. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@bito-code-review Follow-up: on reflection I went ahead and switched the estimate denylist check to resolve the effective schema via |
There was a problem hiding this comment.
Code Review Agent Run #0c2d63
Actionable Suggestions - 1
-
superset/sql/parse.py - 1
- Missing test for three-part matching · Line 1001-1007
Review Details
-
Files reviewed - 5 · Commit Range:
6e97e9f..22ad47d- superset/sql/parse.py
- tests/unit_tests/sql/parse_tests.py
- superset/config.py
- superset/models/helpers.py
- superset/sql_lab.py
-
Files skipped - 1
- UPDATING.md - Reason: Filter setting
-
Tools
- Whispers (Secret Scanner) - ✔︎ Successful
- Detect-secrets (Secret Scanner) - ✔︎ Successful
- MyPy (Static Code Analysis) - ✔︎ Successful
- Astral Ruff (Static Code Analysis) - ✔︎ Successful
Bito Usage Guide
Commands
Type the following command in the pull request comment and save the comment.
-
/review- Manually triggers a full AI review. -
/pause- Pauses automatic reviews on this pull request. -
/resume- Resumes automatic reviews. -
/resolve- Marks all Bito-posted review comments as resolved. -
/abort- Cancels all in-progress reviews.
Refer to the documentation for additional commands.
Configuration
This repository uses Superset You can customize the agent settings here or contact your Bito workspace admin at evan@preset.io.
Documentation & Help
AI Code Review powered by
- Resolve the per-query effective schema once in execute_sql_statements and share it between the denylist check and RLS injection (was resolved twice). - Resolve the SQLExecutor path's schema through the query-aware get_default_schema_for_query, matching the SQL Lab and estimate paths so the denylist check and RLS injection use the schema the engine resolves at runtime and run the engine's per-query gates. - Add get_disallowed_tables coverage for three-part (catalog.schema.table) denylist entries. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@bito-code-review Added the three-part ( |
Code Review Agent Run #7c4747Actionable Suggestions - 0Additional Suggestions - 2
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@bito-code-review Added dedicated unit tests for |
…* coverage - Resolve the chart/adhoc-expression effective schema through the query-aware get_default_schema_for_query (memoized per datasource), matching the SQL Lab / executor gate and avoiding a per-expression inspector round-trip. - Add lo_truncate/lo_truncate64 to the PostgreSQL function list and to the mutating-function detector; add the legacy lo_creat alias to the detector for parity with the function list. - Add a docstring to the estimate denylist test and cover the new schema-resolution behaviour. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Code Review Agent Run #1876d0Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
Thanks for the work here. The overall approach makes sense to me, especially the move toward consistent denylist enforcement across SQL Lab, executor, estimation, and datasource validation paths, along with the schema-qualified matching support and expanded test coverage. I noticed the transient Query construction + db.session.expunge(...) pattern now appears in multiple places (estimate.py, helpers.py, and executor.py) to resolve the effective schema through get_default_schema_for_query(). Would it make sense to centralize this logic into a shared helper/utility so future changes to query-aware schema resolution only need to be made in one place? It feels like these code paths are intentionally trying to maintain execution-parity, and a common helper could help prevent divergence over time. Not a blocker—just wondering if there’s an opportunity to reduce duplication and keep the schema-resolution behavior synchronized across these security-sensitive paths. |
Extract the transient-probe schema resolution shared by the SQL Lab executor, the cost-estimate command, and datasource denylist checks into a single Database.resolve_query_default_schema helper, and resolve the schema consistently across the estimate and executor paths. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@durgaprasadml good call. Centralized the transient-probe schema resolution into a single |
|
@bito-code-review on the flagged points: |
… search_path rejection Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Code Review Agent Run #6c9fc8Actionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
Still lookin' good to me, but needs a rebase, it seems. |
…ylist-7.0 # Conflicts: # UPDATING.md # tests/unit_tests/models/core_test.py
Resolve additive conflicts in UPDATING.md and helpers_test.py by keeping both sides (our entries/tests plus upstream's). Suite green after merge.
| probe_query = Query( | ||
| database=self, | ||
| sql=sql, | ||
| schema=schema or None, | ||
| catalog=catalog, | ||
| client_id=utils.shortid()[:10], | ||
| user_id=utils.get_user_id(), | ||
| ) |
There was a problem hiding this comment.
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 table.lower() in present: | ||
| found_tables.add(table) | ||
| raise SupersetDisallowedSQLTableException(found_tables or disallowed_tables) | ||
| rls_enabled = is_feature_enabled("RLS_IN_SQLLAB") |
There was a problem hiding this comment.
Suggestion: Add an explicit type annotation for this new boolean variable assignment. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This new local is a boolean-like value derived from a feature flag and can be annotated as bool, so omitting a type hint violates the rule requiring type hints on newly added Python variables that can be annotated.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/sql_lab.py
**Line:** 436:436
**Comment:**
*Custom Rule: Add an explicit type annotation for this new boolean variable assignment.
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| # schema the engine actually uses at runtime -- engines without dynamic-schema | ||
| # support ignore the request's selected schema -- so both controls match the | ||
| # execution path instead of a schema that may never apply. | ||
| effective_schema = "" |
There was a problem hiding this comment.
Suggestion: Add a concrete type hint for this new schema variable where it is first declared. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This is a newly introduced local variable initialized to a string and later reused as a schema name, so it is clearly annotatable as str; the lack of a type hint matches the custom rule violation.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/sql_lab.py
**Line:** 445:445
**Comment:**
*Custom Rule: Add a concrete type hint for this new schema variable where it is first declared.
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| found_tables = parsed_script.get_disallowed_tables( | ||
| disallowed_tables, effective_schema | ||
| ) |
There was a problem hiding this comment.
Suggestion: Add an explicit type annotation for this new variable holding parsed denylisted tables. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This new intermediate variable is introduced without an explicit type hint. Since it stores the result of a function call and is a relevant local variable 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/sql_lab.py
**Line:** 452:454
**Comment:**
*Custom Rule: Add an explicit type annotation for this new variable holding parsed denylisted tables.
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 self.schema: | ||
| return self.schema | ||
| if not hasattr(self, "_denylist_default_schema"): | ||
| catalog = self.catalog or self.database.get_default_catalog() |
There was a problem hiding this comment.
Suggestion: Add an explicit type annotation for the catalog local variable to satisfy the type-hint requirement for newly introduced variables. [custom_rule]
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 | ||
| ) |
There was a problem hiding this comment.
Suggestion: Provide a type annotation for the cached _denylist_default_schema assignment so this new instance-level value is not introduced as implicitly typed. [custom_rule]
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| effective_schema = self._resolve_denylist_schema(sql_to_check) | ||
| found_tables = parsed.get_disallowed_tables( | ||
| disallowed_tables, effective_schema | ||
| ) |
There was a problem hiding this comment.
Suggestion: Add explicit type annotations for the new effective_schema and found_tables locals in the disallowed-table validation path. [custom_rule]
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| effective_schema = self._resolve_denylist_schema(sql) | ||
| found_tables = parsed_script.get_disallowed_tables( | ||
| disallowed_tables, effective_schema | ||
| ) |
There was a problem hiding this comment.
Suggestion: Add explicit type annotations for the new effective_schema and found_tables locals in the query-level disallowed-table check. [custom_rule]
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| resolve_mock = mocker.patch.object( | ||
| database, "get_default_schema_for_query", return_value="resolved_schema" | ||
| ) |
There was a problem hiding this comment.
Suggestion: Add an explicit type annotation for this new mock variable to comply with the rule requiring type hints on relevant variables. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This new local variable is introduced without a type annotation, and it is a relevant variable that can be annotated under the Python type-hints rule. The suggestion matches an actual omission in the new code.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/sql/execution/test_executor.py
**Line:** 1410:1412
**Comment:**
*Custom Rule: Add an explicit type annotation for this new mock variable to comply with the rule requiring type hints on 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| ) | ||
|
|
||
| executor = SQLExecutor(database) | ||
| options = QueryOptions(schema="explicit", template_params={"p": 1}) |
There was a problem hiding this comment.
Suggestion: Add a type annotation for this newly introduced options variable so the new code consistently follows the type-hint requirement. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This is a newly introduced Python variable that could be type-annotated, and the code omits an explicit hint. That is a real violation of the type-hints rule.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/sql/execution/test_executor.py
**Line:** 1415:1415
**Comment:**
*Custom Rule: Add a type annotation for this newly introduced options variable so the new code consistently follows the type-hint requirement.
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| gate = mocker.patch.object( | ||
| database, | ||
| "get_default_schema_for_query", | ||
| side_effect=SupersetSecurityException( | ||
| SupersetError( | ||
| message="blocked", | ||
| error_type=SupersetErrorType.QUERY_SECURITY_ACCESS_ERROR, | ||
| level=ErrorLevel.ERROR, | ||
| ) | ||
| ), | ||
| ) |
There was a problem hiding this comment.
Suggestion: Annotate this new patched gate variable with an explicit type to satisfy the type-hints rule for relevant variables. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This patched object is a new local variable and it lacks a type annotation even though it could 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:** tests/unit_tests/sql/execution/test_executor.py
**Line:** 1460:1470
**Comment:**
*Custom Rule: Annotate this new patched gate variable with an explicit type to satisfy the type-hints rule for 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 fixResolve additive conflicts with apache#40499 (SQL_MAX_PARSE_LENGTH): keep both our denylist entries/tests and upstream's parse-length gate. Suite green.
| "information_schema.tables", | ||
| "information_schema.columns", | ||
| "information_schema.schemata", | ||
| "information_schema.views", |
There was a problem hiding this 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]
Severity Level: Major ⚠️
- ❌ 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.Steps of Reproduction ✅
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.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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| if self.schema: | ||
| return self.schema |
There was a problem hiding this 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]
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() | ||
| self._denylist_default_schema = self.database.resolve_query_default_schema( | ||
| sql, None, catalog | ||
| ) | ||
| return self._denylist_default_schema |
There was a problem hiding this 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]
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
| resolved_schema = self._database.resolve_query_default_schema( | ||
| self._sql, self._schema, catalog, self._template_params | ||
| ) |
There was a problem hiding this 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]
Severity Level: Major ⚠️
- ❌ SQL Lab cost estimate denylists may misfire under templating.
- ⚠️ RLS predicates may target wrong schema during estimation.Steps of Reproduction ✅
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.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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| resolved_schema = self._resolve_query_schema(sql, opts, catalog) | ||
| schema = opts.schema or resolved_schema |
There was a problem hiding this 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]
Severity Level: Critical 🚨
- ❌ Core SQL execution security may use mismatched schema context.
- ⚠️ Table denylists could be bypassed with non-deterministic templates.Steps of Reproduction ✅
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.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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| 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 |
There was a problem hiding this 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]
Severity Level: Critical 🚨
- ❌ RESET search_path lets queries evade information_schema table denylists.
- ⚠️ Multi-statement SQLLab queries may bypass schema-qualified protections.Steps of Reproduction ✅
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.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
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
github-project-automation
Bot
moved this from Completed
to Lazy Consensus Reached
in Apache Superset 7.0
|
Bito Automatic Review Skipped – PR Already Merged |
3 participants
Summary
Split from #40421 (which keeps the non-breaking detection half). This is the table-denylist half, which carries a breaking default change, so it targets 7.0.
DISALLOWED_SQL_TABLES(so entries likeinformation_schema.tablesmatch an unqualified reference that resolves under that schema).DISALLOWED_SQL_TABLES/DISALLOWED_SQL_FUNCTIONS:information_schema.*views (table/column/privilege/view-definition metadata)lo_*)These default additions reject queries that reference them where they were previously allowed, hence the 7.0 target.
UPDATING.mddocuments the change and the per-deployment opt-out.No associated ticket; this is the follow-up half of #40421.
Testing