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

CLAUDE.md

@@ -709,6 +709,103 @@ 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 for both** "no such id" and "not yours", via
+  `assertCanRead/ManageConnectionContentOrNotFound`. The first attempt only 404'd the
+  *unknown* case and left an authorized-but-denied row at 403, which still confirms the
+  row exists — a review caught that the code comments claimed a property the code did not
+  have. `query_performance_regression.id` is a sequential `Long`, so walking 1..N would
+  have mapped every tenant's regressions. Same answer
+  `DashboardWorkspaceService.assertCanReadDashboard` already gives. Endpoints keyed on a
+  **connectionId** keep 403: the caller already knows that connection exists, so an
+  actionable "access denied" is better than a misleading 404.
+- **Never take the actor from the request.** `POST /slow-queries/alerts/{id}/acknowledge`
+  took `@RequestParam String userId`; `acknowledgedBy` defaulted to the literal string
+  `"user"`; `resolvedBy`, `updatedBy` and Sentinel's `initiatedBy` came from the request
+  body — so the audit trail was unauthenticated free text and could name any colleague.
+  All of them now use `accessControlService.requireCurrentUsername()`. The parameters are
+  still accepted (wire compatibility) and ignored, which is noted at each site so nobody
+  re-wires them.
+- **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 scanner built on an allowlist of id names can only catch the ids someone
+  remembered.** `ConnectionScopedAuthorizationSafetyTest` first matched
+  `body.contains("connectionId")` plus a hand-written list
+  (`alertId|actionId|regressionId|…`). Both halves leaked: `ProjectController.createProject`
+  reads `request.getConnectionId()` — **capital C** — and `projectId` was not in the list,
+  so `POST /projects` and `GET|PUT|DELETE /projects/{projectId}` were invisible while the
+  suite reported every case green. Now the connection match is case-insensitive and *any*
+  `@PathVariable …Id` counts as connection-owned until proven otherwise, with genuine
+  exceptions in `NOT_CONNECTION_OWNED_IDS` carrying a reason. Inverting it immediately
+  surfaced four `PlaybookController` endpoints — those turned out to be true negatives
+  (`Playbook` has no `connectionId`; playbooks are global templates), and
+  `playbookExemptionHoldsOnlyWhilePlaybooksAreConnectionFree` fails the build if a
+  `connectionId` is ever added to that entity. **A safety test that reports green is
+  evidence only about what it can see.**
+- **Two path variables are as dangerous as a body id.**
+  `POST /schema-changes/{connectionId}/snapshots/{snapshotId}/set-baseline` authorized the
+  connection and then flipped *whatever snapshot id it was handed* to BASELINE and pointed
+  that connection's drift config at it — so manage access on A could retarget B's snapshot
+  and bind A's baseline to it. `setBaseline` now refuses a snapshot whose `connectionId`
+  differs, in the service as well as the controller, and **throws rather than silently
+  skipping**: no-op'ing the snapshot write while still writing the drift config would leave
+  the config pointing at another connection's snapshot. When a handler takes an id
+  alongside a `connectionId`, authorizing the connection is half the check.
+- **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));
     }
 }