From a547a070733a60d224bf0f252564be1f5023b1d5 Mon Sep 17 00:00:00 2001 From: geekypunk Date: Wed, 19 Aug 2026 12:13:55 -0500 Subject: [PATCH 1/3] fix(security): enforce the chat schema allowlist over the whole statement The schema allowlist was implemented as 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 -- and an allowlist that does not enumerate a reference implicitly permits it. Worse, the dispatch gated every check 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 whole block is skipped, and control falls through to QueryGuardDecision.allow(). That failed open on statement shape: it dropped the protected-column and wildcard inspection too, not just the schema check. Four reachable bypasses, each now covered by a test that failed before this change with "Expected UserDataAccessPolicyException to be thrown, but nothing was thrown" -- i.e. the query was allowed: SELECT id FROM marts.orders WHERE total = (SELECT MAX(salary) FROM hr.salaries) SELECT id FROM marts.orders UNION ALL SELECT ssn FROM hr.salaries WITH leaked AS (SELECT ssn FROM hr.salaries) SELECT * FROM leaked SELECT 1 AS x UNION ALL SELECT email FROM customer_profiles Schema enforcement now runs JSqlParser's TablesNamesFinder over the parsed statement, which is exhaustive by construction rather than by remembering to handle each syntax form, and column inspection runs over every PlainSelect in the statement -- set-operation branches, parenthesised selects and CTE bodies included. The old partial walker is deleted rather than left in place, so it cannot be wired back up. Unqualified names stay unchecked exactly as before: they resolve through the session search_path, and CTE names are not tables. Note the pre-existing catch(Exception) fallback is not a safety net for this. It only fires when parsing throws, and every payload above parses cleanly. Verified: 13/13 in UserDataAccessPolicyServiceTest (4 new, 9 pre-existing), 80/80 across the policy and guard suites. Co-Authored-By: Claude Opus 5 (1M context) --- .../service/UserDataAccessPolicyService.java | 96 ++++++++++++++----- .../UserDataAccessPolicyServiceTest.java | 65 +++++++++++++ 2 files changed, 138 insertions(+), 23 deletions(-) diff --git a/backend/src/main/java/com/dbaagent/service/UserDataAccessPolicyService.java b/backend/src/main/java/com/dbaagent/service/UserDataAccessPolicyService.java index 82f7632..0035b06 100644 --- a/backend/src/main/java/com/dbaagent/service/UserDataAccessPolicyService.java +++ b/backend/src/main/java/com/dbaagent/service/UserDataAccessPolicyService.java @@ -25,6 +25,9 @@ import net.sf.jsqlparser.statement.select.ParenthesedSelect; import net.sf.jsqlparser.statement.select.PlainSelect; import net.sf.jsqlparser.statement.select.Select; +import net.sf.jsqlparser.statement.select.SetOperationList; +import net.sf.jsqlparser.statement.select.WithItem; +import net.sf.jsqlparser.util.TablesNamesFinder; import org.springframework.stereotype.Service; import java.util.ArrayList; @@ -117,9 +120,18 @@ public QueryGuardDecision enforcePreExecution( try { Statement parsed = CCJSqlParserUtil.parse(queryRequest.getQuery()); - if (parsed instanceof Select select && select.getPlainSelect() != null) { - enforceAllowedSchemas(select.getPlainSelect(), policy.allowedSchemas()); - QueryInspection inspection = inspectPlainSelect(select.getPlainSelect(), protectedObjects); + if (parsed instanceof Select select) { + // Enumerate over the WHOLE statement, not just FROM/JOIN of the + // outermost PlainSelect. An allowlist is only sound if the walk is + // total: any node left unvisited is implicitly permitted, which is + // how a subquery, UNION branch, or CTE body reached a schema + // outside the caller's scope. + enforceAllowedSchemas(parsed, policy.allowedSchemas()); + // Likewise inspect every branch. Gating this on getPlainSelect() + // != null skipped protection entirely for a SetOperationList, + // because a UNION's body is not a PlainSelect. + for (PlainSelect branch : collectPlainSelects(select)) { + QueryInspection inspection = inspectPlainSelect(branch, protectedObjects); if (inspection.selectsWildcardFromProtectedTable || inspection.rawProtectedColumnsSelected) { logPolicyEvent(SecurityEventType.CHAT_ACCESS_POLICY_BLOCKED, executionContext.actorUsername(), connectionId, "sql_blocked", Map.of( "query", truncate(queryRequest.getQuery()), @@ -132,6 +144,7 @@ public QueryGuardDecision enforcePreExecution( "POLICY_SQL_BLOCKED" ); } + } } } catch (UserDataAccessPolicyException e) { throw e; @@ -324,16 +337,67 @@ private boolean mentionsProtectedTables(String normalized, ConnectionChatAccessP return policy.impactedTables().stream().anyMatch(table -> normalized.contains(table.toLowerCase(Locale.ROOT))); } - private void enforceAllowedSchemas(PlainSelect select, Set allowedSchemas) { + /** + * Collects every PlainSelect in a statement: the top level, each branch of a + * set operation (UNION/INTERSECT/EXCEPT), parenthesised selects, and every + * CTE body. Callers that inspect only the outermost select leave the rest + * unprotected. + */ + private List collectPlainSelects(Select select) { + List found = new ArrayList<>(); + collectPlainSelects(select, found); + return found; + } + + private void collectPlainSelects(Select select, List found) { + if (select == null) { + return; + } + if (select.getWithItemsList() != null) { + for (WithItem item : select.getWithItemsList()) { + if (item != null) { + collectPlainSelects(item.getSelect(), found); + } + } + } + if (select instanceof PlainSelect plain) { + found.add(plain); + } else if (select instanceof SetOperationList setOps) { + if (setOps.getSelects() != null) { + for (Select branch : setOps.getSelects()) { + collectPlainSelects(branch, found); + } + } + } else if (select instanceof ParenthesedSelect parenthesed) { + collectPlainSelects(parenthesed.getSelect(), found); + } + } + + /** + * Enforces the schema allowlist over every table reference in the statement. + * + * Uses JSqlParser's TablesNamesFinder rather than a hand-rolled walk of + * FROM and JOIN. The distinction is the whole fix: enumeration has to be + * exhaustive by construction, because an allowlist implemented as a partial + * walk implicitly permits every syntax position the walker forgot -- here a + * subquery in WHERE/HAVING/SELECT, a UNION branch, and a CTE body. + * + * Bare (unqualified) names stay unchecked, exactly as before: they resolve + * through the session search_path, and CTE names are not tables at all. + */ + private void enforceAllowedSchemas(Statement statement, Set allowedSchemas) { if (allowedSchemas == null || allowedSchemas.isEmpty()) { return; } - Map aliasToTable = buildAliasMap(select); Set referencedSchemas = new LinkedHashSet<>(); - collectReferencedSchemas(select.getFromItem(), aliasToTable, referencedSchemas); - if (select.getJoins() != null) { - for (Join join : select.getJoins()) { - collectReferencedSchemas(join.getRightItem(), aliasToTable, referencedSchemas); + for (String qualifiedName : new TablesNamesFinder<>().getTables(statement)) { + if (qualifiedName == null) { + continue; + } + String[] parts = qualifiedName.split("\\."); + if (parts.length >= 2) { + // db.schema.table and schema.table both put the schema second-to-last. + referencedSchemas.add(parts[parts.length - 2]); } } for (String schema : referencedSchemas) { @@ -346,20 +410,6 @@ private void enforceAllowedSchemas(PlainSelect select, Set allowedSchema } } - private void collectReferencedSchemas(FromItem fromItem, Map aliasToTable, Set schemas) { - if (fromItem instanceof Table table) { - String schema = table.getSchemaName(); - if (schema != null && !schema.isBlank()) { - schemas.add(schema); - return; - } - String qualified = aliasToTable.get(normalizeName(table.getName())); - if (qualified != null && qualified.contains(".")) { - schemas.add(qualified.substring(0, qualified.indexOf('.'))); - } - } - } - private boolean containsDangerousProtectedReference( String normalizedSql, Map protectedObjects diff --git a/backend/src/test/java/com/dbaagent/service/UserDataAccessPolicyServiceTest.java b/backend/src/test/java/com/dbaagent/service/UserDataAccessPolicyServiceTest.java index c0ee0ff..3688e1f 100644 --- a/backend/src/test/java/com/dbaagent/service/UserDataAccessPolicyServiceTest.java +++ b/backend/src/test/java/com/dbaagent/service/UserDataAccessPolicyServiceTest.java @@ -140,6 +140,71 @@ void enforcePreExecution_blocksQueriesOutsideAllowedSchema() { assertThat(exception.getErrorCode()).isEqualTo("POLICY_SCHEMA_BLOCKED"); } + private ConnectionChatAccessPolicyService.EffectivePolicy martsOnlyPolicy() { + return new ConnectionChatAccessPolicyService.EffectivePolicy( + true, "conn-1", "analyst", + Set.of(), Set.of(), Set.of(), Set.of("marts"), + true, true, "Only schema marts", List.of(), List.of() + ); + } + + private UserDataAccessPolicyException assertSchemaScopeBlocks(String sql) { + ConnectionChatAccessPolicyService.EffectivePolicy schemaPolicy = martsOnlyPolicy(); + when(policyService.resolveEffectivePolicy("conn-1", "analyst", false)).thenReturn(schemaPolicy); + lenient().when(policyService.buildProtectionDescriptors(schemaPolicy)).thenReturn(Map.of()); + return assertThrows( + UserDataAccessPolicyException.class, + () -> service.enforcePreExecution( + "conn-1", + new QueryRequest(sql, null, null), + new QueryExecutionContext(QueryExecutionOrigin.CHAT, QueryExecutionContext.MutationMode.READ_ONLY_ONLY, "analyst", false, false) + ) + ); + } + + // The schema allowlist walked only FROM and JOIN, so a forbidden schema + // reached through any other syntax position was never enumerated and the + // allowlist silently permitted it. + + @Test + void enforcePreExecution_blocksForbiddenSchemaInsideWhereSubquery() { + assertThat(assertSchemaScopeBlocks( + "SELECT id FROM marts.orders WHERE total = (SELECT MAX(salary) FROM hr.salaries)" + ).getErrorCode()).isEqualTo("POLICY_SCHEMA_BLOCKED"); + } + + @Test + void enforcePreExecution_blocksForbiddenSchemaInUnionBranch() { + assertThat(assertSchemaScopeBlocks( + "SELECT id FROM marts.orders UNION ALL SELECT ssn FROM hr.salaries" + ).getErrorCode()).isEqualTo("POLICY_SCHEMA_BLOCKED"); + } + + // The same fail-open gate skipped protected-column inspection entirely, so a + // UNION reached restricted columns even inside an allowed schema. + @Test + void enforcePreExecution_blocksProtectedColumnsInUnionBranch() { + when(policyService.resolveEffectivePolicy("conn-1", "analyst", false)).thenReturn(policy()); + + UserDataAccessPolicyException exception = assertThrows( + UserDataAccessPolicyException.class, + () -> service.enforcePreExecution( + "conn-1", + new QueryRequest("SELECT 1 AS x UNION ALL SELECT email FROM customer_profiles", null, null), + new QueryExecutionContext(QueryExecutionOrigin.CHAT, QueryExecutionContext.MutationMode.READ_ONLY_ONLY, "analyst", false, false) + ) + ); + + assertThat(exception.getErrorCode()).isEqualTo("POLICY_SQL_BLOCKED"); + } + + @Test + void enforcePreExecution_blocksForbiddenSchemaInsideCte() { + assertThat(assertSchemaScopeBlocks( + "WITH leaked AS (SELECT ssn FROM hr.salaries) SELECT * FROM leaked" + ).getErrorCode()).isEqualTo("POLICY_SCHEMA_BLOCKED"); + } + @Test void enforcePreExecution_allowsMartsQueriesWhenOtherSchemasHaveProtectedColumns() { ConnectionChatAccessPolicyService.EffectivePolicy schemaPolicy = new ConnectionChatAccessPolicyService.EffectivePolicy( From 70a1472cb1fa324c24757c4c332559c14bf908bd Mon Sep 17 00:00:00 2001 From: geekypunk Date: Wed, 19 Aug 2026 12:25:08 -0500 Subject: [PATCH 2/3] 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) --- .../service/UserDataAccessPolicyService.java | 73 +++++++++++++++++++ .../UserDataAccessPolicyServiceTest.java | 35 +++++++++ 2 files changed, 108 insertions(+) diff --git a/backend/src/main/java/com/dbaagent/service/UserDataAccessPolicyService.java b/backend/src/main/java/com/dbaagent/service/UserDataAccessPolicyService.java index 0035b06..ce6ef6b 100644 --- a/backend/src/main/java/com/dbaagent/service/UserDataAccessPolicyService.java +++ b/backend/src/main/java/com/dbaagent/service/UserDataAccessPolicyService.java @@ -127,6 +127,7 @@ public QueryGuardDecision enforcePreExecution( // how a subquery, UNION branch, or CTE body reached a schema // outside the caller's scope. enforceAllowedSchemas(parsed, policy.allowedSchemas()); + assertProtectedTablesAreInspectable(parsed, collectPlainSelects(select), protectedObjects); // Likewise inspect every branch. Gating this on getPlainSelect() // != null skipped protection entirely for a SetOperationList, // because a UNION's body is not a PlainSelect. @@ -337,6 +338,78 @@ private boolean mentionsProtectedTables(String normalized, ConnectionChatAccessP return policy.impactedTables().stream().anyMatch(table -> normalized.contains(table.toLowerCase(Locale.ROOT))); } + /** + * Fails closed on statement shapes inspection cannot reach. + * + * TablesNamesFinder sees every table in the statement; collectPlainSelects + * deliberately does not descend into a select nested inside FROM/JOIN/WHERE/ + * HAVING, because enumerating arbitrary expression trees correctly is the very + * thing that went wrong here the first time. So instead of trying harder to + * walk, compare the two: when a protected table is referenced somewhere the + * column inspection could not examine, refuse the query. + * + * A syntax form we failed to enumerate must never become an implicit permit -- + * that is exactly how a subquery, a UNION branch and a CTE body each evaded + * the schema allowlist. Refusing costs a conservative block on some safe + * nested aggregates; allowing costs the data. + */ + private void assertProtectedTablesAreInspectable( + Statement statement, + List inspectedBranches, + Map protectedObjects + ) { + if (protectedObjects == null || protectedObjects.isEmpty()) { + return; + } + Set referenced = new LinkedHashSet<>(); + for (String name : new TablesNamesFinder<>().getTables(statement)) { + addNameForms(name, referenced); + } + Set inspected = new LinkedHashSet<>(); + for (PlainSelect branch : inspectedBranches) { + collectDirectTables(branch, inspected); + } + for (ConnectionChatAccessPolicyService.ProtectionDescriptor descriptor : protectedObjects.values()) { + Set forms = new LinkedHashSet<>(); + addNameForms(descriptor.qualifiedTableName(), forms); + boolean isReferenced = forms.stream().anyMatch(referenced::contains); + boolean wasInspected = forms.stream().anyMatch(inspected::contains); + if (isReferenced && !wasInspected) { + throw new UserDataAccessPolicyException( + "This query reaches restricted data through a nested query DeepSQL cannot fully verify, so it was blocked before execution.", + "POLICY_SQL_BLOCKED" + ); + } + } + } + + /** Adds both the qualified name and its bare table part, so schema.t matches t. */ + private void addNameForms(String name, Set out) { + if (name == null || name.isBlank()) { + return; + } + String normalized = normalizeName(name); + out.add(normalized); + int dot = normalized.lastIndexOf('.'); + if (dot > 0 && dot < normalized.length() - 1) { + out.add(normalized.substring(dot + 1)); + } + } + + /** Tables named directly in this branch's FROM/JOIN -- what inspection actually saw. */ + private void collectDirectTables(PlainSelect select, Set out) { + if (select.getFromItem() instanceof Table table) { + addNameForms(table.getFullyQualifiedName(), out); + } + if (select.getJoins() != null) { + for (Join join : select.getJoins()) { + if (join.getRightItem() instanceof Table table) { + addNameForms(table.getFullyQualifiedName(), out); + } + } + } + } + /** * Collects every PlainSelect in a statement: the top level, each branch of a * set operation (UNION/INTERSECT/EXCEPT), parenthesised selects, and every diff --git a/backend/src/test/java/com/dbaagent/service/UserDataAccessPolicyServiceTest.java b/backend/src/test/java/com/dbaagent/service/UserDataAccessPolicyServiceTest.java index 3688e1f..b060b58 100644 --- a/backend/src/test/java/com/dbaagent/service/UserDataAccessPolicyServiceTest.java +++ b/backend/src/test/java/com/dbaagent/service/UserDataAccessPolicyServiceTest.java @@ -166,6 +166,41 @@ private UserDataAccessPolicyException assertSchemaScopeBlocks(String sql) { // reached through any other syntax position was never enumerated and the // allowlist silently permitted it. + // Column inspection cannot reach a select nested inside FROM/JOIN/WHERE, so a + // protected table referenced there must be refused rather than implicitly allowed. + + @Test + void enforcePreExecution_blocksProtectedTableInsideDerivedTable() { + when(policyService.resolveEffectivePolicy("conn-1", "analyst", false)).thenReturn(policy()); + + UserDataAccessPolicyException exception = assertThrows( + UserDataAccessPolicyException.class, + () -> service.enforcePreExecution( + "conn-1", + new QueryRequest("SELECT t.email FROM (SELECT email FROM customer_profiles) t", null, null), + new QueryExecutionContext(QueryExecutionOrigin.CHAT, QueryExecutionContext.MutationMode.READ_ONLY_ONLY, "analyst", false, false) + ) + ); + + assertThat(exception.getErrorCode()).isEqualTo("POLICY_SQL_BLOCKED"); + } + + @Test + void enforcePreExecution_blocksProtectedTableInsideWhereSubquery() { + when(policyService.resolveEffectivePolicy("conn-1", "analyst", false)).thenReturn(policy()); + + UserDataAccessPolicyException exception = assertThrows( + UserDataAccessPolicyException.class, + () -> service.enforcePreExecution( + "conn-1", + new QueryRequest("SELECT id FROM orders WHERE id IN (SELECT email FROM customer_profiles)", null, null), + new QueryExecutionContext(QueryExecutionOrigin.CHAT, QueryExecutionContext.MutationMode.READ_ONLY_ONLY, "analyst", false, false) + ) + ); + + assertThat(exception.getErrorCode()).isEqualTo("POLICY_SQL_BLOCKED"); + } + @Test void enforcePreExecution_blocksForbiddenSchemaInsideWhereSubquery() { assertThat(assertSchemaScopeBlocks( From bb8b8957a5bbf3ecef52e8d22a3ae1291ff01bd3 Mon Sep 17 00:00:00 2001 From: geekypunk Date: Wed, 19 Aug 2026 14:37:18 -0500 Subject: [PATCH 3/3] 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. -- 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) --- .../service/UserDataAccessPolicyService.java | 60 ++++++++++++++----- .../UserDataAccessPolicyServiceTest.java | 36 +++++++++++ 2 files changed, 80 insertions(+), 16 deletions(-) diff --git a/backend/src/main/java/com/dbaagent/service/UserDataAccessPolicyService.java b/backend/src/main/java/com/dbaagent/service/UserDataAccessPolicyService.java index ce6ef6b..a87b214 100644 --- a/backend/src/main/java/com/dbaagent/service/UserDataAccessPolicyService.java +++ b/backend/src/main/java/com/dbaagent/service/UserDataAccessPolicyService.java @@ -363,17 +363,18 @@ private void assertProtectedTablesAreInspectable( } Set referenced = new LinkedHashSet<>(); for (String name : new TablesNamesFinder<>().getTables(statement)) { - addNameForms(name, referenced); + if (name != null && !name.isBlank()) { + referenced.add(normalizeName(name)); + } } Set inspected = new LinkedHashSet<>(); for (PlainSelect branch : inspectedBranches) { collectDirectTables(branch, inspected); } for (ConnectionChatAccessPolicyService.ProtectionDescriptor descriptor : protectedObjects.values()) { - Set forms = new LinkedHashSet<>(); - addNameForms(descriptor.qualifiedTableName(), forms); - boolean isReferenced = forms.stream().anyMatch(referenced::contains); - boolean wasInspected = forms.stream().anyMatch(inspected::contains); + String protectedName = descriptor.qualifiedTableName(); + boolean isReferenced = referenced.stream().anyMatch(name -> namesMatch(protectedName, name)); + boolean wasInspected = inspected.stream().anyMatch(name -> namesMatch(protectedName, name)); if (isReferenced && !wasInspected) { throw new UserDataAccessPolicyException( "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( } } - /** Adds both the qualified name and its bare table part, so schema.t matches t. */ - private void addNameForms(String name, Set out) { - if (name == null || name.isBlank()) { - return; + /** + * Does a query's table reference name the protected table? + * + * Asymmetric on purpose, because the two sides carry different information. + * ConnectionChatAccessPolicyService.qualifyTable() drops the schema when it is + * "public", so a bare PROTECTED name means public.
-- 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; the + * search_path may well point at the protected table. + * protected public -> the qualified reference must actually say public. + * marts.customer_profiles is a different table, and + * treating it as protected refused every other + * schema's copy -- which this product's own + * multi-schema fixtures (crm/sales/finance/hr) hit. + * both qualified -> exact match. + */ + private boolean namesMatch(String protectedName, String referencedName) { + String protectedNorm = normalizeName(protectedName); + String referencedNorm = normalizeName(referencedName); + if (protectedNorm.isEmpty() || referencedNorm.isEmpty()) { + return false; + } + if (!referencedNorm.contains(".")) { + return bareName(protectedNorm).equals(referencedNorm); } - String normalized = normalizeName(name); - out.add(normalized); - int dot = normalized.lastIndexOf('.'); - if (dot > 0 && dot < normalized.length() - 1) { - out.add(normalized.substring(dot + 1)); + if (!protectedNorm.contains(".")) { + return referencedNorm.equals("public." + protectedNorm); } + return protectedNorm.equals(referencedNorm); + } + + private String bareName(String normalizedName) { + int dot = normalizedName.lastIndexOf('.'); + return dot > 0 && dot < normalizedName.length() - 1 + ? normalizedName.substring(dot + 1) + : normalizedName; } /** Tables named directly in this branch's FROM/JOIN -- what inspection actually saw. */ private void collectDirectTables(PlainSelect select, Set out) { if (select.getFromItem() instanceof Table table) { - addNameForms(table.getFullyQualifiedName(), out); + out.add(normalizeName(table.getFullyQualifiedName())); } if (select.getJoins() != null) { for (Join join : select.getJoins()) { if (join.getRightItem() instanceof Table table) { - addNameForms(table.getFullyQualifiedName(), out); + out.add(normalizeName(table.getFullyQualifiedName())); } } } diff --git a/backend/src/test/java/com/dbaagent/service/UserDataAccessPolicyServiceTest.java b/backend/src/test/java/com/dbaagent/service/UserDataAccessPolicyServiceTest.java index b060b58..9939336 100644 --- a/backend/src/test/java/com/dbaagent/service/UserDataAccessPolicyServiceTest.java +++ b/backend/src/test/java/com/dbaagent/service/UserDataAccessPolicyServiceTest.java @@ -201,6 +201,42 @@ void enforcePreExecution_blocksProtectedTableInsideWhereSubquery() { assertThat(exception.getErrorCode()).isEqualTo("POLICY_SQL_BLOCKED"); } + // A qualified protection names exactly one table. marts.customer_profiles is a + // different table from public.customer_profiles and must not be caught by it. + @Test + void enforcePreExecution_allowsSameNamedTableInAnotherSchemaWhenProtectionIsQualified() { + when(policyService.resolveEffectivePolicy("conn-1", "analyst", false)).thenReturn(policy()); + when(policyService.buildProtectionDescriptors(any(ConnectionChatAccessPolicyService.EffectivePolicy.class))) + .thenReturn(Map.of("public.customer_profiles", + descriptor("public", "customer_profiles", false, "email"))); + + service.enforcePreExecution( + "conn-1", + new QueryRequest("SELECT id FROM (SELECT id FROM marts.customer_profiles) t", null, null), + new QueryExecutionContext(QueryExecutionOrigin.CHAT, QueryExecutionContext.MutationMode.READ_ONLY_ONLY, "analyst", false, false) + ); + } + + // The genuinely ambiguous direction is an unqualified REFERENCE, not an + // unqualified protection: qualifyTable() stores public. as bare , so a + // bare protected name means public, while a bare reference in a query + // resolves through search_path and could be any schema. Block that one. + @Test + void enforcePreExecution_blocksUnqualifiedReferenceBecauseSearchPathIsUnknown() { + when(policyService.resolveEffectivePolicy("conn-1", "analyst", false)).thenReturn(policy()); + + UserDataAccessPolicyException exception = assertThrows( + UserDataAccessPolicyException.class, + () -> service.enforcePreExecution( + "conn-1", + new QueryRequest("SELECT id FROM (SELECT id FROM customer_profiles) t", null, null), + new QueryExecutionContext(QueryExecutionOrigin.CHAT, QueryExecutionContext.MutationMode.READ_ONLY_ONLY, "analyst", false, false) + ) + ); + + assertThat(exception.getErrorCode()).isEqualTo("POLICY_SQL_BLOCKED"); + } + @Test void enforcePreExecution_blocksForbiddenSchemaInsideWhereSubquery() { assertThat(assertSchemaScopeBlocks(