Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
geekypunk merged 1 commit into from
Aug 17, 2026
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CLAUDE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
203 changes: 198 additions & 5 deletions backend/src/main/java/com/dbaagent/service/McpSqlGuardService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.");
Expand Down Expand Up @@ -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)
Expand Down
63 changes: 60 additions & 3 deletions backend/src/test/java/com/dbaagent/service/McpSqlGuardServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
}
5 changes: 5 additions & 0 deletions docs/root/CLAUDE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
Loading
Loading