fix: close the three gaps cursor[bot] found on #86

All three were real. Verified each against the code before fixing, and each
fix against the running backend after.

1. ProjectController was half-open, and the safety test could not see it

Only `listProjects` with a non-null connectionId was guarded. `createProject`
took the connection from `request.getConnectionId()`, and the three
projectId-keyed endpoints had no check at all — so any authenticated user
could read, rename or delete another tenant's project.

Root cause was the scanner, exactly as reported: `touchesAConnection` matched
the literal lowercase `connectionId`, so `getConnectionId()` (capital C) did
not register, and `projectId` was absent from its hand-written id allowlist.
Four unguarded endpoints were invisible while the suite reported every case
green. My javadoc on that controller claimed "every endpoint here asserts
access itself", which was false.

Fixed both halves. The connection match is now case-insensitive
(`(?i)connection_?id`), and the id rule is inverted: *any* `@PathVariable
...Id` counts as connection-owned until proven otherwise, with real
exceptions listed in NOT_CONNECTION_OWNED_IDS alongside the reason. An
allowlist can only catch the ids someone remembered to add; this way an
omission fails the build instead of passing silently.

Inverting it surfaced four PlaybookController endpoints. Those are true
negatives — `Playbook` has no connectionId field, playbooks are global
templates, and the endpoints in that file which *do* carry a connection are
already guarded. `playbookId` is therefore exempt, and
`playbookExemptionHoldsOnlyWhilePlaybooksAreConnectionFree` fails the build
if a connectionId is ever added to the entity, so the exemption cannot start
hiding those four endpoints later.

`GET /projects` with no filter spans every connection and cannot be
authorized against one grant, so it now filters to connections the caller can
read, resolving access once per distinct connectionId rather than once per
project (`ConnectionAccessService.resolveAccess` is uncached and hits the
grant table).

2. setBaseline trusted the path connectionId while mutating an unconstrained
   snapshotId

`POST /schema-changes/{connectionId}/snapshots/{snapshotId}/set-baseline`
asserted manage on the connection, then flipped whatever snapshot id it was
handed to BASELINE and pointed that connection's drift config at it. Manage
access on A was enough to retarget B's snapshot and bind A's baseline to it —
the same id-mismatch class this branch claimed to have closed, split across
two path variables instead of hiding in a body.

The controller now binds the snapshot to the path connection, and
`setBaseline` enforces it again in the service so the invariant does not
depend on the caller. It throws rather than skipping: silently no-op'ing the
snapshot write while still writing the drift config would leave the config
referencing another connection's snapshot.

3. The existence oracle was still open, and the comments claimed otherwise

The id helpers 404'd an unknown id but left an existing-but-unauthorized row
at 403, so the pair still confirmed which ids are real —
`query_performance_regression.id` is a sequential Long, so walking 1..N would
have mapped every tenant's regressions. The code comments asserting "404 so
it cannot be used to probe" described only the half that was implemented.

Added `assertCanRead/ManageConnectionContentOrNotFound`, which answers 404 for
both cases, matching what `DashboardWorkspaceService.assertCanReadDashboard`
already does for a dashboard outside the caller's workspace. Applied to all
nine id-keyed helpers. Endpoints keyed on a connectionId keep 403 on purpose:
the caller already knows that connection exists, so an actionable "access
denied" beats a misleading 404. Comments corrected to state the property the
code now has.

Medium items from the same review

* `compareSnapshots` and `setBaseline` threw IllegalArgumentException for a
  missing or cross-connection snapshot, which surfaced as 500. Both now map it
  to 404 — "not something you can compare" is not a server fault, and a 500
  reads as a broken feature.
* Remaining actor fields: Sentinel's `initiatedBy` came from the request body,
  and `acknowledgedBy` still defaulted to the literal string "user" in three
  places. All now use requireCurrentUsername(). The parameters stay accepted
  and ignored, noted at each site so nobody re-wires them.

Verification

Real Maven compile of main and test sources, zero errors. All seven
safety-test cases green against the tree.

Live, against the rebuilt image with a DEVELOPER who has manage on connection
G and no grant on connection U:
  - POST /projects on U -> 403; on G -> 200
  - GET/PUT/DELETE a project owned by U -> 404, 404, 404 (not 403, not 200);
    row intact afterwards; not leaked through the unfiltered list
  - set-baseline binding U's snapshot as G's baseline -> 404; snapshot stayed
    MANUAL; G's drift config still unbound
  - existing-but-unauthorized vs nonexistent history id -> 404 and 404,
    indistinguishable; row survived the refused DELETE
  - 8/8 reads on G still non-403, and the project list shows only G's project

Not covered: mvn test still has not been executed (the image build uses
-DskipTests and this host has no JDK/Maven), so the seven cases are
compile-verified and logic-validated but not JUnit-run.

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

Author: notSumit25

CLAUDE.md

@@ -749,18 +749,48 @@ it against a real database — not a theoretical hardening pass.
   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.
+  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`, 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.
+  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

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

@@ -298,12 +298,13 @@ public static class AffectedQueryItem {
     /**
      * Authorize a write keyed only on an action id. The action carries its own
      * connectionId, so resolve that first and assert against it — an action id
-     * is not a capability. An unknown id reports 404 rather than 403 so the
-     * endpoint cannot be used to probe which action ids exist.
+     * is not a capability. An unknown id and one on a connection the caller cannot
+     * manage both report 404, so the endpoint cannot be used to probe which action
+     * ids exist.
      */
     private void assertCanManageAction(String actionId) {
         PerformanceAction action = aggregatorService.getActionById(actionId)
                 .orElseThrow(() -> new ResponseStatusException(HttpStatus.NOT_FOUND, "Action not found"));
-        accessControlService.assertCanManageConnectionContent(action.getConnectionId());
+        accessControlService.assertCanManageConnectionContentOrNotFound(action.getConnectionId(), "Action");
     }
 }

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

@@ -8,16 +8,21 @@
 import org.springframework.http.ResponseEntity;
 import org.springframework.web.bind.annotation.*;
 
+import java.util.HashMap;
 import java.util.List;
+import java.util.Map;
 
 /**
  * REST API for projects, optionally filtered by connection.
  *
- * <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}.
+ * <p><b>Authorization:</b> a project belongs to a connection, so every endpoint is gated
+ * on that connection's ACL — directly where the request carries a {@code connectionId},
+ * and via the project's own {@code connectionId} where the path carries only a
+ * {@code projectId}. {@code SecurityConfig} only requires an authenticated principal;
+ * nothing upstream inspects a connection id.
+ *
+ * <p>The id-keyed endpoints report 404 rather than 403 for a project the caller may not
+ * touch, so the route cannot be used to test which project ids exist.
  */
 @RestController
 @RequestMapping("/projects")
@@ -28,6 +33,7 @@ public class ProjectController {
 
     @PostMapping
     public ResponseEntity<Project> createProject(@RequestBody CreateProjectRequest request) {
+        accessControlService.assertCanManageConnectionContent(request.getConnectionId());
         Project project = projectService.createProject(
             request.getName(),
             request.getDescription(),
@@ -42,32 +48,42 @@ public ResponseEntity<List<Project>> listProjects(
     ) {
         if (connectionId != null) {
             accessControlService.assertCanReadConnectionContent(connectionId);
+            return ResponseEntity.ok(projectService.getProjectsByConnection(connectionId));
         }
-        List<Project> projects = connectionId != null
-            ? projectService.getProjectsByConnection(connectionId)
-            : projectService.getAllProjects();
-        return ResponseEntity.ok(projects);
+        // No filter means "every project on every connection", which cannot be authorized
+        // against a single connection's grants — so it is scoped to the caller instead.
+        // Access is resolved once per distinct connection, not once per project:
+        // ConnectionAccessService.resolveAccess is uncached and hits the grant table, and
+        // many projects share a connection.
+        Map<String, Boolean> readable = new HashMap<>();
+        return ResponseEntity.ok(projectService.getAllProjects().stream()
+            .filter(p -> readable.computeIfAbsent(
+                    String.valueOf(p.getConnectionId()), c -> canRead(p.getConnectionId())))
+            .toList());
     }
 
     @GetMapping("/{projectId}")
     public ResponseEntity<Project> getProject(@PathVariable String projectId) {
-        return projectService.getProject(projectId)
-            .map(ResponseEntity::ok)
-            .orElse(ResponseEntity.notFound().build());
+        Project project = requireProject(projectId);
+        accessControlService.assertCanReadConnectionContentOrNotFound(
+                project.getConnectionId(), "Project");
+        return ResponseEntity.ok(project);
     }
 
     @PutMapping("/{projectId}")
     public ResponseEntity<Project> updateProject(
         @PathVariable String projectId,
         @RequestBody UpdateProjectRequest request
     ) {
+        assertCanManageProject(projectId);
         return projectService.updateProject(projectId, request.getName(), request.getDescription())
             .map(ResponseEntity::ok)
             .orElse(ResponseEntity.notFound().build());
     }
 
     @DeleteMapping("/{projectId}")
     public ResponseEntity<Void> deleteProject(@PathVariable String projectId) {
+        assertCanManageProject(projectId);
         return projectService.deleteProject(projectId)
             ? ResponseEntity.ok().build()
             : ResponseEntity.notFound().build();
@@ -80,6 +96,31 @@ public static class CreateProjectRequest {
         private String connectionId;
     }
 
+    private Project requireProject(String projectId) {
+        return projectService.getProject(projectId)
+            .orElseThrow(() -> new org.springframework.web.server.ResponseStatusException(
+                    org.springframework.http.HttpStatus.NOT_FOUND, "Project not found"));
+    }
+
+    /** A project id is not a capability: authorize the connection that owns the project. */
+    private void assertCanManageProject(String projectId) {
+        accessControlService.assertCanManageConnectionContentOrNotFound(
+                requireProject(projectId).getConnectionId(), "Project");
+    }
+
+    /** Non-throwing read check, for filtering a cross-connection list. */
+    private boolean canRead(String connectionId) {
+        if (connectionId == null) {
+            return false;
+        }
+        try {
+            accessControlService.assertCanReadConnectionContent(connectionId);
+            return true;
+        } catch (org.springframework.web.server.ResponseStatusException e) {
+            return false;
+        }
+    }
+
     @Data
     public static class UpdateProjectRequest {
         private String name;

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

@@ -280,14 +280,15 @@ public ResponseEntity<Map<String, Object>> triggerAnalysis(@PathVariable String
      * Authorize a write keyed only on a regression id. The regression carries
      * its own connectionId, so resolve that and assert against it — a
      * regression id is not a capability, and these ids are sequential Longs,
-     * so they are trivially enumerable. An unknown id reports 404 so the
-     * endpoint cannot be used to probe which regressions exist.
+     * so they are trivially enumerable — walking 1..N would otherwise map out every
+     * tenant's regressions. An unknown id and one the caller cannot manage both report
+     * 404, so the response does not distinguish them.
      */
     private void assertCanManageRegression(Long regressionId) {
         String connectionId = queryPerformanceService.findConnectionIdForRegression(regressionId)
                 .orElseThrow(() -> new org.springframework.web.server.ResponseStatusException(
                         org.springframework.http.HttpStatus.NOT_FOUND, "Regression not found"));
-        accessControlService.assertCanManageConnectionContent(connectionId);
+        accessControlService.assertCanManageConnectionContentOrNotFound(connectionId, "Regression");
     }
 
     /** The authenticated caller. Never trust a client-supplied actor name. */

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

@@ -108,7 +108,10 @@ public ResponseEntity<List<QueryPlanComparison>> getUnacknowledgedRegressions(@P
     public ResponseEntity<Map<String, Object>> acknowledgeRegressions(
             @PathVariable String connectionId,
             @RequestBody List<String> comparisonIds,
-            @RequestParam(required = false, defaultValue = "user") String acknowledgedBy) {
+            @RequestParam(required = false) String acknowledgedBy) {
+            // Accepted for wire compatibility and deliberately ignored: the actor is
+            // taken from the security context below. It previously defaulted to the
+            // literal string "user", so the trail named nobody.
         accessControlService.assertCanManageConnectionContent(connectionId);
 
         if (!planCacheService.allComparisonsBelongTo(connectionId, comparisonIds)) {
@@ -135,12 +138,13 @@ public ResponseEntity<Map<String, Object>> getPlanStats(@PathVariable String con
     /**
      * Authorize a write keyed only on a plan id. The cached plan carries its own
      * connectionId, so resolve that and assert against it. An unknown id reports
-     * 404 so the endpoint cannot be used to probe which plan ids exist.
+     * 404 — as does a plan belonging to a connection the caller cannot manage, so the
+     * endpoint cannot be used to probe which plan ids exist.
      */
     private void assertCanManagePlan(String planId) {
         String connectionId = planCacheService.findConnectionIdForPlan(planId)
                 .orElseThrow(() -> new org.springframework.web.server.ResponseStatusException(
                         org.springframework.http.HttpStatus.NOT_FOUND, "Plan not found"));
-        accessControlService.assertCanManageConnectionContent(connectionId);
+        accessControlService.assertCanManageConnectionContentOrNotFound(connectionId, "Plan");
     }
 }

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

@@ -80,8 +80,14 @@ public ResponseEntity<Map<String, String>> setBaseline(
             @PathVariable String connectionId,
             @PathVariable String snapshotId) {
         accessControlService.assertCanManageConnectionContent(connectionId);
+        assertSnapshotBelongsTo(connectionId, snapshotId);
 
-        schemaChangeService.setBaseline(connectionId, snapshotId);
+        try {
+            schemaChangeService.setBaseline(connectionId, snapshotId);
+        } catch (IllegalArgumentException e) {
+            throw new org.springframework.web.server.ResponseStatusException(
+                    org.springframework.http.HttpStatus.NOT_FOUND, e.getMessage());
+        }
         return ResponseEntity.ok(Map.of(
                 "status", "success",
                 "message", "Snapshot set as baseline"
@@ -98,7 +104,15 @@ public ResponseEntity<List<SchemaChange>> compareSnapshots(
 
         assertCanReadSnapshot(snapshotId1);
         assertCanReadSnapshot(snapshotId2);
-        return ResponseEntity.ok(schemaChangeService.compareSnapshots(snapshotId1, snapshotId2));
+        try {
+            return ResponseEntity.ok(schemaChangeService.compareSnapshots(snapshotId1, snapshotId2));
+        } catch (IllegalArgumentException e) {
+            // Missing snapshot, or two snapshots from different connections. Both are
+            // "not something you can compare", not a server fault — a 500 here would read
+            // as a broken feature and hide the real reason.
+            throw new org.springframework.web.server.ResponseStatusException(
+                    org.springframework.http.HttpStatus.NOT_FOUND, e.getMessage());
+        }
     }
 
     // ==================== Change Endpoints ====================
@@ -128,7 +142,10 @@ public ResponseEntity<List<SchemaChange>> getUnacknowledgedChanges(@PathVariable
     public ResponseEntity<Map<String, Object>> acknowledgeChanges(
             @PathVariable String connectionId,
             @RequestBody List<String> changeIds,
-            @RequestParam(required = false, defaultValue = "user") String acknowledgedBy) {
+            @RequestParam(required = false) String acknowledgedBy) {
+            // Accepted for wire compatibility and deliberately ignored: the actor is
+            // taken from the security context below. It previously defaulted to the
+            // literal string "user", so the trail named nobody.
         accessControlService.assertCanManageConnectionContent(connectionId);
 
         assertChangesBelongTo(connectionId, changeIds);
@@ -145,7 +162,10 @@ public ResponseEntity<Map<String, Object>> acknowledgeChanges(
     @PostMapping("/{connectionId}/changes/acknowledge-all")
     public ResponseEntity<Map<String, Object>> acknowledgeAllChanges(
             @PathVariable String connectionId,
-            @RequestParam(required = false, defaultValue = "user") String acknowledgedBy) {
+            @RequestParam(required = false) String acknowledgedBy) {
+            // Accepted for wire compatibility and deliberately ignored: the actor is
+            // taken from the security context below. It previously defaulted to the
+            // literal string "user", so the trail named nobody.
         accessControlService.assertCanManageConnectionContent(connectionId);
 
         int count = schemaChangeService.acknowledgeAllChanges(
@@ -207,13 +227,14 @@ public ResponseEntity<Map<String, Object>> triggerDriftCheck(@PathVariable Strin
     /**
      * Authorize a read keyed only on a snapshot id. The snapshot carries its own
      * connectionId, so resolve that and assert against it. An unknown id reports
-     * 404 so the endpoint cannot be used to probe which snapshots exist.
+     * 404 — as does a snapshot on a connection the caller cannot read, so the endpoint
+     * cannot be used to probe which snapshots exist.
      */
     private void assertCanReadSnapshot(String snapshotId) {
         String connectionId = schemaChangeService.findConnectionIdForSnapshot(snapshotId)
                 .orElseThrow(() -> new org.springframework.web.server.ResponseStatusException(
                         org.springframework.http.HttpStatus.NOT_FOUND, "Snapshot not found"));
-        accessControlService.assertCanReadConnectionContent(connectionId);
+        accessControlService.assertCanReadConnectionContentOrNotFound(connectionId, "Snapshot");
     }
 
     /**
@@ -227,4 +248,19 @@ private void assertChangesBelongTo(String connectionId, List<String> changeIds)
                     org.springframework.http.HttpStatus.NOT_FOUND, "Change not found for this connection");
         }
     }
+
+    /**
+     * The snapshot id is a separate path variable from the connection id, so authorizing
+     * the connection says nothing about the snapshot. Without this a caller with manage
+     * access on connection A could flip connection B's snapshot to BASELINE and point A's
+     * drift config at it — the same body/path id-mismatch class as
+     * {@code changes/acknowledge}, just split across two path variables instead.
+     */
+    private void assertSnapshotBelongsTo(String connectionId, String snapshotId) {
+        String owner = schemaChangeService.findConnectionIdForSnapshot(snapshotId).orElse(null);
+        if (!connectionId.equals(owner)) {
+            throw new org.springframework.web.server.ResponseStatusException(
+                    org.springframework.http.HttpStatus.NOT_FOUND, "Snapshot not found");
+        }
+    }
 }