Skip to content

Commit 01aed6c

Browse files
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 <venkatesh.sakamuri@stayflexi.com>
1 parent b99d1b2 commit 01aed6c

23 files changed

Lines changed: 358 additions & 63 deletions

CLAUDE.md

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -245,9 +245,8 @@ their native runners. See `desktop/README.md` for the full picture.
245245
## MCP Server
246246

247247
- `mcp/deepsql-phase1-server.js` implements a Phase 1 stdio MCP server for internal rollout.
248-
- It exposes read-only tools only: listing connections, fetching schema/objects, asking DeepSQL questions, executing read-only SQL, and running EXPLAIN without ANALYZE.
249-
- It wraps existing backend APIs, so it reuses DeepSQL chat orchestration, RAG, connection management, and guardrails instead of exposing raw DB credentials.
250-
- Read-only enforcement is applied in `mcp/deepsql-phase1-lib.js` before calling backend execution endpoints.
248+
- 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.
249+
- It wraps existing backend APIs, so it reuses DeepSQL chat orchestration, RAG, connection management, and `QueryExecutionPolicyService` instead of exposing raw DB credentials.
251250
- Client config examples live in `.cursor/mcp.json` and `mcp/claude_desktop_config.example.json`.
252251
- Usage and env vars are documented in `docs/root/MCP_PHASE1.md`.
253252

agent/SOUL.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ After the answer you may offer **one short follow-up question** (a single line)
1616

1717
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.
1818

19-
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.
19+
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.
2020

2121
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.
2222

backend/src/main/java/com/dbaagent/controller/ExplainController.java

Lines changed: 20 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
import com.dbaagent.service.ClientContext;
1111
import com.dbaagent.service.CredentialService;
1212
import com.dbaagent.service.ExplainPlanService;
13+
import com.dbaagent.service.McpTokenService;
1314
import com.dbaagent.service.QueryExecutionContext;
1415
import com.dbaagent.service.QueryExecutionPolicyException;
1516
import com.dbaagent.service.QueryExecutionPolicyService;
@@ -19,6 +20,7 @@
1920
import lombok.Data;
2021
import lombok.RequiredArgsConstructor;
2122
import lombok.extern.slf4j.Slf4j;
23+
import org.springframework.http.HttpHeaders;
2224
import org.springframework.http.HttpStatus;
2325
import org.springframework.http.ResponseEntity;
2426
import org.springframework.jdbc.BadSqlGrammarException;
@@ -38,9 +40,10 @@
3840
* 1. `useAnalyze=true` actually runs the underlying statement inside
3941
* EXPLAIN ANALYZE — so for any mutating statement, that's a real
4042
* database write. We route those through QueryExecutionPolicyService
41-
* with `QueryExecutionContext.editor(...)` so the same role/WHERE/
42-
* confirmation gates that protect /api/connections/{id}/query
43-
* protect this path too.
43+
* with `QueryExecutionContext.forSqlSurface(...)` so MCP bearers
44+
* keep the MCP DROP/TRUNCATE block and Editor JWT callers keep the
45+
* Editor DROP TABLE block. Role, WHERE, and confirmation gates that
46+
* protect /api/connections/{id}/query protect this path too.
4447
*
4548
* 2. Every call — success, blocked, or failed — emits a SecurityEvent so
4649
* audit dashboards can see CLI/MCP/Editor traffic with one filter.
@@ -71,23 +74,27 @@ public ResponseEntity<?> analyzeQuery(
7174
) {
7275
ClientContext client = ClientContext.fromRequest(httpRequest);
7376
String connectionId = request.getConnectionId();
74-
QueryRequest auditQueryRequest = buildAuditQueryRequest(request);
77+
QueryRequest auditQueryRequest = buildAuditQueryRequest(request, httpRequest);
7578
ConnectionRequest connectionRequest = null;
7679

7780
try {
7881
accessControlService.assertCanUseChatEditor(connectionId);
7982
log.info("EXPLAIN analysis requested for connection: {} (useAnalyze={})", connectionId, request.isUseAnalyze());
8083

8184
// ANALYZE actually executes the SQL. Route the underlying
82-
// statement through the same policy gate the SQL Editor uses so
83-
// a developer can't bypass the mutation guard by sending
84-
// useAnalyze=true with `DELETE FROM users`.
85+
// statement through the same policy gate /connections/{id}/query
86+
// uses so a developer can't bypass the mutation guard by sending
87+
// useAnalyze=true with `DELETE FROM users`, and MCP callers keep
88+
// the DROP/TRUNCATE block.
8589
if (request.isUseAnalyze()) {
8690
connectionRequest = credentialService.getDecryptedConnection(connectionId);
8791
String dbType = providerRegistry.getCanonicalName(connectionRequest.getDbType());
8892
queryExecutionPolicyService.enforce(
8993
auditQueryRequest,
90-
QueryExecutionContext.editor(
94+
QueryExecutionContext.forSqlSurface(
95+
McpTokenService.isMcpAuthorizationHeader(
96+
httpRequest.getHeader(HttpHeaders.AUTHORIZATION)
97+
),
9198
accessControlService.getCurrentUsername(),
9299
accessControlService.isCurrentUserAdmin(),
93100
Boolean.TRUE.equals(request.getMutationConfirmed())
@@ -161,10 +168,13 @@ public ResponseEntity<?> analyzeQuery(
161168
* Carries the user's mutationConfirmed flag through so admin-confirmed
162169
* ANALYZE runs aren't stuck on the confirmation gate.
163170
*/
164-
private QueryRequest buildAuditQueryRequest(ExplainRequest request) {
171+
private QueryRequest buildAuditQueryRequest(ExplainRequest request, HttpServletRequest httpRequest) {
165172
QueryRequest qr = new QueryRequest();
166173
qr.setQuery(request.getQuery());
167-
qr.setExecutionOrigin(QueryExecutionOrigin.EDITOR);
174+
boolean mcpBearer = McpTokenService.isMcpAuthorizationHeader(
175+
httpRequest.getHeader(HttpHeaders.AUTHORIZATION)
176+
);
177+
qr.setExecutionOrigin(mcpBearer ? QueryExecutionOrigin.MCP : QueryExecutionOrigin.EDITOR);
168178
qr.setMutationConfirmed(Boolean.TRUE.equals(request.getMutationConfirmed()));
169179
return qr;
170180
}

backend/src/main/java/com/dbaagent/controller/SchemaController.java

Lines changed: 10 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -191,7 +191,12 @@ public ResponseEntity<Map<String, Object>> executeQuery(
191191
return ResponseEntity.status(HttpStatus.NOT_FOUND).body(response);
192192
}
193193
accessControlService.assertCanUseChatEditor(connectionId);
194-
queryRequest.setExecutionOrigin(QueryExecutionOrigin.EDITOR);
194+
boolean mcpBearer = McpTokenService.isMcpAuthorizationHeader(
195+
httpRequest.getHeader(HttpHeaders.AUTHORIZATION)
196+
);
197+
queryRequest.setExecutionOrigin(
198+
mcpBearer ? QueryExecutionOrigin.MCP : QueryExecutionOrigin.EDITOR
199+
);
195200
connectionRequest = credentialService.getDecryptedConnection(connectionId);
196201

197202
QueryResult result = queryExecutorService.executeQuery(
@@ -438,25 +443,14 @@ public ResponseEntity<Map<String, Object>> getTableStats(
438443
}
439444

440445
private QueryExecutionContext queryExecutionContext(QueryRequest queryRequest, HttpServletRequest httpRequest) {
441-
String username = accessControlService.getCurrentUsername();
442-
boolean admin = accessControlService.isCurrentUserAdmin();
443-
if (isMcpBearer(httpRequest)) {
444-
return QueryExecutionContext.mcp(username, admin);
445-
}
446-
return QueryExecutionContext.editor(
447-
username,
448-
admin,
446+
return QueryExecutionContext.forSqlSurface(
447+
McpTokenService.isMcpAuthorizationHeader(httpRequest.getHeader(HttpHeaders.AUTHORIZATION)),
448+
accessControlService.getCurrentUsername(),
449+
accessControlService.isCurrentUserAdmin(),
449450
Boolean.TRUE.equals(queryRequest.getMutationConfirmed())
450451
);
451452
}
452453

453-
private boolean isMcpBearer(HttpServletRequest httpRequest) {
454-
String authorization = httpRequest.getHeader(HttpHeaders.AUTHORIZATION);
455-
return authorization != null
456-
&& authorization.startsWith("Bearer ")
457-
&& authorization.substring(7).startsWith(McpTokenService.TOKEN_PREFIX);
458-
}
459-
460454
private SchemaMetadata scopedSchema(String connectionId, SchemaMetadata schema) {
461455
return userDataAccessPolicyService.filterSchemaMetadata(
462456
connectionId,

backend/src/main/java/com/dbaagent/service/McpTokenService.java

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -174,6 +174,17 @@ public boolean looksLikeMcpToken(String rawToken) {
174174
return rawToken != null && rawToken.startsWith(TOKEN_PREFIX);
175175
}
176176

177+
/**
178+
* True when the Authorization header is a DeepSQL MCP bearer token
179+
* ({@code Bearer dsql_mcp_…}). JWT and other Bearer schemes return false.
180+
*/
181+
public static boolean isMcpAuthorizationHeader(String authorization) {
182+
if (authorization == null || !authorization.regionMatches(true, 0, "Bearer ", 0, 7)) {
183+
return false;
184+
}
185+
return authorization.substring(7).startsWith(TOKEN_PREFIX);
186+
}
187+
177188
private boolean isExpired(McpToken token) {
178189
return token.getExpiresAt() != null && !token.getExpiresAt().isAfter(LocalDateTime.now());
179190
}

backend/src/main/java/com/dbaagent/service/QueryExecutionContext.java

Lines changed: 27 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -52,15 +52,40 @@ public static QueryExecutionContext mcp(String actorUsername) {
5252
}
5353

5454
public static QueryExecutionContext mcp(String actorUsername, boolean actorIsAdmin) {
55+
return mcp(actorUsername, actorIsAdmin, false);
56+
}
57+
58+
/**
59+
* MCP / coding-agent SQL. Developers stay read-only. Admins may run
60+
* non-destructive DDL/DML after the same confirmation gate as the Editor.
61+
* DROP and TRUNCATE stay blocked in {@link QueryExecutionPolicyService}.
62+
*/
63+
public static QueryExecutionContext mcp(
64+
String actorUsername,
65+
boolean actorIsAdmin,
66+
boolean mutationConfirmed
67+
) {
5568
return new QueryExecutionContext(
5669
QueryExecutionOrigin.MCP,
57-
MutationMode.READ_ONLY_ONLY,
70+
actorIsAdmin ? MutationMode.MAY_MUTATE : MutationMode.READ_ONLY_ONLY,
5871
actorUsername,
5972
actorIsAdmin,
60-
false
73+
mutationConfirmed
6174
);
6275
}
6376

77+
public static QueryExecutionContext forSqlSurface(
78+
boolean mcpBearer,
79+
String actorUsername,
80+
boolean actorIsAdmin,
81+
boolean mutationConfirmed
82+
) {
83+
if (mcpBearer) {
84+
return mcp(actorUsername, actorIsAdmin, mutationConfirmed);
85+
}
86+
return editor(actorUsername, actorIsAdmin, mutationConfirmed);
87+
}
88+
6489
public static QueryExecutionContext scheduled() {
6590
return new QueryExecutionContext(
6691
QueryExecutionOrigin.SCHEDULED,

backend/src/main/java/com/dbaagent/service/QueryExecutionPolicyService.java

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,16 @@ public class QueryExecutionPolicyService {
5555
"^\\s*DROP\\s+(?:IF\\s+EXISTS\\s+)?TABLE\\b",
5656
Pattern.CASE_INSENSITIVE
5757
);
58+
// MCP/coding-agent loops may not run any DROP or TRUNCATE, including
59+
// DROP INDEX / DROP VIEW. EXPLAIN wrappers are stripped before matching.
60+
private static final Pattern DROP_OR_TRUNCATE_HEAD = Pattern.compile(
61+
"^\\s*(DROP|TRUNCATE)\\b",
62+
Pattern.CASE_INSENSITIVE
63+
);
64+
private static final Pattern EXPLAIN_PREFIX_PATTERN = Pattern.compile(
65+
"^\\s*EXPLAIN(?:\\s*\\([^)]*\\)|\\s+ANALYZE)?\\s+",
66+
Pattern.CASE_INSENSITIVE
67+
);
5868

5969
// Text-level backstops for writes that hide inside a statement which reads as
6070
// a SELECT. Applied to SQL with literals and comments already stripped.
@@ -137,6 +147,15 @@ public PolicyDecision enforce(
137147
throw QueryExecutionPolicyException.editorMutationForbidden(mutation.queryType());
138148
}
139149

150+
if (origin == QueryExecutionOrigin.MCP
151+
&& isDropOrTruncateStatement(mutation.queryType(), statements.getFirst())) {
152+
throw QueryExecutionPolicyException.unsafeMutation(
153+
"DROP and TRUNCATE are blocked on MCP and coding-agent loops. "
154+
+ "CREATE, ALTER, and DML still require admin privileges plus confirmation.",
155+
mutation.queryType()
156+
);
157+
}
158+
140159
if (origin == QueryExecutionOrigin.EDITOR
141160
&& isDropTableStatement(mutation.queryType(), statements.getFirst())) {
142161
throw QueryExecutionPolicyException.unsafeMutation(
@@ -204,6 +223,15 @@ private StatementClassification classifyStatement(String statement, QueryExecuti
204223
return new StatementClassification("SELECT", true, false, false, false, false);
205224
}
206225
if (parsed instanceof ExplainStatement) {
226+
String wrappedMutation = detectExplainWrappedMutation(trimmed);
227+
if (wrappedMutation != null) {
228+
boolean requiresWhere = "UPDATE".equalsIgnoreCase(wrappedMutation)
229+
|| "DELETE".equalsIgnoreCase(wrappedMutation);
230+
boolean hasWhere = !requiresWhere || containsWhereClause(trimmed);
231+
return new StatementClassification(
232+
wrappedMutation, false, true, requiresWhere, hasWhere, false
233+
);
234+
}
207235
return new StatementClassification("EXPLAIN", true, false, false, false, false);
208236
}
209237
if (parsed instanceof UseStatement) {
@@ -400,6 +428,23 @@ private boolean isUseStatement(String sql) {
400428
return sql != null && USE_ANY_PATTERN.matcher(stripLeadingComments(sql)).find();
401429
}
402430

431+
/**
432+
* True for any {@code DROP …} or {@code TRUNCATE …}, including EXPLAIN-wrapped forms.
433+
* Used by the MCP origin gate so coding agents cannot confirm around destructive DDL.
434+
*/
435+
private boolean isDropOrTruncateStatement(String queryType, String sql) {
436+
if (queryType != null) {
437+
String upper = queryType.toUpperCase(Locale.ROOT);
438+
if (upper.startsWith("DROP") || upper.startsWith("TRUNCATE")) {
439+
return true;
440+
}
441+
}
442+
String remaining = stripLeadingComments(sql == null ? "" : sql);
443+
remaining = EXPLAIN_PREFIX_PATTERN.matcher(remaining).replaceFirst("");
444+
remaining = stripLeadingComments(remaining);
445+
return DROP_OR_TRUNCATE_HEAD.matcher(remaining).find();
446+
}
447+
403448
/**
404449
* Returns true only for {@code DROP TABLE}. Other DROP variants (INDEX, VIEW, SEQUENCE,
405450
* SCHEMA, FUNCTION, PROCEDURE, …) are permitted as ordinary mutations for confirmed admins.

backend/src/test/java/com/dbaagent/controller/ExplainControllerPolicyTest.java

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,8 @@
66
import com.dbaagent.service.AnalysisHistoryService;
77
import com.dbaagent.service.CredentialService;
88
import com.dbaagent.service.ExplainPlanService;
9+
import com.dbaagent.model.QueryExecutionOrigin;
10+
import com.dbaagent.service.QueryExecutionContext;
911
import com.dbaagent.service.QueryExecutionPolicyException;
1012
import com.dbaagent.service.QueryExecutionPolicyService;
1113
import com.dbaagent.service.SqlExecutionAuditService;
@@ -14,8 +16,10 @@
1416
import org.junit.jupiter.api.BeforeEach;
1517
import org.junit.jupiter.api.Test;
1618
import org.junit.jupiter.api.extension.ExtendWith;
19+
import org.mockito.ArgumentCaptor;
1720
import org.mockito.Mock;
1821
import org.mockito.junit.jupiter.MockitoExtension;
22+
import org.springframework.http.HttpHeaders;
1923
import org.springframework.http.HttpStatus;
2024
import org.springframework.http.ResponseEntity;
2125

@@ -128,6 +132,29 @@ void useAnalyzeTrue_confirmationRequired_propagatesRequiresConfirmation() {
128132
verify(explainPlanService, never()).analyzeQuery(anyString(), anyString(), anyBoolean());
129133
}
130134

135+
@Test
136+
void useAnalyzeTrue_mcpBearer_usesMcpExecutionContext() {
137+
givenConnection("conn-1", "postgres");
138+
when(httpRequest.getHeader(HttpHeaders.AUTHORIZATION))
139+
.thenReturn("Bearer dsql_mcp_public.secret");
140+
when(accessControlService.getCurrentUsername()).thenReturn("admin");
141+
when(accessControlService.isCurrentUserAdmin()).thenReturn(true);
142+
when(explainPlanService.analyzeQuery(eq("conn-1"), anyString(), eq(true)))
143+
.thenReturn(new ExplainPlanAnalysis());
144+
145+
controller.analyzeQuery(
146+
request("conn-1", "CREATE TABLE t_new (id INT PRIMARY KEY)", true),
147+
httpRequest
148+
);
149+
150+
ArgumentCaptor<QueryExecutionContext> captor = ArgumentCaptor.forClass(QueryExecutionContext.class);
151+
verify(queryExecutionPolicyService).enforce(any(), captor.capture(), eq("postgres"));
152+
assertThat(captor.getValue().origin()).isEqualTo(QueryExecutionOrigin.MCP);
153+
assertThat(captor.getValue().mutationMode())
154+
.isEqualTo(QueryExecutionContext.MutationMode.MAY_MUTATE);
155+
assertThat(captor.getValue().actorIsAdmin()).isTrue();
156+
}
157+
131158
@Test
132159
void useAnalyzeFalse_skipsPolicyGate_butStillAudits() {
133160
// Plain EXPLAIN doesn't execute the query, so we don't need to gate

backend/src/test/java/com/dbaagent/service/McpTokenServiceTest.java

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -181,4 +181,13 @@ void listTokensUsesCurrentUserIdentity() {
181181
assertEquals(1, tokens.size());
182182
assertEquals(1L, tokens.get(0).getId());
183183
}
184+
185+
@Test
186+
void isMcpAuthorizationHeaderDetectsBearerPrefix() {
187+
assertTrue(McpTokenService.isMcpAuthorizationHeader("Bearer dsql_mcp_abc.secret"));
188+
assertFalse(McpTokenService.isMcpAuthorizationHeader("Bearer eyJhbGciOi"));
189+
assertFalse(McpTokenService.isMcpAuthorizationHeader("dsql_mcp_abc.secret"));
190+
assertFalse(McpTokenService.isMcpAuthorizationHeader(null));
191+
assertFalse(McpTokenService.isMcpAuthorizationHeader(""));
192+
}
184193
}

backend/src/test/java/com/dbaagent/service/QueryExecutionContextTest.java

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,35 @@ void mcpFactoryHonoursAdminFlagFromSecurityContext() {
2323
assertThat(ctx.origin()).isEqualTo(QueryExecutionOrigin.MCP);
2424
assertThat(ctx.actorUsername()).isEqualTo("admin");
2525
assertThat(ctx.actorIsAdmin()).isTrue();
26+
assertThat(ctx.mutationMode()).isEqualTo(QueryExecutionContext.MutationMode.MAY_MUTATE);
27+
assertThat(ctx.mutationConfirmed()).isFalse();
28+
}
29+
30+
@Test
31+
void mcpAdminConfirmedFactoryPassesConfirmationFlag() {
32+
QueryExecutionContext ctx = QueryExecutionContext.mcp("admin", true, true);
33+
assertThat(ctx.origin()).isEqualTo(QueryExecutionOrigin.MCP);
34+
assertThat(ctx.mutationMode()).isEqualTo(QueryExecutionContext.MutationMode.MAY_MUTATE);
35+
assertThat(ctx.mutationConfirmed()).isTrue();
36+
}
37+
38+
@Test
39+
void mcpNonAdminRemainsReadOnlyEvenWhenConfirmed() {
40+
QueryExecutionContext ctx = QueryExecutionContext.mcp("dev", false, true);
41+
assertThat(ctx.origin()).isEqualTo(QueryExecutionOrigin.MCP);
42+
assertThat(ctx.mutationMode()).isEqualTo(QueryExecutionContext.MutationMode.READ_ONLY_ONLY);
43+
assertThat(ctx.actorIsAdmin()).isFalse();
44+
}
45+
46+
@Test
47+
void forSqlSurfaceSelectsMcpOrEditorOrigin() {
48+
QueryExecutionContext mcp = QueryExecutionContext.forSqlSurface(true, "admin", true, true);
49+
assertThat(mcp.origin()).isEqualTo(QueryExecutionOrigin.MCP);
50+
assertThat(mcp.mutationConfirmed()).isTrue();
51+
52+
QueryExecutionContext editor = QueryExecutionContext.forSqlSurface(false, "admin", true, true);
53+
assertThat(editor.origin()).isEqualTo(QueryExecutionOrigin.EDITOR);
54+
assertThat(editor.mutationConfirmed()).isTrue();
2655
}
2756

2857
@Test

0 commit comments

Comments
 (0)