Fix/brain endpoint authz - #72
Merged
Merged
Conversation
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>
Found by hands-on QA against the running stack, not by reading the diff.
DELETE /brain/calibration/{connectionId} was annotated
`@org.springframework.web.bind.annotation.DeleteMapping` rather than the bare
`@DeleteMapping` every other handler uses. The sweep that added the access
checks matched on `^\s*@(Get|Post|Delete|Put)Mapping`, so this one endpoint was
skipped — and it is destructive. Verified live: a user holding only a
CHAT_EDITOR grant on a *different* connection got 200 from
`DELETE /brain/calibration/<unshared-id>`; it is 403 for both the read-granted
and the ungranted connection now, and still 200 for the owner.
BrainControllerAuthorizationSafetyTest shared the blind spot exactly — it
reported "116 endpoints, 0 unguarded" while that endpoint was open, which is
the vacuous-pass failure mode CLAUDE.md's verification anti-patterns warn
about. Its mapping pattern now accepts an optional package qualifier;
confirmed it reports 117/1-unguarded against the pre-fix shape and 117/0 after.
Also: the three new getConnectionId lookups throw IllegalArgumentException for
an unknown id, and two of their handlers had no IllegalArgumentException catch,
so a bogus simulation/pattern id returned 500 instead of 404 (observed, with
the stack trace in the backend log). Access was still correctly denied — this
was a status-code and log-noise defect, not an exposure. Both now 404, matching
the pre-existing convention at /inferred-relationships/{id}/validate.
Not fixed here, pre-existing and out of scope: POST /brain/tasks and
POST /brain/tasks/{taskId}/status have the same missing-404 shape via
brainTaskService.getConnectionId, and reproduce on v1.2.0.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
geekypunk
approved these changes
Aug 20, 2026
geekypunk
pushed a commit
that referenced
this pull request
Aug 20, 2026
<!-- CURSOR_AGENT_PR_BODY_BEGIN --> ## What was going wrong A **new Agent chat thread** while Viewing as `marts-editor` still returned `crm.customers` rows (including `amount`). SQL Editor already blocked `crm`. Direct MCP API calls with a marts-editor token also blocked `crm`. The Agent path did not. End-to-end logs for the failing turn (`session=fca723ca8171`, 14:14 UTC): 1. `/api/agent/session` minted MCP token **257** for `marts-editor` and provisioned `u-marts-editor`. 2. Hermes `POST /api/profile/switch` returned **200** three times; the session JSON is tagged `"profile": "u-marts-editor"`. 3. `mcp__deepsql__get_brain_context` and `mcp__deepsql__execute_sql` authenticated as **admin** token **253**. 4. Audit: `EDITOR_QUERY_EXECUTED` / `clientType=mcp` / `user_id=1` (`admin@demo.local`) / `SELECT … FROM crm.customers` / `rowCount=2`. 5. Token 257 `last_used_at` is only the Spring `probeMcpAuth` right after mint — Hermes never sent it. 6. `u-marts-editor` has **no** `mcp-stderr.log`; `u-admin` MCP was started on Aug 18 and reused. Hermes keeps **one** DeepSQL MCP stdio process, started from the first loaded profile (`u-admin`). `POST /api/profile/switch` is explicitly `process_wide=False` (cookie / thread-local only). A new chat does not respawn MCP. `resolveEffectivePolicy(..., actorIsAdmin=true)` returns `none()`, so schema and column policy never run. `probeMcpAuth` only proves the minted token works against Spring, not that the live MCP process will use it. ## Fix `scripts/local-agent-provisioner.py` (the Agent container's `/provision` handler) now mirrors the minted token onto `$HERMES_HOME/deepsql.token` **and every** `profiles/*/deepsql.token`. The long-lived MCP client re-reads `DEEPSQL_TOKEN_FILE` per request (mtime cache), so the next `execute_sql` authenticates as the viewed-as user and policy applies. Chat-path policy is also fail-closed for unparseable/unhandled SQL and missing actors, and walks CTEs, UNIONs, and nested `FROM`/`WHERE` selects. ## Merge with main Resolved conflicts with `main` (`#70` whole-statement schema allowlist + nested protected-table inspectability, `#72` brain endpoint authz). Kept this branch's recursive SELECT walker and fail-closed parse/actor rules. Took main's `assertProtectedTablesAreInspectable` / `namesMatch` so a qualified protection (`public.customer_profiles`) does not catch the same table name in another schema, while an unqualified nested reference still fails closed. `UserDataAccessPolicyServiceTest` (24) and `BrainControllerAuthorizationSafetyTest` (2) pass after the merge. ## How to verify 1. View as `marts-editor`, Agent tab, **New chat**. 2. Ask: `show me all customer information from crm tables`. 3. Expect a policy block, not Acme/Globex rows. `security_event.user_id` for that SQL must be the editor, not admin. Rebuild/restart `deepsql-agent` so the container picks up the provisioner change, or copy `scripts/local-agent-provisioner.py` onto `/opt/deepsql-agent/provisioner.py` and restart the provisioner process, then reopen the Agent tab (triggers `/provision`). ## Re-test after the fix Same prompt in a new Agent thread while Viewing as marts-editor. Agent refused CRM; audit `user_id=2` / `clientAgent=marts-editor`. [Agent refuses CRM while viewing as marts-editor](https://cursor.com/agents/bc-019fe687-99b1-76fd-80fa-dd213aecc497/artifacts?path=%2Fopt%2Fcursor%2Fartifacts%2Fagent_view_as_crm_blocked.webp) [agent_view_as_crm_policy_block.mp4](https://cursor.com/agents/bc-019fe687-99b1-76fd-80fa-dd213aecc497/artifacts?path=%2Fopt%2Fcursor%2Fartifacts%2Fagent_view_as_crm_policy_block.mp4) <sub>To show artifacts inline, <a href="https://cursor.com/dashboard/cloud-agents#my-pull-requests">enable</a> in settings.</sub> <!-- CURSOR_AGENT_PR_BODY_END --> <div><a href="https://cursor.com/agents/bc-019fe687-99b1-76fd-80fa-dd213aecc497?cursor_ref=pr_footer&cursor_cta=open_in_web"><picture><source media="(prefers-color-scheme: dark)" srcset="https://cursor.com/assets/images/open-in-web-dark.png"><source media="(prefers-color-scheme: light)" srcset="https://cursor.com/assets/images/open-in-web-light.png"><img alt="Open in Web" width="114" height="28" src="https://cursor.com/assets/images/open-in-web-dark.png"></picture></a> <a href="https://cursor.com/background-agent?bcId=bc-019fe687-99b1-76fd-80fa-dd213aecc497&cursor_ref=pr_footer&cursor_cta=open_in_cursor"><picture><source media="(prefers-color-scheme: dark)" srcset="https://cursor.com/assets/images/open-in-cursor-dark.png"><source media="(prefers-color-scheme: light)" srcset="https://cursor.com/assets/images/open-in-cursor-light.png"><img alt="Open in Cursor" width="131" height="28" src="https://cursor.com/assets/images/open-in-cursor-dark.png"></picture></a> </div> --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com>
![https://cursor.com/assets/images/open-in-web-dark.png"><source](https://cursor.com/assets/images/open-in-web-dark.png"><source){kind=link}
![https://cursor.com/assets/images/open-in-web-light.png"><img](https://cursor.com/assets/images/open-in-web-light.png"><img){kind=link}
![https://cursor.com/assets/images/open-in-web-dark.png"></picture></a> <a](https://cursor.com/assets/images/open-in-web-dark.png"></picture></a> <a){kind=link}
![https://cursor.com/assets/images/open-in-cursor-dark.png"><source](https://cursor.com/assets/images/open-in-cursor-dark.png"><source){kind=link}
![https://cursor.com/assets/images/open-in-cursor-light.png"><img](https://cursor.com/assets/images/open-in-cursor-light.png"><img){kind=link}
![https://cursor.com/assets/images/open-in-cursor-dark.png"></picture></a> </div](https://cursor.com/assets/images/open-in-cursor-dark.png"></picture></a> </div){kind=link}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
93 of 116
BrainControllerendpoints performed no authorization — onlyauthentication.
SecurityConfigasserts.anyRequest().authenticated()andJwtAuthenticationFilteronly resolves a principal; neither inspects aconnectionId, and no filter/interceptor/aspect fills the gap. Connections areprivate per user (
ConnectionAccessService.resolveAccesskeys onownerUsernameconnection 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
assertCanReadConnectionContent(GET),assertCanManageConnectionContent(writes).simulationId/experimentId/patternId) resolve theowning connection first via three new
getConnectionIdlookups.@PreAuthorize("hasRole('ADMIN')").BrainControllerAuthorizationSafetyTestfails the build if a new endpointships 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).
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.