diff --git a/.planning/phases/19.7-generic-mcp-module/19.7-REREVIEW.md b/.planning/phases/19.7-generic-mcp-module/19.7-REREVIEW.md new file mode 100644 index 00000000..8e554e1c --- /dev/null +++ b/.planning/phases/19.7-generic-mcp-module/19.7-REREVIEW.md @@ -0,0 +1,538 @@ +--- +phase: 19.7-generic-mcp-module +reviewed: 2026-10-23 +depth: deep +kind: re-review of the gap closure (plans 06-09) +lanes: [A framework MCP core, B cabana and admin SPA, C plugins and demo] +files_reviewed: 129 +findings: + critical: 1 + warning: 10 + info: 17 + total: 28 +status: issues_found +--- + +# Phase 19.7 re-review of the gap closure + +This re-reviews the changes made by gap-closure plans 06-09, after the first deep review in `19.7-REVIEW.md`. That file is left unchanged because the plans and security documents cite its finding ids. Three deep lanes ran in parallel over: the framework since fef1cbb, sm-user-plugin since 32f9259^, sm-oauthserver-plugin since 671513d^, sm-journal-plugin since b129902^, vue-demo-app since 0125e29^ and sm-demo-app since 00f57bf^. Finding ids carry the lane prefix (RA-, RB-, RC-). Each lane also rules on every earlier critical and warning finding of its lane ("Earlier findings"). + +| Lane | Critical | Warning | Info | Earlier findings not fully closed | +|------|----------|---------|------|-----------------------------------| +| A framework MCP core | 0 | 6 | 6 | A-WR-07 partial; A-WR-04 and A-WR-06 fixed with regressions | +| B cabana and admin SPA | 0 | 2 | 4 | B-WR-02 partial | +| C plugins and demo | 1 | 2 | 7 | C-CR-01 partial, C-WR-02 partial | + + +# Lane A + + +## Phase 19.7 (lane A re-review): Code Review Report + +**Reviewed:** 2026-10-23 +**Depth:** deep +**Files Reviewed:** 45 +**Status:** issues_found + +## Summary + +I re-reviewed lane A's gap-closure changes (`git diff fef1cbb..HEAD`) and the code they touch: the session reaper, the atomic per-binding reservation (memory and Redis Lua), the lock-scope rework, fingerprint-based widening, the stateless listen re-check and eviction close, the Redis warm-up, the narrowed revocation events, the dispatch hardening, the catalog boot checks, bouncer's `auth_time`, towel's client address and the gate script. I traced the calls into go-sdk v1.8.0 (`Server.Sessions`, `changeAndNotify`/`notifySessions`), into cabana (`fireTokenRevoked`, `ResolveBackendToken`, `BackendPrincipal`) and into the publishers of `pact.MCPSubjectChanged` in the OAuth server and user plugins. `go vet -tags summercms_mcp` on pier, surf and bouncer is clean. `go test -race -short ./modules/pier` passes. + +Every earlier lane-A critical and warning has a fix in the code, not only in the plans. The reaper closes expired local copies and logs `expired`. Reserve is atomic in both stores. Path values are cleaned and required. Raw dispatch errors are logged, not returned. Personal tokens without `TokenID` are refused. The Redis store is warmed at mount. Eviction closes the evicted server's sessions. I found no identity or guard bypass. Widening adds tools to a cached server, but the receiving middleware still filters every list and call for the requesting identity, so no request gains reach it does not have. Revocations narrowed by `TokenID` and `ClientID` match the events cabana and the plugins publish. The `ClientID == ""` condition stops an admin-token revoke from also ending an OAuth session that acts as the same admin. + +The new defects are in the concurrency and caching that the fixes introduced: + +- **A-WR-07 is only partly fixed.** Event fan-out still takes each per-session lock while it holds the router-wide lock. Widening and recomputing hold that per-session lock across guard I/O, and widening runs with no deadline. One slow check plus any event still stalls the instance. +- **The fingerprint includes the OAuth access-token id.** Every token rotation re-filters the session. A stateless server shared by two live tokens of one binding flips its tool set on every request, which races calls into "unknown tool" errors. +- **A transient guard error during widening removes tools from a live session** and records the new fingerprint, so the tools stay gone until the next rotation or event. +- **Capacity eviction now closes live listen streams and in-flight calls.** With more concurrent bindings than the LRU capacity, the stateless path keeps forcing clients to reconnect. +- **Prompt `Render` errors still reach the client raw** (the same class as A-WR-08). +- **The gate passes with demo submodules ahead of their go.mod pins** by non-test commits unless `PHASE197_RELEASED=1`. The final "gate passed" line gives no caveat. + +## Earlier findings + +| Id | Verdict | Evidence | +|----|---------|----------| +| A-CR-01 | fixed | `modules/pier/pier.go:143-148` starts `reap`. `modules/pier/session.go:483-531`: `reapOnce` snapshots `sr.local`, calls `store.Get` for each entry, then `closeLocal` and logs `expired`. A store error keeps the copy. `reapInterval` (`:535-537`) is between 1 s and 1 min. The memory store's `Get` honours expiry (`store_memory.go:44-53`). The Redis path relies on PX TTLs. `reaper_test.go` covers both stores. The reaper ctx is never cancelled in production (no shutdown hook, documented at `pier.go:63-67`). | +| A-WR-01 | fixed | `modules/pier/dispatch.go:119-128` refuses empty, `.` and `..` segments in every path value, and `/` in a single-segment value. Wildcard values are escaped per segment (`:376-383`). | +| A-WR-02 | fixed | Boot: `modules/pier/catalog.go:220-224` refuses a path field that is not in `schema.Required`. Runtime: `dispatch.go:142-148` refuses a missing path value. | +| A-WR-03 | fixed | `modules/pier/session.go:143-173` reserves before connecting and releases on a failed connect or initialize. The memory `Reserve` counts and writes under `m.mu` (`store_memory.go:101-117`). The Redis `reserveScript` prunes, counts, SETs and SADDs in one EVAL (`store_redis.go:144-182`). `lazyStore.Reserve` forwards to it (`:326-339`). The non-reserver fallback (`session.go:156-166`, `:187-193`) is still check-then-act, but only a test-wrapped store reaches it (RA-IN-04). | +| A-WR-04 | fixed, with regressions | `modules/pier/events.go:131-141` (`widen`) is called from `session.go:332-335` and `stateless.go:185-190`. The fingerprint is `identity.go:85-116`. The new defects are RA-WR-02 and RA-WR-03. | +| A-WR-05 | fixed | `modules/pier/pier.go:146-148` runs `ls.warm` at mount with a backoff (`store_redis.go:279-294`). `route` kicks the open on every request (`stateless.go:248-250`). `open` subscribes with `context.Background()` (`pier.go:178`). | +| A-WR-06 | fixed, with a regression | The listen re-check is at `modules/pier/stateless.go:98-123`. Capacity eviction closes the evicted server's sessions (`:151-167`). The new defect is RA-WR-04. | +| A-WR-07 | partially fixed | `connect` now runs without `sr.mu` (`session.go:174-186`, `:308-331`), and the stateless build runs without `sp.mu` (`stateless.go:137-150`). But `matchingFunc` still locks each `ls.mu`/`e.mu` while holding `sr.mu`/`sp.mu` (`events.go:190-203`, `stateless.go:205-218`), and `widen`/`recomputeSession` hold `ls.mu` across guard I/O (`session.go:332-335`, `events.go:98-109`, `:138`). See RA-WR-01. | +| A-WR-08 | fixed (dispatch) | `modules/pier/dispatch.go:179-189` logs the cause and returns a fixed text. Only `ErrMCPTokenInvalid` and `errSubjectUnavailable` (`backend_guard.go:73-86`) get specific texts. The prompt path still returns raw errors (RA-WR-05). | +| A-WR-09 | fixed | `modules/pier/identity.go:267-272` refuses `TokenID == 0`. The contract is documented at `pact/mcp.go:241-249`. | + +## Warnings + +### RA-WR-01: Event fan-out still nests per-session locks under the router-wide lock, while those locks are held across guard I/O with no deadline (A-WR-07 not closed) + +**File:** `modules/pier/events.go:190-203`, `:98-109`, `:131-141`; `modules/pier/stateless.go:205-218`, `:185-190`, `:171-181`; `modules/pier/session.go:295-297`, `:332-335` +**Issue:** The fix moved `connect` out of `sr.mu`, but it did not change the lock order. `sessionRouter.matchingFunc` takes `sr.mu` and then each `ls.mu` in turn. `statelessPath.matchingFunc` does the same with `sp.mu` and `e.mu`. Meanwhile: +- `lookup` holds `ls.mu` across `widen`, which calls `catalog.allowed` (database-backed `MCPAllows`/`MCPAllowsMethod`). +- `recomputeSession` and `recomputeStateless` hold `ls.mu`/`e.mu` across `refreshBackend` (the link resolver) and `catalog.allowed`. +- `widen` uses `context.WithoutCancel(ctx)`, which removes the request's deadline as well as its cancellation. `recomputeSession` uses `context.Background()`. + +**Failure scenario:** +1. Session S's request widens after a token rotation (the fingerprint changed, see RA-WR-02), or an event is recomputing S, while the database is slow. +2. A second event arrives. The memory store runs every event on its own goroutine (`store_memory.go:127-135`), so two events in flight at once is normal: one role change fires one event per user. +3. That event's `matchingFunc` holds `sr.mu` and blocks on `S.ls.mu`. +4. Every stateful request on the instance now blocks: `lookup` at `session.go:295`, `serveNew` at `:184`, `closeLocal`, the reaper and the `connect` goroutine. The stateless path behaves the same way through `sp.mu` and `hit()`. +5. The guard check has no deadline. If the database waits on an exhausted pool, the stall lasts until a connection frees. The endpoint is down instance-wide. + +`TestLockScope` only blocks an initialize, which no longer holds a lock. It never combines a blocked `widen` or recompute with an event, so it does not cover this path. +**Fix:** Never wait on a per-session lock while holding the router lock. Snapshot the entries under `sr.mu`, release it, then evaluate the match: +```go +func (sr *sessionRouter) matchingFunc(match func(Identity) bool) []*localSession { + sr.mu.Lock() + all := make([]*localSession, 0, len(sr.local)) + for _, ls := range sr.local { + all = append(all, ls) + } + sr.mu.Unlock() + var out []*localSession + for _, ls := range all { + ls.mu.Lock() + hit := match(ls.identity) + ls.mu.Unlock() + if hit { + out = append(out, ls) + } + } + return out +} +``` +Do the same in `statelessPath.matchingFunc`. Alternatively, keep `identity` behind its own small mutex or an `atomic.Pointer[Identity]` that is never held across I/O. Also give `widen` and the recomputes a bounded context, for example `context.WithTimeout(context.WithoutCancel(ctx), d.cfg.CallTimeout)`. Extend `TestLockScope` with a guard that blocks inside `widen`, plus a concurrent `MCPSubjectChanged`, and assert that another session's call still finishes. + +### RA-WR-02: The fingerprint includes the OAuth access-token id, so a shared stateless server flips its tool set between two live tokens of one binding + +**File:** `modules/pier/identity.go:85-116` (`TokenID` at `:103`), `:216-233`; `modules/pier/stateless.go:128-136`, `:185-190`; `modules/pier/events.go:131-141` +**Issue:** For an OAuth identity, `Frontend.TokenID` and `Backend.TokenID` hold `grant.TokenID`, the access-token row id (`sm-oauthserver-plugin/classes/mcp_verifier.go:110`). It changes with every refresh. The stateless LRU keeps one server per binding (`oauth::`), and every request runs `refresh` → `widen` with that request's identity. When two different tokens of the same binding are live at once, the fingerprint differs on every alternate request. Two cases are common: +- The same static client signed in on two devices. +- After a re-consent with wider scopes, an older narrower token is still valid on another device. + +Each flip runs `catalog.allowed` (guard I/O) under `e.mu` (feeding RA-WR-01). When the scopes differ, `applyDiff` adds and then removes the write tools on every alternation. +**Failure scenario:** Token A (`read write`) calls `create_post`. The receiving middleware checks A's own allowed set and passes the call. Meanwhile a request with token B (`read`) runs `widen` on the same server and calls `RemoveTools("create_post")`. The SDK's tools/call handler then answers "unknown tool" for A's legitimate call. Every flip also queues a `list_changed` notification for every listen stream of the binding. Before the gap closure the tool set was frozen. Now it flaps. +**Fix:** +- Leave token ids out of the fingerprint. The guard decision depends on scopes, subjects and the ceiling, not on which token row carried them. +- Key the stateless LRU by `binding + "|" + fingerprint`, as the earlier review suggested, so identities with different reach never share or mutate one server. +- At minimum, never remove tools in `widen` on the stateless path: the receiving middleware already filters down per request. + +### RA-WR-03: A transient guard error during widening permanently narrows a live session + +**File:** `modules/pier/events.go:131-141`; `modules/pier/filter.go:33-47`, `:49-64` +**Issue:** `catalog.allowed` turns a provider error into "not allowed" (logged). `widen` then runs `applyDiff`, which removes every tool or prompt whose check errored, and stores the new fingerprint. Later requests with the same fingerprint skip `widen`. The receiving middleware can only filter down, so the removed tools cannot come back. They stay hidden until the fingerprint changes again or a subject event recomputes the session. Server construction already had a weaker form of this, but it ran once. Because of RA-WR-02, widening now runs on every OAuth token rotation. +**Failure scenario:** A pool timeout during the widen at a token rotation hides every backend tool of a long-running agent session for a whole access-token lifetime. Calls to those tools fail as "not available to this caller" although nothing changed. +**Fix:** Let `widen` tell an error apart from a refusal. For example, add an `allowedStrict` that returns an error, and on error leave the server untouched and leave `*fp` unchanged, so the next request retries: +```go +tools, prompts, err := p.catalog.allowedStrict(bounded, id) +if err != nil { + logger().Warn("pier: widening deferred", "error", err) + return // keep *fp, retry on the next request +} +applyDiff(...) +*fp = next +``` + +### RA-WR-04: LRU capacity eviction now ends live listen streams and in-flight calls, so the stateless path churns past capacity + +**File:** `modules/pier/stateless.go:151-167`, `:192-197`, `:55-62` +**Issue:** A `subscriptions/listen` POST calls `getServer` once, when it starts. A quiet listener never touches its entry again, so the entry drifts to the back of the LRU (capacity `max_per_binding * 16`, 320 by default). The A-WR-06 fix makes eviction call `closeSessions` on the evicted server. That ends the listen stream, and it also ends any in-flight tools/call of that binding: the per-request `ServerSession` is in `srv.Sessions()`, and closing its connection cancels the dispatched route's context mid-handler. +**Failure scenario:** More than 320 distinct bindings are connected with listen streams. Each new binding's first request evicts the oldest listener. That client reconnects and evicts the next one, and so on. Every stream keeps reconnecting, list changes sent during the gaps are lost, and a write tool call caught by an eviction is cancelled partway, so the agent gets an error for a request whose effect may or may not have committed. +**Fix:** +- Do not count entries with an open listen stream as evictable. For example, keep a per-entry count of active listens, raised in `ServeHTTP` around `sp.handler.ServeHTTP` for listen requests, and skip such entries when evicting. +- Or move such an entry to the front on every bearer re-check tick. +- Or leave the streams open on eviction (the 1-minute bearer re-check already bounds them). Then a revocation must also close the streams it can no longer find, for example through a binding-to-streams index that eviction does not clear. +- Never close the sessions of a request that is still being served. + +### RA-WR-05: Prompt `Render` errors are returned to the MCP client verbatim + +**File:** `modules/pier/session.go:719-722` +**Issue:** `promptHandler` returns `pr.decl.Render`'s error as is. The SDK turns it into the JSON-RPC error message. A plugin's `Render` that reads the database (to list drafts, look up a record or build context) can then send pgx or gorm text, which may include the host, database and user names, to the outside client and its transcript. This is the leak A-WR-08 closed for dispatch, left open for prompts. +**Fix:** +```go +messages, err := pr.decl.Render(ctx, args) +if err != nil { + logger().Warn("pier: prompt render failed", "prompt", pr.name, "error", err) + return nil, &jsonrpc.Error{Code: jsonrpc.CodeInternalError, Message: "pier: the prompt could not be rendered"} +} +``` + +### RA-WR-06: The gate passes while demo submodules carry non-test commits that the go.mod pins do not ship + +**File:** `scripts/check-phase19.7.sh:957-961`, `:977-982`, `:1158-1170` +**Issue:** `demo_pins` now accepts a submodule that is ahead of its pin by non-test commits whenever the canonical checkout's HEAD contains them. It fails only with `PHASE197_RELEASED=1`, and the default is 0. The demo's own `check-pins.sh` exists to prevent exactly this: "they must be the same code, or the tests pass against something that is not shipped". With the default, the consumers stage runs the demo's builds and tests against the submodule code, while a release build (GOWORK=off) compiles the older pinned version. `run_all` still prints "Phase 19.7 gate passed". The acceptance is reported only as a mid-stage `pins:` line. The canonical check also accepts any commit on whatever branch the canonical checkout is on, pushed or not. +**Failure scenario:** The phase is marked verified on "gate passed". Then the demo is tagged and deployed with the old pins, without the gap-closure plugin fixes the tests exercised. +**Fix:** +- Make the release check fail closed by default: require an explicit `PHASE197_ALLOW_UNRELEASED=1` to accept unreleased drift. +- When drift was accepted, end `run_all` with a non-pass verdict, for example `Phase 19.7 gate passed WITH UNRELEASED PINS (not releasable)`, and a distinct exit code. +- Optionally require that the commit exists on the canonical checkout's upstream (`git branch -r --contains`). + +## Info + +### RA-IN-01: `lazyStore`'s doc comment says nothing dials at mount + +**File:** `modules/pier/store_redis.go:231-233` +**Issue:** The comment still says "nothing dials at mount (route:list and tests never dial)". `Mount` now starts `warm`, which dials Redis in the background (`pier.go:146-148`). If a CLI command mounts pier while Redis is unreachable, it also logs "session store not open yet, retrying". +**Fix:** Reword the comment to match `Mount`'s and decide whether non-serving commands should skip `warm`. + +### RA-IN-02: Stream re-checks resolve the bearer without the client address + +**File:** `modules/pier/session.go:438`; `modules/pier/stateless.go:114` +**Issue:** The bearer middleware puts `towel.WithClientIP` on the resolver context (`pier.go:254`), but the GET-stream and listen re-check goroutines call `identities.resolve(ctx, raw)` on a context without it. A re-check that fails, for example a revoked admin token, is logged by `cabana.ResolveBackendToken` with an empty remote. That is the case B-IN-04 set out to make traceable. +**Fix:** Capture `p.clientIP(r)` when the stream starts and resolve with `towel.WithClientIP(ctx, ip)`. + +### RA-IN-03: `reserveScript` touches keys it does not declare + +**File:** `modules/pier/store_redis.go:144-160` +**Issue:** The script runs `EXISTS` on `ARGV[1] .. m` for every set member, so it touches keys missing from `KEYS`. A standalone `redis.Client` accepts this. Redis Cluster refuses it (cross-slot), and so do ACLs restricted by key pattern (undeclared keys). The binding set and the record key would not hash to one slot anyway. +**Fix:** Document that the store needs a standalone Redis, or put a hash tag on the binding in both key names (`mcp:{}:...`) and keep the per-member keys in the same slot. + +### RA-IN-04: The non-reserver fallback in `serveNew` is still check-then-act and briefly exposes an unrecorded session + +**File:** `modules/pier/session.go:156-166`, `:184-193` +**Issue:** Both shipped stores (and `lazyStore`) implement `reserver`, so only a test wrapper (`WrapStore`) reaches this branch. On it, the cap is TOCTOU, and the session sits in `sr.local` before its record is written, so a reaper pass in between closes it as expired. +**Fix:** Make `reserver` part of `Store`, or delete the fallback and refuse a store without `Reserve` at mount. + +### RA-IN-05: A backend token whose scopes no longer resolve gets "account unknown or deactivated" + +**File:** `modules/pier/backend_guard.go:99-103`, `:79-86`; `modules/pier/dispatch.go:184-185` +**Issue:** `principal` returns `nil, nil` when `ScopeLimitFor` reports `known == 0`, for example when a scope's plugin was removed. `MCPDispatch` then answers `errSubjectUnavailable`, and the agent reads that the administrator's account is unknown or deactivated, which is false. +**Fix:** Return a distinct sentinel, for example `errScopeUnavailable` with "the token's scopes no longer grant this route", and map it in `dispatch.go`. + +### RA-IN-06: `demo_pins` passes when `check-pins.sh` exits non-zero with no output + +**File:** `scripts/check-phase19.7.sh:962-990` +**Issue:** With `rc != 0` and empty output, the here-string feeds one empty line, `[[ -z "$line" ]] && continue` skips it, and the function returns 0. The current `check-pins.sh` prints a line on every failure path, so this is unlikely, but the check fails open. +**Fix:** After the loop, refuse unless at least one mismatch line was matched and accepted: track `accepted=1` inside the branch, and `[[ -n "$accepted" ]] || { echo "refuse: check-pins failed without a mismatch line" >&2; return 1; }`. + +--- + +_Reviewed: 2026-10-23_ +_Reviewer: Claude (msd-code-reviewer)_ +_Depth: deep_ + +# Lane B + + +## Phase 19.7 (lane B re-review): Code Review Report + +**Reviewed:** 2026-10-23T19:30:00Z +**Depth:** deep +**Files Reviewed:** 24 +**Status:** issues_found + +## Summary + +This re-review covers the lane B gap-closure changes between fef1cbb and HEAD (commits 9eedfbb, 24cf6cd, 44835e4, 88559d5 and the plan 09 tests). I re-read the code each fix touches and the code it calls: +- the pier event path: `listenEvents`, `handleEvent`, `revokedMatch`, and how identities are resolved in `identity.go`; +- `lagoon.AfterCommit` with no transaction; +- `towel.ClientIP` with a nil ctx; +- `revokeAPIToken`, `normalizeTokenScopes` (api_tokens.go:135-160); +- `FormView`'s `
` wrapper and save button `disabled` conditions; +- `PluginField`'s `summer-change` protocol; +- every i18n key the SPA component uses, in en, pl and the fixture. + +Verified correct: +- **Token revoke event.** `fireTokenRevoked` is called with `row.ID`. The pier `Event` keeps `TokenID` through the store (`token_id` json tag). `revokedMatch` matches only subjects with `TokenID == ev.TokenID && ClientID == ""`. An OAuth backend subject always has `ClientID` set (identity.go:227-233), so OAuth sessions survive. The revoke route has no transaction, so `AfterCommit` fires at once, which is correct. +- **URL rule (server).** `safeAccountActionURL` refuses `javascript:`, `data:`, `vbscript:`, `//host`, `/\host`, `https:host` and `http:/host`, plus a leading space or tab/newline, because Go's `url.Parse` rejects control characters. I confirmed this by running the function. +- **Per-section isolation.** A failing Status sets `error` (`omitempty`, so this only adds data). A failing Applies drops only that action. The SPA toasts on a failed GET and keeps the sections already shown. +- **OpenAPI.** `scripts/check-admin-openapi.sh --check` exits 0, and `scripts/check-admin-dist.sh` reports that the dist matches. +- **Phrase keys.** `form.action_prompt_invalid`, `myaccount.action_confirm`, `myaccount.action_stale` and `myaccount.section_error` sit under the right YAML sections in en and pl and are in the fixture. +- **Logging.** `ResolveBackendToken` logs the remote address on all seven paths, and pier puts the address on the context (pier.go:254). +- **Frontend scope seam.** cabana never reads `HasFrontendTokenScopes` (grep). +- **No raw HTML.** `AccountContributions.vue` has no `v-html` and uses text interpolation only. +- **Checks run.** `go vet`, the targeted cabana `-short` tests, pact and pier `-short`, `TestDocsTree`, the two vitest files and `vue-tsc` all pass. + +No blockers. Two warnings: +- The draft-commit fix for keyboard saves does not reach a focused input inside a plugin field type's shadow root. +- The "one shared name rule" for token scopes is not actually one rule in cabana, because cabana trims the name before checking it and then registers the untrimmed name. + +## Earlier findings + +| ID | Verdict | Evidence | +|----|---------|----------| +| B-WR-01 | fixed | `admin/src/app/hotkeys.ts:121-124` (`originOf` uses `composedPath()[0]`), `:241` (used in `onPlainKey`), `:113` (a custom element inside `form`/`[role=form]` counts as editable for closed roots; FormView wraps fields in `` at `views/FormView.vue:658`). Tests: `admin/tests/app/hotkeysEditing.test.ts:71-129`. | +| B-WR-02 | partially fixed | `admin/src/app/hotkeys.ts:227-233` blurs `doc.activeElement` before `click()`. This covers light-DOM fields (the taglist; tests at `hotkeysEditing.test.ts:131-181`). When focus is inside a shadow root, `doc.activeElement` is the shadow host, and `host.blur()` does not unfocus the inner input (see RB-WR-01). | +| B-WR-03 | fixed | Server: `modules/cabana/account_contributions.go:217-221`, `:233-242`. SPA: `admin/src/components/account/AccountContributions.vue:31-39`, `:64-68`, `:128-135`. The SPA check has a defense-in-depth gap the server covers (RB-IN-01). | +| B-WR-04 | fixed | `modules/cabana/account_tokens.go:167` calls `fireTokenRevoked(..., principal.ID, row.ID)`. `modules/cabana/subject_events.go:51-65` puts `TokenID` on the event. `modules/pier/events.go:21`, `:79-93` narrow the match to that token. Test: `modules/cabana/account_token_events_test.go:20-61` (Postgres). | +| B-WR-05 | fixed | `modules/cabana/admin_openapi.go:802`, `:812`, `:831-832`. Regenerated into `admin/src/api/schema.d.ts:3033`, `:5586`. `check-admin-openapi.sh --check` exits 0. | +| B-WR-06 | fixed (one residual gap, RB-IN-02) | `modules/cabana/account_contributions.go:113-138` (error flag on the section, failing action dropped). `AccountContributions.vue:41-53` (toast on a failed GET), `:125-127` (section error line). Tests: `account_contributions_hardening_test.go:137-188`, `AccountContributions.test.ts` "a 500 on the list reload". | + +Info items from the first review: +- **Fixed:** B-IN-01 (`AccountContributions.vue:74`, `:100`), B-IN-02 (`actions.go:249`, `:269`), B-IN-03 (`account_contributions.go:121`), B-IN-04 (`token_resolver.go:29`). +- **Deferred:** B-IN-05 (deferred-items.md:177). +- **Accepted:** B-IN-06, with the visible link as the fallback. + +## Narrative Findings (AI reviewer) + +## Warnings + +### RB-WR-01: The draft-commit blur does not reach an input inside a plugin field type's shadow root (B-WR-02 residual) + +**File:** `admin/src/app/hotkeys.ts:227-233` +**Issue:** The fix blurs `doc.activeElement`. When focus is on an `` inside a shadow root (the plugin field types and widgets that `modules/cabana/README.md:185` explicitly allows), `document.activeElement` is the shadow host, not the input. Calling `blur()` on a host that is not itself focused does nothing in two places: +- **The HTML spec.** The unfocusing steps return early when the target is not in the current focus chain. The only exception is a host with `delegatesFocus`. +- **Firefox.** `ShouldBlur` compares the element with the focused descendant, which is the inner input. + +Chromium happens to blur here, because `AdjustedFocusedElementInTreeScope() == this`. The behaviour therefore differs by browser. + +`PluginField.vue:13-24` defines the value protocol as a `summer-change` event. A plugin that emits it from the inner input's native `change` event (fires on blur) still loses the typed value in Firefox: +- **Ctrl+S** saves without it. +- **Ctrl+Enter** saves without it and navigates away. + +This is exactly the "plugin field type that commits on the native change event" case B-WR-02 named. The host's `focus()` afterwards is also a no-op for a non-focusable host, so focus is lost. + +The new tests cover only the light-DOM taglist (`hotkeysEditing.test.ts:131-181`). + +**Fix:** do what a pointer click does: focus the button before clicking it. Moving focus works in every browser and for closed roots too, because the inner input is unfocused and its blur/change fire. +```ts +// The deepest focused element reachable through open shadow roots. +let previous: Element | null = doc.activeElement +while (previous?.shadowRoot?.activeElement) { + previous = previous.shadowRoot.activeElement +} +const restore = previous instanceof HTMLElement && previous !== target && previous !== doc.body ? previous : null +// Pointer-equivalent: mousedown focuses the button, which blurs whatever had +// focus, shadow-root inputs included (blur() on the host does not). +if (restore) { + target.focus({ preventScroll: true }) +} +target.click() +if (restore?.isConnected) { + restore.focus() +} +``` +At minimum, descend `shadowRoot.activeElement` for open roots before calling `blur()`. Add a test with an open-shadow-root input whose `change` handler emits `summer-change`. + +### RB-WR-02: A scope name is validated trimmed but registered untrimmed, so the shared name rule is not one rule + +**File:** `modules/cabana/token_scopes.go:106-118` (validation), `:89-98` (registration); `modules/cabana/api_tokens.go:146-152` +**Issue:** `validatePluginTokenScope` validates `strings.TrimSpace(scope.Name)` with `pact.ValidTokenScopeName`. `compileTokenScopes` then stores `scope.Name` untrimmed, both in `compiledTokenScope.Name` and as the `byName` key. With a padded name such as `" blog:read"` or `"blog:read\n"`, three things go wrong: +- **Boot accepts it.** `pact.ValidTokenScopeName(" blog:read")` is false, so the parity this gap closure claims (P-30) does not hold. The docs say "Both catalogs check names with `pact.ValidTokenScopeName`" (`docs/backend/users-and-permissions.md:119`), and the `HasFrontendTokenScopes` doc promises that a bad name fails boot. +- **The scope is listed but can never be granted.** It appears in `GET /account/tokens/scopes` under its padded name. Token creation trims every requested name (api_tokens.go:147), looks it up as `byName["blog:read"]`, misses, and answers `scope_unavailable`. +- **The duplicate check depends on order.** The check looks up the trimmed name. If `" read"` registers first and `"read"` second, the second is not detected as a duplicate, and both register. + +`TestTokenScopeNameParity` (token_scope_name_test.go:12-44) has no padded names, so it does not catch any of this. The pre-existing trim mismatch matters now because this change is what turns `ValidTokenScopeName` into the advertised single rule. + +**Fix:** validate exactly what is registered. Either refuse a name that is not already trimmed: +```go +name := scope.Name +if name == "" || name != strings.TrimSpace(name) || !pact.ValidTokenScopeName(name) { + return fmt.Errorf("cabana: plugin %s registered invalid token scope name %q", pluginID, scope.Name) +} +``` +or register the trimmed `name` (`Name: name`, `cat.byName[name]`). Add `" read"` and `"read\n"` to the parity table, expecting refusal. + +## Info + +### RB-IN-01: The SPA URL check passes `//host`, which browsers resolve off-site + +**File:** `admin/src/components/account/AccountContributions.vue:31-39` +**Issue:** `safeUrl` accepts any string that starts with one `/` and not `//` or `/\`. Browsers strip ASCII tab and newline from URLs, so `"/\t/evil.example/x"` and `"/\n/evil.example"` resolve to `https://evil.example/...` (confirmed with WHATWG `URL`). This is the same-origin bypass the check exists to stop. + +The server check catches it, because Go's `url.Parse` rejects control characters, so this is a defense-in-depth gap only. The server and SPA rules are also documented as "the same rule" (`:27-29`), which they are not. + +**Fix:** strip or refuse control characters first, e.g. `if (/[\u0000-\u001f\u007f]/.test(u)) return ''`. Or resolve with `new URL(u, location.origin)` and require `protocol` http(s) and, for relative input, `origin === location.origin`. + +### RB-IN-02: An action whose Applies fails disappears with no signal + +**File:** `modules/cabana/account_contributions.go:128-135`; `AccountContributions.vue:125-127` +**Issue:** A Status failure sets `error: true` and shows the section error line. An Applies failure only drops the button. If the section's Status still succeeds, the admin sees a normal section (for example "Linked to …") with its Unlink button missing and no message. That is the "fails silently" half of B-WR-06 again, for a single action. +**Fix:** set `view.Error = true` when any action's Applies fails as well (and document `error` as "status or an action could not be loaded"), so the SPA shows the section error line. + +### RB-IN-03: The CLI `admin:token:revoke` fires no revoke event + +**File:** `modules/cabana/token_commands.go:225-255` (interacts with `account_tokens.go:167`) +**Issue:** The My account revoke now ends that token's MCP sessions. The operator revoke command fires nothing, so the token's open sessions (and any open GET listen stream) stay until the next request's bearer re-verification refuses them. The README entry for backend subject events says a revoked personal API token fires the event, without saying this applies to the HTTP route only. +**Fix:** call `fireTokenRevoked(ctx, app, row.BackendUserID, row.ID)` after the update when the command runs with the application bus. Otherwise, document that only the My account revoke fires. + +### RB-IN-04: The custom-element-in-form rule also mutes Esc on focusable non-field widgets + +**File:** `admin/src/app/hotkeys.ts:113` +**Issue:** Any focused element with a hyphenated tag inside a `` counts as editable. That includes `WidgetField` hosts (`components/form/fields/WidgetField.vue:61`) that are focusable but not inputs, such as a chart or a button strip. While one of them has focus, Esc does not leave the form. This is an accepted trade-off for closed roots, but it is undocumented. +**Fix:** mention it in the header comment at `hotkeys.ts:15-25`. Optionally narrow the rule to hosts rendered by `PluginField` (for example via a `data-field-type` attribute the SPA sets on plugin field elements). + +--- + +_Reviewed: 2026-10-23T19:30:00Z_ +_Reviewer: Claude (msd-code-reviewer)_ +_Depth: deep_ + +# Lane C + + +## Phase 19.7: Code Review Report (lane C re-review: plugins and demo) + +**Reviewed:** 2026-10-23T17:19:01Z +**Depth:** deep +**Files Reviewed:** 60 +**Status:** issues_found + +Abbreviations: **U** = sm-user-plugin (`sm-demo-app/plugins/golem15/user`, range 32f9259^..HEAD), **O** = sm-oauthserver-plugin (671513d^..HEAD), **J** = sm-journal-plugin (b129902^..HEAD), **V** = vue-demo-app (0125e29^..HEAD), **D** = sm-demo-app (00f57bf^..HEAD), **F** = summercms.go (framework, read for call chains only). + +Checks run: `go vet ./...` and `go test -short ./...` in U (with `GOWORK=off`, the module's own `replace` to the framework), O and J are green. With `GOWORK=off`, O does not build (see RC-IN-07). Docker-backed tests were not run. + +## Summary + +Most of the gap closure holds up when traced through the code: + +- **Account link proof (C-CR-02).** The confirm handler reads `auth_time` again from the same token `jwt.auth` accepted, checks that its subject matches the authenticated principal, and refuses a token that has no `auth_time`. `bouncer.Refresh` copies `auth_time` unchanged (F `bouncer/refresh.go:141-142`). The plugin's only mint sites (`mintFor`: login, registration, activate-by-code) are real sign-ins. No path in U mints a frontend token without one. +- **Link code transport (C-WR-01).** The code lives only in the fragment. The admin SPA opens the link with `window.open(url, '_blank', 'noopener')`, so the Referer is the admin page and never carries the code. Browsers never put a fragment in a Referer. The page moves the code to `sessionStorage`, strips it with `replaceState`, and sends `/login?next=/link-admin` without it. +- **Ban, password reset and admin-set password (C-CR-01).** Each one revokes OAuth grants in the same transaction as the user write. The MCP verifier and the MCP refresh grant also refuse banned users and tokens issued before the cutoff, so the event is not the only protection. +- **Revocation scoping (C-IN-02).** A token-specific revoke and a client-specific revoke each end only their own sessions. Pier tells the two apart by `ClientID == ""` (F `pier/events.go:85-98`), so a `usr_` token id that equals an OAuth token id does not collide. +- **Journal (C-IN-01).** The `draft_post` ceiling is enforced by pier's `allowsPrompt`. `TestJournalPromptWrites` would catch a regression because it checks both the read-only and the read-write grant. +- **Member token scopes (E-2).** Mint, check, list and discovery work per application. A plugin cannot gain backend reach by naming a member scope `backend`, because pier grants a backend subject only on the OAuth path. +- **Organisation logo (E-1).** The admin delete path (single and bulk) goes through cabana's `deleteRecord`, which runs `FormAfterDelete` inside the delete transaction. + +What remains: + +1. **RC-CR-01 (critical).** A user's own password change does not end an attacker's MCP or OIDC grant if that grant's tokens were created or rotated after the current session's `iat`. An attacker controls rotation timing, so the C-CR-01 scenario ("the owner changes the password after a compromise") still leaves the attacker's agent working, including any linked-admin backend reach. +2. **RC-WR-01.** C-WR-02 is closed for issued tokens only. A consent decided after the link changed, or an MCP code issued before the change and exchanged after it, still produces a `backend` grant for an administrator the consent page never showed. The token revocation also races with a concurrent refresh rotation. +3. **RC-WR-02.** The oauthserver test for the verifier's ban refusal cannot fail. `BanUsers` also moves `tokens_valid_after`, so the cutoff check alone makes the token invalid. + +## Narrative Findings (AI reviewer) + +## Critical Issues + +### RC-CR-01: Self-service password change keeps every OAuth/MCP grant whose tokens were created or rotated after the session's `iat` + +**File:** `U controllers/api_controller.go:722-732`; `O classes/cascade.go:111-113` (`created_at < before`); `O classes/mcp_store.go:252-266` (a rotated refresh token is a new row with `created_at = now`) and `:385-391` (the new access token, the same); `O classes/mcp_verifier.go:133-144` (`issuedBeforeCutoff`); `U README.md:57`; `U access_revocation_test.go:156-170` + +**Issue:** `ChangePassword` sets `tokens_valid_after = iat - 1s` and fires `FireAccessRevoked(..., before = cutoff + 1s = iat, ...)`. Here `iat` belongs to the session token that made the call. A refresh renews `iat`, so it is the time of the last refresh, not of the sign-in. The README sentence "tokens issued since that sign-in survive" is wrong on this point too. + +`revokeUserGrants` revokes only access and refresh rows with `created_at < before`. The MCP refresh grant creates a new refresh row and a new access row with `created_at = now` on every rotation. The verifier's `issuedBeforeCutoff` and `refreshUserUsable` compare that same `created_at` with the cutoff. So any grant that was refreshed after the caller's `iat`: +- is not revoked by the event, +- passes the verifier, and +- passes the refresh check, + +and lives on indefinitely. The client chooses when to refresh: an OAuth client may present its refresh token at any time. An attacker's agent that refreshes every minute is therefore never older than the victim's session unless that session was minted within the last minute. + +The ban, the password reset and the admin-set password all pass `before = time.Now()` and are not affected. Only the self-service change uses the session-relative cutoff, although nothing in the current request needs an OAuth grant kept alive. + +**Failure scenario:** An attacker learns E's password, signs in, connects an MCP agent with `read write backend` (E is linked to admin A) and lets it refresh every few minutes. E notices odd posts, opens the site (session last refreshed 20 minutes ago) and changes the password from Settings. `AccessRevokedEvent` fires with `Before = iat` (20 minutes ago). The agent's current refresh token, created 3 minutes ago, is not revoked, and the verifier accepts it because `created_at` is after the cutoff. The agent keeps calling `journal_update_post` as admin A. `TestAccessRevocation` "ChangePassword fires with the stored cutoff plus one second" pins this behaviour instead of catching it. + +For context: the same session-relative cutoff lets an attacker's frontend session JWT survive if it was refreshed after the victim's `iat`. That part predates this phase, but `auth_time` now makes a better cutoff possible. + +**Fix:** Revoke OAuth grants as of now on a self-service change. The current request needs no OAuth grant, so nothing has to be kept: +```go +// U controllers/api_controller.go, ChangePassword +cutoff := iat.Add(-time.Second) // keeps the calling session JWT, as today +revokedAt := time.Now() +if err := lagoon.Transaction(r.Context(), gdb, func(ctx context.Context, tx *gorm.DB) error { + // ... Updates as today ... + return classes.FireAccessRevoked(ctx, app, tx, []uint{user.ID}, revokedAt, classes.AccessRevokedPasswordChanged) +}); err != nil { ... } +``` +Change the README sentence on line 57 to match. Rewrite the test to create an MCP grant, rotate it after the session's `iat`, change the password, and assert that the rotated grant is revoked and refused. Optionally, base the session cutoff on the calling token's `auth_time`, and have `/refresh` and the jwt guard compare `auth_time` with `tokens_valid_after`, so an attacker's refreshed session JWT also stops surviving. + +## Warnings + +### RC-WR-01: C-WR-02 is closed for issued tokens only: pending consents and unexchanged codes still bind `backend` to whichever administrator is linked later, and the revocation races with refresh rotation + +**File:** `O classes/cascade.go:136-138` (with a non-empty `scope`, the function returns before touching `MCPCode` rows), `:198-216` (`OnBackendLinkChanged`), `:111-134` (plain `SELECT`, then `UPDATE ... WHERE id IN`); `O controllers/mcp_consent_api.go:82-89` (show reads the admin), `:133-144` (decide checks only `linked`, not which admin); `F pier/identity.go:225-235` (backend subject = whoever is linked at request time) + +**Issue:** The fix revokes unrevoked access and refresh tokens that carry `backend` when a link is created or removed. Three paths still let a grant act as an administrator the consent page did not show: +1. **Decide after a link change.** `MCPConsentShow` shows admin X. `MCPConsentDecide` checks only that some link exists (`linked`). If the link moves to Y between show and decide (an administrator's `SetLink` after an unlink, within the pending request's lifetime), the user approves "act as X" and gets a `backend` grant that pier resolves to Y. +2. **Code issued before, exchanged after.** `revokeUserGrants` with `scope = "backend"` returns at line 136-138 and never deletes the user's unused MCP codes. A code issued while linked to X and exchanged after a relink to Y mints a fresh token (`created_at` after the revocation) with `backend`, acting as Y. +3. **Rotation race.** The revocation reads candidate rows with a plain `SELECT` under READ COMMITTED, then updates them by id. If a refresh rotation of the same grant is in flight, the `UPDATE` waits on the old row's lock. Once the rotation commits, the new refresh row (inserted after the `SELECT`'s snapshot) is never revoked and keeps `backend` across the link change. + +The README now promises that "a grant never acts as an administrator the user did not consent to". These paths contradict it. Each window is short, so this is a warning. + +**Fix:** Bind the grant to the consented administrator, so no revocation timing matters (this was option 2 of C-WR-02 and is additive): +- At decide, take the `backend_admin.login` (or id) the page showed in the body and refuse with 409 when the current link differs. +- Store `backend_user_id` on the MCP code, access and refresh rows. Copy it on code exchange and rotation, expose it in `pact.MCPGrant`, and have pier drop the backend subject when the current link's admin id differs. + +As a cheaper stopgap: in `OnBackendLinkChanged` also delete the user's MCP codes with `used_at IS NULL` whose scopes contain `backend`, and lock the user's refresh rows (`SELECT ... FOR UPDATE` on `user_id`) before collecting ids, so a concurrent rotation is serialized behind the revocation. + +### RC-WR-02: The verifier's ban refusal is untested; the "banned user is refused" test passes even with the ban check removed + +**File:** `O mcp_revocation_test.go:138-160`; code under test `O classes/mcp_verifier.go:96-105`, `O classes/mcp_store.go:299-313` (`refreshUserUsable` ban branch) + +**Issue:** The subtest bans with `userclasses.BanUsers`, which calls `RevokeUserAccess` and stamps `tokens_valid_after = now`. The grant was created before that, so the `issuedBeforeCutoff` check already returns `ErrMCPTokenInvalid` and the refresh is already refused. Deleting the `ThrottleStates` and `banned[...]` blocks from both the verifier and `refreshUserUsable` leaves the test green. + +The ban check is the only protection when a ban is written without `RevokeUserAccess`, which the user README explicitly allows applications to do ("after writing a ban directly to `user_throttle`"). The refresh path's pre-cutoff branch has no isolated rotation test either. The suite covers rotation only for a deleted user, while the doc comment of `TestMCPVerifierRevocation` claims "the refresh rotation refuses a banned, a pre-cutoff and a deleted user". The demo's e2e step (a) cannot tell the three mechanisms apart either. + +**Fix:** In the ban subtest, write the ban without moving the cutoff, then assert both refusals: +```go +if err := env.gdb.Exec(`INSERT INTO user_throttle (user_id, is_banned) VALUES (?, TRUE)`, user.ID).Error; err != nil { + t.Fatal(err) +} +invalid(t, "banned (no cutoff moved)", grant) +status, out := env.refreshStatus(t, grant) +refused(t, "banned (no cutoff moved)", status, out) +``` +Add a rotation subtest for a pre-cutoff refresh token (`setGrantCreated` before `setCutoff`, then `refreshStatus` must be `invalid_grant`). The "same second" subtest only partly covers this. + +## Info + +### RC-IN-01: The server still accepts the link code in URLs + +**File:** `U routes.go:46-48`; `U controllers/backend_link_controller.go:36-54, 111-117`; `U controllers/api_controller.go:1166-1189` (`readFields`); `D deploy/nginx/demo.summercms.io.conf:96-103` +**Issue:** The shipped page never puts the code in a URL. The server, though, still serves the deprecated `GET /backend-link/{code}`, kept on purpose for compatibility. In addition, `readFields` falls back to `r.ParseForm()` for any non-JSON body, and `r.Form` includes the query string. So `POST /backend-link/show?code=…` and `POST /backend-link/confirm?code=…` both work, and nginx's default access log records those request lines. A third-party client of this core plugin can reintroduce the leak without noticing. +**Fix:** In `BackendLinkShowPost` and `BackendLinkConfirm`, read the code only from a JSON body (ignore `r.URL.Query()`). Consider `access_log off;` (or a `log_format` without `$request_uri`) for `location ~ ^/_user/api/v1/backend-link/` until the GET route is removed in a major release. + +### RC-IN-02: The `auth_time` proof has a one-second blind spot + +**File:** `U classes/backend_links.go:276` +**Issue:** `auth_time` is truncated to whole seconds and compared with `created_at` truncated to the same second. A sign-in that happened earlier in the same second as the request, but before it, counts as fresh. The design accepts this, and it is not exploitable in practice. It is worth stating in the doc comment next to the D-05 claim. +**Fix:** Document it, or compare `auth_time >= created_at.Truncate(time.Second) + 1s` and accept that a sign-in in the request's own second needs one more sign-in. + +### RC-IN-03: The logo URL fallback ignores the configured public path + +**File:** `U classes/organisation_logo.go:57-61` (copied from `U classes/avatar.go:52-56`) +**Issue:** When `Thumb` fails, the fallback is `"/" + attach.BlobKey(f.DiskName)`. That skips `storage.uploads.public_path_prefix` and `public_url` (`F lagoon/attach/thumb.go:85-104`). With the WinterCMS layout (`/storage/app/uploads/public`) the fallback URL is a 404. +**Fix:** `return f.URL(), true, nil` (and the same in `AvatarURL`). + +### RC-IN-04: nginx's `/mcp` cap now equals pier's, so oversized bodies still get nginx's HTML 413 + +**File:** `D deploy/nginx/demo.summercms.io.conf:217`; `F pier/pier.go:38` +**Issue:** Both caps are 4 MiB. Bodies between 2 and 4 MiB now reach pier (C-IN-03 is fixed). A body over 4 MiB is still refused by nginx with an HTML 413, so the JSON-RPC error the new comment describes is never seen through nginx. +**Fix:** Set `client_max_body_size 5m;` (and update the `check-deploy.sh` string), so pier's own refusal is the one clients get. + +### RC-IN-05: Test gaps and tests that pass without running + +**File:** `U organisation_logo_test.go:138-142`; `V app/lib/linkCode.test.ts:13-21`; the new `U` Postgres tests (`backend_link_auth_time_test.go:25-27`, `backend_link_show_test.go:24-26`, `access_revocation_test.go:113-115`, `member_token_scopes_test.go:64-66`, `organisation_logo_test.go:58-60`, `api_token_events_test.go:17-19`, `mcp_guard_client_ip_test.go:17-19`), `O mcp_revocation_test.go:65-67, 184-186`, `D mcp_revocation_e2e_test.go:247-249` +**Issue:** +- The organisation delete test checks only that the `system_files` rows are gone. The after-commit `attach.DeleteKeys` part of `FormAfterDelete` (the blobs and thumbnails) is never asserted, so a broken bucket hand-off would pass. +- In `linkCode.test.ts`, the case `#next=/account&code=${code}x` sits under "foreign", but it returns `''` because of the trailing `x`. The behaviour the plan asked about (a valid code followed by other parameters) is not pinned at all. +- The Postgres tests use `if testing.Short() { return }` instead of `t.Skip`, so `-short` runs report them as passed rather than skipped. +**Fix:** After the delete, wait for the commit and assert that `bucket.Exists(ctx, attach.BlobKey(logoDisk))` is false. Add `expect(readLinkCode('#code=' + code + '&next=/x')).toBe(code)`. Use `t.Skip("Postgres")`. + +### RC-IN-06: The legacy `MintAPIToken` fallback is process-global and is widened even when boot fails + +**File:** `U classes/api_token_scopes.go:125, 144-166`; `U plugin.go:127-146` +**Issue:** `CompileAPITokenScopes` adds every declared name to the process-wide `declaredScopes` set before `bootAPITokenScopes` checks the phrase keys. A boot that then fails on a missing key has already widened what `MintAPIToken` accepts. In a process with several applications (tests, or a future multi-app host), `MintAPIToken` also accepts another application's scopes. Both are documented, and production runs one app per process. +**Fix:** Add the names only after the phrase check passes (return the declared list from `CompileAPITokenScopes` and add it in `bootAPITokenScopes` just before `app.Publish`). Mark `MintAPIToken` as `Deprecated:` in favour of `MintAPITokenFor`. + +### RC-IN-07: sm-oauthserver-plugin builds only through its workspace file + +**File:** `O go.mod:6` (`sm-user-plugin v0.1.10`); `O go.work` (tracked, `replace ... => ../sm-demo-app/plugins/golem15/user`) +**Issue:** O now uses `userclasses.AccessRevokedEvent`, `FireSubjectEvent` and `FireSubjectEventAfterCommit`, which v0.1.10 does not have. `GOWORK=off go build ./...` fails with `undefined: userclasses.AccessRevokedEvent`. The planned release order (tag U, then bump O) covers this, but nothing enforces it. +**Fix:** Make the O release checklist require bumping `sm-user-plugin` to the new U tag and running a `GOWORK=off go build ./...` gate before tagging O. + +## Ruling: `readLinkCode` with other fragment parameters + +`readLinkCode('#code=<64 hex>&next=/x')` returns the code. **This is not a defect.** `URLSearchParams.get('code')` returns the first `code` parameter, and the value still has to match `^[a-f0-9]{64}$` exactly, so nothing beyond a well-formed code is ever accepted. Other parameters are ignored, the fragment is stripped right away by `replaceState` (`V link-admin.vue:55`), and the code is only ever sent in a JSON body. Accepting extra parameters is the more robust behaviour: an analytics tool or a redirect that appends a fragment parameter does not break the link. The plan's expectation of an empty result was wrong. The one gap is that no test pins the behaviour (RC-IN-05). + +## Earlier findings + +| ID | Verdict | Evidence | +|----|---------|----------| +| C-CR-01 (ban / password reset keep MCP grants) | partially fixed | Fixed for ban (`U controllers/users_admin_controller.go:651-683`), reset (`U controllers/api_controller.go:454-466`) and admin-set password (`U controllers/users_admin_controller.go:247-255`). The verifier refuses banned users and pre-cutoff tokens (`O classes/mcp_verifier.go:96-105`), and so does the refresh grant (`O classes/mcp_store.go:281-313`). `LinkedBackendUser` excludes banned users (`U classes/backend_links.go:437-443`). Not fixed for the self-service password change: `U controllers/api_controller.go:722-732` revokes only grants older than the session's `iat` (RC-CR-01). | +| C-CR-02 (fresh-sign-in proof defeated by refresh) | fixed | `U controllers/backend_link_controller.go:102-110` reads `auth_time` again from the accepted token and checks the subject; `U classes/backend_links.go:276` compares it with `created_at`; `F bouncer/refresh.go:141-142` carries `auth_time` forward; the page shows the signed-in email with "Not you?" (`V link-admin.vue:150-155`). One-second blind spot only (RC-IN-02). | +| C-WR-01 (link code in logged URLs) | fixed | URL is `…#code=` (`U account_contributions.go:120`); the page reads the hash, stores the code in `sessionStorage` and strips it (`V link-admin.vue:50-60`); show is a POST with a JSON body (`V link-admin.vue:89`, `U routes.go:48`); the sign-in link carries no code (`V link-admin.vue:24`). The server still accepts the code in URLs (deprecated GET route, query fallback in `readFields`): RC-IN-01. | +| C-WR-02 (`backend` grant re-targeted by a later link) | partially fixed | Issued access and refresh tokens carrying `backend` are revoked on link and unlink (`O classes/cascade.go:173-216`, `O plugin.go:103-105`). Pending consents, unexchanged MCP codes and a refresh rotation racing the revocation still bind `backend` to a later admin (RC-WR-01). | +| C-IN-01 (draft_post for read-only grants) | fixed | `J mcp.go:160-162` `Writes: true`; enforced by `F pier/filter.go:52`; `J mcp_prompt_writes_test.go` checks both grants. | +| C-IN-02 (one revoke ends every MCP session) | fixed | `U controllers/api_tokens.go:142` (TokenID); `O controllers/authorizations.go:122-126` (ClientID); `F pier/events.go:85-98` tells the two apart. | +| C-IN-03 (nginx 2 MiB < pier 4 MiB) | fixed | `D deploy/nginx/demo.summercms.io.conf:217`, gated by `D scripts/check-deploy.sh:254-256`. Over-cap bodies still get the HTML 413 (RC-IN-04). | +| C-IN-04 (no `last_used_ip` on MCP personal tokens) | fixed | `U classes/mcp_guard.go:172-176`; pier sets the client IP (`F pier/pier.go:254`). | +| C-IN-05 (dead and duplicated code) | fixed | `requestUsable` calls `IsUsableAt` (`U classes/backend_links.go:227-235`); `BackendLinksFor` is used by both callers (`U classes/backend_links.go:57-64`, `U account_contributions.go:57-59`, `U controllers/backend_link_controller.go:16-18`); O's subject helpers delegate to U (`O classes/subject_events.go:18-26`). | +| C-IN-06 (hard-coded link page path) | fixed | `golem15.user.backend_links.page_path` (`U classes/backend_links.go:85-104`, `U config/config.yaml:48-52`), tested in `U backend_link_show_test.go:94-109`. | + +--- + +_Reviewed: 2026-10-23T17:19:01Z_ +_Reviewer: Claude (msd-code-reviewer)_ +_Depth: deep_