Skip to content

Commit bb8b895

Browse files
geekypunkclaude
andcommitted
fix(security): stop the fail-closed check conflating same-named tables across schemas
The previous commit matched a protected table against a query's tables by adding both the qualified name and its bare part to each side, then intersecting. That collapsed public.customer_profiles and marts.customer_profiles to the same key, so protecting one refused queries against the other -- with a message naming a table the user never referenced. This product added multi-schema support in #55 and an acme_erp fixture with crm/sales/finance/hr/inventory in #65, so same-named tables across schemas are the expected shape here, not a corner case. Over-blocking is the safe direction, which is exactly why it would have survived review and surfaced later as unexplained refusals. Matching is now asymmetric, because the two sides carry different information. ConnectionChatAccessPolicyService.qualifyTable() drops the schema when it is "public", so a bare PROTECTED name means public.<table> -- it is not unknown. A bare REFERENCE in a query is genuinely unknown: it resolves through the session search_path and could be any schema. 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". Also replaces a characterization test asserting the opposite. It was written before qualifyTable's public-collapsing was discovered and encoded the wrong belief that a bare protected name is ambiguous; the case genuinely worth pinning is a bare reference, which it now covers. Verified: 17/17 in UserDataAccessPolicyServiceTest. Regression baseline on the same suite selection unchanged at 8 failures / 16 errors (414 run vs 412 before, the delta being these two tests passing). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 70a1472 commit bb8b895

2 files changed

Lines changed: 80 additions & 16 deletions

File tree

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

Lines changed: 44 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -363,17 +363,18 @@ private void assertProtectedTablesAreInspectable(
363363
}
364364
Set<String> referenced = new LinkedHashSet<>();
365365
for (String name : new TablesNamesFinder<>().getTables(statement)) {
366-
addNameForms(name, referenced);
366+
if (name != null && !name.isBlank()) {
367+
referenced.add(normalizeName(name));
368+
}
367369
}
368370
Set<String> inspected = new LinkedHashSet<>();
369371
for (PlainSelect branch : inspectedBranches) {
370372
collectDirectTables(branch, inspected);
371373
}
372374
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);
375+
String protectedName = descriptor.qualifiedTableName();
376+
boolean isReferenced = referenced.stream().anyMatch(name -> namesMatch(protectedName, name));
377+
boolean wasInspected = inspected.stream().anyMatch(name -> namesMatch(protectedName, name));
377378
if (isReferenced && !wasInspected) {
378379
throw new UserDataAccessPolicyException(
379380
"This query reaches restricted data through a nested query DeepSQL cannot fully verify, so it was blocked before execution.",
@@ -383,28 +384,55 @@ private void assertProtectedTablesAreInspectable(
383384
}
384385
}
385386

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;
387+
/**
388+
* Does a query's table reference name the protected table?
389+
*
390+
* Asymmetric on purpose, because the two sides carry different information.
391+
* ConnectionChatAccessPolicyService.qualifyTable() drops the schema when it is
392+
* "public", so a bare PROTECTED name means public.<table> -- it is not unknown.
393+
* A bare REFERENCE in a query is genuinely unknown: it resolves through the
394+
* session search_path and could be any schema.
395+
*
396+
* reference unqualified -> match on bare name. Ambiguous, so block; the
397+
* search_path may well point at the protected table.
398+
* protected public -> the qualified reference must actually say public.
399+
* marts.customer_profiles is a different table, and
400+
* treating it as protected refused every other
401+
* schema's copy -- which this product's own
402+
* multi-schema fixtures (crm/sales/finance/hr) hit.
403+
* both qualified -> exact match.
404+
*/
405+
private boolean namesMatch(String protectedName, String referencedName) {
406+
String protectedNorm = normalizeName(protectedName);
407+
String referencedNorm = normalizeName(referencedName);
408+
if (protectedNorm.isEmpty() || referencedNorm.isEmpty()) {
409+
return false;
410+
}
411+
if (!referencedNorm.contains(".")) {
412+
return bareName(protectedNorm).equals(referencedNorm);
390413
}
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));
414+
if (!protectedNorm.contains(".")) {
415+
return referencedNorm.equals("public." + protectedNorm);
396416
}
417+
return protectedNorm.equals(referencedNorm);
418+
}
419+
420+
private String bareName(String normalizedName) {
421+
int dot = normalizedName.lastIndexOf('.');
422+
return dot > 0 && dot < normalizedName.length() - 1
423+
? normalizedName.substring(dot + 1)
424+
: normalizedName;
397425
}
398426

399427
/** Tables named directly in this branch's FROM/JOIN -- what inspection actually saw. */
400428
private void collectDirectTables(PlainSelect select, Set<String> out) {
401429
if (select.getFromItem() instanceof Table table) {
402-
addNameForms(table.getFullyQualifiedName(), out);
430+
out.add(normalizeName(table.getFullyQualifiedName()));
403431
}
404432
if (select.getJoins() != null) {
405433
for (Join join : select.getJoins()) {
406434
if (join.getRightItem() instanceof Table table) {
407-
addNameForms(table.getFullyQualifiedName(), out);
435+
out.add(normalizeName(table.getFullyQualifiedName()));
408436
}
409437
}
410438
}

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

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -201,6 +201,42 @@ void enforcePreExecution_blocksProtectedTableInsideWhereSubquery() {
201201
assertThat(exception.getErrorCode()).isEqualTo("POLICY_SQL_BLOCKED");
202202
}
203203

204+
// A qualified protection names exactly one table. marts.customer_profiles is a
205+
// different table from public.customer_profiles and must not be caught by it.
206+
@Test
207+
void enforcePreExecution_allowsSameNamedTableInAnotherSchemaWhenProtectionIsQualified() {
208+
when(policyService.resolveEffectivePolicy("conn-1", "analyst", false)).thenReturn(policy());
209+
when(policyService.buildProtectionDescriptors(any(ConnectionChatAccessPolicyService.EffectivePolicy.class)))
210+
.thenReturn(Map.of("public.customer_profiles",
211+
descriptor("public", "customer_profiles", false, "email")));
212+
213+
service.enforcePreExecution(
214+
"conn-1",
215+
new QueryRequest("SELECT id FROM (SELECT id FROM marts.customer_profiles) t", null, null),
216+
new QueryExecutionContext(QueryExecutionOrigin.CHAT, QueryExecutionContext.MutationMode.READ_ONLY_ONLY, "analyst", false, false)
217+
);
218+
}
219+
220+
// The genuinely ambiguous direction is an unqualified REFERENCE, not an
221+
// unqualified protection: qualifyTable() stores public.<t> as bare <t>, so a
222+
// bare protected name means public, while a bare reference in a query
223+
// resolves through search_path and could be any schema. Block that one.
224+
@Test
225+
void enforcePreExecution_blocksUnqualifiedReferenceBecauseSearchPathIsUnknown() {
226+
when(policyService.resolveEffectivePolicy("conn-1", "analyst", false)).thenReturn(policy());
227+
228+
UserDataAccessPolicyException exception = assertThrows(
229+
UserDataAccessPolicyException.class,
230+
() -> service.enforcePreExecution(
231+
"conn-1",
232+
new QueryRequest("SELECT id FROM (SELECT id FROM customer_profiles) t", null, null),
233+
new QueryExecutionContext(QueryExecutionOrigin.CHAT, QueryExecutionContext.MutationMode.READ_ONLY_ONLY, "analyst", false, false)
234+
)
235+
);
236+
237+
assertThat(exception.getErrorCode()).isEqualTo("POLICY_SQL_BLOCKED");
238+
}
239+
204240
@Test
205241
void enforcePreExecution_blocksForbiddenSchemaInsideWhereSubquery() {
206242
assertThat(assertSchemaScopeBlocks(

0 commit comments

Comments
 (0)