Skip to content

Commit 2da9721

Browse files
notSumit25claude
andcommitted
fix(brain): authorize every Brain endpoint against its connection (#70)
BrainController shipped with 93 of its 116 endpoints performing no authorization at all. SecurityConfig only asserts `.anyRequest().authenticated()` and JwtAuthenticationFilter only resolves a principal — neither inspects a connectionId, and there is no filter, interceptor or aspect that does. Connections are private per user (ConnectionAccessService.resolveAccess keys on ownerUsername plus an explicit grant table), so any authenticated user who passed somebody else's connection id to /brain/health-scores/{id}, /brain/data-sensitivity/{id} (which names the PII columns), /brain/cost-attribution/{id}, /brain/ml-overview/{id} and ~90 others got that user's schema, sensitivity, cost and workload intelligence back. Only the first ~15 endpoints had the check. The misses clustered by when a section was written — every later "Phase" block omitted it — not by read/write semantics, so the scalability, brain-score, classification, column-values, insights, workload, config-tuning, statistics, executions, patterns, ml-overview and query-intelligence families were open in full. All 116 now authorize: assertCanReadConnectionContent for GETs, assertCanManageConnectionContent for writes. Endpoints whose path carries some other id resolve the owning connection first, via three new getConnectionId lookups (ScalabilitySimulationService, ConfigTuningService, PlanPatternLibraryService) matching the existing BrainNoteService / BrainTaskService precedent. Two endpoints have no connection scope and are admin-only instead: /column-values/embed-all spans every connection, and /key-columns/anti-pattern/{patternId}/acknowledge is an unimplemented stub whose body never loads the anti-pattern, so there is nothing to authorize against yet. Asserts go inside each handler's try, before the catch-all that returns 500, relying on the existing `catch (ResponseStatusException e) { throw e; }` so a denial surfaces as a real 403 rather than looking like a broken feature. BrainControllerAuthorizationSafetyTest locks both properties structurally — every mapping is authorized, and every inline assert rethrows. Verified it fails on 92 endpoints against the pre-fix file and passes on 0 after, so it catches the regression rather than passing vacuously. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 088ac35 commit 2da9721

6 files changed

Lines changed: 280 additions & 0 deletions

File tree

CLAUDE.md

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -349,6 +349,42 @@ it against a real database — not a theoretical hardening pass.
349349
`POST /users/admin/reset` on every install that had run `setup-agent.sh`, since that
350350
mints an admin MCP token on each run.
351351

352+
### Endpoint Authorization Rules
353+
354+
- **Authentication is not authorization.** `SecurityConfig` only asserts
355+
`.anyRequest().authenticated()` and `JwtAuthenticationFilter` only resolves a
356+
principal — neither looks at a `connectionId`. Connections are **private per user**
357+
(`ConnectionAccessService.resolveAccess` keys on `ownerUsername` plus an explicit
358+
grant table), so any endpoint taking a caller-supplied `connectionId` **must** call
359+
`accessControlService.assertCanReadConnectionContent` (reads) or
360+
`assertCanManageConnectionContent` (writes) itself. There is no filter, interceptor
361+
or aspect that does this for you.
362+
- **`BrainController` shipped with 93 of its 116 endpoints unguarded.** Only the first
363+
~15 (`/understanding`, `/notes/*`, `/tasks/*`, `/key-columns/*`,
364+
`/inferred-relationships/*`) had the check; every later "Phase" block did not — so an
365+
authenticated user could pass someone else's connection id to
366+
`/brain/health-scores/{id}`, `/brain/data-sensitivity/{id}` (which names the PII
367+
columns), `/brain/cost-attribution/{id}`, `/brain/ml-overview/{id}` and ~90 more and
368+
read that user's database intelligence. All 116 are now guarded, and
369+
`BrainControllerAuthorizationSafetyTest` fails the build if a new one is not. The
370+
misses clustered by **when a section was written**, not by read/write semantics —
371+
when adding a controller section, guard it as you write it.
372+
- **When the path carries some other id** (`simulationId`, `experimentId`, `patternId`,
373+
`noteId`, `taskId`), resolve the owning connection first via that service's
374+
`getConnectionId(id)` and assert on the result. Do not skip the check because the
375+
path has no `connectionId` in it.
376+
- **An endpoint with no connection scope at all is admin-only.**
377+
`POST /brain/column-values/embed-all` spans every connection, so it carries
378+
`@PreAuthorize("hasRole('ADMIN')")` — it cannot be authorized against one
379+
connection's grants. `@EnableMethodSecurity(prePostEnabled = true)` is on in
380+
`SecurityConfig`, so `@PreAuthorize` is live.
381+
- **Assert inside the `try`, and rethrow `ResponseStatusException` before the
382+
catch-all.** Every handler in `BrainController` ends with a
383+
`catch (Exception) -> 500`; without the earlier
384+
`catch (ResponseStatusException e) { throw e; }` a 403 is swallowed and reported as a
385+
server error, so a client cannot tell "not yours" from "broken". The safety test
386+
asserts this too.
387+
352388
### MCP & CLI Release Rules
353389

354390
**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:**

0 commit comments

Comments
 (0)