docs(19.4): add code review report

2026-10-19 22:05:21 +02:00
parent 0cb5be3bbe
commit 1453631f60
1 changed files with 337 additions and 0 deletions
--- 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
@@ -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.<name>` 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_