fix(mcp): align execute_sql access check with SQL Lab UI#41196
Open
LaurinBrechter wants to merge 1 commit into
Open
fix(mcp): align execute_sql access check with SQL Lab UI#41196LaurinBrechter wants to merge 1 commit into
LaurinBrechter wants to merge 1 commit into
Conversation
The MCP execute_sql tool denied queries based solely on can_access_database(), rejecting users who lack the blanket database_access permission but who own or have datasource_access on every dataset referenced by the query. The SQL Lab web UI allows these users through via security_manager.raise_for_access(..., force_dataset_match=True), so the same query could fail via MCP while succeeding in the UI for the same user. Use raise_for_access in the MCP tool to match the UI's access semantics. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
dosubot
Bot
added
authentication:access-control
Contributor
Code Review Agent Run #70f50aActionable 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 |
✅ Deploy Preview for superset-docs-preview ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![codeant-ai-for-open-source[bot]](https://avatars.githubusercontent.com/u/151810221?s=60&v=4){kind=small}
Comment on lines
+121
to
+130
| try: | ||
| security_manager.raise_for_access( | ||
| database=database, | ||
| sql=request.sql, | ||
| schema=request.schema_name, | ||
| catalog=request.catalog, | ||
| template_params=request.template_params, | ||
| force_dataset_match=True, | ||
| ) | ||
| except SupersetSecurityException as security_exc: |
Contributor
There was a problem hiding this comment.
Suggestion: The new access pre-check only handles SupersetSecurityException, but raise_for_access(...) can also raise non-security exceptions during SQL/Jinja parsing (for example template/parsing failures). Those exceptions now bypass this block and hit the outer generic except Exception path, which re-raises and turns a user input error into an unhandled tool failure. Catch and map the known access-check parse/template exceptions to a structured ExecuteSqlResponse (as done for other validation failures) instead of letting them escape. [api mismatch]
Severity Level: Major ⚠️
- ❌ MCP execute_sql crashes on malformed Jinja-templated SQL.
- ⚠️ SQL exploration via MCP returns opaque tool failures.
- ⚠️ Inconsistent error handling vs DDL parse validation.Steps of Reproduction ✅
1. Invoke the MCP `execute_sql` tool as documented in `superset/mcp_service/app.py:81-99`
(e.g. via an MCP client) using the `execute_sql(request={{\"database_id\": <id>, \"sql\":
\"SELECT {{ foo }}\"}})` workflow.
2. Ensure the target database exists and is accessible, then call `execute_sql()` in
`superset/mcp_service/sql_lab/tool/execute_sql.py:72-138` with `request.sql` containing
invalid or unrenderable Jinja (e.g. `SELECT {{ missing_var }}`) and
`request.template_params={}` or mismatched params.
3. Inside `execute_sql`, the code enters the DB validation block at
`execute_sql.py:97-139` and calls `security_manager.raise_for_access(database=database,
sql=request.sql, schema=request.schema_name, catalog=request.catalog,
template_params=request.template_params, force_dataset_match=True)` within the inner
`try:` at lines 121-129.
4. `SupersetSecurityManager.raise_for_access` in `superset/security/manager.py:3115-3225`
constructs a SQL Lab `Query` and calls `process_jinja_sql(sql_for_parse, database,
parse_template_params)` at `manager.py:135-139`, which is implemented in
`superset/sql/parse.py:1744-69` and documented to raise `jinja2.exceptions.TemplateError`
when the Jinja SQL cannot be rendered.
5. When `process_jinja_sql` raises `TemplateError` (or another
non-`SupersetSecurityException` parsing error), this exception is not caught by the inner
`except SupersetSecurityException as security_exc:` at `execute_sql.py:130-138`, so it
bubbles up to the outer `except Exception as e:` at `execute_sql.py:256-259`, which logs
`"SQL execution failed"` and then re-raises, causing the MCP tool call to fail with an
unhandled server error instead of returning a structured `ExecuteSqlResponse` like the DDL
parse block does at `execute_sql.py:145-181`.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** superset/mcp_service/sql_lab/tool/execute_sql.py
**Line:** 121:130
**Comment:**
*Api Mismatch: The new access pre-check only handles `SupersetSecurityException`, but `raise_for_access(...)` can also raise non-security exceptions during SQL/Jinja parsing (for example template/parsing failures). Those exceptions now bypass this block and hit the outer generic `except Exception` path, which re-raises and turns a user input error into an unhandled tool failure. Catch and map the known access-check parse/template exceptions to a structured `ExecuteSqlResponse` (as done for other validation failures) instead of letting them escape.
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
Contributor
|
The flagged issue is correct. The current implementation only catches Here is the suggested fix for try:
security_manager.raise_for_access(
database=database,
sql=request.sql,
schema=request.schema_name,
catalog=request.catalog,
template_params=request.template_params,
force_dataset_match=True,
)
except (SupersetSecurityException, Exception) as exc:
# Map security or parsing/template errors to a structured response
error_msg = str(exc) if not isinstance(exc, SupersetSecurityException) else exc.error.message
return ExecuteSqlResponse(
success=False,
error=error_msg,
error_type=SupersetErrorType.DATABASE_SECURITY_ACCESS_ERROR.value,
)There are no other comments on this PR to address. Would you like me to implement this change? superset/mcp_service/sql_lab/tool/execute_sql.py |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SUMMARY
The MCP
execute_sqltool gated access purely onsecurity_manager.can_access_database(), which only passes for users with the blanketdatabase_accesspermission (orall_database_access/all_datasource_access). The SQL Lab web UI uses a richer check —security_manager.raise_for_access(..., force_dataset_match=True)— which additionally allows a user through if they own, or havedatasource_accesson, every Superset dataset referenced by the query, even without the blanket database-level grant.This meant the same user, running the same query against the same database, could succeed in the SQL Lab UI but get
Access denied to database Xthrough the MCPexecute_sqltool — purely because the MCP tool used a narrower check than the UI.This PR switches the MCP tool's access check to call
raise_for_access(database=..., sql=..., schema=..., catalog=..., template_params=..., force_dataset_match=True), matchingCanAccessQueryValidatorImpl(the validator the SQL Lab UI command path uses), so MCP and the UI enforce the same access semantics for the same user.RELATED WORK
This was found while investigating the false-positive disallowed-function bug fixed in #40963 (different bug, same
execute_sqlcode path) — debugging a real-world MCP query failure against a StarRocks-backed database surfaced this separate, pre-existing access-check inconsistency between MCP and the SQL Lab UI.FOLLOW-UP (not in this PR)
The unified
Database.execute()API (superset/sql/execution/executor.py,QueryExecutor._check_security()) that MCP'sexecute_sqlcalls into has no equivalent ofraise_for_access/dataset-fallback access check at all — it only checks disallowed functions/tables and DML permission. This PR only patches the MCP tool's own pre-check, which closes the immediate bug, but any other future caller of the unifieddatabase.execute()API (other MCP tools, new REST endpoints, etc.) could hit the same gap. Moving theraise_for_accesscall into the shared executor itself, so every caller of the unified API gets the same security semantics as SQL Lab automatically, would be a more general fix — but it's a larger, more invasive change to shared security-sensitive code and is left as a separate follow-up.TESTING INSTRUCTIONS
tests/unit_tests/mcp_service/sql_lab/tool/test_execute_sql.py::test_execute_sql_access_deniedto mockraise_for_accessraisingSupersetSecurityExceptioninstead ofcan_access_databasereturningFalse.datasource_access(or ownership) on a dataset in a schema, but withhold blanketdatabase_accesson the parent database connection. Run the dataset's query via SQL Lab UI (succeeds) and via MCPexecute_sql(previously failed with "Access denied to database X", now succeeds).ADDITIONAL INFORMATION