fix: authorize every connection-scoped endpoint (116 were unguarded)

12 controllers took a caller-supplied connectionId and never checked it.
SecurityConfig only asserts .anyRequest().authenticated() and no filter,
interceptor or aspect inspects a connection id, so authentication was the
only barrier. Verified against a running install, not inferred: a DEVELOPER
holding no grant on any connection could

  - read literal-bearing slow-query SQL with real customer ids and names
    (GET /slow-query-analytics/{id}/query/{fp}/samples returned 200 while
    GET /slow-log-source/{id} returned 403 in the same session),
  - enumerate another tenant's schema and table statistics, via two endpoints
    that decrypt the target connection's credentials and open a live JDBC
    session (/tenant-column-suggestions and /config),
  - and permanently delete that tenant's analysis history
    (DELETE /slow-queries/history/connection/{id} -> 200, row gone).

Affected: SlowQueryController (43), SlowQueryAnalyticsController (13),
SchemaChangeController (13), SentinelAnalyticsController (10),
PerformanceActionController (9), QueryPerformanceController (8),
QueryPlanController (8), IndexAdvisorController (7),
PerformanceInsightsController (5), AdvisorController (3),
ResourceLimitsController (3), BusinessRuleController (3).

This is the same class of defect BrainController carried (93 of 116
unguarded). The safety test added then hardcodes one Path.of(...), so it
could not see any of these.

What changed

* 127 guard calls: assertCanReadConnectionContent on reads,
  assertCanManageConnectionContent on writes and deletes.

* An id is not a capability. For endpoints keyed on alertId, actionId,
  regressionId, recommendationId, fingerprintId, planId, ruleId, snapshotId
  or historyId, resolve the owning connection and assert on that. 15 new
  findConnectionIdFor* accessors where no lookup existed. These report 404,
  not 403, for an unknown id — a 403 confirms the row exists, turning the
  endpoint into an id oracle, and regressionId is a sequential Long.

* Ids arriving in the request body are not constrained by a path-variable
  check. Four holes survived exactly that kind of fix:
    - schema-changes/snapshots/compare took two snapshot ids and no
      connectionId at all, so it would diff tenant A's schema against
      tenant B's; compareSnapshots now refuses a mismatch outright.
    - PUT /performance-actions/batch-status took an arbitrary actionIds list
      with no scope; it now authorizes every id before mutating any, so a
      mixed batch fails atomically.
    - changes/acknowledge and regressions/acknowledge authorized the path
      connection and then acted on whatever ids the body named;
      allChangesBelongTo / allComparisonsBelongTo verify membership, and an
      id that resolves to nothing fails too, so unknown ids cannot be mixed
      into an otherwise valid batch.

* Never take the actor from the request. userId was a query parameter and
  acknowledgedBy/resolvedBy/updatedBy defaulted to the literal string
  "user", so the acknowledgement trail was unauthenticated free text that
  could name any colleague. 10 sites now use requireCurrentUsername(). The
  parameters are still accepted for wire compatibility and ignored.

* ConnectionScopedAuthorizationSafetyTest replaces the per-file approach: it
  scans every *Controller.java, so a new controller is covered the day it is
  written. Six cases — connection-scoped endpoints authorized, body-supplied
  id collections scoped, 403 not swallowed into 500, controller advices not
  swallowing denials, exemptions still true, delegated service checks still
  present. The exemption list is itself guarded, so it cannot rot into a way
  of hiding a real gap.

Two things found by writing and running the fix, not by reading it

* The generalized test immediately found 9 more unguarded endpoints in
  controllers nobody was looking at: StatsController, ProjectController,
  DashboardController, and a destructive
  DELETE /sentinel/demo/cleanup/{connectionId}. Three had been in my draft
  exemption list on the assumption they were connection-free; they were not.

* Testing the fix found a bug reading it never would. 24 endpoints returned
  403 and index-advisor returned 500: IndexAdvisorExceptionHandler's
  @ExceptionHandler(Exception.class) swallowed the denial and reported
  "Index operation failed" with the 403's text in the body. The guard held,
  but the response blamed the index store. It now handles
  ResponseStatusException first, and the safety test asserts no advice with
  a catch-all omits that.

Also drops @crossorigin(origins = "*") from SentinelAnalyticsController.
Tested and inert — an evil-origin preflight gets 403 with no
Access-Control-Allow-Origin because the SecurityConfig allowlist wins, while
an allowed origin gets 200 + ACAO — but it reads like an intentional hole.

Verification

Real Maven compile of main and test sources, zero errors.
SlowQueryControllerS3Test needed the new constructor argument and was
updated rather than left red.

Live, against the rebuilt image with a DEVELOPER holding no grant on the
target connection:
  - 40/40 previously-leaking reads -> 403
  - 10/10 writes and destructive endpoints -> 403, and psql confirms nothing
    was mutated
  - 7/7 orphan-id and body-scoped paths -> 404, no existence oracle
  - 30/30 same endpoint shapes on a granted connection -> 200, zero false
    denials; confirmed again from a real browser session
  - index-advisor now returns 403 "Read access denied for this connection"

Not covered: mvn test was not executed (the image build uses -DskipTests and
this host has no JDK/Maven). The six safety-test cases were validated by
re-implementing their scan logic against the tree and the file compiles, but
they have not been run by JUnit.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Author: notSumit25

CLAUDE.md

@@ -709,6 +709,73 @@ it against a real database — not a theoretical hardening pass.
   `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.
+- **Then it happened again, on 12 more controllers — 116 endpoints, zero checks.**
+  `BrainControllerAuthorizationSafetyTest` hardcodes one `Path.of(...)`, so it could not
+  see `SlowQueryController` (43), `SlowQueryAnalyticsController` (13),
+  `SchemaChangeController` (13), `SentinelAnalyticsController` (10),
+  `PerformanceActionController` (9), `QueryPerformanceController` (8),
+  `QueryPlanController` (8), `IndexAdvisorController` (7),
+  `PerformanceInsightsController` (5), `AdvisorController` (3),
+  `ResourceLimitsController` (3) or `BusinessRuleController` (3). Verified live, not
+  inferred: a DEVELOPER holding **no grant on any connection** read literal-bearing
+  slow-query SQL with real customer ids and names
+  (`/slow-query-analytics/{id}/query/{fp}/samples` → 200 while
+  `/slow-log-source/{id}` → 403 in the same session), enumerated another tenant's
+  schema, and **deleted that tenant's analysis history** via
+  `DELETE /slow-queries/history/connection/{id}`. All 116 are now guarded.
+  `ConnectionScopedAuthorizationSafetyTest` replaces the per-file approach: it scans
+  **every** `*Controller.java`, so a new controller is covered the day it is written.
+  Writing it immediately found 9 more unguarded endpoints in controllers nobody was
+  looking at, including `StatsController`, `ProjectController`, `DashboardController`
+  and a destructive `DELETE /sentinel/demo/cleanup/{connectionId}`.
+- **Two endpoints decrypted another user's credentials before anyone checked access.**
+  `GET /slow-query-analytics/{id}/tenant-column-suggestions` and `/config` reach
+  `suggestTenantColumns` → `getJdbcTemplateForBackgroundJob` →
+  `credentialService.getDecryptedConnection`, opening a live JDBC session to the target
+  database. An unguarded read is not only a data leak; it can be a credential-use
+  primitive. Check before the work, not after.
+- **A path-variable sweep is not enough — ids in the request body need their own
+  check.** Four holes survived exactly that kind of fix: `snapshots/compare` (two
+  snapshot ids, no `connectionId` at all — it would diff tenant A's schema against
+  tenant B's), `PUT /performance-actions/batch-status` (an arbitrary `actionIds` list,
+  no scope), and `changes/acknowledge` / `regressions/acknowledge` (path connection
+  authorized, body ids unchecked). `allChangesBelongTo` / `allComparisonsBelongTo`
+  verify membership, and **an id that resolves to nothing fails too** — otherwise
+  unknown ids can be mixed into an otherwise valid batch. The safety test has a
+  dedicated case for body-supplied id collections.
+- **An id is not a capability.** For `alertId`, `actionId`, `regressionId`,
+  `recommendationId`, `fingerprintId`, `planId`, `ruleId`, `snapshotId`, `historyId`:
+  resolve the owning connection and assert on that. Several services had no such
+  accessor, so `findConnectionIdFor*` was added to `QueryPerformanceService`,
+  `QueryPlanCacheService`, `SentinelAnalyticsService`, `BusinessRuleMemoryService`,
+  `SlowQueryAlertService`, `QueryFingerprintService` and `SchemaChangeTrackingService`.
+  These helpers report **404, not 403**, for an unknown id — a 403 confirms the row
+  exists, turning the endpoint into an id oracle. `regressionId` is a sequential
+  `Long`, so that mattered.
+- **Never take the actor from the request.** `POST /slow-queries/alerts/{id}/acknowledge`
+  took `@RequestParam String userId`, and three acknowledge endpoints took
+  `acknowledgedBy` defaulting to the literal string `"user"` — so the audit trail was
+  unauthenticated free text and could name any colleague. All seven sites now use
+  `accessControlService.requireCurrentUsername()`. The parameters are still accepted
+  (wire compatibility) and ignored.
+- **Guarded vs unguarded is an existence oracle.** A guarded endpoint 404s an unknown
+  connection id (`resolveCurrentUserAccess` wraps the lookup); an unguarded one returned
+  200. That difference alone enumerated valid connection ids.
+- **A `@ControllerAdvice` catch-all swallows a 403 the same way an in-method one does,
+  and it is easier to miss because it lives in another file.**
+  `IndexAdvisorExceptionHandler` has `@ExceptionHandler(Exception.class)`, so the newly
+  added guard on `/index-advisor/{id}/health-report` returned
+  `500 "Index operation failed"` with the 403's text in the body — the denial held, but
+  the response blamed the index store. Found by *testing the fix*, not by reading it: the
+  other 24 endpoints returned 403 and this one did not. It now has an
+  `@ExceptionHandler(ResponseStatusException.class)` that preserves the status, and
+  `ConnectionScopedAuthorizationSafetyTest` asserts every advice with a catch-all also
+  handles `ResponseStatusException`.
+- **`@CrossOrigin(origins = "*")` on a controller is dead code here, and worth
+  deleting.** `SentinelAnalyticsController` carried it. Tested: an evil-origin preflight
+  gets `403` with no `Access-Control-Allow-Origin` (the `SecurityConfig` allowlist wins),
+  while an allowed origin gets `200` + ACAO — so the annotation never had effect. It
+  still reads like an intentional hole to the next person.
 
 ### MCP & CLI Release Rules
 

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

@@ -3,20 +3,31 @@
 import com.dbaagent.model.IndexRecommendation;
 import com.dbaagent.model.PerformanceAnalysis;
 import com.dbaagent.service.DatabaseAdvisorService;
+import com.dbaagent.service.security.AccessControlService;
 import lombok.RequiredArgsConstructor;
 import lombok.extern.slf4j.Slf4j;
 import org.springframework.http.ResponseEntity;
 import org.springframework.web.bind.annotation.*;
 
 import java.util.List;
 
+/**
+ * REST API for the performance advisor (analysis, missing indexes, health summary).
+ *
+ * <p><b>Authorization:</b> every endpoint here takes a caller-supplied connection id, so
+ * each one asserts access itself ({@code assertCanReadConnectionContent} for reads,
+ * {@code assertCanManageConnectionContent} for writes). {@code SecurityConfig} only
+ * requires an authenticated principal — nothing upstream inspects a connection id. See
+ * {@code ConnectionScopedAuthorizationSafetyTest}.
+ */
 @RestController
 @RequestMapping("/advisor")
 @RequiredArgsConstructor
 @Slf4j
 public class AdvisorController {
 
     private final DatabaseAdvisorService advisorService;
+    private final AccessControlService accessControlService;
 
     /**
      * Get comprehensive performance analysis
@@ -25,6 +36,7 @@ public class AdvisorController {
     public ResponseEntity<PerformanceAnalysis> analyzePerformance(
         @PathVariable String connectionId
     ) {
+        accessControlService.assertCanReadConnectionContent(connectionId);
         try {
             log.info("Performance analysis requested for connection: {}", connectionId);
             PerformanceAnalysis analysis = advisorService.analyzePerformance(connectionId);
@@ -44,6 +56,7 @@ public ResponseEntity<PerformanceAnalysis> analyzePerformance(
     public ResponseEntity<List<IndexRecommendation>> getMissingIndexes(
         @PathVariable String connectionId
     ) {
+        accessControlService.assertCanReadConnectionContent(connectionId);
         try {
             log.info("Index recommendations requested for connection: {}", connectionId);
             PerformanceAnalysis analysis = advisorService.analyzePerformance(connectionId);
@@ -63,6 +76,7 @@ public ResponseEntity<List<IndexRecommendation>> getMissingIndexes(
     public ResponseEntity<HealthSummary> getHealthSummary(
         @PathVariable String connectionId
     ) {
+        accessControlService.assertCanReadConnectionContent(connectionId);
         try {
             log.info("Health summary requested for connection: {}", connectionId);
             PerformanceAnalysis analysis = advisorService.analyzePerformance(connectionId);

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

@@ -2,6 +2,7 @@
 
 import com.dbaagent.model.brain.BrainRule;
 import com.dbaagent.service.BusinessRuleMemoryService;
+import com.dbaagent.service.security.AccessControlService;
 import lombok.RequiredArgsConstructor;
 import org.springframework.http.ResponseEntity;
 import org.springframework.web.bind.annotation.*;
@@ -11,13 +12,20 @@
 
 /**
  * API endpoints for connection-scoped learned SQL business rules.
+ *
+ * <p><b>Authorization:</b> every endpoint here takes a caller-supplied connection id, so
+ * each one asserts access itself ({@code assertCanReadConnectionContent} for reads,
+ * {@code assertCanManageConnectionContent} for writes). {@code SecurityConfig} only
+ * requires an authenticated principal — nothing upstream inspects a connection id. See
+ * {@code ConnectionScopedAuthorizationSafetyTest}.
  */
 @RestController
 @RequestMapping("/business-rules")
 @RequiredArgsConstructor
 public class BusinessRuleController {
 
     private final BusinessRuleMemoryService businessRuleMemoryService;
+    private final AccessControlService accessControlService;
 
     /**
      * Returns all active rules for the connection plus the subset applicable to an optional question.
@@ -26,6 +34,7 @@ public class BusinessRuleController {
     public ResponseEntity<Map<String, Object>> getRules(
             @PathVariable String connectionId,
             @RequestParam(required = false) String question) {
+        accessControlService.assertCanReadConnectionContent(connectionId);
         List<BrainRule> activeRules = businessRuleMemoryService.getActiveRules(connectionId);
         List<BusinessRuleMemoryService.SqlGuardrail> applicable = businessRuleMemoryService
             .resolveApplicableGuardrails(connectionId, question, null);
@@ -49,6 +58,7 @@ public ResponseEntity<Map<String, Object>> getRules(
     public ResponseEntity<Map<String, Object>> learn(
             @PathVariable String connectionId,
             @RequestBody LearnRuleRequest request) {
+        accessControlService.assertCanManageConnectionContent(connectionId);
         int learned = businessRuleMemoryService.learnFromFeedback(
             connectionId,
             request.text(),
@@ -70,6 +80,10 @@ public ResponseEntity<Map<String, Object>> learn(
      */
     @DeleteMapping("/rule/{ruleId}")
     public ResponseEntity<Map<String, Object>> deactivateRule(@PathVariable String ruleId) {
+        String connectionId = businessRuleMemoryService.findConnectionIdForRule(ruleId)
+                .orElseThrow(() -> new org.springframework.web.server.ResponseStatusException(
+                        org.springframework.http.HttpStatus.NOT_FOUND, "Rule not found"));
+        accessControlService.assertCanManageConnectionContent(connectionId);
         boolean deactivated = businessRuleMemoryService.deactivateRule(ruleId);
         return ResponseEntity.ok(Map.of(
             "ruleId", ruleId,

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

@@ -1,13 +1,20 @@
 package com.dbaagent.controller;
 
 import com.dbaagent.service.DashboardService;
+import com.dbaagent.service.security.AccessControlService;
 import lombok.RequiredArgsConstructor;
 import lombok.extern.slf4j.Slf4j;
 import org.springframework.http.ResponseEntity;
 import org.springframework.web.bind.annotation.*;
 
 /**
  * REST API for performance dashboard
+ *
+ * <p><b>Authorization:</b> every endpoint here takes a caller-supplied connection id, so
+ * each one asserts access itself ({@code assertCanReadConnectionContent} for reads,
+ * {@code assertCanManageConnectionContent} for writes). {@code SecurityConfig} only
+ * requires an authenticated principal — nothing upstream inspects a connection id. See
+ * {@code ConnectionScopedAuthorizationSafetyTest}.
  */
 @RestController
 @RequestMapping("/dashboard")
@@ -16,6 +23,7 @@
 public class DashboardController {
 
     private final DashboardService dashboardService;
+    private final AccessControlService accessControlService;
 
     /**
      * Get performance dashboard data for a connection
@@ -26,6 +34,7 @@ public ResponseEntity<DashboardService.DashboardData> getPerformanceDashboard(
         @RequestParam(required = false, defaultValue = "30") Integer days
     ) {
         try {
+            accessControlService.assertCanReadConnectionContent(connectionId);
             log.info("Fetching performance dashboard for connection: {}, days: {}", connectionId, days);
 
             DashboardService.DashboardData data = dashboardService.getDashboardData(connectionId, days);

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

@@ -2,6 +2,7 @@
 
 import com.dbaagent.service.IndexAdvisorService;
 import com.dbaagent.service.PerformanceMonitoringService;
+import com.dbaagent.service.security.AccessControlService;
 import lombok.RequiredArgsConstructor;
 import lombok.extern.slf4j.Slf4j;
 import org.springframework.http.ResponseEntity;
@@ -12,6 +13,12 @@
 
 /**
  * REST API for enhanced index advisor functionality
+ *
+ * <p><b>Authorization:</b> every endpoint here takes a caller-supplied connection id, so
+ * each one asserts access itself ({@code assertCanReadConnectionContent} for reads,
+ * {@code assertCanManageConnectionContent} for writes). {@code SecurityConfig} only
+ * requires an authenticated principal — nothing upstream inspects a connection id. See
+ * {@code ConnectionScopedAuthorizationSafetyTest}.
  */
 @RestController
 @RequestMapping("/index-advisor")
@@ -21,12 +28,14 @@ public class IndexAdvisorController {
 
     private final IndexAdvisorService indexAdvisorService;
     private final PerformanceMonitoringService performanceMonitoringService;
+    private final AccessControlService accessControlService;
 
     /**
      * Get comprehensive index health report
      */
     @GetMapping("/{connectionId}/health-report")
     public ResponseEntity<Map<String, Object>> getHealthReport(@PathVariable String connectionId) {
+        accessControlService.assertCanReadConnectionContent(connectionId);
         return ResponseEntity.ok(indexAdvisorService.getIndexHealthReport(connectionId));
     }
 
@@ -35,6 +44,7 @@ public ResponseEntity<Map<String, Object>> getHealthReport(@PathVariable String
      */
     @GetMapping("/{connectionId}/unused")
     public ResponseEntity<List<Map<String, Object>>> getUnusedIndexes(@PathVariable String connectionId) {
+        accessControlService.assertCanReadConnectionContent(connectionId);
         return ResponseEntity.ok(performanceMonitoringService.getUnusedIndexes(connectionId));
     }
 
@@ -43,6 +53,7 @@ public ResponseEntity<List<Map<String, Object>>> getUnusedIndexes(@PathVariable
      */
     @GetMapping("/{connectionId}/duplicates")
     public ResponseEntity<List<Map<String, Object>>> getDuplicateIndexes(@PathVariable String connectionId) {
+        accessControlService.assertCanReadConnectionContent(connectionId);
         return ResponseEntity.ok(performanceMonitoringService.getDuplicateIndexes(connectionId));
     }
 
@@ -53,6 +64,7 @@ public ResponseEntity<List<Map<String, Object>>> getDuplicateIndexes(@PathVariab
     public ResponseEntity<Map<String, Object>> estimateIndexCreation(
             @PathVariable String connectionId,
             @RequestBody Map<String, Object> request) {
+        accessControlService.assertCanReadConnectionContent(connectionId);
 
         String tableName = (String) request.get("tableName");
         @SuppressWarnings("unchecked")
@@ -75,6 +87,7 @@ public ResponseEntity<Map<String, Object>> estimateIndexCreation(
     public ResponseEntity<Map<String, Object>> estimateIndexDrop(
             @PathVariable String connectionId,
             @RequestBody Map<String, Object> request) {
+        accessControlService.assertCanReadConnectionContent(connectionId);
 
         String tableName = (String) request.get("tableName");
         String indexName = (String) request.get("indexName");
@@ -94,6 +107,7 @@ public ResponseEntity<Map<String, Object>> estimateIndexDrop(
     public ResponseEntity<List<Map<String, Object>>> getIndexUsageStats(
             @PathVariable String connectionId,
             @PathVariable String tableName) {
+        accessControlService.assertCanReadConnectionContent(connectionId);
         return ResponseEntity.ok(performanceMonitoringService.getIndexUsageStats(connectionId, tableName));
     }
 
@@ -102,6 +116,7 @@ public ResponseEntity<List<Map<String, Object>>> getIndexUsageStats(
      */
     @GetMapping("/{connectionId}/cache-stats")
     public ResponseEntity<Map<String, Double>> getCacheStats(@PathVariable String connectionId) {
+        accessControlService.assertCanReadConnectionContent(connectionId);
         return ResponseEntity.ok(performanceMonitoringService.getCacheHitRatios(connectionId));
     }
 }

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

@@ -61,6 +61,27 @@ public ResponseEntity<Map<String, Object>> handleDataAccess(Exception ex) {
         return ResponseEntity.status(HttpStatus.SERVICE_UNAVAILABLE).body(body);
     }
 
+    /**
+     * An authorization denial is a deliberate answer, not a failure of this feature.
+     * {@code handleGeneric} below matches {@code Exception}, so without this more specific
+     * handler a {@code ResponseStatusException} from
+     * {@code assertCanReadConnectionContent} was reported as
+     * {@code 500 "Index operation failed"} — the denial still held, but the caller could
+     * not tell "not yours" from "the index store is broken", and the message named the
+     * wrong subsystem. Verified: a non-granted user hitting
+     * {@code /index-advisor/{id}/health-report} got a 500 whose body carried the 403 text.
+     */
+    @ExceptionHandler(org.springframework.web.server.ResponseStatusException.class)
+    public ResponseEntity<Map<String, Object>> handleStatus(
+            org.springframework.web.server.ResponseStatusException ex) {
+        Map<String, Object> body = new LinkedHashMap<>();
+        body.put("timestamp", Instant.now().toString());
+        body.put("status", ex.getStatusCode().value());
+        body.put("error", ex.getStatusCode().toString());
+        body.put("message", ex.getReason() != null ? ex.getReason() : ex.getMessage());
+        return ResponseEntity.status(ex.getStatusCode()).body(body);
+    }
+
     /** Any other uncaught error from these endpoints → a clean message, not an opaque 500. */
     @ExceptionHandler(Exception.class)
     public ResponseEntity<Map<String, Object>> handleGeneric(Exception ex) {