docs(19.6): add code review fix report

.planning/phases/19.6-two-factor-auth-and-device-flow-in-sm-user-plugin/19.6-REVIEW-FIX.md

@@ -0,0 +1,180 @@
+---
+phase: 19.6-two-factor-auth-and-device-flow-in-sm-user-plugin
+fixed_at: 2026-10-26T16:40:00Z
+review_path: .planning/phases/19.6-two-factor-auth-and-device-flow-in-sm-user-plugin/19.6-REVIEW.md
+iteration: 1
+findings_in_scope: 9
+fixed: 8
+partial: 1
+skipped: 0
+status: partial
+---
+
+# Phase 19.6: Code Review Fix Report
+
+**Fixed at:** 2026-10-26T16:40:00Z
+**Source review:** .planning/phases/19.6-two-factor-auth-and-device-flow-in-sm-user-plugin/19.6-REVIEW.md
+**Iteration:** 1
+**Scope:** CR-01 and WR-01..WR-08 (critical_warning); the ten info findings were left alone.
+
+**Summary:**
+- Findings in scope: 9
+- Fixed: 8 (5 of them flagged for human verification of the logic)
+- Partial: 1 (WR-05; a full fix needs an API contract change)
+- Skipped: 0
+
+`workflow.use_worktrees` is `false`, so every fix was edited, tested and committed in the main checkout of the repository that owns it. Nothing was pushed or tagged. The plugin commits carry no planning IDs and no trailers.
+
+| Finding | Outcome | Repo | Commit | Regression test |
+|---------|---------|------|--------|-----------------|
+| CR-01 | fixed: requires human verification | sm-user-plugin | cc3636c | `TestDeviceRevokeRacesRefresh` |
+| WR-01 | fixed: requires human verification | sm-user-plugin | 07196a4 | `TestDeviceInitiate/answer_and_row`, `TestDeviceAuthorize/token_field_takes_the_short_code`, `TestDeviceHandOverConcurrent/only_the_initiating_client_collects_the_token`, `TestDeviceAuthURL`, `TestDeviceFlowSmoke` |
+| WR-02 | fixed: requires human verification | sm-mfa-plugin | eb555e8 | `TestChallengeAfterRevocation` |
+| WR-03 | fixed: requires human verification | sm-user-plugin | 557c03c | `TestDeviceAccessRevocation` |
+| WR-04 | fixed | sm-user-plugin | 20e7b17 | `TestResetTwoFactorAction/privileged_member_needs_the_privileged_permission` |
+| WR-05 | partial | sm-mfa-plugin | 463c750 | `TestPasswordless/password_change_demotes_passkeys` |
+| WR-06 | fixed | sm-mfa-plugin | 0a08070 | `TestPasswordConfirmationThrottle` |
+| WR-07 | fixed: requires human verification | sm-mfa-plugin | f71d35d | `TestCompletionFailureSpendsNothing`, `TestEveryLoginPathIsGated` (lists updated) |
+| WR-08 | fixed: requires human verification | sm-sociallogin-plugin | ecfb8a7 | `TestChallengedSignInFiresOnCompletion` |
+
+Each regression test was checked against the unfixed code (the fix's source files stashed, the test run, the stash restored): every one failed without its fix and passes with it.
+
+## Fixed Issues
+
+### CR-01: A device revoke is lost to a concurrent refresh or a failed jti hand-over
+
+**Files modified:** `fonoteka.go/plugins/golem15/user/classes/device_auth.go`, `controllers/api_controller.go`, `controllers/devices.go`, `device_auth_test.go`, `README.md`
+**Commit:** cc3636c (sm-user-plugin)
+**Status:** fixed: requires human verification (race and state handling)
+**Applied fix:**
+- `DeviceDestroy` no longer reads the jti with a plain SELECT. It revokes with `UPDATE … RETURNING jwt_jti` inside the transaction and blacklists the returned jti. That jti is read under the row lock, and a row that is gone answers the same 404 as before.
+- The new `classes.FollowDeviceJTI(ctx, db, oldJTI, newJTI) (revoked bool, err error)` makes the guarded hand-over UPDATE (which takes the same row lock). When nothing moved but a session with the old jti exists in another status, it reports `revoked`. `HandOverDeviceJTI` keeps its signature as a wrapper.
+- `Refresh` calls the hand-over before stamping last_seen and fails closed:
+  - a revoked lineage answers 401 with the body bouncer gives a blacklisted token, `{"error":"Could not refresh token","msg":"The token has been blacklisted"}`, and the new token is never sent;
+  - a hand-over error answers the opaque 500 (it was only logged before).
+- Blacklisting the unsent `next` token is unnecessary, because it never leaves the server.
+- Whichever side commits second now sees the other. The `blacklist_grace > 0` fork stays the documented residual risk.
+
+### WR-01: The device token in the QR code is also the credential that collects the session JWT
+
+**Files modified:** `fonoteka.go/plugins/golem15/user/classes/device_auth.go`, `config/config.yaml`, `controllers/devices.go`, `device_auth_test.go`, `device_flow_smoke_test.go`, `README.md`
+**Commit:** 07196a4 (sm-user-plugin)
+**Status:** fixed: requires human verification (default value change, see contract note)
+**Applied fix:** the polling secret is kept off the screen without changing any response shape:
+- **Template placeholder.** `golem15.user.device_auth.auth_url` supports a new `{short_code}` placeholder (`classes.DeviceAuthURLFor(app, token, shortCode)`; `DeviceAuthURL` keeps its signature). The default is now `{frontend_url}/device/scan/{short_code}`, so `auth_url` (the QR code) carries only the short code.
+- **Authorize accepts the short code as `token`.** `POST /devices/authorize` accepts a short code in its `token` field (a 64-character device token never normalizes as one). A scan page that posts whatever its URL carries as `token` therefore works with both templates.
+- **Hand-over bound to the initiating client.** `GET /devices/status/{token}` hands the session token only to a poll with the `User-Agent` stored at initiate. Another client gets the existing `{"success":true,"data":{"status":"confirmed"}}` and consumes nothing, so the device still collects its session.
+
+**Contract note:** body shapes and keys are unchanged, but two values and behaviours are new:
+- The *value* of `auth_url` changes for apps that do not set `device_auth.auth_url`. sm-demo-app, sm-bm-app and fonoteka.go set no override. I found no frontend in the workspace that calls the device routes.
+- The token hand-over now requires the same `User-Agent` at initiate and poll. A device client that initiates through a server-side proxy and polls from the browser (different User-Agents) would wait until expiry. The PHP status route never handed over a token (that is Go's D-12a), so this binding constrains only the new Go contract.
+
+A separate `poll_token` key would be the clean RFC 8628-style fix if the contract may grow.
+
+### WR-02: Challenge completion mints a token for a user banned, or whose password changed, after the password step
+
+**Files modified:** `sm-mfa-plugin/classes/challenge.go`, `classes/admin.go`, `controllers/challenge.go`, `revoked_challenge_test.go`, `README.md`
+**Commit:** eb555e8 (sm-mfa-plugin)
+**Status:** fixed: requires human verification (logic)
+**Applied fix:**
+- `attempt` (used by every verify route: totp, email, recovery and webauthn) calls the new `classes.ChallengeRevoked` after loading the user. It refuses with the route's existing generic 401 when:
+  - the user is banned from any address (`ThrottleStates`);
+  - the user is suspended at the verifying address (`RejectIfThrottled`, the check `/login` makes);
+  - the user's `tokens_valid_after` is later than the challenge's `created_at` (compared at whole seconds, like the jwt guard).
+- I used suspension at the verifying address rather than suspension anywhere, so a stranger who fails five logins with the victim's email cannot block the victim's second factor.
+- `OnAccessRevoked` now runs `ExpireOpenChallenges` on `ev.Tx` for every event, bans included. Trusted devices are still revoked only on a password change.
+- `CompleteChallenge` also requires `expires_at > NOW()`, which closes the window between an attempt and its completion.
+- Only existing sm-user-plugin v0.1.12 API is used.
+
+### WR-03: Device hand-over ignores `tokens_valid_after`, and a password change leaves confirmed device sessions live
+
+**Files modified:** `fonoteka.go/plugins/golem15/user/classes/device_auth.go`, `controllers/devices.go`, `plugin.go`, `device_auth_test.go`, `README.md`
+**Commit:** 557c03c (sm-user-plugin)
+**Status:** fixed: requires human verification (logic)
+**Applied fix:**
+- `handOverDevice` now also refuses (and marks the row `revoked`) when `confirmed_at`, truncated to whole seconds, is before the user's `tokens_valid_after`.
+- Boot registers an `AccessRevokedEvent` listener that runs the new `classes.RevokeDeviceSessions` on `ev.Tx` (falling back to the published DB when `Tx` is nil). It sets `status = 'revoked'` for the users' `pending` and `confirmed` sessions. A listener error rolls the write back.
+- Used sessions keep their status; their tokens end through `tokens_valid_after`.
+
+### WR-04: Reset 2FA bypasses the privileged-group guard
+
+**Files modified:** `fonoteka.go/plugins/golem15/user/controllers/users_admin_controller.go`, `lang/en/lang.yaml`, `lang/pl/lang.yaml`, `reset_two_factor_test.go`, `README.md`
+**Commit:** 20e7b17 (sm-user-plugin)
+**Status:** fixed
+**Applied fix:**
+- The action's `Run` calls `guardPrivilegedMember` before `svc.Reset`. Without `golem15.users.manage_privileged_groups`, a privileged-group member answers cabana's 403 with the new message `privileged_member_reset_two_factor_forbidden` (en and pl), and `Reset` does not run.
+- I tried the optional hiding in `Applies`, then dropped it. Cabana answers a hidden action with a generic 409, and the existing password and delete guards show their 403 message rather than hiding the action.
+
+### WR-06: `confirmPassword` is an unthrottled password oracle for a session holder
+
+**Files modified:** `sm-mfa-plugin/controllers/management.go`, `confirm_password_test.go`, `README.md`
+**Commit:** 0a08070 (sm-mfa-plugin)
+**Status:** fixed
+**Applied fix:**
+- `confirmPassword` now records every non-empty password check through golem15.user's `CheckAndRecordLogin` for the client address. Five failures suspend the account there, as at `/login`, and a right password resets the count.
+- While the account is banned or suspended from any address (`ThrottleStates`), it answers the existing 403 `{"error":"Password confirmation required."}`, even to the right password.
+- An empty password is refused without counting.
+
+### WR-07: Failures after the challenge commit consume the challenge and the recovery code without delivering a token
+
+**Files modified:** `sm-mfa-plugin/classes/login.go`, `controllers/challenge.go`, `controllers/helpers.go`, `login_paths_test.go`, `complete_failure_test.go`, `README.md`
+**Commit:** f71d35d (sm-mfa-plugin)
+**Status:** fixed: requires human verification (transaction ordering)
+**Applied fix:**
+- **Before the transaction:** `completeSignIn` resolves the settings (`effective`, only when `trust_device` is truthy) and the session cookie (new `sessionCookie`).
+- **One transaction:** it then completes the challenge, spends the factor, issues the trusted device on `tx`, and runs the new `classes.FinishSignIn`. That function makes `IssueToken`, then `APIArray`, then golem15.user's `FinishLogin` on `tx`. The payload is built before `FinishLogin` so that no `GetApiArrayEvent` listener can block on the users-row lock that `FinishLogin` takes.
+- **After commit:** only then do the login event (new `classes.FireLogin`, source two_factor) and the verified event fire, and the answer is written by the infallible `writeSessionWith`.
+- `CompleteLogin` and `CompleteLoginAs` (the passkey path) are unchanged.
+- `TestEveryLoginPathIsGated` lists the new mint site `classes/login.go FinishSignIn usercontrollers.IssueToken` and the completion caller `controllers/challenge.go completeSignIn FinishSignIn`, and its call-site regexp now includes `FinishSignIn`.
+- Residual: `FinishLogin` sends a deactivated account's reactivate mail before the commit, so a failed commit after it would leave a mail for a restore that rolled back.
+
+### WR-08: A challenged social sign-in never fires `SignedInEvent`
+
+**Files modified:** `sm-sociallogin-plugin/classes/challenged.go` (new), `classes/codes.go`, `classes/flow.go`, `plugin.go`, `challenged_signin_test.go`, `README.md`
+**Commit:** ecfb8a7 (sm-sociallogin-plugin)
+**Status:** fixed: requires human verification (cross-plugin coupling)
+**Applied fix:**
+- **Holding the event.** When a gate challenges, `signIn` seals the `SignedInEvent` fields in the cache (`putSealedAt`, a new helper beside `putSealed`). The key is `oauth-2fa-signin:` plus the SHA-256 hex of the gate's `challenge_token`; the entry lives until the challenge's `expires_at`, at most 60 minutes. A cache error fails the sign-in, as the completion code would.
+- **Firing it.** Boot registers `classes.OnTwoFactorLogin`, a listener on golem15.user's `LoginEvent`. On source `two_factor` it reads the user's challenges completed in the last 5 minutes from `golem15_user_two_factor_challenges` (golem15.mfa's table, PHP's name), pulls each one's held-back sign-in once, and fires `SignedInEvent` with the original provider, identity, tenant, email, action and created flag.
+- **Timing.** The event fires after the 2FA token was minted, so a listener's writes reach later requests but not the completion's own payload. The README documents this.
+- **Dependencies.** sociallogin does not import mfa; it only reads that table name. No new sm-user-plugin API is used.
+
+## Partial Issues
+
+### WR-05: Second factors can be removed, replaced or added with a bearer session alone
+
+**File:** `sm-mfa-plugin/controllers/management.go`, `controllers/webauthn.go`, `classes/methods.go`, `classes/admin.go`
+**Commit:** 463c750 (sm-mfa-plugin)
+**Status:** partial
+**Applied mitigation (contract-safe):**
+- Every password change (own, reset, admin-set) now runs the new `classes.DemotePasskeys` in the password write's transaction, through `OnAccessRevoked`. It clears `passwordless` on the user's WebAuthn credentials; they stay enabled as second-factor keys. A passkey registered with a stolen session therefore no longer survives the password change that locks the intruder out.
+- The trade-off: the user's own passkeys must be registered again as passkeys after any password change, the same treatment trusted devices already get under D-07.
+- README "Passkeys" and "Security notes" record the decision and the remaining exposure.
+
+**Not fixed, because it needs a contract change:** PHP's `TwoFactorApiController` requires no password on `totpConfirm`, `totpDisable`, `emailDisable`, `webauthnRegister` or `webauthnRemove`, and the frontends send none. A full fix needs a step-up on:
+- `DELETE /2fa/totp`
+- `DELETE /2fa/email`
+- `DELETE /2fa/webauthn/{id}`
+- `POST /2fa/totp/confirm` while TOTP is already enabled
+- `POST /2fa/webauthn/register`, at least with `passwordless: true`
+
+The step-up is a `password` field (query or body, as `recovery-codes` takes it), or a current factor, refused with the existing 403 `{"error":"Password confirmation required."}` and counted through the throttle as in WR-06. Every frontend (the demo's `security.vue` and any production client) would have to send it, so it needs the user's decision and a coordinated frontend release. Until then, a stolen session can still disable or replace factors while it lives.
+
+---
+
+## Verification
+
+All checks ran in the **main checkouts** (`workflow.use_worktrees: false`; no worktree was created):
+- `/media/nvme/dev/golem15/summercms.io/summercms/fonoteka.go/plugins/golem15/user`
+- `/media/nvme/dev/golem15/summercms.io/summercms/sm-mfa-plugin`
+- `/media/nvme/dev/golem15/summercms.io/summercms/sm-sociallogin-plugin`
+
+sm-mfa-plugin and sm-sociallogin-plugin build against the local user plugin through their committed `go.work`. Docker was available to the session user, so no sudo was needed.
+
+- Before each commit: `gofmt`, `go vet ./...` and the repository's full `go test ./...` (Postgres testcontainers included) were green.
+- `scripts/check-phase19.6.sh --userplugin`, `--mfa`, `--sociallogin` and `--docs` all passed (run one stage per invocation; the script takes a single mode). The userplugin additive check reported no exported declaration removed or changed since 50da92c.
+- Final run after the last commit: `go vet ./...` and `go test -count=1 ./...` (Docker included) green in sm-user-plugin (root 89s, classes 48s, models, updates), sm-mfa-plugin (47s) and sm-sociallogin-plugin (43s). `go build ./...` of the fonoteka.go workspace, which uses the changed user plugin, also succeeds.
+
+_Fixed: 2026-10-26T16:40:00Z_
+_Fixer: Claude (msd-code-fixer)_
+_Iteration: 1_