fix(security): escape identifiers in the brain statistics SQL

CardinalityEstimationService.quoteIdentifier wrapped table and column names in
quotes and never doubled an embedded one, which is exactly as protective as
"'" + value + "'" is for a string literal: the attacker's own quote closes the
identifier and everything after it is live SQL. Six String.format sinks consume
it, none with a bind parameter, fed by an unvalidated @PathVariable tableName on
POST /brain/statistics/{connectionId}/tables/{tableName}.

Reproduced against a real PostgreSQL in an isolated schema. The payload

  victim" AS t; DROP TABLE zz_v.probe; SELECT 1 FROM zz_v."victim

produced and executed

  SELECT COUNT(*) FROM zz_v."victim" AS t; DROP TABLE zz_v.probe;
  SELECT 1 FROM zz_v."victim"

with no errors at all: the count returned, the probe table went from present to
gone, and the trailing select returned its rows. The payload contains no slash,
so StrictHttpFirewall does not block it, and this path never reaches
QueryExecutorService so there is no setReadOnly(true) backstop either.

Sweeping every quoter rather than trusting the reported count found four of six
already correct — the three provider classes plus SlackDailyDigestService. The
two that were wrong were both reimplementations in service classes. Rather than
patch both in place, they now delegate to one SqlIdentifier utility: two copies
of a security primitive is the defect, since one gets fixed and the other is
missed.

SqlIdentifier.requireSafe adds a second layer and runs at the top of
collectTableStatistics, ahead of getDecryptedConnection — validating after it
would make a hostile name a credential-use primitive even when the statement
never runs. Its pattern is deliberately permissive enough for v_daily_revenue,
public.orders and tableName$, since a validator that rejects real names is one
the next person deletes. BrainController returns 400 rather than letting the
catch-all report a bad request as a 500.

Verified: tests fail to compile before the utility exists, 10 pass after, and 3
fail when the escaping is stubbed out. Against the live database the vulnerable
quoter dropped the probe table (1 -> 0) and the fixed one did not (1 -> 1),
with PostgreSQL reporting the whole payload as a single missing relation. 104
tests green, compile clean, and the test schema was dropped.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Author: notSumit25

CLAUDE.md

@@ -878,6 +878,36 @@ it against a real database — not a theoretical hardening pass.
   constructs a real `MySQLQueryExecutionProvider`. Do not reintroduce a stubbed
   dialect here; the mock is what let the blocker ship.
 
+### SQL Identifier Quoting
+
+- **Quoting an identifier without doubling the embedded quote is not protection.**
+  `CardinalityEstimationService.quoteIdentifier` returned `"\"" + identifier + "\""` and never
+  escaped, which is exactly as safe as `"'" + value + "'"` is for a string literal. Six
+  `String.format` sinks consumed it with no bind parameter, fed by an unvalidated
+  `@PathVariable tableName` on `POST /brain/statistics/{connectionId}/tables/{tableName}`.
+  Reproduced against a real PostgreSQL: the payload
+  `victim" AS t; DROP TABLE zz_v.probe; SELECT 1 FROM zz_v."victim` executed with **no errors**
+  and the probe table went from 1 row to gone. It carries no `/`, so `StrictHttpFirewall` does
+  not block it, and the path **never reaches `QueryExecutorService`** so there is no
+  `setReadOnly(true)` backstop — `grep setReadOnly` over `src/main/java` still returns one hit,
+  and it is not here.
+- **Use `SqlIdentifier.quote(identifier, dbType)`. Do not write another quoter.** Four of the
+  six quoters in the backend were already correct; the two that were not were both
+  reimplementations in *service* classes, while the *provider* classes got it right — the same
+  "clustered by when it was written" signature the `BrainController` authorization misses had.
+  Two copies of a security primitive is the defect: one gets fixed, the other is missed.
+- **`SqlIdentifier.requireSafe` is the second layer, and it runs before the connection work.**
+  Escaping makes injection impossible but still lets a caller address an object the feature
+  never meant to touch. It is called at the top of `collectTableStatistics`, ahead of
+  `getDecryptedConnection` — validating after it would make a hostile name a credential-use
+  primitive even when the statement never runs. The pattern `[A-Za-z0-9_$.]+` is deliberately
+  permissive enough for `v_daily_revenue` / `public.orders` / `tableName$`; a validator that
+  rejects real names is one the next person deletes. A rejected name is a **400**, not a 500.
+- **A grep is not an audit.** `SlackDailyDigestService:3028` escapes via
+  `identifier.replace(quote, quote + quote)` with a *variable*, so a literal-matching grep
+  reported it vulnerable when it is not. Read the body before believing the pattern.
+  See `docs/security/2026-09-16-sql-injection-quote-identifier.md`.
+
 ### Data Model Rules
 
 - **`mcp_tokens.user_id` is a non-null FK with no cascade.** Deleting a user who holds

backend/src/main/java/com/dbaagent/controller/BrainController.java

@@ -1952,6 +1952,11 @@ public ResponseEntity<List<ColumnStatistics>> collectTableStatistics(
             return ResponseEntity.ok(stats);
         } catch (ResponseStatusException e) {
             throw e;
+        } catch (IllegalArgumentException e) {
+            // A rejected identifier is a bad request, not a server fault. Without this the
+            // catch-all below reports 500 and sends the caller looking for an outage.
+            log.warn("Rejected table name for statistics collection: {}", e.getMessage());
+            return ResponseEntity.badRequest().build();
         } catch (Exception e) {
             log.error("Error collecting table statistics", e);
             return ResponseEntity.internalServerError().build();

backend/src/main/java/com/dbaagent/service/brain/keycolumn/ColumnValueCollectionService.java

@@ -22,6 +22,7 @@
 import java.time.LocalDateTime;
 import java.util.*;
 import java.util.stream.Collectors;
+import com.dbaagent.util.SqlIdentifier;
 
 /**
  * Service for collecting and caching column values, especially for low-cardinality columns.
@@ -447,12 +448,13 @@ private String buildDistinctValuesQuery(String tableName, String columnName, Str
     /**
      * Quote identifier based on database type.
      */
+    /**
+     * Delegates to {@link SqlIdentifier}, which doubles an embedded quote. This copy had the
+     * same missing-escape bug as the one in {@code CardinalityEstimationService}; it is fed
+     * catalog-derived names today, so it was not exploitable, but it was one caller away.
+     */
     private String quoteIdentifier(String identifier, String dbType) {
-        if (dbType != null && dbType.toLowerCase().contains("mysql")) {
-            return "`" + identifier + "`";
-        }
-        // PostgreSQL and others use double quotes
-        return "\"" + identifier + "\"";
+        return SqlIdentifier.quote(identifier, dbType);
     }
 
     /**

backend/src/main/java/com/dbaagent/service/brain/query/CardinalityEstimationService.java

@@ -15,6 +15,7 @@
 import java.time.LocalDateTime;
 import java.util.*;
 import java.util.stream.Collectors;
+import com.dbaagent.util.SqlIdentifier;
 
 /**
  * Brain 2.0: Cardinality Estimation Service
@@ -47,6 +48,11 @@ public class CardinalityEstimationService {
      */
     @Transactional
     public List<ColumnStatistics> collectTableStatistics(String connectionId, String tableName) {
+        // Refused before any connection work. getDecryptedConnection below decrypts stored
+        // credentials and opens a JDBC session, so validating after it would make a hostile
+        // name a credential-use primitive even when the statement never runs — the same
+        // "check before the work, not after" rule the slow-query analytics endpoints learned.
+        SqlIdentifier.requireSafe(tableName);
         log.info("Collecting column statistics for table: {} in connection: {}", tableName, connectionId);
 
         try {
@@ -498,12 +504,17 @@ private String getColumnDataType(JdbcTemplate jdbc, String dbType, String tableN
         }
     }
 
+    /**
+     * Delegates to {@link SqlIdentifier}, which doubles an embedded quote.
+     *
+     * <p>This used to wrap without doubling, so a {@code tableName} path variable carrying a
+     * quote closed the identifier and the rest became live SQL. Verified against a real
+     * PostgreSQL: the injected {@code DROP TABLE} executed, with none of the six
+     * {@code String.format} sinks below using a bind parameter, and this path never reaches
+     * {@code QueryExecutorService} so there is no {@code setReadOnly(true)} backstop either.
+     */
     private String quoteIdentifier(String dbType, String identifier) {
-        if ("postgres".equals(dbType)) {
-            return "\"" + identifier + "\"";
-        } else {
-            return "`" + identifier + "`";
-        }
+        return SqlIdentifier.quote(identifier, dbType);
     }
 
     private boolean isNumericType(String dataType) {

backend/src/main/java/com/dbaagent/util/SqlIdentifier.java

@@ -0,0 +1,78 @@
+package com.dbaagent.util;
+
+import java.util.regex.Pattern;
+
+/**
+ * Quoting and validation for table and column names interpolated into SQL.
+ *
+ * <p>Identifiers cannot be bind parameters, so every dialect's answer is to quote them — and
+ * quoting is only protection if an embedded quote is <em>doubled</em>. Wrapping without
+ * doubling is exactly as safe as {@code "'" + value + "'"} is for a string literal, which is
+ * to say not at all.
+ *
+ * <p>Two services had written their own quoter and both omitted the doubling, while the three
+ * provider classes next to them did it correctly. Verified against a real PostgreSQL rather
+ * than inferred: a {@code tableName} path variable of
+ * {@code victim" AS t; DROP TABLE zz_inj.probe; SELECT 1 FROM zz_inj."victim} reaching
+ * {@code CardinalityEstimationService} produced
+ *
+ * <pre>SELECT COUNT(*) FROM zz_inj."victim" AS t; DROP TABLE zz_inj.probe; SELECT 1 FROM zz_inj."victim"</pre>
+ *
+ * which executed with no errors at all — the count returned, the table was dropped, and the
+ * trailing select returned its rows. That payload contains no {@code /}, so Spring's
+ * {@code StrictHttpFirewall} does not block it.
+ *
+ * <p>This lives in one place on purpose. A security primitive copied into each caller is a
+ * primitive that gets fixed in one copy and missed in the others — the drift the SQL guard is
+ * kept mirrored to avoid, and the reason the two renderers in Agent chat now share one escape.
+ */
+public final class SqlIdentifier {
+
+    /**
+     * What a real table or column name looks like: letters, digits, underscore, dollar, and a
+     * dot for a schema-qualified name. Deliberately permissive enough for the schemas this
+     * product actually meets — a validator that rejected {@code v_daily_revenue} or
+     * {@code order_items_2026} would be deleted by the next person to hit it.
+     */
+    private static final Pattern SAFE_IDENTIFIER = Pattern.compile("[A-Za-z0-9_$.]+");
+
+    private SqlIdentifier() {
+    }
+
+    /**
+     * Quotes an identifier for the dialect, doubling any embedded quote character.
+     *
+     * <p>An unknown or null dialect gets ANSI double quotes. Defaulting to MySQL backticks
+     * would be the riskier guess: a double quote arriving in a backtick-quoted identifier is
+     * inert, while a backtick arriving in a double-quoted one is inert too — but ANSI is what
+     * every non-MySQL dialect here uses, so it is the correct default rather than merely the
+     * safe one.
+     */
+    public static String quote(String identifier, String dbType) {
+        String quote = isMysql(dbType) ? "`" : "\"";
+        return quote + identifier.replace(quote, quote + quote) + quote;
+    }
+
+    /**
+     * Returns the identifier if it could name a real table or column, and throws otherwise.
+     *
+     * <p>{@link #quote} already makes injection impossible; this is the second layer. Escaping
+     * turns a hostile name into a harmless one, but it still lets a caller address an object
+     * the feature never meant to touch, and it leaves a confusing error when the "table" was
+     * never a table. Refusing early says so plainly.
+     */
+    public static String requireSafe(String identifier) {
+        if (identifier == null || identifier.isBlank()) {
+            throw new IllegalArgumentException("Identifier is required");
+        }
+        if (!SAFE_IDENTIFIER.matcher(identifier).matches()) {
+            throw new IllegalArgumentException(
+                "Not a valid table or column name: " + identifier);
+        }
+        return identifier;
+    }
+
+    private static boolean isMysql(String dbType) {
+        return dbType != null && dbType.toLowerCase().contains("mysql");
+    }
+}

backend/src/test/java/com/dbaagent/util/SqlIdentifierTest.java

@@ -0,0 +1,174 @@
+package com.dbaagent.util;
+
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.*;
+
+/**
+ * Identifier quoting for the SQL that cannot be parameterised.
+ *
+ * <p>Table and column names cannot be bind parameters, so they are interpolated into SQL by
+ * hand. {@code CardinalityEstimationService.quoteIdentifier} wrapped them in quotes and never
+ * doubled an embedded one, which is no protection at all — the same way
+ * {@code "'" + value + "'"} is no protection for a string literal.
+ *
+ * <p>Verified against a real PostgreSQL, not inferred. A {@code tableName} path variable of
+ * {@code victim" AS t; DROP TABLE zz_inj.probe; SELECT 1 FROM zz_inj."victim} produced
+ *
+ * <pre>SELECT COUNT(*) FROM zz_inj."victim" AS t; DROP TABLE zz_inj.probe; SELECT 1 FROM zz_inj."victim"</pre>
+ *
+ * which ran with no errors: the count returned, the table was dropped (it existed before and
+ * did not after), and the trailing select returned its rows. The payload carries no {@code /},
+ * so Spring's {@code StrictHttpFirewall} does not stand in its way.
+ */
+class SqlIdentifierTest {
+
+    // ── the injection that was proven to execute ──────────────────────────────
+
+    @Test
+    void doublesAnEmbeddedDoubleQuoteSoTheIdentifierCannotBeClosed() {
+        String payload = "victim\" AS t; DROP TABLE zz_inj.probe; SELECT 1 FROM zz_inj.\"victim";
+
+        String quoted = SqlIdentifier.quote(payload, "postgres");
+
+        assertEquals(
+            "\"victim\"\" AS t; DROP TABLE zz_inj.probe; SELECT 1 FROM zz_inj.\"\"victim\"",
+            quoted);
+        // the whole payload is now one identifier: no unescaped quote can terminate it
+        assertEquals(1, countUnescapedQuotes(quoted, '"'),
+            "an unescaped quote inside the body would end the identifier early: " + quoted);
+    }
+
+    @Test
+    void doublesAnEmbeddedBacktickForMysql() {
+        String payload = "victim` ; DROP TABLE probe; SELECT 1 FROM `victim";
+
+        String quoted = SqlIdentifier.quote(payload, "mysql");
+
+        assertEquals("`victim`` ; DROP TABLE probe; SELECT 1 FROM ``victim`", quoted);
+        assertEquals(1, countUnescapedQuotes(quoted, '`'), quoted);
+    }
+
+    /**
+     * The sinks build {@code SELECT COUNT(*) FROM %s}, so the property that matters is that the
+     * whole payload lands inside one identifier rather than becoming a second statement.
+     *
+     * <p>Asserted on the parse, not on the text. A first version of this test used the regex
+     * {@code .*"\s*;.*} and failed on correct output, because {@code "";} is an <em>escaped</em>
+     * quote followed by a semicolon <em>inside</em> the identifier — textually close to a
+     * terminator and semantically its opposite. That is the same confusion the vulnerable
+     * quoter made, so it is worth not repeating in the test.
+     *
+     * <p>Confirmed against a real PostgreSQL: this exact SQL answers
+     * {@code ERROR: relation "t"; DROP TABLE zz_v.probe; --" does not exist} — the server read
+     * the payload as one table name — and the probe table it names was still there afterwards,
+     * where the unescaped form had dropped it.
+     */
+    @Test
+    void theInjectedStatementCollapsesIntoASingleIdentifier() {
+        String payload = "t\"; DROP TABLE probe; --";
+
+        String quoted = SqlIdentifier.quote(payload, "postgres");
+        String sql = "SELECT COUNT(*) FROM " + quoted;
+
+        assertEquals("SELECT COUNT(*) FROM \"t\"\"; DROP TABLE probe; --\"", sql);
+        assertTrue(sql.endsWith(quoted), "the identifier must be the whole tail of the statement");
+        assertEquals(1, countUnescapedQuotes(quoted, '"'),
+            "only the closing quote may be unescaped, or the identifier ends early: " + quoted);
+    }
+
+    // ── ordinary identifiers must keep working ────────────────────────────────
+
+    @Test
+    void leavesAnOrdinaryIdentifierAloneApartFromTheQuotes() {
+        assertEquals("\"orders\"", SqlIdentifier.quote("orders", "postgres"));
+        assertEquals("\"total_amount\"", SqlIdentifier.quote("total_amount", "postgres"));
+        assertEquals("`orders`", SqlIdentifier.quote("orders", "mysql"));
+    }
+
+    @Test
+    void picksTheQuoteCharacterFromTheDialect() {
+        assertEquals("`t`", SqlIdentifier.quote("t", "mysql"));
+        assertEquals("`t`", SqlIdentifier.quote("t", "MySQL"));
+        assertEquals("\"t\"", SqlIdentifier.quote("t", "postgres"));
+        assertEquals("\"t\"", SqlIdentifier.quote("t", "postgresql"));
+    }
+
+    /**
+     * An unknown or null dialect must not fall through to "no quoting". Postgres double quotes
+     * are the ANSI form and the safe default; guessing MySQL backticks for an unknown dialect
+     * would be the riskier direction.
+     */
+    @Test
+    void defaultsToAnsiQuotingForAnUnknownDialect() {
+        assertEquals("\"t\"", SqlIdentifier.quote("t", null));
+        assertEquals("\"t\"", SqlIdentifier.quote("t", "oracle"));
+        assertEquals("\"t\"", SqlIdentifier.quote("t", ""));
+    }
+
+    // ── rejecting what should never reach SQL at all ──────────────────────────
+
+    /**
+     * Escaping alone makes injection impossible but still lets a caller name an identifier the
+     * feature never meant to touch. {@code requireSafe} is the second layer: the brain's
+     * statistics paths only ever address real tables and columns, so anything that cannot be
+     * one is refused before a statement is built.
+     */
+    @Test
+    void refusesAnIdentifierCarryingSqlSyntax() {
+        for (String bad : new String[] {
+            "victim\" AS t; DROP TABLE probe; --",
+            "t; DROP TABLE probe",
+            "t--comment",
+            "t/*x*/",
+            "t'or'1'='1"
+        }) {
+            assertThrows(IllegalArgumentException.class,
+                () -> SqlIdentifier.requireSafe(bad), "should refuse: " + bad);
+        }
+    }
+
+    @Test
+    void refusesBlankAndNull() {
+        assertThrows(IllegalArgumentException.class, () -> SqlIdentifier.requireSafe(null));
+        assertThrows(IllegalArgumentException.class, () -> SqlIdentifier.requireSafe("   "));
+    }
+
+    /**
+     * Real schemas carry all of these. A validator that refused them would break the feature it
+     * is protecting, which is the usual reason such a check gets deleted later.
+     */
+    @Test
+    void acceptsTheIdentifiersRealSchemasActuallyUse() {
+        for (String ok : new String[] {
+            "orders",
+            "total_amount",
+            "Orders",
+            "order_items_2026",
+            "public.orders",
+            "_private",
+            "v_daily_revenue",
+            "tableName$"
+        }) {
+            assertEquals(ok, SqlIdentifier.requireSafe(ok), "should accept: " + ok);
+        }
+    }
+
+    @Test
+    void requireSafeReturnsTheIdentifierSoItComposesWithQuote() {
+        assertEquals("\"orders\"",
+            SqlIdentifier.quote(SqlIdentifier.requireSafe("orders"), "postgres"));
+    }
+
+    /** Counts quote characters that are not part of a doubled pair. */
+    private static int countUnescapedQuotes(String quoted, char q) {
+        String body = quoted.substring(1, quoted.length() - 1);
+        int unescaped = 0;
+        for (int i = 0; i < body.length(); i++) {
+            if (body.charAt(i) != q) continue;
+            if (i + 1 < body.length() && body.charAt(i + 1) == q) { i++; continue; }
+            unescaped++;
+        }
+        return unescaped + 1; // the closing quote
+    }
+}