fix(sql): avoid false positives in disallowed function checks by LaurinBrechter · Pull Request #40963 · apache/superset

LaurinBrechter · 2026-06-11T06:43:03Z

SUMMARY

The SQL executor's disallowed-function check used substring matching on the raw SQL string, causing false positives when column or table names contained a disallowed function name as a substring (e.g. skilllevel triggering the kill blocklist for MySQL).

This change uses SQLScript.check_functions_present() — the same AST-based approach already used in sql_lab.py and models/helpers.py — so only actual function invocations are blocked.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

N/A — backend-only change.

TESTING INSTRUCTIONS

Configure DISALLOWED_SQL_FUNCTIONS with {"mysql": {"kill"}} in superset_config.py
Run a query that references a column like skilllevel — it should succeed
Run SELECT KILL(123) — it should be rejected with an error mentioning kill

Run unit tests:

pytest tests/unit_tests/sql/execution/test_executor.py::test_execute_disallowed_function_not_matched_in_identifiers tests/unit_tests/sql/execution/test_executor.py::test_execute_disallowed_mysql_kill_function -v

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

Use SQLScript.check_functions_present() instead of substring matching so identifiers containing disallowed function names (e.g. "kill" in "skilllevel") are not incorrectly blocked. Co-authored-by: Cursor <cursoragent@cursor.com>

bito-code-review · 2026-06-11T06:43:14Z

Code Review Agent Run #beec35

Actionable Suggestions - 0

Additional Suggestions - 1

tests/unit_tests/sql/execution/test_executor.py - 1

Inconsistent config mocking · Line 386-397

The test `test_execute_disallowed_mysql_kill_function` is missing `SQL_MAX_ROW` and `QUERY_LOGGER` config keys that are consistently set in other tests in this file (e.g., lines 364-373, 321, 343). While the test will still function, this inconsistency could cause issues if the test fixture's default values change.

Code suggestion

--- a/tests/unit_tests/sql/execution/test_executor.py
+++ b/tests/unit_tests/sql/execution/test_executor.py
 @@ -391,6 +391,8 @@ def test_execute_disallowed_mysql_kill_function(
      mocker.patch.dict(
          current_app.config,
          {
+            "SQL_MAX_ROW": None,
              "SQL_QUERY_MUTATOR": None,
              "SQLLAB_TIMEOUT": 30,
+            "QUERY_LOGGER": None,
              "DISALLOWED_SQL_FUNCTIONS": {"mysql": {"kill"}},
          },
      )

Review Details

Files reviewed - 2 · Commit Range: d28783b..d28783b
- superset/sql/execution/executor.py
- tests/unit_tests/sql/execution/test_executor.py
Files skipped - 0
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

LaurinBrechter · 2026-06-11T07:08:35Z

/review

bito-code-review

Code Review Agent Run #1be33c

Actionable Suggestions - 1

superset/sql/parse.py - 1
- Missing unit tests for new method · Line 838-871

Review Details

Files reviewed - 3 · Commit Range: d28783b..bc26b66
- superset/sql/execution/executor.py
- superset/sql/parse.py
- tests/unit_tests/sql/execution/test_executor.py
Files skipped - 0
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

bito-code-review · 2026-06-11T07:32:40Z

@@ -864,7 +868,7 @@ def check_functions_present(self, functions: set[str]) -> bool:
            else:
                present.add(function.name.upper())

-        return any(function.upper() in present for function in functions)
+        return present


Missing unit tests for new method

The new public method get_present_functions() has no direct unit tests. Only indirect coverage exists via check_functions_present tests. Per BITO.md rule [11730], new tools should have dedicated unit tests covering success paths, error scenarios, and edge cases.

Code Review Run #1be33c

Should Bito avoid suggestions like this for future reviews? (Manage Rules)

Yes, avoid them

bito-code-review · 2026-06-11T10:19:48Z

Code Review Agent Run #32fd30

Actionable Suggestions - 0

Additional Suggestions - 1

superset/sql/parse.py - 1
- Missing unit tests for get_present_functions · Line 1472-1479
  
  The new `get_present_functions()` public method on `SQLScript` lacks dedicated unit tests. While `check_functions_present` indirectly tests this functionality, BITO.md rule [11730] recommends comprehensive unit tests for new tools covering success paths, error scenarios, and edge cases.

Review Details

Files reviewed - 3 · Commit Range: d28783b..bc26b66
- superset/sql/execution/executor.py
- superset/sql/parse.py
- tests/unit_tests/sql/execution/test_executor.py
Files skipped - 0
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

pull-request-size Bot added the size/M label Jun 11, 2026

dosubot Bot added the sqllab Namespace | Anything related to the SQL Lab label Jun 11, 2026

codeant-ai-for-open-source Bot reviewed Jun 11, 2026

View reviewed changes

Comment thread superset/sql/execution/executor.py Outdated

Comment thread tests/unit_tests/sql/execution/test_executor.py Outdated

fix(sql): use single AST pass for disallowed function checks

bc26b66

pull-request-size Bot added size/L and removed size/M labels Jun 11, 2026

bito-code-review Bot reviewed Jun 11, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(sql): avoid false positives in disallowed function checks#40963

fix(sql): avoid false positives in disallowed function checks#40963
LaurinBrechter wants to merge 2 commits into
apache:masterfrom
LaurinBrechter:fix/sql-executor-disallowed-function-false-positives

LaurinBrechter commented Jun 11, 2026

Uh oh!

bito-code-review Bot commented Jun 11, 2026 •

edited

Loading

Code Review Agent Run #beec35

Uh oh!

Uh oh!

Uh oh!

LaurinBrechter commented Jun 11, 2026

Uh oh!

bito-code-review Bot left a comment •

edited

Loading

Uh oh!

bito-code-review Bot Jun 11, 2026

Uh oh!

bito-code-review Bot commented Jun 11, 2026 •

edited

Loading

Code Review Agent Run #32fd30

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

LaurinBrechter commented Jun 11, 2026

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Uh oh!

bito-code-review Bot commented Jun 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review Agent Run #beec35

Uh oh!

Uh oh!

Uh oh!

LaurinBrechter commented Jun 11, 2026

Uh oh!

bito-code-review Bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Code Review Agent Run #1be33c

Uh oh!

bito-code-review Bot Jun 11, 2026

Choose a reason for hiding this comment

Uh oh!

bito-code-review Bot commented Jun 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review Agent Run #32fd30

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

bito-code-review Bot commented Jun 11, 2026 •

edited

Loading

bito-code-review Bot left a comment •

edited

Loading

bito-code-review Bot commented Jun 11, 2026 •

edited

Loading