fix(security): enforce the chat schema allowlist over the whole statement

[geekypunk]

Authorization bypass in the per-user chat access policy. Live on main since #65 (0aa2b77).

The flaw

The schema allowlist was a partial walk. enforceAllowedSchemas visited only getFromItem() and getJoins() of the outermost PlainSelect — so a table reached through any other syntax position was never enumerated. An allowlist that doesn't enumerate a reference implicitly permits it.

Worse, the dispatch gated everything on:

if (parsed instanceof Select select && select.getPlainSelect() != null)

A UNION parses as a Select whose body is a SetOperationList, so getPlainSelect() returns null, the entire block is skipped, and control falls through to QueryGuardDecision.allow(). That fails open on statement shape — dropping the protected-column and wildcard inspection too, not just the schema check.

Four reachable bypasses

Each has a test that failed before this change with Expected UserDataAccessPolicyException to be thrown, but nothing was thrown — meaning the query was allowed:

payload	evaded
SELECT id FROM marts.orders WHERE total = (SELECT MAX(salary) FROM hr.salaries)	schema scope
SELECT id FROM marts.orders UNION ALL SELECT ssn FROM hr.salaries	schema scope
WITH leaked AS (SELECT ssn FROM hr.salaries) SELECT * FROM leaked	schema scope
SELECT 1 AS x UNION ALL SELECT email FROM customer_profiles	protected columns

Actor: any non-admin with a restricted schema scope who can submit SQL through chat. Read-only enforcement gives no cover — reading another schema is the harm.

Fix

• Schema enforcement runs TablesNamesFinder over the parsed statement, which is exhaustive by construction rather than by remembering to handle each syntax form.
• Column inspection runs over every PlainSelect in the statement — set-operation branches, parenthesised selects, and CTE bodies.
• The old partial walker is deleted, not left beside the new one, so it can't be wired back up.

Unqualified names stay unchecked exactly as before — they resolve through the session search_path, and CTE names aren't tables. That keeps the change scoped to the bypass rather than tightening unrelated behaviour.

Verification

• Wrote all 4 tests first and watched each fail for the right reason (allowed, not errored)
• UserDataAccessPolicyServiceTest 13/13 (4 new + 9 pre-existing)
• 80/80 across the policy and guard suites — McpSqlGuardServiceTest, QueryExecutionPolicyServiceTest, ConnectionChatAccessPolicyServiceTest, ExplainControllerPolicyTest, ChatScopeGuardServiceTest and others, no regressions

Notes for the reviewer

• The pre-existing catch (Exception) fallback (containsDangerousProtectedReference) is not a safety net here: it only fires when parsing throws, and every payload above parses cleanly. It guards malformed SQL, not well-formed attacks.
• Out of scope, worth a follow-up: non-SELECT statements still fall through to allow() in this method. Mutation is blocked upstream by McpSqlGuardService + QueryExecutionContext, so this isn't currently exploitable — but the policy engine failing open on any shape it can't analyse is the same pattern that produced this bug, and inverting it to fail closed deserves its own change with its own tests.

🤖 Generated with Claude Code

[geekypunk]

Re-review of my own PR found it incomplete — pushed a second commit

Correction to the original description. It said column inspection "runs over every PlainSelect in the statement." That was wrong. collectPlainSelects descends into CTE bodies, set-operation branches and parenthesised selects — but never into a select nested inside a PlainSelect's own FROM/JOIN/WHERE/HAVING. Two payloads still reached a protected column and were allowed:

SELECT t.email FROM (SELECT email FROM customer_profiles) t
SELECT id FROM orders WHERE id IN (SELECT email FROM customer_profiles)

That is the same category error this PR set out to remove. The first commit replaced a partial walk with a less partial walk and described it as total. Both payloads also work against main today, so this was an incomplete fix rather than a regression — but the write-up overstated the coverage, which is worse than the gap itself.

Second commit (70a1472) closes it by inverting the failure direction rather than walking harder. Enumerating arbitrary expression trees correctly is precisely what went wrong here the first time, so instead the two views are compared:

• TablesNamesFinder — every table in the statement, exhaustive by construction (a library invariant, not something I have to remember)
• collectDirectTables — what the column inspection actually examined

When a protected table appears in the first set and not the second, the query is refused. A syntax form nobody enumerated now costs a conservative block instead of a silent permit, and future parser features inherit the safe default automatically.

Deliberate tradeoff: some safe nested aggregates over a protected table are now blocked. Refusing costs a rejected query; allowing costs the data.

Verification

• Both new tests written first and watched failing — Expected UserDataAccessPolicyException to be thrown, but nothing was thrown (allowed, not errored)

• UserDataAccessPolicyServiceTest 15/15

• Regression baseline measured, not assumed — same suite selection, stashing the change:

  	tests run	failures	errors
  without this commit	410	8	16
  with it	412	8	16

  Identical failure counts; the +2 are the new tests passing. The 8/16 are pre-existing (CLAUDE.md documents main as carrying ~77 failing backend tests).

The out-of-scope note from the original description still stands: non-SELECT statements continue to fall through to allow() in this method. Not currently exploitable — mutations are blocked upstream by McpSqlGuardService — but it is the same fail-open shape and deserves its own change.

[geekypunk]

Third review pass — found a false positive in my own fail-closed check, fixed in d0c9e8f

Adversarial probing of the previous commit turned up two results.

No security bypass. Identifier quoting and case do not evade the check — SELECT t.email FROM (SELECT email FROM "Customer_Profiles") t is still blocked, because normalizeName strips quotes and lowercases.

But a real false positive. The check matched a protected table against the query's tables by adding both the qualified and bare form to each side and intersecting. That collapsed public.customer_profiles and marts.customer_profiles into the same key, so protecting one refused queries against the other — with an error naming a table the user never referenced.

That matters here specifically: multi-schema support landed in #55 and the acme_erp fixture in #65 ships crm/sales/finance/hr/inventory. Same-named tables across schemas are the expected shape in this product, not a corner case.

The non-obvious part

ConnectionChatAccessPolicyService.qualifyTable() deliberately drops the schema when it is public:

if (schema == null || schema.isBlank() || "public".equalsIgnoreCase(schema)) {
    return table;   // "customer_profiles", never "public.customer_profiles"
}

So a bare protected name is not unknown — it means public.<table>. A bare reference in a query genuinely is unknown, since it resolves via search_path. The two sides carry different information, so matching has to be asymmetric:

reference unqualified -> match on bare name (ambiguous, so block)
protected public      -> a qualified reference must actually say public
both qualified        -> exact match

Every bypass stays closed: an unqualified reference to a protected table is still refused, and hr.salaries still matches a bare salaries.

One test was wrong and got replaced

I'd added a characterization test asserting an unqualified protection should still block another schema. That was written before I found qualifyTable's public-collapsing, and it encoded the wrong belief that a bare protected name is ambiguous. The case actually worth pinning is a bare reference, which the replacement covers.

Verification

• UserDataAccessPolicyServiceTest 17/17, new test watched failing first
• Regression baseline re-measured, same suite selection: 414 run / 8 failures / 16 errors vs 412 / 8 / 16 before — delta is exactly these two tests passing. The 8/16 are pre-existing.