mikebridge commented on code in PR #42014:
URL: https://github.com/apache/superset/pull/42014#discussion_r3574832273
##########
superset/mcp_service/sql_lab/tool/execute_sql.py:
##########
@@ -55,6 +55,52 @@
logger = logging.getLogger(__name__)
+async def _validate_non_destructive_sql(
+ request: ExecuteSqlRequest,
+ ctx: Context,
+ database: Any,
+ sql_preview: str,
+) -> 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 = request.sql
Review Comment:
Applied in 94b5abf24f: `sql_to_check` now has an explicit `str` annotation.
##########
superset/mcp_service/sql_lab/tool/execute_sql.py:
##########
@@ -122,44 +168,11 @@ async def execute_sql(request: ExecuteSqlRequest, ctx:
Context) -> ExecuteSqlRes
# 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.
- with event_logger.log_context(action="mcp.execute_sql.ddl_check"):
- try:
- sql_to_check = request.sql
- if request.template_params:
- 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
- )
-
- script = SQLScript(sql_to_check,
database.db_engine_spec.engine)
- if script.has_destructive():
- await ctx.error(
- "Destructive DDL blocked: sql_preview=%r" % sql_preview
- )
- return ExecuteSqlResponse(
- success=False,
- error=(
- "Destructive DDL statements (DROP, TRUNCATE,
ALTER) "
- "are not allowed through MCP. Use the Superset SQL
"
- "Lab UI for administrative database operations."
- ),
-
error_type=SupersetErrorType.DML_NOT_ALLOWED_ERROR.value,
- )
- except Exception as parse_err:
- 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,
- )
+ validation_error = await _validate_non_destructive_sql(
+ request, ctx, database, sql_preview
+ )
Review Comment:
Applied in 94b5abf24f: `validation_error` now has an explicit
`ExecuteSqlResponse | None` annotation.
##########
superset/mcp_service/sql_lab/tool/execute_sql.py:
##########
@@ -55,6 +55,52 @@
logger = logging.getLogger(__name__)
+async def _validate_non_destructive_sql(
+ request: ExecuteSqlRequest,
+ ctx: Context,
+ database: Any,
+ sql_preview: str,
+) -> 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 = request.sql
+ if request.template_params:
+ 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
+ )
+
+ script = SQLScript(sql_to_check, database.db_engine_spec.engine)
+ if script.has_destructive():
+ await ctx.error("Destructive DDL blocked: sql_preview=%r" %
sql_preview)
+ return ExecuteSqlResponse(
+ success=False,
+ error=(
+ "Destructive DDL statements (DROP, TRUNCATE, ALTER) "
+ "are not allowed through MCP. Use the Superset SQL "
+ "Lab UI for administrative database operations."
+ ),
+ error_type=SupersetErrorType.DML_NOT_ALLOWED_ERROR.value,
+ )
+ except Exception as parse_err:
+ await ctx.error(
+ "DDL pre-check failed to parse SQL, blocking query: %s" %
str(parse_err)
+ )
+ return ExecuteSqlResponse(
+ success=False,
Review Comment:
Thanks. I am retaining the broad catch intentionally because this is a
fail-closed security boundary: the guarded block includes optional Jinja
rendering, engine lookup, and SQL parsing, each of which can raise backend- or
extension-specific exceptions. Narrowing it to `SyntaxError`/`ValueError` would
let an unexpected validation failure escape instead of returning the documented
blocked-query response. The code was moved unchanged from `execute_sql`; this
PR does not broaden the existing exception policy.
--
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]
