codeant-ai-for-open-source[bot] commented on code in PR #37411:
URL: https://github.com/apache/superset/pull/37411#discussion_r2757554072
##########
superset/sql/execution/executor.py:
##########
@@ -684,6 +696,34 @@ def _check_disallowed_functions(self, script: SQLScript)
-> set[str] | None:
return found if found else None
+ def _check_disallowed_tables(self, script: SQLScript) -> set[str] | None:
+ """
+ Check for disallowed SQL tables/views.
+
+ :param script: Parsed SQL script
+ :returns: Set of disallowed tables found, or None if none found
+ """
+ disallowed_config = app.config.get("DISALLOWED_SQL_TABLES", {})
+ engine_name = self.database.db_engine_spec.engine
+
+ # Get disallowed tables for this engine
+ engine_disallowed = disallowed_config.get(engine_name, set())
+ if not engine_disallowed:
+ return None
+
+ # Use AST-based table detection
+ if script.check_tables_present(engine_disallowed):
+ # Return the specific tables that were found
+ found = set()
+ for statement in script.statements:
+ present = {table.table.lower() for table in statement.tables}
+ for table in engine_disallowed:
+ if table.lower() in present:
+ found.add(table)
+ return found if found else None
Review Comment:
**Suggestion:** The disallowed-table check can silently allow queries that
`SQLScript.check_tables_present` has identified as using blocked tables if the
secondary scan over `statement.tables` fails to match any names, effectively
turning a positive detection into "no disallowed tables" and creating a
potential security bypass. [security]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ System catalog queries may bypass table blocklist.
- ⚠️ MCP SQL validation can be weakened.
- ❌ Sensitive infrastructure data potentially exposed.
```
</details>
```suggestion
if found:
return found
# Fallback: if the AST reported that disallowed tables are
present
# but we couldn't map them back via statement.tables, return the
full
# configured blocklist to ensure the query is still rejected.
return set(engine_disallowed)
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure per-engine blocklist in superset/config.py under
DISALLOWED_SQL_TABLES and
ensure the engine name matches database.db_engine_spec.engine used by
SQLExecutor
(executor.py references engine at line ~707).
2. Submit a query that triggers SQLScript.check_tables_present(...). The
executor calls
_check_security(script) which calls _check_disallowed_tables(script)
(executor.py:
_check_security -> _check_disallowed_tables, see executor.py lines ~446 and
~699).
3. If SQLScript.check_tables_present(engine_disallowed) returns True (AST
detected a
blocked table), control enters the if-block (executor.py line ~715) but the
code then
enumerates statement.tables and attempts to map AST table nodes back to
configured names.
4. In cases where script.check_tables_present reports presence but
statement.tables does
not contain a matching .table entry (e.g., mapping failure or representation
differences),
the inner loop yields an empty found set and the function returns None ->
treating the
query as allowed. This turns a positive AST detection into a no-op and can
let a
blocked-table query pass. The improved_code returns the configured blocklist
as a safe
fallback so the query is rejected.
```
</details>
<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:** 723:723
**Comment:**
*Security: The disallowed-table check can silently allow queries that
`SQLScript.check_tables_present` has identified as using blocked tables if the
secondary scan over `statement.tables` fails to match any names, effectively
turning a positive detection into "no disallowed tables" and creating a
potential security bypass.
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.
```
</details>
--
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]
