From 5aa846c5e85368b534e03a1d19f441bb34de193c Mon Sep 17 00:00:00 2001 From: Jakub Zych Date: Thu, 5 Nov 2026 10:42:26 +0100 Subject: [PATCH] docs(quick-261105-eab): OIDC grant flows serialized with revoke --- .../32.1-SECURITY.md | 1 + .../261105-eab-PLAN.md | 280 ++++++++++++++++++ .../261105-eab-SUMMARY.md | 133 +++++++++ 3 files changed, 414 insertions(+) create mode 100644 .planning/quick/261105-eab-fix-oidc-consent-decide-race-with-revoke/261105-eab-PLAN.md create mode 100644 .planning/quick/261105-eab-fix-oidc-consent-decide-race-with-revoke/261105-eab-SUMMARY.md diff --git a/.planning/phases/32.1-mcp-oauth-for-plugin-apps-declared-scopes-mcp-subject-on-con/32.1-SECURITY.md b/.planning/phases/32.1-mcp-oauth-for-plugin-apps-declared-scopes-mcp-subject-on-con/32.1-SECURITY.md index 33d0540c..90389589 100644 --- a/.planning/phases/32.1-mcp-oauth-for-plugin-apps-declared-scopes-mcp-subject-on-con/32.1-SECURITY.md +++ b/.planning/phases/32.1-mcp-oauth-for-plugin-apps-declared-scopes-mcp-subject-on-con/32.1-SECURITY.md @@ -73,6 +73,7 @@ created: "2026-11-05" | Finding | Severity | Touches | Gap | |---------|----------|---------|-----| | CR-01 | critical | T-32.1-07, T-32.1-06 | Fixed in sm-oauthserver-plugin 21f5985 (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`. | +| CR-01 sibling (OIDC) | high | T-32.1-07 | Fixed in sm-oauthserver-plugin 7b9899f (2026-11-05, unreleased): the OIDC decide was already consistent (consent upsert and code in one transaction, ordered against revoke by the consent row lock; pinned by `TestOIDCConsentAllowThenRevoke`), but a silent resume could issue a code after a revoke, a refresh rotation's successor tokens survived a revoke and a code exchange could deadlock with one. The OIDC store now takes the user's grants lock (shared) before its first grant row. Tests `TestOIDCResumeSerializesWithRevoke`, `TestOIDCRefreshSerializesWithRevoke`, `TestOIDCExchangeSerializesWithRevoke`. | | WR-01, WR-02 | warning | T-32.1-05 | `mcpEffectiveScopes` re-implies a declared scope the user cleared on /connect while read/write remains, and does not intersect the catalog with `golem15.oauthserver.mcp_scopes`. | | WR-04 | warning | T-32.1-06 | narrowing a consent does not revoke refresh tokens carrying the removed scopes or offline_access. | | WR-03 | warning | T-32.1-06 | the consent backfill seeds from tokens the verifier would refuse (nil-expiry, inactive/revoked clients, rotated refresh tokens). | diff --git a/.planning/quick/261105-eab-fix-oidc-consent-decide-race-with-revoke/261105-eab-PLAN.md b/.planning/quick/261105-eab-fix-oidc-consent-decide-race-with-revoke/261105-eab-PLAN.md new file mode 100644 index 00000000..fce29f6e --- /dev/null +++ b/.planning/quick/261105-eab-fix-oidc-consent-decide-race-with-revoke/261105-eab-PLAN.md @@ -0,0 +1,280 @@ +--- +phase: quick-261105-eab +plan: 01 +type: execute +wave: 1 +depends_on: [] +files_modified: + - /media/nvme/dev/golem15/summercms.io/summercms/sm-oauthserver-plugin/oidc_grants_race_test.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/classes/oidc_store.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/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-SECURITY.md +autonomous: true +requirements: + - quick-261105-eab + +estimate: + tokens: 90000 + raw_tokens: 90000 + tasks: 3 + confidence: low + +must_haves: + truths: + - "Silent resume vs revoke: if a revoke lands while GET /oauth/authorize/resume/{request} is in flight for the same user and client (the consent covers the request), then once both have answered there is no consent row and no unconsumed auth code for that user and client, and the code the resume issued does not exchange (400)." + - "Refresh vs revoke: if a revoke lands while an OIDC refresh rotation of the same pair is in flight, then once both have answered (refresh 200, revoke 200) the pair has zero access-token and zero refresh-token rows. The new refresh token answers invalid_grant and the new access token gets 401 at /oauth/userinfo." + - "Code exchange vs revoke: if a revoke lands while an OIDC code exchange of the same pair is in flight, both answer 200 with no deadlock and no 500, and afterwards the pair has no tokens." + - "Consent decision vs revoke (pinning): if a revoke lands while an OIDC allow is blocked just before it issues its code, the allow answers 302 with a code and the revoke answers 200. Afterwards there is no consent row, no live code and no token for the pair. This test already passes on the code before the fix, which is the evidence that the decide path was consistent." + - "The plugin's OIDC store (classes/oidc_store.go oidcTx) takes the user's grants lock in shared mode before it takes the first row lock or foreign-key lock on that user's grant rows. SubjectFor, ConsentFor and SaveConsent take it on entry. ConsumeCode and RefreshTokenByHashForUpdate take it after they read the row's user_id by hash. Revoke and the grant cascade take this lock exclusively." + - "summercms.go does not change (nothing under modules/), so no framework tag is needed. No exported plugin identifier is added or changed, and the MCP code paths are untouched. go vet ./... and the full non-short plugin suite are green at the commit. Nothing blocks the plugin's v0.1.7 tag: go.mod still requires summercms v0.6.0." + artifacts: + - path: /media/nvme/dev/golem15/summercms.io/summercms/sm-oauthserver-plugin/oidc_grants_race_test.go + provides: "TestOIDCResumeSerializesWithRevoke, TestOIDCRefreshSerializesWithRevoke, TestOIDCExchangeSerializesWithRevoke (red before the fix), TestOIDCConsentAllowThenRevoke (pinning); goroutine-safe asyncDo, lock-waiter polling, gate trigger and pair-count helpers" + - path: /media/nvme/dev/golem15/summercms.io/summercms/sm-oauthserver-plugin/classes/oidc_store.go + provides: "oidcTx.SubjectFor, ConsentFor, SaveConsent, ConsumeCode, RefreshTokenByHashForUpdate take lockUserGrantsShared first; Store doc states the invariant" + key_links: + - from: "wristband.Server.Resume / decideConsent / redeemCode / rotateOIDCRefresh / UserInfo (framework, unchanged)" + to: "classes.oidcTx (plugin) via classes.LazyStore -> Store.WithinOIDCTx" + via: "SetOIDCBackend(&classes.LazyStore{...}) in servers.go:154-155" + pattern: "lockUserGrantsShared" + - from: "controllers/authorizations.go revoke (exclusive LockUserGrants)" + to: "classes/oidc_store.go oidcTx (shared lockUserGrantsShared)" + via: "the same pg_advisory key hashtext('golem15_oauthserver_user_grants:')" + pattern: "userGrantsLockKey" +--- + + +Investigate the OIDC sibling of CR-01 in sm-oauthserver-plugin and fix it in the plugin only. The investigation is done (findings below). Task 1 turns the findings into deterministic tests that fail on the current code before Task 2 changes anything. + +Findings (from reading the current code; the plugin is at HEAD 21f5985, the framework at summercms.go master): + +Wiring. The plugin's servers.go:154-155 gives wristband's OIDC server a `classes.LazyStore`, which runs `classes.Store.WithinOIDCTx`: one GORM transaction per call, over `classes.oidcTx`. OIDC codes are stored in `golem15_oauthserver_auth_codes`, consents in `_consents`, and tokens in `_access_tokens` and `_refresh_tokens`. Revoke (controllers/authorizations.go:137-165) deletes rows from exactly these tables after it takes the exclusive grants lock. No oidcTx method takes the grants lock. Only the MCP store's `ByCodeHashForUpdate` and `ByTokenHashForUpdate` take it (shared; classes/mcp_store.go:201, :342). The plugin's JSON consent endpoints (controllers/consent_api.go) call `srv.ConsentRequest` and `srv.DecideConsent`, so they go through the same store. + +1. Decide (`wristband.decideConsent`, oidc_consent.go:60-129, used by the HTML POST and by ConsentDecide) is CONSISTENT. `SaveConsent` (oidc_store.go:328-362) and `mintCode` run in one transaction. Two orderings are possible: + - The revoke has already deleted the consent but not committed. The INSERT ... ON CONFLICT DO NOTHING conflict check uses a dirty snapshot, so it waits for the revoke, then inserts a fresh row. That is the order revoke, then allow, and it ends with a consent and a code. + - SaveConsent locks the existing row FOR UPDATE before the revoke's SELECT FOR UPDATE. The revoke then waits for the commit, and its later DELETEs (READ COMMITTED takes a new snapshot per statement) see the issued code. That is the order allow, then revoke, and it ends with nothing. + The only defect: if a revoke commits between the DO NOTHING and the FOR UPDATE, Take returns ErrRecordNotFound, the transaction rolls back and the allow answers 500. That fails closed and leaves nothing behind. Test D pins the consistent outcome. +2. Silent resume (`wristband.Server.Resume`, oidc_authorize.go:223-256) is a RACE. `ConsentFor` is a plain read. If a revoke commits (or holds the deleted row) before `TouchConsent`, the UPDATE by id hits 0 rows without an error, and `mintCode` inserts a code after the revoke's DELETE snapshot. The result is a live code with no consent. The exchange does not require a consent (oidc_token.go:273-280 touches it only when present), so the client gets tokens that the authorized-apps list cannot show, and `DELETE /oauth/authorizations/{client}` answers 404 for them. +3. Refresh (`rotateOIDCRefresh`, oidc_token.go:352-430) is a RACE. `RefreshTokenByHashForUpdate` locks R1. The revoke's DELETE on access_tokens and refresh_tokens uses statement snapshots that do not see the successor A2 and R2. Its DELETE on refresh_tokens waits on R1 and deletes R1 after the rotation commits, so A2 and R2 survive the revoke. access_tokens has no foreign keys, so nothing cascades. The OIDC half of the grant cascade has the same hole, although the comment on revokeUserGrants claims otherwise. +4. Code exchange (`redeemCode`, oidc_token.go:197-301) can DEADLOCK. `ConsumeCode` row-locks the code. The revoke deletes the consent and then waits on the code row. The exchange's `TouchConsent` then waits on the deleted consent row. Postgres detects a deadlock (40P01) and one side answers 500. + +Chosen design (plugin only, no framework change, no new exported identifier): make the OIDC store follow the rule the MCP store already follows. Every oidcTx entry point that touches a user's grant rows first calls `lockUserGrantsShared(ctx, t.db, userID)`, before any row lock or foreign-key lock on them: +- `SubjectFor`, `ConsentFor` and `SaveConsent` take the lock on entry. +- `ConsumeCode` and `RefreshTokenByHashForUpdate` first read the row's user_id by hash, take the lock, and only then run their guarded UPDATE or SELECT FOR UPDATE. This is the same pattern as MCP's ByCodeHashForUpdate. + +The lock is shared because shared already serializes these flows with revoke and the cascade, which take it exclusively. OIDC flows of the same user do not need to exclude each other: the upsert and the guarded UPDATEs already handle that. + +SubjectFor needs the lock because it is the first user-keyed statement in Resume, ConsentRequest and userinfo. Its INSERT takes a KEY SHARE lock on the users row through the foreign key. A user delete takes the exclusive grants lock in the cascade and then runs DELETE on users, so taking the lock after SubjectFor would let a delete and a resume deadlock. In userinfo, SubjectFor also comes before TouchAccessToken's row lock. + +Alternatives considered and rejected: +- Wrapping wristband's handlers in WithinUserGrantsLock, as din did for MCPConsentDecide. The plugin mounts wristband's Resume, authorize and token handlers directly, and the token endpoint does not know the user until it reads the code or the token. +- Any framework hook. Not needed, because the framework passes every grant access through the plugin's oidcTx. + +Framework change: none. The v0.1.7 plugin tag is not blocked, because go.mod keeps requiring summercms v0.6.0. Tagging stays with the orchestrator. + +Purpose: a revoke of an OIDC client is final, and the "ban / password change ends every grant" cascade also catches a rotation that is in flight. +Output: one plugin commit with the four tests, the store fix and the README. A residual-risk row in 32.1-SECURITY.md, which the orchestrator commits. + + + +@~/.claude/msd-core/workflows/execute-plan.md +@~/.claude/msd-core/templates/summary.md + + + +@/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/quick/261105-din-fix-32-1-cr-01-serialize-mcp-consent-all/261105-din-SUMMARY.md + +Repository facts the executor must respect: +- The plugin repo is /media/nvme/dev/golem15/summercms.io/summercms/sm-oauthserver-plugin: its own git repo, branch master, HEAD 21f5985 (the CR-01 fix, unpushed), latest tag v0.1.6. `go.work` has an UNRELATED uncommitted change. Never stage, edit, revert or stash it. Never use `git stash`, `git add -A` or `git add .` in that repo. +- Framework code, read-only. Do not change anything in summercms.go/modules: + - summercms.go/modules/wristband/oidc_authorize.go: Resume :188-269, mintCode :413-436. + - oidc_consent.go: decideConsent :60-129. + - oidc_token.go: redeemCode :197-301, rotateOIDCRefresh :349-430. + - oidc_userinfo.go :33-63. Its order is AccessTokenByHash, client, IdentityClaims, subjectFor, TouchAccessToken. + - oidc_subject.go:62-68: subjectFor calls tx.SubjectFor. +- Plugin store: classes/oidc_store.go. Store.WithinOIDCTx is at :46-55. The methods to change are ConsumeCode :200-219, ConsentFor :316-326, SaveConsent :328-362, SubjectFor :371-389 and RefreshTokenByHashForUpdate :426-449. The lock helper `lockUserGrantsShared(ctx, db, userID)` is in classes/mcp_backend_binding.go:77-87: it is a no-op for userID 0 and uses pg_advisory_xact_lock_shared(hashtext(key)). The pattern to copy is MCP `ByCodeHashForUpdate` in classes/mcp_store.go:191-215: it reads user_id by hash with Model/Select/Where/Limit(1)/Scan, takes the lock, then locks the row. Model user_id types: AuthCode.UserID and RefreshToken.UserID are `uint`. +- Schema (updates/202610190001): auth_codes, consents, subjects and refresh_tokens have `user_id REFERENCES users ON DELETE CASCADE`. access_tokens has no foreign key at all. +- Test harness (package oauthserver_test, every Postgres test skips under -short): + - `bootOAuth(t, envs...)` boots a fresh database per test. `env.gdb` is the app's *gorm.DB, and `env.issuer` is the base URL. + - Identity: `env.user(t)`, `env.sessionToken(t, user)`, `env.oidcClient(t, name, mutate...)` (returns clientID and secret, allowed openid profile email at `testRedirect`). + - Flow helpers: `pkce(t)`, `authorizeQuery(clientID, challenge, scope, state, nonce)`, `env.consentRedirect(t, token, clientID, challenge, scope, state, nonce)` (drives authorize, resume and allow, and returns the redirect URL; it handles the silent 302), `hiddenFields(page)`, `env.do(t, method, path, cookieToken, form, header)` (fatal only on a transport error), `env.exchangeCode` (fatal unless 200), `env.signIn(t, token, clientID, secret)`, `env.userInfo(t, accessToken)` (authorize_test.go:148), `env.browser()`. + - From mcp_consent_race_test.go: `asyncResult{status, body, err}`, `env.asyncJSON` (sends a Bearer token, so use it for the revoke DELETE), `receive(t, what, ch)` (30 s). + - A refresh-enabled boot needs the env value SUMMER_GOLEM15__OAUTHSERVER__REFRESH__ENABLED=true (discovery_test.go:196) and a client mutated to OfflineAccess=true with scopes openid, profile, email, offline_access. + - The token endpoint uses client_secret_basic. Copy the Basic header that exchangeCode builds (oidc_flow_smoke_test.go:32-38). + - Never call a t-using helper from a goroutine. Never log a code or a token: on a 200 or 302, withhold the body or Location in failure messages, as din's waitForGrantsWaiters does. +- Docker: if the Docker socket is denied, run the same command through `sudo -u golem bash -c '...'`. A skipped test is not a pass, so every verify greps for explicit `--- PASS:` or `--- FAIL:` lines. + + + + + + Task 1: Reproduce first, with deterministic OIDC-vs-revoke race tests (RED on the current code, D pins the decide path) + /media/nvme/dev/golem15/summercms.io/summercms/sm-oauthserver-plugin/oidc_grants_race_test.go, /media/nvme/dev/golem15/summercms.io/summercms/sm-oauthserver-plugin/mcp_consent_race_test.go + + - sm-oauthserver-plugin/mcp_consent_race_test.go (whole file: helper style, how failure messages withhold the body) + - sm-oauthserver-plugin/harness_test.go :150-345 (user, sessionToken, oidcClient, do, consentRedirect) + - sm-oauthserver-plugin/oidc_flow_smoke_test.go :22-54 (tokenResponse, exchangeCode Basic header) + - sm-oauthserver-plugin/controllers/authorizations.go :137-165 (revoke statement order) + - summercms.go/modules/wristband/oidc_authorize.go :223-256 and oidc_token.go :197-301, :352-430 (the blocking points the tests rely on) + + In /media/nvme/dev/golem15/summercms.io/summercms/sm-oauthserver-plugin, `git status --short` shows only ` M go.work`, and classes/oidc_store.go is unchanged since 21f5985. `docker info` succeeds; if it does not, use sudo -u golem. + + - TestOIDCResumeSerializesWithRevoke (A, RED today). The user signs in once, so a consent [openid profile email] exists. A second authorize with the same scopes parks request R. A holder transaction locks R's auth_requests row FOR UPDATE. An async GET of the resume path blocks on that row after it has read the consent; the test waits until 1 backend waits on a lock. An async revoke follows. The test waits until either the revoke answers or 2 backends wait on a lock, then rolls the holder back and collects both answers. Expected: resume 302 with a code, revoke 200. Afterwards: 0 consent rows and 0 unconsumed auth codes for the pair, and exchanging the resume's code does not return 200. Today the revoke completes while the resume is parked, and the resume then issues a code after the revoke: 1 live code, 0 consents. + - TestOIDCRefreshSerializesWithRevoke (B, RED today). Boot with refresh enabled and an OfflineAccess client. Sign in with "openid offline_access" and parse refresh_token R1 from the exchange body. Install the gate trigger (BEFORE INSERT on access_tokens) and hold the gate key. An async refresh with R1 blocks in the trigger, after it has locked R1 (1 waiter). An async revoke then blocks (2 waiters): today on R1's row, after the fix on the grants lock. Release the gate. Expected: refresh 200 with a new access token and refresh token, revoke 200. Afterwards: 0 access-token and 0 refresh-token rows for the pair, the new refresh token answers 400 at /oauth/token, and the new access token gets 401 at /oauth/userinfo. Today the new access and refresh tokens survive (counts 1 and 1). + - TestOIDCExchangeSerializesWithRevoke (C, RED today). Sign in once, then get a second code C2 through the silent consentRedirect. Install the gate trigger on access_tokens and hold the gate key. An async exchange of C2 blocks in the trigger after ConsumeCode (1 waiter). An async revoke blocks (2 waiters). Release the gate. Expected: exchange 200 and revoke 200, then 0 tokens and 0 unconsumed codes for the pair, and the exchanged access token gets 401 at /oauth/userinfo. Today the exchange's TouchConsent and the revoke's code DELETE deadlock, and one side answers 500 after about 1 s. + - TestOIDCConsentAllowThenRevoke (D, PINNING; passes today and after the fix). Sign in with "openid profile". A second authorize with "openid profile email" shows the incremental consent page; take request_id and consent_token from hiddenFields. Install the gate trigger (BEFORE INSERT on auth_codes) and hold the gate key. An async POST /oauth/authorize allow blocks in the trigger after SaveConsent (1 waiter). An async revoke blocks (2 waiters): today on the consent row, after the fix on the grants lock. Release the gate. Expected: allow 302 with a code, revoke 200. Afterwards: 0 consents, 0 unconsumed codes and 0 tokens for the pair, and the allow's code does not exchange. + + +Write the tests only. Do not change production code in this task, and do not commit: CLAUDE.md requires `go test ./...` to be green at every commit, so Task 2 commits the tests together with the fix. + +In mcp_consent_race_test.go, add one field to asyncResult: `location string`. This is additive; asyncJSON leaves it empty. + +New file oidc_grants_race_test.go (package oauthserver_test; imports the plugin's models package). Every helper below must be goroutine-safe or run only on the test goroutine, as noted for each. +- `env.asyncDo(ctx, method, path, cookieToken string, form url.Values, header http.Header) <-chan asyncResult`. It mirrors env.do without t. It sends from a goroutine with env.browser(), adds the token cookie (name "token") when cookieToken is set, and sets the form Content-Type when a form is given. It delivers status, body, the Location header and the error on a buffered channel of size 1. +- `env.lockWaiters(ctx) (int64, error)`. On env.gdb it counts pg_stat_activity rows where datname = current_database(), wait_event_type = 'Lock' and pid <> pg_backend_pid(). This one counter covers waits on advisory locks, transaction ids and tuples, so the same test works on the old and the new code. +- `env.waitForLockWaiters(t, n, what string, inflight ...<-chan asyncResult)`. Test goroutine only. Poll every 10 ms with a 15 s deadline. Return once the count reaches n. If an inflight channel delivers first, fail with " answered before backends waited on a lock"; for a 200 or 302, print "(withheld: it carries a code or token)" in place of the body. On the deadline, fail with the current count. +- `env.waitForAnswerOrLockWaiters(t, ch, n) (asyncResult, bool)`. Test goroutine only. It is the same poll, but a delivery on ch is a valid outcome: it returns (result, true). Reaching n waiters returns (zero, false). The deadline fails the test. +- `env.holdGate(t) *gorm.DB`. Begin a transaction on env.gdb with t.Context() and run SELECT pg_advisory_xact_lock(861105, 1). The two-int form is used because it can never collide with the grants lock's single-bigint hashtext key. Register a Cleanup that rolls the transaction back and ignores the error. +- `env.installGateTrigger(t, table string)`. Use plain Exec on env.gdb. Create a plpgsql function oas_test_gate() RETURNS trigger whose body does PERFORM pg_advisory_xact_lock(861105, 1) and RETURN NEW. Create a trigger oas_test_gate BEFORE INSERT ON FOR EACH ROW EXECUTE FUNCTION oas_test_gate(). Register a Cleanup that drops the trigger (IF EXISTS ... ON table) and the function (IF EXISTS). The database is per test, so fixed names are safe. Take table names from models.AccessToken{}.TableName() and models.AuthCode{}.TableName(). +- `env.holdRequestRow(t, requestID) *gorm.DB`. Begin a transaction and run SELECT id FROM models.AuthRequest{}.TableName() WHERE request_id = ? FOR UPDATE. Assert that it found the row. Register a rollback Cleanup. +- `env.pairRows(t, userID uint, clientID string) map[string]int64`. Count the consents rows, the auth_codes rows with consumed_at IS NULL, the access_tokens rows and the refresh_tokens rows where user_id and client_id match. +- `env.tokenCall(t, clientID, secret string, form url.Values) (int, string)`. A non-fatal POST /oauth/token through env.do, with the Basic header built exactly as exchangeCode builds it. Also add `env.asyncTokenCall(ctx, clientID, secret, form) <-chan asyncResult`, built on asyncDo with the same header. +- The revoke runs on env.asyncJSON(ctx, DELETE, "/oauth/authorizations/"+clientID, sessionToken, nil), the Bearer path din already uses. + +Tests. Follow the behavior bullets exactly. Each test starts with bootOAuth (B adds the refresh env value), env.user, env.sessionToken and env.oidcClient. Wait details: +- A: after starting the async resume, call waitForLockWaiters(1, "resume", resumeCh). After starting the revoke, call waitForAnswerOrLockWaiters(revokeCh, 2): today the revoke answers here, after the fix 2 waiters appear. Then roll back the holder and receive whatever has not been delivered yet. +- B, C, D: waitForLockWaiters(1, ...) with the flow's channel, then waitForLockWaiters(2, ...) with both channels, then roll back the gate holder and receive both answers. +Extract codes with url.Parse(location).Query().Get("code"). Assert that each one is non-empty without printing it. Final assertions use pairRows, tokenCall and env.userInfo; failure messages print only statuses and counts. Doc comment of each test: what it races, what today's code does (A, B, C) or that it pins a consistent ordering (D), and "Skips under -short". + +RED run, on the unchanged production code: +cd /media/nvme/dev/golem15/summercms.io/summercms/sm-oauthserver-plugin && go test -count=1 -v -run 'TestOIDC(Resume|Refresh|Exchange)SerializesWithRevoke|TestOIDCConsentAllowThenRevoke' . +Keep the --- FAIL and --- PASS lines and the failure messages for the SUMMARY. + +Gate. These decisions control Task 2: +- Expected result: A, B and C fail for the reasons in the behavior bullets, and D passes. Continue to Task 2. +- If one of A, B or C passes: rerun it with -count=5. If it still passes, the race on that path does not reproduce as analysed; record that in the SUMMARY. If all three pass, the outcome of this task is "not reproducible": Task 2 commits the tests as pinning tests with no change to classes/oidc_store.go, and Task 3 uses the not-reproducible wording. If at least one fails, apply the whole Task 2 fix anyway: it is one uniform invariant. +- If D fails today: record that the decide path is racy after all, with the failure message. Task 2's SaveConsent lock covers it; continue. +- If a test fails for a reason other than the one predicted (a helper bug, a timeout before the expected waiter count), fix the test until the failure is the predicted one. Do not weaken the assertions. + + + cd /media/nvme/dev/golem15/summercms.io/summercms/sm-oauthserver-plugin && go vet ./... && go test -count=1 -v -run 'TestOIDC(Resume|Refresh|Exchange)SerializesWithRevoke|TestOIDCConsentAllowThenRevoke' . 2>&1 | grep -E -- '^(--- (PASS|FAIL|SKIP)|ok|FAIL|panic)|oidc_grants_race_test.go:[0-9]+:' + + go vet passes. The output shows `--- FAIL:` for TestOIDCResumeSerializesWithRevoke (a live code with no consent), TestOIDCRefreshSerializesWithRevoke (successor tokens survive) and TestOIDCExchangeSerializesWithRevoke (a 500 from the deadlock), `--- PASS:` for TestOIDCConsentAllowThenRevoke, and no SKIP. The failure lines show no code or token. Nothing is committed, and production code is untouched. Any deviation from the gate's expected result is recorded with its evidence. + + + + Task 2: Take the user's grants lock (shared) in the plugin's OIDC store, update the README, run the full suite, make one plugin commit + /media/nvme/dev/golem15/summercms.io/summercms/sm-oauthserver-plugin/classes/oidc_store.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/README.md, /media/nvme/dev/golem15/summercms.io/summercms/sm-oauthserver-plugin/oidc_grants_race_test.go, /media/nvme/dev/golem15/summercms.io/summercms/sm-oauthserver-plugin/mcp_consent_race_test.go + + - sm-oauthserver-plugin/classes/oidc_store.go (whole file) + - sm-oauthserver-plugin/classes/mcp_store.go :187-215 (the ByCodeHashForUpdate pattern to copy) + - sm-oauthserver-plugin/classes/mcp_backend_binding.go :71-87 (userGrantsLockKey, lockUserGrantsShared doc) + - sm-oauthserver-plugin/README.md lines 23, 30, 32 and the Testing paragraph at 205 (grep for "**Stores.**", "An MCP allow takes the same lock", "which the MCP code exchange and refresh rotation take too", "TestMCPConsentFailureRollsBackCode") + + + - After the fix, all four tests from Task 1 pass. Existing OIDC, consent, token, authorized-apps, cascade and MCP tests stay green. + + +Skip the production-code part if Task 1's gate concluded "not reproducible" on all three paths. In that case, commit only the tests and the README Testing sentence, with the commit message "test(oidc): pin OIDC grant flows against a concurrent revoke". + +Fix in classes/oidc_store.go, per the design in the objective: +- `SubjectFor`: the first statement is lockUserGrantsShared(ctx, t.db, userID); return its error. It runs before the INSERT ... ON CONFLICT, because that insert's foreign key takes KEY SHARE on the users row. Taking the advisory lock after it would let a user delete deadlock with a resume, since the cascade holds the exclusive lock and then runs DELETE on users. +- `ConsentFor` and `SaveConsent`: take the same lock as the first statement. In Resume and the code exchange the lock is already held by then; taking it again is a cheap re-entry. SaveConsent is the first grant-row access of the consent decision. Before it, the decision holds only its auth_requests row, which no exclusive lock holder touches. +- `ConsumeCode`: keep the empty-hash early return. Next, read user_id from models.AuthCode by code_hash, without a row lock (Model, Select("user_id"), Where, Limit(1), Scan into a struct with a uint field). When the user id is non-zero, call lockUserGrantsShared. Then run the existing guarded UPDATE unchanged. Because the read does not filter on consumed_at, the replay branch of redeemCode (CodeByHash followed by RevokeAccessTokensForCode) also runs under the lock. +- `RefreshTokenByHashForUpdate`: same pattern on models.RefreshToken by token_hash, before the existing SELECT ... FOR UPDATE. +- Do not add the lock to any other method. TouchConsent, CreateCode, CreateAccessToken, CreateRefreshToken, MarkRefreshRotated, RevokeRefreshLineage and TouchAccessToken always run after one of the five methods above in the same transaction (userinfo calls SubjectFor before TouchAccessToken). AccessTokenByHash stays a plain read. +- Doc comments. On `Store`, add a paragraph stating the invariant: every OIDC transaction that touches a user's grant rows takes that user's grants lock in shared mode before its first row or foreign-key lock on them, so revoke and the grant cascade (exclusive) either wait for a resume, consent decision, code exchange, refresh rotation or userinfo call in flight and then see what it wrote, or run first and leave it the post-revoke state. Each of the five methods gets one sentence saying it takes the lock and why. Extend the lockUserGrantsShared doc in classes/mcp_backend_binding.go: the MCP code exchange and refresh rotation take it, and so do the OIDC store's SubjectFor, ConsentFor, SaveConsent, ConsumeCode and RefreshTokenByHashForUpdate. +- Shared, not exclusive, is deliberate. It is enough to serialize with revoke and the cascade, OIDC flows of one user do not need to exclude each other, and it matches the MCP exchange and refresh. Record this choice in the SUMMARY. +- Do not touch: summercms.go, controllers/*, cascade.go, mcp_store.go, any exported identifier. Do not touch go.work. + +Run the four Task 1 tests (GREEN), then run them with -count=10 for stability. A flaky test is a test bug: fix the wait condition, not the assertions. + +README (the plugin's module README; behaviour and tests change in the same commit): +- Line 23, **Stores.** bullet: append one sentence. Every OIDC transaction that touches a user's grant rows first takes that user's grants lock (shared). SubjectFor, ConsentFor and SaveConsent take it on entry; ConsumeCode and RefreshTokenByHashForUpdate take it after reading the row's user. A revoke or the grant cascade therefore either waits for a resume, consent decision, code exchange, refresh rotation or userinfo call in flight and then deletes what it issued, or runs first. +- Line 30, after "An MCP allow takes the same lock, so a revoke and an allow of the same user never interleave.": add "The OIDC store takes it too (shared), so a revoke never leaves behind a code or a successor token that a silent resume, a code exchange or a refresh rotation in flight issued." +- Line 32: change "which the MCP code exchange and refresh rotation take too (shared, before they lock their row)" to "which the MCP and OIDC code exchanges and refresh rotations take too (shared, before they lock their row)". +- Testing paragraph: after the TestMCPConsentFailureRollsBackCode sentence, add one sentence. It names TestOIDCResumeSerializesWithRevoke, TestOIDCRefreshSerializesWithRevoke and TestOIDCExchangeSerializesWithRevoke: a revoke that lands while a silent resume, a refresh rotation or a code exchange is parked mid-transaction leaves no code or token for the pair, and the exchange does not deadlock. It also names TestOIDCConsentAllowThenRevoke: a revoke queued behind a consent decision removes the consent and the code that decision issued. +- Every identifier you write must exist. SubjectFor and the other method names are wristband.OIDCTx methods that oidcTx implements. + +Gates, run in the plugin directory in this order: 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. If a failure also reproduces at 21f5985 without your change, it is not yours: record it and stop for the orchestrator. + +Commit in the plugin repo, staging explicit paths only and never go.work: classes/oidc_store.go, classes/mcp_backend_binding.go, oidc_grants_race_test.go, mcp_consent_race_test.go, README.md. Commit message "fix(oidc): serialize OIDC grant flows with revoke under the user grants lock". The body says three things. First, this is the OIDC sibling of 32.1 CR-01. Second, a silent resume could issue a code after a revoke, a refresh rotation's successor tokens survived a revoke, and a code exchange could deadlock with one. Third, the OIDC store now takes the user's grants lock (shared) before its first grant row, as the MCP store does, and the decide path was already consistent (pinned). No co-author or attribution trailer: the user's CLAUDE.md overrides that. Do not push or tag. Afterwards `git status --short` must show only ` M go.work`. Record the short hash for Task 3. + + + cd /media/nvme/dev/golem15/summercms.io/summercms/sm-oauthserver-plugin && go vet ./... && go test -count=1 -v -run 'TestOIDC(Resume|Refresh|Exchange)SerializesWithRevoke|TestOIDCConsentAllowThenRevoke|TestOIDCFlowSmoke|TestConsentOnce|TestIncrementalConsent|TestTokenEndpoint|TestRevokeAuthorization|TestAuthorizationsSmoke|TestUserCascadeSmoke|TestMCPConsentAllowSerializesWithRevoke|TestMCPBackendBinding|TestMCPGrantRevocation' . 2>&1 | grep -E -- '^(--- (PASS|FAIL|SKIP)|ok|FAIL|panic)' && go test -short ./... && go test -count=1 ./... 2>&1 | tail -3 && git status --short && git log -1 --format='%h %s' && git show --stat --format= HEAD + + Every test named in the verify run prints `--- PASS:`, with no FAIL or SKIP; this includes the four new tests, and the -count=10 run also passed. go vet is clean, and both the -short and the full non-short suites print ok. HEAD in sm-oauthserver-plugin is one new commit touching exactly the five listed files (or, in the not-reproducible branch, the tests and the README only). It has no attribution trailer and is unpushed and untagged. `git status --short` shows only ` M go.work`. `git -C /media/nvme/dev/golem15/summercms.io/summercms/summercms.go status --short -- modules` is empty. + + + + Task 3: Add a one-line note on the OIDC sibling to 32.1-SECURITY.md (edit only; the orchestrator commits) + /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 + + - 32.1-SECURITY.md lines 69-82 (Residual Risk intro and table) + + +HASH below is the plugin short hash from Task 2. Use Edit with a scoped replacement, never Write. + +In the Residual Risk table, insert one row directly after the CR-01 row. Pick the variant that matches Task 1's outcome. +- Fixed variant: | CR-01 sibling (OIDC) | high | T-32.1-07 | Fixed in sm-oauthserver-plugin HASH (2026-11-05, unreleased): the OIDC decide was already consistent (consent upsert and code in one transaction, ordered against revoke by the consent row lock; pinned by `TestOIDCConsentAllowThenRevoke`), but a silent resume could issue a code after a revoke, a refresh rotation's successor tokens survived a revoke and a code exchange could deadlock with one. The OIDC store now takes the user's grants lock (shared) before its first grant row. Tests `TestOIDCResumeSerializesWithRevoke`, `TestOIDCRefreshSerializesWithRevoke`, `TestOIDCExchangeSerializesWithRevoke`. | +- Not-reproducible variant: | CR-01 sibling (OIDC) | n/a | T-32.1-07 | Not reproducible: the OIDC decide, silent resume, code exchange and refresh paths stay consistent under a concurrent revoke; pinned by tests in sm-oauthserver-plugin HASH. | +- For a partial outcome, name only the paths that reproduced, and say the uniform fix covers all of them. + +Change nothing else: not the frontmatter, the register, the sign-off or the audit trail. Do not stage or commit. Leave the files of other sessions untouched (.planning/STATE.md, state.json, G15OFFICE.md and the untracked phase dirs). + + + cd /media/nvme/dev/golem15/summercms.io/summercms/summercms.go && H=$(git -C /media/nvme/dev/golem15/summercms.io/summercms/sm-oauthserver-plugin rev-parse --short HEAD) && grep -n "| CR-01 sibling (OIDC) |.*$H" .planning/phases/32.1-mcp-oauth-for-plugin-apps-declared-scopes-mcp-subject-on-con/32.1-SECURITY.md && git status --short -- .planning/phases/32.1-mcp-oauth-for-plugin-apps-declared-scopes-mcp-subject-on-con && git diff --stat -- .planning/phases/32.1-mcp-oauth-for-plugin-apps-declared-scopes-mcp-subject-on-con/32.1-SECURITY.md + + 32.1-SECURITY.md has exactly one new Residual Risk row for the OIDC sibling, and it carries the plugin hash. The file shows as modified and unstaged with a one-line insertion. This task touched no other file. + + + + + +## Trust Boundaries + +| Boundary | Description | +|----------|-------------| +| browser (signed-in user) → GET /oauth/authorize/resume/{request}, POST /oauth/authorize, POST/GET /oauth/authorize/requests/{request} | resume (silent re-grant) and consent decision for an OIDC client | +| relying party → POST /oauth/token | code exchange and refresh rotation, authenticated with the client secret | +| browser (signed-in user) → DELETE /oauth/authorizations/{client_id} | the revoke that must be final | +| wristband (framework) → classes.oidcTx (plugin) | every OIDC grant read and write goes through the plugin's store, which takes the lock | + +## STRIDE Threat Register + +| Threat ID | Category | Component | Severity | Disposition | Mitigation Plan | +|-----------|----------|-----------|----------|-------------|-----------------| +| T-261105-eab-01 | Elevation of privilege | OIDC silent resume vs revoke | high | mitigate | oidcTx.SubjectFor and ConsentFor take lockUserGrantsShared before the consent read, so a revoke cannot commit between the read and TouchConsent plus mintCode; proven by TestOIDCResumeSerializesWithRevoke | +| T-261105-eab-02 | Elevation of privilege | OIDC refresh rotation vs revoke and grant cascade | high | mitigate | RefreshTokenByHashForUpdate reads the token's user, then takes the shared lock before the row lock, so revoke and the cascade wait and their fresh statement snapshots see the successor tokens; proven by TestOIDCRefreshSerializesWithRevoke | +| T-261105-eab-03 | Denial of service | OIDC code exchange vs revoke deadlock | medium | mitigate | ConsumeCode takes the shared lock before the code's row lock, which removes the code-row / consent-row cycle; proven by TestOIDCExchangeSerializesWithRevoke | +| T-261105-eab-04 | Denial of service | new lock waits and cycles | medium | mitigate | the lock is shared only and is taken before any row or foreign-key lock on grant rows in every OIDC transaction. SubjectFor precedes the subject insert's KEY SHARE on users (user delete) and TouchAccessToken (userinfo). The decide holds only its auth_requests row, which no exclusive holder touches, before SaveConsent. No new cross-connection access inside a lock | +| T-261105-eab-05 | Tampering | OIDC consent decision vs revoke | low | accept | already consistent (one transaction, ordered by the consent row lock); pinned by TestOIDCConsentAllowThenRevoke. The fail-closed 500 window (revoke between DO NOTHING and FOR UPDATE) closes with SaveConsent's lock | +| T-261105-eab-SC | Tampering | npm/pip/cargo installs | low | accept | no packages installed, no dependency added | + + + +- Task 1 RED evidence: A, B and C show `--- FAIL:` with the predicted reasons and D shows `--- PASS:` on 21f5985. Recorded in the SUMMARY. +- Task 2 GREEN: all four new tests and the regression set print `--- PASS:` with no SKIP, the -count=10 run is stable, the full non-short suite is green, and there is one plugin commit with explicit paths. go.work stays modified and unstaged. Nothing is pushed or tagged. +- No change under summercms.go/modules, so no framework release is needed. The plugin's go.mod is unchanged (summercms v0.6.0), so the orchestrator's v0.1.7 tag is not blocked. +- Task 3: one Residual Risk row in 32.1-SECURITY.md with the hash, left uncommitted. + + + +- A revoke of an OIDC client can no longer be outlived by a code from a silent resume, or by tokens from a refresh rotation, that was in flight. A code exchange no longer deadlocks with a revoke. +- The finding for the decide path is pinned by a deterministic test. +- The fix is plugin only: no framework change, no exported API change, MCP paths untouched. +- Deterministic tests: a parked transaction (row lock or gate trigger) plus a pg_stat_activity waiter count. They fail on the old code and pass on the new. + + + +Create /media/nvme/dev/golem15/summercms.io/summercms/summercms.go/.planning/quick/261105-eab-fix-oidc-consent-decide-race-with-revoke/261105-eab-SUMMARY.md when done. Include: +- the plugin commit hash, the RED and GREEN test lines, and the gate outcome; +- the shared-vs-exclusive choice; +- the finding per path (decide consistent; resume, refresh and exchange fixed); +- a note for the orchestrator: no framework change, v0.1.7 not blocked; +- an out-of-scope observation, not changed here: the grant cascade still filters tokens by created_at < Before, so a token a concurrent rotation issued just after Before is not revoked. MCP has the same behaviour. + diff --git a/.planning/quick/261105-eab-fix-oidc-consent-decide-race-with-revoke/261105-eab-SUMMARY.md b/.planning/quick/261105-eab-fix-oidc-consent-decide-race-with-revoke/261105-eab-SUMMARY.md new file mode 100644 index 00000000..2c2adfb7 --- /dev/null +++ b/.planning/quick/261105-eab-fix-oidc-consent-decide-race-with-revoke/261105-eab-SUMMARY.md @@ -0,0 +1,133 @@ +--- +phase: quick-261105-eab +plan: 01 +subsystem: sm-oauthserver-plugin (OIDC store) +status: complete +tags: [oidc, oauth, revoke, concurrency, advisory-lock, security, cr-01] +requires: + - quick-261105-din (CR-01 MCP fix, plugin 21f5985) +provides: + - OIDC store takes the user's grants lock (shared) before its first grant row + - deterministic OIDC-vs-revoke race tests (resume, refresh, exchange, decide) +affects: + - 32.1-SECURITY.md residual risk (CR-01 sibling row, uncommitted) +tech-stack: + added: [] + patterns: + - "read row owner by hash, take pg_advisory_xact_lock_shared, then row-lock (copied from MCP ByCodeHashForUpdate)" + - "race tests: parked transaction (row lock or BEFORE INSERT gate trigger on a two-int advisory lock) plus pg_stat_activity lock-waiter count" +key-files: + created: + - /media/nvme/dev/golem15/summercms.io/summercms/sm-oauthserver-plugin/oidc_grants_race_test.go + modified: + - /media/nvme/dev/golem15/summercms.io/summercms/sm-oauthserver-plugin/classes/oidc_store.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/mcp_consent_race_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-SECURITY.md (uncommitted) +decisions: + - "OIDC store takes the user grants lock in SHARED mode (not exclusive): shared already serializes with revoke and the grant cascade (exclusive); OIDC flows of one user need not exclude each other (upsert and guarded UPDATEs handle that); matches the MCP exchange and refresh." + - "Lock points: SubjectFor, ConsentFor, SaveConsent on entry; ConsumeCode and RefreshTokenByHashForUpdate after reading the row's user_id by hash. No other method locks." + - "Plugin-only fix: no framework (summercms.go/modules) change, no exported identifier added or changed, MCP paths untouched." +metrics: + duration: ~15min + completed: 2026-11-05 + tasks: 3 +actuals: + tokens: 13000 + tasks: 3 + commits: 1 +plan_head_before: 21f59850d4f96789a363c52eeb02449c2d6498c1 +plan_head_after: 7b9899f36f45a13daccf3ec6f7d5c729ebfcc5f1 +--- + +# Quick 261105-eab: OIDC sibling of CR-01 (OIDC grant flows vs revoke) Summary + +The plugin's OIDC store now takes the user's grants advisory lock (shared) before its first grant-row or foreign-key lock, so a revoke or the grant cascade can no longer be outlived by a code from a silent resume or by successor tokens from a refresh rotation in flight, and a code exchange no longer deadlocks with a revoke. + +The repo of record for code is sm-oauthserver-plugin; `commits`, `plan_head_before` and `plan_head_after` above are measured there (`git rev-list --count 21f5985..HEAD` = 1). + +## Plugin commit + +- `7b9899f` fix(oidc): serialize OIDC grant flows with revoke under the user grants lock (sm-oauthserver-plugin, master, unpushed, untagged). Files: classes/oidc_store.go, classes/mcp_backend_binding.go, oidc_grants_race_test.go, mcp_consent_race_test.go, README.md. No attribution trailer. `git status --short` afterwards: only ` M go.work` (untouched). + +## Gate outcome (Task 1) + +Expected result, as predicted: A, B and C failed on 21f5985 for the analysed reasons, D passed. Task 2's full fix applied. + +## RED evidence (21f5985, production code unchanged) + +``` +oidc_grants_race_test.go:322: after resume and revoke (revoke answered while the resume was parked: true): 0 consent rows, 1 live codes, want 0 and 0 +oidc_grants_race_test.go:326: the resume's code exchanged (200) after the revoke +--- FAIL: TestOIDCResumeSerializesWithRevoke (0.75s) +oidc_grants_race_test.go:386: after refresh and revoke: 1 access tokens, 1 refresh tokens, want 0 and 0 +oidc_grants_race_test.go:395: successor refresh token after the revoke = 200, error "", want 400 invalid_grant +oidc_grants_race_test.go:398: userinfo with the successor access token after the revoke = 200, want 401 +--- FAIL: TestOIDCRefreshSerializesWithRevoke (0.63s) +oidc_grants_race_test.go:437: exchange = 200 (withheld: it carries a code or token), revoke = 500 {"error":"server_error"}, want 200 and 200 +--- FAIL: TestOIDCExchangeSerializesWithRevoke (1.67s) +--- PASS: TestOIDCConsentAllowThenRevoke (0.81s) +FAIL +``` + +No code or token appears in any failure line (200/302 bodies are withheld). The 1.67 s on C is Postgres's deadlock_timeout (1 s) firing; the revoke was the victim in this run. + +## GREEN evidence (after the fix) + +``` +--- PASS: TestOIDCResumeSerializesWithRevoke (0.89s) +--- PASS: TestOIDCRefreshSerializesWithRevoke (0.62s) +--- PASS: TestOIDCExchangeSerializesWithRevoke (0.62s) +--- PASS: TestOIDCConsentAllowThenRevoke (0.65s) +ok git.golem15.com/golem15/sm-oauthserver-plugin 7.594s +``` + +- `-count=10` stability run: 40/40 `--- PASS:`, no FAIL, `ok` (32.0 s). +- Regression set (verify command): `--- PASS:` for TestAuthorizationsSmoke, TestUserCascadeSmoke, TestRevokeAuthorization, TestConsentOnce, TestIncrementalConsent, TestMCPBackendBinding, TestMCPConsentAllowSerializesWithRevoke, TestMCPGrantRevocation, TestOIDCFlowSmoke, TestTokenEndpoint and the four new tests; no FAIL, no SKIP. +- `go vet ./...` clean; `go test -short ./...` ok; full non-short `go test -count=1 ./...` ok (101.3 s). A verbose full run printed 71 top-level `--- PASS:` lines, no `--- FAIL:`, and one pre-existing `--- SKIP: TestIDTokenShapeParity`: it skips without the PHP parity recordings (`requireParityRecordings`, parity_test.go), unrelated to this change. Docker was reachable directly (no sudo -u golem needed). + +## Finding per path + +| Path | Before | After | +|------|--------|-------| +| Consent decide (`decideConsent`: SaveConsent + mintCode) | Consistent (one tx; ordered against revoke by the consent row lock). Only defect: a fail-closed 500 if a revoke committed between ON CONFLICT DO NOTHING and FOR UPDATE. Pinned by D. | Same outcome, now ordered by the grants lock; the 500 window is closed by SaveConsent's lock. | +| Silent resume (`Server.Resume`) | RACE: revoke committed while resume was parked; resume then issued a live code with no consent (A). | Fixed: SubjectFor/ConsentFor hold the shared lock; the revoke waits and deletes the issued code. | +| Refresh rotation (`rotateOIDCRefresh`) | RACE: successor access and refresh tokens survived the revoke (B). | Fixed: RefreshTokenByHashForUpdate takes the lock before the row lock; the revoke's fresh statement snapshots see the successors. | +| Code exchange (`redeemCode`) | DEADLOCK: code row (exchange) vs consent row (revoke); one side 500 (C). | Fixed: ConsumeCode takes the lock before the code's row lock; both answer 200, no tokens left. | + +## Shared vs exclusive + +Shared, deliberately. Revoke and the grant cascade take the lock exclusively, so a shared holder already serializes every OIDC flow with them. OIDC flows of the same user do not need to exclude each other (the consent upsert and the guarded UPDATEs handle concurrent flows), and shared matches what the MCP code exchange and refresh rotation already take. + +## Deviations from Plan + +None in code or tests; the plan executed as written. + +- Process note: the plugin commit landed on the plugin repo's `master`, as the plan and orchestrator constraints direct (that repo's convention, as for 21f5985). The executor's generic protected-branch pre-commit assertion was not applied to the sibling plugin repo. +- The orchestrator's constraint (and the user's CLAUDE.md) overrode the session's default co-author trailer: none was added. + +## Notes for the orchestrator + +- No framework change: `git -C summercms.go status --short -- modules` is empty. No framework tag needed. +- The plugin's go.mod is unchanged (still requires summercms v0.6.0), so nothing blocks the plugin's v0.1.7 tag. Tagging and pushing are left to the orchestrator. +- 32.1-SECURITY.md has one new Residual Risk row "CR-01 sibling (OIDC)" carrying hash 7b9899f; it is modified and unstaged (1 insertion), not committed. +- Not committed per instructions: this SUMMARY, STATE.md, the PLAN and the 32.1 docs. ROADMAP.md untouched. + +## Out of scope (observed, not changed) + +The grant cascade (`Cascade.OnAccessRevoked`) still filters tokens by `created_at < Before`. A token that a concurrent rotation issues just after `Before` is therefore not revoked by the cascade, even though the rotation now waits for the cascade's lock. MCP has the same behaviour. This is the cascade's cutoff semantics, not a lock-ordering gap, and was left as is. + +## Threat surface + +No new endpoints, auth paths or schema changes. The new test-only trigger and function (`oas_test_gate`) live only in the per-test database and are dropped in Cleanup. + +## Known Stubs + +None. + +## Self-Check: PASSED + +- FOUND: sm-oauthserver-plugin/oidc_grants_race_test.go +- FOUND: commit 7b9899f in sm-oauthserver-plugin +- FOUND: "| CR-01 sibling (OIDC) |" row with 7b9899f in 32.1-SECURITY.md