diff --git a/CLAUDE.md b/CLAUDE.md index 52aac16..091b7ce 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1088,6 +1088,25 @@ it against a real database — not a theoretical hardening pass. while an allowed origin gets `200` + ACAO — so the annotation never had effect. It still reads like an intentional hole to the next person. +- **A guard can be present and still authorize the wrong id.** Twelve endpoints across + `CodeScanController` (8), `CompanyKnowledgeController` (2) and `DashboardAlertController` (2) + each called `assertCanManageConnectionContent` — so the scanner passed them — but on an id + unrelated to what they operated on. The scan endpoints checked a `@RequestParam connectionId` + the caller owns while deleting/reading a `@PathVariable sourceId`/`jobId`/`suggestionId` + belonging to another tenant; `CompanyKnowledgeController.update` wrapped its guard in + `if (entry.getConnectionId() != null)`, so omitting the field skipped it entirely; + `DashboardAlertController` authorized `dashboardId` but acted on an `alertId` never bound to + it. Verified live: `analyst` (grant on one connection) deleted another tenant's scan source, + deleted and overwrote a knowledge entry, and deleted an alert — **200 unpatched, 404 patched** + in every case. **Resolve the row's own connection and assert on that** — `CodeScanService` + `findConnectionIdForSource/Job/Suggestion`, `CompanyKnowledgeService.findConnectionIdForEntry`, + `DashboardAlertService.findDashboardIdForAlert` — never a caller-supplied id. **404 for both + "unknown" and "not yours"** so the endpoint is not an existence oracle; **`bulk-decide` checks + every id and fails on one that resolves to nothing**. The `ConnectionScopedAuthorizationSafetyTest` + `AUTHORIZED` regex is presence-only (does *an* assert appear), not dataflow (does it assert on + the *right* id), so it cannot catch this class — the live cross-tenant test is the real guard. + See `docs/security/2026-09-16-wrong-id-authorization.md`. + ### MCP & CLI Release Rules **Whenever you add, rename, or remove an MCP tool or a CLI subcommand, you MUST update all of these in the same commit — they are agent-facing surfaces and drift silently breaks discoverability:** diff --git a/backend/src/main/java/com/dbaagent/controller/CodeScanController.java b/backend/src/main/java/com/dbaagent/controller/CodeScanController.java index fb3198c..9d6d992 100644 --- a/backend/src/main/java/com/dbaagent/controller/CodeScanController.java +++ b/backend/src/main/java/com/dbaagent/controller/CodeScanController.java @@ -57,7 +57,7 @@ public ResponseEntity updateFocus( @RequestParam("connectionId") String connectionId, @RequestBody UpdateFocusRequest body ) { - accessControlService.assertCanManageConnectionContent(connectionId); + assertCanManageSource(sourceId); return ResponseEntity.ok( codeScanService.updateFocus(sourceId, body == null ? null : body.focus()) ); @@ -72,7 +72,7 @@ public ResponseEntity> listSources(@RequestParam("connectio @DeleteMapping("/sources/{sourceId}") public ResponseEntity deleteSource(@PathVariable String sourceId, @RequestParam("connectionId") String connectionId) { - accessControlService.assertCanManageConnectionContent(connectionId); + assertCanManageSource(sourceId); codeScanService.deleteSource(sourceId); return ResponseEntity.ok().build(); } @@ -86,7 +86,7 @@ public ResponseEntity startScan( @RequestParam(value = "focus", required = false) String focus, @RequestParam("file") MultipartFile file ) throws IOException { - accessControlService.assertCanManageConnectionContent(connectionId); + assertCanManageSource(sourceId); return ResponseEntity.ok( codeScanService.startScan(sourceId, file, focus, accessControlService.getCurrentUsername()) ); @@ -95,7 +95,7 @@ public ResponseEntity startScan( @GetMapping("/jobs/{jobId}") public ResponseEntity getJob(@PathVariable String jobId, @RequestParam("connectionId") String connectionId) { - accessControlService.assertCanManageConnectionContent(connectionId); + assertCanManageJob(jobId); return codeScanService.getJob(jobId) .map(ResponseEntity::ok) .orElseGet(() -> ResponseEntity.notFound().build()); @@ -104,14 +104,14 @@ public ResponseEntity getJob(@PathVariable String jobId, @GetMapping("/sources/{sourceId}/jobs") public ResponseEntity> listJobs(@PathVariable String sourceId, @RequestParam("connectionId") String connectionId) { - accessControlService.assertCanManageConnectionContent(connectionId); + assertCanManageSource(sourceId); return ResponseEntity.ok(codeScanService.recentJobs(sourceId)); } @GetMapping(value = "/jobs/{jobId}/stream", produces = MediaType.TEXT_EVENT_STREAM_VALUE) public SseEmitter streamJob(@PathVariable String jobId, @RequestParam("connectionId") String connectionId) { - accessControlService.assertCanManageConnectionContent(connectionId); + assertCanManageJob(jobId); return codeScanService.subscribeJob(jobId); } @@ -137,7 +137,7 @@ public ResponseEntity decide( @RequestParam("connectionId") String connectionId, @RequestBody DecideRequest body ) { - accessControlService.assertCanManageConnectionContent(connectionId); + assertCanManageSuggestion(suggestionId); return ResponseEntity.ok( codeScanService.decide( suggestionId, @@ -153,10 +153,15 @@ public ResponseEntity> bulkDecide( @RequestParam("connectionId") String connectionId, @RequestBody BulkDecideRequest body ) { - accessControlService.assertCanManageConnectionContent(connectionId); if (body == null || body.ids() == null || body.ids().isEmpty()) { return ResponseEntity.badRequest().body(Map.of("error", "ids required")); } + // Every id must resolve to a connection the caller can manage, and an id that + // resolves to nothing fails too — otherwise an unknown id rides into an otherwise + // valid batch. connectionId is accepted for wire compatibility but not trusted. + for (String suggestionId : body.ids()) { + assertCanManageSuggestion(suggestionId); + } var result = codeScanService.bulkDecide( body.ids(), body.decision(), @@ -171,6 +176,25 @@ public ResponseEntity> bulkDecide( return ResponseEntity.ok(payload); } + // Resolve the row's own connection and authorise against that — never the caller-supplied + // connectionId, which the caller may legitimately own while the id targets another tenant. + // 404 for both "no such id" and "not yours", so the endpoint is not an existence oracle, + // matching the rule DashboardWorkspaceService.assertCanReadDashboard already follows. + private void assertCanManageSource(String sourceId) { + accessControlService.assertCanManageConnectionContentOrNotFound( + codeScanService.findConnectionIdForSource(sourceId).orElse(null), "Scan source"); + } + + private void assertCanManageJob(String jobId) { + accessControlService.assertCanManageConnectionContentOrNotFound( + codeScanService.findConnectionIdForJob(jobId).orElse(null), "Scan job"); + } + + private void assertCanManageSuggestion(String suggestionId) { + accessControlService.assertCanManageConnectionContentOrNotFound( + codeScanService.findConnectionIdForSuggestion(suggestionId).orElse(null), "Suggestion"); + } + private static CodeKnowledgeSuggestion.Status parseStatus(String s) { try { return CodeKnowledgeSuggestion.Status.valueOf(s.toUpperCase(Locale.ROOT)); diff --git a/backend/src/main/java/com/dbaagent/controller/CompanyKnowledgeController.java b/backend/src/main/java/com/dbaagent/controller/CompanyKnowledgeController.java index 59c1419..0fcb7ee 100644 --- a/backend/src/main/java/com/dbaagent/controller/CompanyKnowledgeController.java +++ b/backend/src/main/java/com/dbaagent/controller/CompanyKnowledgeController.java @@ -36,9 +36,11 @@ public ResponseEntity create(@RequestBody CompanyKnowledg public ResponseEntity update( @PathVariable String entryId, @RequestBody CompanyKnowledgeEntry entry) { - if (entry.getConnectionId() != null && !entry.getConnectionId().isBlank()) { - accessControlService.assertCanManageConnectionContent(entry.getConnectionId()); - } + // Authorise against the stored entry's connection, unconditionally. The old check ran + // only when the body carried a connectionId, so omitting that field skipped it and let + // any authenticated user edit any tenant's entry. The body's connectionId is never + // trusted here; updateEntry already refuses to change it. + assertCanManageEntry(entryId); if (entry.getCreatedBy() == null || entry.getCreatedBy().isBlank()) { entry.setCreatedBy(accessControlService.getCurrentUsername()); } @@ -48,9 +50,15 @@ public ResponseEntity update( @DeleteMapping("/{entryId}") public ResponseEntity delete( @PathVariable String entryId, - @RequestParam String connectionId) { - accessControlService.assertCanManageConnectionContent(connectionId); + @RequestParam(required = false) String connectionId) { + // The connectionId param was never compared to the entry being deleted; authorise on + // the entry's own connection instead. Accepted for wire compatibility, not trusted. + assertCanManageEntry(entryId); companyKnowledgeService.deleteEntry(entryId); return ResponseEntity.ok().build(); } + private void assertCanManageEntry(String entryId) { + accessControlService.assertCanManageConnectionContentOrNotFound( + companyKnowledgeService.findConnectionIdForEntry(entryId).orElse(null), "Knowledge entry"); + } } diff --git a/backend/src/main/java/com/dbaagent/controller/DashboardAlertController.java b/backend/src/main/java/com/dbaagent/controller/DashboardAlertController.java index fb10ed7..4ca3b8e 100644 --- a/backend/src/main/java/com/dbaagent/controller/DashboardAlertController.java +++ b/backend/src/main/java/com/dbaagent/controller/DashboardAlertController.java @@ -9,6 +9,7 @@ import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; import org.springframework.http.HttpStatus; +import org.springframework.web.server.ResponseStatusException; import org.springframework.http.ResponseEntity; import org.springframework.web.bind.annotation.*; @@ -68,6 +69,7 @@ public ResponseEntity> update(@PathVariable UUID dashboardId try { SavedDashboard dashboard = requireDashboard(dashboardId); accessControlService.assertCanManageConnectionContent(dashboard.getConnectionId()); + requireAlertOnDashboard(alertId, dashboardId); DashboardAlert updated = alertService.updateAlert(alertId, updates); return ResponseEntity.ok(Map.of("success", true, "alert", updated)); } catch (IllegalArgumentException e) { @@ -85,6 +87,7 @@ public ResponseEntity> delete(@PathVariable UUID dashboardId try { SavedDashboard dashboard = requireDashboard(dashboardId); accessControlService.assertCanManageConnectionContent(dashboard.getConnectionId()); + requireAlertOnDashboard(alertId, dashboardId); alertService.deleteAlert(alertId); return ResponseEntity.ok(Map.of("success", true)); } catch (org.springframework.web.server.ResponseStatusException e) { @@ -100,6 +103,16 @@ public ResponseEntity> delete(@PathVariable UUID dashboardId * membership gate applies to all of them at once. The connection check stays with * each caller because read and write paths need different assertions. */ + // Bind the alertId to the dashboard we just authorised. Authorising the dashboard is only + // half the check when a second id rides alongside it: 404 (not 403) for an unknown alert or + // one under a different dashboard, so this cannot confirm another dashboard's alert exists. + private void requireAlertOnDashboard(UUID alertId, UUID dashboardId) { + UUID owner = alertService.findDashboardIdForAlert(alertId).orElse(null); + if (owner == null || !owner.equals(dashboardId)) { + throw new ResponseStatusException(HttpStatus.NOT_FOUND, "Alert not found"); + } + } + private SavedDashboard requireDashboard(UUID dashboardId) { SavedDashboard dashboard = savedDashboardService.getDashboardById(dashboardId) .orElseThrow(() -> new IllegalArgumentException("Dashboard not found")); diff --git a/backend/src/main/java/com/dbaagent/service/CompanyKnowledgeService.java b/backend/src/main/java/com/dbaagent/service/CompanyKnowledgeService.java index f034bc7..16983ff 100644 --- a/backend/src/main/java/com/dbaagent/service/CompanyKnowledgeService.java +++ b/backend/src/main/java/com/dbaagent/service/CompanyKnowledgeService.java @@ -404,6 +404,17 @@ public CompanyKnowledgeEntry createEntry(CompanyKnowledgeEntry entry) { return annotateEntry(saved, schema); } + /** + * The connection an entry belongs to, or empty if there is no such entry. The controller + * authorises against this — not a caller-supplied connectionId that it never compares to + * the entry, and that on the update path was only consulted when the caller chose to send + * it, so omitting it skipped the check entirely. + */ + public java.util.Optional findConnectionIdForEntry(String entryId) { + return companyKnowledgeEntryRepository.findById(entryId) + .map(CompanyKnowledgeEntry::getConnectionId); + } + @Transactional public CompanyKnowledgeEntry updateEntry(String entryId, CompanyKnowledgeEntry request) { validate(request, false); diff --git a/backend/src/main/java/com/dbaagent/service/DashboardAlertService.java b/backend/src/main/java/com/dbaagent/service/DashboardAlertService.java index 67abe73..cbcd7fd 100644 --- a/backend/src/main/java/com/dbaagent/service/DashboardAlertService.java +++ b/backend/src/main/java/com/dbaagent/service/DashboardAlertService.java @@ -85,6 +85,17 @@ public void deleteAlert(UUID alertId) { alertRepository.deleteById(alertId); } + /** + * The dashboard an alert belongs to, or empty if there is no such alert. The controller + * authorises the dashboard, then binds the alert to it with this — update/delete took an + * alertId beside the dashboardId and acted on the alert without checking it belonged to the + * authorised dashboard, so a dashboard you own paired with another tenant's alertId let you + * repoint or delete their alert. + */ + public java.util.Optional findDashboardIdForAlert(UUID alertId) { + return alertRepository.findById(alertId).map(DashboardAlert::getDashboardId); + } + private DashboardAlert requireAlert(UUID id) { return alertRepository.findById(id) .orElseThrow(() -> new IllegalArgumentException("Alert not found with id: " + id)); diff --git a/backend/src/main/java/com/dbaagent/service/codescan/CodeScanService.java b/backend/src/main/java/com/dbaagent/service/codescan/CodeScanService.java index 5b4a6da..ae1e179 100644 --- a/backend/src/main/java/com/dbaagent/service/codescan/CodeScanService.java +++ b/backend/src/main/java/com/dbaagent/service/codescan/CodeScanService.java @@ -174,6 +174,30 @@ public List listSources(String connectionId) { return sourceRepository.findByConnectionIdAndActiveTrueOrderByCreatedAtDesc(connectionId); } + /** + * The connection a scan source belongs to, or empty if there is no such source. + * + *

The controller authorises against this, not against a caller-supplied {@code + * connectionId}. Every scan endpoint took a {@code @RequestParam connectionId} beside a + * {@code @PathVariable sourceId}/{@code jobId} and asserted on the param — so a caller + * passed a connection they own next to another tenant's source id, the assert passed, and + * the operation hit a row they had no access to. + */ + public java.util.Optional findConnectionIdForSource(String sourceId) { + return sourceRepository.findById(sourceId).map(CodeScanSource::getConnectionId); + } + + /** The connection a scan job belongs to, or empty if there is no such job. */ + public java.util.Optional findConnectionIdForJob(String jobId) { + return jobRepository.findById(jobId).map(CodeScanJob::getConnectionId); + } + + /** The connection a suggestion belongs to, or empty if there is no such suggestion. */ + public java.util.Optional findConnectionIdForSuggestion(String suggestionId) { + return suggestionRepository.findById(suggestionId) + .map(CodeKnowledgeSuggestion::getConnectionId); + } + @Transactional public void deleteSource(String sourceId) { sourceRepository.findById(sourceId).ifPresent(s -> { diff --git a/docs/security/2026-09-16-wrong-id-authorization.md b/docs/security/2026-09-16-wrong-id-authorization.md new file mode 100644 index 0000000..42073f4 --- /dev/null +++ b/docs/security/2026-09-16-wrong-id-authorization.md @@ -0,0 +1,130 @@ +# Twelve endpoints authorized the wrong id + +*Found 2026-09-10 in a repository-wide security audit; reproduced live against the running +stack 2026-09-16. Severity: high (cross-tenant read, write and delete).* + +## What was wrong + +Every one of these endpoints *had* an access check — which is why the safety scanner passed +them — but the check verified a **different id** than the one the endpoint operated on. + +Two shapes: + +**The check was on a caller-supplied connection, the operation on an unrelated row.** Every +scan endpoint took a `@RequestParam connectionId` beside a `@PathVariable sourceId`/`jobId`, +and asserted on the param: + +```java +@DeleteMapping("/sources/{sourceId}") +public ResponseEntity deleteSource(@PathVariable String sourceId, + @RequestParam("connectionId") String connectionId) { + accessControlService.assertCanManageConnectionContent(connectionId); // checks connectionId + codeScanService.deleteSource(sourceId); // deletes sourceId +} +``` + +The caller controls both. They pass a connection they *do* own (so the check passes) and a +`sourceId` belonging to a **different** tenant. The service resolves it by `findById(sourceId)` +with no ownership filter, and the operation lands on a row they have no access to. + +**The check was made conditional on a field the attacker controls.** +`CompanyKnowledgeController.update` ran its guard only when the body carried a `connectionId`: + +```java +if (entry.getConnectionId() != null && !entry.getConnectionId().isBlank()) { + accessControlService.assertCanManageConnectionContent(entry.getConnectionId()); +} +``` + +So the attacker simply **omits the field**. The guard is skipped entirely and +`updateEntry(entryId, ...)` overwrites any tenant's entry. The service even *rejects* a +mismatched `connectionId`, so supplying it correctly triggers the check while omitting it +skips both — the safe-looking validation is exactly what makes omission the best move. + +The twelve: + +| Controller | Endpoints | Unchecked id | +|---|---|---| +| `CodeScanController` | `PUT /sources/{id}/focus`, `DELETE /sources/{id}`, `POST /sources/{id}/scan`, `GET /jobs/{id}`, `GET /sources/{id}/jobs`, `GET /jobs/{id}/stream`, `POST /suggestions/{id}/decide`, `POST /suggestions/bulk-decide` | `sourceId` / `jobId` / `suggestionId` (bulk: a whole list) | +| `CompanyKnowledgeController` | `PUT /{entryId}`, `DELETE /{entryId}` | `entryId` | +| `DashboardAlertController` | `PUT /{alertId}`, `DELETE /{alertId}` | `alertId` (dashboard authorized, alert not bound to it) | + +The dashboard-alert pair is the "two path variables" case: `requireDashboard(dashboardId)` +authorized the dashboard correctly, but `updateAlert(alertId)` / `deleteAlert(alertId)` acted on +an alert never checked to belong to that dashboard. Pair a dashboard you own with another +tenant's `alertId` and you repoint or delete their alert. + +## Reproduced live, not inferred + +Two tenants on the running stack: `analyst` holds a grant on **Demo Shop** only; `admin` owns +**QA Vault Copy**. Victim rows (a scan source, a knowledge entry, a dashboard alert) were +planted on QA Vault Copy, then `analyst` attacked each by passing their *own* connection/dashboard +beside the victim's row id: + +| Attack | Unpatched | Patched | +|---|---|---| +| Delete another tenant's scan source | **200**, `active` t→f | **404**, survives | +| Delete another tenant's knowledge entry | **200**, row 1→0 | **404**, survives | +| Overwrite another tenant's entry via PUT with no `connectionId` | **200**, title → "PWNED by analyst" | **404**, unchanged | +| Delete another tenant's alert via own dashboard + their alertId | **200**, alert 1→0 | **404**, survives | + +And the fix does not break legitimate access, also verified live: `analyst` deletes a source on +their *own* granted connection (200), and `admin` reads QA Vault Copy's rows (200). + +## The fix + +Resolve the row's **own** connection and authorize against that; never trust the +caller-supplied id. + +- `CodeScanService` gained `findConnectionIdForSource/Job/Suggestion`; the controller asserts + through `assertCanManage{Source,Job,Suggestion}` helpers. +- `CompanyKnowledgeService.findConnectionIdForEntry`; the controller asserts + unconditionally on the entry's connection (the `connectionId` field is no longer consulted + for the check). +- `DashboardAlertService.findDashboardIdForAlert`; the controller binds the alert to the + already-authorized dashboard with `requireAlertOnDashboard`. + +Two rules held throughout: + +- **404, not 403**, for both "no such id" and "not yours" (`assertCan...OrNotFound`, and the + alert-binding throws `NOT_FOUND`), so the endpoint is not an existence oracle — the caller + cannot tell a row they may not touch from one that does not exist. This matches + `DashboardWorkspaceService.assertCanReadDashboard`. +- **`bulk-decide` checks every id, and an id that resolves to nothing fails too**, so an unknown + id cannot ride into an otherwise valid batch. + +The `connectionId` params are still accepted (wire compatibility) and ignored, noted at each +site so nobody re-wires them. + +## Why the scanner missed it, and why it still does + +`ConnectionScopedAuthorizationSafetyTest`'s `AUTHORIZED` check is presence-only: it asks whether +*some* `assertCan...` call appears in the handler body, not *which id* it verifies. All twelve +contained an assert, so all twelve passed. A general dataflow scanner that proved "the assert's +argument is derived from the operated-upon id" is a much larger undertaking than this fix and is +not attempted here; the durable protection is the live cross-tenant test above, which is the +same standard the audit itself used. The safety test still passes because the new helpers +contain `assertCan...`, which is correct — they now assert on the resolved connection. + +## Verification + +| Step | Result | +|---|---| +| Live cross-tenant attacks, 4 variants | **200 unpatched → 404 patched**, every one | +| Legitimate access (own connection; admin on own rows) | **200**, unchanged | +| Backend suites | **32 tests, 0 failures** | +| `mvn compile` | clean | + +Both users' password hashes and every planted row were restored; the database is back to its +prior state (all three tables empty as they were). + +## Residual work + +- **`DashboardAlertController` returns 500, not 404, for a non-existent `dashboardId`.** + `requireDashboard` throws `IllegalArgumentException("Dashboard not found")`, and the delete + handler's catch block maps only `ResponseStatusException` and the generic `Exception` — so the + missing-dashboard case falls through to 500. Surfaced by this QA but pre-existing and separate + from the wrong-id fix; it deserves its own change (map `IllegalArgumentException` to 404, as + the update handler already does). +- A dataflow-aware version of the authorization scanner would catch this class structurally. + Worth doing, but a research-shaped task rather than a fix.