From fb36bc739f7f631224288d60c8b7cdbb668ee258 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Mon, 17 Aug 2026 18:15:04 +0000 Subject: [PATCH] fix(security): do not treat COMMENT/CALL table names as mutations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bare \bCOMMENT\b / \bCALL\b scans rejected SELECT * FROM comment. Match statement verbs only (mutating CTEs, WITH … DML, FOR UPDATE). Co-authored-by: Venkat SF --- CLAUDE.md | 5 + .../dbaagent/service/McpSqlGuardService.java | 203 +++++++++++++++++- .../service/McpSqlGuardServiceTest.java | 63 +++++- docs/root/CLAUDE.md | 5 + mcp/deepsql-phase1-lib.js | 177 ++++++++++++++- mcp/deepsql-phase1-lib.test.js | 47 ++++ 6 files changed, 488 insertions(+), 12 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index bc7dfc3..652ae89 100644 --- a/CLAUDE.md +++ b/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 diff --git a/backend/src/main/java/com/dbaagent/service/McpSqlGuardService.java b/backend/src/main/java/com/dbaagent/service/McpSqlGuardService.java index f3ce084..c2ccadf 100644 --- a/backend/src/main/java/com/dbaagent/service/McpSqlGuardService.java +++ b/backend/src/main/java/com/dbaagent/service/McpSqlGuardService.java @@ -45,6 +45,28 @@ public class McpSqlGuardService { "COMMENT" ); + private static final Set 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 statements nested inside an otherwise read-only wrapper + * (WITH … DELETE, mutating CTEs, FOR UPDATE, EXPLAIN DELETE). + * + *

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 splitStatements(String sql) { return List.of(normalizeSqlForInspection(sql).split(";")).stream() .map(String::trim) diff --git a/backend/src/test/java/com/dbaagent/service/McpSqlGuardServiceTest.java b/backend/src/test/java/com/dbaagent/service/McpSqlGuardServiceTest.java index b634a90..f874a67 100644 --- a/backend/src/test/java/com/dbaagent/service/McpSqlGuardServiceTest.java +++ b/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()); + } } diff --git a/docs/root/CLAUDE.md b/docs/root/CLAUDE.md index 85e4bcb..9c21e97 100644 --- a/docs/root/CLAUDE.md +++ b/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. diff --git a/mcp/deepsql-phase1-lib.js b/mcp/deepsql-phase1-lib.js index 6cc4d2e..5e899ed 100644 --- a/mcp/deepsql-phase1-lib.js +++ b/mcp/deepsql-phase1-lib.js @@ -27,6 +27,15 @@ const FORBIDDEN_SQL_KEYWORDS = [ "COMMENT", ]; +const FORBIDDEN_SQL_KEYWORD_SET = new Set(FORBIDDEN_SQL_KEYWORDS); +const FORBIDDEN_ALTERNATION = FORBIDDEN_SQL_KEYWORDS.join("|"); +const CTE_MUTATION_PATTERN = new RegExp( + `\\bAS(?:\\s+NOT)?(?:\\s+MATERIALIZED)?\\s*\\(\\s*(${FORBIDDEN_ALTERNATION})\\b`, +); +const FOR_UPDATE_PATTERN = /\bFOR\s+(?:NO\s+KEY\s+)?UPDATE\b/; +const TRAILING_DML_PATTERN = new RegExp(`\\)\\s*(${FORBIDDEN_ALTERNATION})\\b`); +const EXPLAIN_PREFIX_PATTERN = /^EXPLAIN(?:\s*\([^)]*\))?/; + const TOOL_DEFINITIONS = [ { name: "list_connections", @@ -966,11 +975,171 @@ function stripTrailingSemicolons(sql) { .replace(/;+\s*$/, ""); } +function firstWord(sql) { + const match = String(sql || "").trim().match(/^([A-Za-z]+)/); + return match ? match[1].toUpperCase() : null; +} + +function isIdentChar(ch) { + return /[A-Za-z0-9_]/.test(ch); +} + +function skipWhitespace(sql, i) { + while (i < sql.length && /\s/.test(sql[i])) { + i += 1; + } + return i; +} + +function regionMatches(sql, offset, token) { + if (offset < 0 || offset + token.length > sql.length) { + return false; + } + if (!sql.startsWith(token, offset)) { + return false; + } + const next = offset + token.length; + return next === sql.length || !isIdentChar(sql[next]); +} + +function skipIdent(sql, i) { + if (i >= sql.length) { + return -1; + } + const c = sql[i]; + if (c === '"' || c === "`" || c === "'") { + const quote = c; + i += 1; + while (i < sql.length && sql[i] !== quote) { + i += 1; + } + return i >= sql.length ? -1 : i + 1; + } + if (!/[A-Za-z_]/.test(c)) { + return -1; + } + i += 1; + while (i < sql.length && isIdentChar(sql[i])) { + i += 1; + } + return i; +} + +function skipBalancedParens(sql, openAt) { + if (sql[openAt] !== "(") { + return -1; + } + let depth = 0; + for (let i = openAt; i < sql.length; i += 1) { + if (sql[i] === "(") { + depth += 1; + } else if (sql[i] === ")") { + depth -= 1; + if (depth === 0) { + return i + 1; + } + } + } + return -1; +} + +function remainderAfterWithClause(sql) { + if (!sql || !sql.startsWith("WITH")) { + return null; + } + let i = skipWhitespace(sql, 4); + if (regionMatches(sql, i, "RECURSIVE")) { + i = skipWhitespace(sql, i + 9); + } + while (i < sql.length) { + const next = skipIdent(sql, i); + if (next < 0) { + return null; + } + i = skipWhitespace(sql, next); + if (sql[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 (sql[i] !== "(") { + return null; + } + i = skipBalancedParens(sql, i); + if (i < 0) { + return null; + } + i = skipWhitespace(sql, i); + if (sql[i] === ",") { + i = skipWhitespace(sql, i + 1); + continue; + } + return i < sql.length ? sql.slice(i) : ""; + } + return null; +} + +function findForbiddenMutation(sql) { + if (FOR_UPDATE_PATTERN.test(sql)) { + return "UPDATE"; + } + + const cteMutation = sql.match(CTE_MUTATION_PATTERN); + if (cteMutation) { + return cteMutation[1]; + } + + const first = firstWord(sql); + if (first === "EXPLAIN") { + const inner = sql.replace(EXPLAIN_PREFIX_PATTERN, "").trim(); + const innerFirst = firstWord(inner); + if (!innerFirst) { + return null; + } + if (!ALLOWED_READ_ONLY_KEYWORDS.has(innerFirst)) { + return innerFirst; + } + return findForbiddenMutation(inner); + } + + if (first === "WITH") { + const main = remainderAfterWithClause(sql); + if (main != null) { + const mainFirst = firstWord(main); + if (mainFirst && FORBIDDEN_SQL_KEYWORD_SET.has(mainFirst)) { + return mainFirst; + } + } else { + const trailing = sql.match(TRAILING_DML_PATTERN); + if (trailing) { + return trailing[1]; + } + } + } + + return null; +} + +/** + * Detect mutating statements nested inside an otherwise read-only wrapper. + * Do not scan bare \bKEYWORD\b — COMMENT/CALL are common table names + * (`SELECT * FROM comment`) and REPLACE() is a function. + */ function containsForbiddenKeyword(sql) { - const normalized = normalizeSqlForInspection(sql); - return FORBIDDEN_SQL_KEYWORDS.find((keyword) => - new RegExp(`\\b${keyword}\\b`, "i").test(normalized), - ); + const inspect = normalizeSqlForInspection(sql).toUpperCase(); + return findForbiddenMutation(inspect); } function validateReadOnlySql(sql, { allowExplain = true } = {}) { diff --git a/mcp/deepsql-phase1-lib.test.js b/mcp/deepsql-phase1-lib.test.js index 1dc066e..5ae3ea1 100644 --- a/mcp/deepsql-phase1-lib.test.js +++ b/mcp/deepsql-phase1-lib.test.js @@ -107,6 +107,53 @@ test("validateReadOnlySql rejects mutating CTEs", () => { assert.match(result.reason, /DELETE/i); }); +test("validateReadOnlySql rejects mutating CTE bodies", () => { + const result = validateReadOnlySql(` + WITH doomed AS ( + DELETE FROM users RETURNING id + ) + SELECT * FROM doomed + `); + + assert.equal(result.ok, false); + assert.match(result.reason, /DELETE/i); +}); + +test("validateReadOnlySql accepts COMMENT and CALL as table names", () => { + assert.equal(validateReadOnlySql("SELECT * FROM comment").ok, true); + assert.equal(validateReadOnlySql("SELECT * FROM call").ok, true); + assert.equal( + validateReadOnlySql( + "SELECT comment.id FROM public.comment JOIN call ON call.id = comment.call_id", + ).ok, + true, + ); +}); + +test("validateReadOnlySql accepts COMMENT columns and REPLACE()", () => { + assert.equal(validateReadOnlySql("SELECT comment FROM posts").ok, true); + assert.equal(validateReadOnlySql("SELECT COALESCE(comment, '') FROM posts").ok, true); + assert.equal(validateReadOnlySql("SELECT REPLACE(name, 'a', 'b') FROM users").ok, true); +}); + +test("validateReadOnlySql still rejects top-level CALL/COMMENT/DELETE", () => { + assert.equal(validateReadOnlySql("DELETE FROM comment").ok, false); + assert.equal(validateReadOnlySql("CALL do_thing()").ok, false); + assert.equal(validateReadOnlySql("COMMENT ON TABLE posts IS 'x'").ok, false); +}); + +test("validateReadOnlySql rejects FOR UPDATE but allows EXPLAIN of comment tables", () => { + const locked = validateReadOnlySql("SELECT * FROM orders FOR UPDATE"); + assert.equal(locked.ok, false); + assert.match(locked.reason, /UPDATE/i); + + const explainedDelete = validateReadOnlySql("EXPLAIN DELETE FROM users"); + assert.equal(explainedDelete.ok, false); + + const explainedComment = validateReadOnlySql("EXPLAIN SELECT * FROM comment"); + assert.equal(explainedComment.ok, true); +}); + test("validateReadOnlySql rejects EXPLAIN ANALYZE", () => { const result = validateReadOnlySql("EXPLAIN ANALYZE SELECT * FROM orders"); assert.equal(result.ok, false);