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

docs(19.6): add code review report

Browse Source
This commit is contained in:
Jakub Zych
2026-10-26 15:45:43 +01:00
parent a70dbb6ff0
commit 680d4021e4
1 changed files with 354 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files

354
.planning/phases/19.6-two-factor-auth-and-device-flow-in-sm-user-plugin/19.6-REVIEW.md Normal file
View File

@@ -0,0 +1,354 @@
---
phase: 19.6-two-factor-auth-and-device-flow-in-sm-user-plugin
reviewed: 2026-10-26T14:43:47Z
depth: standard
files_reviewed: 65
files_reviewed_list:
- fonoteka.go/plugins/golem15/user/classes/login_gate.go
- fonoteka.go/plugins/golem15/user/classes/two_factor_admin.go
- fonoteka.go/plugins/golem15/user/classes/device_auth.go
- fonoteka.go/plugins/golem15/user/classes/events.go
- fonoteka.go/plugins/golem15/user/controllers/login_gates.go
- fonoteka.go/plugins/golem15/user/controllers/devices.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/models/device_auth_session.go
- fonoteka.go/plugins/golem15/user/updates/202610260001_create_device_auth_sessions.go
- fonoteka.go/plugins/golem15/user/admin_permissions.go
- fonoteka.go/plugins/golem15/user/plugin.go
- fonoteka.go/plugins/golem15/user/routes.go
- fonoteka.go/plugins/golem15/user/config/config.yaml
- sm-mfa-plugin/plugin.go
- sm-mfa-plugin/routes.go
- sm-mfa-plugin/buckets.go
- sm-mfa-plugin/mail.go
- sm-mfa-plugin/admin_permissions.go
- sm-mfa-plugin/admin_settings.go
- sm-mfa-plugin/classes/admin.go
- sm-mfa-plugin/classes/challenge.go
- sm-mfa-plugin/classes/config.go
- sm-mfa-plugin/classes/email.go
- sm-mfa-plugin/classes/events.go
- sm-mfa-plugin/classes/gate.go
- sm-mfa-plugin/classes/login.go
- sm-mfa-plugin/classes/methods.go
- sm-mfa-plugin/classes/passwordless.go
- sm-mfa-plugin/classes/qr.go
- sm-mfa-plugin/classes/recovery.go
- sm-mfa-plugin/classes/totp.go
- sm-mfa-plugin/classes/trusted.go
- sm-mfa-plugin/classes/webauthn.go
- sm-mfa-plugin/controllers/challenge.go
- sm-mfa-plugin/controllers/helpers.go
- sm-mfa-plugin/controllers/management.go
- sm-mfa-plugin/controllers/passwordless.go
- sm-mfa-plugin/controllers/webauthn.go
- sm-mfa-plugin/models/json_map.go
- sm-mfa-plugin/models/registry.go
- sm-mfa-plugin/models/settings.go
- sm-mfa-plugin/models/string_list.go
- sm-mfa-plugin/models/trusted_device.go
- sm-mfa-plugin/models/two_factor_challenge.go
- sm-mfa-plugin/models/two_factor_method.go
- sm-mfa-plugin/models/two_factor_recovery_code.go
- sm-mfa-plugin/models/webauthn_credential.go
- sm-mfa-plugin/updates/202610220001_create_golem15_user_two_factor_methods.go
- sm-mfa-plugin/updates/202610220002_create_golem15_user_two_factor_challenges.go
- sm-mfa-plugin/updates/202610220003_create_golem15_user_two_factor_recovery_codes.go
- sm-mfa-plugin/updates/202610220004_create_golem15_user_trusted_devices.go
- sm-mfa-plugin/updates/202610220005_create_golem15_mfa_settings.go
- sm-mfa-plugin/updates/202610220006_create_golem15_user_webauthn_credentials.go
- sm-mfa-plugin/updates/registry.go
- sm-mfa-plugin/config/config.yaml
- sm-sociallogin-plugin/classes/flow.go
- sm-sociallogin-plugin/classes/codes.go
- sm-sociallogin-plugin/controllers/callback.go
- sm-sociallogin-plugin/controllers/complete.go
- sm-sociallogin-plugin/controllers/register_complete.go
- sm-demo-app/vue-demo-app/app/lib/twoFactor.ts
- sm-demo-app/vue-demo-app/app/composables/useTwoFactor.ts
- sm-demo-app/vue-demo-app/app/composables/useDemoAuth.ts
- sm-demo-app/vue-demo-app/app/pages/account/security.vue
- sm-demo-app/vue-demo-app/app/pages/login.vue
findings:
critical: 1
warning: 8
info: 10
total: 19
status: issues_found
---
# Phase 19.6: Code Review Report
**Reviewed:** 2026-10-26T14:43:47Z
**Depth:** standard
**Files Reviewed:** 65 (paths relative to `/media/nvme/dev/golem15/summercms.io/summercms/`)
**Status:** issues_found
## Summary
I reviewed the production code of phase 19.6: sm-user-plugin `50da92c..1f3266a` (login gates, the login event, device authorization, the Reset 2FA action), every non-test file of the new sm-mfa-plugin, the social-login gate call in sm-sociallogin-plugin (`a8e3e78^..HEAD`), and the demo frontend files from `44a5229`. I read the tests only to understand intent. I did not re-report anything 19.6-SECURITY-REVIEW.md already records (F-1, D-12c phishing, the `blacklist_grace` fork, proxy logging of the status URL, the memory cooler, the numeric user handle, key order, accepted T-19.6-25).
Several things hold up. The challenge lifecycle is correct: the attempt counter is a guarded increment, completion is one guarded UPDATE in the same transaction that consumes the factor, the TOTP step is a guarded counter, and recovery codes are consumed with a guarded UPDATE. Secrets at rest are hashed or encrypted. Short codes use rejection sampling. TOTP compares in constant time (pquerna `hotp`), and recovery and email codes are compared with bcrypt. The WebAuthn RP ID and origins come from configuration, never from the request. Every sm-user mint site except F-1 is gated or carries its own proof.
The problems are at the edges of the flows:
- A device revoke can be lost to a concurrent refresh, or to a hand-over write that fails and is only logged. A revoked device then keeps refreshing indefinitely (CR-01).
- The QR token shown on the waiting device is also the secret that collects the session JWT (WR-01).
- Challenge completion re-checks neither a ban nor `tokens_valid_after`. The device hand-over does not check `tokens_valid_after` either (WR-02, WR-03).
- Reset 2FA skips the privileged-group guard that password and delete edits enforce (WR-04).
- Second factors can be removed, replaced or added with a bearer session alone. With passkey sign-in on, a stolen 60-minute JWT becomes a permanent login that survives a password change (WR-05).
## Narrative Findings (AI reviewer)
## Critical Issues
### CR-01: A device revoke is lost to a concurrent refresh or a failed jti hand-over, and the revoked device keeps refreshing
**File:** `fonoteka.go/plugins/golem15/user/controllers/devices.go:483-506`, `fonoteka.go/plugins/golem15/user/controllers/api_controller.go:276,315-321`, `fonoteka.go/plugins/golem15/user/classes/device_auth.go:137-144`
**Issue:** `DeviceDestroy` reads `row.JWTJTI` with a plain SELECT (line 484), outside its transaction and with no row lock. It then blacklists that value (line 506). `Refresh` mints the new token first (`bouncer.Refresh`, line 276, which with the default grace of 0 also blacklists the old jti). Only after that does it move the device row to the new jti with `HandOverDeviceJTI`, whose UPDATE requires `status = 'used'`.
Here is how the race goes:
1. Revoke reads jti A.
2. A refresh verifies A, mints B and runs `UPDATE … SET jwt_jti = B WHERE jwt_jti = A AND status = 'used'`, which succeeds.
3. Revoke marks the row revoked and blacklists A, which is already dead.
The device now holds B, which is not blacklisted. Its next refresh (B to C) finds no `used` row, so `HandOverDeviceJTI` changes nothing and nobody ever blacklists the lineage. The reverse order (revoke commits between `bouncer.Refresh` and the hand-over) gives the same result: the UPDATE matches no row and `next` is returned anyway.
There is also a deterministic fail-open path that needs no race. Any error from `HandOverDeviceJTI` is only logged ("never fails the refresh", lines 309-320). After one transient database error, the row keeps a stale jti forever, and a later revoke blacklists a token the device no longer uses. The revoked or stolen device stays signed in, and keeps refreshing, until a password change.
T-19.6-08 is marked closed, but its tests cover only sequential refresh and revoke. A device holder that refreshes in a tight loop widens the window.
**Fix:** Make both sides atomic and fail closed:
```go
// DeviceDestroy: read the jti that is current at commit time
var jti *string
err = lagoon.Transaction(r.Context(), gdb, func(ctx context.Context, tx *gorm.DB) error {
if err := tx.Raw(`UPDATE `+deviceTable+` SET status = 'revoked', updated_at = NOW()
WHERE id = ? AND user_id = ? RETURNING jwt_jti`, row.ID, user.ID).Scan(&jti).Error; err != nil {
return err
}
... // blacklist *jti
})
// Refresh: lock the device row by the old jti before minting, refuse a revoked lineage,
// and fail the refresh (or blacklist `next`) when the hand-over UPDATE errors or
// matches a row that is no longer 'used'.
```
At minimum, give `HandOverDeviceJTI` a signal that a row with `jwt_jti = old` exists in status `revoked`. When it does, blacklist `next` and answer the refresh 401, and treat an error from it as a refresh failure.
## Warnings
### WR-01: The device token in the QR code is also the credential that collects the session JWT
**File:** `fonoteka.go/plugins/golem15/user/classes/device_auth.go:96-105`, `fonoteka.go/plugins/golem15/user/controllers/devices.go:147-153,170-198,360-361`
**Issue:** `DeviceInitiate` builds `auth_url` (the QR code the device displays) from the same `token` that `GET /devices/status/{token}` accepts to hand over the session JWT. Whoever sees the QR code or the scan URL (a photo of the screen, browser history, a Referer or analytics script on the `/device/scan/{token}` page, a QR scanner app) can poll the status route. The first poll after the victim confirms receives `{"token": …}`, and the guarded UPDATE makes the legitimate device lose with `{"status":"used"}`.
This is not the phishing case D-12c accepts. The victim authorizes their own device and the observer takes the session. RFC 8628 keeps the polled `device_code` separate from the displayed `user_code` for this reason. The security review's T-19.6-09 covers storage and logs only.
**Fix:** Keep the polling secret off the screen. Without changing the existing response shapes:
- support a `{short_code}` placeholder in `device_auth.auth_url` and make it the default, so the QR code carries only the short code that `DeviceAuthorize` already accepts;
- additionally bind the hand-over to the initiating client, for example by refusing a poll whose user agent differs from `device_user_agent`.
A separate `poll_token` returned only by `initiate` is the clean fix if the contract can grow a key.
### WR-02: Challenge completion mints a token for a user banned, or whose password changed, after the password step
**File:** `sm-mfa-plugin/controllers/challenge.go:62-96,296-327`, `sm-mfa-plugin/classes/login.go:355-391`
**Issue:** The ban and suspension checks run once, at `/login` (`RejectIfThrottled`). The challenge stays usable for `two_factor_challenge_ttl` (up to 60 minutes through settings). Neither `attempt` nor `completeSignIn` re-checks `ThrottleStates`, and neither compares the challenge's `created_at` with the user's `tokens_valid_after`.
A ban, or a password reset meant to lock out an attacker who has the password and a second factor (for example a stolen recovery code), does not invalidate the pending challenge. Its completion mints a token with `iat` after the revocation cutoff, which the jwt guard accepts (`PrincipalByID` checks no ban). `/refresh` would refuse the user, but the token is good for its full TTL.
The passkey path (`passwordless.go:118-131`) and the device hand-over (`devices.go:255-261`) both check bans, so this path is the inconsistent one.
**Fix:** In `attempt`, after `ChallengeUser`, refuse with the method's generic 401 when:
```go
banned, suspended, err := userclasses.ThrottleStates(userclasses.ContextWithThrottle(ctx, app), db, []uint{user.ID})
if banned[user.ID] || suspended[user.ID] ||
(user.TokensValidAfter != nil && ch.CreatedAt != nil && user.TokensValidAfter.After(*ch.CreatedAt)) { refuse(w); return ... }
```
Also delete the user's open challenges in `OnAccessRevoked` (`classes/admin.go:471-484`) on the event's transaction.
### WR-03: Device hand-over ignores `tokens_valid_after`, and a password change leaves confirmed device sessions live
**File:** `fonoteka.go/plugins/golem15/user/controllers/devices.go:245-271`
**Issue:** `handOverDevice` refuses only a missing, soft-deleted or banned user. A user who authorized a device and then changes the password (or an admin who sets one), intending to end every session, still has the `confirmed` row live until `expires_at`. The next poll mints a fresh JWT whose `iat` is after the cutoff. No `AccessRevokedEvent` listener touches `golem15_user_device_auth_sessions`.
**Fix:** In `handOverDevice`, treat `user.TokensValidAfter != nil && row.ConfirmedAt != nil && user.TokensValidAfter.After(*row.ConfirmedAt)` as refused (mark the row revoked). Also add an `AccessRevokedEvent` listener in sm-user-plugin that runs, on `ev.Tx`:
```sql
UPDATE golem15_user_device_auth_sessions SET status = 'revoked'
WHERE user_id IN ? AND status IN ('pending', 'confirmed')
```
### WR-04: Reset 2FA bypasses the privileged-group guard
**File:** `fonoteka.go/plugins/golem15/user/controllers/users_admin_controller.go:802-827` (compare `193-210`, `537`, `549`)
**Issue:** On the Users screen, setting the password of a privileged-group member (whose group "makes a site administrator") or deleting one requires `golem15.users.manage_privileged_groups` (`guardPrivilegedMember`). The new `reset_two_factor` action's `Run` calls `svc.Reset` directly and requires only `golem15.users.reset_two_factor`. An administrator granted only the reset permission (for example a support role) can strip every second factor, trusted device and passkey from a privileged account. That leaves the account behind its password alone, which is the downgrade the privileged guard exists to prevent.
**Fix:** Run the same guard before the reset:
```go
func(ctx context.Context, tx *gorm.DB, user *models.User) error {
if err := c.guardPrivilegedMember(ctx, user.ID, usersLang+"privileged_member_forbidden", nil); err != nil {
return err
}
...
return svc.Reset(ctx, tx, user)
}
```
Optionally, also hide the action in `Applies` for privileged members when the principal lacks `PermissionManagePrivilegedGroups`.
### WR-05: Second factors can be removed, replaced or added with a bearer session alone; a stolen JWT becomes a permanent passkey login
**File:** `sm-mfa-plugin/controllers/management.go:87-128,152-171,176-208`, `sm-mfa-plugin/controllers/webauthn.go:88-195,202-243`, `sm-mfa-plugin/classes/methods.go:79-85`, `sm-mfa-plugin/classes/admin.go:471-484`
**Issue:** Only recovery codes ask for the password (`confirmPassword`). The following need nothing beyond `jwt.auth`:
- `DELETE /2fa/totp`, `DELETE /2fa/email` and `DELETE /2fa/webauthn/{id}` disable factors.
- `POST /2fa/totp/confirm` overwrites an already enabled TOTP secret: `EnableMethod` runs `ON CONFLICT … DO UPDATE SET secret = EXCLUDED.secret`, without proof of the current factor.
- `POST /2fa/webauthn/register` adds credentials.
With `two_factor_passwordless_login` on, anyone holding a session JWT for 60 minutes (XSS, a leaked token, a device obtained through WR-01) can register their own passkey with `passwordless=true`. `OnAccessRevoked` revokes only trusted devices, so that passkey keeps signing in without a password or a challenge after the victim changes the password. T-19.6-25 accepted a client-chosen secret for the caller's own first setup, not the overwrite of an enabled factor or the persistence path.
**Fix:**
- Require `confirmPassword` (or a fresh factor) on factor removal, on TOTP confirm when a TOTP method is already enabled, and on WebAuthn registration (at least when `passwordless` is requested).
- On a password change, either delete `passwordless` credentials or record the change and flag credentials created since the last password change, as the README's threat notes decide.
### WR-06: `confirmPassword` is an unthrottled password oracle for a session holder
**File:** `sm-mfa-plugin/controllers/management.go:212-224`
**Issue:** A wrong password on `GET /2fa/recovery-codes?password=` or on `regenerate` answers 403 without recording a failed attempt in golem15.user's throttle. Those routes allow 120 requests a minute per user (`user-api`, keyed by the bearer subject). A stolen session can therefore brute-force the account password at about 170,000 guesses a day without ever triggering the login suspension. Recovering the password then defeats the password half of every later login.
**Fix:** Count a failed confirmation through the user plugin's throttle (export a helper around `refuseLogin`'s recording, or call `classes.CheckAndRecordLogin(ctx, db, user, ip, false)`). Refuse while the account is suspended. Alternatively, give the two routes a tight per-user bucket (for example 5 a minute).
### WR-07: Failures after the challenge commit consume the challenge and the recovery code without delivering a token
**File:** `sm-mfa-plugin/controllers/challenge.go:296-351`, `sm-mfa-plugin/controllers/helpers.go:569-582`
**Issue:** `completeSignIn` commits the completed challenge (and the consumed recovery code or TOTP step) first. After that it calls:
1. `CompleteLogin`: `FinishLogin` restores the account and stamps it, the token is minted, and `LoginEvent` fires.
2. The `VerifiedEvent`.
3. `withTrustedDevice`, which runs `effective` and `IssueTrustedDevice` (an INSERT).
4. `writeSession`, which may fail on `bouncer.LoadCookieConfig`.
Any error in steps 3 or 4 answers the opaque 500 after the login event fired and a token was minted but never sent. The user has to sign in again, and when the factor was a recovery code, that single-use code is gone. An error in step 1 burns the code the same way. Audit listeners see a `golem15.user.login` for a sign-in that never happened.
**Fix:**
- Resolve the cookie config and `effective` before the transaction.
- Create the trusted-device row inside the completing transaction (it only needs `db`).
- Mint and fire events only after everything that can fail has succeeded.
- For a failure inside `CompleteLogin`, consider rolling the recovery-code consumption into the same transaction as `FinishLogin`.
### WR-08: A challenged social sign-in never fires `SignedInEvent`, so its listeners are skipped for 2FA users
**File:** `sm-sociallogin-plugin/classes/flow.go:340-353`
**Issue:** When the gate challenges, `signIn` returns before `fireSignedIn`. The second factor's completion (`mfa CompleteLogin`) fires only golem15.user's `LoginEvent` with source `two_factor`, which carries no provider, identity or tenant. Any application listener on `SignedInEvent` never runs for users who enabled a second factor while `two_factor_social_login` is on. The plugin README cites one such listener: an organisation assigned by tenant, whose write is meant to reach the token. The README documents the skip, but no replacement event exists, so the listener's effect is silently lost on exactly the accounts with the strongest protection.
**Fix:** Store the pending `SignedInEvent` fields with the challenge (the completion payload already goes through the sealed cache), and fire `SignedInEvent` on completion. One way is a `LoginEvent` listener in sociallogin that recognises a challenge it created, for example keyed by challenge id. Another is a documented hook in mfa's `CompleteLogin`. At minimum, state in the README that `SignedInEvent` listeners must also handle `LoginEvent{Source: two_factor}`.
## Info
### IN-01: The email-send cap is hard-coded in SQL next to an unused constant
**File:** `sm-mfa-plugin/classes/email.go:25,83`
**Issue:** `MaxEmailSends = 3` is documented as the cap, but the guarded UPDATE says `email_sends < 3` literally. Changing the constant would silently not change the behaviour.
**Fix:** Bind the constant: `… AND email_sends < ?`, passing `MaxEmailSends`.
### IN-02: `two_factor_email_code_ttl` changes only the mail text; the code lives as long as the challenge
**File:** `sm-mfa-plugin/classes/email.go:111-136`, `sm-mfa-plugin/config/config.yaml` (`two_factor_email_code_ttl`)
**Issue:** `VerifyEmailCode` never checks the send time. Settings allow a challenge TTL of up to 60 minutes and a code TTL of 1 minute, and the code then works for the full hour although the mail promised one minute.
**Fix:** Store the send instant (or `code_expires_at`) in the guarded UPDATE and refuse after `EmailCodeMinutes`.
### IN-03: Confirmed device sessions that are never polled are never expired, cleaned or hidden
**File:** `fonoteka.go/plugins/golem15/user/controllers/devices.go:97-100,385-392,431-434`
**Issue:** A row only becomes `expired` when it is polled or re-authorized, and the cleanup deletes only `pending` and `expired` rows. A `confirmed` row whose device went away therefore stays forever, appears in `GET /devices` as an authorized device, and keeps its unique short code. Line 388 also discards the error of the expiry UPDATE.
**Fix:** Include `status = 'confirmed' AND expires_at < NOW() - INTERVAL '1 hour'` in the cleanup, and filter expired confirmed rows out of `DeviceIndex`, or mark them expired there. Log the line-388 error.
### IN-04: A short code typed with a space answers 422 although the doc says spaces are optional
**File:** `fonoteka.go/plugins/golem15/user/controllers/devices.go:312-313,343-348`
**Issue:** The length check runs after removing hyphens only, so "ABCD EFGH" (9 bytes) is refused before `NormalizeDeviceShortCode`, which would accept it.
**Fix:** Strip spaces as well before the length check, or make the doc comment match PHP's behaviour.
### IN-05: The default WebAuthn RP ID comes from `app.url`, contradicting the configuration's own guidance
**File:** `sm-mfa-plugin/classes/config.go:178-184`
**Issue:** The config comment says the RP ID must be the host of the frontend pages. With an empty `rp_id`, though, the default is the API host (`app.url`). When `golem15.user.frontend_url` is another host, every ceremony fails in the browser. This fails closed, but it is a confusing default.
**Fix:** Default to the `frontend_url` host when it is set, and refuse at boot (or log) an origin whose host is neither the RP ID nor a subdomain of it.
### IN-06: `MaskEmail` slices bytes and can split a multi-byte character
**File:** `sm-mfa-plugin/classes/email.go:41-51`
**Issue:** `local[:1]` and `local[len(local)-1:]` cut UTF-8 sequences in internationalised local parts, which produces U+FFFD in the JSON answer.
**Fix:** Operate on `[]rune(local)`.
### IN-07: A gate result with a nil Body is answered as `null`
**File:** `fonoteka.go/plugins/golem15/user/controllers/api_controller.go:150-156`
**Issue:** sociallogin normalizes a nil `gate.Body` to `{}` (`flow.go:349-352`), but `Login` writes it as is.
**Fix:** Apply the same `if body == nil { body = map[string]any{} }` before `writeJSON`.
### IN-08: The second-factor WebAuthn assertion is not held to the 60-second ceremony timeout
**File:** `sm-mfa-plugin/classes/webauthn.go:453-460`
**Issue:** `FinishAssertion` sets `session.Expires` to the challenge's `expires_at` (5 minutes or more). The enforced `WebAuthnTimeout` therefore applies only to registration and passkey state, not to the login step.
**Fix:** Store the options' issue time with the challenge, and use `min(issued+WebAuthnTimeout, ch.ExpiresAt)`.
### IN-09: The demo login cannot complete an email-only or WebAuthn-only challenge
**File:** `sm-demo-app/vue-demo-app/app/pages/login.vue:21,35,98`
**Issue:** Without `totp` in `available_methods`, the page falls back to recovery-code mode and hides the switch. A user whose only method is email can sign in only by spending a recovery code.
**Fix:** Add the `email/send` and `verify method=email` step, or show a message that the method is not supported in the demo.
### IN-10: Passkey sign-in refuses non-activated accounts even when activation is not required
**File:** `sm-mfa-plugin/classes/passwordless.go:115-117`
**Issue:** `Login` refuses a not-activated account only while `golem15.user.activation.require_activation` is on. The passkey path refuses it unconditionally. This fails closed, but the two sign-ins are inconsistent.
**Fix:** Use the user plugin's registration settings (`RegistrationSettings`) for the same check, or document the stricter rule.
---
_Reviewed: 2026-10-26T14:43:47Z_
_Reviewer: Claude (msd-code-reviewer)_
_Depth: standard_