Skip to content

Commit 43dee87

Browse files
geekypunkgeekypunk
authored
Merge branch 'main' into fix/editor-sql-guard-hardening
2 parents 9cb0584 + 659394f commit 43dee87

6 files changed

Lines changed: 488 additions & 12 deletions

File tree

‎CLAUDE.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -261,6 +261,11 @@ broken. Assert the *outcome*, never the attempt:
261261
- **Silent-failure rule, concretely:** the CLI rendered an unreachable server as
262262
`No databases connected yet` because one `catch` covered both the connection fetch
263263
and decorative extras. An unreachable host must never look like an empty account.
264+
- **SQL mutation guards must match statement verbs, not identifiers.**
265+
`McpSqlGuardService` / `mcp/deepsql-phase1-lib.js` used `\bCOMMENT\b` / `\bCALL\b`,
266+
so `SELECT * FROM comment` was rejected as "potentially mutating." Plenty of
267+
schemas have a `comment` table. Assert `SELECT * FROM comment` is allowed *and*
268+
that `WITH x AS (DELETE …) SELECT …` / `WITH x AS (…) DELETE …` still are not.
264269

265270
### SQL Editor Guard Rules
266271

‎backend/src/main/java/com/dbaagent/service/McpSqlGuardService.java‎

Lines changed: 198 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,28 @@ public class McpSqlGuardService {
4545
"COMMENT"
4646
);
4747

48+
private static final Set<String> FORBIDDEN_SQL_KEYWORD_SET = Set.copyOf(FORBIDDEN_SQL_KEYWORDS);
49+
50+
private static final String FORBIDDEN_ALTERNATION = String.join("|", FORBIDDEN_SQL_KEYWORDS);
51+
52+
// WITH cte AS (DELETE ...) / AS MATERIALIZED (INSERT ...) — statement verb after AS (
53+
private static final Pattern CTE_MUTATION_PATTERN = Pattern.compile(
54+
"\\bAS(?:\\s+NOT)?(?:\\s+MATERIALIZED)?\\s*\\(\\s*(" + FORBIDDEN_ALTERNATION + ")\\b"
55+
);
56+
57+
private static final Pattern FOR_UPDATE_PATTERN = Pattern.compile(
58+
"\\bFOR\\s+(?:NO\\s+KEY\\s+)?UPDATE\\b"
59+
);
60+
61+
// Fail-closed fallback when the WITH-list scanner cannot parse: ") DELETE FROM ..."
62+
private static final Pattern TRAILING_DML_PATTERN = Pattern.compile(
63+
"\\)\\s*(" + FORBIDDEN_ALTERNATION + ")\\b"
64+
);
65+
66+
private static final Pattern EXPLAIN_PREFIX_PATTERN = Pattern.compile(
67+
"^EXPLAIN(?:\\s*\\([^)]*\\))?"
68+
);
69+
4870
public ValidationOutcome validateReadOnlySql(String sql, boolean allowExplain) {
4971
if (sql == null || sql.isBlank()) {
5072
return ValidationOutcome.invalid("Query is required.");
@@ -92,17 +114,188 @@ String firstKeyword(String sql) {
92114
return match.find() ? match.group(1).toUpperCase(Locale.ROOT) : null;
93115
}
94116

117+
/**
118+
* Detect mutating <em>statements</em> nested inside an otherwise read-only wrapper
119+
* (WITH … DELETE, mutating CTEs, FOR UPDATE, EXPLAIN DELETE).
120+
*
121+
* <p>Do not use a bare {@code \bKEYWORD\b} scan: {@code COMMENT} and {@code CALL}
122+
* are common table/column names ({@code SELECT * FROM comment}), and
123+
* {@code REPLACE()} is a function. Match statement verbs only.
124+
*/
95125
String containsForbiddenKeyword(String sql) {
96-
String normalized = normalizeSqlForInspection(sql);
97-
for (String keyword : FORBIDDEN_SQL_KEYWORDS) {
98-
if (Pattern.compile("\\b" + Pattern.quote(keyword) + "\\b", Pattern.CASE_INSENSITIVE)
99-
.matcher(normalized).find()) {
100-
return keyword;
126+
String inspect = normalizeSqlForInspection(sql).toUpperCase(Locale.ROOT);
127+
return findForbiddenMutation(inspect);
128+
}
129+
130+
private String findForbiddenMutation(String sql) {
131+
if (FOR_UPDATE_PATTERN.matcher(sql).find()) {
132+
return "UPDATE";
133+
}
134+
135+
var cteMutation = CTE_MUTATION_PATTERN.matcher(sql);
136+
if (cteMutation.find()) {
137+
return cteMutation.group(1);
138+
}
139+
140+
String first = firstWord(sql);
141+
if ("EXPLAIN".equals(first)) {
142+
String inner = EXPLAIN_PREFIX_PATTERN.matcher(sql).replaceFirst("").trim();
143+
String innerFirst = firstWord(inner);
144+
if (innerFirst == null) {
145+
return null;
146+
}
147+
if (!ALLOWED_READ_ONLY_KEYWORDS.contains(innerFirst)) {
148+
return innerFirst;
149+
}
150+
return findForbiddenMutation(inner);
151+
}
152+
153+
if ("WITH".equals(first)) {
154+
String main = remainderAfterWithClause(sql);
155+
if (main != null) {
156+
String mainFirst = firstWord(main);
157+
if (mainFirst != null && FORBIDDEN_SQL_KEYWORD_SET.contains(mainFirst)) {
158+
return mainFirst;
159+
}
160+
} else {
161+
var trailing = TRAILING_DML_PATTERN.matcher(sql);
162+
if (trailing.find()) {
163+
return trailing.group(1);
164+
}
101165
}
102166
}
167+
103168
return null;
104169
}
105170

171+
private static String firstWord(String sql) {
172+
if (sql == null || sql.isBlank()) {
173+
return null;
174+
}
175+
var match = FIRST_KEYWORD_PATTERN.matcher(sql.trim());
176+
return match.find() ? match.group(1).toUpperCase(Locale.ROOT) : null;
177+
}
178+
179+
/**
180+
* Skip {@code WITH [RECURSIVE] name [ (cols) ] AS [NOT] [MATERIALIZED] (...), ...}
181+
* and return the main statement that follows the CTE list, or {@code null} if
182+
* the shape cannot be parsed.
183+
*/
184+
String remainderAfterWithClause(String sql) {
185+
if (sql == null || !sql.startsWith("WITH")) {
186+
return null;
187+
}
188+
int i = skipWhitespace(sql, 4);
189+
if (regionMatches(sql, i, "RECURSIVE")) {
190+
i = skipWhitespace(sql, i + 9);
191+
}
192+
while (i < sql.length()) {
193+
int next = skipIdent(sql, i);
194+
if (next < 0) {
195+
return null;
196+
}
197+
i = skipWhitespace(sql, next);
198+
if (i < sql.length() && sql.charAt(i) == '(') {
199+
i = skipBalancedParens(sql, i);
200+
if (i < 0) {
201+
return null;
202+
}
203+
i = skipWhitespace(sql, i);
204+
}
205+
if (!regionMatches(sql, i, "AS")) {
206+
return null;
207+
}
208+
i = skipWhitespace(sql, i + 2);
209+
if (regionMatches(sql, i, "NOT")) {
210+
i = skipWhitespace(sql, i + 3);
211+
}
212+
if (regionMatches(sql, i, "MATERIALIZED")) {
213+
i = skipWhitespace(sql, i + 12);
214+
}
215+
if (i >= sql.length() || sql.charAt(i) != '(') {
216+
return null;
217+
}
218+
i = skipBalancedParens(sql, i);
219+
if (i < 0) {
220+
return null;
221+
}
222+
i = skipWhitespace(sql, i);
223+
if (i < sql.length() && sql.charAt(i) == ',') {
224+
i = skipWhitespace(sql, i + 1);
225+
continue;
226+
}
227+
return i < sql.length() ? sql.substring(i) : "";
228+
}
229+
return null;
230+
}
231+
232+
private static boolean regionMatches(String sql, int offset, String token) {
233+
return offset >= 0
234+
&& offset + token.length() <= sql.length()
235+
&& sql.startsWith(token, offset)
236+
&& (offset + token.length() == sql.length()
237+
|| !isIdentChar(sql.charAt(offset + token.length())));
238+
}
239+
240+
private static boolean isIdentChar(char c) {
241+
return Character.isLetterOrDigit(c) || c == '_';
242+
}
243+
244+
private static int skipWhitespace(String sql, int i) {
245+
while (i < sql.length() && Character.isWhitespace(sql.charAt(i))) {
246+
i++;
247+
}
248+
return i;
249+
}
250+
251+
private static int skipIdent(String sql, int i) {
252+
if (i >= sql.length()) {
253+
return -1;
254+
}
255+
char c = sql.charAt(i);
256+
if (c == '"' || c == '`' || c == '\'') {
257+
char quote = c;
258+
i++;
259+
while (i < sql.length() && sql.charAt(i) != quote) {
260+
i++;
261+
}
262+
if (i >= sql.length()) {
263+
return -1;
264+
}
265+
return i + 1;
266+
}
267+
if (c == '.' ) {
268+
return -1;
269+
}
270+
if (!Character.isLetter(c) && c != '_') {
271+
return -1;
272+
}
273+
i++;
274+
while (i < sql.length() && isIdentChar(sql.charAt(i))) {
275+
i++;
276+
}
277+
return i;
278+
}
279+
280+
private static int skipBalancedParens(String sql, int openAt) {
281+
if (openAt >= sql.length() || sql.charAt(openAt) != '(') {
282+
return -1;
283+
}
284+
int depth = 0;
285+
for (int i = openAt; i < sql.length(); i++) {
286+
char c = sql.charAt(i);
287+
if (c == '(') {
288+
depth++;
289+
} else if (c == ')') {
290+
depth--;
291+
if (depth == 0) {
292+
return i + 1;
293+
}
294+
}
295+
}
296+
return -1;
297+
}
298+
106299
List<String> splitStatements(String sql) {
107300
return List.of(normalizeSqlForInspection(sql).split(";")).stream()
108301
.map(String::trim)

‎backend/src/test/java/com/dbaagent/service/McpSqlGuardServiceTest.java‎

Lines changed: 60 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -56,13 +56,70 @@ void ignoresMutatingKeywordsInsideStringsAndComments() {
5656
}
5757

5858
@Test
59-
void rejectsMutatingKeywordInReadOnlyCte() {
59+
void rejectsMutatingCteBody() {
6060
var result = service.validateReadOnlySql("""
61-
WITH recent AS (SELECT * FROM orders)
62-
SELECT * FROM recent UPDATE
61+
WITH doomed AS (
62+
DELETE FROM users RETURNING id
63+
)
64+
SELECT * FROM doomed
6365
""", true);
6466

67+
assertFalse(result.ok());
68+
assertEquals("Blocked potentially mutating SQL keyword: DELETE.", result.reason());
69+
}
70+
71+
@Test
72+
void rejectsWithClauseFollowedByDelete() {
73+
var result = service.validateReadOnlySql("""
74+
WITH doomed AS (
75+
SELECT id FROM users
76+
)
77+
DELETE FROM users WHERE id IN (SELECT id FROM doomed)
78+
""", true);
79+
80+
assertFalse(result.ok());
81+
assertEquals("Blocked potentially mutating SQL keyword: DELETE.", result.reason());
82+
}
83+
84+
@Test
85+
void acceptsCommentAndCallAsTableNames() {
86+
assertTrue(service.validateReadOnlySql("SELECT * FROM comment", true).ok());
87+
assertTrue(service.validateReadOnlySql("SELECT * FROM call", true).ok());
88+
assertTrue(service.validateReadOnlySql(
89+
"SELECT comment.id FROM public.comment JOIN call ON call.id = comment.call_id",
90+
true
91+
).ok());
92+
}
93+
94+
@Test
95+
void acceptsCommentAsColumnAndFunctionArgument() {
96+
assertTrue(service.validateReadOnlySql("SELECT comment FROM posts", true).ok());
97+
assertTrue(service.validateReadOnlySql("SELECT COALESCE(comment, '') FROM posts", true).ok());
98+
assertTrue(service.validateReadOnlySql("SELECT REPLACE(name, 'a', 'b') FROM users", true).ok());
99+
}
100+
101+
@Test
102+
void stillRejectsTopLevelMutations() {
103+
assertFalse(service.validateReadOnlySql("DELETE FROM comment", true).ok());
104+
assertFalse(service.validateReadOnlySql("CALL do_thing()", true).ok());
105+
assertFalse(service.validateReadOnlySql("COMMENT ON TABLE posts IS 'x'", true).ok());
106+
}
107+
108+
@Test
109+
void rejectsSelectForUpdate() {
110+
var result = service.validateReadOnlySql("SELECT * FROM orders FOR UPDATE", true);
111+
65112
assertFalse(result.ok());
66113
assertEquals("Blocked potentially mutating SQL keyword: UPDATE.", result.reason());
67114
}
115+
116+
@Test
117+
void rejectsExplainOfDeleteButAllowsExplainOfCommentTable() {
118+
var deletePlan = service.validateReadOnlySql("EXPLAIN DELETE FROM users", true);
119+
assertFalse(deletePlan.ok());
120+
assertEquals("Blocked potentially mutating SQL keyword: DELETE.", deletePlan.reason());
121+
122+
var commentPlan = service.validateReadOnlySql("EXPLAIN SELECT * FROM comment", true);
123+
assertTrue(commentPlan.ok());
124+
}
68125
}

‎docs/root/CLAUDE.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,11 @@ Do NOT ask "should I update CLAUDE.md?" - just update it as part of task complet
2323

2424
## Recent Changes
2525

26+
- 2026-08-17: `McpSqlGuardService` (and the matching MCP JS shim) no longer treats
27+
`COMMENT` / `CALL` / `REPLACE` as mutating when they appear as table, column, or
28+
function names. The guard matches statement verbs: mutating CTEs, `WITH … DELETE`,
29+
`FOR UPDATE`, and `EXPLAIN DELETE`. `SELECT * FROM comment` is allowed. Dashboards
30+
and `/api/mcp/query-readonly` share this guard.
2631
- 2026-02-04: Moved all Markdown docs into `docs/` (root docs under `docs/root/`), added a root `README.md` stub, and updated doc links.
2732
- 2026-03-12: Added a Phase 1 DeepSQL MCP stdio server in `mcp/` with read-only tools for connections, schema, chat, SQL execution, and EXPLAIN. See `docs/root/MCP_PHASE1.md`.
2833
- 2026-03-30: Main chat execution was tightened to stay schema-agnostic. Do not add customer-specific table names, column names, SQL templates, or prompt-to-table shortcuts in chat classifier, planner, resolver, composer, or execution paths. Fix chat behavior through generic semantic ranking, context retrieval, and guardrails instead.

0 commit comments

Comments
 (0)