From 01aed6c24b23d1087eeef4fd510ca324f799cf35 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Mon, 24 Aug 2026 14:12:03 +0000 Subject: [PATCH 1/2] feat: allow MCP admin CREATE/ALTER while blocking DROP and TRUNCATE Coding agents were stuck on MCP's read-only query context. Admins can now run DML and non-destructive DDL through execute_sql with the existing confirmation and WHERE gates. DROP and TRUNCATE stay blocked on MCP even when confirmed; the SQL Editor still only blocks DROP TABLE. Co-authored-by: Venkat SF --- CLAUDE.md | 5 +- agent/SOUL.md | 2 +- .../controller/ExplainController.java | 30 +++-- .../dbaagent/controller/SchemaController.java | 26 ++-- .../com/dbaagent/service/McpTokenService.java | 11 ++ .../service/QueryExecutionContext.java | 29 +++- .../service/QueryExecutionPolicyService.java | 45 +++++++ .../ExplainControllerPolicyTest.java | 27 ++++ .../dbaagent/service/McpTokenServiceTest.java | 9 ++ .../service/QueryExecutionContextTest.java | 29 ++++ .../QueryExecutionPolicyServiceTest.java | 124 ++++++++++++++++++ docs/public/cli-and-mcp.md | 6 +- docs/root/CLAUDE.md | 9 +- docs/root/MCP_PHASE1.md | 12 +- mcp/CLAUDE.md | 17 ++- mcp/README.md | 2 +- mcp/deepsql-phase1-lib.js | 14 +- mcp/deepsql-phase1-lib.test.js | 1 + mcp/deepsql-phase1-server.js | 2 +- mcp/package.json | 2 +- mcp/skills/SKILL_BODY.md | 7 +- mcp/src/cli.js | 6 +- mcp/src/commands/query.js | 6 +- 23 files changed, 358 insertions(+), 63 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index b729f63..bf3af60 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -245,9 +245,8 @@ their native runners. See `desktop/README.md` for the full picture. ## MCP Server - `mcp/deepsql-phase1-server.js` implements a Phase 1 stdio MCP server for internal rollout. -- It exposes read-only tools only: listing connections, fetching schema/objects, asking DeepSQL questions, executing read-only SQL, and running EXPLAIN without ANALYZE. -- It wraps existing backend APIs, so it reuses DeepSQL chat orchestration, RAG, connection management, and guardrails instead of exposing raw DB credentials. -- Read-only enforcement is applied in `mcp/deepsql-phase1-lib.js` before calling backend execution endpoints. +- Schema/retrieval tools stay read-only. `execute_sql` is role-gated: developers stay read-only; admins can run DML and non-destructive DDL (`CREATE`, `ALTER`) with the same two-step confirmation as the SQL Editor. `DROP` and `TRUNCATE` stay blocked on MCP even when confirmed. +- It wraps existing backend APIs, so it reuses DeepSQL chat orchestration, RAG, connection management, and `QueryExecutionPolicyService` instead of exposing raw DB credentials. - Client config examples live in `.cursor/mcp.json` and `mcp/claude_desktop_config.example.json`. - Usage and env vars are documented in `docs/root/MCP_PHASE1.md`. diff --git a/agent/SOUL.md b/agent/SOUL.md index 5e27f9b..ebc1fcb 100644 --- a/agent/SOUL.md +++ b/agent/SOUL.md @@ -16,7 +16,7 @@ After the answer you may offer **one short follow-up question** (a single line) 4. **Table-qualify every column** in generated SQL (`table.column`). Honor business rules and anti-patterns silently — if a rule says `always_filter_cancelled`, your query includes the filter without asking permission to follow the user's own rule. -5. **Read-only by default.** Developers cannot mutate; admins can with a **two-step confirmation**. If `execute_sql` returns `requiresConfirmation: true`, surface the warnings verbatim, get explicit human approval, then re-call with `confirmMutation: true`. NEVER auto-confirm — that defeats the safety gate. Never try to work around a 403/`EDITOR_MUTATION_FORBIDDEN`; surface it. +5. **Read-only by default.** Developers cannot mutate; admins can run DML and non-destructive DDL (`CREATE`, `ALTER`) with a **two-step confirmation**. `DROP` and `TRUNCATE` cannot be run via `execute_sql` — they stay blocked even after confirmation. If `execute_sql` returns `requiresConfirmation: true`, surface the warnings verbatim, get explicit human approval, then re-call with `confirmMutation: true`. NEVER auto-confirm — that defeats the safety gate. Never try to work around a 403/`EDITOR_MUTATION_FORBIDDEN` or an `UNSAFE_MUTATION_BLOCKED` DROP/TRUNCATE; surface it. 6. **One execution tool, one analysis tool.** Use `execute_sql` to run SQL; use `analyze_query_plan` for plans. Don't hand-wrap `EXPLAIN` inside `execute_sql`, and don't run a query just to see its plan. `EXPLAIN`/`EXPLAIN ANALYZE` are read-only SQL when you do need them — but `analyze_query_plan` gives the AI-enriched summary. diff --git a/backend/src/main/java/com/dbaagent/controller/ExplainController.java b/backend/src/main/java/com/dbaagent/controller/ExplainController.java index 7f38804..396f35c 100644 --- a/backend/src/main/java/com/dbaagent/controller/ExplainController.java +++ b/backend/src/main/java/com/dbaagent/controller/ExplainController.java @@ -10,6 +10,7 @@ import com.dbaagent.service.ClientContext; import com.dbaagent.service.CredentialService; import com.dbaagent.service.ExplainPlanService; +import com.dbaagent.service.McpTokenService; import com.dbaagent.service.QueryExecutionContext; import com.dbaagent.service.QueryExecutionPolicyException; import com.dbaagent.service.QueryExecutionPolicyService; @@ -19,6 +20,7 @@ import lombok.Data; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; +import org.springframework.http.HttpHeaders; import org.springframework.http.HttpStatus; import org.springframework.http.ResponseEntity; import org.springframework.jdbc.BadSqlGrammarException; @@ -38,9 +40,10 @@ * 1. `useAnalyze=true` actually runs the underlying statement inside * EXPLAIN ANALYZE — so for any mutating statement, that's a real * database write. We route those through QueryExecutionPolicyService - * with `QueryExecutionContext.editor(...)` so the same role/WHERE/ - * confirmation gates that protect /api/connections/{id}/query - * protect this path too. + * with `QueryExecutionContext.forSqlSurface(...)` so MCP bearers + * keep the MCP DROP/TRUNCATE block and Editor JWT callers keep the + * Editor DROP TABLE block. Role, WHERE, and confirmation gates that + * protect /api/connections/{id}/query protect this path too. * * 2. Every call — success, blocked, or failed — emits a SecurityEvent so * audit dashboards can see CLI/MCP/Editor traffic with one filter. @@ -71,7 +74,7 @@ public ResponseEntity analyzeQuery( ) { ClientContext client = ClientContext.fromRequest(httpRequest); String connectionId = request.getConnectionId(); - QueryRequest auditQueryRequest = buildAuditQueryRequest(request); + QueryRequest auditQueryRequest = buildAuditQueryRequest(request, httpRequest); ConnectionRequest connectionRequest = null; try { @@ -79,15 +82,19 @@ public ResponseEntity analyzeQuery( log.info("EXPLAIN analysis requested for connection: {} (useAnalyze={})", connectionId, request.isUseAnalyze()); // ANALYZE actually executes the SQL. Route the underlying - // statement through the same policy gate the SQL Editor uses so - // a developer can't bypass the mutation guard by sending - // useAnalyze=true with `DELETE FROM users`. + // statement through the same policy gate /connections/{id}/query + // uses so a developer can't bypass the mutation guard by sending + // useAnalyze=true with `DELETE FROM users`, and MCP callers keep + // the DROP/TRUNCATE block. if (request.isUseAnalyze()) { connectionRequest = credentialService.getDecryptedConnection(connectionId); String dbType = providerRegistry.getCanonicalName(connectionRequest.getDbType()); queryExecutionPolicyService.enforce( auditQueryRequest, - QueryExecutionContext.editor( + QueryExecutionContext.forSqlSurface( + McpTokenService.isMcpAuthorizationHeader( + httpRequest.getHeader(HttpHeaders.AUTHORIZATION) + ), accessControlService.getCurrentUsername(), accessControlService.isCurrentUserAdmin(), Boolean.TRUE.equals(request.getMutationConfirmed()) @@ -161,10 +168,13 @@ public ResponseEntity analyzeQuery( * Carries the user's mutationConfirmed flag through so admin-confirmed * ANALYZE runs aren't stuck on the confirmation gate. */ - private QueryRequest buildAuditQueryRequest(ExplainRequest request) { + private QueryRequest buildAuditQueryRequest(ExplainRequest request, HttpServletRequest httpRequest) { QueryRequest qr = new QueryRequest(); qr.setQuery(request.getQuery()); - qr.setExecutionOrigin(QueryExecutionOrigin.EDITOR); + boolean mcpBearer = McpTokenService.isMcpAuthorizationHeader( + httpRequest.getHeader(HttpHeaders.AUTHORIZATION) + ); + qr.setExecutionOrigin(mcpBearer ? QueryExecutionOrigin.MCP : QueryExecutionOrigin.EDITOR); qr.setMutationConfirmed(Boolean.TRUE.equals(request.getMutationConfirmed())); return qr; } diff --git a/backend/src/main/java/com/dbaagent/controller/SchemaController.java b/backend/src/main/java/com/dbaagent/controller/SchemaController.java index 12abb24..76f33e3 100644 --- a/backend/src/main/java/com/dbaagent/controller/SchemaController.java +++ b/backend/src/main/java/com/dbaagent/controller/SchemaController.java @@ -191,7 +191,12 @@ public ResponseEntity> executeQuery( return ResponseEntity.status(HttpStatus.NOT_FOUND).body(response); } accessControlService.assertCanUseChatEditor(connectionId); - queryRequest.setExecutionOrigin(QueryExecutionOrigin.EDITOR); + boolean mcpBearer = McpTokenService.isMcpAuthorizationHeader( + httpRequest.getHeader(HttpHeaders.AUTHORIZATION) + ); + queryRequest.setExecutionOrigin( + mcpBearer ? QueryExecutionOrigin.MCP : QueryExecutionOrigin.EDITOR + ); connectionRequest = credentialService.getDecryptedConnection(connectionId); QueryResult result = queryExecutorService.executeQuery( @@ -438,25 +443,14 @@ public ResponseEntity> getTableStats( } private QueryExecutionContext queryExecutionContext(QueryRequest queryRequest, HttpServletRequest httpRequest) { - String username = accessControlService.getCurrentUsername(); - boolean admin = accessControlService.isCurrentUserAdmin(); - if (isMcpBearer(httpRequest)) { - return QueryExecutionContext.mcp(username, admin); - } - return QueryExecutionContext.editor( - username, - admin, + return QueryExecutionContext.forSqlSurface( + McpTokenService.isMcpAuthorizationHeader(httpRequest.getHeader(HttpHeaders.AUTHORIZATION)), + accessControlService.getCurrentUsername(), + accessControlService.isCurrentUserAdmin(), Boolean.TRUE.equals(queryRequest.getMutationConfirmed()) ); } - private boolean isMcpBearer(HttpServletRequest httpRequest) { - String authorization = httpRequest.getHeader(HttpHeaders.AUTHORIZATION); - return authorization != null - && authorization.startsWith("Bearer ") - && authorization.substring(7).startsWith(McpTokenService.TOKEN_PREFIX); - } - private SchemaMetadata scopedSchema(String connectionId, SchemaMetadata schema) { return userDataAccessPolicyService.filterSchemaMetadata( connectionId, diff --git a/backend/src/main/java/com/dbaagent/service/McpTokenService.java b/backend/src/main/java/com/dbaagent/service/McpTokenService.java index e98194e..4a19bc1 100644 --- a/backend/src/main/java/com/dbaagent/service/McpTokenService.java +++ b/backend/src/main/java/com/dbaagent/service/McpTokenService.java @@ -174,6 +174,17 @@ public boolean looksLikeMcpToken(String rawToken) { return rawToken != null && rawToken.startsWith(TOKEN_PREFIX); } + /** + * True when the Authorization header is a DeepSQL MCP bearer token + * ({@code Bearer dsql_mcp_…}). JWT and other Bearer schemes return false. + */ + public static boolean isMcpAuthorizationHeader(String authorization) { + if (authorization == null || !authorization.regionMatches(true, 0, "Bearer ", 0, 7)) { + return false; + } + return authorization.substring(7).startsWith(TOKEN_PREFIX); + } + private boolean isExpired(McpToken token) { return token.getExpiresAt() != null && !token.getExpiresAt().isAfter(LocalDateTime.now()); } diff --git a/backend/src/main/java/com/dbaagent/service/QueryExecutionContext.java b/backend/src/main/java/com/dbaagent/service/QueryExecutionContext.java index c8ad685..a4f38b3 100644 --- a/backend/src/main/java/com/dbaagent/service/QueryExecutionContext.java +++ b/backend/src/main/java/com/dbaagent/service/QueryExecutionContext.java @@ -52,15 +52,40 @@ public static QueryExecutionContext mcp(String actorUsername) { } public static QueryExecutionContext mcp(String actorUsername, boolean actorIsAdmin) { + return mcp(actorUsername, actorIsAdmin, false); + } + + /** + * MCP / coding-agent SQL. Developers stay read-only. Admins may run + * non-destructive DDL/DML after the same confirmation gate as the Editor. + * DROP and TRUNCATE stay blocked in {@link QueryExecutionPolicyService}. + */ + public static QueryExecutionContext mcp( + String actorUsername, + boolean actorIsAdmin, + boolean mutationConfirmed + ) { return new QueryExecutionContext( QueryExecutionOrigin.MCP, - MutationMode.READ_ONLY_ONLY, + actorIsAdmin ? MutationMode.MAY_MUTATE : MutationMode.READ_ONLY_ONLY, actorUsername, actorIsAdmin, - false + mutationConfirmed ); } + public static QueryExecutionContext forSqlSurface( + boolean mcpBearer, + String actorUsername, + boolean actorIsAdmin, + boolean mutationConfirmed + ) { + if (mcpBearer) { + return mcp(actorUsername, actorIsAdmin, mutationConfirmed); + } + return editor(actorUsername, actorIsAdmin, mutationConfirmed); + } + public static QueryExecutionContext scheduled() { return new QueryExecutionContext( QueryExecutionOrigin.SCHEDULED, diff --git a/backend/src/main/java/com/dbaagent/service/QueryExecutionPolicyService.java b/backend/src/main/java/com/dbaagent/service/QueryExecutionPolicyService.java index ff7c1c7..4bd250c 100644 --- a/backend/src/main/java/com/dbaagent/service/QueryExecutionPolicyService.java +++ b/backend/src/main/java/com/dbaagent/service/QueryExecutionPolicyService.java @@ -55,6 +55,16 @@ public class QueryExecutionPolicyService { "^\\s*DROP\\s+(?:IF\\s+EXISTS\\s+)?TABLE\\b", Pattern.CASE_INSENSITIVE ); + // MCP/coding-agent loops may not run any DROP or TRUNCATE, including + // DROP INDEX / DROP VIEW. EXPLAIN wrappers are stripped before matching. + private static final Pattern DROP_OR_TRUNCATE_HEAD = Pattern.compile( + "^\\s*(DROP|TRUNCATE)\\b", + Pattern.CASE_INSENSITIVE + ); + private static final Pattern EXPLAIN_PREFIX_PATTERN = Pattern.compile( + "^\\s*EXPLAIN(?:\\s*\\([^)]*\\)|\\s+ANALYZE)?\\s+", + Pattern.CASE_INSENSITIVE + ); // Text-level backstops for writes that hide inside a statement which reads as // a SELECT. Applied to SQL with literals and comments already stripped. @@ -137,6 +147,15 @@ public PolicyDecision enforce( throw QueryExecutionPolicyException.editorMutationForbidden(mutation.queryType()); } + if (origin == QueryExecutionOrigin.MCP + && isDropOrTruncateStatement(mutation.queryType(), statements.getFirst())) { + throw QueryExecutionPolicyException.unsafeMutation( + "DROP and TRUNCATE are blocked on MCP and coding-agent loops. " + + "CREATE, ALTER, and DML still require admin privileges plus confirmation.", + mutation.queryType() + ); + } + if (origin == QueryExecutionOrigin.EDITOR && isDropTableStatement(mutation.queryType(), statements.getFirst())) { throw QueryExecutionPolicyException.unsafeMutation( @@ -204,6 +223,15 @@ private StatementClassification classifyStatement(String statement, QueryExecuti return new StatementClassification("SELECT", true, false, false, false, false); } if (parsed instanceof ExplainStatement) { + String wrappedMutation = detectExplainWrappedMutation(trimmed); + if (wrappedMutation != null) { + boolean requiresWhere = "UPDATE".equalsIgnoreCase(wrappedMutation) + || "DELETE".equalsIgnoreCase(wrappedMutation); + boolean hasWhere = !requiresWhere || containsWhereClause(trimmed); + return new StatementClassification( + wrappedMutation, false, true, requiresWhere, hasWhere, false + ); + } return new StatementClassification("EXPLAIN", true, false, false, false, false); } if (parsed instanceof UseStatement) { @@ -400,6 +428,23 @@ private boolean isUseStatement(String sql) { return sql != null && USE_ANY_PATTERN.matcher(stripLeadingComments(sql)).find(); } + /** + * True for any {@code DROP …} or {@code TRUNCATE …}, including EXPLAIN-wrapped forms. + * Used by the MCP origin gate so coding agents cannot confirm around destructive DDL. + */ + private boolean isDropOrTruncateStatement(String queryType, String sql) { + if (queryType != null) { + String upper = queryType.toUpperCase(Locale.ROOT); + if (upper.startsWith("DROP") || upper.startsWith("TRUNCATE")) { + return true; + } + } + String remaining = stripLeadingComments(sql == null ? "" : sql); + remaining = EXPLAIN_PREFIX_PATTERN.matcher(remaining).replaceFirst(""); + remaining = stripLeadingComments(remaining); + return DROP_OR_TRUNCATE_HEAD.matcher(remaining).find(); + } + /** * Returns true only for {@code DROP TABLE}. Other DROP variants (INDEX, VIEW, SEQUENCE, * SCHEMA, FUNCTION, PROCEDURE, …) are permitted as ordinary mutations for confirmed admins. diff --git a/backend/src/test/java/com/dbaagent/controller/ExplainControllerPolicyTest.java b/backend/src/test/java/com/dbaagent/controller/ExplainControllerPolicyTest.java index 570988a..b0fa8a1 100644 --- a/backend/src/test/java/com/dbaagent/controller/ExplainControllerPolicyTest.java +++ b/backend/src/test/java/com/dbaagent/controller/ExplainControllerPolicyTest.java @@ -6,6 +6,8 @@ import com.dbaagent.service.AnalysisHistoryService; import com.dbaagent.service.CredentialService; import com.dbaagent.service.ExplainPlanService; +import com.dbaagent.model.QueryExecutionOrigin; +import com.dbaagent.service.QueryExecutionContext; import com.dbaagent.service.QueryExecutionPolicyException; import com.dbaagent.service.QueryExecutionPolicyService; import com.dbaagent.service.SqlExecutionAuditService; @@ -14,8 +16,10 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.ArgumentCaptor; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; +import org.springframework.http.HttpHeaders; import org.springframework.http.HttpStatus; import org.springframework.http.ResponseEntity; @@ -128,6 +132,29 @@ void useAnalyzeTrue_confirmationRequired_propagatesRequiresConfirmation() { verify(explainPlanService, never()).analyzeQuery(anyString(), anyString(), anyBoolean()); } + @Test + void useAnalyzeTrue_mcpBearer_usesMcpExecutionContext() { + givenConnection("conn-1", "postgres"); + when(httpRequest.getHeader(HttpHeaders.AUTHORIZATION)) + .thenReturn("Bearer dsql_mcp_public.secret"); + when(accessControlService.getCurrentUsername()).thenReturn("admin"); + when(accessControlService.isCurrentUserAdmin()).thenReturn(true); + when(explainPlanService.analyzeQuery(eq("conn-1"), anyString(), eq(true))) + .thenReturn(new ExplainPlanAnalysis()); + + controller.analyzeQuery( + request("conn-1", "CREATE TABLE t_new (id INT PRIMARY KEY)", true), + httpRequest + ); + + ArgumentCaptor captor = ArgumentCaptor.forClass(QueryExecutionContext.class); + verify(queryExecutionPolicyService).enforce(any(), captor.capture(), eq("postgres")); + assertThat(captor.getValue().origin()).isEqualTo(QueryExecutionOrigin.MCP); + assertThat(captor.getValue().mutationMode()) + .isEqualTo(QueryExecutionContext.MutationMode.MAY_MUTATE); + assertThat(captor.getValue().actorIsAdmin()).isTrue(); + } + @Test void useAnalyzeFalse_skipsPolicyGate_butStillAudits() { // Plain EXPLAIN doesn't execute the query, so we don't need to gate diff --git a/backend/src/test/java/com/dbaagent/service/McpTokenServiceTest.java b/backend/src/test/java/com/dbaagent/service/McpTokenServiceTest.java index 9deed1f..a35f8f9 100644 --- a/backend/src/test/java/com/dbaagent/service/McpTokenServiceTest.java +++ b/backend/src/test/java/com/dbaagent/service/McpTokenServiceTest.java @@ -181,4 +181,13 @@ void listTokensUsesCurrentUserIdentity() { assertEquals(1, tokens.size()); assertEquals(1L, tokens.get(0).getId()); } + + @Test + void isMcpAuthorizationHeaderDetectsBearerPrefix() { + assertTrue(McpTokenService.isMcpAuthorizationHeader("Bearer dsql_mcp_abc.secret")); + assertFalse(McpTokenService.isMcpAuthorizationHeader("Bearer eyJhbGciOi")); + assertFalse(McpTokenService.isMcpAuthorizationHeader("dsql_mcp_abc.secret")); + assertFalse(McpTokenService.isMcpAuthorizationHeader(null)); + assertFalse(McpTokenService.isMcpAuthorizationHeader("")); + } } diff --git a/backend/src/test/java/com/dbaagent/service/QueryExecutionContextTest.java b/backend/src/test/java/com/dbaagent/service/QueryExecutionContextTest.java index 34c23d3..d471825 100644 --- a/backend/src/test/java/com/dbaagent/service/QueryExecutionContextTest.java +++ b/backend/src/test/java/com/dbaagent/service/QueryExecutionContextTest.java @@ -23,6 +23,35 @@ void mcpFactoryHonoursAdminFlagFromSecurityContext() { assertThat(ctx.origin()).isEqualTo(QueryExecutionOrigin.MCP); assertThat(ctx.actorUsername()).isEqualTo("admin"); assertThat(ctx.actorIsAdmin()).isTrue(); + assertThat(ctx.mutationMode()).isEqualTo(QueryExecutionContext.MutationMode.MAY_MUTATE); + assertThat(ctx.mutationConfirmed()).isFalse(); + } + + @Test + void mcpAdminConfirmedFactoryPassesConfirmationFlag() { + QueryExecutionContext ctx = QueryExecutionContext.mcp("admin", true, true); + assertThat(ctx.origin()).isEqualTo(QueryExecutionOrigin.MCP); + assertThat(ctx.mutationMode()).isEqualTo(QueryExecutionContext.MutationMode.MAY_MUTATE); + assertThat(ctx.mutationConfirmed()).isTrue(); + } + + @Test + void mcpNonAdminRemainsReadOnlyEvenWhenConfirmed() { + QueryExecutionContext ctx = QueryExecutionContext.mcp("dev", false, true); + assertThat(ctx.origin()).isEqualTo(QueryExecutionOrigin.MCP); + assertThat(ctx.mutationMode()).isEqualTo(QueryExecutionContext.MutationMode.READ_ONLY_ONLY); + assertThat(ctx.actorIsAdmin()).isFalse(); + } + + @Test + void forSqlSurfaceSelectsMcpOrEditorOrigin() { + QueryExecutionContext mcp = QueryExecutionContext.forSqlSurface(true, "admin", true, true); + assertThat(mcp.origin()).isEqualTo(QueryExecutionOrigin.MCP); + assertThat(mcp.mutationConfirmed()).isTrue(); + + QueryExecutionContext editor = QueryExecutionContext.forSqlSurface(false, "admin", true, true); + assertThat(editor.origin()).isEqualTo(QueryExecutionOrigin.EDITOR); + assertThat(editor.mutationConfirmed()).isTrue(); } @Test diff --git a/backend/src/test/java/com/dbaagent/service/QueryExecutionPolicyServiceTest.java b/backend/src/test/java/com/dbaagent/service/QueryExecutionPolicyServiceTest.java index a457770..309c4f2 100644 --- a/backend/src/test/java/com/dbaagent/service/QueryExecutionPolicyServiceTest.java +++ b/backend/src/test/java/com/dbaagent/service/QueryExecutionPolicyServiceTest.java @@ -491,4 +491,128 @@ void insertIntoSelect_isStillClassifiedAsInsert() { ); assertThat(decision.primaryQueryType()).isEqualTo("INSERT"); } + + // --- MCP / coding-agent surface: CREATE/ALTER go through; DROP/TRUNCATE do not --- + + @Test + void mcpAdminCreate_unconfirmedRequiresConfirmation() { + QueryExecutionPolicyException exception = assertThrows( + QueryExecutionPolicyException.class, + () -> service.enforce( + new QueryRequest("CREATE TABLE t_new (id INT PRIMARY KEY)", null, null), + QueryExecutionContext.mcp("admin", true, false), + "mysql" + ) + ); + assertThat(exception.getErrorCode()) + .isEqualTo(QueryExecutionPolicyException.EDITOR_MUTATION_CONFIRMATION_REQUIRED); + assertThat(exception.isRequiresConfirmation()).isTrue(); + } + + @Test + void mcpAdminCreate_confirmedIsAllowed() { + QueryExecutionPolicyService.PolicyDecision decision = service.enforce( + new QueryRequest("CREATE TABLE t_new (id INT PRIMARY KEY)", null, null), + QueryExecutionContext.mcp("admin", true, true), + "mysql" + ); + assertThat(decision.mutating()).isTrue(); + assertThat(decision.primaryQueryType()).isEqualTo("CREATE"); + } + + @Test + void mcpAdminAlter_confirmedIsAllowed() { + QueryExecutionPolicyService.PolicyDecision decision = service.enforce( + new QueryRequest("ALTER TABLE customers ADD COLUMN tag VARCHAR(64)", null, null), + QueryExecutionContext.mcp("admin", true, true), + "mysql" + ); + assertThat(decision.mutating()).isTrue(); + assertThat(decision.primaryQueryType()).isEqualTo("ALTER"); + } + + @Test + void mcpAdminCreateIndex_confirmedIsAllowed() { + QueryExecutionPolicyService.PolicyDecision decision = service.enforce( + new QueryRequest("CREATE INDEX idx_customers_tag ON customers (tag)", null, null), + QueryExecutionContext.mcp("admin", true, true), + "mysql" + ); + assertThat(decision.mutating()).isTrue(); + assertThat(decision.primaryQueryType()).startsWith("CREATE"); + } + + @Test + void mcpDeveloperCreate_isForbidden() { + QueryExecutionPolicyException exception = assertThrows( + QueryExecutionPolicyException.class, + () -> service.enforce( + new QueryRequest("CREATE TABLE t_new (id INT PRIMARY KEY)", null, null), + QueryExecutionContext.mcp("dev", false, true), + "mysql" + ) + ); + assertThat(exception.getErrorCode()) + .isEqualTo(QueryExecutionPolicyException.EDITOR_MUTATION_FORBIDDEN); + } + + @Test + void mcpAdminDropTable_isBlockedEvenWhenConfirmed() { + QueryExecutionPolicyException exception = assertThrows( + QueryExecutionPolicyException.class, + () -> service.enforce( + new QueryRequest("DROP TABLE temp_rollup", null, null), + QueryExecutionContext.mcp("admin", true, true), + "mysql" + ) + ); + assertThat(exception.getErrorCode()) + .isEqualTo(QueryExecutionPolicyException.UNSAFE_MUTATION_BLOCKED); + assertThat(exception.getMessage()).contains("DROP and TRUNCATE"); + } + + @Test + void mcpAdminDropIndex_isBlockedEvenWhenConfirmed() { + QueryExecutionPolicyException exception = assertThrows( + QueryExecutionPolicyException.class, + () -> service.enforce( + new QueryRequest("DROP INDEX idx_bookings_hotel ON bookings", null, null), + QueryExecutionContext.mcp("admin", true, true), + "mysql" + ) + ); + assertThat(exception.getErrorCode()) + .isEqualTo(QueryExecutionPolicyException.UNSAFE_MUTATION_BLOCKED); + assertThat(exception.getMessage()).contains("DROP and TRUNCATE"); + } + + @Test + void mcpAdminTruncate_isBlockedEvenWhenConfirmed() { + QueryExecutionPolicyException exception = assertThrows( + QueryExecutionPolicyException.class, + () -> service.enforce( + new QueryRequest("TRUNCATE TABLE temp_rollup", null, null), + QueryExecutionContext.mcp("admin", true, true), + "mysql" + ) + ); + assertThat(exception.getErrorCode()) + .isEqualTo(QueryExecutionPolicyException.UNSAFE_MUTATION_BLOCKED); + assertThat(exception.getMessage()).contains("DROP and TRUNCATE"); + } + + @Test + void mcpAdminExplainDrop_isBlockedEvenWhenConfirmed() { + QueryExecutionPolicyException exception = assertThrows( + QueryExecutionPolicyException.class, + () -> service.enforce( + new QueryRequest("EXPLAIN DROP TABLE temp_rollup", null, null), + QueryExecutionContext.mcp("admin", true, true), + "mysql" + ) + ); + assertThat(exception.getErrorCode()) + .isEqualTo(QueryExecutionPolicyException.UNSAFE_MUTATION_BLOCKED); + assertThat(exception.getMessage()).contains("DROP and TRUNCATE"); + } } diff --git a/docs/public/cli-and-mcp.md b/docs/public/cli-and-mcp.md index be10761..ca1f802 100644 --- a/docs/public/cli-and-mcp.md +++ b/docs/public/cli-and-mcp.md @@ -252,7 +252,7 @@ The MCP surface mirrors the `deepsql` CLI for almost every read/diagnostic opera | Tool | What it does | |---|---| -| `execute_sql` | Run any single SQL statement. Developers get SELECT/WITH/SHOW/EXPLAIN; admins also get DML/DDL with a two-step confirmation (DROP is blocked). | +| `execute_sql` | Run any single SQL statement. Developers get SELECT/WITH/SHOW/EXPLAIN; admins also get DML and CREATE/ALTER with a two-step confirmation (DROP and TRUNCATE are blocked). | | `analyze_query_plan` | AI-enriched plan analysis: parsed plan tree, performance issues, index recommendations, written summary using your schema + business rules. `useAnalyze: true` actually executes the query (EXPLAIN ANALYZE). | ### What's CLI-first @@ -276,8 +276,8 @@ These are reachable from any terminal where `deepsql` is installed and logged in DeepSQL's CLI and MCP `execute_sql` use **the same `QueryExecutionPolicyService` the web SQL Editor uses**. The rules: 1. **Developers** can run read-only SQL: `SELECT`, `WITH … SELECT`, `SHOW`, `EXPLAIN`. Anything else is rejected immediately. -2. **Admins** can additionally run `INSERT`, `UPDATE`, `DELETE`, `MERGE`, `UPSERT`, plus `CREATE`, `ALTER`, `TRUNCATE`. -3. `DROP` is **blocked from CLI/MCP/Editor**. Use your database's admin tooling for schema removal. +2. **Admins** can additionally run `INSERT`, `UPDATE`, `DELETE`, `MERGE`, `UPSERT`, plus non-destructive DDL (`CREATE`, `ALTER`, `CREATE INDEX`) from CLI/MCP. +3. `DROP` and `TRUNCATE` are **blocked from CLI/MCP** (coding-agent loops), even with confirmation. The web SQL Editor still blocks only `DROP TABLE`; other Editor DROPs and `TRUNCATE` stay confirm-gated. 4. `UPDATE`/`DELETE` **must include a `WHERE` clause** — unbounded mutations are rejected. 5. Multi-statement input (e.g. `UPDATE a; UPDATE b;`) is rejected. Use CTEs or run statements separately. 6. Mutations use a **two-step confirmation**. The first call returns: diff --git a/docs/root/CLAUDE.md b/docs/root/CLAUDE.md index f46936c..c71e9bc 100644 --- a/docs/root/CLAUDE.md +++ b/docs/root/CLAUDE.md @@ -21,6 +21,11 @@ Do NOT ask "should I update CLAUDE.md?" - just update it as part of task complet - Batch multiple related changes into a single commit - Provide a custom commit message if needed +- 2026-08-24: MCP / coding-agent SQL (`execute_sql`, `deepsql query`) allows + confirmed admin `CREATE`/`ALTER`/DML. `DROP` and `TRUNCATE` stay blocked on + that surface (`UNSAFE_MUTATION_BLOCKED`) even with `confirmMutation`. The + web SQL Editor still blocks only `DROP TABLE`. See `QueryExecutionContext.mcp` + and `QueryExecutionPolicyService`. - 2026-08-20: Chat access policy enforcement is fail-closed. `UserDataAccessPolicyService` walks the whole parse tree (`TablesNamesFinder` plus CTEs/UNIONs/subqueries), denies unparseable or unhandled SQL, requires an actor except `INTERNAL`/`SCHEDULED`, and @@ -264,8 +269,8 @@ Controller Layer → Service Layer → Repository Layer → Database - `get_anti_patterns` — wraps `/brain/table-anti-patterns/{cid}` and `/brain/query-anti-patterns/{cid}` - `analyze_slow_queries` — wraps `/slow-queries/analyze/{cid}` - `get_index_recommendations` — wraps `/index-recommendations/{cid}/top?limit=N`; serves the workload-weighted, recurrence-ranked top-N index recommendations with evidence + optional HypoPG cost-delta - - `apply_index_recommendation` — wraps `/index-recommendations/{rec_id}/apply?mode=&confirm=`; the only write tool in the MCP surface that takes a server-resolved recommendation id (DDL is server-generated, never client-supplied). DRY_RUN uses HypoPG (Postgres) to estimate cost-delta without writes. APPLY runs `CREATE INDEX CONCURRENTLY` / `DROP INDEX CONCURRENTLY` (configurable via `concurrent=false`). APPLY_AND_MEASURE also runs `EXPLAIN ANALYZE` for wall-clock timings. APPLY modes require `confirm=true`. - - `execute_sql` — runs any SQL through the canonical Editor endpoint `POST /connections/{cid}/query`. Backend enforces role-based policy via `QueryExecutionPolicyService`: developers get SELECT/WITH/SHOW/EXPLAIN; admins additionally get DML/DDL with a two-step `confirmMutation` flow plus a WHERE-clause guard for `UPDATE`/`DELETE`. EXPLAIN and EXPLAIN ANALYZE are just SQL — pass them as the query string. + - `apply_index_recommendation` — wraps `/index-recommendations/{rec_id}/apply?mode=&confirm=`; apply (or dry-run) a server-resolved recommendation (DDL is server-generated, never client-supplied). DRY_RUN uses HypoPG (Postgres) to estimate cost-delta without writes. APPLY runs `CREATE INDEX CONCURRENTLY` / `DROP INDEX CONCURRENTLY` (configurable via `concurrent=false`). APPLY_AND_MEASURE also runs `EXPLAIN ANALYZE` for wall-clock timings. APPLY modes require `confirm=true`. This is the only MCP path that can drop an index; `execute_sql` blocks `DROP`. + - `execute_sql` — runs SQL through the canonical Editor endpoint `POST /connections/{cid}/query`. Backend enforces role-based policy via `QueryExecutionPolicyService`: developers get SELECT/WITH/SHOW/EXPLAIN; admins additionally get DML and non-destructive DDL (`CREATE`, `ALTER`, `CREATE INDEX`) with a two-step `confirmMutation` flow plus a WHERE-clause guard for `UPDATE`/`DELETE`. `DROP` and `TRUNCATE` are blocked on MCP even when confirmed. EXPLAIN and EXPLAIN ANALYZE are just SQL — pass them as the query string. - `analyze_query_plan` — wraps `POST /explain/analyze` for AI-enriched plan analysis. Returns the parsed plan tree, performance issues, index recommendations, and an LLM-written summary that takes the connection's schema + business rules into account. `useAnalyze=true` actually runs the query (`EXPLAIN ANALYZE` semantics); for mutating statements the same admin/WHERE/confirm gates apply. - **Origin propagation**: every call carries `X-DeepSQL-Client-Type` (cli/mcp/editor), `X-DeepSQL-Client-Agent` (claude-code/cursor/codex/terminal/web — sourced from `DEEPSQL_MCP_USER_ID` for the MCP server, `--caller-agent` / `DEEPSQL_CALLER_AGENT` for the CLI), and `X-DeepSQL-Client-Version` headers. `SqlExecutionAuditService` writes them into every `security_events` row so admins can trace which surface ran which statement. - **Server-side query truncation**: pg_stat_statements (`track_activity_query_size`, default 1024B) and performance_schema (`performance_schema_max_sql_text_length`, default 1024B) silently truncate long queries. Provider code flags such rows with `SlowQuery.sourceTruncated=true`; `SlowQueryService.recoverTruncatedQueriesFromLineage(...)` then looks in `query_lineage` (vault DB) for a previously-ingested log-file copy with the full text and sets `queryTextRecoveredFromLogs=true` on success. Chat / MCP summarizers report three states (clean / recovered / still-truncated). diff --git a/docs/root/MCP_PHASE1.md b/docs/root/MCP_PHASE1.md index 53b84df..c02f51d 100644 --- a/docs/root/MCP_PHASE1.md +++ b/docs/root/MCP_PHASE1.md @@ -1,6 +1,6 @@ # DeepSQL MCP Phase 1 -Phase 1 MCP is a **stdio MCP server** that wraps DeepSQL backend APIs and enforces **read-only** database access on both the client shim and the backend. +Phase 1 MCP is a **stdio MCP server** that wraps DeepSQL backend APIs. Schema/retrieval tools stay read-only; `execute_sql` is role-gated (developers read-only, admins can run DML and CREATE/ALTER with confirmation; DROP/TRUNCATE stay blocked). ## Goals @@ -139,7 +139,7 @@ A background scheduler refreshes the ledger every 6h (`performance.recommendatio #### `apply_index_recommendation` `POST /api/index-recommendations/{recommendationId}/apply?mode=&confirm=` — apply (or dry-run) a recommendation against its target connection and measure the before/after benefit on the contributing queries that motivated it. -**This is the only write-capable tool in the phase-1 MCP surface.** All other tools are read-only. +**This is the only MCP path that can drop an index.** `execute_sql` blocks every `DROP`/`TRUNCATE`. All other tools are read-only or confirm-gated writes that cannot remove objects. Modes: @@ -184,12 +184,16 @@ What the backend allows depends on the actor's role: - **Developer (`ROLE_DEVELOPER`)** — SELECT / WITH / SHOW / EXPLAIN only. Any DML or DDL is rejected with `EDITOR_MUTATION_FORBIDDEN` (HTTP 403). -- **Admin (`ROLE_ADMIN`)** — DDL/DML accepted but gated by a two-step - confirmation flow: +- **Admin (`ROLE_ADMIN`)** — DML and non-destructive DDL (`CREATE`, `ALTER`, + `CREATE INDEX`) accepted but gated by a two-step confirmation flow: - First call with `confirmMutation=false` returns `requiresConfirmation: true` with a warnings list (e.g. "DELETE without WHERE is blocked" for unsafe shapes). - Client re-sends with `confirmMutation=true` to actually execute. +- **DROP / TRUNCATE** — blocked on this surface (`UNSAFE_MUTATION_BLOCKED`) + even for confirmed admins, including EXPLAIN-wrapped forms. Use database + admin tooling, or `apply_index_recommendation` for advisor-sourced index + drops. The web SQL Editor still blocks only `DROP TABLE`. `EXPLAIN` and `EXPLAIN ANALYZE` are valid SQL — pass them as the query. `EXPLAIN ANALYZE` of a mutating statement (`EXPLAIN ANALYZE DELETE FROM …`) diff --git a/mcp/CLAUDE.md b/mcp/CLAUDE.md index 5566f77..05f110f 100644 --- a/mcp/CLAUDE.md +++ b/mcp/CLAUDE.md @@ -79,8 +79,8 @@ in this version; ask before mid-session admin work. | `get_slow_query_insights` | Pre-computed AI insights for slow queries grouped by `kind`: `hotspots` (most total DB time), `remediation` (actionable fixes), `tail-risk` (p95/max outliers), `plan-drift` (execution plan changed), `skew` (one tenant disproportionately loaded). Default `all` returns the combined list. Accepts `window` (`LAST_24_HOURS` / `LAST_7_DAYS` / `LAST_30_DAYS`) and `limit`. | | `optimize_slow_query` | AI query REWRITE + plan diagnosis for one specific SQL. Single-query scoped, synchronous. Does NOT recommend indexes — index/pre-aggregation recs require whole-workload context (`get_index_recommendations` / Workload Analysis). Pass `avgExecutionTimeMs` to anchor the impact estimate. | | `get_index_recommendations` | **Workload-weighted DBA-grade index advisor.** Pre-computed top-N (default 5) recommendations ranked by net benefit (`Σ calls × mean_exec_time` − write-cost). Each result carries up to 5 contributing query fingerprints, the role each column played, and optional HypoPG cost-delta on Postgres. Covers both `CREATE_INDEX` and `DROP_INDEX` (unused + redundant-prefix) candidates. | -| **`apply_index_recommendation`** | **The only write-capable MCP tool.** Apply (or dry-run) a recommendation against its target connection and measure the before/after benefit on contributing queries. `DRY_RUN` (default) uses HypoPG (Postgres-only) for zero-write cost-delta. `APPLY` runs real `CREATE/DROP INDEX CONCURRENTLY` (configurable via `concurrent`). `APPLY_AND_MEASURE` additionally runs `EXPLAIN ANALYZE` for wall-clock timings. Write modes require `confirm: true`. The DDL is server-generated from the recommendation row — clients never supply SQL. | -| **`execute_sql`** | **Run any SQL statement.** Policy is server-enforced: developers can run SELECT/WITH/SHOW/EXPLAIN; admins can also run DML/DDL with a two-step confirmation. EXPLAIN and EXPLAIN ANALYZE are just SQL — no separate flag. | +| **`apply_index_recommendation`** | Apply (or dry-run) a recommendation against its target connection and measure the before/after benefit on contributing queries. `DRY_RUN` (default) uses HypoPG (Postgres-only) for zero-write cost-delta. `APPLY` runs real `CREATE/DROP INDEX CONCURRENTLY` (configurable via `concurrent`). `APPLY_AND_MEASURE` additionally runs `EXPLAIN ANALYZE` for wall-clock timings. Write modes require `confirm: true`. The DDL is server-generated from the recommendation row — clients never supply SQL. This is the only MCP path that can drop an index; `execute_sql` blocks `DROP`. | +| **`execute_sql`** | **Run a SQL statement.** Policy is server-enforced: developers can run SELECT/WITH/SHOW/EXPLAIN; admins can also run DML and non-destructive DDL (`CREATE`, `ALTER`, `CREATE INDEX`) with a two-step confirmation. `DROP` and `TRUNCATE` are blocked on this surface even for confirmed admins. EXPLAIN and EXPLAIN ANALYZE are just SQL — no separate flag. | | **`analyze_query_plan`** | **AI-enriched plan analysis** for a query. Returns the parsed plan tree, performance issues, index recommendations, and a written summary that takes the connection's schema and business rules into account. Pass `useAnalyze: true` to run `EXPLAIN ANALYZE` (actually executes the query). | --- @@ -98,7 +98,10 @@ in this version; ask before mid-session admin work. mutation that needs admin confirmation. Show the warnings to the user, wait for their explicit OK, then re-call with `confirmMutation: true`. Do not silently retry with `confirmMutation: true` on the user's behalf — - that defeats the whole point of the gate. + that defeats the whole point of the gate. `DROP` and `TRUNCATE` cannot + be confirmed around: they return `UNSAFE_MUTATION_BLOCKED`. Use database + admin tooling (or `apply_index_recommendation` for advisor-sourced + `DROP INDEX`). 3. **Developers cannot run mutations.** If the user's token has `Role.DEVELOPER` and they ask you to `UPDATE users …`, the server will @@ -236,9 +239,11 @@ gates that protect `execute_sql` kick in. You'll get back `requiresConfirmation` if you forgot. **"Apply this migration"** / **"Add this column"** / **"Delete these rows"** -→ `execute_sql` with the DDL/DML. Only admins can do it. On first call you'll -get `requiresConfirmation` — surface the warnings to the user verbatim, wait -for them to say yes, then re-call with `confirmMutation: true`. +→ `execute_sql` with the DML or non-destructive DDL (`CREATE`/`ALTER`). +Only admins can do it. On first call you'll get `requiresConfirmation` — +surface the warnings to the user verbatim, wait for them to say yes, then +re-call with `confirmMutation: true`. Do not send `DROP` or `TRUNCATE` +through `execute_sql`; those stay blocked. **"What indexes should we add?"** → `get_index_recommendations`. Returns the workload-weighted top-N (default 5) with net benefit, contributing diff --git a/mcp/README.md b/mcp/README.md index edad8ea..0eb874c 100644 --- a/mcp/README.md +++ b/mcp/README.md @@ -192,7 +192,7 @@ per-user admin ops (`users`/`access`/`permissions`). | Tool | Purpose | |---|---| -| `execute_sql` | Run any SQL — backend enforces role-based policy (developers read-only, admins can mutate with two-step confirm) | +| `execute_sql` | Run any SQL — backend enforces role-based policy (developers read-only; admins can run DML and CREATE/ALTER with two-step confirm; DROP/TRUNCATE blocked) | | `analyze_query_plan` | AI-enriched plan analysis (parsed plan tree, performance issues, index recommendations, written summary that uses the connection's schema + business rules) | EXPLAIN and EXPLAIN ANALYZE are just SQL — pass them as the query to diff --git a/mcp/deepsql-phase1-lib.js b/mcp/deepsql-phase1-lib.js index 5396d52..3a8e585 100644 --- a/mcp/deepsql-phase1-lib.js +++ b/mcp/deepsql-phase1-lib.js @@ -509,10 +509,12 @@ const TOOL_DEFINITIONS = [ description: "Execute a SQL statement through DeepSQL. Routes through the same policy " + "gate as the SQL Editor: developers can run SELECT/WITH/SHOW/EXPLAIN; admins " - + "can additionally run DML/DDL with a two-step confirmation. Pass `confirmMutation: " - + "true` to confirm a mutation. EXPLAIN and EXPLAIN ANALYZE are valid SQL — just " - + "type them as the query, no separate mode flag needed. Multi-statement input " - + "and unsafe DELETE/UPDATE without WHERE are still rejected.", + + "can additionally run DML and non-destructive DDL (CREATE, ALTER, CREATE INDEX) " + + "with a two-step confirmation. DROP and TRUNCATE are blocked on this surface " + + "even for confirmed admins. Pass `confirmMutation: true` to confirm a mutation. " + + "EXPLAIN and EXPLAIN ANALYZE are valid SQL — just type them as the query, no " + + "separate mode flag needed. Multi-statement input and unsafe DELETE/UPDATE " + + "without WHERE are still rejected.", inputSchema: { type: "object", properties: { @@ -524,8 +526,8 @@ const TOOL_DEFINITIONS = [ type: "string", description: "SQL to execute. Any single-statement SQL the connection's actor is " - + "allowed to run: SELECT/WITH/SHOW/EXPLAIN for any role, plus DML/DDL " - + "for admins.", + + "allowed to run: SELECT/WITH/SHOW/EXPLAIN for any role, plus DML and " + + "non-destructive DDL (CREATE/ALTER) for admins. DROP and TRUNCATE are blocked.", }, limit: { type: "integer", diff --git a/mcp/deepsql-phase1-lib.test.js b/mcp/deepsql-phase1-lib.test.js index 55aad48..de9fb95 100644 --- a/mcp/deepsql-phase1-lib.test.js +++ b/mcp/deepsql-phase1-lib.test.js @@ -325,6 +325,7 @@ test("execute_sql tool schema advertises mutation flow (confirmMutation) and bou assert.equal(def.inputSchema.required.includes("query"), true); assert.ok(def.inputSchema.properties.confirmMutation, "needs confirmMutation hint for agents"); assert.equal(def.inputSchema.properties.limit.maximum, 1000); + assert.match(def.description, /DROP and TRUNCATE are blocked/i); }); test("analyze_query_plan schema advertises useAnalyze + confirmMutation", () => { diff --git a/mcp/deepsql-phase1-server.js b/mcp/deepsql-phase1-server.js index 0f1dcb3..cddd6f4 100755 --- a/mcp/deepsql-phase1-server.js +++ b/mcp/deepsql-phase1-server.js @@ -193,7 +193,7 @@ class DeepSqlPhase1McpServer { }, serverInfo: SERVER_INFO, instructions: - "DeepSQL MCP exposes the user's database catalogs plus DeepSQL's retrieval brain (relevant tables/columns/FKs, business rules, inferred relationships, anti-patterns, slow-query analysis). Workflow: call list_connections first to get UUIDs; call get_brain_context with the user's question to ground generation in retrieved schema; call execute_sql to run the query (admins can run DDL/DML with a two-step confirmMutation flow, developers are server-enforced read-only); call analyze_query_plan for AI-enriched plan analysis with the connection's schema + business rules in scope. EXPLAIN and EXPLAIN ANALYZE are valid SQL — pass them as the query to execute_sql; use analyze_query_plan when you want the LLM-written summary, not raw plan rows. Always pass connectionId (UUID), not connection names.", + "DeepSQL MCP exposes the user's database catalogs plus DeepSQL's retrieval brain (relevant tables/columns/FKs, business rules, inferred relationships, anti-patterns, slow-query analysis). Workflow: call list_connections first to get UUIDs; call get_brain_context with the user's question to ground generation in retrieved schema; call execute_sql to run the query (admins can run DML and non-destructive DDL such as CREATE/ALTER with a two-step confirmMutation flow; DROP and TRUNCATE stay blocked; developers are server-enforced read-only); call analyze_query_plan for AI-enriched plan analysis with the connection's schema + business rules in scope. EXPLAIN and EXPLAIN ANALYZE are valid SQL — pass them as the query to execute_sql; use analyze_query_plan when you want the LLM-written summary, not raw plan rows. Always pass connectionId (UUID), not connection names.", }); return; } diff --git a/mcp/package.json b/mcp/package.json index 4daaec7..f834892 100644 --- a/mcp/package.json +++ b/mcp/package.json @@ -1,6 +1,6 @@ { "name": "@deepsql/mcp", - "version": "0.27.1", + "version": "0.28.0", "description": "DeepSQL CLI, DBA Agent (thin client), and stdio MCP server for self-hosted deployments", "bin": { "deepsql": "bin/deepsql.js", diff --git a/mcp/skills/SKILL_BODY.md b/mcp/skills/SKILL_BODY.md index 6736195..c5ec4e2 100644 --- a/mcp/skills/SKILL_BODY.md +++ b/mcp/skills/SKILL_BODY.md @@ -104,7 +104,7 @@ usually doesn't know about either; that's exactly why DeepSQL exists. | `optimize_slow_query(connectionId, queryText, avgExecutionTimeMs?)` | AI query REWRITE + plan diagnosis for one SQL (single-query scoped). NOT indexes — those need the whole workload; use `get_index_recommendations` or Workload Analysis. | | `get_table_growth(connectionId, tableName?, days?)` | Persistent stats history: per-table size/row time series + headline rollups. Use to answer "which tables are growing fastest?" or "how much has X grown in the last month?" without scanning the live DB. | | `get_growth_anomalies(connectionId, tableName?, unacknowledgedOnly?, days?)` | DeepSQL-flagged sudden growth spikes with severity (CRITICAL/WARNING/INFO), anomaly type, before/after sizes, confidence score. Check this BEFORE walking the user through a slow-query plan — a recent growth anomaly is often the real root cause. | -| `execute_sql(connectionId, query, ...)` | Run any SQL — SELECT for everyone, DML/DDL for admins (two-step confirm). | +| `execute_sql(connectionId, query, ...)` | Run SQL — SELECT for everyone; DML and CREATE/ALTER for admins (two-step confirm). DROP/TRUNCATE blocked. | | `analyze_query_plan(connectionId, query, useAnalyze=false)` | AI-enriched plan analysis (issues + index recs + summary). | | `get_current_user()` | Authenticated user + role + `callerCapabilities`. Read `doNotOffer` before suggesting any write. | | `test_connection(connectionId)` | Validates a saved connection (privilege report + SSH tunnel check). Read-only on the customer's DB. | @@ -233,12 +233,15 @@ deepsql [options] --caller-agent --json Tell the user: "Your DeepSQL role doesn't allow DML/DDL on this connection; ask the workspace admin to grant write access or to run the change." -- **Admin + DML/DDL (no `confirmMutation`)** → returns +- **Admin + CREATE/ALTER/DML (no `confirmMutation`)** → returns `requiresConfirmation: true` with a `warnings` array. **Show the warnings to the user verbatim. Wait for explicit OK.** Then re-call with `confirmMutation: true` (MCP) or `--write` (CLI). **Do not silently retry on the user's behalf** — that defeats the confirmation step. +- **Admin + DROP/TRUNCATE** → blocked (`UNSAFE_MUTATION_BLOCKED`) even + with `confirmMutation: true`. Use database admin tooling, or + `apply_index_recommendation` for advisor-sourced index drops. ## Row limits diff --git a/mcp/src/cli.js b/mcp/src/cli.js index 61ec0fd..5db07b5 100644 --- a/mcp/src/cli.js +++ b/mcp/src/cli.js @@ -55,7 +55,7 @@ const COMMAND_LIST = [ ["config", true, "Manage saved CLI profiles"], ["mcp", true, "Run the MCP server or install it into an editor config"], ["connections", true, "Manage database connections"], - ["query", false, "Execute a SQL statement (admin: DDL/DML with --write)"], + ["query", false, "Execute a SQL statement (admin: CREATE/ALTER/DML with --write; DROP/TRUNCATE blocked)"], ["analyze", false, "AI-enriched query plan analysis (use --analyze for EXPLAIN ANALYZE)"], ["schema", false, "Dump connection schema or DB objects as JSON"], ["digest", true, "Show DeepSQL daily digests"], @@ -192,7 +192,7 @@ const COMMAND_HELP = { }, query: { - description: "Execute a SQL statement against a connection. Same policy gate as the web SQL Editor: developers can run SELECT/WITH/SHOW/EXPLAIN; admins can additionally run DML/DDL with a two-step confirm.", + description: "Execute a SQL statement against a connection. Same policy gate as the web SQL Editor: developers can run SELECT/WITH/SHOW/EXPLAIN; admins can additionally run DML and non-destructive DDL (CREATE/ALTER) with a two-step confirm. DROP and TRUNCATE are blocked on this surface.", usage: 'deepsql query "" --connection [options]', options: [ ["--connection ", "Connection to run against"], @@ -202,7 +202,7 @@ const COMMAND_HELP = { ["--write", "Confirm a mutation upfront (skips interactive prompt; scripts/CI)"], ["--json", "Raw JSON output"], ], - notes: "EXPLAIN and EXPLAIN ANALYZE are valid SQL — type them directly. For the AI-enriched plan analysis, use `deepsql analyze`.", + notes: "EXPLAIN and EXPLAIN ANALYZE are valid SQL — type them directly. DROP and TRUNCATE are blocked even for admins (use database admin tooling). For the AI-enriched plan analysis, use `deepsql analyze`.", }, analyze: { diff --git a/mcp/src/commands/query.js b/mcp/src/commands/query.js index 70538a4..170fa67 100644 --- a/mcp/src/commands/query.js +++ b/mcp/src/commands/query.js @@ -10,13 +10,15 @@ * - Developer + SELECT/WITH/SHOW/EXPLAIN → runs immediately * - Developer + DML/DDL → backend returns 403 with a * clear EDITOR_MUTATION_FORBIDDEN - * - Admin + DML/DDL (no --write) → server returns + * - Admin + CREATE/ALTER/DML (no --write)→ server returns * requiresConfirmation; we print * the warnings, prompt y/N, and * re-send with confirmMutation=true - * - Admin + DML/DDL + --write → confirmation flag is set + * - Admin + CREATE/ALTER/DML + --write → confirmation flag is set * upfront, no prompt; useful in * scripts / CI + * - Admin + DROP/TRUNCATE → blocked (UNSAFE_MUTATION_BLOCKED) + * even with --write * * `EXPLAIN` and `EXPLAIN ANALYZE` are just SQL — no special flag needed. * For the AI-enriched plan analysis, use `deepsql analyze ""`. From 6ac5807c33a24b60012cf18f149c1404e7c1b63e Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Mon, 24 Aug 2026 14:13:31 +0000 Subject: [PATCH 2/2] test: stub ExplainController MCP header lookups leniently Mockito strict stubbing failed when ClientContext read other X-DeepSQL-* headers besides Authorization. Co-authored-by: Venkat SF --- .../com/dbaagent/controller/ExplainControllerPolicyTest.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/backend/src/test/java/com/dbaagent/controller/ExplainControllerPolicyTest.java b/backend/src/test/java/com/dbaagent/controller/ExplainControllerPolicyTest.java index b0fa8a1..4188dc3 100644 --- a/backend/src/test/java/com/dbaagent/controller/ExplainControllerPolicyTest.java +++ b/backend/src/test/java/com/dbaagent/controller/ExplainControllerPolicyTest.java @@ -30,6 +30,7 @@ import static org.mockito.ArgumentMatchers.anyBoolean; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.lenient; import static org.mockito.Mockito.never; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; @@ -135,6 +136,7 @@ void useAnalyzeTrue_confirmationRequired_propagatesRequiresConfirmation() { @Test void useAnalyzeTrue_mcpBearer_usesMcpExecutionContext() { givenConnection("conn-1", "postgres"); + lenient().when(httpRequest.getHeader(anyString())).thenReturn(null); when(httpRequest.getHeader(HttpHeaders.AUTHORIZATION)) .thenReturn("Bearer dsql_mcp_public.secret"); when(accessControlService.getCurrentUsername()).thenReturn("admin");