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

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>
Co-authored-by: Venkat SF <venkatesh.sakamuri@stayflexi.com>

Author: 3 people

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));
     }
 }