diff --git a/.planning/phases/19.4-oidc-provider-on-wristband/19.4-REVIEW.md b/.planning/phases/19.4-oidc-provider-on-wristband/19.4-REVIEW.md new file mode 100644 index 00000000..97b38132 --- /dev/null +++ b/.planning/phases/19.4-oidc-provider-on-wristband/19.4-REVIEW.md @@ -0,0 +1,337 @@ +--- +phase: 19.4-oidc-provider-on-wristband +reviewed: 2026-10-19T20:03:37Z +depth: standard +files_reviewed: 134 +files_reviewed_list: + - docs/services/oauth-server.md + - docs/services/parity-testing.md + - ../fonoteka.go/plugins/golem15/fonoteka/oauth_resource_test.go + - ../fonoteka.go/plugins/golem15/fonoteka/plugin.go + - ../fonoteka.go/plugins/golem15/user/avatar_url_test.go + - ../fonoteka.go/plugins/golem15/user/classes/avatar.go + - ../fonoteka.go/plugins/golem15/user/classes/events.go + - ../fonoteka.go/plugins/golem15/user/classes/session_user.go + - ../fonoteka.go/plugins/golem15/user/controllers/api_controller.go + - ../fonoteka.go/plugins/golem15/user/controllers/registration.go + - ../fonoteka.go/plugins/golem15/user/controllers/users_admin_controller.go + - ../fonoteka.go/plugins/golem15/user/deactivating_event_test.go + - ../fonoteka.go/plugins/golem15/user/README.md + - ../fonoteka.go/plugins/golem15/user/session_user_test.go + - modules/tide/capture_test.go + - modules/tide/README.md + - modules/tide/variables.go + - modules/wristband/authorize.go + - modules/wristband/authorize_test.go + - modules/wristband/client_issue.go + - modules/wristband/consent_test.go + - modules/wristband/example_test.go + - modules/wristband/oidc_authorize.go + - modules/wristband/oidc_authorize_smoke_test.go + - modules/wristband/oidc_authorize_test.go + - modules/wristband/oidc_consent.go + - modules/wristband/oidc_consent_test.go + - modules/wristband/oidc_discovery.go + - modules/wristband/oidc_discovery_test.go + - modules/wristband/oidc_fakes_test.go + - modules/wristband/oidc_flavour_test.go + - modules/wristband/oidc_keys.go + - modules/wristband/oidc_keys_test.go + - modules/wristband/oidc_options.go + - modules/wristband/oidc_redirect.go + - modules/wristband/oidc_redirect_test.go + - modules/wristband/oidc_refresh_test.go + - modules/wristband/oidc_security_test.go + - modules/wristband/oidc_stores.go + - modules/wristband/oidc_subject.go + - modules/wristband/oidc_subject_test.go + - modules/wristband/oidc_token.go + - modules/wristband/oidc_token_smoke_test.go + - modules/wristband/oidc_token_test.go + - modules/wristband/oidc_tracer_test.go + - modules/wristband/oidc_userinfo.go + - modules/wristband/oidc_userinfo_test.go + - modules/wristband/README.md + - modules/wristband/register.go + - modules/wristband/registration_test.go + - modules/wristband/server.go + - modules/wristband/server_test.go + - modules/wristband/stores.go + - modules/wristband/token.go + - modules/wristband/token_test.go + - README.md + - scripts/check-phase19.4.sh + - ../sm-oauthserver-plugin/admin.go + - ../sm-oauthserver-plugin/admin_navigation.go + - ../sm-oauthserver-plugin/admin_permissions.go + - ../sm-oauthserver-plugin/admin_smoke_test.go + - ../sm-oauthserver-plugin/admin_surface_test.go + - ../sm-oauthserver-plugin/authorizations_smoke_test.go + - ../sm-oauthserver-plugin/authorizations_test.go + - ../sm-oauthserver-plugin/authorize_test.go + - ../sm-oauthserver-plugin/branding_test.go + - ../sm-oauthserver-plugin/buckets.go + - ../sm-oauthserver-plugin/buckets_test.go + - ../sm-oauthserver-plugin/cascade_test.go + - ../sm-oauthserver-plugin/claims_test.go + - ../sm-oauthserver-plugin/classes/branding.go + - ../sm-oauthserver-plugin/classes/cascade.go + - ../sm-oauthserver-plugin/classes/claims.go + - ../sm-oauthserver-plugin/classes/host.go + - ../sm-oauthserver-plugin/classes/keyring.go + - ../sm-oauthserver-plugin/classes/lazy.go + - ../sm-oauthserver-plugin/classes/mcp_store.go + - ../sm-oauthserver-plugin/classes/oidc_store.go + - ../sm-oauthserver-plugin/classes/reveal.go + - ../sm-oauthserver-plugin/clients_admin_test.go + - ../sm-oauthserver-plugin/commands_smoke_test.go + - ../sm-oauthserver-plugin/commands_test.go + - ../sm-oauthserver-plugin/config/config.yaml + - ../sm-oauthserver-plugin/consent_test.go + - ../sm-oauthserver-plugin/console/helpers.go + - ../sm-oauthserver-plugin/console/import_keys.go + - ../sm-oauthserver-plugin/console/import_settings.go + - ../sm-oauthserver-plugin/console/import_winter.go + - ../sm-oauthserver-plugin/console/prune.go + - ../sm-oauthserver-plugin/console/rotate_keys.go + - ../sm-oauthserver-plugin/controllers/admin_registry.go + - ../sm-oauthserver-plugin/controllers/authorizations.go + - ../sm-oauthserver-plugin/controllers/branding.go + - ../sm-oauthserver-plugin/controllers/clients.go + - ../sm-oauthserver-plugin/controllers/consent_api.go + - ../sm-oauthserver-plugin/discovery_test.go + - ../sm-oauthserver-plugin/go.mod + - ../sm-oauthserver-plugin/harness_test.go + - ../sm-oauthserver-plugin/import_cutover_smoke_test.go + - ../sm-oauthserver-plugin/import_keys_errors_test.go + - ../sm-oauthserver-plugin/import_winter_test.go + - ../sm-oauthserver-plugin/interop_test.go + - ../sm-oauthserver-plugin/keyring_test.go + - ../sm-oauthserver-plugin/lang/en/lang.yaml + - ../sm-oauthserver-plugin/lang/pl/lang.yaml + - ../sm-oauthserver-plugin/limits_smoke_test.go + - ../sm-oauthserver-plugin/mcp_store_test.go + - ../sm-oauthserver-plugin/middleware.go + - ../sm-oauthserver-plugin/models/client.go + - ../sm-oauthserver-plugin/models/settings.go + - ../sm-oauthserver-plugin/parity_test.go + - ../sm-oauthserver-plugin/php_method_map_test.go + - ../sm-oauthserver-plugin/plugin.go + - ../sm-oauthserver-plugin/prune_test.go + - ../sm-oauthserver-plugin/README.md + - ../sm-oauthserver-plugin/route_guard_test.go + - ../sm-oauthserver-plugin/routes.go + - ../sm-oauthserver-plugin/schedule.go + - ../sm-oauthserver-plugin/scripts/winter-export.sql + - ../sm-oauthserver-plugin/servers.go + - ../sm-oauthserver-plugin/subject_test.go + - ../sm-oauthserver-plugin/testdata/parity/deviations.yaml + - ../sm-oauthserver-plugin/testdata/parity/README.md + - ../sm-oauthserver-plugin/testdata/parity/rules.yaml + - ../sm-oauthserver-plugin/testdata/PHP_METHOD_MAP.md + - ../sm-oauthserver-plugin/testdata/php_methods.txt + - ../sm-oauthserver-plugin/token_test.go + - ../sm-oauthserver-plugin/updates/202610190001_create_golem15_oauthserver_tables.go + - ../sm-oauthserver-plugin/updates/202610190002_create_golem15_oauthserver_settings.go + - ../sm-oauthserver-plugin/updates/202610190003_create_golem15_oauthserver_mcp_codes.go + - ../sm-oauthserver-plugin/userinfo_test.go + - ../sm-oauthserver-plugin/views/consent.html.tmpl + - ../sm-oauthserver-plugin/views/error.html.tmpl + - ../sm-oauthserver-plugin/zero_edit_test.go +findings: + critical: 0 + warning: 6 + info: 9 + total: 15 +status: issues_found +--- + +# Phase 19.4: Code Review Report + +**Reviewed:** 2026-10-19T20:03:37Z +**Depth:** standard +**Files Reviewed:** 134 +**Status:** issues_found + +## Summary + +I reviewed the production code at standard depth: wristband's OIDC flavour (`oidc_*.go` plus the flavour and kind dispatch in `authorize.go`, `token.go`, `server.go`, `register.go` and `stores.go`), all of the new `sm-oauthserver-plugin` (stores, key ring, host and templates, claims, cascade, branding, reveal store, admin controllers, JSON controllers, buckets, middleware, routes, servers, migrations, console commands), the sm-user-plugin additions, `tide/variables.go` and `scripts/check-phase19.4.sh`. I skimmed the tests only for assertions that could hide a defect. + +The core protocol paths hold up under tracing: +- Codes and parked requests are consumed by guarded single UPDATEs, and the consent token is checked under `FOR UPDATE`. +- Kind isolation is enforced in SQL. +- Client authentication runs in uniform constant time. +- id_token claims are written so that protocol claims always win over identity claims. +- Logging stays free of credentials. + +I found no blocker. I did not re-report the four accepted findings of `19.4-SECURITY-REVIEW.md` (T-19.4-36..39) or behaviour that deliberately matches PHP. + +The defects found are concentrated in features that are new relative to PHP, where no parity constraint applies: +- the opt-in refresh lineage, which code replay does not fully revoke; +- the consent page, which never shows the `offline_access` scope; +- the `--force` re-import, which leaves refresh tokens behind and wipes MCP clients; +- admin editing of imported clients, which the new redirect URI check blocks; +- pairwise subjects, which change silently when a client is edited. + +## Warnings + +### WR-01: A replayed authorization code does not revoke refresh tokens rotated after the first exchange + +**File:** `../sm-oauthserver-plugin/classes/oidc_store.go:301-312` (called from `modules/wristband/oidc_token.go:207-217`) +**Issue:** On a replay of a consumed code, `redeemCode` calls `RevokeAccessTokensForCode(spent.ID)`. That revokes the access tokens with `auth_code_id = code` and the refresh tokens linked to them, and nothing more. When the first refresh token has already been rotated, the successors stay alive: +- `rotateOIDCRefresh` mints the new access token with `codeID == nil` (`oidc_token.go:386`); +- it links the successor refresh token to that new access token (`oidc_token.go:390`). + +So an attacker who exchanged an intercepted code and refreshed once keeps a working refresh lineage (30 days by default) after the legitimate client's replay is detected. Revoking the first refresh token does not stop the chain, because `RevokedAt` is only checked on the presented token, not on its ancestors. RFC 6749 §4.1.2 says tokens issued from the code should be revoked. This only applies with `refresh.enabled: true`, which is off by default and new relative to PHP, so no parity constraint applies. +**Fix:** Revoke whole lineages, starting from every refresh token issued directly from the code: +```go +func (t *oidcTx) RevokeAccessTokensForCode(ctx context.Context, codeID uint, now time.Time) error { + db := t.db.WithContext(ctx) + tokenIDs := db.Model(&models.AccessToken{}).Select("id").Where("auth_code_id = ?", codeID) + var heads []uint + if err := db.Model(&models.RefreshToken{}).Where("access_token_id IN (?)", tokenIDs).Pluck("id", &heads).Error; err != nil { + return err + } + for _, id := range heads { + if err := revokeRefreshLineage(ctx, db, id, now); err != nil { // follows rotated_to_id and revokes each linked access token + return err + } + } + return db.Model(&models.AccessToken{}).Where("auth_code_id = ? AND revoked_at IS NULL", codeID). + UpdateColumns(map[string]any{"revoked_at": now, "updated_at": now}).Error +} +``` +Add a wristband smoke test that runs exchange, refresh and replay of the code, then asserts that the rotated refresh token is refused. + +### WR-02: The consent page never shows the `offline_access` grant + +**File:** `../sm-oauthserver-plugin/classes/host.go:271-285`, `../sm-oauthserver-plugin/lang/en/lang.yaml:22-23` (same in `lang/pl/lang.yaml`) +**Issue:** `scopeLines` drops every scope without a `consent.scope.` phrase. Neither language file defines `consent.scope.offline_access`, so the scope is never listed (only `profile` and `email` have lines). A user who clicks Allow grants 30-day offline refresh tokens (`oidc_token.go:286`) without being told. Two related problems: +- On an incremental re-consent where only `offline_access` is new, the page shows "It's now also asking for:" followed by an empty list. +- The JSON consent mode passes `view.Scopes` with `offline_access` included, so only the server-rendered page under-discloses. + +`offline_access` is a Go-only feature (D-17), so no PHP parity constraint applies. +**Fix:** Add the phrase in both languages, for example: +```yaml +consent: + scope: + offline_access: "Stay signed in to :client when you are not using it (offline access)" +``` +Also make the `range .Scopes` in `views/consent.html.tmpl` impossible to render empty for a scope that has no phrase: fall back to the raw scope name instead of skipping it. + +### WR-03: `import-winter --force` leaves refresh tokens and MCP state behind and wipes MCP clients + +**File:** `../sm-oauthserver-plugin/console/import_winter.go:216-253`, `:268-275` +**Issue:** +1. `winterTables` does not include `refresh_tokens` or `mcp_codes`, yet `--force` runs `DELETE FROM` on `access_tokens` and then moves the id sequence to the imported `MAX(id)`. Refresh tokens issued before the re-import survive, and their `access_token_id` values now point at re-used ids of imported tokens that belong to other users: + - `revokeRefreshLineage` (`classes/oidc_store.go:482-487`) revokes `access_tokens.id = *m.AccessTokenID` on a replay, so a stale lineage can revoke an unrelated imported user's access token; + - a surviving refresh token still works for any client whose `client_id` is re-imported. +2. `DELETE FROM golem15_oauthserver_clients` deletes the `kind = 'mcp'` clients too. These never appear in a PHP export. The non-empty check (`:218-227`) counts them as well, so an application with `mcp.enabled` can only import with `--force`, which destroys every dynamically registered MCP client and orphans their `mcp_codes` and refresh rows. + +**Fix:** Delete only what the import owns, and clear dependent rows: +```go +// inside the transaction, when replacing: +tx.Exec(`DELETE FROM golem15_oauthserver_refresh_tokens WHERE client_id IN (SELECT client_id FROM golem15_oauthserver_clients WHERE kind = 'oidc')`) +for i := len(winterTables) - 1; i >= 0; i-- { + q := `DELETE FROM golem15_oauthserver_` + winterTables[i] + if winterTables[i] == "clients" { + q += ` WHERE kind = 'oidc'` + } + ... +} +``` +Count only `kind = 'oidc'` clients in the "already hold rows" check. Also drop the unused `existing` map (see IN-05). + +### WR-04: Imported PHP clients whose redirect URIs Go now rejects cannot be edited in the admin + +**File:** `../sm-oauthserver-plugin/controllers/clients.go:330-352` (with `modules/wristband/register.go:224-241`) +**Issue:** PHP stored any redirect URI (plan 03 deviation 3), and `import-winter` copies them unchecked (`import_winter.go:313`). `FormBeforeUpdate` runs `normalizeClient`, which refuses all of the following: +- any URI that `RejectRedirectURI` rejects: custom schemes for native apps, `http://[::1]`, http on non-loopback hosts, URIs longer than 512 characters; +- any `http://` URI on a non-dev client, including `http://localhost` on an imported client with `is_dev = 0`. + +These URIs still work at `/oauth/authorize` (`matchOIDCRedirectURI` compares byte for byte), but every save of such a client fails. That includes switching `is_active` off during an incident, unless the administrator first deletes the URI the relying party depends on. +**Fix:** Validate only lines that are new or changed relative to the stored `RedirectURIs`. In `FormBeforeUpdate`, load the persisted row and skip `RejectRedirectURI` and the http check for URIs already present. Alternatively, run the strict check on create only and report legacy URIs as a warning. + +### WR-05: Editing a pairwise client silently changes every user's `sub` + +**File:** `modules/wristband/oidc_subject.go:160-188`, `../sm-oauthserver-plugin/controllers/clients.go:366-384` +**Issue:** A pairwise client without `sector_identifier` derives its sector from the host of its redirect URIs, and subjects are stored per `(user, sector)`. If an administrator moves the redirect URIs to a new host, adds a sector identifier later, or switches `subject_type` between public and pairwise, `subjectFor` then allocates fresh random subs for everyone. Every relying party sees all existing users as new accounts, and their old links are orphaned. `normalizeClient` only checks that some sector resolves; it neither blocks nor warns about a sector change. Neither README mentions the hazard. +**Fix:** In `FormBeforeUpdate`, compute `SubjectSector` for the stored row and for the edited row. When they differ and the client has subjects in the old sector, refuse with a validation message, or require an explicit confirmation field. Document in the plugin README that the sector is part of the subject identity and should be pinned with `sector_identifier` before the first sign-in. + +### WR-06: The communities claim's "log and return []" fallback poisons the token transaction on Postgres + +**File:** `../sm-oauthserver-plugin/classes/claims.go:452-471` (runs inside `WithinOIDCTx` through `classes/oidc_store.go:393-399`) +**Issue:** `communities` documents and implements "the read fails, log and return `[]`, as PHP does". It runs on `t.db`, the open OIDC transaction. On Postgres a failed statement aborts the whole transaction (SQLSTATE 25P02), so after a logged failure every later statement fails: +- in the token path: `mintAccessToken`, `subjectFor`, `TouchConsent` and the commit; +- the same in userinfo and refresh. + +The request answers 500 instead of an id_token with `communities: []`. On the code path the rollback also un-burns the code. The PHP fallback held on MySQL because a failed statement there does not abort the transaction. The bundled schema matches today, so the trigger is transient (cancellation, lock timeout, a schema drift of the optional userfriends plugin). Still, the documented degradation does not exist. +**Fix:** Run the optional read in a savepoint, so a failure rolls back only that read: +```go +err := db.WithContext(ctx).Transaction(func(sp *gorm.DB) error { // nested Transaction = SAVEPOINT + return sp.Table(communityMembersTable+" AS m").Select(...).Joins(...).Where("m.user_id = ?", userID).Scan(&rows).Error +}) +``` +Alternatively, read communities outside the transaction before it opens, or correct the comment and return the error. + +## Info + +### IN-01: `http://[::1]` is a loopback host for matching but cannot be registered + +**File:** `modules/wristband/oidc_redirect.go:83`, `modules/wristband/register.go:237` +**Issue:** `loopbackHosts` (and the `oidc_redirect.go` doc comment and README) accept `[::1]` for dev port variance, but `RejectRedirectURI` only allows `127.0.0.1` and `localhost`, so the admin form refuses an `http://[::1]` URI. +**Fix:** Add `::1` to `rejectRedirectURI`'s loopback set, or drop it from `loopbackHosts` and the docs. + +### IN-02: `RevealStore.Take` reads and forgets in two non-atomic steps + +**File:** `../sm-oauthserver-plugin/classes/reveal.go:57-73` +**Issue:** `Get` followed by `Forget` lets two concurrent form loads by the same administrator (double fetch, two tabs) both receive the secret. The "shown once" guarantee is therefore best effort. +**Fix:** Use an atomic get-and-delete if cooler offers one (Redis `GETDEL`), or document the limitation. + +### IN-03: `KeyRing` caches signers by kid forever + +**File:** `../sm-oauthserver-plugin/classes/keyring.go:196-215` +**Issue:** `import-winter --key-dir --force` deletes and re-inserts a kid (`console/import_keys.go:224-252`). If the key material behind that kid changed, running instances keep signing with the cached old private key while JWKS publishes the new public key, and id_tokens fail verification until a restart. +**Fix:** Cache by kid plus a digest of `public_pem` (already selected in `PublishedKeys`), or tell the operator to restart after a forced key import. + +### IN-04: Concurrent rotations fail instead of being idempotent + +**File:** `../sm-oauthserver-plugin/classes/keyring.go:140-157` +**Issue:** The second of two concurrent `Rotate` calls re-evaluates `WHERE active FOR UPDATE` after the first commits and finds no row. It then inserts a second active key, which hits `oas_keys_one_active` (23505), so the command exits with "Key rotation failed". This is harmless, but the accepted T-19.4-33 says a duplicate rotation "would only add one retired key". +**Fix:** Treat a unique violation in `Rotate` as "another instance rotated" and report success, or correct the T-19.4-33 wording. + +### IN-05: Dead `existing` map in the import + +**File:** `../sm-oauthserver-plugin/console/import_winter.go:216`, `:223` +**Issue:** `existing[table] = n` is written and never read. +**Fix:** Remove it. + +### IN-06: The refresh grant issues an id_token for a narrowed scope without `openid` + +**File:** `modules/wristband/oidc_token.go:368-377`, `:401-410` +**Issue:** `scope=profile` on a refresh is accepted as a subset and still returns an id_token. OIDC Core §12.2 ties the id_token to `openid`. +**Fix:** Refuse (`invalid_scope`) a narrowed request that drops `openid`, or always keep `openid` in `scopes`. + +### IN-07: The consent redirect URL breaks a consent URL that has a fragment + +**File:** `../sm-oauthserver-plugin/classes/host.go:230-236` +**Issue:** `consent_url: https://app/#/consent` produces `https://app/#/consent?request=…`, which puts the query inside the fragment. That may be intended for hash-routed SPAs, but it is undocumented. A URL with an existing fragment and a query gets the parameter appended to the fragment. +**Fix:** Parse with `net/url` and set `RawQuery`, or document the hash-route behaviour in the `consent_url` config comment. + +### IN-08: The decision and switch-account POSTs have no body cap of their own + +**File:** `../sm-oauthserver-plugin/routes.go:340-345` +**Issue:** The routes are raw, so surf's `body.limit` does not apply. `ParseForm` in `oidcDecide` and `SwitchAccount` falls back to the standard library's 10 MB cap. The form has three short fields. +**Fix:** Add `"oauthserver.body.limit:16384"` to the POST authorize and switch-account routes. + +### IN-09: The gate's additive check misses some export removals + +**File:** `scripts/check-phase19.4.sh:436-438` +**Issue:** `removed_exports` only matches `-func X`, `-func (r) X` and `-type X`. It misses removed exported `var` and `const` declarations, types inside grouped `type (...)` blocks, exported interface methods and exported struct fields. Any of these can break consumers despite the D-05 "additive only" claim. +**Fix:** Compare `go doc -all` (or a `go/types` export dump) of the user plugin packages at `USER_BASE` and at HEAD, instead of grepping the diff. + +--- + +_Reviewed: 2026-10-19T20:03:37Z_ +_Reviewer: Claude (msd-code-reviewer)_ +_Depth: standard_