fix(editor): report unparseable SQL as a syntax error, not a permissions denial

A user pasted a SELECT that still carried the double quotes it had in source
code ("select h.id, ...) and was told "Only admins can execute DDL or DML
from the SQL Editor". That reads as a permissions problem and sent people
looking for a role fix. The statement is neither DDL nor DML — with an
unclosed double quote the whole thing is one quoted identifier, so it is not
valid SQL at all.

Two keyword heuristics disagreed and the disagreement was resolved as
"mutation". QueryNormalizer.detectQueryType sanitizes a prefix away and
answers SELECT; the provider's isReadOnlyQuery strips only comments, still
sees the leading quote, and answers false. mutating was computed as
(!readOnly && type != UNKNOWN), so a statement classifyStatement had itself
labelled SELECT became a mutation.

classifyStatement now records that the parser rejected the statement and,
when the detected verb is read-only and no hidden write was found, returns
notParseable. enforce throws STATEMENT_NOT_PARSEABLE ahead of both the
read-only branch and the mutation-confirmation branch, with a message naming
the likely cause.

The statement is still blocked, admins included — only the diagnosis
changed. An admin is deliberately not offered a confirmation prompt for a
statement nothing managed to classify, since confirming past the guard is
the one way this could become a bypass. The reclassification is gated on
isReadOnlyVerb(queryType) and hiddenWrite == null, which is what keeps a
write the parser happens to reject from being excused as a typo: an
unparseable DELETE, and a malformed data-modifying CTE, both keep their
mutation handling, and both are covered by tests.

QueryExecutionPolicyServiceTest: 49 tests pass, including every pre-existing
guard case. Full backend suite diffed against pristine HEAD — failure sets
identical, no regressions. The MCP guard already reported this case honestly
("Only read-only SQL is allowed ...") and was left alone.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

Author: venkateshsakamuri-lab

CLAUDE.md

@@ -724,6 +724,25 @@ it against a real database — not a theoretical hardening pass.
   it killed **every** active query on the connection, including other users' work.
   The cancel endpoint is scoped to the connection *and* the user who started the
   run, so an execution id is not a kill primitive for someone else's query.
+- **A statement DeepSQL cannot parse is a syntax error, not DDL/DML — say so.** A user
+  pasted a SELECT still carrying the double quotes it had in source code
+  (`"select h.id, ...`) and got **"Only admins can execute DDL or DML from the SQL
+  Editor"**, which reads as a permissions problem and sends people hunting for a role fix.
+  Two keyword heuristics disagreed and the code resolved the disagreement as "mutation":
+  `QueryNormalizer.detectQueryType` sanitizes a prefix away and answered `SELECT`, while
+  the provider's `isReadOnlyQuery` strips only *comments*, still saw the leading `"`, and
+  answered false — so `mutating = !readOnly && type != UNKNOWN` labelled a SELECT a
+  mutation. `classifyStatement` now records that the parser rejected the statement and,
+  when the detected verb is read-only and no hidden write was found, returns
+  `notParseable`; `enforce` throws `STATEMENT_NOT_PARSEABLE` ahead of both the read-only
+  and confirmation branches. **The statement is still blocked, for admins too** — only the
+  diagnosis changed, and an admin is deliberately *not* offered a confirmation prompt for
+  something nothing managed to classify. The reclassification is gated on
+  `isReadOnlyVerb(queryType)` and `hiddenWrite == null`, which is what keeps it from
+  becoming a bypass: an unparseable `DELETE`, and a malformed data-modifying CTE, both keep
+  their mutation handling (covered by `anUnparseableWriteIsStillTreatedAsAMutation` and
+  `aMalformedDataModifyingCteIsStillBlockedAsAWrite`). The MCP guard already reported this
+  case honestly ("Only read-only SQL is allowed …") and was left alone.
 - **Keep the client timeout under the proxy's.** `docker/nginx/default.conf` gives
   up at `proxy_read_timeout 300s`; the Editor used to ask for 600s, so a 6-minute
   query returned an opaque 504 while still running. `QUERY_TIMEOUT_SECONDS = 240`

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

@@ -12,6 +12,7 @@ public class QueryExecutionPolicyException extends RuntimeException {
     public static final String UNSAFE_MUTATION_BLOCKED = "UNSAFE_MUTATION_BLOCKED";
     public static final String DB_WRITE_PRIVILEGE_DENIED = "DB_WRITE_PRIVILEGE_DENIED";
     public static final String MULTI_STATEMENT_MISSING_SEMICOLONS = "MULTI_STATEMENT_MISSING_SEMICOLONS";
+    public static final String STATEMENT_NOT_PARSEABLE = "STATEMENT_NOT_PARSEABLE";
 
     private final String errorCode;
     private final HttpStatus httpStatus;
@@ -57,6 +58,33 @@ public static QueryExecutionPolicyException editorMutationForbidden(String query
         );
     }
 
+    /**
+     * The statement reads as a SELECT by keyword but is not valid SQL, so DeepSQL cannot
+     * verify it is read-only.
+     *
+     * <p>It stays blocked — an unclassifiable statement is exactly what the guard exists
+     * to stop — but it is <em>not</em> DDL or DML, and saying so sent a user hunting for a
+     * permissions problem that did not exist. The trigger was a query pasted with the
+     * surrounding double quotes it had in source code: {@code QueryNormalizer.detectQueryType}
+     * sanitizes the prefix away and answers SELECT, while {@code isReadOnlyQuery} strips only
+     * comments, still sees a leading {@code "}, and answers "not read-only". Those two
+     * answers together mean "malformed", not "mutation".
+     */
+    public static QueryExecutionPolicyException statementNotParseable(String queryType) {
+        return new QueryExecutionPolicyException(
+            STATEMENT_NOT_PARSEABLE,
+            HttpStatus.BAD_REQUEST,
+            "DeepSQL could not parse this statement, so it was blocked before running. "
+                + "It starts like a SELECT but is not valid SQL — check for a stray quote, "
+                + "bracket or backtick. SQL copied out of code or JSON often keeps the "
+                + "surrounding \" characters, which makes the whole statement one quoted "
+                + "identifier.",
+            false,
+            queryType,
+            List.of()
+        );
+    }
+
     public static QueryExecutionPolicyException confirmationRequired(String queryType, List<String> warnings) {
         return new QueryExecutionPolicyException(
             EDITOR_MUTATION_CONFIRMATION_REQUIRED,

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

@@ -121,6 +121,19 @@ public PolicyDecision enforce(
             throw QueryExecutionPolicyException.multiStatementMissingSemicolons();
         }
 
+        // Same idea, one step earlier: a statement DeepSQL could not parse is blocked for
+        // everyone, but it is a syntax error, not DDL/DML. Reported ahead of both the
+        // read-only branch and the mutation-confirmation branch, so an admin gets the same
+        // accurate diagnosis instead of being offered a confirmation prompt for a
+        // statement nothing has actually classified.
+        StatementClassification unparseable = classifications.stream()
+            .filter(StatementClassification::notParseable)
+            .findFirst()
+            .orElse(null);
+        if (unparseable != null) {
+            throw QueryExecutionPolicyException.statementNotParseable(unparseable.queryType());
+        }
+
         if (effectiveContext.mutationMode() == QueryExecutionContext.MutationMode.READ_ONLY_ONLY) {
             if (!allReadOnlyOrPreamble || anyMutation) {
                 if (origin == QueryExecutionOrigin.CHAT) {
@@ -210,6 +223,7 @@ private StatementClassification classifyStatement(String statement, QueryExecuti
         // classified as a read.
         String hiddenWrite = detectHiddenWrite(trimmed);
 
+        boolean parseFailed = false;
         try {
             Statement parsed = CCJSqlParserUtil.parse(trimmed);
             if (parsed instanceof Select select) {
@@ -272,6 +286,7 @@ private StatementClassification classifyStatement(String statement, QueryExecuti
                 return new StatementClassification("TRUNCATE", false, true, false, true, false);
             }
         } catch (Exception parseError) {
+            parseFailed = true;
             log.debug("Falling back to keyword SQL classification: {}", parseError.getMessage());
         }
 
@@ -294,9 +309,41 @@ private StatementClassification classifyStatement(String statement, QueryExecuti
         boolean mutating = !readOnly && !"UNKNOWN".equalsIgnoreCase(queryType);
         boolean requiresWhere = "UPDATE".equalsIgnoreCase(queryType) || "DELETE".equalsIgnoreCase(queryType);
         boolean hasWhere = !requiresWhere || containsWhereClause(trimmed);
+
+        // The two keyword heuristics can contradict each other, and their disagreement
+        // means "malformed", not "mutation". `QueryNormalizer.detectQueryType` sanitizes a
+        // prefix away before matching, so it answers SELECT for `"select ...`; the
+        // provider's `isReadOnlyQuery` strips only comments, still sees the leading quote,
+        // and answers false. That combination used to fall through as mutating=true, and a
+        // user pasting a SELECT with the double quotes it carried in source code was told
+        // "Only admins can execute DDL or DML" — a permissions error for a syntax problem.
+        //
+        // It stays blocked: the parser rejected it, so nothing here can vouch for it being
+        // read-only, and this is deliberately reported the same way to admins rather than
+        // routed into the mutation-confirmation flow. Only the diagnosis changes.
+        if (parseFailed && mutating && hiddenWrite == null && isReadOnlyVerb(queryType)) {
+            return new StatementClassification(queryType, false, false, false, false, false, true);
+        }
+
         return new StatementClassification(queryType, readOnly, mutating, requiresWhere, hasWhere, false);
     }
 
+    /**
+     * True for the statement verbs that never write. Narrow on purpose: it gates the
+     * "malformed, not a mutation" reclassification above, and a write verb the parser
+     * happens to reject (a Postgres DDL form JSqlParser does not model, say) must keep its
+     * existing mutation handling rather than be re-labelled a syntax error.
+     */
+    private boolean isReadOnlyVerb(String queryType) {
+        if (queryType == null) {
+            return false;
+        }
+        return switch (queryType.toUpperCase(Locale.ROOT)) {
+            case "SELECT", "SHOW", "DESCRIBE", "DESC", "EXPLAIN" -> true;
+            default -> false;
+        };
+    }
+
     /**
      * Detects a write hidden inside a statement that parses as a {@link Select}:
      * a data-modifying CTE ({@code WITH x AS (DELETE ...) SELECT ...}, which
@@ -548,7 +595,18 @@ public record StatementClassification(
         boolean mutating,
         boolean requiresWhereClause,
         boolean hasWhereClause,
-        boolean sessionPreamble
+        boolean sessionPreamble,
+        boolean notParseable
     ) {
+        public StatementClassification(
+            String queryType,
+            boolean readOnly,
+            boolean mutating,
+            boolean requiresWhereClause,
+            boolean hasWhereClause,
+            boolean sessionPreamble
+        ) {
+            this(queryType, readOnly, mutating, requiresWhereClause, hasWhereClause, sessionPreamble, false);
+        }
     }
 }

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

@@ -615,4 +615,122 @@ void mcpAdminExplainDrop_isBlockedEvenWhenConfirmed() {
             .isEqualTo(QueryExecutionPolicyException.UNSAFE_MUTATION_BLOCKED);
         assertThat(exception.getMessage()).contains("DROP and TRUNCATE");
     }
+
+    // ── A malformed statement is a syntax error, not a permissions problem ──────────
+    //
+    // Reported from the field: a user pasted a SELECT that still carried the double
+    // quotes it had in source code and was told "Only admins can execute DDL or DML from
+    // the SQL Editor", which reads as a permissions problem and sent them looking for a
+    // role fix. The statement is neither DDL nor DML — it is not valid SQL at all.
+    //
+    // The cause is two keyword heuristics disagreeing: QueryNormalizer.detectQueryType
+    // sanitizes the prefix away and answers SELECT, while the provider's isReadOnlyQuery
+    // strips only comments, still sees the leading quote, and answers false. mutating was
+    // computed as (!readOnly && type != UNKNOWN), so "SELECT" became a mutation.
+
+    private static final String QUOTED_SELECT =
+        "\"select h.id, h.name, h.city, case when h.country = 'India' then 'IN' "
+            + "when h.country = 'United States' then 'US' else 'XX' end country_code from hotel h";
+
+    @Test
+    void selectPastedWithItsSurroundingQuotes_isReportedAsASyntaxErrorNotAPermissionError() {
+        QueryExecutionPolicyException exception = assertThrows(
+            QueryExecutionPolicyException.class,
+            () -> service.enforce(
+                new QueryRequest(QUOTED_SELECT, 10, 30),
+                QueryExecutionContext.editor("analyst", false, false),
+                "mysql"
+            )
+        );
+
+        assertThat(exception.getErrorCode()).isEqualTo(QueryExecutionPolicyException.STATEMENT_NOT_PARSEABLE);
+        assertThat(exception.getMessage()).doesNotContain("Only admins");
+        assertThat(exception.getMessage()).contains("could not parse");
+    }
+
+    /**
+     * An admin gets the same diagnosis rather than a confirmation prompt. Offering to
+     * "confirm this DDL/DML" for a statement nothing managed to classify would invite
+     * confirming past the guard, and the statement cannot run anyway.
+     */
+    @Test
+    void aMalformedSelectIsNotOfferedToAdminsAsAConfirmableMutation() {
+        QueryExecutionPolicyException exception = assertThrows(
+            QueryExecutionPolicyException.class,
+            () -> service.enforce(
+                new QueryRequest(QUOTED_SELECT, 10, 30),
+                QueryExecutionContext.editor("admin", true, false),
+                "mysql"
+            )
+        );
+
+        assertThat(exception.getErrorCode()).isEqualTo(QueryExecutionPolicyException.STATEMENT_NOT_PARSEABLE);
+        assertThat(exception.isRequiresConfirmation()).isFalse();
+    }
+
+    /** Confirmation cannot get a malformed statement through either. */
+    @Test
+    void aConfirmedAdminStillCannotRunAMalformedStatement() {
+        QueryExecutionPolicyException exception = assertThrows(
+            QueryExecutionPolicyException.class,
+            () -> service.enforce(
+                new QueryRequest(QUOTED_SELECT, 10, 30),
+                QueryExecutionContext.editor("admin", true, true),
+                "mysql"
+            )
+        );
+
+        assertThat(exception.getErrorCode()).isEqualTo(QueryExecutionPolicyException.STATEMENT_NOT_PARSEABLE);
+    }
+
+    /**
+     * The reclassification is gated on the detected verb being read-only, so a write the
+     * parser rejects keeps its mutation handling instead of being excused as a typo. This
+     * is the half that stops the fix from becoming a bypass.
+     */
+    @Test
+    void anUnparseableWriteIsStillTreatedAsAMutation() {
+        QueryExecutionPolicyException exception = assertThrows(
+            QueryExecutionPolicyException.class,
+            () -> service.enforce(
+                new QueryRequest("DELETE FROM hotel WHERE (((", 10, 30),
+                QueryExecutionContext.editor("analyst", false, false),
+                "mysql"
+            )
+        );
+
+        assertThat(exception.getErrorCode()).isEqualTo(QueryExecutionPolicyException.EDITOR_MUTATION_FORBIDDEN);
+    }
+
+    /**
+     * The data-modifying CTE this whole guard exists for must not slip through the new
+     * branch: `detectHiddenWrite` vetoes it before the parse result is consulted, so a
+     * malformed variant is still a blocked write rather than a reported typo.
+     */
+    @Test
+    void aMalformedDataModifyingCteIsStillBlockedAsAWrite() {
+        QueryExecutionPolicyException exception = assertThrows(
+            QueryExecutionPolicyException.class,
+            () -> service.enforce(
+                new QueryRequest("WITH x AS (DELETE FROM hotel RETURNING *) SELECT * FROM x WHERE (((", 10, 30),
+                QueryExecutionContext.editor("analyst", false, false),
+                "mysql"
+            )
+        );
+
+        assertThat(exception.getErrorCode()).isEqualTo(QueryExecutionPolicyException.EDITOR_MUTATION_FORBIDDEN);
+    }
+
+    /** The same query without the stray quote is an ordinary read. */
+    @Test
+    void theSameSelectWithoutTheStrayQuoteIsAllowed() {
+        QueryExecutionPolicyService.PolicyDecision decision = service.enforce(
+            new QueryRequest(QUOTED_SELECT.substring(1), 10, 30),
+            QueryExecutionContext.editor("analyst", false, false),
+            "mysql"
+        );
+
+        assertThat(decision.mutating()).isFalse();
+        assertThat(decision.primaryQueryType()).isEqualTo("SELECT");
+    }
 }