This is an automated email from the ASF dual-hosted git repository.
sha174n pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/superset.git
The following commit(s) were added to refs/heads/master by this push:
new 7b83ed83746 fix(mcp): align execute_sql table-access checks with the
SQL Lab path (#43998)
7b83ed83746 is described below
commit 7b83ed83746f64e1ad61b28b7340a59269dc54b3
Author: Shaitan <[email protected]>
AuthorDate: Wed Sep 9 14:02:35 2026 +0100
fix(mcp): align execute_sql table-access checks with the SQL Lab path
(#43998)
Co-authored-by: Claude Opus 4.8 <[email protected]>
---
superset/mcp_service/sql_lab/tool/execute_sql.py | 101 +++++++----
.../mcp_service/sql_lab/tool/test_execute_sql.py | 186 +++++++++++++++++----
2 files changed, 228 insertions(+), 59 deletions(-)
diff --git a/superset/mcp_service/sql_lab/tool/execute_sql.py
b/superset/mcp_service/sql_lab/tool/execute_sql.py
index e79fbe8e930..4e5d980c765 100644
--- a/superset/mcp_service/sql_lab/tool/execute_sql.py
+++ b/superset/mcp_service/sql_lab/tool/execute_sql.py
@@ -28,6 +28,7 @@ from typing import Any
import pandas as pd
from fastmcp import Context
+from jinja2.exceptions import TemplateError
from superset_core.mcp.decorators import tool, ToolAnnotations
from superset_core.queries.types import (
CacheOptions,
@@ -37,7 +38,13 @@ from superset_core.queries.types import (
)
from superset.errors import SupersetErrorType
-from superset.exceptions import OAuth2Error, OAuth2RedirectError
+from superset.exceptions import (
+ OAuth2Error,
+ OAuth2RedirectError,
+ SupersetParseError,
+ SupersetSecurityException,
+ SupersetTemplateException,
+)
from superset.extensions import event_logger
from superset.mcp_service.sql_lab.schemas import (
ColumnInfo,
@@ -55,28 +62,39 @@ from superset.sql.parse import SQLScript
logger = logging.getLogger(__name__)
+def _invalid_sql_response() -> ExecuteSqlResponse:
+ """Response for SQL that could not be rendered or parsed for validation."""
+ return ExecuteSqlResponse(
+ success=False,
+ error=(
+ "SQL could not be parsed for security validation. "
+ "Please check your SQL syntax and try again."
+ ),
+ error_type=SupersetErrorType.INVALID_SQL_ERROR.value,
+ )
+
+
async def _validate_non_destructive_sql(
request: ExecuteSqlRequest,
ctx: Context,
database: Any,
sql_preview: str,
+ template_params: dict[str, Any],
) -> ExecuteSqlResponse | None:
"""Return an error response when SQL cannot safely be executed."""
with event_logger.log_context(action="mcp.execute_sql.ddl_check"):
try:
- sql_to_check: str = request.sql
- # Render whenever template_params is not None, mirroring the
- # executor (SQLExecutor._render_sql_template), which also renders
- # for an empty dict. A truthiness check would let destructive SQL
- # that only appears after rendering slip past the guard when
- # template_params={}.
- if request.template_params is not None:
- from superset.jinja_context import get_template_processor
-
- tp = get_template_processor(database=database)
- sql_to_check = tp.process_template(
- request.sql, **request.template_params
- )
+ # Render the same way the executor does
+ # (``SQLExecutor._render_sql_template`` -> ``process_template``) so
+ # this guard inspects the string that will actually run and
+ # destructive SQL that only appears after rendering cannot slip
+ # past it. Deliberately not ``process_jinja_sql``, which
+ # neutralizes partition macros for parsing and so yields a
+ # different string.
+ from superset.jinja_context import get_template_processor
+
+ tp = get_template_processor(database=database)
+ sql_to_check = tp.process_template(request.sql, **template_params)
script = SQLScript(sql_to_check, database.db_engine_spec.engine)
if script.has_destructive():
@@ -94,14 +112,7 @@ async def _validate_non_destructive_sql(
await ctx.error(
"DDL pre-check failed to parse SQL, blocking query: %s" %
str(parse_err)
)
- return ExecuteSqlResponse(
- success=False,
- error=(
- "SQL could not be parsed for security validation. "
- "Please check your SQL syntax and try again."
- ),
- error_type=SupersetErrorType.INVALID_SQL_ERROR.value,
- )
+ return _invalid_sql_response()
return None
@@ -143,6 +154,15 @@ async def execute_sql(request: ExecuteSqlRequest, ctx:
Context) -> ExecuteSqlRes
from superset import db, is_feature_enabled, security_manager
from superset.models.core import Database
+ # The access check below renders unconditionally
+ # (``raise_for_access`` -> ``process_jinja_sql``), so execution has to
+ # render too, otherwise the authorized SQL is not the SQL that runs and
+ # a template that hides a table reference from the renderer (e.g. one
+ # wrapped in ``{% if 0 %}`` inside a comment) would be authorized in
+ # its rendered form and executed in its raw form. SQL Lab has no such
+ # gap because its ``template_params`` defaults to ``{}``; match that.
+ template_params = request.template_params or {}
+
# 1. Get database and check access
with event_logger.log_context(action="mcp.execute_sql.db_validation"):
database = (
@@ -161,24 +181,43 @@ async def execute_sql(request: ExecuteSqlRequest, ctx:
Context) -> ExecuteSqlRes
error_type=SupersetErrorType.DATABASE_NOT_FOUND_ERROR.value,
)
- if not security_manager.can_access_database(database):
+ # Authorize through the same entry point as the SQL Lab
+ # execution path (``superset/sqllab/validators.py``), so both
+ # surfaces scope a query the same way: it covers database-level
+ # access and, for a user without it, requires every table the
+ # query references to resolve to a dataset they are granted.
+ try:
+ security_manager.raise_for_access(
+ database=database,
+ sql=request.sql,
+ catalog=request.catalog,
+ schema=request.schema_name,
+ template_params=template_params,
+ force_dataset_match=True,
+ )
+ except SupersetSecurityException as ex:
await ctx.warning(
- "Access denied to database: %s" % database.database_name
+ "Access denied for query on database: %s" %
database.database_name
)
return ExecuteSqlResponse(
success=False,
- error=f"Access denied to database
{database.database_name}",
-
error_type=SupersetErrorType.DATABASE_SECURITY_ACCESS_ERROR.value,
+ error=ex.error.message,
+ error_type=ex.error.error_type.value,
)
+ except (SupersetParseError, SupersetTemplateException,
TemplateError):
+ # Authorising a query the user has no database-wide grant on
+ # means rendering and parsing it, so malformed Jinja or SQL can
+ # surface here rather than in the DDL pre-check below.
+ await ctx.error("Query could not be parsed for access
validation")
+ return _invalid_sql_response()
# 2. Block destructive DDL (DROP, TRUNCATE, ALTER)
# Fail-closed: if parsing fails, block the query rather than
# allowing potentially destructive SQL to bypass the check.
# Render Jinja2 templates first so templated SQL can be parsed.
- validation_error: (
- ExecuteSqlResponse | None
- ) = await _validate_non_destructive_sql(request, ctx, database,
sql_preview)
- if validation_error is not None:
+ if validation_error := await _validate_non_destructive_sql(
+ request, ctx, database, sql_preview, template_params
+ ):
return validation_error
# 3. Build QueryOptions and execute query
@@ -188,7 +227,7 @@ async def execute_sql(request: ExecuteSqlRequest, ctx:
Context) -> ExecuteSqlRes
schema=request.schema_name,
limit=request.limit,
timeout_seconds=request.timeout,
- template_params=request.template_params,
+ template_params=template_params,
dry_run=request.dry_run,
cache=cache_opts,
)
diff --git a/tests/unit_tests/mcp_service/sql_lab/tool/test_execute_sql.py
b/tests/unit_tests/mcp_service/sql_lab/tool/test_execute_sql.py
index 8116b5601fc..b88ff947139 100644
--- a/tests/unit_tests/mcp_service/sql_lab/tool/test_execute_sql.py
+++ b/tests/unit_tests/mcp_service/sql_lab/tool/test_execute_sql.py
@@ -31,8 +31,15 @@ import pandas as pd
import pytest
from fastmcp import Client
from fastmcp.exceptions import ToolError
+from jinja2.exceptions import TemplateSyntaxError
from superset_core.queries.types import QueryResult, QueryStatus,
StatementResult
+from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
+from superset.exceptions import (
+ SupersetParseError,
+ SupersetSecurityException,
+ SupersetTemplateException,
+)
from superset.mcp_service.app import mcp
from superset.mcp_service.sql_lab.schemas import ColumnInfo
@@ -152,7 +159,6 @@ class TestExecuteSql:
mock_db.session.query.return_value.filter_by.return_value.first.return_value = (
mock_database
)
- mock_security_manager.can_access_database.return_value = True
request = {
"database_id": 1,
@@ -201,7 +207,6 @@ class TestExecuteSql:
mock_db.session.query.return_value.filter_by.return_value.first.return_value = (
mock_database
)
- mock_security_manager.can_access_database.return_value = True
request = {
"database_id": 1,
@@ -247,7 +252,6 @@ class TestExecuteSql:
mock_db.session.query.return_value.filter_by.return_value.first.return_value = (
mock_database
)
- mock_security_manager.can_access_database.return_value = True
mock_is_feature_enabled.return_value = False
request = {
@@ -283,7 +287,6 @@ class TestExecuteSql:
mock_db.session.query.return_value.filter_by.return_value.first.return_value = (
mock_database
)
- mock_security_manager.can_access_database.return_value = True
mock_is_feature_enabled.return_value = True
request = {
@@ -315,7 +318,6 @@ class TestExecuteSql:
mock_db.session.query.return_value.filter_by.return_value.first.return_value = (
mock_database
)
- mock_security_manager.can_access_database.return_value = True
mock_is_feature_enabled.return_value = False
request = {"database_id": 1, "sql": "SELECT id FROM users"}
@@ -356,30 +358,148 @@ class TestExecuteSql:
assert data["success"] is False
assert "Database with ID 999 not found" in data["error"]
+ # ``new_callable=MagicMock`` is required here: ``security_manager`` is a
+ # LocalProxy, which a bare ``patch`` replaces with an AsyncMock whose
+ # ``side_effect`` fires only when awaited. ``raise_for_access`` is called
+ # synchronously, so the deny path would never be exercised.
@patch("superset.security_manager", new_callable=MagicMock)
@patch("superset.db")
@pytest.mark.asyncio
+ @pytest.mark.parametrize(
+ "error_type, message",
+ [
+ (
+ SupersetErrorType.DATABASE_SECURITY_ACCESS_ERROR,
+ "You need access to the following database: examples",
+ ),
+ (
+ SupersetErrorType.TABLE_SECURITY_ACCESS_ERROR,
+ "You need access to the following tables: secret_table",
+ ),
+ ],
+ )
async def test_execute_sql_access_denied(
- self, mock_db, mock_security_manager, mcp_server
+ self, mock_db, mock_security_manager, mcp_server, error_type, message
):
- """Test error when user lacks database access."""
+ """A query the user is not authorized to run, at either the database or
+ the table level, is rejected and never executed."""
mock_database = _mock_database()
mock_db.session.query.return_value.filter_by.return_value.first.return_value = (
mock_database
)
- mock_security_manager.can_access_database.return_value = False
+ mock_security_manager.raise_for_access.side_effect =
SupersetSecurityException(
+ SupersetError(
+ message=message,
+ error_type=error_type,
+ level=ErrorLevel.ERROR,
+ )
+ )
request = {
"database_id": 1,
- "sql": "SELECT 1",
- "limit": 1,
+ "sql": "SELECT * FROM secret_table",
+ "limit": 10,
+ }
+
+ async with Client(mcp_server) as client:
+ result = await client.call_tool("execute_sql", {"request":
request})
+ data = result.structured_content
+ assert data["success"] is False
+ assert data["error"] == message
+ assert data["error_type"] == error_type.value
+
+ mock_security_manager.raise_for_access.assert_called_once_with(
+ database=mock_database,
+ sql="SELECT * FROM secret_table",
+ catalog=None,
+ schema=None,
+ template_params={},
+ force_dataset_match=True,
+ )
+ mock_database.execute.assert_not_called()
+
+ @patch("superset.security_manager", new_callable=MagicMock)
+ @patch("superset.db")
+ @pytest.mark.asyncio
+ @pytest.mark.parametrize("template_params", [None, {}, {"a": 1}])
+ async def test_execute_sql_authorizes_the_sql_that_executes(
+ self, mock_db, mock_security_manager, mcp_server, template_params
+ ):
+ """The access check and the executor must be handed the same
+ template_params, otherwise the authorized SQL is not the SQL that runs
+ and Jinja expanding on only one side escapes the table-access check.
+
+ ``None`` is normalized to ``{}`` rather than passed through: the check
+ renders unconditionally, so leaving the executor unrendered would let a
+ template that hides a table from the renderer be authorized in its
+ rendered form and executed in its raw form.
+ """
+ mock_database = _mock_database()
+ mock_database.execute.return_value = _create_select_result(
+ rows=[{"id": 1}],
+ columns=["id"],
+ )
+
mock_db.session.query.return_value.filter_by.return_value.first.return_value = (
+ mock_database
+ )
+
+ request: dict[str, Any] = {
+ "database_id": 1,
+ "sql": "SELECT id FROM users",
+ "limit": 10,
+ }
+ if template_params is not None:
+ request["template_params"] = template_params
+
+ async with Client(mcp_server) as client:
+ result = await client.call_tool("execute_sql", {"request":
request})
+ assert result.structured_content["success"] is True
+
+ authorized = mock_security_manager.raise_for_access.call_args.kwargs[
+ "template_params"
+ ]
+ executed = mock_database.execute.call_args[0][1].template_params
+ assert authorized == executed
+ assert executed == (template_params or {})
+
+ @patch("superset.security_manager", new_callable=MagicMock)
+ @patch("superset.db")
+ @pytest.mark.asyncio
+ @pytest.mark.parametrize(
+ "side_effect",
+ [
+ TemplateSyntaxError("unexpected end of template", 1),
+ # raised by macros such as ``metric()`` while rendering; a
+ # SupersetException rather than a jinja2 one, so it needs
+ # catching separately to avoid surfacing as a crash
+ SupersetTemplateException("Please specify the Dataset ID"),
+ SupersetParseError("SELECT", "postgresql", message="cannot parse"),
+ ],
+ )
+ async def test_execute_sql_malformed_template(
+ self, mock_db, mock_security_manager, mcp_server, side_effect
+ ):
+ """Malformed Jinja is reported as invalid SQL, not as a crash: the
+ access check renders the template, so it fails there first."""
+ mock_database = _mock_database()
+
mock_db.session.query.return_value.filter_by.return_value.first.return_value = (
+ mock_database
+ )
+ mock_security_manager.raise_for_access.side_effect = side_effect
+
+ request = {
+ "database_id": 1,
+ "sql": "SELECT * FROM {{ table",
+ "limit": 10,
}
async with Client(mcp_server) as client:
result = await client.call_tool("execute_sql", {"request":
request})
data = result.structured_content
assert data["success"] is False
- assert "Access denied to database" in data["error"]
+ assert data["error_type"] ==
SupersetErrorType.INVALID_SQL_ERROR.value
+
+ mock_database.execute.assert_not_called()
@patch("superset.security_manager")
@patch("superset.db")
@@ -397,7 +517,6 @@ class TestExecuteSql:
mock_db.session.query.return_value.filter_by.return_value.first.return_value = (
mock_database
)
- mock_security_manager.can_access_database.return_value = True
request = {
"database_id": 1,
@@ -431,7 +550,6 @@ class TestExecuteSql:
mock_db.session.query.return_value.filter_by.return_value.first.return_value = (
mock_database
)
- mock_security_manager.can_access_database.return_value = True
request = {
"database_id": 1,
@@ -465,7 +583,6 @@ class TestExecuteSql:
mock_db.session.query.return_value.filter_by.return_value.first.return_value = (
mock_database
)
- mock_security_manager.can_access_database.return_value = True
request = {
"database_id": 1,
@@ -515,7 +632,6 @@ class TestExecuteSql:
mock_db.session.query.return_value.filter_by.return_value.first.return_value = (
mock_database
)
- mock_security_manager.can_access_database.return_value = True
request = {
"database_id": 1,
@@ -560,7 +676,6 @@ class TestExecuteSql:
mock_db.session.query.return_value.filter_by.return_value.first.return_value = (
mock_database
)
- mock_security_manager.can_access_database.return_value = True
request = {
"database_id": 1,
@@ -611,7 +726,6 @@ class TestExecuteSql:
mock_db.session.query.return_value.filter_by.return_value.first.return_value = (
mock_database
)
- mock_security_manager.can_access_database.return_value = True
request = {
"database_id": 1,
@@ -694,7 +808,6 @@ class TestExecuteSql:
mock_db.session.query.return_value.filter_by.return_value.first.return_value = (
mock_database
)
- mock_security_manager.can_access_database.return_value = True
request = {
"database_id": 1,
@@ -774,7 +887,6 @@ class TestExecuteSql:
mock_db.session.query.return_value.filter_by.return_value.first.return_value = (
mock_database
)
- mock_security_manager.can_access_database.return_value = True
request = {
"database_id": 1,
@@ -837,7 +949,6 @@ class TestExecuteSql:
mock_db.session.query.return_value.filter_by.return_value.first.return_value = (
mock_database
)
- mock_security_manager.can_access_database.return_value = True
request = {
"database_id": 1,
@@ -934,7 +1045,6 @@ class TestExecuteSql:
mock_db.session.query.return_value.filter_by.return_value.first.return_value = (
mock_database
)
- mock_security_manager.can_access_database.return_value = True
# No 'limit' key — should default to None (no override)
request = {
@@ -968,7 +1078,6 @@ class TestExecuteSql:
mock_db.session.query.return_value.filter_by.return_value.first.return_value = (
mock_database
)
- mock_security_manager.can_access_database.return_value = True
request = {
"database_id": 1,
@@ -1029,7 +1138,6 @@ class TestExecuteSql:
mock_db.session.query.return_value.filter_by.return_value.first.return_value = (
mock_database
)
- mock_security_manager.can_access_database.return_value = True
request = {
"database_id": 1,
@@ -1088,7 +1196,6 @@ class TestExecuteSql:
mock_db.session.query.return_value.filter_by.return_value.first.return_value = (
mock_database
)
- mock_security_manager.can_access_database.return_value = True
request = {
"database_id": 1,
@@ -1216,7 +1323,6 @@ class TestExecuteSqlOAuth2:
mock_db.session.query.return_value.filter_by.return_value.first.return_value = (
mock_database
)
- mock_security_manager.can_access_database.return_value = True
request = {
"database_id": 1,
@@ -1249,7 +1355,6 @@ class TestExecuteSqlOAuth2:
mock_db.session.query.return_value.filter_by.return_value.first.return_value = (
mock_database
)
- mock_security_manager.can_access_database.return_value = True
request = {
"database_id": 1,
@@ -1327,13 +1432,12 @@ class TestDestructiveDDLBlocking:
"""Common mock wiring for DDL blocking tests."""
with (
patch("superset.db") as mock_db,
- patch("superset.security_manager") as mock_sm,
+ patch("superset.security_manager"),
):
mock_database = _mock_database()
mock_database.db_engine_spec.engine = "postgresql"
query_chain = mock_db.session.query.return_value
query_chain.filter_by.return_value.first.return_value =
mock_database
- mock_sm.can_access_database.return_value = True
yield mock_database
@pytest.mark.asyncio
@@ -1445,6 +1549,32 @@ class TestDestructiveDDLBlocking:
mock_tp.process_template.assert_called_once()
ddl_mocks.execute.assert_not_called()
+ @pytest.mark.asyncio
+ async def test_no_template_params_still_renders(self, ddl_mocks,
mcp_server):
+ """Omitting template_params must not skip rendering. The access check
+ renders unconditionally, so the guard and the executor have to render
+ as well, otherwise they inspect and run a different string than the one
+ that was authorized.
+ """
+ sql = "SELECT * FROM logs WHERE msg = 'x'"
+ ddl_mocks.execute.return_value = _create_select_result(
+ rows=[{"msg": "x"}], columns=["msg"], original_sql=sql
+ )
+
+ with patch("superset.jinja_context.get_template_processor") as
mock_get_tp:
+ mock_get_tp.return_value.process_template.return_value = sql
+ async with Client(mcp_server) as client:
+ result = await client.call_tool(
+ "execute_sql",
+ {"request": {"database_id": 1, "sql": sql}},
+ )
+
+ data = result.structured_content
+ assert data["success"] is True
+ mock_get_tp.return_value.process_template.assert_called_once_with(sql)
+ # the executor renders the same way, so it is handed {} rather than
None
+ assert ddl_mocks.execute.call_args[0][1].template_params == {}
+
@pytest.mark.asyncio
async def test_select_allowed(self, ddl_mocks, mcp_server):
"""SELECT queries pass through the DDL check."""
