golem15/summercms
4
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

docs(19.7-05): security review and contract, deferred items, folded todos closed

Browse Source 
- 19.7-SECURITY-REVIEW.md: T-19.7-01..41 and T-19.7-SC with file, line and
  pinning test; two production fixes (c49983d, 1111828); superuser reach
  accepted as designed with audit and ceiling
- 19.7-SECURITY.md: threats_open 0, accepted risks AR-19.7-01..05
- the frontend permission and account link todos move to todos/done with
  their closing notes
This commit is contained in:
Jakub Zych
2026-10-23 14:29:47 +02:00
parent 0b8e027b81
commit 4730e37742
5 changed files with 275 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files

123
.planning/phases/19.7-generic-mcp-module/19.7-SECURITY-REVIEW.md Normal file
View File

@@ -0,0 +1,123 @@
# Phase 19.7 Security Review: generic MCP module
**Date:** 2026-10-23
**Level:** OWASP ASVS L1 (RESEARCH "Security Domain")
**Scope:** the framework seams of plan 01 (bouncer dispatch marker, cabana `ResolveBackend` and the account and prompted-action seams, surf's compiled-handler lookup and stream route kind, wristband's scope allow-list), the `pier` module of plan 02, sm-user-plugin's account link, frontend permission catalog and MCP guard (plan 03), sm-oauthserver-plugin's MCP verifier and consent JSON, sm-journal-plugin's MCP tools and the demo wiring (plan 04), and plan 05's gate.
**Threats reviewed:** T-19.7-01..T-19.7-41 and T-19.7-SC from the threat models of plans 01-05.
**Result:** every threat is verified, fixed in a commit or accepted with a reason. The review found two production defects (a frontend dispatch marker honoured by cabana's `backend` middleware, and an ended session written back by a request still in flight) and fixed both in their own commits. The superuser verdict (D-22, T-19.7-34) is "accepted as designed with audit and ceiling"; no switch was added and no tag is needed for it.
Repositories and abbreviations used below: **F** = `summercms.go` (framework, paths under `modules/`), **U** = sm-user-plugin (`sm-demo-app/plugins/golem15/user`), **O** = `sm-oauthserver-plugin`, **J** = `sm-journal-plugin`, **D** = `sm-demo-app`, **V** = `sm-demo-app/vue-demo-app`.
## Method
1. For each threat, read the code that implements the mitigation and record the file and line (grep-level check at L1, plus a read of the surrounding logic).
2. For each threat, find the test that pins the mitigation (the plan 01-04 smokes and the plan 05 tests). Where the code was right but no test pinned it, add the test in the owning repository as its own commit.
3. Where the code was wrong, fix it in its own `fix(` commit in the owning repository, then add or extend the pinning test.
4. Cross-cutting sweeps (below): link proof, dispatch marker, session binding, scope ceilings, superuser reach, logs, `no-store`.
5. Run the phase gate `bash scripts/check-phase19.7.sh --all` on a Docker host: every stage PASS.
## Gaps fixed during the review
| Commit | Repo | Threats | What changed |
|--------|------|---------|--------------|
| c49983d | F | T-19.7-01, T-19.7-02 (new finding) | `fix(cabana)`: cabana's `backend` middleware (the admin API and every route behind `backend`) honoured a dispatch marker listing the `backend` guard even when its principal was a frontend one, so a frontend user id would have been read as an administrator's. `refuseFrontendMarker` now answers 401 for such a marker before the guard honours it, as `ResolveBackend` already did. Pinned by `TestDispatchMarker` (cabana, Postgres part, commit 3bbbe6c). No framework caller produces such a marker today (pier's backend provider builds backend principals only); this closes the seam for any future dispatcher or guard provider. |
| 1111828 | F | T-19.7-15, T-19.7-35, T-19.7-11 | `fix(pier)`: a request that finished after its session ended (DELETE, a `revoked` or `deleted` subject event, the idle expiry) persisted the session state with a plain Put and brought the record back; the next request rehydrated the ended session. The persist step now only rewrites an existing record (Redis `SET XX`, the memory store likewise) and closes the local copy when the record is gone. Found by `TestFanOut` (about one run in a hundred before the fix; 300 consecutive green runs after it). |
| 0b8e027 | F | T-19.7-17, T-19.7-18, T-19.7-19, T-19.7-37 | Test pins: `TestEndpointGuards` (foreign Origin 403 before the bearer is read, 413 over the body cap, a GET stream ends once its grant stops verifying), `TestDispatch` refuses an undeclared argument key, the gate requires both and runs the demo's deploy check. Code unchanged. |
| 3bbbe6c, f14eae9 | F | T-19.7-01..07, T-19.7-09..20, T-19.7-22 | The validation-map tests of Tasks 1-2 (`TestDispatchMarker` x2, `TestToolsList`, `TestCatalog`, `TestDispatch`, `TestGuardRules`, `TestSessions`, `TestFanOut`, `TestStreamingRoute`, `TestPierMount`, `TestMCPScopes`, `TestAccountContributions`, `TestRecordActionPrompt`, `TestScopeLimitForMethods`). |
| 72de822 | U | T-19.7-22..29 | `TestBackendLink`, `TestFrontendPermissionCatalog`, `TestUserMCPGuard`. Code unchanged. |
| 2929c91 | O | T-19.7-30..33, T-19.7-35 | `TestMCPTokenVerifier`, `TestMCPConsent`. Code unchanged. |
| a9a9b80 | J | T-19.7-36, T-19.7-17 | `TestJournalMCP`. Code unchanged. |
| 6fa2d56 | D | T-19.7-13, T-19.7-15, T-19.7-35, SC-4 | `TestMCPEndToEnd` revoke matrix (unlink list_changed, read grant, revoked grant ends the session, personal tokens off). Code unchanged. |
A third fix found by the tests is functional, not a security gap: c29d228 `fix(pier)` treats a `tools/call` without arguments (or with `null`) as an empty object (`TestDispatch`).
## Threat register
| Threat | Mitigation as built | Implementation | Test (repo) | Status |
|--------|---------------------|----------------|-------------|--------|
| T-19.7-01 | The marker lives under an unexported context key; `Dispatched` is the only reader and answers only for an exact guard name with a principal; no header, cookie or query is read for it; the only non-test caller of `WithDispatch` is pier's dispatcher (the gate's `marker` stage); cabana's `backend` middleware refuses a frontend marker. | F `bouncer/dispatch.go:5`, `:29`, `:40`; F `bouncer/registry.go:86`; F `pier/dispatch.go:159`; F `cabana/http.go:252` | `TestDispatchMarker` (bouncer: forged headers, cookies and query; nil and unlisted markers), `TestDispatchMarker` (cabana: over HTTP the marker never arrives), gate stage `marker` (F) | fixed in commit c49983d |
| T-19.7-02 | `ResolveBackend` honours a marker only for guard `backend` with `Principal.Backend`; a frontend marker is `ErrUnauthenticated`; a scoped principal is still checked against the cover codes; the `backend` middleware applies the same rule. | F `cabana/token_guard.go:209-216`; F `cabana/http.go:252` | `TestDispatchMarkerSmoke`, `TestDispatchMarker` (cabana, incl. the in-process admin API case) (F) | fixed in commit c49983d |
| T-19.7-03 | `Router.Handler` returns the fully compiled chain (constraints, named middleware, body limit, recovery, CORS profile, pattern), the same chain the mux serves. | F `surf/routetable.go:49`; F `surf/router.go:373` | `TestDispatchSmoke` (F), `TestDispatchMarker` (cabana, Journal-shaped route through `Handler`) (F) | verified |
| T-19.7-04 | `recoverStream` writes the opaque 500 only before the first header, byte or flush; afterwards the body is cut short and the panic value is never written. | F `surf/streaming.go:63-80` | `TestStreamingRouteSmoke`, `TestStreamingRoute` (F) | verified |
| T-19.7-05 | `mcp.enabled: true` without the build tag fails boot naming `summercms_mcp`. | F `surf/router.go:701` | `TestPierMountSmoke`, `TestPierMount` (untagged branch) (F) | verified |
| T-19.7-06 | Account contribution routes sit behind the `backend` guard and `interactiveOnly` (personal tokens refused), `requireAjax` on POST, `allowAction` for the action's permissions, `Applies` re-checked before `Run` (409), `Run` inside a transaction with only the signed-in admin's id. | F `cabana/http.go:319`; F `cabana/account_contributions.go:169-181`, `:189` | `TestAccountContributionsSmoke`, `TestAccountContributions` (incl. the Postgres transaction case) (F) | verified |
| T-19.7-07 | `payload` is accepted only for an action with a Prompt, only as a JSON string, trimmed, non-empty and capped in runes; every other record action still refuses a payload; a plain action runs with an empty Value. | F `cabana/actions.go:217`, `:256` | `TestRecordActionPromptSmoke`, `TestRecordActionPrompt`, `TestRecordActionPromptRun` (F) | verified |
| T-19.7-08 | Contributed labels, statuses, messages and URLs render as text and attributes (no `v-html`); a URL opens with `noopener` and the visible link carries `rel="noopener noreferrer"`. | F `admin/src/components/account/AccountContributions.vue` | `admin/tests/account/AccountContributions.test.ts` ("Text, never HTML", URL attributes) (F) | verified |
| T-19.7-09 | wristband's authorize allow-list is exactly `Options.ScopesSupported`; the default server refuses `backend`; `openid` and unlisted scopes stay refused on the MCP flavour. | F `wristband/authorize.go:31`, `:231` | `TestMCPScopesSmoke`, `TestMCPScopes` (F) | verified |
| T-19.7-10 | Every request re-verifies its bearer (`auth.RequireBearerToken` with pier's resolver); a session never replaces it; a failure is 401 with the RFC 9728 challenge and `scope="read"`. | F `pier/pier.go:235-260`; F `pier/identity.go:144` | `TestPierTracerSmoke` (bad bearer), `TestEndpointGuards` (stream ends after revocation), `TestMCPEndToEnd` "revoked token" (F, D) | verified |
| T-19.7-11 | The session record stores only the binding (`oauth:<client>:<user>` or the personal token id), compared on every POST, GET and DELETE (403 `session_binding`); ids from `rand.Text()`; sliding idle TTL; per-binding cap. | F `pier/session.go:143`, `:211`, `:242`, `:139` | `TestPierSessionsSmoke`, `TestSessions` (Redis and memory) (F) | verified |
| T-19.7-12 | The inner request is built fresh (`httptest.NewRequestWithContext`) with no Authorization or Cookie; identity travels only in the marker; client address, X-Forwarded-For, X-Real-IP and Accept-Language are copied. | F `pier/dispatch.go:159-180` | `TestPierTracerSmoke` (echo route `auth: false`), `TestDispatch` (outer Cookie and Authorization absent) (F) | verified |
| T-19.7-13 | A backend subject exists only with `backend` in the grant AND a link the resolver confirms per request (5 s cache), or an admin `g15_` token behind `mcp.allow_personal_tokens`; the read/write ceiling applies on top; list and call use the same `allowed`. | F `pier/identity.go:152`, `:187-198`; F `pier/filter.go:16`, `:37` | `TestToolsList` (no link, no backend tool; union with a link), `TestPierTracerSmoke` "personal tokens behind the flag", `TestMCPEndToEnd` (no backend tool without the scope) (F, D) | verified |
| T-19.7-14 | One SDK server per identity; the receiving middleware re-filters `tools/list` and refuses `tools/call` and `prompts/get` outside the current request's list (JSON-RPC error, the route is never reached); stateless servers are keyed by binding. | F `pier/session.go:493`; F `pier/stateless.go:80` | `TestToolsList` (per principal; unlisted call never reaches the route), `TestPierStatelessSmoke` (F) | verified |
| T-19.7-15 | Rehydration builds the server from the current bearer's identity, never from stored state; `revoked`/`deleted` events delete the record and close local copies on every instance; a request finishing after the end no longer writes the record back; link events drop the link cache and recompute. | F `pier/session.go:230-280`, `:320-360`; F `pier/events.go:42-60`; F `pier/store_redis.go` (`Update`) | `TestFanOut` (revocation, deletion, link change, cache bypass), `TestSessions`, `TestMCPEndToEnd` "revoked grant ends the session" (F, D) | fixed in commit 1111828 |
| T-19.7-16 | Only `oas_at_` tokens reach the verifier; the verifier accepts only rows of a kind-`mcp` client; PRM `resource` is `mcp.resource` and a boot warning names a mismatch with the verifier's resource. | F `pier/identity.go:149`; F `pier/prm.go:23-31`; O `classes/mcp_verifier.go:79-88` | `TestMCPTokenVerifierSmoke` (OIDC client's token refused), `TestMCPEndToEnd` "protected resource metadata" (O, D) | verified |
| T-19.7-17 | Arguments are validated against the reflected schema (additionalProperties false, required fields); a single-segment value may not contain `/`; path and query keys are removed from the forwarded body; the route's own constraints and body limit still run. | F `pier/dispatch.go:85`, `:102`; F `pier/catalog.go:209`, `:340` (`inputFields`) | `TestDispatch` (undeclared key refused, `a/b` refused, omitted fields absent), `TestJournalMCP` (input keys are the route's keys) (F, J) | fixed in commit 0b8e027 (test pin; code unchanged) |
| T-19.7-18 | Inline `mcp.throttle` on the endpoint routes, `max_per_binding` (429), idle TTL, 4 MiB body cap, `mcp.call_timeout`, `mcp.max_result_bytes`, stream re-verification every minute. | F `pier/pier.go:122`, `:225`; F `pier/session.go:139`, `:369`; F `pier/dispatch.go:145`, `:186` | `TestPierMount` (tagged: `throttle:120,1`), `TestStreamingRoute` (throttle on a stream route), `TestSessions` (429), `TestDispatch` (timeout, size cap), `TestEndpointGuards` (413, stream re-verification) (F) | fixed in commit 0b8e027 (test pin; code unchanged) |
| T-19.7-19 | `originCheck` answers 403 for a browser Origin outside `mcp.allowed_origins` (default: the app's own origin) before the bearer or the body is read; no Origin passes. | F `pier/pier.go:201-220` | `TestEndpointGuards` "origin check" (F) | fixed in commit 0b8e027 (test pin; code unchanged) |
| T-19.7-20 | The audit line carries tool, guard, user/admin/client ids, token kind, an 8-character session prefix, status and duration; never a token, argument or body. | F `pier/audit.go:40-66` | `TestPierTracerSmoke` "audit line" (F) | verified |
| T-19.7-21 | Descriptions are plugin-authored static text, capped at 1024 runes at boot. | F `pier/catalog.go:21` | `TestCatalog` (boot refusals) (F) | accepted (see Accepted risks) |
| T-19.7-SC | The user approved go-sdk v1.8.0 and jsonschema-go v0.4.3 before `go get`; versions pinned in go.mod; the decision note lists the transitive graph and govulncheck; the SDK links only into tagged binaries. | F `.planning/notes/2026-10-22-mcp-go-sdk.md`; F `.planning/research/STACK.md:56-57`; F `go.mod` | gate stages `docs` (note and STACK rows) and `go` (the untagged hello binary links no SDK) (F) | verified |
| T-19.7-22 | The self-service path writes a link only inside `ConfirmLinkRequest`: a one-shot hashed code bound to the admin, row lock, `jwt.auth` principal equal to the token's subject, `iat` not before the request's `created_at` (in seconds); no email match and no user list. | U `classes/backend_links.go:213-262` (`:227` lock, `:239` iat); U `controllers/backend_link_controller.go:60-106` | `TestBackendLinkSmoke`, `TestBackendLink` ("fresh sign-in at the second") (U) | verified |
| T-19.7-23 | 64-hex code stored as SHA-256, `used_at` set in the linking transaction, expired, used and unknown codes all 404; the show route answers only the admin's login and name and the expiry. | U `classes/backend_links.go:105`, `:129`; U `controllers/backend_link_controller.go:47` | `TestBackendLinkSmoke` (replay, expiry), `TestBackendLink` (show route keys) (U) | verified |
| T-19.7-24 | Admin-set links need `golem15.users.manage_backend_links` (no default role), match an exact email, record `linked_by`, fire `BackendLinkedEvent` and both subject events after commit; cabana's outranking check runs first; 1:1 refuses a second link. | U `backend_links_admin.go:33`, `:50`, `:99`; U `classes/backend_links.go:269-325` | `TestBackendLinkAdminSmoke`, `TestBackendLink` ("SetLink and RemoveLink events", none after a rollback) (U) | verified |
| T-19.7-25 | Confirm is a POST behind `jwt.auth` with the bearer re-read for `iat`; My account link and unlink run behind the backend guard, `interactiveOnly` and `requireAjax`; no state change on GET. | U `controllers/backend_link_controller.go:60-74`; F `cabana/http.go:319` | `TestBackendLinkSmoke` (anonymous confirm 401), `TestAccountContributions` (personal token refused) (U, F) | verified |
| T-19.7-26 | `LinkedBackendUser` is one indexed query per call (no plugin cache) excluding deleted or deactivated admins and deleted users; user deletion and deactivation remove the link in their transaction and fire events after commit. | U `classes/backend_links.go:382-412`, `:455-490` | `TestBackendLink` ("deactivated or deleted administrator", "deactivating the frontend user"), `TestBackendLinkSmoke` (delete) (U) | verified |
| T-19.7-27 | `MCPAllows` evaluates the route's scopes and every permission code (route's and tool's) with the same group grants `user.permission` checks; an unknown `user.*` middleware fails boot. | U `classes/mcp_guard.go:58-117` | `TestUserMCPGuardSmoke`, `TestUserMCPGuard` (parser refusals, deleted user, dispatch credential against `user.scope:read`) (U) | verified |
| T-19.7-28 | Personal tokens resolve only for the plugin's prefix, by hash, `IsUsableAt` (revoked and expired refused) and a loadable user; the subject carries the token's own scopes; the dispatch credential is in memory (ID 0); personal tokens at `/mcp` need `mcp.allow_personal_tokens` (off by default and in the demo). | U `classes/mcp_guard.go:143-172`; F `pier/identity.go:152`; D `config/mcp.yaml:19` | `TestUserMCPGuard` (expired token, deleted user, ID 0 credential), `TestMCPEndToEnd` "personal tokens are off" (U, D) | verified |
| T-19.7-29 | `linked_by` on admin-set links, `created_at` on links and requests, link and unlink events, cabana's record action audit. | U `classes/backend_links.go:293-322` | `TestBackendLinkAdminSmoke`, `TestBackendLink` (`linked_by`) (U) | verified |
| T-19.7-30 | The verifier gate: `oas_at_` prefix, hash lookup, not revoked, not expired, client of kind `mcp`, active and not revoked, user loadable. | O `classes/mcp_verifier.go:55-110` | `TestMCPTokenVerifierSmoke`, `TestMCPTokenVerifier` (deactivated client, user deactivated after minting, conditional stamp) (O) | verified |
| T-19.7-31 | The consent JSON shows the client name (capped), the redirect host and the linked admin only when `backend` was requested and a link exists; `backend` is granted only with a link at consent. | O `controllers/mcp_consent_api.go:54-92`, `:134-142` | `TestMCPConsentSmoke` ("no link", "linked") (O) | verified |
| T-19.7-32 | `PendingRequest`, `IssueCode` and `DenyPending` resolve the request for the acting JWT principal only; a foreign or replayed request is 404; answers are `no-store, private`. | F `wristband/consent.go:47`, `:74`, `:106`, `:130`; O `controllers/consent_api.go:123-133` | `TestMCPConsentSmoke` (foreign request), `TestMCPConsent` (replayed decision, no-store) (O) | verified |
| T-19.7-33 | granted = submitted ∩ requested, minus `backend` without a link; an empty grant is 422 `no_grantable_scopes`; a non-JSON body is 400. | O `controllers/mcp_consent_api.go:105-165` | `TestMCPConsentSmoke`, `TestMCPConsent` (extra scopes dropped, `scopes: []`, form body) (O) | verified |
| T-19.7-34 | The backend principal is the admin's real principal, superuser included (D-22). Reach is bounded by: the explicit `backend` scope on the consent page with the admin's name, the fresh-sign-in link proof, the read/write ceiling, the tools plugins declare (no generic admin API), the per-call audit line with admin_id and client_id, and per-request re-checks. | F `pier/backend_guard.go:59-100`; F `pier/audit.go:40` | `TestToolsList` (ceiling, union), `TestScopeLimitForMethods` (an admin token's scope per method), `TestMCPEndToEnd` (read grant hides write tools) (F, D) | accepted (see Superuser reach) |
| T-19.7-35 | Revocation from the authorized-apps list and the user cascade fire `revoked`/`deleted` events that end sessions everywhere; the verifier refuses a revoked token on the next request; an ended session is no longer written back. MCP grants have no consent row, so the authorized-apps list cannot revoke them yet (a revoked client or deleted user still ends every grant). | O `controllers/authorizations.go:101-122`; O `classes/cascade.go:68`; F `pier/events.go:42`; F `pier/session.go:320` | `TestMCPConsentSmoke` ("revocation fires revoked", "cascade fires deleted"), `TestMCPEndToEnd` ("revoked token", "revoked grant ends the session"), `TestFanOut` (O, D, F) | fixed in commit 1111828; the missing self-service revoke is accepted (see Accepted risks) |
| T-19.7-36 | Journal's update input fields are pointers or `omitempty`; pier forwards the agent's raw JSON minus path keys, so omitted fields stay omitted. | J `mcp.go:52` (`mcpUpdatePostInput`); F `pier/dispatch.go:91-123` (path and query keys removed from the forwarded body) | `TestDispatch` ("argument split": omitted `note` absent), `TestJournalMCP` (input keys) (F, J) | verified |
| T-19.7-37 | pier's inline throttle, idle TTL and per-binding cap; nginx opens exactly `= /mcp` (unbuffered, 1 h read timeout), the two MCP metadata paths and `/oauth/mcp/`; `check-deploy.sh` refuses anything else. | D `deploy/nginx/demo.summercms.io.conf:213-224`; D `scripts/check-deploy.sh` | gate stage `consumers` runs `check-deploy.sh`; `TestPierMount` (tagged throttle) (D, F) | fixed in commit 0b8e027 (gate pin; config unchanged) |
| T-19.7-38 | The consent page navigates only to the `redirect_uri` the server answers, which wristband builds from the client's registered URI; the login hop keeps a same-origin `next`. | V `app/pages/connect.vue:48`, `:57`, `:70-75`; F `wristband/consent.go:74-128` | `TestMCPConsentSmoke` ("deny": the redirect carries the registered URI's state) (O) | verified |
| T-19.7-39 | Every required test is listed by name; the detectors refuse failed, skipped, zero-match, "no tests to run" and missing names; the self-test proves them on planted output; docker is NOT RUN only without a daemon and fails with `PHASE197_REQUIRE_DOCKER=1`. | F `scripts/check-phase19.7.sh` (`detect_go`, `detect_vitest`, `run_self_test`, `docker_gate`) | `bash scripts/check-phase19.7.sh --self-test`; the go stage refused `TestFireSubjectChangedWaitsForCommit` when it skipped under `-short` during this plan (F) | verified |
| T-19.7-40 | The `marker` stage fails on any non-test caller of `WithDispatch` outside `modules/pier` and `modules/bouncer` and on a `modules/pact` dependency on the SDK, jsonschema-go or bouncer. | F `scripts/check-phase19.7.sh` (`run_marker`, `marker_callers`) | `--self-test` (planted caller found, pier and test files ignored), `--marker` (F) | verified |
| T-19.7-41 | Tests mint their own tokens and codes on throwaway Postgres containers and miniredis; no fixture holds a production credential; the docs stage keeps consuming applications out of the phase's framework tests. | F `modules/pier/helpers_test.go` (fake grants); U, O, D test harnesses (testcontainers) | gate stage `docs` (names) (F) | verified |
## Cross-cutting findings
1. **Link proof.** The only writers of `golem15_user_backend_links` are `insertLink` (called by `ConfirmLinkRequest` and `SetLink`) and the deletes in `RemoveLink` (U `classes/backend_links.go:293`, `:325`); `SetLink` has one non-test caller, the permission-guarded Administrators record action (U `backend_links_admin.go:99`, behind `golem15.users.manage_backend_links` and cabana's outranking check). The confirm compares the token's `iat` with the request's `created_at` in whole seconds (U `classes/backend_links.go:239`), and the route requires the token's subject to equal the `jwt.auth` principal.
2. **Dispatch marker.** The only non-test caller of `bouncer.WithDispatch` is F `pier/dispatch.go:159` (gate stage `marker`). Nothing reads a header, cookie or query string for it (`TestDispatchMarker` sends all three). `ResolveBackend` and, since c49983d, the `backend` middleware refuse a frontend marker. The inner request carries no Authorization or Cookie (`TestDispatch`).
3. **Session binding.** The binding is compared on every stateful POST, GET and DELETE (F `pier/session.go:211`, `:242`); DELETE and revocation remove the record through the store and close every instance's copy (`TestSessions`, `TestFanOut`); rehydration takes identity from the current bearer (the stored record holds only binding, protocol state and creation time). Since 1111828 an ended session cannot be written back by a late request.
4. **Scope ceilings.** `read` hides tools whose annotations are not read-only (F `pier/filter.go:37`, `TestToolsList`, `TestMCPEndToEnd` "read grant"); `backend` without a link yields no backend subject (F `pier/identity.go:187`, `TestToolsList`); consent drops `backend` without a link (O `controllers/mcp_consent_api.go:134`, `TestMCPConsentSmoke`); personal tokens carry their own scopes and are off by default (F `pier/config.go`, `TestPierTracerSmoke`, `TestMCPEndToEnd`); an admin token's scope is filtered per dispatched method (`TestScopeLimitForMethods`, `TestGuardRules`).
5. **Superuser reach (P-23, D-22).** See the section below.
6. **Logs.** pier logs ids, names, statuses, durations, an 8-character session prefix and the binding key (`oauth:<client>:<user>`, which holds no secret); store errors carry cooler's fixed texts, not the Redis URL (F `cooler/redis.go:28-32`). The verifier's only log line names the token id (O `classes/mcp_verifier.go:119`). The link code is never logged. No token, argument or body appears in any line (`TestPierTracerSmoke` "audit line").
7. **`no-store`.** Every consent answer (show, decide, refusals) carries `Cache-Control: no-store, private` (`TestMCPConsent`). The link show and confirm answers carry `no-cache, private` (U `controllers/api_controller.go:1266`) and hold neither the code nor a token. The one answer that carries a link code, My account's "link" action, is a POST response of the admin API, which shared caches do not store without explicit freshness; accepted at L1.
## Superuser reach
**Verdict: accepted as designed with audit and ceiling. `mcp.refuse_superuser_backend` is not added.**
Reasons:
- Scope of reach. An agent acting as a linked superuser reaches only the tools plugins declare (`pact.HasMCP`), each bound to one route; there is no generic admin API tool. The reach is that of the declared backend tools (today the seven Journal tools), not of the admin panel.
- Consent. The backend grant requires the frontend user to request it, see "also act as <admin name>" on the consent page, and hold a link made by a fresh-sign-in proof or by an administrator with `manage_backend_links`.
- Ceiling. A `read` grant hides every tool that is not read-only; scopes are per grant and the verifier refuses a revoked grant on the next request.
- Audit. Every call is logged with tool, admin_id, client_id and status.
- A switch would refuse a legitimate use (a solo site owner who is the superuser and the only author) while a cautious site already has the alternative: link the agent's frontend account to a non-superuser administrator with exactly the permissions the agent needs.
Revisit when a plugin declares a superuser-only or destructive backend tool beyond content editing; the switch is then a `fix(pier)` plus README and docs and a framework patch tag (recorded in deferred-items.md as a conditional follow-up).
## Accepted risks
| Risk | Threat | Reason |
|------|--------|--------|
| Tool and prompt descriptions are passed to agents unchanged | T-19.7-21 | They are static text the plugin author writes (capped at 1024 runes); dynamic text in a description is a plugin bug. The docs page tells authors to keep them factual. |
| Superuser reach through an agent | T-19.7-34 | See "Superuser reach". |
| No self-service revoke of an MCP grant | T-19.7-35 | MCP grants write no consent row, so the authorized-apps list neither lists nor revokes them (deferred-items.md). Access still ends: an administrator can revoke the client, deleting or deactivating the user ends every grant, the access token expires, and a revoked token is refused at once. A consent row per MCP grant is the follow-up. |
| A POST response carrying a link code has no explicit `no-store` | cross-cutting 7 | POST responses are not stored by shared caches without explicit freshness; the code is one-shot, expires in 10 minutes and needs the frontend user's fresh sign-in. |
| Stateful sessions run with the SDK's 2025-03-26 stream defaults; an `initialized` notification may land on another instance | T-19.7-11, T-19.7-15 | deferred-items.md (open). Both are availability details of multi-instance deployments; identity, binding and revocation checks are unaffected. The demo runs one instance. |
## Residual items
- Framework fixes c49983d, c29d228 and 1111828 are on framework master only: applications pinning summercms v0.4.4 (the demo release build, oauthserver v0.1.2, Journal v0.2.3) do not carry them until the next framework tag (v0.5.0 at phase completion). Recorded in deferred-items.md.
- The plugin test commits (U 72de822, O 2929c91, J a9a9b80) and the demo test commit (D 6fa2d56) are local and untagged; they change no shipped code.
- The real MCP client check against the demo (plan 04) is still a manual item.
## Suites run for this review
`bash scripts/check-phase19.7.sh --all` on a Docker host: self-test, go (framework `go vet` untagged and tagged, `go test -short ./...`, examples/hello, the tagged build, the required names untagged and tagged), plugins (vet, short tests and additive checks of the user, OAuth server and blog plugins), consumers (fonoteka.go, sm-bm-app, sm-demo-app untagged and tagged, the pin check, the deploy check), admin (typecheck, vitest, OpenAPI, dist), docs, marker, docker (cabana, user plugin, OAuth server and demo Postgres suites) all PASS.

110
.planning/phases/19.7-generic-mcp-module/19.7-SECURITY.md Normal file
View File

@@ -0,0 +1,110 @@
---
phase: "19.7"
slug: "generic-mcp-module"
status: verified
# threats_open = count of OPEN threats at or above workflow.security_block_on severity (the blocking gate)
threats_open: 0
asvs_level: 1
created: "2026-10-23"
---
# Phase 19.7 — Security
> Per-phase security contract: threat register, accepted risks, and audit trail. The evidence (file and line, pinning test) for every threat is in `19.7-SECURITY-REVIEW.md`.
---
## Trust Boundaries
| Boundary | Description | Data Crossing |
|----------|-------------|---------------|
| MCP client to `/mcp` | any agent on the internet presents a bearer, a session id and JSON-RPC bodies | OAuth access token or personal token, session id, tool arguments (high) |
| pier to plugin routes (in-process dispatch) | pier calls a plugin's compiled route chain as the caller's pre-resolved principal | dispatch marker with a principal, arguments as path, query and body (high) |
| frontend user to the link confirm and consent pages | a signed-in browser confirms a link code or decides a consent | session JWT, link code, consent decision (high) |
| administrator to My account and the Administrators screen | admin actions that start, set or remove a link | admin JWT, prompted email value (high) |
| verifier and resolvers to pier | the OAuth server plugin and the user plugin answer who a bearer is and which admin a user is linked to | grants, subjects, link answers (high) |
| instances to the shared session store | Redis holds session records and carries subject events between instances | binding keys, protocol state, events (medium) |
| nginx to the Go binary | the public vhost opens the MCP paths only | streamed MCP traffic (medium) |
| test results to the phase verdict | the gate's output decides whether the phase is verified | test output (medium) |
---
## Threat Register
| Threat ID | Category | Component | Severity | Disposition | Mitigation | Status |
|-----------|----------|-----------|----------|-------------|------------|--------|
| T-19.7-01 | Spoofing | bouncer dispatch marker | high | mitigate | 19.7-SECURITY-REVIEW.md: fixed in commit c49983d | closed |
| T-19.7-02 | Elevation of privilege | cabana ResolveBackend and backend middleware with a marker | high | mitigate | 19.7-SECURITY-REVIEW.md: fixed in commit c49983d | closed |
| T-19.7-03 | Elevation of privilege | surf Handler lookup | medium | mitigate | 19.7-SECURITY-REVIEW.md: verified | closed |
| T-19.7-04 | Tampering | streaming route recovery | medium | mitigate | 19.7-SECURITY-REVIEW.md: verified | closed |
| T-19.7-05 | Denial of service | mcp.enabled without the tag | low | mitigate | 19.7-SECURITY-REVIEW.md: verified | closed |
| T-19.7-06 | Elevation of privilege | account contribution actions | high | mitigate | 19.7-SECURITY-REVIEW.md: verified | closed |
| T-19.7-07 | Tampering | prompted record action value | medium | mitigate | 19.7-SECURITY-REVIEW.md: verified | closed |
| T-19.7-08 | Information disclosure | account contribution rendering in the SPA | low | mitigate | 19.7-SECURITY-REVIEW.md: verified | closed |
| T-19.7-09 | Elevation of privilege | wristband MCP scope allow-list | medium | mitigate | 19.7-SECURITY-REVIEW.md: verified | closed |
| T-19.7-10 | Spoofing | bearer verification | high | mitigate | 19.7-SECURITY-REVIEW.md: verified | closed |
| T-19.7-11 | Spoofing | session hijack with a stolen Mcp-Session-Id | high | mitigate | 19.7-SECURITY-REVIEW.md: verified | closed |
| T-19.7-12 | Elevation of privilege | token passthrough / confused deputy | high | mitigate | 19.7-SECURITY-REVIEW.md: verified | closed |
| T-19.7-13 | Elevation of privilege | scope escalation to backend tools | high | mitigate | 19.7-SECURITY-REVIEW.md: verified | closed |
| T-19.7-14 | Information disclosure | tool-list leakage across principals | high | mitigate | 19.7-SECURITY-REVIEW.md: verified | closed |
| T-19.7-15 | Elevation of privilege | rehydration of a revoked session | high | mitigate | 19.7-SECURITY-REVIEW.md: fixed in commit 1111828 | closed |
| T-19.7-16 | Spoofing | audience confusion at /mcp | medium | mitigate | 19.7-SECURITY-REVIEW.md: verified | closed |
| T-19.7-17 | Tampering | argument injection into routes | medium | mitigate | 19.7-SECURITY-REVIEW.md: fixed in commit 0b8e027 (test pin; code unchanged) | closed |
| T-19.7-18 | Denial of service | sessions, streams and large results | medium | mitigate | 19.7-SECURITY-REVIEW.md: fixed in commit 0b8e027 (test pin; code unchanged) | closed |
| T-19.7-19 | Spoofing | DNS rebinding / browser Origin | medium | mitigate | 19.7-SECURITY-REVIEW.md: fixed in commit 0b8e027 (test pin; code unchanged) | closed |
| T-19.7-20 | Information disclosure | audit log contents | medium | mitigate | 19.7-SECURITY-REVIEW.md: verified | closed |
| T-19.7-21 | Tampering | prompt injection through tool metadata | low | accept | 19.7-SECURITY-REVIEW.md: accepted (static plugin text, capped) | closed |
| T-19.7-SC | Tampering | go get of go-sdk and jsonschema-go | high | mitigate | 19.7-SECURITY-REVIEW.md: verified | closed |
| T-19.7-22 | Elevation of privilege | link without proof | high | mitigate | 19.7-SECURITY-REVIEW.md: verified | closed |
| T-19.7-23 | Spoofing | replayed or guessed link code | medium | mitigate | 19.7-SECURITY-REVIEW.md: verified | closed |
| T-19.7-24 | Elevation of privilege | admin sets a link | high | mitigate | 19.7-SECURITY-REVIEW.md: verified | closed |
| T-19.7-25 | Elevation of privilege | cross-site confirm or unlink | medium | mitigate | 19.7-SECURITY-REVIEW.md: verified | closed |
| T-19.7-26 | Information disclosure | stale link resolution | medium | mitigate | 19.7-SECURITY-REVIEW.md: verified | closed |
| T-19.7-27 | Elevation of privilege | list and call disagree on frontend permissions | medium | mitigate | 19.7-SECURITY-REVIEW.md: verified | closed |
| T-19.7-28 | Spoofing | personal token resolution at /mcp | medium | mitigate | 19.7-SECURITY-REVIEW.md: verified | closed |
| T-19.7-29 | Repudiation | who linked whom | low | mitigate | 19.7-SECURITY-REVIEW.md: verified | closed |
| T-19.7-30 | Spoofing | audience confusion at the verifier | high | mitigate | 19.7-SECURITY-REVIEW.md: verified | closed |
| T-19.7-31 | Elevation of privilege | consent phishing for the backend scope | high | mitigate | 19.7-SECURITY-REVIEW.md: verified | closed |
| T-19.7-32 | Elevation of privilege | consent decided for another user's request | medium | mitigate | 19.7-SECURITY-REVIEW.md: verified | closed |
| T-19.7-33 | Elevation of privilege | submitted scopes beyond the request | medium | mitigate | 19.7-SECURITY-REVIEW.md: verified | closed |
| T-19.7-34 | Spoofing | superuser reach through an agent (D-22) | high | mitigate | 19.7-SECURITY-REVIEW.md: accepted as designed with audit and ceiling (no switch) | closed |
| T-19.7-35 | Information disclosure | revoked grant keeps a live session | medium | mitigate | 19.7-SECURITY-REVIEW.md: fixed in commit 1111828; missing self-service revoke accepted | closed |
| T-19.7-36 | Tampering | partial update tools zeroing fields | medium | mitigate | 19.7-SECURITY-REVIEW.md: verified | closed |
| T-19.7-37 | Denial of service | public /mcp and SSE through nginx | medium | mitigate | 19.7-SECURITY-REVIEW.md: fixed in commit 0b8e027 (gate pin; config unchanged) | closed |
| T-19.7-38 | Spoofing | open redirect from the consent page | low | mitigate | 19.7-SECURITY-REVIEW.md: verified | closed |
| T-19.7-39 | Repudiation | a green gate that measured nothing | high | mitigate | 19.7-SECURITY-REVIEW.md: verified | closed |
| T-19.7-40 | Tampering | the marker rule eroding over time | medium | mitigate | 19.7-SECURITY-REVIEW.md: verified | closed |
| T-19.7-41 | Information disclosure | test fixtures leaking credentials | low | mitigate | 19.7-SECURITY-REVIEW.md: verified | closed |
*Status: open · closed. Every high threat is verified or fixed; the accepted items are listed below with their reasons.*
---
## Accepted Risks Log
| Risk ID | Threat Ref | Rationale | Accepted By | Date |
|---------|------------|-----------|-------------|------|
| AR-19.7-01 | T-19.7-21 | Tool and prompt descriptions are static plugin text, capped at 1024 runes at boot; dynamic text is a plugin bug. | plan 19.7-05 security review | 2026-10-23 |
| AR-19.7-02 | T-19.7-34 | A linked superuser's agent reaches only the declared backend tools, behind an explicit `backend` consent with the admin's name, the read/write ceiling and a per-call audit line; a refusal switch would block the solo-owner case while a non-superuser link already gives the narrower alternative. Revisit when a plugin declares a superuser-only or destructive backend tool beyond content editing. | plan 19.7-05 security review | 2026-10-23 |
| AR-19.7-03 | T-19.7-35 | MCP grants have no consent row, so the authorized-apps list does not revoke them yet; access still ends through client revocation, user deletion or deactivation, token expiry and the verifier's refusal of a revoked token. Follow-up in deferred-items.md. | plan 19.7-05 security review | 2026-10-23 |
| AR-19.7-04 | cross-cutting `no-store` | The POST answer of My account's link action carries a one-shot, 10-minute link code without explicit `no-store`; POST responses are not stored by shared caches without explicit freshness. | plan 19.7-05 security review | 2026-10-23 |
| AR-19.7-05 | T-19.7-11, T-19.7-15 | Stateful sessions use the SDK's 2025-03-26 stream defaults, and an `initialized` notification may land on another instance (deferred-items.md); multi-instance availability details that leave identity, binding and revocation checks unchanged. | plan 19.7-05 security review | 2026-10-23 |
---
## Security Audit Trail
| Audit Date | Threats Total | Closed | Open | Run By |
|------------|---------------|--------|------|--------|
| 2026-10-23 | 42 | 42 | 0 | plan 19.7-05 executor (Task 4), gate `bash scripts/check-phase19.7.sh --all` green |
---
## Sign-Off
- [x] All threats have a disposition (mitigate / accept / transfer)
- [x] Accepted risks documented in Accepted Risks Log
- [x] No open threat (frontmatter count confirmed)
- [x] `status: verified` set in frontmatter
**Approval:** verified 2026-10-23

34
.planning/phases/19.7-generic-mcp-module/deferred-items.md
View File

@@ -85,6 +85,11 @@ was changed by the plan that recorded it.
agent" for MCP yet. A consent row per MCP grant (written at `IssueCode`)
would make the existing list and revocation (which already fires the
`revoked` event) cover MCP clients; plan 05's revoke matrix should decide.
**Plan 05 decision:** accepted for this phase (19.7-SECURITY.md AR-19.7-03):
access still ends through client revocation, user deletion or
deactivation, token expiry and the verifier's refusal of a revoked token,
and `TestMCPEndToEnd` pins that the `revoked` event ends the session. The
consent row per MCP grant stays the follow-up.
- Pre-existing `gofmt` drift in sm-journal-plugin (found during 19.7-04 Task 3)
status: open
**What:** `gofmt -l` lists `classes/blog_editor_test.go` before this plan
@@ -132,3 +137,32 @@ was changed by the plan that recorded it.
retracted (quadratic time on crafted CDF files). It arrives indirectly
and predates this plan; a `go get github.com/gabriel-vasile/mimetype@latest`
in the framework (and a tag) would lift it for every consumer.
- Framework fixes of plan 05 are on master only (found during 19.7-05 Tasks 1-2)
status: open
**What:** c49983d `fix(cabana)` (the `backend` middleware refuses a
dispatch marker with a frontend principal), c29d228 `fix(pier)` (a
`tools/call` without arguments) and 1111828 `fix(pier)` (an ended session
is not written back by a late request) are not in any tag. Applications
pinning summercms v0.4.4 (the demo's release build, oauthserver v0.1.2,
Journal v0.2.3) lack them until they require the next framework tag
(v0.5.0 at phase completion, after the user's go-ahead).
- Plan 05 test commits are local and untagged (found during 19.7-05 Task 3)
status: open
**What:** sm-user-plugin 72de822 (in the sm-demo-app submodule checkout
`plugins/golem15/user`), sm-oauthserver-plugin 2929c91, sm-journal-plugin
a9a9b80 and sm-demo-app 6fa2d56 add tests only; none is pushed or tagged.
sm-demo-app's `plugins/golem15/user` working tree is ahead of its v0.1.10
pin by that test-only commit and the pointer is not committed;
`scripts/check-pins.sh` reports the mismatch, which the phase gate accepts
only because the commits touch `*_test.go` files alone. A patch tag
(sm-user-plugin v0.1.11, sm-oauthserver-plugin v0.1.3, sm-journal-plugin
v0.2.4) is optional; it would change no shipped code. Push and tags wait
for the user.
- Superuser refusal switch, conditional (found during 19.7-05 Task 4)
status: open
**What:** the security review accepted superuser reach through an agent
(19.7-SECURITY.md AR-19.7-02) and did not add
`mcp.refuse_superuser_backend`. When a plugin declares a superuser-only or
destructive backend tool beyond content editing, add the switch (a
`fix(pier)` with README and docs) and cut a framework patch tag with the
user's go-ahead.

4
.planning/todos/pending/2026-10-17-frontend-backend-account-link.md → .planning/todos/done/2026-10-17-frontend-backend-account-link.md
View File

@@ -14,3 +14,7 @@ Let one person hold a frontend user (`users`) and a backend user (`backend_users
- Additive tables and routes only; no change to existing user or cabana payloads.
Prerequisite of Phase 19.7 (generic MCP module). Design: `.planning/notes/2026-10-17-generic-mcp-module.md`.
## Closed 2026-10-23
Shipped in sm-user-plugin v0.1.10 (plan 19.7-03, D-03 to D-08): the tables `golem15_user_backend_links` (1:1 on both sides) and `golem15_user_backend_link_requests`, the proof round-trip (My account "Link frontend account" starts a one-shot hashed code, the site's `/link-admin` page confirms it after a fresh sign-in), `classes.BackendLinks` published as `pact.MCPLinkedBackendResolver`, and the Administrators-screen record actions `link_frontend_account` and `unlink_frontend_account` behind `golem15.users.manage_backend_links`. No email matching on the self-service path; unlinking, deleting or deactivating the user drops the backend subject from MCP grants. Pinned by `TestBackendLinkSmoke`, `TestBackendLinkAdminSmoke` and `TestBackendLink` (plan 19.7-05). g15office task `tqyvnzxx`.

4
.planning/todos/pending/2026-10-17-plugin-registered-frontend-permissions.md → .planning/todos/done/2026-10-17-plugin-registered-frontend-permissions.md
View File

@@ -11,3 +11,7 @@ sm-user-plugin's frontend permission catalogue (`golem15_user_frontend_permissio
Add a Go capability, the frontend counterpart of `pact.HasPermissions` (WinterCMS `registerPermissions()`): a plugin returns its codes with label, tab and comment, and the user plugin offers them in the user and group permission editors next to the seeded rows. Additive only: existing seeded rows, table, codes and payloads stay as they are (core plugin contract).
Prerequisite of Phase 19.7 (generic MCP module): `tools/list` is filtered by these codes. Design: `.planning/notes/2026-10-17-generic-mcp-module.md`.
## Closed 2026-10-23
Shipped in phase 19.7 (D-09). The framework capability `pact.HasFrontendPermissions` (plan 19.7-01, framework v0.4.4) lets a plugin declare its frontend permission codes with label, tab and comment. sm-user-plugin v0.1.10 (plan 19.7-03) offers them in the user and user group permission editors after the seeded rows of `golem15_user_frontend_permissions` (`classes.FrontendPermissionCatalog`; a seeded row wins, a code is offered once). Pinned by `TestFrontendPermissionCatalogSmoke` and `TestFrontendPermissionCatalog` (plan 19.7-05). g15office task `hmguritm`.