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

[notSumit25]

The problem

Table and column names can't be bind parameters, so they're interpolated by hand and protected by quoting. CardinalityEstimationService quoted like this:

private String quoteIdentifier(String dbType, String identifier) {
    if ("postgres".equals(dbType)) {
        return "\"" + identifier + "\"";   // wraps, never doubles
    } else {
        return "`" + identifier + "`";
    }
}

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 String.format sinks consume it, none with a bind parameter, fed by an unvalidated @PathVariable tableName.

⚠️ Correction from hands-on QA (see comment)

Hands-on QA against the running stack found that the ;-based DROP TABLE payload below is not reachable through this HTTP endpoint — Spring's StrictHttpFirewall rejects a semicolon in a path segment with a 400 before the controller runs (ord;ers → 400, while ord"ers → 200). The direct-psql reproduction is accurate for the quoter, but the HTTP exploit is single-statement quote breakout, which needs no semicolon:

tableName = orders" AS x, same input	Result
Unpatched	200, [] — quote broke out, "orders" AS x executed as an aliased table
Patched	400 — quote doubled, requireSafe rejected it

The vulnerability is real and the fix is correct; only the multi-statement ; framing below is overstated. Treat the DROP TABLE example as a quoter-level demonstration, not an HTTP-reachable one.

Reproduced against a live PostgreSQL

Payload as the tableName path variable (isolated zz_v schema, created and dropped for the test):

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"

probe before: 1
DROP TABLE
probe after : 0

No errors at all — count returned, table dropped, trailing select returned rows. The payload contains no /, so StrictHttpFirewall doesn't block it.

No second line of defence on this path: it never reaches QueryExecutorService, so no setReadOnly(true), no policy service, no row cap. grep setReadOnly over src/main/java returns exactly one hit — not here.

It was an outlier, not a convention

I swept every quoter rather than trusting the audit's count:

Quoter	Escapes?
PostgresSamplingProvider:21	✅
MySQLSamplingProvider:21	✅
PostgresIntrospectionProvider:953	✅
SlackDailyDigestService:3028	✅
ColumnValueCollectionService:450	❌
CardinalityEstimationService:501	❌

The three provider classes all escape correctly; the service classes that reimplemented the primitive later both got it wrong — the same "clustered by when it was written" signature as the BrainController authorization misses.

A grep is not an audit. SlackDailyDigestService escapes via identifier.replace(quote, quote + quote) with a variable, so my first literal-matching grep flagged it as vulnerable. Reading it settled that it's safe. Pattern matching produces false positives as readily as false negatives.

The fix

One shared SqlIdentifier utility; both broken copies delegate to it. Centralised rather than patched in place — two copies of a security primitive is the defect, since one gets fixed and the other missed (same reason the SQL guard is kept mirrored Java↔JS).

Second layer, 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 [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. BrainController now returns 400, not 500: a bad request shouldn't read as an outage.

Verification

Step	Result
Before the utility existed (RED)	compile failure — symbol not found
After the fix (GREEN)	10 pass
Escaping stubbed out (mutation)	3 fail — 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 is the proof of why it's 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. zz_v was dropped; DB back to its prior state.

A test I got wrong first

My first assertion was !sql.matches(".*\"\\s*;.*") — it failed against correct output, because ""; is an escaped quote followed by a semicolon inside the identifier: textually close to a terminator, semantically its opposite. That's precisely the confusion the vulnerable quoter made. It now asserts on the parse property, backed by the live result above.

Residual work

• ColumnValueCollectionService is fed catalog-derived names today so it wasn't exploitable — but it was one caller away, which is why it's 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.

Write-up: docs/security/2026-09-16-sql-injection-quote-identifier.md

🤖 Generated with Claude Code

[notSumit25]

Hands-on QA against the live stack — 11/11 PASS, with one correction to my exploit claim

QA'd against the real running stack: patched backend (:8098) vs unpatched main (:8080), same live Postgres, real admin + analyst sessions via the actual login endpoint (hashes set directly in users, then restored byte-for-byte).

⚠️ Correction: the DROP TABLE; payload is firewall-blocked over HTTP

The PR's headline payload uses a ;, and Spring's StrictHttpFirewall rejects a semicolon in a path segment with a 400 before the controller runs — I verified this with a single-character probe:

char in tableName	HTTP
ord"ers (quote)	200 — reaches the app
ord;ers (semicolon)	400 — blocked at the edge

So a reader pasting my exact victim"; DROP TABLE ... payload into the HTTP route sees a 400 even on the unpatched backend, and could wrongly conclude the report was bogus. My direct-psql reproduction was correct for the quoter, but overstated the HTTP exploit path. Multi-statement chaining via ; is not reachable through this endpoint.

The vulnerability is still real — via single-statement quote breakout, which needs no semicolon. Proven with identical input on both backends:

tableName = orders" AS x	Result
UNPATCHED :8080	HTTP 200, [] — quote broke out, "orders" AS x parsed as an aliased table, query executed
PATCHED :8098	HTTP 400 — quote doubled to "orders"" AS x", requireSafe rejected it

nonexistent_xyz" AS x also returns 200 unpatched — the attacker controls the entire FROM clause through the breakout, independent of any real table name.

All 11 scenarios

ID	Scenario	Result
S1	Injection refused on patched (probe survives)	PASS
S2	Same breakout executes on unpatched (200 vs 400)	PASS
S3	Valid orders → 200, 19 stats, DB grew to 39 rows	PASS
S4	v_daily_revenue, order_items_2026, Orders, _private all 200	PASS
S5	Rejected name → 400, not 500	PASS
S6	'or'1, --x, /*x*/, WHERE 1=1, ;DROP all 400	PASS
S7	Rejection logs before any decrypt — WARN Rejected table name 2ms after entry, no JDBC line	PASS
S8	Adjacent brain endpoints (GET /statistics/{id}, /high-cardinality) still 200	PASS
S9	no-auth → 401; analyst on ungranted conn → 403; analyst injection on granted conn → 400	PASS
S10	GET returns all 39 stat entries consistently	PASS
S11	Collect customers ×2 → row count stable at 15 (idempotent upsert)	PASS

One thing I double-checked rather than reported as a bug

analyst initially returned 200 on admin's connection, which looked like a broken-access gap — but connection_access_grant shows analyst holds an explicit CHAT_EDITOR grant on it, so 200 is correct. Against a connection with no grant, analyst gets 403. Authorization is intact.

Environment

• Health via logs (Started DbaAgentApplication) + real requests (200/401), not docker ps.
• No UI: collectColumnStatistics exists in client.js but zero components call it — API-only surface, so verification is curl + psql.
• DB restored: both password hashes byte-identical to backup, zz_qa dropped, patched container removed. column_statistics (0→39) left intact — that's the endpoint's legitimate output on real tables, not test pollution.

Verdict: READY — zero blocking issues. The PR body should soften the DROP TABLE/; framing to single-statement quote-breakout, which is what's actually reachable through the endpoint.