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

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
+    }
+}