fix(security): do not treat COMMENT/CALL table names as mutations (#64)

<!-- CURSOR_AGENT_PR_BODY_BEGIN -->
## Summary

Yes, this is a real bug and it should be fixed before anyone hits it on
a `comment` table.

`McpSqlGuardService.containsForbiddenKeyword` (and the matching MCP JS
shim) scanned the whole statement with `\bCOMMENT\b` / `\bCALL\b`. That
is an identifier match, not a statement-verb match, so these were
rejected as "potentially mutating":

- `SELECT * FROM comment`
- `SELECT * FROM call`
- `JOIN comment ON comment.id = …`
- `SELECT COALESCE(comment, '')`
- `SELECT REPLACE(name, 'a', 'b')` (`REPLACE` is also on the forbidden
list)

This guard sits on **dashboard artifact queries** (`POST
/api/dashboards/query` and the public share query path) as well as
`/api/mcp/query-readonly`. A generated dashboard against a `comment`
table would fail at the gate.

### Fix

Stop scanning bare words. Detect mutating **statements** only:

- Mutating CTE body: `WITH x AS (DELETE FROM t RETURNING …) SELECT …`
- WITH then DML: `WITH x AS (SELECT …) DELETE FROM t …`
- `SELECT … FOR UPDATE`
- `EXPLAIN DELETE …` (while `EXPLAIN SELECT * FROM comment` stays
allowed)

Top-level `DELETE` / `CALL` / `COMMENT ON` were already rejected by the
first-keyword allowlist and still are.

Java and JS stay in lockstep.

## Test plan

- [x] `./mvnw test -Dtest=McpSqlGuardServiceTest` — 12 tests, 0 failures
(`FROM comment` / `FROM call` allowed; mutating CTEs and `WITH … DELETE`
still blocked)
- [x] `node --test mcp/deepsql-phase1-lib.test.js` — 78 tests, 0
failures
<!-- CURSOR_AGENT_PR_BODY_END -->

<div><a
href="https://cursor.com/agents/bc-019fe687-99b1-76fd-80fa-dd213aecc497?cursor_ref=pr_footer&cursor_cta=open_in_web"><picture><source
media="(prefers-color-scheme: dark)"
srcset="https://cursor.com/assets/images/open-in-web-dark.png"><source
media="(prefers-color-scheme: light)"
srcset="https://cursor.com/assets/images/open-in-web-light.png"><img
alt="Open in Web" width="114" height="28"
src="https://cursor.com/assets/images/open-in-web-dark.png"></picture></a>&nbsp;<a
href="https://cursor.com/background-agent?bcId=bc-019fe687-99b1-76fd-80fa-dd213aecc497&cursor_ref=pr_footer&cursor_cta=open_in_cursor"><picture><source
media="(prefers-color-scheme: dark)"
srcset="https://cursor.com/assets/images/open-in-cursor-dark.png"><source
media="(prefers-color-scheme: light)"
srcset="https://cursor.com/assets/images/open-in-cursor-light.png"><img
alt="Open in Cursor" width="131" height="28"
src="https://cursor.com/assets/images/open-in-cursor-dark.png"></picture></a>&nbsp;</div>

Co-authored-by: Cursor Agent <cursoragent@cursor.com>

Author: venkateshsakamuri-lab

CLAUDE.md

@@ -261,6 +261,11 @@ broken. Assert the *outcome*, never the attempt:
 - **Silent-failure rule, concretely:** the CLI rendered an unreachable server as
   `No databases connected yet` because one `catch` covered both the connection fetch
   and decorative extras. An unreachable host must never look like an empty account.
+- **SQL mutation guards must match statement verbs, not identifiers.**
+  `McpSqlGuardService` / `mcp/deepsql-phase1-lib.js` used `\bCOMMENT\b` / `\bCALL\b`,
+  so `SELECT * FROM comment` was rejected as "potentially mutating." Plenty of
+  schemas have a `comment` table. Assert `SELECT * FROM comment` is allowed *and*
+  that `WITH x AS (DELETE …) SELECT …` / `WITH x AS (…) DELETE …` still are not.
 
 ### Data Model Rules
 

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

@@ -45,6 +45,28 @@ public class McpSqlGuardService {
         "COMMENT"
     );
 
+    private static final Set<String> FORBIDDEN_SQL_KEYWORD_SET = Set.copyOf(FORBIDDEN_SQL_KEYWORDS);
+
+    private static final String FORBIDDEN_ALTERNATION = String.join("|", FORBIDDEN_SQL_KEYWORDS);
+
+    // WITH cte AS (DELETE ...) / AS MATERIALIZED (INSERT ...) — statement verb after AS (
+    private static final Pattern CTE_MUTATION_PATTERN = Pattern.compile(
+        "\\bAS(?:\\s+NOT)?(?:\\s+MATERIALIZED)?\\s*\\(\\s*(" + FORBIDDEN_ALTERNATION + ")\\b"
+    );
+
+    private static final Pattern FOR_UPDATE_PATTERN = Pattern.compile(
+        "\\bFOR\\s+(?:NO\\s+KEY\\s+)?UPDATE\\b"
+    );
+
+    // Fail-closed fallback when the WITH-list scanner cannot parse: ") DELETE FROM ..."
+    private static final Pattern TRAILING_DML_PATTERN = Pattern.compile(
+        "\\)\\s*(" + FORBIDDEN_ALTERNATION + ")\\b"
+    );
+
+    private static final Pattern EXPLAIN_PREFIX_PATTERN = Pattern.compile(
+        "^EXPLAIN(?:\\s*\\([^)]*\\))?"
+    );
+
     public ValidationOutcome validateReadOnlySql(String sql, boolean allowExplain) {
         if (sql == null || sql.isBlank()) {
             return ValidationOutcome.invalid("Query is required.");
@@ -92,17 +114,188 @@ String firstKeyword(String sql) {
         return match.find() ? match.group(1).toUpperCase(Locale.ROOT) : null;
     }
 
+    /**
+     * Detect mutating <em>statements</em> nested inside an otherwise read-only wrapper
+     * (WITH … DELETE, mutating CTEs, FOR UPDATE, EXPLAIN DELETE).
+     *
+     * <p>Do not use a bare {@code \bKEYWORD\b} scan: {@code COMMENT} and {@code CALL}
+     * are common table/column names ({@code SELECT * FROM comment}), and
+     * {@code REPLACE()} is a function. Match statement verbs only.
+     */
     String containsForbiddenKeyword(String sql) {
-        String normalized = normalizeSqlForInspection(sql);
-        for (String keyword : FORBIDDEN_SQL_KEYWORDS) {
-            if (Pattern.compile("\\b" + Pattern.quote(keyword) + "\\b", Pattern.CASE_INSENSITIVE)
-                .matcher(normalized).find()) {
-                return keyword;
+        String inspect = normalizeSqlForInspection(sql).toUpperCase(Locale.ROOT);
+        return findForbiddenMutation(inspect);
+    }
+
+    private String findForbiddenMutation(String sql) {
+        if (FOR_UPDATE_PATTERN.matcher(sql).find()) {
+            return "UPDATE";
+        }
+
+        var cteMutation = CTE_MUTATION_PATTERN.matcher(sql);
+        if (cteMutation.find()) {
+            return cteMutation.group(1);
+        }
+
+        String first = firstWord(sql);
+        if ("EXPLAIN".equals(first)) {
+            String inner = EXPLAIN_PREFIX_PATTERN.matcher(sql).replaceFirst("").trim();
+            String innerFirst = firstWord(inner);
+            if (innerFirst == null) {
+                return null;
+            }
+            if (!ALLOWED_READ_ONLY_KEYWORDS.contains(innerFirst)) {
+                return innerFirst;
+            }
+            return findForbiddenMutation(inner);
+        }
+
+        if ("WITH".equals(first)) {
+            String main = remainderAfterWithClause(sql);
+            if (main != null) {
+                String mainFirst = firstWord(main);
+                if (mainFirst != null && FORBIDDEN_SQL_KEYWORD_SET.contains(mainFirst)) {
+                    return mainFirst;
+                }
+            } else {
+                var trailing = TRAILING_DML_PATTERN.matcher(sql);
+                if (trailing.find()) {
+                    return trailing.group(1);
+                }
             }
         }
+
         return null;
     }
 
+    private static String firstWord(String sql) {
+        if (sql == null || sql.isBlank()) {
+            return null;
+        }
+        var match = FIRST_KEYWORD_PATTERN.matcher(sql.trim());
+        return match.find() ? match.group(1).toUpperCase(Locale.ROOT) : null;
+    }
+
+    /**
+     * Skip {@code WITH [RECURSIVE] name [ (cols) ] AS [NOT] [MATERIALIZED] (...), ...}
+     * and return the main statement that follows the CTE list, or {@code null} if
+     * the shape cannot be parsed.
+     */
+    String remainderAfterWithClause(String sql) {
+        if (sql == null || !sql.startsWith("WITH")) {
+            return null;
+        }
+        int i = skipWhitespace(sql, 4);
+        if (regionMatches(sql, i, "RECURSIVE")) {
+            i = skipWhitespace(sql, i + 9);
+        }
+        while (i < sql.length()) {
+            int next = skipIdent(sql, i);
+            if (next < 0) {
+                return null;
+            }
+            i = skipWhitespace(sql, next);
+            if (i < sql.length() && sql.charAt(i) == '(') {
+                i = skipBalancedParens(sql, i);
+                if (i < 0) {
+                    return null;
+                }
+                i = skipWhitespace(sql, i);
+            }
+            if (!regionMatches(sql, i, "AS")) {
+                return null;
+            }
+            i = skipWhitespace(sql, i + 2);
+            if (regionMatches(sql, i, "NOT")) {
+                i = skipWhitespace(sql, i + 3);
+            }
+            if (regionMatches(sql, i, "MATERIALIZED")) {
+                i = skipWhitespace(sql, i + 12);
+            }
+            if (i >= sql.length() || sql.charAt(i) != '(') {
+                return null;
+            }
+            i = skipBalancedParens(sql, i);
+            if (i < 0) {
+                return null;
+            }
+            i = skipWhitespace(sql, i);
+            if (i < sql.length() && sql.charAt(i) == ',') {
+                i = skipWhitespace(sql, i + 1);
+                continue;
+            }
+            return i < sql.length() ? sql.substring(i) : "";
+        }
+        return null;
+    }
+
+    private static boolean regionMatches(String sql, int offset, String token) {
+        return offset >= 0
+            && offset + token.length() <= sql.length()
+            && sql.startsWith(token, offset)
+            && (offset + token.length() == sql.length()
+                || !isIdentChar(sql.charAt(offset + token.length())));
+    }
+
+    private static boolean isIdentChar(char c) {
+        return Character.isLetterOrDigit(c) || c == '_';
+    }
+
+    private static int skipWhitespace(String sql, int i) {
+        while (i < sql.length() && Character.isWhitespace(sql.charAt(i))) {
+            i++;
+        }
+        return i;
+    }
+
+    private static int skipIdent(String sql, int i) {
+        if (i >= sql.length()) {
+            return -1;
+        }
+        char c = sql.charAt(i);
+        if (c == '"' || c == '`' || c == '\'') {
+            char quote = c;
+            i++;
+            while (i < sql.length() && sql.charAt(i) != quote) {
+                i++;
+            }
+            if (i >= sql.length()) {
+                return -1;
+            }
+            return i + 1;
+        }
+        if (c == '.' ) {
+            return -1;
+        }
+        if (!Character.isLetter(c) && c != '_') {
+            return -1;
+        }
+        i++;
+        while (i < sql.length() && isIdentChar(sql.charAt(i))) {
+            i++;
+        }
+        return i;
+    }
+
+    private static int skipBalancedParens(String sql, int openAt) {
+        if (openAt >= sql.length() || sql.charAt(openAt) != '(') {
+            return -1;
+        }
+        int depth = 0;
+        for (int i = openAt; i < sql.length(); i++) {
+            char c = sql.charAt(i);
+            if (c == '(') {
+                depth++;
+            } else if (c == ')') {
+                depth--;
+                if (depth == 0) {
+                    return i + 1;
+                }
+            }
+        }
+        return -1;
+    }
+
     List<String> splitStatements(String sql) {
         return List.of(normalizeSqlForInspection(sql).split(";")).stream()
             .map(String::trim)

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

@@ -56,13 +56,70 @@ void ignoresMutatingKeywordsInsideStringsAndComments() {
     }
 
     @Test
-    void rejectsMutatingKeywordInReadOnlyCte() {
+    void rejectsMutatingCteBody() {
         var result = service.validateReadOnlySql("""
-            WITH recent AS (SELECT * FROM orders)
-            SELECT * FROM recent UPDATE
+            WITH doomed AS (
+              DELETE FROM users RETURNING id
+            )
+            SELECT * FROM doomed
             """, true);
 
+        assertFalse(result.ok());
+        assertEquals("Blocked potentially mutating SQL keyword: DELETE.", result.reason());
+    }
+
+    @Test
+    void rejectsWithClauseFollowedByDelete() {
+        var result = service.validateReadOnlySql("""
+            WITH doomed AS (
+              SELECT id FROM users
+            )
+            DELETE FROM users WHERE id IN (SELECT id FROM doomed)
+            """, true);
+
+        assertFalse(result.ok());
+        assertEquals("Blocked potentially mutating SQL keyword: DELETE.", result.reason());
+    }
+
+    @Test
+    void acceptsCommentAndCallAsTableNames() {
+        assertTrue(service.validateReadOnlySql("SELECT * FROM comment", true).ok());
+        assertTrue(service.validateReadOnlySql("SELECT * FROM call", true).ok());
+        assertTrue(service.validateReadOnlySql(
+            "SELECT comment.id FROM public.comment JOIN call ON call.id = comment.call_id",
+            true
+        ).ok());
+    }
+
+    @Test
+    void acceptsCommentAsColumnAndFunctionArgument() {
+        assertTrue(service.validateReadOnlySql("SELECT comment FROM posts", true).ok());
+        assertTrue(service.validateReadOnlySql("SELECT COALESCE(comment, '') FROM posts", true).ok());
+        assertTrue(service.validateReadOnlySql("SELECT REPLACE(name, 'a', 'b') FROM users", true).ok());
+    }
+
+    @Test
+    void stillRejectsTopLevelMutations() {
+        assertFalse(service.validateReadOnlySql("DELETE FROM comment", true).ok());
+        assertFalse(service.validateReadOnlySql("CALL do_thing()", true).ok());
+        assertFalse(service.validateReadOnlySql("COMMENT ON TABLE posts IS 'x'", true).ok());
+    }
+
+    @Test
+    void rejectsSelectForUpdate() {
+        var result = service.validateReadOnlySql("SELECT * FROM orders FOR UPDATE", true);
+
         assertFalse(result.ok());
         assertEquals("Blocked potentially mutating SQL keyword: UPDATE.", result.reason());
     }
+
+    @Test
+    void rejectsExplainOfDeleteButAllowsExplainOfCommentTable() {
+        var deletePlan = service.validateReadOnlySql("EXPLAIN DELETE FROM users", true);
+        assertFalse(deletePlan.ok());
+        assertEquals("Blocked potentially mutating SQL keyword: DELETE.", deletePlan.reason());
+
+        var commentPlan = service.validateReadOnlySql("EXPLAIN SELECT * FROM comment", true);
+        assertTrue(commentPlan.ok());
+    }
 }

docs/root/CLAUDE.md

@@ -23,6 +23,11 @@ Do NOT ask "should I update CLAUDE.md?" - just update it as part of task complet
 
 ## Recent Changes
 
+- 2026-08-17: `McpSqlGuardService` (and the matching MCP JS shim) no longer treats
+  `COMMENT` / `CALL` / `REPLACE` as mutating when they appear as table, column, or
+  function names. The guard matches statement verbs: mutating CTEs, `WITH … DELETE`,
+  `FOR UPDATE`, and `EXPLAIN DELETE`. `SELECT * FROM comment` is allowed. Dashboards
+  and `/api/mcp/query-readonly` share this guard.
 - 2026-02-04: Moved all Markdown docs into `docs/` (root docs under `docs/root/`), added a root `README.md` stub, and updated doc links.
 - 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`.
 - 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.