Merge branch 'main' into cursor/view-as-policy-identity-c497

Keep fail-closed parse/actor rules and recursive SELECT inspection from this
branch, and take main's whole-statement schema allowlist plus
assertProtectedTablesAreInspectable. Blank outer-column provenance from a
derived table is no longer treated as unresolved so a qualified protection
does not catch the same table name in another schema.

Co-authored-by: Venkat SF <venkatesh.sakamuri@stayflexi.com>

Author: cursoragent

CLAUDE.md

@@ -375,6 +375,42 @@ it against a real database — not a theoretical hardening pass.
   `POST /users/admin/reset` on every install that had run `setup-agent.sh`, since that
   mints an admin MCP token on each run.
 
+### Endpoint Authorization Rules
+
+- **Authentication is not authorization.** `SecurityConfig` only asserts
+  `.anyRequest().authenticated()` and `JwtAuthenticationFilter` only resolves a
+  principal — neither looks at a `connectionId`. Connections are **private per user**
+  (`ConnectionAccessService.resolveAccess` keys on `ownerUsername` plus an explicit
+  grant table), so any endpoint taking a caller-supplied `connectionId` **must** call
+  `accessControlService.assertCanReadConnectionContent` (reads) or
+  `assertCanManageConnectionContent` (writes) itself. There is no filter, interceptor
+  or aspect that does this for you.
+- **`BrainController` shipped with 93 of its 116 endpoints unguarded.** Only the first
+  ~15 (`/understanding`, `/notes/*`, `/tasks/*`, `/key-columns/*`,
+  `/inferred-relationships/*`) had the check; every later "Phase" block did not — so an
+  authenticated user could pass someone else's connection id to
+  `/brain/health-scores/{id}`, `/brain/data-sensitivity/{id}` (which names the PII
+  columns), `/brain/cost-attribution/{id}`, `/brain/ml-overview/{id}` and ~90 more and
+  read that user's database intelligence. All 116 are now guarded, and
+  `BrainControllerAuthorizationSafetyTest` fails the build if a new one is not. The
+  misses clustered by **when a section was written**, not by read/write semantics —
+  when adding a controller section, guard it as you write it.
+- **When the path carries some other id** (`simulationId`, `experimentId`, `patternId`,
+  `noteId`, `taskId`), resolve the owning connection first via that service's
+  `getConnectionId(id)` and assert on the result. Do not skip the check because the
+  path has no `connectionId` in it.
+- **An endpoint with no connection scope at all is admin-only.**
+  `POST /brain/column-values/embed-all` spans every connection, so it carries
+  `@PreAuthorize("hasRole('ADMIN')")` — it cannot be authorized against one
+  connection's grants. `@EnableMethodSecurity(prePostEnabled = true)` is on in
+  `SecurityConfig`, so `@PreAuthorize` is live.
+- **Assert inside the `try`, and rethrow `ResponseStatusException` before the
+  catch-all.** Every handler in `BrainController` ends with a
+  `catch (Exception) -> 500`; without the earlier
+  `catch (ResponseStatusException e) { throw e; }` a 403 is swallowed and reported as a
+  server error, so a client cannot tell "not yours" from "broken". The safety test
+  asserts this too.
+
 ### MCP & CLI Release Rules
 
 **Whenever you add, rename, or remove an MCP tool or a CLI subcommand, you MUST update all of these in the same commit — they are agent-facing surfaces and drift silently breaks discoverability:**

backend/src/main/java/com/dbaagent/controller/BrainController.java

@@ -160,6 +160,7 @@ public QueryGuardDecision enforcePreExecution(
                 );
             }
             enforceAllowedSchemas(select, policy.allowedSchemas());
+            assertProtectedTablesAreInspectable(inspectable, collectPlainSelects(select), protectedObjects);
             QueryInspection inspection = inspectSelect(select, protectedObjects, policy.allowAggregates());
             if (inspection.selectsWildcardFromProtectedTable
                 || inspection.rawProtectedColumnsSelected
@@ -501,6 +502,18 @@ private Statement unwrapExplain(Statement parsed) {
         return parsed;
     }
 
+    /**
+     * 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. Enumeration has to be exhaustive by construction: an
+     * allowlist implemented as a partial walk implicitly permits every syntax
+     * position the walker forgot (subquery, UNION branch, CTE body).
+     *
+     * Unqualified names fail closed when a schema allowlist is present: they
+     * resolve through search_path and could be any schema. CTE aliases are
+     * skipped because they are not tables.
+     */
     private void enforceAllowedSchemas(Select select, Set<String> allowedSchemas) {
         if (allowedSchemas == null || allowedSchemas.isEmpty()) {
             return;
@@ -534,6 +547,125 @@ private void enforceAllowedSchemas(Select select, Set<String> allowedSchemas) {
         }
     }
 
+    /**
+     * Fails closed on statement shapes column inspection cannot reach.
+     *
+     * TablesNamesFinder sees every table in the statement; collectPlainSelects
+     * does not descend into a select nested inside FROM/JOIN/WHERE/HAVING.
+     * When a protected table is referenced somewhere the column inspection
+     * could not examine, refuse the query.
+     */
+    private void assertProtectedTablesAreInspectable(
+        Statement statement,
+        List<PlainSelect> inspectedBranches,
+        Map<String, ConnectionChatAccessPolicyService.ProtectionDescriptor> protectedObjects
+    ) {
+        if (protectedObjects == null || protectedObjects.isEmpty()) {
+            return;
+        }
+        Set<String> referenced = new LinkedHashSet<>();
+        for (String name : new TablesNamesFinder<>().getTables(statement)) {
+            if (name != null && !name.isBlank()) {
+                referenced.add(normalizeName(name));
+            }
+        }
+        Set<String> inspected = new LinkedHashSet<>();
+        for (PlainSelect branch : inspectedBranches) {
+            collectDirectTables(branch, inspected);
+        }
+        for (ConnectionChatAccessPolicyService.ProtectionDescriptor descriptor : protectedObjects.values()) {
+            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.",
+                    "POLICY_SQL_BLOCKED"
+                );
+            }
+        }
+    }
+
+    /**
+     * Does a query's table reference name the protected table?
+     *
+     * Asymmetric on purpose. {@code qualifyTable()} drops the schema when it is
+     * {@code public}, so a bare protected name means {@code public.<table>}.
+     * A bare reference in a query resolves through search_path and could be
+     * any schema, so it matches on bare name (ambiguous → block).
+     * A qualified protection must not catch the same table name in another
+     * schema ({@code marts.customer_profiles} vs {@code public.customer_profiles}).
+     */
+    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);
+        }
+        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. */
+    private void collectDirectTables(PlainSelect select, Set<String> out) {
+        if (select.getFromItem() instanceof Table table) {
+            out.add(normalizeName(table.getFullyQualifiedName()));
+        }
+        if (select.getJoins() != null) {
+            for (Join join : select.getJoins()) {
+                if (join.getRightItem() instanceof Table table) {
+                    out.add(normalizeName(table.getFullyQualifiedName()));
+                }
+            }
+        }
+    }
+
+    /**
+     * Collects every PlainSelect in a statement: the top level, each branch of a
+     * set operation, parenthesised selects, and every CTE body.
+     */
+    private List<PlainSelect> collectPlainSelects(Select select) {
+        List<PlainSelect> found = new ArrayList<>();
+        collectPlainSelects(select, found);
+        return found;
+    }
+
+    private void collectPlainSelects(Select select, List<PlainSelect> 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);
+        }
+    }
+
     private Set<String> findReferencedTables(Select select) {
         TablesNamesFinder finder = new TablesNamesFinder();
         List<String> tableList = finder.getTableList((Statement) select);
@@ -660,11 +792,12 @@ private QueryInspection inspectPlainSelect(
                 return;
             }
             for (ColumnReference reference : referencedColumns) {
+                // Outer SELECT id FROM (subquery) t has no concrete FROM table. Nested
+                // selects are already walked by inspectFromItem; unqualified nested
+                // table names are refused by assertProtectedTablesAreInspectable + namesMatch.
+                // Treating blank provenance as unresolved would also block a same-named
+                // table in another schema (marts.customer_profiles vs public.customer_profiles).
                 if ((reference.tableName() == null || reference.tableName().isBlank()) && defaultTableName.isBlank()) {
-                    if (!protectedObjects.isEmpty()) {
-                        inspection.unresolvedProtectedReference = true;
-                        inspection.reason = "Protected column provenance could not be established";
-                    }
                     continue;
                 }
                 if (isProtectedReference(protectedObjects, reference)) {

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

@@ -504,6 +504,12 @@ public List<TableGrowthPrediction> getTablePredictions(String simulationId) {
         return predictionRepository.findByScalabilitySimulationId(simulationId);
     }
 
+    public String getConnectionId(String simulationId) {
+        return simulationRepository.findById(simulationId)
+            .map(ScalabilitySimulation::getConnectionId)
+            .orElseThrow(() -> new IllegalArgumentException("Simulation not found"));
+    }
+
     /**
      * Get high-risk tables.
      */

backend/src/main/java/com/dbaagent/service/brain/analysis/ScalabilitySimulationService.java

@@ -723,6 +723,12 @@ public void cancelExperiment(String experimentId) {
         experimentRepository.delete(experiment);
     }
 
+    public String getConnectionId(String experimentId) {
+        return experimentRepository.findById(experimentId)
+            .map(TuningExperiment::getConnectionId)
+            .orElseThrow(() -> new IllegalArgumentException("Experiment not found: " + experimentId));
+    }
+
     /**
      * Get experiment history.
      */

backend/src/main/java/com/dbaagent/service/brain/config/ConfigTuningService.java

@@ -194,6 +194,12 @@ public void recordFeedback(String patternId, boolean wasSuccessful) {
         }
     }
 
+    public String getConnectionId(String patternId) {
+        return patternRepository.findById(patternId)
+            .map(PlanPattern::getConnectionId)
+            .orElseThrow(() -> new IllegalArgumentException("Pattern not found: " + patternId));
+    }
+
     /**
      * Get reliable patterns for a connection.
      */

backend/src/main/java/com/dbaagent/service/brain/query/PlanPatternLibraryService.java

@@ -0,0 +1,131 @@
+package com.dbaagent.controller;
+
+import org.junit.jupiter.api.Test;
+
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+/**
+ * Every {@code /brain/**} endpoint must authorize the caller against the connection it
+ * touches, not merely require a logged-in user.
+ *
+ * <p>This shipped with 93 of 116 endpoints unguarded. {@code SecurityConfig} only asserts
+ * {@code .anyRequest().authenticated()} and {@code JwtAuthenticationFilter} only resolves a
+ * principal — neither inspects a {@code connectionId}. Connections are private per user
+ * ({@code ConnectionAccessService.resolveAccess} keys on {@code ownerUsername} plus an
+ * explicit grant table), so an authenticated user who passed somebody else's connection id
+ * to {@code /brain/health-scores/{id}}, {@code /brain/data-sensitivity/{id}} (which names
+ * the PII columns), {@code /brain/cost-attribution/{id}} and ~90 others got that user's
+ * database intelligence back.
+ *
+ * <p>Resource-level authorization here is opt-in per method, and the misses clustered by
+ * when a section was written rather than by read/write semantics — the first ~15 endpoints
+ * had it and every later "Phase" block did not. That is a defect a reviewer catches once
+ * and a test catches forever, so it is asserted structurally: scanned as text, because the
+ * property under test is that no endpoint method body lacks the call, whatever it does.
+ */
+class BrainControllerAuthorizationSafetyTest {
+
+    private static final Path CONTROLLER =
+        Path.of("src/main/java/com/dbaagent/controller/BrainController.java");
+
+    /**
+     * Matches a fully-qualified annotation as well as a bare one. A
+     * {@code @org.springframework.web.bind.annotation.DeleteMapping} on
+     * {@code DELETE /calibration/{connectionId}} is exactly how the one destructive
+     * endpoint escaped the first sweep of this fix — a bare-name-only pattern silently
+     * skips it, so the endpoint reads as "not an endpoint" rather than "unguarded".
+     */
+    private static final Pattern MAPPING = Pattern.compile(
+        "^\\s*@(?:[\\w.]*\\.)?(Get|Post|Delete|Put|Patch)Mapping\\b");
+
+    /** A method is authorized by a per-connection assert, or by being admin-only. */
+    private static final Pattern AUTHORIZED = Pattern.compile(
+        "accessControlService\\.assertCan(Read|Manage)ConnectionContent\\(|@PreAuthorize");
+
+    private record Endpoint(int line, String mapping, String body) {}
+
+    /**
+     * Slices the controller into one entry per handler: from its mapping annotation to the
+     * method's closing brace, which at this nesting level is a line that is exactly
+     * {@code "    }"}.
+     */
+    private static List<Endpoint> endpoints() throws IOException {
+        List<String> lines = Files.readAllLines(CONTROLLER);
+        List<Endpoint> endpoints = new ArrayList<>();
+
+        for (int i = 0; i < lines.size(); i++) {
+            Matcher matcher = MAPPING.matcher(lines.get(i));
+            if (!matcher.find()) {
+                continue;
+            }
+            StringBuilder body = new StringBuilder();
+            int end = i;
+            while (end < lines.size()) {
+                body.append(lines.get(end)).append('\n');
+                if (end > i && lines.get(end).equals("    }")) {
+                    break;
+                }
+                end++;
+            }
+            endpoints.add(new Endpoint(i + 1, lines.get(i).trim(), body.toString()));
+        }
+        return endpoints;
+    }
+
+    @Test
+    void everyBrainEndpointAuthorizesTheCallerAgainstTheConnection() throws IOException {
+        List<String> offenders = new ArrayList<>();
+
+        for (Endpoint endpoint : endpoints()) {
+            if (!AUTHORIZED.matcher(endpoint.body()).find()) {
+                offenders.add(CONTROLLER + ":" + endpoint.line() + " " + endpoint.mapping());
+            }
+        }
+
+        assertThat(offenders)
+            .as("Each of these BrainController endpoints takes a caller-supplied id and "
+                + "never authorizes it. Authentication is not authorization: connections "
+                + "are private per user, so this hands one user another user's schema, "
+                + "sensitivity, cost and workload intelligence. Add "
+                + "accessControlService.assertCanReadConnectionContent(connectionId) to "
+                + "reads and assertCanManageConnectionContent(connectionId) to writes, "
+                + "resolving the connection id first when the path carries some other id. "
+                + "An endpoint with no connection scope at all is admin-only (@PreAuthorize).")
+            .isEmpty();
+    }
+
+    /**
+     * The asserts live inside each handler's {@code try}, and every handler ends with a
+     * {@code catch (Exception)} that returns 500. Without an earlier
+     * {@code catch (ResponseStatusException e) { throw e; }} the 403 would be swallowed and
+     * reported as a server error — the denial would still hold, but it would look like a
+     * bug in the feature rather than a permission boundary, and a client could not tell
+     * "not yours" from "broken".
+     */
+    @Test
+    void authorizationFailuresPropagateAsForbiddenRatherThanServerError() throws IOException {
+        List<String> offenders = new ArrayList<>();
+
+        for (Endpoint endpoint : endpoints()) {
+            String body = endpoint.body();
+            boolean guardedInline = body.contains("accessControlService.assertCan");
+            if (guardedInline && !body.contains("catch (ResponseStatusException e)")) {
+                offenders.add(CONTROLLER + ":" + endpoint.line() + " " + endpoint.mapping());
+            }
+        }
+
+        assertThat(offenders)
+            .as("These endpoints assert access inside a try whose catch-all converts the "
+                + "403 into a 500. Rethrow it first: "
+                + "catch (ResponseStatusException e) { throw e; }")
+            .isEmpty();
+    }
+}