fix(mcp): let SQL Server scripts past the execution gate and keep GO lines out of single statements - #3124
Merged
Conversation
…lines out of single statements
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Automations to automatically generate PRs for you. |
This branch was successfully deployed
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
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.
Follow-up to #3123, which was merged before review finished. Two defects a reviewer found in it, both confirmed.
1. SQL Server scripts were still refused on MCP, AppleScript and the assistant
#3123 let a SQL Server script past the first gate,
ExternalStatementGate.classify, by passingallowsMultiStatement. Every caller then runs the second gate,DefaultExecutionGate. It counts statements withQueryClassifier.isMultiStatementand denies the text with "Multiple statements are not permitted for this client" when the caller lacks.mayRunMultiStatement. That check runs before any Safe Mode level, so it applied at Silent too. No caller passed the capability:execute_query:MCPStatementGatebuilt[.mayWrite]plus the destructive and consent flags.run query:ScriptQueryRunnerpassed[.mayWrite, .mayRunDestructive].execute_query:context.writeCapabilities.So only a script that scans as one statement ran. The #3078 script (
DECLARE ...;and fourSELECT ...;) andSELECT 1 AS a/GO/SELECT 2 AS bwere refused. The #3123 tests called the bridge and the executor directly and never reached the second gate.Change. The fact that lets a script past
classifynow also grants.mayRunMultiStatement, so the two gates cannot disagree:MCPStatementGate.authorizeinserts it whenallowsMultiStatementis true, next to the existingallowsDestructiveinsert.ExternalStatementGate.capabilities(_:takingScriptsOn:)adds it whereacceptsScripts(on:)holds.ScriptQueryRunner.capabilities(on:)builds the script's set from it, andrunfeeds that one set to bothclassifyandauthorizeExecution.ExecuteQueryChatTooldoes the same withcontext.writeCapabilities.Nothing else loosens.
DefaultExecutionGatestill tiers the whole text by its worst statement, and a caller that does not take scripts still gets no capability.2. A leading
GOline still reached the server, andGO nstill ran oncerunStatementsendsSQLStatementScanner.executableText, which kept everything from the start of the text. #3123 routedexecute_queryaround it and left the other tools that send one statement:confirm_destructive_operation(MCP and assistant),explain_queryandexport_data. AGOline alone is a segment with no statement, so both gates countedGO\nDROP TABLE tas one statement and passed it.Measured against Azure SQL Edge 15 with a db-lib probe, one
dbcmdper text as the driver sends it:GO\nDROP TABLE dbo.staledbsqlexecFAILOBJECT_IDNULL)DROP TABLE dbo.staleSo
confirm_destructive_operationdropped the table and then reported a failure.Change.
executableTextnow starts where the first statement starts, as it already ended where the last one ends. A leadingGOline, a leading Oracle/line or a stray leading;no longer reaches the driver. Comments before the first statement stay: a probe over 12 inputs on SQL Server, PostgreSQL, MySQL and Oracle changed only those leading separators.QueryClassifier.isMultiStatementalso counts a statement that aGO nline runs more than once. Every gate for a caller that does not take scripts now refusesTRUNCATE TABLE t\nGO 2before anyone is prompted, with "Send one statement at a time." Before, the count was dropped and the statement ran once.execute_query,run queryand the assistant'sexecute_queryon SQL Server still honourGO n. The editor carries.mayRunMultiStatementand is unaffected.CHANGELOG: defect 1 completes the unreleased #3123 entry, so it gets no line of its own. Defect 2 was in 0.75.0 and gets a Fixed line.
docs/external-api/mcp-tools.mdxnow says aGO nstatement is refused outsideexecute_query.Tests
Each of these fails on main:
MCPStatementGateRefusalTests.sqlServerScriptClearsBothGates: the SQL Server batch variables are lost between statements #3078 script, a GO script andGO 3throughMCPStatementGate.authorizeand the real sharedDefaultExecutionGateat Silent. On main the second gate throwsforbidden.MCPScriptResultTests.executeQueryToolRunsTheScriptandchatToolRunsTheScript:ExecuteQueryTool.performandExecuteQueryChatTool.executeend to end, on an injected SQL Server session with a fake batch driver, for a;script and a GO script. Both gates run, and the payload has both result sets.ScriptingPolicyTests.sqlServerScriptClearsTheExecutionGate:ScriptQueryRunner.capabilities(on:)through a realDefaultExecutionGate. SQL Server scripts pass, andSELECT 1; SELECT 2on PostgreSQL is still denied.scriptsMayNotRunMultipleStatementsis now scoped to engines that do not take scripts.MCPScriptResultTests.leadingSeparatorIsNotSent:GO\nDROP TABLE dbo.staleon the statement path reaches the driver asDROP TABLE dbo.stale.SQLBatchSeparatorTests.leadingSeparatorIsNotSent(package) andSQLStatementPLSQLSplittingTests.executableTextDropsALeadingSlash.QueryClassifierMultiStatementTests.repeatedBatchIsMultiStatementandExternalStatementGateTests.repeatedStatementIsRefused. A bareGO,GO 1and aGO 3that ends an empty batch still count as one statement.ExternalStatementGateTests.scriptCapabilityFollowsTheEngine.What each red case rests on: removing the
.mayRunMultiStatementinsert turns the MCP and AppleScript cases red, passingcontext.writeCapabilitiesagain turns the assistant case red, startingexecutableTextat offset 0 turns the separator cases red, and dropping therepeatCountclause turns theGO ncases red. I did not run that mutation pass locally. The session's permission check blocked it as weakening a security gate. The first gate test fails on main from source reading: the shared gate returns.deniedandMCPAuthPolicythrowsforbidden.Verification
verify.sh build: PASS (aftergenerate, which picked up a file from fix(plugins): run SQL Server imports as GO batches and end each statement of a SQL Server dump with GO #3122).verify.sh teston the 6 edited suites: 97/97 PASS. Every argument of the parameterised cases ran, checked in the xcresult.verify.sh teston 33 neighbouring suites that own the changed types (QueryClassifier*,ExecutionGateTests,ExternalStatementGateLexicalTests,DatabaseAccessBridge*,ExportDataTool*,ConfirmDestructiveOperationToolTests,MCPStatementConsentGuardTests,ExecuteQueryToolTests, the chat tool suites and others): 495/495 PASS.swift test --filter TableProSQLGrammarTests: 86/86 PASS.verify.sh linton all 13 Swift files: 0 violations.verify.sh docs: PASS.check-ios-shared-isolation.py: OK.Not covered: no live run of the MCP server or AppleScript against SQL Server. The sandboxed Debug app does not start the MCP server without a Settings change in the UI, and AppleScript needs the Automation consent prompt. The server's side of the leading-
GOdefect is covered by the probe in the table. No driver code changed, soscripts/check-mssql-batch-results.shdoes not apply. No UI automation either: the flow needs a SQL Server connection, which the bundled Chinook SQLite sample cannot stand in for.Found while fixing #3078 (#3105).