Noticed while reviewing #51. Pre-existing — not introduced by that PR.
SavedDashboardController calls accessControlService.assertCanReadConnectionContent(...) on only three endpoints, all under /share:
| Line |
Endpoint |
Access check |
| 29 |
POST /{id}/share |
✅ line 34 |
| 50 |
PUT /{id}/share/password |
✅ line 55 |
| 70 |
DELETE /{id}/share |
✅ line 75 |
| 92 |
POST / (create) |
❌ none |
| 178 |
PUT /{id} (update) |
❌ none |
| 211 |
DELETE /{id} |
❌ none |
| 237 |
POST /{id}/favorite |
❌ none |
createDashboard accepts a SavedDashboard body carrying an arbitrary connectionId and persists it with no verification that the caller may touch that connection:
@PostMapping
public ResponseEntity<Map<String, Object>> createDashboard(@RequestBody SavedDashboard savedDashboard) {
log.info("Creating saved dashboard: {} for connection: {}", ...);
SavedDashboard created = savedDashboardService.saveDashboard(savedDashboard);
So any authenticated user can create a dashboard attached to a connection they have no access to, and — pending confirmation on PUT/DELETE — read or modify one by id.
Why it matters beyond the write itself
Dashboards are a sharing surface. POST /{id}/share mints a public token and flips is_public, after which PublicDashboardController serves the artifact and runs its queries permitAll. A dashboard whose creation was never access-checked is a poor starting point for that chain.
Suggested fix
Apply the same check the /share endpoints already use, resolving the connection from the persisted row rather than the request body for PUT/DELETE/favorite:
accessControlService.assertCanReadConnectionContent(existing.getConnectionId());
Worth deciding at the same time whether create/update/delete should require a write-level permission rather than assertCanReadConnectionContent, which is a read check. Same question came up on #51: that PR gates dashboard creation behind the read check, which is stricter than this path's current behaviour but still a read permission authorising a write.
Scope
Only SavedDashboardController audited here. Sibling controllers are worth the same pass.
Noticed while reviewing #51. Pre-existing — not introduced by that PR.
SavedDashboardControllercallsaccessControlService.assertCanReadConnectionContent(...)on only three endpoints, all under/share:POST /{id}/sharePUT /{id}/share/passwordDELETE /{id}/sharePOST /(create)PUT /{id}(update)DELETE /{id}POST /{id}/favoritecreateDashboardaccepts aSavedDashboardbody carrying an arbitraryconnectionIdand persists it with no verification that the caller may touch that connection:So any authenticated user can create a dashboard attached to a connection they have no access to, and — pending confirmation on
PUT/DELETE— read or modify one by id.Why it matters beyond the write itself
Dashboards are a sharing surface.
POST /{id}/sharemints a public token and flipsis_public, after whichPublicDashboardControllerserves the artifact and runs its queriespermitAll. A dashboard whose creation was never access-checked is a poor starting point for that chain.Suggested fix
Apply the same check the
/shareendpoints already use, resolving the connection from the persisted row rather than the request body forPUT/DELETE/favorite:Worth deciding at the same time whether create/update/delete should require a write-level permission rather than
assertCanReadConnectionContent, which is a read check. Same question came up on #51: that PR gates dashboard creation behind the read check, which is stricter than this path's current behaviour but still a read permission authorising a write.Scope
Only
SavedDashboardControlleraudited here. Sibling controllers are worth the same pass.