Skip to content

Commit 323da34

Browse files
notSumit25claude
andauthored
Fix/brain endpoint authz (#72)
## Problem 93 of 116 `BrainController` endpoints performed **no authorization** — only authentication. `SecurityConfig` asserts `.anyRequest().authenticated()` and `JwtAuthenticationFilter` only resolves a principal; neither inspects a `connectionId`, and no filter/interceptor/aspect fills the gap. Connections are private per user (`ConnectionAccessService.resolveAccess` keys on `ownerUsername` + an explicit grant table), so any authenticated user with someone else's connection id could read their database intelligence. **Verified live against the pre-fix binary (v1.2.0):** a CHAT_EDITOR user pulled **91 KB of health-score data**, **71 KB of cost-attribution data**, and **131 table names** from a connection they were never granted — all HTTP 200. Same calls are 403 after this change. ## Fix - All 116 endpoints now authorize: `assertCanReadConnectionContent` (GET), `assertCanManageConnectionContent` (writes). - Indirect-ID endpoints (`simulationId`/`experimentId`/`patternId`) resolve the owning connection first via three new `getConnectionId` lookups. - 2 endpoints have no connection scope → `@PreAuthorize("hasRole('ADMIN')")`. - `BrainControllerAuthorizationSafetyTest` fails the build if a new endpoint ships unguarded. ## Found during hands-on QA (2nd commit) - `DELETE /brain/calibration/{id}` used a **fully-qualified** `@DeleteMapping`, so the first sweep's regex skipped it — a *destructive* endpoint left open. The safety test shared the same blind spot and reported "0 unguarded" while it was live. Regex hardened; differential-checked (old pattern: 116/0 → misses; new: 117/1 → catches). - Bogus indirect ids returned 500 instead of 404 (access was still denied). ## Verification API (curl), real browser session (Chrome DevTools MCP), and DB read-back all agree: granted → 200 with real data, ungranted → 403; owner writes still 200 with rows confirmed in Postgres. Adjacent features unaffected. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 088ac35 commit 323da34

6 files changed

Lines changed: 293 additions & 1 deletion

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)