fix: authorize every connection-scoped endpoint (116 were unguarded) - #86
Conversation
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>
venkateshsakamuri-lab
added
the
product: query-performance
Review — regressions / usability / securityVerdict: Conditional NO-GO — close three blockers, then this is a GO. The core sweep is real and high-value; it is not as complete as the PR text claims. What looks strong
Blockers (must fix before merge)1. Only Root cause: 2. Controller asserts manage on the path connection only; service updates whatever snapshot id is supplied and points that connection’s drift config at it. Same body/path ID class this PR claims to have closed — a user with manage on A can retarget B’s snapshot / bind A’s baseline to B. 3. Existence oracle still present (contradicts PR text) ID helpers do: unknown → 404, known-but-unauthorized → 403. That confirms the row exists. Bad for sequential Medium / follow-ups
Regressions / usability
Process
AskPlease fix (1) ProjectController + scanner gap, (2) setBaseline snapshot↔connection bind, (3) unauthorized→404 on ID helpers (at least for sequential IDs). After that, GO. |
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>
…ion' into fix/connection-scoped-authorization
|
All three blockers were real. I verified each against the code before fixing it, and each fix against the running backend afterwards. Pushed in 1. ProjectController + scanner gap. Confirmed: Fixed both halves. The connection match is now case-insensitive, and the id rule is inverted: any Inverting it surfaced four
2. setBaseline cross-id bind. Confirmed: the service flipped whatever 3. Existence oracle. You were right and my comments were wrong: unknown → 404 but existing-but-unauthorized → 403 still confirms which ids are real. Added One deliberate deviation: endpoints keyed on a connectionId keep 403. The caller already knows that connection exists — they typed its id — so hiding it buys nothing and an actionable "access denied" is the better answer. The 404 collapse is for opaque row ids only. Mediums also fixed: Live verification (DEVELOPER with manage on G, no grant on U):
Also merged Still open, and worth stating plainly: On your remaining mediums: the safety test still does not do MockMvc behavioural deny tests (the checks are structural), and the |
|
cursor review |
|
Skipping Bugbot: Unable to authenticate your request. Please make sure Bugbot is properly installed and configured for this repository. |
venkateshsakamuri-lab
left a comment
venkateshsakamuri-lab
left a comment
There was a problem hiding this comment.
update to latest and merge
2 participants
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
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:
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:
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.