Skip to content

Commit 70a1472

Browse files
geekypunkclaude
andcommitted
fix(security): fail closed when a protected table sits where inspection cannot see
The first commit fixed schema scoping completely but left column protection a partial walk, and the PR text claimed otherwise. 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: SELECT t.email FROM (SELECT email FROM customer_profiles) t SELECT id FROM orders WHERE id IN (SELECT email FROM customer_profiles) Both were allowed. Same category error the first commit set out to remove: an allowlist enforced by a walk is only as complete as the walk. Rather than attempt an exhaustive traversal of arbitrary expression trees -- getting that wrong is what caused this in the first place -- the two views are compared. TablesNamesFinder enumerates every table in the statement and is exhaustive by construction; collectDirectTables reports what the column inspection actually examined. When a protected table appears in the first set and not the second, the query is refused. That inverts the failure direction. A syntax form nobody enumerated now costs a conservative block instead of a silent permit, and any future parser feature inherits the safe default without anyone remembering to handle it. The tradeoff is deliberate: some safe nested aggregates over a protected table are now blocked. Refusing costs a rejected query; allowing costs the data. Verified: 15/15 in UserDataAccessPolicyServiceTest (2 new, both watched failing first). Regression baseline on the same suite selection is unchanged -- 8 failures / 16 errors before and after, all pre-existing, with the 2 added tests passing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent a547a07 commit 70a1472

2 files changed

Lines changed: 108 additions & 0 deletions

File tree

backend/src/main/java/com/dbaagent/service/UserDataAccessPolicyService.java

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,7 @@ public QueryGuardDecision enforcePreExecution(
127127
// how a subquery, UNION branch, or CTE body reached a schema
128128
// outside the caller's scope.
129129
enforceAllowedSchemas(parsed, policy.allowedSchemas());
130+
assertProtectedTablesAreInspectable(parsed, collectPlainSelects(select), protectedObjects);
130131
// Likewise inspect every branch. Gating this on getPlainSelect()
131132
// != null skipped protection entirely for a SetOperationList,
132133
// because a UNION's body is not a PlainSelect.
@@ -337,6 +338,78 @@ private boolean mentionsProtectedTables(String normalized, ConnectionChatAccessP
337338
return policy.impactedTables().stream().anyMatch(table -> normalized.contains(table.toLowerCase(Locale.ROOT)));
338339
}
339340

341+
/**
342+
* Fails closed on statement shapes inspection cannot reach.
343+
*
344+
* TablesNamesFinder sees every table in the statement; collectPlainSelects
345+
* deliberately does not descend into a select nested inside FROM/JOIN/WHERE/
346+
* HAVING, because enumerating arbitrary expression trees correctly is the very
347+
* thing that went wrong here the first time. So instead of trying harder to
348+
* walk, compare the two: when a protected table is referenced somewhere the
349+
* column inspection could not examine, refuse the query.
350+
*
351+
* A syntax form we failed to enumerate must never become an implicit permit --
352+
* that is exactly how a subquery, a UNION branch and a CTE body each evaded
353+
* the schema allowlist. Refusing costs a conservative block on some safe
354+
* nested aggregates; allowing costs the data.
355+
*/
356+
private void assertProtectedTablesAreInspectable(
357+
Statement statement,
358+
List<PlainSelect> inspectedBranches,
359+
Map<String, ConnectionChatAccessPolicyService.ProtectionDescriptor> protectedObjects
360+
) {
361+
if (protectedObjects == null || protectedObjects.isEmpty()) {
362+
return;
363+
}
364+
Set<String> referenced = new LinkedHashSet<>();
365+
for (String name : new TablesNamesFinder<>().getTables(statement)) {
366+
addNameForms(name, referenced);
367+
}
368+
Set<String> inspected = new LinkedHashSet<>();
369+
for (PlainSelect branch : inspectedBranches) {
370+
collectDirectTables(branch, inspected);
371+
}
372+
for (ConnectionChatAccessPolicyService.ProtectionDescriptor descriptor : protectedObjects.values()) {
373+
Set<String> forms = new LinkedHashSet<>();
374+
addNameForms(descriptor.qualifiedTableName(), forms);
375+
boolean isReferenced = forms.stream().anyMatch(referenced::contains);
376+
boolean wasInspected = forms.stream().anyMatch(inspected::contains);
377+
if (isReferenced && !wasInspected) {
378+
throw new UserDataAccessPolicyException(
379+
"This query reaches restricted data through a nested query DeepSQL cannot fully verify, so it was blocked before execution.",
380+
"POLICY_SQL_BLOCKED"
381+
);
382+
}
383+
}
384+
}
385+
386+
/** Adds both the qualified name and its bare table part, so schema.t matches t. */
387+
private void addNameForms(String name, Set<String> out) {
388+
if (name == null || name.isBlank()) {
389+
return;
390+
}
391+
String normalized = normalizeName(name);
392+
out.add(normalized);
393+
int dot = normalized.lastIndexOf('.');
394+
if (dot > 0 && dot < normalized.length() - 1) {
395+
out.add(normalized.substring(dot + 1));
396+
}
397+
}
398+
399+
/** Tables named directly in this branch's FROM/JOIN -- what inspection actually saw. */
400+
private void collectDirectTables(PlainSelect select, Set<String> out) {
401+
if (select.getFromItem() instanceof Table table) {
402+
addNameForms(table.getFullyQualifiedName(), out);
403+
}
404+
if (select.getJoins() != null) {
405+
for (Join join : select.getJoins()) {
406+
if (join.getRightItem() instanceof Table table) {
407+
addNameForms(table.getFullyQualifiedName(), out);
408+
}
409+
}
410+
}
411+
}
412+
340413
/**
341414
* Collects every PlainSelect in a statement: the top level, each branch of a
342415
* set operation (UNION/INTERSECT/EXCEPT), parenthesised selects, and every

backend/src/test/java/com/dbaagent/service/UserDataAccessPolicyServiceTest.java

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -166,6 +166,41 @@ private UserDataAccessPolicyException assertSchemaScopeBlocks(String sql) {
166166
// reached through any other syntax position was never enumerated and the
167167
// allowlist silently permitted it.
168168

169+
// Column inspection cannot reach a select nested inside FROM/JOIN/WHERE, so a
170+
// protected table referenced there must be refused rather than implicitly allowed.
171+
172+
@Test
173+
void enforcePreExecution_blocksProtectedTableInsideDerivedTable() {
174+
when(policyService.resolveEffectivePolicy("conn-1", "analyst", false)).thenReturn(policy());
175+
176+
UserDataAccessPolicyException exception = assertThrows(
177+
UserDataAccessPolicyException.class,
178+
() -> service.enforcePreExecution(
179+
"conn-1",
180+
new QueryRequest("SELECT t.email FROM (SELECT email FROM customer_profiles) t", null, null),
181+
new QueryExecutionContext(QueryExecutionOrigin.CHAT, QueryExecutionContext.MutationMode.READ_ONLY_ONLY, "analyst", false, false)
182+
)
183+
);
184+
185+
assertThat(exception.getErrorCode()).isEqualTo("POLICY_SQL_BLOCKED");
186+
}
187+
188+
@Test
189+
void enforcePreExecution_blocksProtectedTableInsideWhereSubquery() {
190+
when(policyService.resolveEffectivePolicy("conn-1", "analyst", false)).thenReturn(policy());
191+
192+
UserDataAccessPolicyException exception = assertThrows(
193+
UserDataAccessPolicyException.class,
194+
() -> service.enforcePreExecution(
195+
"conn-1",
196+
new QueryRequest("SELECT id FROM orders WHERE id IN (SELECT email FROM customer_profiles)", null, null),
197+
new QueryExecutionContext(QueryExecutionOrigin.CHAT, QueryExecutionContext.MutationMode.READ_ONLY_ONLY, "analyst", false, false)
198+
)
199+
);
200+
201+
assertThat(exception.getErrorCode()).isEqualTo("POLICY_SQL_BLOCKED");
202+
}
203+
169204
@Test
170205
void enforcePreExecution_blocksForbiddenSchemaInsideWhereSubquery() {
171206
assertThat(assertSchemaScopeBlocks(

0 commit comments

Comments
 (0)