fix(mcp): Block destructive DDL (DROP, TRUNCATE, ALTER) in execute_sql#39621
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #39621 +/- ##
==========================================
- Coverage 64.54% 64.43% -0.11%
==========================================
Files 2565 2566 +1
Lines 133665 134024 +359
Branches 31056 31109 +53
==========================================
+ Hits 86269 86360 +91
- Misses 45904 46169 +265
- Partials 1492 1495 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| except Exception as parse_err: | ||
| await ctx.error( | ||
| "DDL pre-check failed to parse SQL, blocking query: %s" | ||
| % str(parse_err) | ||
| ) |
There was a problem hiding this comment.
I'm wondering if do we need to be more specific about the error here?
There was a problem hiding this comment.
I tried narrowing to SqlglotError but the disallowed-sql-import pylint rule blocks sqlglot imports outside superset/sql/.
dosubot
Bot
added
change:backend
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Code Review Agent Run #855e40Actionable Suggestions - 0Additional Suggestions - 1
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 |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
| """Tests for destructive DDL blocking in execute_sql.""" | ||
|
|
||
| @pytest.fixture | ||
| def ddl_mocks(self): |
There was a problem hiding this comment.
Suggestion: Add explicit type annotations to the fixture method parameters and return type so the new test fixture follows the typing rule for newly added functions. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The newly added fixture method is unannotated: self has no type and the function has no return type annotation. This matches the typing rule violation described by the suggestion.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/sql_lab/tool/test_execute_sql.py
**Line:** 1227:1227
**Comment:**
*Custom Rule: Add explicit type annotations to the fixture method parameters and return type so the new test fixture follows the typing rule for newly added functions.
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| yield mock_database | ||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_drop_table_blocked(self, ddl_mocks, mcp_server): |
There was a problem hiding this comment.
Suggestion: Add explicit type hints for all parameters and the return type on this newly added async test method. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This added async test method has no type annotations for its parameters and no return type annotation. The suggestion correctly identifies a real typing omission in the new code.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/sql_lab/tool/test_execute_sql.py
**Line:** 1241:1241
**Comment:**
*Custom Rule: Add explicit type hints for all parameters and the return type on this newly added async test 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| ddl_mocks.execute.assert_not_called() | ||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_truncate_blocked(self, ddl_mocks, mcp_server): |
There was a problem hiding this comment.
Suggestion: Annotate this new async test method with explicit parameter types and a return type to satisfy the typing requirement. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The test method is newly introduced and untyped, with no parameter annotations and no return annotation. This is a genuine match for the stated typing requirement.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/sql_lab/tool/test_execute_sql.py
**Line:** 1255:1255
**Comment:**
*Custom Rule: Annotate this new async test method with explicit parameter types and a return type to satisfy the typing 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| ddl_mocks.execute.assert_not_called() | ||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_alter_table_blocked(self, ddl_mocks, mcp_server): |
There was a problem hiding this comment.
Suggestion: Add type annotations for each argument and the return type on this newly introduced async method. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
This new async test function lacks both parameter type hints and a return type. The suggestion is therefore addressing a real violation of the typing rule.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/sql_lab/tool/test_execute_sql.py
**Line:** 1268:1268
**Comment:**
*Custom Rule: Add type annotations for each argument and the return type on this newly introduced async 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| ddl_mocks.execute.assert_not_called() | ||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_drop_in_multi_statement_blocked(self, ddl_mocks, mcp_server): |
There was a problem hiding this comment.
Suggestion: Update this new async test method to include explicit parameter type hints and an annotated return type. [custom_rule]
Severity Level: Minor
Why it matters? 🤔
The method is newly added and untyped, with no annotations on its parameters or return value. This is a valid typing-related issue under the stated rule.
Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** tests/unit_tests/mcp_service/sql_lab/tool/test_execute_sql.py
**Line:** 1286:1286
**Comment:**
*Custom Rule: Update this new async test method to include explicit parameter type hints and an annotated return type.
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
Code Review Agent Run #a60d28Actionable 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 |
3 participants
SUMMARY
Root cause:
The MCP
execute_sqltool delegated all SQL validation to the executor's_check_security(), which only blocks mutations whendatabase.allow_dml=False. Whenallow_dml=True, destructive DDL (DROP TABLE, TRUNCATE, ALTER) passed through unchecked — the only protection was database-level permissions. If a user had DDL privileges on the database, MCP would happily executeDROP TABLE birth_names.Solution:
Added a fail-closed DDL pre-check in the MCP tool layer (not the executor, since SQL Lab admins may legitimately need DDL). Before the query reaches the executor:
SQLScriptand check for destructive DDL nodes (exp.Drop,exp.TruncateTable,exp.Alter)New methods
is_destructive_ddl()/has_destructive_ddl()onSQLStatement/SQLScriptdistinguish destructive DDL from safe DML (INSERT/UPDATE/DELETE), whichis_mutating()groups together.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION