codeant-ai-for-open-source[bot] commented on code in PR #40963:
URL: https://github.com/apache/superset/pull/40963#discussion_r3434966403
##########
superset/sql/execution/executor.py:
##########
@@ -691,16 +716,36 @@ def _check_disallowed_functions(self, script: SQLScript)
-> set[str] | None:
if not engine_disallowed:
return None
- # Check each statement for disallowed functions
- found = set()
- for statement in script.statements:
- # Use the statement's AST to check for function calls
- statement_str = str(statement).upper()
- for func in engine_disallowed:
- if func.upper() in statement_str:
- found.add(func)
+ present = script.get_present_functions()
+ found = {func for func in engine_disallowed if func.upper() in present}
+ return found or None
+
+ def _check_disallowed_functions_in_raw_sql(self, sql: str) -> set[str] |
None:
+ """
+ Fallback, text-based disallowed-function check for SQL that failed
+ to parse entirely.
+
+ This is intentionally only used when AST parsing fails (see
+ ``_prepare_sql``) — using a word-boundary regex on raw SQL as the
+ primary check would reintroduce the false positives the AST-based
+ ``_check_disallowed_functions`` was built to avoid.
+
+ :param sql: Raw (rendered) SQL that could not be parsed
+ :returns: Set of disallowed functions found, or None if none found
+ """
+ disallowed_config = app.config.get("DISALLOWED_SQL_FUNCTIONS", {})
+ engine_name = self.database.db_engine_spec.engine
+
+ engine_disallowed = disallowed_config.get(engine_name, set())
+ if not engine_disallowed:
+ return None
- return found if found else None
+ found = {
+ func
+ for func in engine_disallowed
+ if re.search(rf"\b{re.escape(func)}\b", sql, re.IGNORECASE)
+ }
+ return found or None
Review Comment:
**Suggestion:** The parse-error fallback matches disallowed names as
standalone words anywhere in raw SQL, so malformed queries that merely mention
a blocked name in identifiers, comments, or string literals will be
misclassified as disallowed-function violations instead of parse errors.
Restrict the fallback to function-invocation syntax (for example, name followed
by `(` with optional whitespace/comments) so it only flags actual calls. [logic
error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ SQL MCP tool mislabels syntax errors as disallowed functions.
- ⚠️ SQL Lab users see misleading disallowed-function error messages.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Use the MCP SQL tool `execute_sql` defined in
`superset/mcp_service/sql_lab/tool/execute_sql.py:68-88`, which builds
`QueryOptions` and
calls `database.execute(request.sql, options)` in the block at
`execute_sql.py:160-20`.
2. Ensure the target `Database` model (loaded in `execute_sql.py:93-99` and
executing via
`Database.execute` in `superset/models/core.py:6-20`) is configured with
`DISALLOWED_SQL_FUNCTIONS` for its engine (e.g. MySQL `"kill"`), which is
read in
`_check_disallowed_functions_in_raw_sql` via
`app.config.get("DISALLOWED_SQL_FUNCTIONS",
{})` at `superset/sql/execution/executor.py:736-742`.
3. From the MCP client, submit an intentionally malformed SQL statement that
merely
mentions the blocked name in a non-call context, for example `SELECT 'kill`
(unterminated
string literal) or `SELECT col AS kill /*` (unclosed comment); this SQL
flows through
`Database.execute()` (`superset/models/core.py:18-20`) into
`SQLExecutor.execute()`
(`superset/sql/execution/executor.py:17-76`), which calls `_prepare_sql()` at
`executor.py:231` and `355`.
4. Inside `_prepare_sql` (`superset/sql/execution/executor.py:408-96`),
`SQLScript(rendered_sql, ...)` at lines 65-68 raises `SupersetParseError`
due to the
malformed SQL, triggering the `except SupersetParseError` block at lines
69-78; this calls
`_check_disallowed_functions_in_raw_sql(rendered_sql)`, whose `found = { ...
if
re.search(rf"\b{re.escape(func)}\b", sql, re.IGNORECASE) }` comprehension at
PR lines
744-748 matches the word `kill` inside the string/comment, returns
`{"kill"}`, and causes
`_prepare_sql` to raise `SupersetSecurityException("Disallowed SQL
functions: kill")`,
which `SQLExecutor.execute()` catches in the `except
SupersetSecurityException` branch at
`executor.py:78-85` and returns as a `QueryResult` with status `FAILED` and a
disallowed-function error message instead of surfacing a parse/syntax error.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=e818788d579e4252bdc9e06fc750c109&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=e818788d579e4252bdc9e06fc750c109&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/sql/execution/executor.py
**Line:** 744:748
**Comment:**
*Logic Error: The parse-error fallback matches disallowed names as
standalone words anywhere in raw SQL, so malformed queries that merely mention
a blocked name in identifiers, comments, or string literals will be
misclassified as disallowed-function violations instead of parse errors.
Restrict the fallback to function-invocation syntax (for example, name followed
by `(` with optional whitespace/comments) so it only flags actual calls.
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%2F40963&comment_hash=0fb3c9725708c5158ef805fbdfb285e82b2dba69921689c105fedca25e766663&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F40963&comment_hash=0fb3c9725708c5158ef805fbdfb285e82b2dba69921689c105fedca25e766663&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]
