diff --git a/CLAUDE.md b/CLAUDE.md index 52aac16..a92f761 100644 --- a/CLAUDE.md +++ b/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 diff --git a/backend/src/main/java/com/dbaagent/controller/BrainController.java b/backend/src/main/java/com/dbaagent/controller/BrainController.java index 6c1e8e9..5979616 100644 --- a/backend/src/main/java/com/dbaagent/controller/BrainController.java +++ b/backend/src/main/java/com/dbaagent/controller/BrainController.java @@ -1952,6 +1952,11 @@ public ResponseEntity> 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(); diff --git a/backend/src/main/java/com/dbaagent/service/brain/keycolumn/ColumnValueCollectionService.java b/backend/src/main/java/com/dbaagent/service/brain/keycolumn/ColumnValueCollectionService.java index 17a57e2..6ba4268 100644 --- a/backend/src/main/java/com/dbaagent/service/brain/keycolumn/ColumnValueCollectionService.java +++ b/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); } /** diff --git a/backend/src/main/java/com/dbaagent/service/brain/query/CardinalityEstimationService.java b/backend/src/main/java/com/dbaagent/service/brain/query/CardinalityEstimationService.java index 18cb2d1..56183f0 100644 --- a/backend/src/main/java/com/dbaagent/service/brain/query/CardinalityEstimationService.java +++ b/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 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. + * + *

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) { diff --git a/backend/src/main/java/com/dbaagent/util/SqlIdentifier.java b/backend/src/main/java/com/dbaagent/util/SqlIdentifier.java new file mode 100644 index 0000000..5526028 --- /dev/null +++ b/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. + * + *

Identifiers cannot be bind parameters, so every dialect's answer is to quote them — and + * quoting is only protection if an embedded quote is doubled. Wrapping without + * doubling is exactly as safe as {@code "'" + value + "'"} is for a string literal, which is + * to say not at all. + * + *

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

SELECT COUNT(*) FROM zz_inj."victim" AS t; DROP TABLE zz_inj.probe; SELECT 1 FROM zz_inj."victim"
+ * + * 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. + * + *

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. + * + *

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. + * + *

{@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"); + } +} diff --git a/backend/src/test/java/com/dbaagent/util/SqlIdentifierTest.java b/backend/src/test/java/com/dbaagent/util/SqlIdentifierTest.java new file mode 100644 index 0000000..868b324 --- /dev/null +++ b/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. + * + *

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. + * + *

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

SELECT COUNT(*) FROM zz_inj."victim" AS t; DROP TABLE zz_inj.probe; SELECT 1 FROM zz_inj."victim"
+ * + * 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. + * + *

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 escaped + * quote followed by a semicolon inside 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. + * + *

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 + } +} diff --git a/docs/security/2026-09-16-sql-injection-quote-identifier.md b/docs/security/2026-09-16-sql-injection-quote-identifier.md new file mode 100644 index 0000000..48bace1 --- /dev/null +++ b/docs/security/2026-09-16-sql-injection-quote-identifier.md @@ -0,0 +1,174 @@ +# SQL injection through an unescaped identifier quoter + +*Found 2026-09-10 in a repository-wide security audit; reproduced against a live PostgreSQL +2026-09-16. Severity: high.* + +## What was wrong + +Table and column names cannot be bind parameters, so they are interpolated into SQL by hand +and protected by quoting. `CardinalityEstimationService` quoted them like this: + +```java +private String quoteIdentifier(String dbType, String identifier) { + if ("postgres".equals(dbType)) { + return "\"" + identifier + "\""; + } else { + return "`" + identifier + "`"; + } +} +``` + +It wraps, and never **doubles** an embedded quote. That 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 sinks consume it, none with a bind parameter — `CardinalityEstimationService.java:166`, +`:171`, `:201`, `:241`, `:298`, `:326`, all `String.format` into `jdbc.queryForObject` / +`queryForList`. + +The tainted value is a path variable, unvalidated from the edge: + +``` +POST /api/brain/statistics/{connectionId}/tables/{tableName} + BrainController.java:1947 @PathVariable String tableName + -> cardinalityEstimationService.collectTableStatistics(connectionId, tableName) + -> quoteIdentifier(dbType, tableName) + -> String.format("SELECT COUNT(*) FROM %s", quotedTable) +``` + +## Reproduced, not inferred + +In an isolated `zz_v` schema created and dropped for the test, with the payload passed as the +`tableName` path variable: + +``` +victim" AS t; DROP TABLE zz_v.probe; SELECT 1 FROM zz_v."victim +``` + +the quoter produced, and PostgreSQL executed: + +```sql +SELECT COUNT(*) FROM zz_v."victim" AS t; DROP TABLE zz_v.probe; SELECT 1 FROM zz_v."victim" +``` + +``` +probe before: 1 +DROP TABLE +probe after : 0 +``` + +No errors at all — the count returned, the table was dropped, the trailing select returned its +rows. The payload contains no `/`, so Spring's `StrictHttpFirewall` (which rejects `%2F`) does +not stand in its way. + +**HTTP exploit path — corrected by hands-on QA.** The reproduction above uses `psql` and a +`;`-separated `DROP TABLE`, which demonstrates the *quoter* flaw exactly. But over the real HTTP +endpoint, Spring's `StrictHttpFirewall` rejects a `;` in a path segment (400) before the +controller runs, so multi-statement chaining is **not** reachable that way. The double quote +*is* allowed through, so the HTTP-reachable exploit is a **single-statement quote breakout**: +`tableName` = `orders" AS x` returned **200** on the unpatched backend (the quote closed the +identifier and `"orders" AS x` executed as an aliased table reference) and **400** on the fixed +one. `nonexistent_xyz" AS x` also returned 200 unpatched, so the attacker controls the whole +FROM clause regardless of any real table name. The fix blocks both the quote breakout and, +defensively, the semicolon form. + +**There is no second line of defence on this path.** It never reaches +`QueryExecutorService`, so it gets no `connection.setReadOnly(true)`, no policy service and no +row cap — a `grep` for `setReadOnly` over `src/main/java` returns exactly one hit, and it is +not here. + +## It was an outlier, not a convention + +Sweeping every identifier quoter in the backend rather than trusting the reported count: + +| Quoter | Escapes? | +|---|---| +| `PostgresSamplingProvider:21` | yes | +| `MySQLSamplingProvider:21` | yes | +| `PostgresIntrospectionProvider:953` | yes | +| `SlackDailyDigestService:3028` | yes | +| `ColumnValueCollectionService:450` | **no** | +| `CardinalityEstimationService:501` | **no** | + +Four of six were already correct. The pattern is worth noting: the three **provider** classes +— written by whoever was thinking about dialects — all escape. The **service** classes that +reimplemented the same primitive later got it wrong, which is the same "clustered by when it +was written" signature the `BrainController` authorization misses had. + +`SlackDailyDigestService` is a near miss worth recording: it escapes via +`identifier.replace(quote, quote + quote)` with a *variable* rather than a literal, so a first +grep flagged it as vulnerable. Reading it settled that it is safe. A pattern-matched audit +produces false positives as readily as false negatives. + +## The fix + +One shared `SqlIdentifier` utility, with both broken copies delegating to it: + +```java +public static String quote(String identifier, String dbType) { + String quote = isMysql(dbType) ? "`" : "\""; + return quote + identifier.replace(quote, quote + quote) + quote; +} +``` + +Centralised rather than patched in place, because two copies of a security primitive is the +defect: one gets fixed and the other is missed. The same reason the SQL guard is kept mirrored +between Java and JS, and the two Agent-chat renderers now share one escape. + +A second layer refuses what should never reach SQL at all: + +```java +private static final Pattern SAFE_IDENTIFIER = Pattern.compile("[A-Za-z0-9_$.]+"); +``` + +Escaping alone makes injection impossible but still lets a caller address an object the +feature never meant to touch. `requireSafe` runs at the **top** of `collectTableStatistics`, +before `getDecryptedConnection` — validating after it would make a hostile name a +credential-use primitive even when the statement never runs, which is the "check before the +work, not after" rule the slow-query analytics endpoints already learned. + +The pattern is deliberately permissive enough for real schemas (`v_daily_revenue`, +`order_items_2026`, `public.orders`, `tableName$`). A validator that rejects legitimate names +is one the next person deletes. + +`BrainController` now returns **400** for a rejected name rather than letting the catch-all +report 500 — a bad request should not read as an outage. + +## Verification + +| Step | Result | +|---|---| +| Tests before the utility existed (RED) | compilation failure — symbol not found | +| Tests after the fix (GREEN) | 10 pass | +| Escaping stubbed out (mutation) | 3 fail — the tests guard the fix | +| Live DB, vulnerable quoter | `DROP TABLE` ran; probe **1 → 0** | +| Live DB, fixed quoter | refused; probe **1 → 1** | +| Backend suites | **104 tests, 0 failures** | +| `mvn compile` | clean | + +The fixed path's own error message is the proof of why it is safe: + +``` +ERROR: relation "zz_v.victim" AS t; DROP TABLE zz_v.probe; SELECT 1 FROM zz_v."victim" does not exist +``` + +PostgreSQL read the whole payload as **one table name**, not three statements. + +The `zz_v` schema created for this test was dropped; the database is back to its prior state. + +## A note on the test that was wrong first + +The first version of `theInjectedStatementCollapsesIntoASingleIdentifier` asserted +`!sql.matches(".*\"\\s*;.*")` and **failed against correct output**, because `"";` is an +*escaped* quote followed by a semicolon *inside* the identifier — textually close to a +terminator and semantically its opposite. That is precisely the confusion the vulnerable +quoter made. It now asserts on the parse property (only the closing quote is unescaped), +backed by the live-database result above. + +## Residual work + +- `ColumnValueCollectionService` is fed catalog-derived names today, so it was not exploitable + — but it was one caller away, which is why it was fixed rather than noted. +- Other `String.format`-built SQL in the brain services should be swept for the same shape. + This PR fixes the proven-exploitable path and the identical copy beside it; a broader sweep + is a separate change with its own blast radius.