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

docs(quick-261105-din): fix 32.1 CR-01, consent allow serialized with revoke

Browse Source
This commit is contained in:
Jakub Zych
2026-11-05 10:00:02 +01:00
parent 8c6b3171a8
commit 71dc0485ee
4 changed files with 347 additions and 9 deletions
Download Patch File Download Diff File Expand all files Collapse all files

224
.planning/quick/261105-din-fix-32-1-cr-01-serialize-mcp-consent-all/261105-din-PLAN.md Normal file
View File

@@ -0,0 +1,224 @@
---
phase: quick-261105-din
plan: 01
type: execute
wave: 1
depends_on: []
files_modified:
- /media/nvme/dev/golem15/summercms.io/summercms/sm-oauthserver-plugin/classes/mcp_backend_binding.go
- /media/nvme/dev/golem15/summercms.io/summercms/sm-oauthserver-plugin/classes/mcp_store.go
- /media/nvme/dev/golem15/summercms.io/summercms/sm-oauthserver-plugin/controllers/mcp_consent_api.go
- /media/nvme/dev/golem15/summercms.io/summercms/sm-oauthserver-plugin/mcp_consent_race_test.go
- /media/nvme/dev/golem15/summercms.io/summercms/sm-oauthserver-plugin/mcp_consent_smoke_test.go
- /media/nvme/dev/golem15/summercms.io/summercms/sm-oauthserver-plugin/README.md
- /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/.planning/phases/32.1-mcp-oauth-for-plugin-apps-declared-scopes-mcp-subject-on-con/32.1-REVIEW-DISPOSITION.md
- /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/.planning/phases/32.1-mcp-oauth-for-plugin-apps-declared-scopes-mcp-subject-on-con/32.1-SECURITY.md
autonomous: true
requirements:
- quick-261105-din
- CR-01
estimate:
tokens: 70000
raw_tokens: 70000
tasks: 3
confidence: low
must_haves:
truths:
- "A revoke (DELETE /oauth/authorizations/{client_id}) that lands while an MCP allow is in flight is never undone. The allow's code issue (MarkIssued + MarkConsented inside srv.IssueCode) and its consent write (TouchConsent or replaceMCPConsent) run in ONE transaction that first takes the user's exclusive grants lock (classes.LockUserGrants), the same lock revoke takes. A revoke therefore either precedes both writes, and the allow reads the post-revoke state, or follows both and deletes the issued code and the consent."
- "While another transaction holds the user's grants lock, POST /oauth/mcp/requests/{request} with decision allow does not answer and writes nothing. The pending row keeps its request_id with a NULL code_hash, and the consent row's updated_at is unchanged."
- "The silent re-grant path (a consent row already covers the request: consent_required false, TouchConsent) runs inside the same locked transaction. After allow then revoke, the pair has no consent row and no MCP code, the code from the allow does not exchange, and a fresh authorize shows consent_required true."
- "A consent write that fails rolls the code issue back. The answer is 500 server_error, the request stays pending (request_id set, code_hash NULL), the client's consented_at stays NULL, and once the failure is gone the same request id can be allowed and its code exchanged."
- "No change in summercms.go (framework). No exported plugin signature changes. The plugin only adds classes.WithinUserGrantsLock. The OIDC flow, revoke, cascade, code exchange and refresh code paths are untouched."
- "Both new tests failed on the code before the fix, for the reason CR-01 describes. go vet ./... and the full non-short plugin suite are green at the commit."
artifacts:
- path: /media/nvme/dev/golem15/summercms.io/summercms/sm-oauthserver-plugin/classes/mcp_backend_binding.go
provides: "exported WithinUserGrantsLock(ctx, db, userIDs, fn); unexported grantsTxKey and grantsTxFrom(ctx)"
- path: /media/nvme/dev/golem15/summercms.io/summercms/sm-oauthserver-plugin/classes/mcp_store.go
provides: "MCPStore.WithinTx runs fn on the grants-lock transaction carried in ctx instead of opening its own"
- path: /media/nvme/dev/golem15/summercms.io/summercms/sm-oauthserver-plugin/controllers/mcp_consent_api.go
provides: "MCPConsentDecide allow path: IssueCode + recordMCPConsent inside WithinUserGrantsLock; response written after commit"
- path: /media/nvme/dev/golem15/summercms.io/summercms/sm-oauthserver-plugin/mcp_consent_race_test.go
provides: "TestMCPConsentAllowSerializesWithRevoke, TestMCPConsentFailureRollsBackCode (testcontainers Postgres, skip under -short)"
key_links:
- from: "controllers/mcp_consent_api.go MCPConsentDecide"
to: "classes.WithinUserGrantsLock"
via: "allow path wraps srv.IssueCode(txCtx, ...) and recordMCPConsent(txCtx, tx, ...)"
pattern: "WithinUserGrantsLock"
- from: "wristband.Server.IssueCode (framework, unchanged)"
to: "classes.MCPStore.WithinTx"
via: "s.backend.WithinTx(ctx, ...) with the ctx WithinUserGrantsLock handed to fn (LazyMCPStore delegates to NewMCPStore(db).WithinTx)"
pattern: "grantsTxFrom"
---
<objective>
Fix 32.1 review finding CR-01 in sm-oauthserver-plugin: an in-flight MCP allow can recreate a consent the user just revoked.
Root cause (verified against current code): `MCPConsentDecide` (controllers/mcp_consent_api.go:227-241) calls `srv.IssueCode`, which commits `MarkIssued` + `MarkConsented` in its own `s.backend.WithinTx` transaction. Only after that does it write the consent through `recordMCPConsent` / `replaceMCPConsent` on the pool `db`, and neither step takes `classes.LockUserGrants`. Revoke (controllers/authorizations.go:138-165) takes that exclusive advisory lock, then deletes the consent and the pair's AuthCode, AccessToken, RefreshToken and MCPCode rows. A revoke that commits between IssueCode and the consent write is undone: `Take` finds no row, so `replaceMCPConsent` inserts one. The next authorize then answers `consent_required:false` and /connect auto-allows.
Chosen design (smallest correct; no framework change). wristband's `IssueCode` does all its DB work through `s.backend.WithinTx(ctx, fn)` with the caller's ctx, and on the MCP server that backend is the plugin's `classes.LazyMCPStore`, which delegates to `classes.MCPStore.WithinTx`. So the plugin can make IssueCode's writes join a caller-owned transaction through a plugin-private context key:
1. A new `classes.WithinUserGrantsLock(ctx, db, userIDs, fn)` opens a transaction. Its first statement takes `lockUserGrants` (exclusive, ascending ids). It then calls `fn(txCtx, tx)`, where txCtx carries tx under an unexported key.
2. `MCPStore.WithinTx` checks that key. When present, it runs fn on the carried tx without BEGIN or SAVEPOINT, and the owner commits or rolls back.
3. The allow path runs `srv.IssueCode(txCtx, ...)` and `recordMCPConsent(txCtx, tx, ...)` inside that function and writes the response only after the commit.
Rejected alternative: "consent + lock first in its own transaction, IssueCode after". A revoke landing between the two commits deletes the consent but cannot see the not-yet-issued code. That leaves a live code without a consent, which is worse.
Purpose: revoke is final again. The silent re-grant (`TouchConsent`) path and the ban/password/link cascade (`revokeUserGrants`, which takes the same exclusive lock) also stop racing the allow, because the whole allow unit now holds the lock.
Output: plugin code fix + two deterministic regression tests + plugin README, in one plugin commit; 32.1 disposition and security docs updated (the orchestrator commits the docs).
</objective>
<execution_context>
@~/.claude/msd-core/workflows/execute-plan.md
@~/.claude/msd-core/templates/summary.md
</execution_context>
<context>
@/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/CLAUDE.md
@/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/.planning/STATE.md
@/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/.planning/phases/32.1-mcp-oauth-for-plugin-apps-declared-scopes-mcp-subject-on-con/32.1-REVIEW.md
Repository facts the executor must respect:
- The plugin repo is /media/nvme/dev/golem15/summercms.io/summercms/sm-oauthserver-plugin (own git repo, branch master, HEAD d600e2c, latest tag v0.1.6). Its `go.work` has an UNRELATED uncommitted change (sm-user-plugin replace pointing at ../sm-demo-app/plugins/golem15/user). Do not stage, edit, revert or stash it. Tests run with it as it is. Never use `git stash` or `git add -A` / `git add .` in that repo.
- Framework code used (read-only, do not change): /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/modules/wristband/consent.go `IssueCode` (:78-105) calls `lookupOwnedPending` (its own `s.backend.WithinTx`, :134-) and then one `s.backend.WithinTx` holding `MarkIssued` + `MarkConsented`, always passing the caller's ctx. No other DB access.
- Interfaces in play: `wristband.Backend.WithinTx(ctx, fn func(wristband.Tx) error) error`; plugin `classes.MCPStore{db *gorm.DB}`, `mcpTx{db *gorm.DB; backendUserID uint}`; `classes.LazyMCPStore.WithinTx` → `NewMCPStore(db).WithinTx(ctx, fn)` (classes/lazy.go:62-71); `classes.LockUserGrants(ctx, db, userIDs []uint)` / unexported `lockUserGrants` (classes/mcp_backend_binding.go:89-111); `classes.ErrDatabaseUnavailable` (classes/lazy.go:15); `classes.NewStore(db).WithinOIDCTx` + `oidcTx.TouchConsent` (classes/oidc_store.go:48-55, :364-367, an UPDATE by id).
- Lock takers today: revoke (exclusive), cascade `revokeUserGrants` (exclusive, classes/cascade.go:108), code exchange `ByCodeHashForUpdate` and refresh `ByTokenHashForUpdate` (shared, classes/mcp_store.go:195, :336). Each takes the advisory lock before any row lock it holds.
- Model fields: `models.MCPCode.RequestID *string`, `CodeHash *string`, table `golem15_oauthserver_mcp_codes`; `models.Consent` table `golem15_oauthserver_consents` (UNIQUE user_id, client_id; FK users ON DELETE CASCADE); `models.Client.ConsentedAt *time.Time`.
- Test harness (package oauthserver_test): `bootOAuth(t, envs...)` boots a fresh per-test database (testmain_test.go CREATE DATABASE per boot) and skips under -short; `env.gdb` is the app's own *gorm.DB (no pool cap); helpers `registerMCPClient`, `user`, `sessionToken`, `mcpPending` (mcp_consent_smoke_test.go:62), `mcpPendingPKCE` + `exchangeMCPCode` (mcp_backend_binding_test.go:22, :46), `consentShow` / `consentDecide` (mcp_consent_smoke_test.go:86, :99), `jsonCall` (pages_smoke_test.go:240), `browser()` (harness_test.go:212), `consentScopes` / `issuedCode` (mcp_consent_test.go:216, :226), `sha256Hex` (import_cutover_smoke_test.go:26). These helpers call t.Fatal, so never call them from a goroutine. MCP env for every MCP test: SUMMER_GOLEM15__OAUTHSERVER__MCP__ENABLED=true and SUMMER_GOLEM15__OAUTHSERVER__MCP__RESOURCE=+mcpVerifierResource (copy from TestMCPConsent).
- Docker: the tests need Docker. If the socket is denied, run the same command through `sudo -u golem bash -c '...'` (CLAUDE.md). A skipped test is not a pass: every verify below greps for the explicit `--- PASS:` lines.
</context>
<tasks>
<task type="tracer" tdd="true">
<name>Task 1: Failing regression tests, then serialize the MCP allow with revoke under the user's grants lock</name>
<files>/media/nvme/dev/golem15/summercms.io/summercms/sm-oauthserver-plugin/mcp_consent_race_test.go, /media/nvme/dev/golem15/summercms.io/summercms/sm-oauthserver-plugin/mcp_consent_smoke_test.go, /media/nvme/dev/golem15/summercms.io/summercms/sm-oauthserver-plugin/classes/mcp_backend_binding.go, /media/nvme/dev/golem15/summercms.io/summercms/sm-oauthserver-plugin/classes/mcp_store.go, /media/nvme/dev/golem15/summercms.io/summercms/sm-oauthserver-plugin/controllers/mcp_consent_api.go</files>
<read_first>
- sm-oauthserver-plugin/controllers/mcp_consent_api.go (whole file; allow path :185-242, recordMCPConsent :348-364, replaceMCPConsent :366-390)
- sm-oauthserver-plugin/controllers/authorizations.go :136-165 (revoke: lock, SELECT consent FOR UPDATE, deletes)
- sm-oauthserver-plugin/classes/mcp_backend_binding.go :71-111 and classes/mcp_store.go :34-65 (WithinTx, mcpTx)
- sm-oauthserver-plugin/mcp_consent_test.go (style, helpers) and mcp_consent_smoke_test.go :25-107 (mcpConsentView, helpers)
- summercms.go/modules/wristband/consent.go :78-160 (IssueCode, lookupOwnedPending)
</read_first>
<precondition>In /media/nvme/dev/golem15/summercms.io/summercms/sm-oauthserver-plugin, `git status --short` shows only ` M go.work` and HEAD is d600e2c or a descendant that leaves controllers/mcp_consent_api.go and classes/mcp_store.go unchanged. `docker info` succeeds (else use sudo -u golem).</precondition>
<behavior>
- TestMCPConsentAllowSerializesWithRevoke. The user already holds a consent [read write], and the second request "read" (via mcpPendingPKCE) shows consent_required false, so this is the silent TouchConsent path. A separate transaction holds classes.LockUserGrants for the user. In that state the async allow does not answer, and pg_locks shows exactly one ungranted advisory waiter on the holder's lock. The pending row still has request_id = the id and a NULL code_hash, and the consent row's updated_at is unchanged. A real async DELETE /oauth/authorizations/{client} queues as the second waiter. After the holder rolls back, the allow answers 200 with a redirect_uri carrying a code, and the revoke answers 200 {"revoked":true}. Afterwards: no consent row for (user, client); zero MCP code rows with user_id = user and client_id = client; exchangeMCPCode of the allow's code is not 200 and carries an error; a fresh mcpPending(client, "read") shows consent_required true.
- TestMCPConsentFailureRollsBackCode. A fresh MCP client with no consent row, so the allow takes the replace path. A test-installed BEFORE INSERT OR UPDATE trigger on golem15_oauthserver_consents raises an exception. The allow of a "read" request (mcpPendingPKCE) answers 500 {"error":"server_error"}. The MCP code row for that request id still has request_id set and code_hash NULL, the client's consented_at is NULL, and no consent row exists. After the trigger is dropped, the same request id allows with 200, the consent row is [read], and exchangeMCPCode returns 200 with an access_token.
- On the pre-fix code, test 1 fails: the allow answers while the lock is held. Test 2 also fails: code_hash is set and the retry answers 404 request_not_found.
</behavior>
<action>
RED first: write the tests, run them, confirm both fail on the current code, and keep the failure output for the SUMMARY. Do not commit the red state, because CLAUDE.md requires `go test ./...` green at every commit.
Tests (new file mcp_consent_race_test.go, package oauthserver_test, imports the plugin's classes and models packages):
- In mcp_consent_smoke_test.go add the field ConsentRequired bool with json tag consent_required to mcpConsentView. The change is additive; no existing assertion changes.
- Add goroutine-safe helpers in the new file that never touch t. First, an async JSON call that builds the request like jsonCall (Accept and Content-Type application/json, Bearer token, env.issuer + path), sends it with env.browser(), and delivers status, body and error on a buffered channel. Second, a waiter counter that runs, on env.gdb (a different pool connection), a COUNT over pg_locks w where w.locktype is advisory and w.granted is false, and a granted advisory row h exists with the same classid, objid and objsubid and h.pid equal to the holder's backend pid. Third, a wait helper that polls the counter every 10 ms until it reaches n, with a 15 s deadline. While polling it selects on the in-flight result channels: a result arriving first fails with "allow answered <status> <body> while the user's grants lock was held", and the deadline fails with the current count.
- Holder: begin a transaction on env.gdb with t.Context(), call classes.LockUserGrants(ctx, holder, []uint{user.ID}), and read SELECT pg_backend_pid() inside it. Register a t.Cleanup that rolls it back and ignores the error when it is already rolled back.
- TestMCPConsentAllowSerializesWithRevoke follows the first behavior bullet in order. First allow: mcpPending(client, "read write") with scopes [read, write]; assert consentScopes is [read write]. Second request: mcpPendingPKCE(client, "read"). Assert consentShow is 200 with ConsentRequired false, which proves the silent path. Take the consent row's updated_at. Start the holder, start the async allow (decision allow, scopes [read]), and wait for 1 waiter. Assert the MCPCode row with request_id = id2 has a nil CodeHash and the consent updated_at is unchanged. Start the async DELETE /oauth/authorizations/{client} and wait for 2 waiters. Roll back the holder, then collect both results with a 30 s timeout per channel. Then run the final assertions. Postgres grants queued conflicting advisory requests in queue order, so the allow, which waited first, runs before the revoke. That makes the "revoke follows both" outcome deterministic.
- TestMCPConsentFailureRollsBackCode follows the second behavior bullet. Install the trigger with plain Exec on env.gdb: a plpgsql function that RAISEs EXCEPTION, and a BEFORE INSERT OR UPDATE FOR EACH ROW trigger on models.Consent{}.TableName(). The database is per test, so no WHEN clause or client-id interpolation is needed. A t.Cleanup drops the trigger and the function IF EXISTS. Drop them explicitly before the retry.
- Never log a code or token (32.1 rule: tests read code rows by sha256Hex, as issuedCode does).
GREEN: the fix, per the chosen design in the objective.
- classes/mcp_backend_binding.go: add an unexported context key type grantsTxKey and an unexported grantsTxFrom(ctx) returning (*gorm.DB, bool); it is nil-safe on ctx and returns false for a nil tx. Add the exported WithinUserGrantsLock(ctx context.Context, db *gorm.DB, userIDs []uint, fn func(ctx context.Context, tx *gorm.DB) error) error. A nil db returns ErrDatabaseUnavailable. Otherwise it runs db.WithContext(ctx).Transaction, the same style as revoke. Inside, the first statement is lockUserGrants(ctx, tx, userIDs), which is exclusive and in ascending id order. Then it calls fn with context.WithValue(ctx, grantsTxKey{}, tx) and tx. The doc comment must say all of the following. The lock is the exclusive one revoke and the cascade take. The ctx handed to fn makes MCPStore.WithinTx, and therefore wristband.Server.IssueCode on the MCP server, run on tx instead of opening its own transaction, so code issue and any other write in fn commit or roll back together. ctx and tx are valid only until fn returns. Everything in fn must use tx: a query on a second pool connection that takes the same user's grants lock would wait on this transaction for good. Exclusive rather than shared is deliberate. It matches the review's fix, also serializes two allows of the same pair, and costs only a few milliseconds of that one user's exchanges and refreshes. Record this choice in the SUMMARY.
- classes/mcp_store.go MCPStore.WithinTx: keep the nil-store check. Then, when grantsTxFrom(ctx) reports a transaction, return fn of a new mcpTx bound to that tx directly, with no nested Transaction call and no savepoint. An error from fn propagates, and the owner rolls the whole unit back. Otherwise keep today's own-transaction path. Update the doc comment to name WithinUserGrantsLock. Do not change mcpTx methods, lock takers, exchange or refresh.
- controllers/mcp_consent_api.go MCPConsentDecide: keep everything up to and including the computation of issueScopes, the offline flag and the WithGrantedOfflineAccess / WithConsentedBackend ctx values. Compute stored (issueScopes plus offline_access when granted) before the transaction. Replace the separate IssueCode call and recordMCPConsent call with one classes.WithinUserGrantsLock(ctx, db, []uint{userID}, ...) whose function does three things in order. It calls srv.IssueCode with the function's ctx, which already carries the backend and offline values because it derives from ctx, and returns its error unchanged. It then calls recordMCPConsent with the function's ctx and tx, returning its error. Finally it assigns the redirect target to a variable declared outside. After the transaction returns, call writeDecision(w, target, err). That keeps today's mapping: ErrPendingNotFound 404, ErrNoGrantableScopes 422, any other error 500 server_error, and nil 200 {redirect_uri}. The answer is written only after the commit. Leave the pre-lock reads (PendingRequest, mcpPendingClientID, linkedBackend and the 409 check) where they are, because IssueCode re-validates the pending request inside the lock through lookupOwnedPending.
- recordMCPConsent keeps its signature. Its db parameter now receives the locked tx. Rename the parameter to tx and update its doc to say it runs inside the caller's grants-lock transaction. TouchConsent decision: the touch branch needs no lock of its own, because an UPDATE by id cannot recreate a deleted row. Its covers-check read and the replace both must see the post-revoke state, and they now do because they run in the locked tx. Keep the classes.NewStore(tx).WithinOIDCTx call, which nests as a GORM savepoint on tx. replaceMCPConsent is unchanged apart from receiving tx. Update the MCPConsentDecide doc comment: the allow issues the code and records the consent in one transaction under the user's grants lock, so a revoke precedes or follows both, and a failed consent write leaves the request pending.
- Do not touch summercms.go, the OIDC decide path (controllers/consent_api.go / wristband oidc_consent.go), revoke, cascade or any exported signature. The only API addition is WithinUserGrantsLock.
</action>
<verify>
<automated>cd /media/nvme/dev/golem15/summercms.io/summercms/sm-oauthserver-plugin && go vet ./... && go test -count=1 -v -run 'TestMCPConsentAllowSerializesWithRevoke|TestMCPConsentFailureRollsBackCode|TestMCPConsent$|TestMCPConsentReplace|TestMCPConsentSmoke|TestMCPOfflineAccessUntick|TestMCPRevokeDeletesCode|TestMCPBackendBinding' . 2>&1 | grep -E -- '^(--- (PASS|FAIL|SKIP)|ok|FAIL|panic)'</automated>
</verify>
<done>The RED run, recorded in the SUMMARY, shows TestMCPConsentAllowSerializesWithRevoke failing with "allow answered ... while the user's grants lock was held". It shows TestMCPConsentFailureRollsBackCode failing on a non-NULL code_hash or on the 404 retry. After the fix, the verify output lists `--- PASS:` for both new tests and for TestMCPConsent, TestMCPConsentReplace, TestMCPConsentSmoke, TestMCPOfflineAccessUntick, TestMCPRevokeDeletesCode and TestMCPBackendBinding, with no FAIL or SKIP line, and `go vet ./...` is clean. Nothing is committed yet; Task 2 commits the unit together with the README.</done>
</task>
<task type="auto">
<name>Task 2: Plugin README, full suite, one plugin commit</name>
<files>/media/nvme/dev/golem15/summercms.io/summercms/sm-oauthserver-plugin/README.md</files>
<read_first>
- sm-oauthserver-plugin/README.md lines 30, 33, 78 and the Testing paragraph at line 205 (grep for "After the code is issued", "LockUserGrants(ctx, db, userIDs)" and "TestMCPBackendBinding pins")
</read_first>
<action>
README (the plugin README is the module README; CLAUDE.md requires an exported API change and its README in the same commit):
- Line 33, MCP consent passage: replace the sentence beginning "After the code is issued, a consent row that already covers the request is only touched" so it says the following. The allow runs in one transaction that first takes the user's grants lock (classes.WithinUserGrantsLock, the exclusive lock revoke and the grant cascade take). In it the code is issued (IssueCode, whose MCP store joins that transaction) and a consent row that already covers the request is only touched (TouchConsent); otherwise the stored scopes become the granted set. Keep the existing offline_access, replace-not-union and SaveConsent wording. Add one sentence: a revoke therefore lands before or after both writes, so it is never undone by an allow in flight, and a failed consent write leaves the request pending with no code.
- Line 30, authorized-apps passage: after the LockUserGrants sentence, add that an MCP allow takes the same lock, so a revoke and an allow of the same user never interleave.
- Line 78, classes API list: after LockUserGrants(ctx, db, userIDs), add WithinUserGrantsLock(ctx, db, userIDs, fn). Every identifier written must exist in the package.
- Testing paragraph: append one sentence naming TestMCPConsentAllowSerializesWithRevoke (with another transaction holding the user's grants lock, the allow waits and writes nothing, and a revoke queued behind it removes the code and the consent it then wrote) and TestMCPConsentFailureRollsBackCode (a failing consent write rolls the code back and leaves the request decidable).
Gates, in this order, in the plugin dir: `go vet ./...`, then `go test -short ./...`, then the full non-short `go test -count=1 ./...` (use sudo -u golem bash -c '...' if the Docker socket is denied). Fix any failure your change caused. A failure that also reproduces at HEAD without your change is not yours to fix: record it in the SUMMARY and stop for the orchestrator.
Commit in the plugin repo with explicit paths only, never go.work. Stage classes/mcp_backend_binding.go, classes/mcp_store.go, controllers/mcp_consent_api.go, mcp_consent_race_test.go, mcp_consent_smoke_test.go and README.md. Commit as one logical change with the message "fix(mcp): serialize the consent allow with revoke under the user grants lock". The body should say: CR-01 of the 32.1 review; IssueCode and the consent write now commit in one transaction holding LockUserGrants, so a revoke can no longer be undone by an allow in flight, and a failed consent write rolls the code back. No co-author or attribution trailer (user CLAUDE.md overrides). Do not push or tag. Afterwards `git status --short` must show only ` M go.work`. Record the short hash (git rev-parse --short HEAD) for Task 3.
</action>
<verify>
<automated>cd /media/nvme/dev/golem15/summercms.io/summercms/sm-oauthserver-plugin && go vet ./... && go test -short ./... && go test -count=1 ./... 2>&1 | tail -5 && git status --short && git log -1 --format='%h %s' && git show --stat --format= HEAD</automated>
</verify>
<done>go vet is clean, and both the -short and the full non-short suites print ok with no FAIL. HEAD in sm-oauthserver-plugin is the new fix commit touching exactly the six listed files, with no attribution trailer, unpushed and untagged. `git status --short` shows only ` M go.work`. The README names WithinUserGrantsLock and both new tests.</done>
</task>
<task type="auto">
<name>Task 3: Mark CR-01 fixed in the 32.1 disposition and security docs (edit only; the orchestrator commits)</name>
<files>/media/nvme/dev/golem15/summercms.io/summercms/summercms.go/.planning/phases/32.1-mcp-oauth-for-plugin-apps-declared-scopes-mcp-subject-on-con/32.1-REVIEW-DISPOSITION.md, /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/.planning/phases/32.1-mcp-oauth-for-plugin-apps-declared-scopes-mcp-subject-on-con/32.1-SECURITY.md</files>
<read_first>
- 32.1-REVIEW-DISPOSITION.md (whole file, 48 lines)
- 32.1-SECURITY.md lines 38-48 (register rows T-32.1-06, T-32.1-07) and 66-100 (Residual Risk, Sign-Off)
</read_first>
<action>
Use the plugin short hash from Task 2, called HASH below. Use Edit with scoped replacements, not Write.
32.1-REVIEW-DISPOSITION.md:
- Frontmatter: in the CR-01 finding entry, change disposition from open to fixed. Change the top-level open count from 7 to 6. Leave total, recorded and the other findings unchanged.
- Table: the CR-01 row becomes critical, fixed, with the Source cell "sm-oauthserver-plugin HASH (unreleased; ships with the next plugin tag)".
32.1-SECURITY.md:
- Residual Risk table, CR-01 row: keep the Finding, Severity and Touches cells. Replace the Gap cell with: "Fixed in sm-oauthserver-plugin HASH (2026-11-05, unreleased): `MCPConsentDecide` issues the code and writes the consent in one transaction under `classes.WithinUserGrantsLock` (exclusive `LockUserGrants`, the lock revoke takes); a failed consent write rolls the code back. Tests `TestMCPConsentAllowSerializesWithRevoke`, `TestMCPConsentFailureRollsBackCode`."
- The intro sentence under the Residual Risk heading: append "CR-01 has since been fixed (sm-oauthserver-plugin HASH)."
- Register: in T-32.1-06's evidence cell, change "Residual: CR-01, WR-03, WR-04" to "Residual: WR-03, WR-04 (CR-01 fixed in HASH)". In T-32.1-07's cell, change "Closed at L1 only — see CR-01" to "CR-01 fixed in HASH: the MCP allow takes the same lock". Keep both statuses as closed.
- Sign-Off: tick the checkbox and make it read "[x] CR-01 given a disposition in 32.1-REVIEW-DISPOSITION.md (fixed: sm-oauthserver-plugin HASH)". Change the Approval line to "verified 2026-11-05 (ASVS L1; CR-01 fixed in sm-oauthserver-plugin HASH, unreleased)".
- Do not change frontmatter status, threats_open or the audit trail table.
Do not commit and do not stage. The orchestrator makes the docs commit in summercms.go. Leave the other sessions' uncommitted files (.planning/STATE.md, state.json, G15OFFICE.md and the untracked phase dirs) untouched.
</action>
<verify>
<automated>cd /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/.planning/phases/32.1-mcp-oauth-for-plugin-apps-declared-scopes-mcp-subject-on-con && H=$(git -C /media/nvme/dev/golem15/summercms.io/summercms/sm-oauthserver-plugin rev-parse --short HEAD) && grep -n "^open: 6$" 32.1-REVIEW-DISPOSITION.md && grep -n "| CR-01 | critical | fixed | sm-oauthserver-plugin $H" 32.1-REVIEW-DISPOSITION.md && grep -c "$H" 32.1-SECURITY.md && grep -n "\[x\] CR-01 given a disposition" 32.1-SECURITY.md && git -C /media/nvme/dev/golem15/summercms.io/summercms/summercms.go status --short -- .planning/phases/32.1-mcp-oauth-for-plugin-apps-declared-scopes-mcp-subject-on-con</automated>
</verify>
<done>The disposition file shows CR-01 fixed with the plugin hash in both frontmatter and table, and open: 6. SECURITY.md carries the hash in the residual row, the intro, both register cells, the ticked sign-off box and the approval line (grep -c reports at least 6). Both files show as modified and unstaged in summercms.go, with no other file there touched by this task.</done>
</task>
</tasks>
<threat_model>
## Trust Boundaries
| Boundary | Description |
|----------|-------------|
| browser (signed-in user) → POST /oauth/mcp/requests/{request} | the allow decision; the consent page may auto-post it when consent_required is false |
| browser (signed-in user) → DELETE /oauth/authorizations/{client_id} | the revoke that must be final |
| plugin context key → wristband.Server.IssueCode (framework) | IssueCode passes the caller's ctx to the plugin store; the store joins the carried transaction |
## STRIDE Threat Register
| Threat ID | Category | Component | Severity | Disposition | Mitigation Plan |
|-----------|----------|-----------|----------|-------------|-----------------|
| T-261105-01 | Elevation of privilege | MCPConsentDecide allow path vs revoke (CR-01) | critical | mitigate | IssueCode (MarkIssued, MarkConsented) and recordMCPConsent run in one transaction opened by classes.WithinUserGrantsLock, whose first statement takes the exclusive per-user advisory lock revoke takes; MCPStore.WithinTx joins it via the unexported ctx key; proven by TestMCPConsentAllowSerializesWithRevoke |
| T-261105-02 | Tampering | partial allow (code issued, consent write fails) | high | mitigate | the consent error is returned from fn so the whole transaction, including MarkIssued and MarkConsented, rolls back; the answer is written only after the commit; proven by TestMCPConsentFailureRollsBackCode |
| T-261105-03 | Denial of service | grants lock contention / self-wait | medium | mitigate | lock is per user and held only for two UPDATEs and one upsert; IssueCode's store calls run on the joined tx (no second connection); WithinUserGrantsLock doc forbids pool access inside fn. No new lock cycle: every taker (revoke, cascade, exchange, refresh, allow) takes the advisory lock before any row lock it holds. The cascade's user-event transaction may hold a NO KEY UPDATE lock on the users row, and that is compatible with the KEY SHARE lock the consent insert's foreign key check takes |
| T-261105-04 | Information disclosure | transaction handle leaking through ctx | low | mitigate | key type unexported, set only by WithinUserGrantsLock, documented valid only inside fn; the handler answers with the outer ctx after the transaction returns |
| T-261105-SC | Tampering | npm/pip/cargo installs | low | accept | this task installs no packages and adds no dependency |
</threat_model>
<verification>
- Task 1 verify output: `--- PASS:` for TestMCPConsentAllowSerializesWithRevoke and TestMCPConsentFailureRollsBackCode plus the existing MCP consent, revoke and backend-binding tests; no SKIP (a skip means Docker did not run; rerun via sudo -u golem).
- The RED evidence for both new tests is recorded in the SUMMARY (output from the pre-fix run).
- Task 2: full non-short plugin suite green; one plugin commit with six explicit files; go.work still unstaged and modified; nothing pushed or tagged.
- No file under /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/modules changed (`git -C .../summercms.go status --short -- modules` is empty).
- Task 3: the disposition and security docs show CR-01 fixed with the plugin hash, uncommitted.
</verification>
<success_criteria>
- A revoke can no longer be undone by an MCP allow in flight: allow code issue + consent write are atomic and serialized with revoke (and with the grant cascade) under the user's exclusive grants lock.
- A failed consent write leaves no live code.
- Deterministic regression tests (lock holder + pg_locks waiter checks, DB trigger) that fail on the old code and pass on the new.
- No framework change and no breaking plugin API change; one plugin commit; 32.1 docs updated for the orchestrator's docs commit.
</success_criteria>
<output>
Create /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/.planning/quick/261105-din-fix-32-1-cr-01-serialize-mcp-consent-all/261105-din-SUMMARY.md when done. Include the plugin commit hash, the RED and GREEN test output lines, the exclusive-vs-shared lock choice, and a note for the orchestrator: wristband's OIDC decide path (oidc_consent.go:66-120, SaveConsent and mintCode in one WithinOIDCTx) is atomic but does not take LockUserGrants, so it may have a related revoke race. That is unverified, outside CR-01 and not changed here; it is a candidate follow-up.
</output>

114
.planning/quick/261105-din-fix-32-1-cr-01-serialize-mcp-consent-all/261105-din-SUMMARY.md Normal file
View File

@@ -0,0 +1,114 @@
---
phase: quick-261105-din
plan: 01
subsystem: sm-oauthserver-plugin (MCP consent)
status: complete
tags: [security, oauth, mcp, concurrency, CR-01]
requires: [32.1 MCP consent JSON, classes.LockUserGrants]
provides: [classes.WithinUserGrantsLock, atomic MCP allow serialized with revoke]
affects: [controllers.MCPConsentDecide, classes.MCPStore.WithinTx]
tech-stack:
added: []
patterns: [caller-owned transaction joined via an unexported context key]
key-files:
created:
- /media/nvme/dev/golem15/summercms.io/summercms/sm-oauthserver-plugin/mcp_consent_race_test.go
modified:
- /media/nvme/dev/golem15/summercms.io/summercms/sm-oauthserver-plugin/classes/mcp_backend_binding.go
- /media/nvme/dev/golem15/summercms.io/summercms/sm-oauthserver-plugin/classes/mcp_store.go
- /media/nvme/dev/golem15/summercms.io/summercms/sm-oauthserver-plugin/controllers/mcp_consent_api.go
- /media/nvme/dev/golem15/summercms.io/summercms/sm-oauthserver-plugin/mcp_consent_smoke_test.go
- /media/nvme/dev/golem15/summercms.io/summercms/sm-oauthserver-plugin/README.md
- .planning/phases/32.1-mcp-oauth-for-plugin-apps-declared-scopes-mcp-subject-on-con/32.1-REVIEW-DISPOSITION.md
- .planning/phases/32.1-mcp-oauth-for-plugin-apps-declared-scopes-mcp-subject-on-con/32.1-SECURITY.md
decisions:
- "The MCP allow takes the user's grants lock exclusively (not shared), which matches the review's fix, also serializes two allows of the same pair, and costs only a few ms of that user's exchanges and refreshes"
- "IssueCode joins the caller's transaction through an unexported ctx key read by MCPStore.WithinTx, so there is no framework change"
metrics:
completed: 2026-11-05
actuals:
tokens: 11961
tasks: 3
commits: 1
plugin_commit: 21f5985
plan_head_before: d600e2c
plan_head_after: 21f5985
---
# Quick 261105-din: Fix 32.1 CR-01, serialize the MCP consent allow with revoke
**What changed:** the MCP consent allow now issues the code and writes the consent in a single Postgres transaction. Its first statement takes the user's exclusive grants advisory lock, which is the same lock revoke takes. A revoke therefore lands before both writes or after both, and can no longer be undone by an allow in flight. If the consent write fails, the code issue rolls back with it.
**Plugin commit:** `21f5985` in sm-oauthserver-plugin, on master: "fix(mcp): serialize the consent allow with revoke under the user grants lock". It has no co-author trailer and is not pushed or tagged. After the commit, `git status --short` shows only ` M go.work`, the unrelated change that was already there.
## What was built
- `classes.WithinUserGrantsLock(ctx, db, userIDs, fn)` is new in `classes/mcp_backend_binding.go`. It opens a transaction, takes `lockUserGrants` first (exclusive, in ascending id order), then calls `fn(txCtx, tx)`. `txCtx` carries `tx` under the unexported `grantsTxKey`. `grantsTxFrom(ctx)` is nil-safe. A nil db returns `ErrDatabaseUnavailable`.
- `MCPStore.WithinTx`: when ctx carries a grants-lock transaction, `fn` runs on it directly, with no nested transaction and no savepoint. Every other case keeps the store's own transaction. `LazyMCPStore` delegates here, so `wristband.Server.IssueCode` (`lookupOwnedPending`, `MarkIssued`, `MarkConsented`) joins the caller's transaction with no framework change.
- `MCPConsentDecide` allow path: one `WithinUserGrantsLock` runs `srv.IssueCode(txCtx, ...)` and then `recordMCPConsent(txCtx, tx, ...)`. The handler calls `writeDecision` only after the commit, so the status mapping is unchanged (404, 422, 500, 200). The reads before the lock stay where they were, because IssueCode checks the pending request again inside the lock.
- `recordMCPConsent` keeps its signature; its parameter was renamed to `tx`. Its covers-check, `TouchConsent` (nested `WithinOIDCTx`, which becomes a GORM savepoint) and `replaceMCPConsent` all run inside the locked transaction.
- README: the MCP consent passage, the authorized-apps passage, the `classes` API list (`WithinUserGrantsLock(ctx, db, userIDs, fn)`) and the Testing paragraph, which now names both new tests.
- 32.1 docs, edited but not committed: in `32.1-REVIEW-DISPOSITION.md`, CR-01 is `fixed` (`open: 6`) and its table row cites `21f5985`. In `32.1-SECURITY.md`, the residual row, intro, T-32.1-06 and T-32.1-07 cells, sign-off checkbox and approval line now cite `21f5985`.
## Lock choice: exclusive vs shared
The allow takes the exclusive lock, the one revoke and the cascade (`revokeUserGrants`) take. A shared lock would also have serialized the allow against revoke. Exclusive was chosen deliberately for three reasons:
- It matches the review's proposed fix.
- It serializes two allows of the same pair as well.
- It costs only a few milliseconds of that one user's code exchanges and refresh rotations, which take the lock shared while the allow holds it.
The lock order has no new cycle: every lock taker takes the advisory lock before any row lock.
## TDD evidence
**RED, pre-fix code (tests written first; nothing committed in the red state):**
```
mcp_consent_race_test.go:189: allow answered 200 (redirect withheld: it carries a code) while the user's grants lock was held (err <nil>)
--- FAIL: TestMCPConsentAllowSerializesWithRevoke (0.56s)
mcp_consent_race_test.go:301: after the failed consent write: code issued true, request id kept false
--- FAIL: TestMCPConsentFailureRollsBackCode (0.43s)
FAIL
```
Test 1 failed for the CR-01 reason: the allow answered while another transaction held the lock. Test 2 failed because the code was issued (`code_hash` set, `request_id` cleared by `MarkIssued`) even though the consent write failed. The test stops at that assertion, before reaching the 404 retry.
**GREEN, after the fix (Task 1 verify, Docker-backed and not skipped):**
```
--- PASS: TestMCPBackendBinding (1.15s)
--- PASS: TestMCPConsentAllowSerializesWithRevoke (0.67s)
--- PASS: TestMCPConsentFailureRollsBackCode (0.45s)
--- PASS: TestMCPConsentSmoke (0.78s)
--- PASS: TestMCPConsent (0.65s)
--- PASS: TestMCPConsentReplace (0.56s)
--- PASS: TestMCPOfflineAccessUntick (0.73s)
--- PASS: TestMCPRevokeDeletesCode (0.60s)
ok git.golem15.com/golem15/sm-oauthserver-plugin 10.351s
```
I also ran the two new tests with `-count=10`; all ten runs passed (ok, 15.97s), so they are stable.
**Plugin gates before the commit:** `go vet ./...` is clean. `go test -short ./...` passes (ok, 1.45s). The full non-short `go test -count=1 ./...` passes (ok, 96.35s). Docker was available directly, so `sudo -u golem` was not needed.
## Deviations from Plan
1. **[Rule 2 - Security] The early-answer failure message withholds the body on a 200.** The first RED run printed the allow's redirect body, which contains an authorization code. That breaks the 32.1 rule that tests never log a code. `waitForGrantsWaiters` now prints `(redirect withheld: it carries a code)` in place of a 200 body; any other status still prints its body. The plan's message shape, "allow answered <status> ... while the user's grants lock was held", is unchanged. The recorded RED output above comes from the redacted rerun. The leaked code belonged to a throwaway per-test database and was only ever printed to the local terminal.
2. **Test 2 reads the code row by client id, not by request id.** `MarkIssued` sets `request_id` to NULL, so on the pre-fix code a lookup by request id finds nothing. The test reads the client's single MCP code row and asserts `request_id == id` and `code_hash IS NULL`. It checks the same thing and gives a clearer RED message.
3. **The plugin commit is on the plugin repo's `master`.** The orchestrator's constraints and the plan say to commit there, and that is the project's normal workflow for sibling plugin repos. The executor's protected-branch halt does not apply.
## Follow-up flag for the orchestrator
wristband's OIDC decide path (`modules/wristband/oidc_consent.go:66-120`) runs `SaveConsent` and `mintCode` in one `WithinOIDCTx`, so it is atomic. It does not take `LockUserGrants`, though, so a revoke landing during an OIDC allow may race in a similar way. This is unverified and outside CR-01; nothing was changed there. It is a candidate follow-up.
## Known Stubs
None.
## Threat Flags
None. No new endpoint or trust boundary was added. The transaction handle in ctx is unexported, is set only by `WithinUserGrantsLock`, and is valid only inside `fn` (T-261105-04).
## Self-Check: PASSED
- FOUND: sm-oauthserver-plugin/mcp_consent_race_test.go
- FOUND: commit 21f5985 in sm-oauthserver-plugin (6 files, no attribution trailer)
- FOUND: 32.1-REVIEW-DISPOSITION.md with `open: 6` and the CR-01 row fixed at 21f5985; 32.1-SECURITY.md has 6 references to 21f5985 and the ticked sign-off box
- `git -C summercms.go status --short -- modules` is empty (no framework change)