docs(19.7): add code review report
This commit is contained in:
Jakub Zych
590
.planning/phases/19.7-generic-mcp-module/19.7-REVIEW.md
Normal file
590
.planning/phases/19.7-generic-mcp-module/19.7-REVIEW.md
Normal file
@@ -0,0 +1,590 @@
|
||||
---
|
||||
phase: 19.7-generic-mcp-module
|
||||
reviewed: 2026-10-23
|
||||
depth: deep
|
||||
lanes: [A framework MCP core, B cabana and admin SPA, C plugins and demo]
|
||||
files_reviewed: 128
|
||||
findings:
|
||||
critical: 3
|
||||
warning: 17
|
||||
info: 20
|
||||
total: 40
|
||||
status: issues_found
|
||||
---
|
||||
|
||||
# Phase 19.7 code review
|
||||
|
||||
Three deep review lanes run in parallel over the phase's changes (framework since 1bd91a4^, sm-user-plugin since 3828a65, sm-oauthserver-plugin since d31f8e7, sm-journal-plugin since ffa2c8f, sm-demo-app since a25444e^, vue-demo-app since f397df2). Finding ids carry the lane prefix (A-, B-, C-). Each lane's file list is in its section's source report.
|
||||
|
||||
| Lane | Critical | Warning | Info |
|
||||
|------|----------|---------|------|
|
||||
| A framework MCP core | 1 | 9 | 8 |
|
||||
| B cabana and admin SPA | 0 | 6 | 6 |
|
||||
| C plugins and demo | 2 | 2 | 6 |
|
||||
|
||||
# Lane A
|
||||
|
||||
|
||||
## Phase 19.7: Code Review Report (lane A, framework MCP core)
|
||||
|
||||
**Reviewed:** 2026-10-23
|
||||
**Depth:** deep
|
||||
**Files Reviewed:** 32
|
||||
**Status:** issues_found
|
||||
|
||||
### Summary
|
||||
|
||||
I reviewed pier (identity, catalog, dispatch, stateful session router, stateless path, stores, fan-out), the surf glue (`pier_on.go`, `MountStream`, `Router.Handler`/`compileHandlers`), the bouncer dispatch marker and registry, the pact MCP contracts, wristband's scope allow-list and cooler's `KeyPrefix`. I traced the call chains into the SDK (go-sdk v1.8.0 `streamable.go` and `server.go`), cabana (`ResolveBackend`, `ResolveBackendToken`, `ScopeLimitFor`, `Allows`, `refuseFrontendMarker`, `BackendUsers.FindByID`) and the user plugin's personal-token resolver.
|
||||
|
||||
The core identity model holds up. Every request resolves its own identity, the receiving middleware re-filters list and call for that identity, the binding check applies to POST, GET and DELETE, the marker stays in the context only, and the backend provider's per-method scope limit matches cabana's HTTP guard (a `methodOK == 0` token gets an empty limit, which `Allows` refuses). I found no identity or guard bypass.
|
||||
|
||||
The defects are in lifecycle and edge handling:
|
||||
|
||||
- Stateful local sessions are never reaped, so every abandoned session leaks an SDK server and its goroutines until the process restarts.
|
||||
- Dispatch hands routes path values that the HTTP mux can never produce (`..`, traversal inside `{x...}`, an empty `{id}`).
|
||||
- The per-binding cap has a TOCTOU race.
|
||||
- A session's or stateless server's tool set never widens when the same binding comes back with more scope.
|
||||
- On Redis, an instance that serves only stateless clients never subscribes to fan-out events, and stateless listen streams are never re-verified.
|
||||
- Global mutexes are held across guard and database I/O.
|
||||
- Raw database errors reach the MCP client.
|
||||
|
||||
I did not re-report items that are accepted or deferred in 19.7-SECURITY-REVIEW.md or deferred-items.md: superuser reach, descriptions, the `initialized` notification landing on another instance, the protocol-version context key, the MCP grant revoke list, the duplicate `*pier.Pier` publish and the 5 s link cache.
|
||||
|
||||
### Critical Issues
|
||||
|
||||
#### A-CR-01: Stateful local sessions are never expired or reaped (unbounded server and goroutine leak)
|
||||
|
||||
**File:** `modules/pier/session.go:50`, `:143-149`, `:289-315`; `modules/pier/store_memory.go:27-34`; `modules/pier/audit.go:67`
|
||||
**Issue:** Every session opened through `serveNew` (and every copy rehydrated in `lookup`) is stored in `sr.local` with a live `*mcp.Server`, a `StreamableServerTransport`, a connected `*mcp.ServerSession` (jsonrpc2 reader goroutines) and the `ss.Wait()` goroutine started in `connect`. A local copy is removed in only three cases: a DELETE, a revoked or deleted subject event, or a later request for the same id whose record has gone (`lookup` → `closeLocal`). pier serves the transport directly, so it bypasses `StreamableHTTPHandler`. In go-sdk v1.8.0 the idle close (`StreamableHTTPOptions.SessionTimeout`, `streamable.go:721`) exists only in that handler; the transport has no idle close of its own. The store forgets idle sessions (memory sweep, Redis PX TTL) but nothing closes the local copy. The documented `expired` log step (`audit.go:67`) is never emitted anywhere.
|
||||
**Failure scenario:** Most MCP clients exit without sending DELETE (a desktop client closed, a CLI agent run finishing). Each such session stays in memory with its goroutines for the life of the process. Once the record expires, `Count` drops, so the same binding can open `max_per_binding` new sessions every `idle_ttl`. One token can therefore grow memory and goroutines without bound (about 20 sessions per 30 min per binding with the defaults, multiplied by bindings), up to an OOM. The leaked sessions also stay in `sessionRouter.matching`, so every permission or link event recomputes `allowed(...)` (database-backed guard checks) for each dead session.
|
||||
**Fix:** Reap local copies whose record has expired. One option is a janitor at the store's TTL granularity:
|
||||
```go
|
||||
// in Mount, after p.sessions is built
|
||||
go p.sessions.reap(ctx, p.cfg.IdleTTL/2)
|
||||
|
||||
func (sr *sessionRouter) reap(ctx context.Context, every time.Duration) {
|
||||
t := time.NewTicker(every)
|
||||
defer t.Stop()
|
||||
for {
|
||||
select {
|
||||
case <-ctx.Done():
|
||||
return
|
||||
case <-t.C:
|
||||
sr.mu.Lock()
|
||||
ids := make([]string, 0, len(sr.local))
|
||||
for id := range sr.local {
|
||||
ids = append(ids, id)
|
||||
}
|
||||
sr.mu.Unlock()
|
||||
for _, id := range ids {
|
||||
if _, ok, err := sr.p.store.Get(ctx, id); err == nil && !ok {
|
||||
sr.closeLocal(id)
|
||||
logSession(logger(), "expired", id, "")
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
```
|
||||
A simpler alternative is a per-`localSession` `time.AfterFunc(IdleTTL)` that is reset in `lookup` and closes the session, mirroring the SDK's `sessionInfo.timer`. Add a test that a session idle past `idle_ttl` disappears from `sr.local` and its `ss.Wait()` returns.
|
||||
|
||||
### Warnings
|
||||
|
||||
#### A-WR-01: Dispatched path values bypass the mux's path cleaning (`..` and traversal inside `{x...}`)
|
||||
|
||||
**File:** `modules/pier/dispatch.go:96-104`, `:117-124`, `:181-183`
|
||||
**Issue:** A single-segment value is refused only when it contains `/`. A wildcard (`{p...}`) value is split on `/` and each part is `url.PathEscape`d, which leaves `.` and `..` unescaped. `req.SetPathValue` then hands the raw value to the compiled chain. Over HTTP, `net/http.ServeMux` cleans the path and redirects before any handler runs. I checked this: `GET /files/a/../../etc/passwd` → 307 `/etc/passwd`, and `GET /items/..` → 307 `/`. A route's `{id}` is therefore never `..` or `.`, and a `{p...}` never contains `..` segments or empty segments. `Router.Handler` serves the route's chain directly (no mux), so pier delivers values the route has never seen over HTTP. `T-19.7-03` states that the in-process chain runs "the same constraints … as an HTTP request", but the mux's cleaning invariant is not part of that chain.
|
||||
**Failure scenario:** A plugin declares a tool on `GET /_media/api/files/{path...}` whose handler does `os.Open(filepath.Join(root, r.PathValue("path")))`. That code is safe over HTTP because the mux cleans the path. An agent calls the tool with `{"path":"a/../../../etc/passwd"}`: `filepath.Join` resolves outside `root` and the route serves the file. For a single segment, `{"id":".."}` reaches a route that over HTTP could only see a real id.
|
||||
**Fix:** Refuse what the mux would never deliver:
|
||||
```go
|
||||
case argPath:
|
||||
s, err := scalarString(v)
|
||||
...
|
||||
wild := strings.Contains(t.pattern, "{"+f.key+"...}")
|
||||
for _, seg := range strings.Split(s, "/") {
|
||||
if seg == "" || seg == "." || seg == ".." {
|
||||
return toolError(fmt.Sprintf("pier: invalid arguments: %q must not contain empty, . or .. segments", f.name)), 0
|
||||
}
|
||||
}
|
||||
if !wild && strings.Contains(s, "/") { ... }
|
||||
```
|
||||
Alternatively, require `path.Clean("/"+s) == "/"+s`. Add a `TestDispatch` case for `..` and `a/../b`.
|
||||
|
||||
#### A-WR-02: An optional path field leaves `{key}` literal in the target and an empty path value
|
||||
|
||||
**File:** `modules/pier/catalog.go:213-235`; `modules/pier/dispatch.go:90-93`, `:117-124`, `:181-183`
|
||||
**Issue:** `compileTool` checks that every `{segment}` has a `path:` field, but not that the field is required. jsonschema-go v0.4.3 (`infer.go:342`) makes a field optional when it carries `omitempty` or `omitzero`. If the caller omits that field, `run` skips it (`if !ok { continue }`), the target keeps the literal `{id}` (`httptest.NewRequest` accepts it), and `SetPathValue(seg, pathValues[seg])` sets `""`. Over HTTP a `{id}` segment never matches an empty segment.
|
||||
**Failure scenario:** A plugin writes `ID string \`json:"id,omitempty" path:"id"\`` on `DELETE /api/items/{id}`. The agent omits `id`, and the route runs with `PathValue("id") == ""` and `URL.Path == "/api/items/{id}"`. A route that queries `WHERE slug = ?` with `""`, or that treats an empty id as "all", acts on data that no HTTP request could reach.
|
||||
**Fix:** At boot, refuse a path field that the schema does not list as required:
|
||||
```go
|
||||
for _, f := range fields {
|
||||
if f.kind == argPath && !slices.Contains(schema.Required, f.name) {
|
||||
return nil, fmt.Errorf("%s: path field %q must be required (drop omitempty/omitzero)", errPrefix, f.name)
|
||||
}
|
||||
}
|
||||
```
|
||||
Also refuse a missing segment value at runtime before building the request (defence in depth).
|
||||
|
||||
#### A-WR-03: `max_per_binding` is check-then-act (TOCTOU) and can be exceeded by concurrent initializes
|
||||
|
||||
**File:** `modules/pier/session.go:134-159`; `modules/pier/store_redis.go:41-52`, `:119-137`
|
||||
**Issue:** `serveNew` calls `store.Count`, then `connect`, then `store.Put`, with no reservation in between. N concurrent `initialize` POSTs from one binding all read `Count == k < max` and all `Put`. The Redis path is non-atomic as well (`SMEMBERS` plus `EXISTS` per member, then a separate `MULTI`).
|
||||
**Failure scenario:** A client (or one stolen token) sends 100 parallel initializes. All pass the cap of 20 and 100 sessions open, each with a local server (see A-CR-01). The cap is the per-binding control cited for T-19.7-18, but it only holds for serial requests. The inline throttle (120/min per IP) limits the rate, not concurrency.
|
||||
**Fix:** Make the reservation atomic in the store, for example `Reserve(ctx, binding, id, max, ttl) (bool, error)`. On Redis, use a Lua script: prune dead members, `SCARD`, and if below max `SADD` plus `SET` of a placeholder record. On memory, do the same under `m.mu`. Call it in place of `Count`/`Put`, and release on connect or initialize failure.
|
||||
|
||||
#### A-WR-04: A session's or stateless server's tool set never widens when the same binding returns with more scope
|
||||
|
||||
**File:** `modules/pier/stateless.go:87-93`; `modules/pier/session.go:276-278`, `:463-487`, `:516-530`
|
||||
**Issue:** `newServer` registers only the tools that `allowed(...)` grants the identity that built the server. On a later request with the same binding, `getServer` (stateless hit) and `lookup` (stateful) only overwrite `identity`. The server's tool set is recomputed only by a fan-out event (`handleEvent`). The receiving middleware can only filter `tools/list` down (`kept` is a subset of what the server holds). A tool that the new identity may call, but that the server never registered, is missing from the list, and `tools/call` passes pier's check and then fails in the SDK as an unknown tool.
|
||||
**Failure scenario:** An OAuth client first connects with `scope=read` (binding `oauth:<client>:<user>`). The stateless LRU builds the server without write tools. The user re-authorizes with `read write`; the binding is the same. Every following request hits the cached entry, which `MoveToFront` keeps alive, so the write tools never appear and calls to them fail until LRU eviction or an unrelated subject event. The same happens to a stateful session whose client refreshes into a wider grant, and when `backend` is added to a grant.
|
||||
**Fix:** On a hit, compare a fingerprint of the authorization-relevant parts of the identity (sorted scopes, frontend and backend subject ids, ceiling). When it changed, run the same diff as for an event:
|
||||
```go
|
||||
if e.fingerprint != fp(id) {
|
||||
tools, prompts := sp.p.catalog.allowed(r.Context(), id)
|
||||
applyDiff(e.server, sp.p, id, tools, prompts, e.tools, e.prompts)
|
||||
e.fingerprint = fp(id)
|
||||
}
|
||||
```
|
||||
Do the same in `lookup` under `ls.mu`. An alternative for the stateless path is to key the LRU by binding plus fingerprint.
|
||||
|
||||
#### A-WR-05: With the Redis store, an instance that serves only stateless clients never subscribes to fan-out events
|
||||
|
||||
**File:** `modules/pier/pier.go:140-167`; `modules/pier/store_redis.go:189-204`; `modules/pier/stateless.go:74-76`, `:146-174`; `modules/pier/events.go:13-27`
|
||||
**Issue:** The Redis store, and with it the `mcp:events` subscription (`rs.Subscribe` inside `open`), is created lazily by the first store call. Only the stateful router (`Count`/`Put`/`Get`/`Touch`/`Delete`) and a locally fired subject event (`listenEvents` → `Publish`) make that call. The stateless path (`route` → `statelessPath.ServeHTTP` → SDK) never touches the store.
|
||||
**Failure scenario:** In a two-instance Redis deployment, instance B receives only 2026-07-28 clients (or only `server/discover` traffic). An admin revokes a grant on instance A. A publishes `revoked`, but B never subscribed: its LRU entries for that subject are not evicted, their `subscriptions/listen` streams are not closed (see A-WR-06), and permission or link changes never send `list_changed` on B. Per-request bearer checks still refuse calls, but D-26 and D-27 fan-out is broken on B.
|
||||
**Fix:** Open the store and subscribe when the first request arrives on any path, for example `if ls, ok := p.store.(*lazyStore); ok { _, _ = ls.get(r.Context()) }` at the top of `route`. Better, start a background goroutine at mount that retries `open` with backoff, so the subscription exists before traffic arrives. Keep the 503 behaviour for stateful requests when the store is down.
|
||||
|
||||
#### A-WR-06: Stateless `subscriptions/listen` streams are never re-verified, and LRU eviction orphans them from revocation
|
||||
|
||||
**File:** `modules/pier/stateless.go:98-106`, `:128-140`; `modules/pier/session.go:369-392`
|
||||
**Issue:** The stateful GET stream re-resolves its bearer every `streamRecheckInterval` (Pitfall 10). The stateless path has no equivalent. In go-sdk v1.8.0 a 2026-07-28 `subscriptions/listen` is a long-lived POST (`shouldPropagateCancellation` with `isSubscriptionsListen`), so the only thing that ends it is `evict` on a `revoked` or `deleted` event. In addition, when `getServer` drops the oldest entry at capacity (`sp.order.Remove(last); delete(sp.entries, ...)`), it does not close that server's sessions. A later `evict` for that subject can then no longer find the entry.
|
||||
**Failure scenario:** A token expires (no event is fired), or a revocation event is missed (A-WR-05, or a go-redis pub/sub drop), or the identity's entry was LRU-evicted before the revocation. The client's listen stream stays open indefinitely and keeps receiving list-change notifications for an identity that no longer authenticates, which the stateful path explicitly prevents.
|
||||
**Fix:** When evicting for capacity, close the evicted server's sessions, as `evict` does:
|
||||
```go
|
||||
old := last.Value.(*statelessEntry)
|
||||
sp.order.Remove(last)
|
||||
delete(sp.entries, old.binding)
|
||||
go func() { for ss := range old.server.Sessions() { _ = ss.Close() } }()
|
||||
```
|
||||
Also wrap the stateless handler with the same periodic bearer re-check that `serveStream` uses when the request body is a `subscriptions/listen` (`peekMethods` is already available).
|
||||
|
||||
#### A-WR-07: Global mutexes are held across guard and database I/O, so one slow check stalls every session on the instance
|
||||
|
||||
**File:** `modules/pier/session.go:144-149`, `:247-275`; `modules/pier/stateless.go:85-107`
|
||||
**Issue:** `serveNew` and `lookup` hold `sr.mu` (the router-wide lock) while `connect` → `newServer` → `catalog.allowed` runs `MCPAllows` or `MCPAllowsMethod` for every catalog item. For the backend provider each call is a `cabana.BackendPrincipal` database query, and the user plugin's guard also queries. `getServer` holds `sp.mu` over the same work. `matching` (event fan-out) and every `lookup` need the same locks.
|
||||
**Failure scenario:** The database slows down (lock contention, pool exhaustion) during one client's initialize or rehydration. Every other MCP request on the instance (POSTs to existing local sessions, GET streams being established, DELETEs, subject events) blocks on `sr.mu` or `sp.mu` until that request's context ends. One client's slow guard check becomes an instance-wide stall of the endpoint.
|
||||
**Fix:** Build the server outside the lock and insert it under the lock with a re-check:
|
||||
```go
|
||||
ls, err := sr.connect(ctx, sid, id, state) // no lock held
|
||||
sr.mu.Lock()
|
||||
if cur := sr.local[sid]; cur != nil { sr.mu.Unlock(); _ = ls.ss.Close(); ls = cur } else { sr.local[sid] = ls; sr.mu.Unlock() }
|
||||
```
|
||||
Use the same pattern for `getServer` (build on miss without `sp.mu`, then insert or adopt the existing entry).
|
||||
|
||||
#### A-WR-08: Raw provider and database errors are returned to the MCP client as tool text
|
||||
|
||||
**File:** `modules/pier/dispatch.go:147-150`; `modules/cabana/auth.go:44-50` (callee)
|
||||
**Issue:** `MCPDispatch` errors become `toolError("pier: " + err.Error())`. For the built-in backend provider the error comes from `cabana.BackendPrincipal` → `BackendUsers.FindByID`, which returns the raw gorm or pgx error. Plugin providers (`MCPDispatch` in the user guard) also return raw database errors. pgx error strings include the server address, the database and user names, and SQL state details.
|
||||
**Failure scenario:** The database fails between the listing check (which only logs its error) and the dispatch, or a provider's `MCPDispatch` queries while its `MCPAllows` does not. The agent and its transcript then receive text such as `pier: failed to connect to user=summer database=summer: 10.x.x.x:5432 ... dial error`, which leaks infrastructure details to an outside client.
|
||||
**Fix:** Log the error server-side and return a fixed text:
|
||||
```go
|
||||
if err != nil {
|
||||
logger().Warn("pier: guard dispatch failed", "tool", t.name, "guard", t.guardName, "error", err)
|
||||
return toolError("pier: the tool could not be run for this caller"), 0
|
||||
}
|
||||
```
|
||||
Keep the specific "unknown or deactivated" message only for the provider's own sentinel errors.
|
||||
|
||||
#### A-WR-09: A personal-token resolver that leaves `TokenID` zero puts every user on one binding
|
||||
|
||||
**File:** `modules/pier/identity.go:229-245`; `modules/pact/mcp.go:237-244`
|
||||
**Issue:** The binding of a frontend personal token is `"pat:user:" + subject.TokenID`. `resolvePersonalToken` checks `UserID != 0` but not `TokenID`, and the `MCPPersonalTokenResolver` contract never says `TokenID` is required. The shipped user plugin sets it, but any other resolver that omits it maps every token of every user to `pat:user:0`.
|
||||
**Failure scenario:** With a third-party resolver and `mcp.allow_personal_tokens: true`, all personal-token users share one binding. The 20-session `max_per_binding` cap becomes global, so one user exhausts it for everyone (429). User B can DELETE user A's session (the binding check passes) when B learns the id. The stateless LRU serves one shared server whose `identity` (used for fan-out matching) flips between users.
|
||||
**Fix:** Refuse a subject without a token id, or include the user id in the binding:
|
||||
```go
|
||||
if err != nil || !ok || subject.UserID == 0 || subject.TokenID == 0 {
|
||||
return Identity{}, ErrUnauthorized
|
||||
}
|
||||
```
|
||||
Document in `pact.MCPPersonalTokenResolver` that `TokenID` must identify the token row.
|
||||
|
||||
### Info
|
||||
|
||||
#### A-IN-01: The GET stream re-check ignores a session whose record is gone
|
||||
|
||||
**File:** `modules/pier/session.go:381-387`; `modules/pier/store_redis.go:91-101`; `modules/pier/store_memory.go:68-79`
|
||||
**Issue:** Every minute the recheck calls `store.Touch` and ignores the result. `Touch` on a missing record returns `nil` in both stores. If the `ended` pub/sub event for a DELETE served on another instance is lost, the local GET stream outlives the ended session for as long as the bearer resolves.
|
||||
**Fix:** In the ticker branch, `if _, ok, err := sr.p.store.Get(ctx, ls.id); err == nil && !ok { cancel(); sr.closeLocal(ls.id); return }`.
|
||||
|
||||
#### A-IN-02: `KeyPrefix`'s doc comment swallowed `keyPrefix`'s
|
||||
|
||||
**File:** `modules/cooler/cooler.go:353-366`
|
||||
**Issue:** The new comment was inserted directly under `keyPrefix`'s existing comment block. `go doc cooler.KeyPrefix` now starts with "keyPrefix returns …", and `keyPrefix` has no doc.
|
||||
**Fix:** Move the original five lines down to sit directly above `func keyPrefix`.
|
||||
|
||||
#### A-IN-03: The `MCPPersonalTokenResolver` doc describes a resolver chain that pier does not have
|
||||
|
||||
**File:** `modules/pact/mcp.go:237-244`; `modules/pier/identity.go:224-232`
|
||||
**Issue:** The doc says "the boolean is false when the token's prefix is not the resolver's, so the next resolver is tried", but pier looks up one published resolver (`app.Lookup`) and treats `ok == false` as unauthorized. A second plugin cannot publish another one (duplicate provider).
|
||||
**Fix:** Reword the doc (one resolver per application; false means "not mine, unauthorized"), or support a registry like `MCPGuards`.
|
||||
|
||||
#### A-IN-04: Integer path and query arguments above 2^53 lose precision
|
||||
|
||||
**File:** `modules/pier/dispatch.go:256-271`, `:301-311`
|
||||
**Issue:** `scalarString` and `queryStrings` decode into `any`, which gives `float64`. An id of `9007199254740993` is forwarded as `9007199254740992`, so the call addresses a different record.
|
||||
**Fix:** Decode with `json.NewDecoder(bytes.NewReader(raw)).UseNumber()` and render `json.Number` as its string.
|
||||
|
||||
#### A-IN-05: The dispatched request carries the `httptest` Host `example.com` and no scheme
|
||||
|
||||
**File:** `modules/pier/dispatch.go:161-180`
|
||||
**Issue:** `httptest.NewRequestWithContext` sets `Host: example.com` and no TLS. pier copies the client address, X-Forwarded-For, X-Real-IP and Accept-Language, but not Host, X-Forwarded-Proto or X-Forwarded-Host. A route that builds absolute URLs from the request, or compares Host, behaves differently than over HTTP (wrong links in tool results).
|
||||
**Fix:** Copy the outer `r.Host` into `callInfo` and set `req.Host`. Copy `X-Forwarded-Proto` and `X-Forwarded-Host` like the other forwarding headers.
|
||||
|
||||
#### A-IN-06: Audit and lifecycle labels do not match what happens
|
||||
|
||||
**File:** `modules/pier/audit.go:67`; `modules/pier/session.go:171-192`, `:572-575`
|
||||
**Issue:** The `expired` session step is documented but never logged (see A-CR-01). Tool calls served by `serveEphemeral` (stateful path, no session id) are logged with session `stateless`.
|
||||
**Fix:** Emit `expired` from the reaper, and log ephemeral calls with a distinct label (for example `ephemeral`).
|
||||
|
||||
#### A-IN-07: Embedded-struct fields on a GET or DELETE tool are dropped silently
|
||||
|
||||
**File:** `modules/pier/catalog.go:236-242`, `:380-385`; `modules/pier/dispatch.go:129-136`
|
||||
**Issue:** `inputFields` skips anonymous fields without tags because "their promoted fields are body fields". The GET/DELETE body check only looks at `fields`, so an embedded struct on a GET tool passes boot. Its promoted properties are in the schema and validated, then never sent, because a GET has no body.
|
||||
**Fix:** In `inputFields`, recurse into embedded structs (classifying the promoted fields), or refuse an embedded field on GET/DELETE tools at boot.
|
||||
|
||||
#### A-IN-08: pier does not constrain the marker's guard list to the tool's guard
|
||||
|
||||
**File:** `modules/pier/dispatch.go:151-159`
|
||||
**Issue:** `disp.Guards` comes from the provider and is copied into the marker as is. A frontend provider that answers `Guards: ["backend"]` (a provider bug) would have its principal honoured by every `backend`-guarded middleware except cabana's (which checks `Principal.Backend` since c49983d). pier is the only dispatcher, so it is the cheapest place to enforce the invariant.
|
||||
**Fix:** Refuse the call if `t.guardName != BackendGuard && slices.Contains(disp.Guards, BackendGuard)`, or if `principal.Backend != (t.guardName == BackendGuard)`.
|
||||
|
||||
---
|
||||
|
||||
_Reviewed: 2026-10-23_
|
||||
_Reviewer: Claude (msd-code-reviewer)_
|
||||
_Depth: deep_
|
||||
|
||||
# Lane B
|
||||
|
||||
|
||||
## Phase 19.7 (lane B): Code Review Report
|
||||
|
||||
**Reviewed:** 2026-10-23
|
||||
**Depth:** deep
|
||||
**Files Reviewed:** 37
|
||||
**Status:** issues_found
|
||||
|
||||
### Summary
|
||||
|
||||
Lane B covers the cabana seams (dispatch marker in `ResolveBackend` and the `backend` middleware, `ResolveBackendToken` / `ScopeLimitFor` / `BackendPrincipal`, My account contributions, prompted record actions, subject events, OpenAPI) and the admin SPA side (ConfirmDialog prompt, RecordActions, AccountContributions, plus the keyboard-shortcut and remount work committed in the same range).
|
||||
|
||||
What I traced and found correct:
|
||||
- **Marker:** `refuseFrontendMarker` wraps the guard middleware that surf registers as `backend` (surf/router.go:685, `admin.Middleware`), so plugin routes are covered as well as the admin API. `ResolveBackend` applies the same Backend and scope checks.
|
||||
- **Subject events:** `fireSubjectChanged` runs after commit on every path that fires it. Save, Delete, RecordAction and BulkAction all go through `CRUDService.transaction`, which wraps `lagoon.Transaction`. On those paths the ctx carries the after-commit buffer, so a rollback fires nothing. Outside a transaction (token revoke) the event fires at once. The prior-state snapshots are taken at the right moment: `is_superuser` comes from a virtual field set in `FormBeforeUpdate` after the snapshot, and permissions/role are applied after `FormBeforeUpdate`.
|
||||
- **Prompt validation:** the value is trimmed, must be non-empty and is capped in runes. A payload on a non-prompted action is still refused. The value reaches `Run` unchanged through the Administrators `guardOutranked` wrapper.
|
||||
- **Account contributions:** the action route is protected by `interactiveOnly`, `requireAjax` and `allowAction`, with `Applies` checked again inside the transaction. `pier`'s per-request bearer re-verification makes a missed event only a stale tool list, never a reopened session.
|
||||
- **Response shapes:** the new `prompt` key on `RecordAction` is `omitempty`, so it only adds data.
|
||||
- **Vue:** nothing uses `v-html`, and every i18n key used exists in both `en` and `pl` phrasebooks.
|
||||
|
||||
No BLOCKERs. The defects below are:
|
||||
- two keyboard-shortcut bugs that lose user input or navigate away unexpectedly;
|
||||
- an unvalidated URL rendered into `href`;
|
||||
- an over-broad session kill on personal-token revoke;
|
||||
- a stale API contract;
|
||||
- an all-or-nothing contributions list that fails silently.
|
||||
|
||||
### Warnings
|
||||
|
||||
#### B-WR-01: Plain-key shortcuts fire while typing inside a plugin field type's shadow DOM ("?" is swallowed, Esc leaves the form)
|
||||
|
||||
**File:** `admin/src/app/hotkeys.ts:208-236` (check at `:213`, helper `:100-105`)
|
||||
**Issue:** `onPlainKey` is a `window` listener and decides "is the user typing?" with `isEditableTarget(event.target)`. Keyboard events are composed, so a keydown that starts in an `<input>` inside a shadow root reaches `window` with `event.target` retargeted to the shadow host (the plugin's custom element). `host.closest('input, textarea, select, [contenteditable]')` is null, so the check says "not editable".
|
||||
|
||||
The framework explicitly supports shadow DOM in plugin field types and widgets. `modules/cabana/README.md:185` says "any widget shadow DOM may read only these public variables", and the 10.1 research's reference element calls `attachShadow`.
|
||||
|
||||
Failure scenario (plugin field type with a shadow-root input on a FormView):
|
||||
- Typing `?` opens the shortcut help and calls `preventDefault()`, so the character is never inserted. The admin cannot type a question mark in that field.
|
||||
- Pressing Esc (for example to dismiss the plugin's own autocomplete) clicks the form's Cancel (`hotkey="escape"` in FormView.vue). A clean form navigates away immediately.
|
||||
- `/` focuses the list search when the page has one.
|
||||
|
||||
**Fix:** test the real origin of the event:
|
||||
```ts
|
||||
function originOf(event: Event): EventTarget | null {
|
||||
const path = typeof event.composedPath === 'function' ? event.composedPath() : []
|
||||
return path[0] ?? event.target
|
||||
}
|
||||
// in onPlainKey
|
||||
if (event.defaultPrevented || isEditableTarget(originOf(event))) {
|
||||
return
|
||||
}
|
||||
```
|
||||
Also treat any custom element (a tag name containing `-`) inside a form as editable for plain keys, for closed shadow roots, where `composedPath()` stops at the host.
|
||||
|
||||
#### B-WR-02: Ctrl/Cmd+S and Ctrl/Cmd+Enter save without the taglist's typed-but-uncommitted tag
|
||||
|
||||
**File:** `admin/src/app/hotkeys.ts:187-206` (`target.click()` at `:205`); `admin/src/components/form/fields/TaglistField.vue:73`, `:167`
|
||||
**Issue:** TaglistField commits the draft tag only on Enter (`onKeydown`) or on blur (`@blur="addDraft"`). With the mouse, pressing Save first blurs the input (on mousedown), so the draft is committed before the click handler reads `values`.
|
||||
|
||||
The shortcut path calls `target.click()` without moving focus, so no blur happens. `onModifierKey` also runs in the window capture phase and calls `stopPropagation()`, so for Ctrl+Enter the taglist's own Enter handler never runs either.
|
||||
|
||||
Failure scenario: the admin types `news` in a tags field and presses Ctrl+Enter (Save and close). The record is saved without the tag and the view navigates away, losing the input silently. Ctrl+S leaves `news` visible in the input after a "saved" toast, but the stored record does not have it. Any plugin field type that commits on the native `change` event (which fires on blur) has the same problem.
|
||||
|
||||
**Fix:** commit pending edits before clicking, the way a pointer click would:
|
||||
```ts
|
||||
const active = doc.activeElement as HTMLElement | null
|
||||
if (active && active !== target && typeof active.blur === 'function') {
|
||||
active.blur() // fires blur/change: taglist addDraft, native change
|
||||
}
|
||||
target.click()
|
||||
```
|
||||
Optionally restore focus after the save. Add a test that drafts a tag and presses Ctrl+S.
|
||||
|
||||
#### B-WR-03: An account action's URL goes into `href` and `window.open` with no scheme check (`javascript:` executes on the admin origin)
|
||||
|
||||
**File:** `admin/src/components/account/AccountContributions.vue:43-46`, `:103-111`; `modules/cabana/account_contributions.go:212`
|
||||
**Issue:** `AccountActionResult.URL` is passed through unchanged by the server (`URL: result.URL`) and the SPA:
|
||||
- binds it to `<a :href="links[section.name]">`;
|
||||
- calls `window.open(answer.url, '_blank', 'noopener')`.
|
||||
|
||||
Vue does not sanitize `href`. A `javascript:` (or `data:`) URL therefore runs script with the admin's session the moment the admin clicks the visible link. That is stored-XSS-equivalent in the admin if any plugin builds the URL from data it does not fully control, for example a configured site URL or a value from the linked frontend account.
|
||||
|
||||
T-19.7-08 records only "no v-html" and "noopener" as the mitigation, so this sink is not covered by the accepted risk.
|
||||
|
||||
**Fix:** allow only http(s) (and same-origin relative) URLs at the framework seam, and again in the SPA:
|
||||
```go
|
||||
// account_contributions.go, before WriteData
|
||||
if result.URL != "" {
|
||||
u, err := url.Parse(result.URL)
|
||||
if err != nil || (u.Scheme != "https" && u.Scheme != "http") {
|
||||
moduleLog.ErrorContext(ctx, "cabana: account contribution action answered a non-http URL", "section", item.section.Name, "action", action.Name)
|
||||
result.URL = ""
|
||||
}
|
||||
}
|
||||
```
|
||||
```ts
|
||||
const safe = /^https?:\/\//i.test(answer.url) ? answer.url : ''
|
||||
```
|
||||
|
||||
#### B-WR-04: Revoking any personal token ends every MCP session of the admin, including OAuth agent sessions of the linked frontend user
|
||||
|
||||
**File:** `modules/cabana/account_tokens.go:167`
|
||||
**Issue:** `fireSubjectChanged(..., pact.MCPSubjectRevoked, principal.ID)` names only the admin. `pier/events.go:42-50` closes and deletes every local session whose identity `has("backend", adminID)`. That includes:
|
||||
- sessions opened with other, still-valid `g15_` tokens;
|
||||
- every OAuth session whose linked backend subject is this admin, which also carries the frontend user's tools.
|
||||
|
||||
Stateless servers are evicted too. The comment says "A revoked token ends what it opened elsewhere", but the effect is per admin, not per token.
|
||||
|
||||
Failure scenario: an admin revokes an old CI token. Every running agent session of their linked frontend account (and of their other tokens) is torn down on all instances. Per-request bearer verification already refuses the revoked token on its next request, so the broad kill adds no security for the revoked token itself.
|
||||
|
||||
**Fix:** make the event token-specific. Add `TokenID uint` to `pact.MCPSubjectChanged` (additive), fire it with `row.ID`, and in `pier.handleEvent` close only sessions whose `Identity.Backend.TokenID == ev.TokenID && Identity.Backend.ClientID == ""` when `TokenID != 0`. Until then, drop the event here and rely on the per-request re-verify.
|
||||
|
||||
#### B-WR-05: The record action API contract still says payload is refused
|
||||
|
||||
**File:** `modules/cabana/admin_openapi.go:798`, `:808`, `:825-827`
|
||||
**Issue:** The changes in this phase make `POST /{vendor}/{plugin}/{controller}/{id}/actions/{action}` accept `payload` (a JSON string) for a prompted action, and validate it with a 422 on `payload`. Three annotations were not updated:
|
||||
- the `AdminRecordAction` description still says "The body must be {}: record_id, values and payload are all refused";
|
||||
- the body param is documented as "Empty object";
|
||||
- `AdminActionRequest.Payload` says "Toolbar and record routes refuse it."
|
||||
|
||||
These texts are generated into `admin/openapi/admin.json` and `admin/src/api/schema.d.ts`, so the published contract contradicts the server. The project rule (CLAUDE.md) requires docs to change with the API.
|
||||
|
||||
**Fix:**
|
||||
- Update the description to: "The body must be {} for an action without a prompt; an action whose record response carries `prompt` takes `{\"payload\": \"<text>\"}` (trimmed, non-empty, at most prompt.max_length runes; 422 on payload otherwise)."
|
||||
- Change the body param text and the `Payload` field comment to match.
|
||||
- Run `scripts/check-admin-openapi.sh`.
|
||||
|
||||
#### B-WR-06: One contributing plugin's Status/Applies error removes every My account section, and the SPA hides the failure
|
||||
|
||||
**File:** `modules/cabana/account_contributions.go:110-116`, `:126`; `admin/src/components/account/AccountContributions.vue:25-32`
|
||||
**Issue:** `accountContributionsList` answers 500 for the whole list as soon as any section's `Status` or any action's `Applies` returns an error. `load()` then sets `sections = []` on any non-2xx result (`result.data?.data ?? []`) with no toast.
|
||||
|
||||
Failure scenario: plugin A's status query fails (a missing table after a partial migration, or a timeout). Plugin B's unrelated section, such as the MCP account link with its Unlink button, disappears from My account with no message. The admin cannot reach B's actions until A is fixed.
|
||||
|
||||
**Fix:** isolate failures per section. Log the error, then either omit that section or send it with an empty status and an `error: true` flag. Keep the other sections. In the SPA, show a toast (`backend::lang.form.error_generic`) when the GET fails, instead of silently rendering nothing.
|
||||
|
||||
### Info
|
||||
|
||||
#### B-IN-01: My account confirm and stale texts talk about "this record"
|
||||
|
||||
**File:** `admin/src/components/account/AccountContributions.vue:52`, `:78`
|
||||
**Issue:** The default confirm uses `backend::lang.form.action_confirm` ("Run “:action” on this record?" / "Wykonać „:action” na tym rekordzie?"). A 409 shows `action_stale` ("This action no longer applies to this record. The page has been refreshed."). My account has no record, so both texts are wrong there.
|
||||
**Fix:** add `backend::lang.myaccount.action_confirm` and `backend::lang.myaccount.action_stale` (en and pl, plus `tests/fixtures/lang.json`) and use them here.
|
||||
|
||||
#### B-IN-02: One prompt refusal is not localized
|
||||
|
||||
**File:** `modules/cabana/actions.go:268`
|
||||
**Issue:** `"The payload must be a string."` is hard-coded English. The other two prompt refusals use phrase keys. RecordActions shows `details.payload[0]` as the toast, so a Polish admin can see English. It is reachable only by a non-SPA client, so impact is low.
|
||||
**Fix:** reuse `promptRequiredKey`, or add `backend::lang.form.action_prompt_invalid`.
|
||||
|
||||
#### B-IN-03: Plugin status text goes through `translateKey`, not `localizeDetail`
|
||||
|
||||
**File:** `modules/cabana/account_contributions.go:116`
|
||||
**Issue:** `Status` is typically dynamic text such as "Linked to jan@example.com". `translateKey` looks every string up as a phrase key. In development that logs a missing-key warning on every My account load, which `localizeDetail` (crud.go) exists to avoid.
|
||||
**Fix:** `view.Status = localizeDetail(ctx, tr, status)`.
|
||||
|
||||
#### B-IN-04: Failed `g15_` token attempts at `/mcp` are logged without the client address
|
||||
|
||||
**File:** `modules/cabana/token_resolver.go:31`, `:38`, `:43`, `:47`, `:51`, `:61`
|
||||
**Issue:** `ResolveBackendToken` takes no request, so every `admin.auth` line it writes has `method=""` and `remote=""`. Over HTTP, the same failures carry the remote address, which the audit relies on to trace token guessing.
|
||||
**Fix:** take the remote address (or an `*http.Request`) as an optional parameter. pier has the outer request when it resolves the bearer.
|
||||
|
||||
#### B-IN-05: A prompted action refused with 422 closes the dialog and discards the typed text
|
||||
|
||||
**File:** `admin/src/components/form/RecordActions.vue:288-315`, `:336-339`; `admin/src/components/ui/confirm.ts:50-67`
|
||||
**Issue:** `confirm()` always settles after the action, so a 422 on `payload` (for example over the server cap on a non-SPA-checked path, or a plugin's own ValidationError on `payload`) shows a toast and the input is gone. The pact doc says the admin refuses such a value "with a validation error on the input".
|
||||
**Fix:** let the action return `false` to keep the dialog open, show `detail` under the input (`aria-invalid`, `aria-describedby`), and keep `value`.
|
||||
|
||||
#### B-IN-06: `window.open` after an awaited POST can be blocked by popup blockers
|
||||
|
||||
**File:** `admin/src/components/account/AccountContributions.vue:45`
|
||||
**Issue:** The open runs after `await api.POST(...)` (and after the confirm dialog). That is outside the click's synchronous user-activation window in Safari, and in Chrome when the request is slow. The visible link (`:103-111`) is the fallback, so nothing is lost.
|
||||
**Fix:** none required. Optionally skip `window.open` and rely on the link, or open a blank window synchronously and set its `location` afterwards.
|
||||
|
||||
---
|
||||
|
||||
_Reviewed: 2026-10-23_
|
||||
_Reviewer: Claude (msd-code-reviewer)_
|
||||
_Depth: deep_
|
||||
|
||||
# Lane C
|
||||
|
||||
|
||||
## Phase 19.7: Code Review Report (lane C: plugins and demo)
|
||||
|
||||
**Reviewed:** 2026-10-23
|
||||
**Depth:** deep (call chains followed into summercms.go `bouncer`, `cabana`, `lagoon`, `pier`, `wristband`)
|
||||
**Files Reviewed:** 59
|
||||
**Status:** issues_found
|
||||
|
||||
Repository paths used below:
|
||||
- **U** = sm-user-plugin (`/media/nvme/dev/golem15/summercms.io/summercms/sm-demo-app/plugins/golem15/user`, baseline 3828a65)
|
||||
- **O** = sm-oauthserver-plugin (`/media/nvme/dev/golem15/summercms.io/summercms/sm-oauthserver-plugin`, baseline d31f8e7)
|
||||
- **J** = sm-journal-plugin (`/media/nvme/dev/golem15/summercms.io/summercms/sm-journal-plugin`, baseline ffa2c8f)
|
||||
- **D** = sm-demo-app (baseline a25444e^)
|
||||
- **V** = vue-demo-app (`sm-demo-app/vue-demo-app`, baseline f397df2)
|
||||
- **F** = summercms.go (framework, read for call chains only)
|
||||
|
||||
### Summary
|
||||
|
||||
These parts hold up under review: the link-request storage (hashed 256-bit code, row lock, one-shot `used_at`, 1:1 unique indexes plus the 23505 mapping), the consent intersection (granted = submitted ∩ requested, `backend` dropped without a link), the Journal `optionalBackendPrincipal` change (`cabana.ResolveBackend` reads only the bearer or the dispatch marker, never the admin cookie), the Vue pages (text interpolation only, no `v-html`; Nuxt 4.4.8 `navigateTo` refuses script protocols, and wristband DCR requires a hostname), and the migration and its rollback. No breaking API change was found in U, O or J. All additions are new methods, routes, events or hooks.
|
||||
|
||||
The review found four real defects, two of them in security controls the phase security review lists as "verified":
|
||||
|
||||
1. Banning a frontend user, or resetting their password, does not end their MCP grants. Through the account link, a banned user keeps acting as the linked administrator (C-CR-01).
|
||||
2. The "fresh sign-in" proof of the account link (D-05) compares the JWT `iat` with the request time. `POST /_user/api/v1/refresh` mints a new `iat` without any sign-in, so a long-lived session passes as fresh (C-CR-02).
|
||||
3. The link code is a bearer secret that links whoever confirms it. It travels in request paths and query strings that nginx logs, which contradicts the security review's "the link code is never logged" (C-WR-01).
|
||||
4. An MCP grant that carries `backend` is not bound to the administrator shown on the consent page. A later link to a different administrator re-targets the existing grant without new consent (C-WR-02).
|
||||
|
||||
Not re-reported: the missing self-service revoke of MCP grants (AR-19.7-03), superuser reach (T-19.7-34), the v0.4.4 pin gaps, and the other items in deferred-items.md.
|
||||
|
||||
### Narrative Findings (AI reviewer)
|
||||
|
||||
### Critical Issues
|
||||
|
||||
#### C-CR-01: Banned users and password-reset users keep their MCP grants, including the linked admin's backend reach
|
||||
|
||||
**File:** `O classes/mcp_verifier.go:89-95`; `U classes/backend_links.go:396-401`; `U classes/admin_actions.go:106-120` (RevokeUserAccess), `:182-212` (BanUsers); `U controllers/api_controller.go:449-456` (password reset)
|
||||
**Issue:** `MCPTokenVerifier.MCPVerifyToken` accepts a token whenever `userclasses.PrincipalByID` loads the user. `PrincipalByID` (`U classes/user_lookup.go:35-60`) checks only that the row exists and is not soft-deleted. It does not check the ban flag in `user_throttle`, and it does not compare the token's `created_at` with `users.tokens_valid_after`. The plugin's ban and revoke paths do not touch OAuth tokens:
|
||||
- `BanUsers` calls `RevokeUserAccess`, which stamps `tokens_valid_after` and revokes `user_api_tokens` only. It fires no event: no `DeactivatingEvent`, so oauthserver's `Cascade` never runs, and no `pact.MCPSubjectChanged`.
|
||||
- Password reset (`api_controller.go:453-455`) and password change (`:715`) only set `tokens_valid_after`.
|
||||
- `LinkedBackendUser` (`backend_links.go:396-401`) also ignores bans.
|
||||
|
||||
Pier has no other user-state check (`F pier/identity.go:162-199`). MCP refresh tokens (`O classes/mcp_store.go:275-311`) are rotated with no user check either, so the grant survives access-token expiry as well.
|
||||
|
||||
**Failure scenario:** An editor's frontend account E is linked to admin A and holds an MCP grant with `read write backend`. Staff ban E on the Users screen after abuse, or E's owner resets the password after a compromise. The attacker's agent keeps calling `journal_update_post` and `journal_delete_post` as admin A, and keeps refreshing its access token. The plugin documents the ban as ending "every session JWT ... and every personal API token", but the MCP grant is a session-equivalent credential that this phase newly made usable against real routes.
|
||||
|
||||
**Fix:** Make the verifier honour the same refusals as `/refresh` (`refreshSubjectRefused`). Also revoke on ban or reset, and fire the session-ending event:
|
||||
```go
|
||||
// O classes/mcp_verifier.go, after loading principal
|
||||
if principal.TokensValidAfter != (time.Time{}) && token.CreatedAt != nil &&
|
||||
token.CreatedAt.Before(principal.TokensValidAfter) {
|
||||
return pact.MCPGrant{}, pact.ErrMCPTokenInvalid
|
||||
}
|
||||
banned, _, err := userclasses.ThrottleStates(ctx, db, []uint{token.UserID})
|
||||
if err != nil {
|
||||
return pact.MCPGrant{}, err
|
||||
}
|
||||
if banned[token.UserID] {
|
||||
return pact.MCPGrant{}, pact.ErrMCPTokenInvalid
|
||||
}
|
||||
```
|
||||
In U, make `RevokeUserAccess` fire `MCPSubjectRevoked` after commit for each id, or add a `user.AccessRevoked` event that oauthserver's `Cascade` handles by revoking the user's access and refresh tokens. Add `AND NOT EXISTS (SELECT 1 FROM user_throttle t WHERE t.user_id = l.user_id AND t.is_banned)` to `LinkedBackendUser`. These are additive.
|
||||
|
||||
#### C-CR-02: The account-link "fresh sign-in" proof (D-05) is defeated by token refresh
|
||||
|
||||
**File:** `U classes/backend_links.go:239`; `U controllers/backend_link_controller.go:74-85`; `F modules/bouncer/refresh.go:137` with `F modules/bouncer/mint.go:48-60`; `U controllers/api_controller.go:240`; `V app/pages/link-admin.vue:86-91`
|
||||
**Issue:** `ConfirmLinkRequest` treats `iat >= request.created_at` as proof that the user signed in for this link. `POST /_user/api/v1/refresh` (`bouncer.Refresh` → `MintAudience`) mints a new token with `IssuedAt: now` from any token still inside the refresh window. The demo sets `refresh_ttl` to 43200 minutes (30 days), and refresh needs no password, no social login and no 2FA. Refreshing after the request was created therefore turns a weeks-old session into a "fresh" one. Clients do this automatically: the demo's own API client refreshes on any 401 (`V app/lib/api/http.ts:127-183`), and so does sm-bm-app's. The link page also never shows which frontend account is about to be linked; it shows only the admin's name.
|
||||
|
||||
The security review rows T-19.7-22 and cross-cutting finding 1 record this control as "verified". It is not: the property D-05 calls one-way ("a link created without proof ... cannot be told apart afterwards") does not hold.
|
||||
|
||||
**Failure scenario:** An administrator starts "Link frontend account" and opens `/link-admin?code=…` on a shared or family computer where the site is still signed in as someone else (X). X's session refreshes when any page gets a 401, or X's script calls `/refresh`. The admin clicks "Link the accounts", sees "Your account is now linked", and X's account now holds the admin's backend reach for every MCP client X connects with `backend`. Nothing on the page tells the admin that X, not they, is signed in.
|
||||
|
||||
**Fix:** Bind the proof to an actual authentication event that refresh carries forward unchanged:
|
||||
- Mint an `auth_time` claim at login, registration, social login and 2FA completion. Have `bouncer.refreshAudience` copy the existing `auth_time` into the rotated token instead of resetting it.
|
||||
- In `BackendLinkConfirm`, read `auth_time` (refuse when it is absent) and compare that with `req.CreatedAt`, not `iat`.
|
||||
|
||||
Until then, a smaller fix: record `last_login_at` per sign-in and require `auth_time`/`last_login_at >= created_at` together with a `jti` minted by that login. In `link-admin.vue`, show the signed-in account's email next to the confirm button ("You are signed in as …"), so the admin can catch the wrong account.
|
||||
|
||||
### Warnings
|
||||
|
||||
#### C-WR-01: The one-shot link code links whoever presents it, and it is written to nginx access logs
|
||||
|
||||
**File:** `U routes.go:45-46` and `U controllers/backend_link_controller.go:36-38` (`GET /_user/api/v1/backend-link/{code}`); `U account_contributions.go:122` (`/link-admin?code=<code>`); `V app/pages/link-admin.vue:15-16, 37`; `D deploy/nginx/demo.summercms.io.conf:96` (`location ^~ /_user/`, default access log)
|
||||
**Issue:** `ConfirmLinkRequest` binds the link to the frontend user who confirms. The code is the only proof of the admin's intent, so anyone who sees it within the TTL can sign in to their own account and link it to the admin. The code reaches the server in the URL path of the show call and in the query string of the SSR page load (`/link-admin?code=…`, proxied to Nuxt by `location /`). nginx's default `access_log` records both request lines, as do Nuxt/Nitro request logs and any log shipping. The security review (cross-cutting 6) says "The link code is never logged", which is wrong for these logs. Through `/login?next=/link-admin?code=…` the code also lands in browser history.
|
||||
**Failure scenario:** Someone with read access to the web server logs, or to a log pipeline or analytics collector, takes the code within 10 minutes. They sign in to their own frontend account and POST `/backend-link/confirm`. Their account is now linked to the admin, and a `backend` MCP grant acts as that admin. The admin only sees "link expired or used", and later "Linked to <someone else's email>" on My account.
|
||||
**Fix:**
|
||||
- Keep the code out of every URL the server sees: answer `…/link-admin#code=<code>` (the fragment is not sent or logged), have the page read `location.hash`, and change the show route to `POST /_user/api/v1/backend-link/show` with the code in the JSON body.
|
||||
- Optionally add `access_log off` or a redacting `log_format` for `/link-admin`.
|
||||
- For defence in depth, require the admin to approve on My account the frontend email that confirmed (two-sided confirmation) before the link becomes active.
|
||||
|
||||
#### C-WR-02: A `backend` MCP grant is not bound to the administrator the user consented to; a later link re-targets it silently
|
||||
|
||||
**File:** `O controllers/mcp_consent_api.go:133-145`; `U classes/backend_links.go:322-348` (`RemoveLink`), `:293-320` (`insertLink`); `F pier/identity.go:187-198`; `V i18n/locales/en.json` `connect.actAsAdmin`
|
||||
**Issue:** At consent the page states "It will also act as admin {name} ({login})", and `backend` is granted when some link exists. The grant stores only the scope string. Pier resolves whichever administrator is linked at each request. Unlink fires only `MCPSubjectLink`, which recomputes the tool list. The oauthserver plugin does not listen to `BackendUnlinkedEvent` or `BackendLinkedEvent`, so the token and refresh token keep `backend` after unlinking. When the user is later linked to a different administrator, by self-service or by a holder of `golem15.users.manage_backend_links` via `SetLink`, the old grant resumes with the new admin's permissions, and nobody was asked again. D-08 decided not to store the link in the grant. That decision covers revocation, but not this re-targeting.
|
||||
**Failure scenario:** User U connects an agent while linked to editor admin X (consent shows X). U is unlinked. Months later an administrator links U's email to superuser admin Y for onboarding (no proof needed on that path). U's long-lived agent, running on refresh tokens, now acts as superuser Y, although U only ever approved X.
|
||||
**Fix:** Pick one of these:
|
||||
- Have oauthserver listen to `*userclasses.BackendUnlinkedEvent` and revoke that user's MCP access and refresh tokens that carry `backend` (in the event's transaction), so a new link always needs a new consent.
|
||||
- Store the consented `backend_user_id` with the grant (access and refresh rows) and have the verifier expose it in `pact.MCPGrant`, so pier refuses the backend subject when the current link differs. This is an additive field.
|
||||
|
||||
### Info
|
||||
|
||||
#### C-IN-01: The `journal_draft_post` prompt is listed for read-only grants although it directs a write tool
|
||||
|
||||
**File:** `J mcp.go:154-165, 179-183`; `F pier/filter.go:48-58`
|
||||
**Issue:** `allowsPrompt` applies no read/write ceiling. A `read` grant sees `draft_post`, whose text tells the agent to call `journal_create_post`, a tool that grant does not have.
|
||||
**Fix:** Add an additive `Writes bool` (or `ReadOnly *bool`) to `pact.MCPPrompt` that the ceiling honours, and set it on `draft_post`.
|
||||
|
||||
#### C-IN-02: Revoking one personal token or one OIDC app ends every MCP session of the user
|
||||
|
||||
**File:** `U controllers/api_tokens.go:110-111`; `O controllers/authorizations.go:121-122`; `F pier/events.go:42-50`
|
||||
**Issue:** Both fire `MCPSubjectRevoked` for the whole user. Pier closes every session of that subject, including unrelated OAuth MCP sessions. Clients must re-initialize. Access is not widened, so this is only an availability issue.
|
||||
**Fix:** Fire the event only when the deleted token or client is the one a session is bound to, or carry a token or client id in the event and match on the session binding.
|
||||
|
||||
#### C-IN-03: nginx body cap (2 MiB) is below pier's 4 MiB MCP body cap
|
||||
|
||||
**File:** `D deploy/nginx/demo.summercms.io.conf:65` (server-level `client_max_body_size 2m`), `:213-226` (`location = /mcp`)
|
||||
**Issue:** A `tools/call` between 2 and 4 MiB (for example a long `journal_create_post` body) gets nginx's HTML 413 instead of pier's JSON-RPC error.
|
||||
**Fix:** Add `client_max_body_size 4m;` inside `location = /mcp`, and check it in `check-deploy.sh`.
|
||||
|
||||
#### C-IN-04: The personal-token MCP path does not stamp `last_used_ip`
|
||||
|
||||
**File:** `U classes/mcp_guard.go:169`, compared with `U middleware` `APITokenGuard` (`:71-78`)
|
||||
**Issue:** The `user.api_token` guard records `last_used_ip`. `MCPResolvePersonalToken` stamps only `last_used_at`, so a token used through `/mcp` (with `mcp.allow_personal_tokens` on) shows a stale IP on the tokens page.
|
||||
**Fix:** Pass the client IP through `ctx` (pier copies the client address onto the request) and stamp it as the guard does.
|
||||
|
||||
#### C-IN-05: Dead and duplicated code
|
||||
|
||||
**File:** `U models/backend_link_request.go:27` (`IsUsableAt`, never called; `classes/backend_links.go:193` `requestUsable` duplicates it); `U classes/subject_events.go` and `O classes/subject_events.go` (identical `FireSubjectChanged` and `FireSubjectChangedAfterCommit`); `U account_contributions.go:57-64` and `U controllers/backend_link_controller.go:15-22` (the same `backendLinks` lookup)
|
||||
**Fix:** Have `requestUsable` call `req.IsUsableAt` (keep the error split), and have oauthserver call `userclasses.FireSubjectChanged*` (it already imports U).
|
||||
|
||||
#### C-IN-06: The link page path is hard-coded for every consuming site
|
||||
|
||||
**File:** `U account_contributions.go:122`
|
||||
**Issue:** `golem15.user.frontend_url` sets only the origin. Every site using this core plugin must serve exactly `/link-admin`. A site with locale-prefixed routes, or an API-only `app.url` without `frontend_url`, sends admins to a 404.
|
||||
**Fix:** Add `golem15.user.backend_links.page_path` (default `/link-admin`). This is additive.
|
||||
|
||||
---
|
||||
|
||||
_Reviewed: 2026-10-23_
|
||||
_Reviewer: Claude (msd-code-reviewer)_
|
||||
_Depth: deep_
|
||||
Reference in New Issue
Block a user