From 07f4632dc2e5bb68a4bbcf513e228e005cf491ac Mon Sep 17 00:00:00 2001 From: Jakub Zych Date: Mon, 26 Oct 2026 15:34:38 +0100 Subject: [PATCH] docs(19.6): v0.1.1 release recorded, finding F-1 deferred - security review: T-19.6-37 closed by sm-mfa-plugin v0.1.1, F-1 deferred with the corrected reason for the activate-by-code mint path - validation map: plan 04 Tasks 5 and 6 green - todo for F-1; deferred items: tag message resolved, F-1 deferred --- .../19.6-SECURITY-REVIEW.md | 17 ++++++++++------- .../19.6-VALIDATION.md | 6 +++--- .../deferred-items.md | 6 +++--- ...026-10-26-activate-by-code-second-factor.md | 18 ++++++++++++++++++ 4 files changed, 34 insertions(+), 13 deletions(-) create mode 100644 .planning/todos/pending/2026-10-26-activate-by-code-second-factor.md diff --git a/.planning/phases/19.6-two-factor-auth-and-device-flow-in-sm-user-plugin/19.6-SECURITY-REVIEW.md b/.planning/phases/19.6-two-factor-auth-and-device-flow-in-sm-user-plugin/19.6-SECURITY-REVIEW.md index d613b08a..843699e1 100644 --- a/.planning/phases/19.6-two-factor-auth-and-device-flow-in-sm-user-plugin/19.6-SECURITY-REVIEW.md +++ b/.planning/phases/19.6-two-factor-auth-and-device-flow-in-sm-user-plugin/19.6-SECURITY-REVIEW.md @@ -4,7 +4,7 @@ **Level:** OWASP ASVS L1 (RESEARCH "Security Domain"), block on high; D-14 asks for an explicit review of passwordless passkey login. **Scope:** this phase's sm-user-plugin additions (login gates, the login event, the password-change hook, the Reset 2FA slot, device authorization), the new `sm-mfa-plugin` (gate, challenge store, TOTP and QR, recovery and email codes, failure cap, trusted devices, settings, admin reset, WebAuthn second factor, passwordless passkeys), the social-login gate call in `sm-sociallogin-plugin`, the adoption in sm-demo-app, sm-bm-app and fonoteka.go, and this plan's gate. **Threats reviewed:** T-19.6-01..T-19.6-38 and the four T-19.6-SC rows of the threat models of plans 01, 02, 03 and 04, plus RESEARCH's Security Domain table and Assumptions A1-A13. -**Result:** every threat is closed by a test that fails when the mitigation is removed, or accepted with a reason. One defect found by the new tests was fixed in its own commit (sm-mfa-plugin `71d8f27`, T-19.6-24). One new finding, **F-1** (activate-by-code mints a token without the second factor when `require_activation` is off), contradicts the reason plan 01 recorded for an accepted mint path; it is open and raised at plan 04's Task 5 checkpoint. T-19.6-37 stays open until the Task 5 reply (one post-release fix to release). +**Result:** every threat is closed by a test that fails when the mitigation is removed, or accepted with a reason. One defect found by the new tests was fixed in its own commit (sm-mfa-plugin `71d8f27`, T-19.6-24). One new finding, **F-1** (activate-by-code mints a token without the second factor when `require_activation` is off), corrects the reason plan 01 recorded for an accepted mint path; the user deferred it at plan 04's Task 5 (todo `2026-10-26-activate-by-code-second-factor.md`). T-19.6-37 is closed: the fix shipped as sm-mfa-plugin v0.1.1 and every pin contains it. Repositories and abbreviations: **F** = summercms.go, **U** = sm-user-plugin (`fonoteka.go/plugins/golem15/user`), **M** = `sm-mfa-plugin`, **S** = `sm-sociallogin-plugin`, **D** = sm-demo-app (with `vue-demo-app`), **B** = sm-bm-app, **A** = the fonoteka.go workspace. @@ -29,7 +29,7 @@ Repositories and abbreviations: **F** = summercms.go, **U** = sm-user-plugin (`f |--------|------|--------|----------|-----| | 71d8f27 | M | T-19.6-24 | `TestSettings/form_round_trip_keeps_the_configuration` | The settings form shows a NULL `two_factor_available_methods` as an empty taglist and sends it back as `[]` on every save, so saving the form once (for example to turn trusted devices on) stored `[]` and made every method unavailable: TOTP setup answered 403, and no user could enable a second factor. An empty list is now an empty value: `Settings.BeforeValidate` stores it as NULL and the overlay ignores an empty stored list (methods and enforce groups), as the README already promised. README and the en and pl comments updated. | -The fix is a production change after plan 03's release (sm-mfa-plugin v0.1.0); it reaches the applications only through a patch release (T-19.6-37, plan 04 Tasks 5 and 6). +The fix is a production change after plan 03's release (sm-mfa-plugin v0.1.0); under the Task 5 reply it shipped as sm-mfa-plugin v0.1.1 (`d08f1ec`), pinned by sm-demo-app `9166a1e` (gitlink and go.mod) and sm-bm-app `fe4e292` (T-19.6-37). ## Threat register @@ -74,8 +74,8 @@ The fix is a production change after plan 03's release (sm-mfa-plugin v0.1.0); i | T-19.6-35 | ceremony state in the memory cooler on several instances (A11) | Documented: multi-instance applications use the Redis cooler; a ceremony that reaches another instance fails closed. | M README | `TestPasswordless/refused_challenges_and_bodies` (unknown state refused) (M) | accepted (README) | | T-19.6-SC (plan 03) | Go module installs | go-webauthn v0.18.2 after the recorded approval; fonoteka.go's graph has no go-webauthn. | M `go.mod`; A `go.mod` | gate `--mfa`, `--fonoteka` (F) | closed | | T-19.6-36 | the gate's verdict | The `go test -json` detector fails on fail, skip, zero matches, "no tests to run" and missing names; a subtest pass never counts for its parent; the vitest detector needs the named file; docker NOT RUN explicit and fatal with `PHASE196_REQUIRE_DOCKER=1`. | F `scripts/check-phase19.6.sh` | `scripts/check-phase19.6.sh --self-test` (planted output for every detector, the additive, pin and migration checks, the docker rule on a faked PATH) (F) | closed | -| T-19.6-37 | post-release fixes not reaching the applications | Task 4's table lists every production fix against every pin; Task 6 releases in D-17 order under the Task 5 reply and checks each pin with `merge-base --is-ancestor`; hold writes a todo. | 19.6-04-SUMMARY.md "Post-release fixes" | Task 6 merge-base checks | **open**: one row (M `71d8f27`) awaits the Task 5 reply | -| T-19.6-38 | a token mint path that skips the second factor (assumption-delta debt) | Every mint site of U and M listed with its reason and checked against the source; every first-factor path of a TOTP user driven to a challenge; passkey and device poll mint only through their own proof. | M `login_paths_test.go` | `TestEveryLoginPathIsGated` (M) | closed for the listed paths; **open finding F-1** for activate-by-code (below) | +| T-19.6-37 | post-release fixes not reaching the applications | Task 4's table lists every production fix against every pin; Task 6 releases in D-17 order under the Task 5 reply and checks each pin with `merge-base --is-ancestor`; hold writes a todo. | 19.6-04-SUMMARY.md "Post-release fixes" | Task 6: `merge-base --is-ancestor 71d8f27` exits 0 for tag v0.1.1, the sm-demo-app gitlink and go.mod require, and the sm-bm-app gitlink | closed (released as v0.1.1; sm-bm-app's push waits on another session's unpushed commits, see 19.6-04-SUMMARY.md) | +| T-19.6-38 | a token mint path that skips the second factor (assumption-delta debt) | Every mint site of U and M listed with its reason and checked against the source; every first-factor path of a TOTP user driven to a challenge; passkey and device poll mint only through their own proof. | M `login_paths_test.go` | `TestEveryLoginPathIsGated` (M) | closed for the listed paths; activate-by-code is accepted debt with the corrected reason of finding F-1 (deferred, below) | | T-19.6-SC (plan 04) | Go module installs | Plan 04 installs no module: no go.mod of U, M or S changed since the releases. | U, M, S `go.mod` | gate `--mfa` pins; `git diff` of the go.mod files since the release tags is empty | closed | ## D-14: passwordless passkey login @@ -145,14 +145,17 @@ None carries a token, code, secret, password, assertion or the device token. `Te - **JSON key order:** multi-key success bodies (the login challenge, `status`, `totp/setup`, `email/send`, the register answers) carry PHP's keys in Go's sorted order, not PHP's insertion order. JSON clients do not depend on key order, and the error bodies (single key or alphabetical in PHP too) and the 422 bodies (ordered explicitly) are byte-identical; recorded, not changed. - **Route constraint ordering (framework):** surf checks a `{id}` digit constraint inside the route's middleware, so an unauthenticated `DELETE …/trusted-devices/abc` answers jwt.auth's 401 where Laravel answered 404; with a session it is the router's 404. No information leaks either way. -## Open findings +## Deferred findings ### F-1: activate-by-code mints a session without the second factor when `require_activation` is off +**Status: deferred** by the user at plan 04 Task 5 (2026-10-26); tracked in `.planning/todos/pending/2026-10-26-activate-by-code-second-factor.md`. **Corrected reason for the accepted mint path:** activate-by-code skips the login gates; with the default `require_activation: true` a not-activated account cannot sign in, so it cannot have a second factor and nothing is skipped. With `require_activation: false` a not-activated account can enable one, and then possession of the mailed activation code stands in for the second factor once, at activation. + + - **What:** plan 01's assumption-delta decision accepts `POST /_user/api/v1/activate-by-code` as an ungated mint because "a new or not-yet-activated account cannot have 2FA yet". That holds with the default `golem15.user.activation.require_activation: true` (a not-activated account cannot sign in). With `require_activation: false` and `activate_mode: user`, a not-activated account signs in with its password, can enable TOTP, and a later `activate-by-code` with the mailed activation code answers `{"message":"Account activated","token":…}` without a challenge. - **Reproduced:** on this host through the mfa test harness (register in mode user, password login, TOTP setup and confirm, password login now challenged, `IssueActivationCode`, activate-by-code answers 200 with a token). - **Impact:** low. It needs a non-default configuration, a user who enabled a second factor before activating, and an attacker holding that user's unexpired activation code (their mailbox). It lets the mailbox alone stand in for the second factor, which a password reset does not (a reset mints no token). -- **Options (raised at plan 04 Task 5):** +- **Options (raised at plan 04 Task 5; the user chose (c)):** (a) fix in sm-user-plugin: after the code verifies, `ActivateByCode` asks the login gates (for example with source password, so sm-mfa-plugin v0.1.0's gate challenges it) and answers the gate's body instead of the token; the account is still activated. A patch release of sm-user-plugin and pointer bumps in sm-demo-app, sm-bm-app and fonoteka.go. (b) accept with the corrected reason ("only with `require_activation: false`; the mailed code stands in for the second factor") recorded in plan 01's decision and the README. (c) defer to a todo for a later phase. @@ -160,4 +163,4 @@ None carries a token, code, secret, password, assertion or the device token. `Te ## Sign-off -Every T-19.6 threat of plans 01-04 is closed by a passing test, accepted with its reason, or open with its decision pending at Task 5 (T-19.6-37, F-1 under T-19.6-38). `scripts/check-phase19.6.sh --all` result: see 19.6-04-SUMMARY.md. +Every T-19.6 threat of plans 01-04 is closed by a passing test or accepted with its reason; F-1 (under T-19.6-38) is deferred by the user with its corrected reason. `scripts/check-phase19.6.sh --all` passed with Docker (all 11 stages); see 19.6-04-SUMMARY.md. diff --git a/.planning/phases/19.6-two-factor-auth-and-device-flow-in-sm-user-plugin/19.6-VALIDATION.md b/.planning/phases/19.6-two-factor-auth-and-device-flow-in-sm-user-plugin/19.6-VALIDATION.md index 34a6ca0c..61dbf89a 100644 --- a/.planning/phases/19.6-two-factor-auth-and-device-flow-in-sm-user-plugin/19.6-VALIDATION.md +++ b/.planning/phases/19.6-two-factor-auth-and-device-flow-in-sm-user-plugin/19.6-VALIDATION.md @@ -61,8 +61,8 @@ Gate stages are `scripts/check-phase19.6.sh --`; a test named under a sta | 19.6-04-02 | 04 | 4 | SC-4, D-02, D-07, D-11, D-12a, D-12b, D-12c, D-15, D-16, D-18 | T-19.6-01..T-19.6-12 | Every user-plugin seam and device-flow state pinned by name | unit + integration | `--userplugin` and `--docker` (rows 19.6-01-01..04) | ✅ | ✅ green | | 19.6-04-03 | 04 | 4 | SC-4, D-05..D-10, D-12..D-16, D-18, D-19 | T-19.6-13..T-19.6-31, T-19.6-38 | Every second-factor decision, every PHP body and the login-path invariant pinned by name | unit + integration | `--mfa`, `--sociallogin` and `--docker` (rows 19.6-02-01..19.6-03-02) | ✅ | ✅ green | | 19.6-04-04 | 04 | 4 | SC-4, D-14 | all T-19.6 | PHP security test mapped; security review with the D-14 section; the whole gate green | unit + review | `check-phase19.6.sh --all`; `--mfa`: TestPHPMethodMap, TestPHPUser005; 19.6-SECURITY-REVIEW.md | ✅ | ✅ green | -| 19.6-04-05 | 04 | 4 | D-17 | T-19.6-37 | Go-ahead before releasing post-release fixes | manual (checkpoint) | reply recorded in 19.6-04-SUMMARY.md | ✅ | ⬜ pending (awaiting the reply) | -| 19.6-04-06 | 04 | 4 | D-17 | T-19.6-37 | Each post-release fix reaches every pin (`merge-base --is-ancestor`) or is a tracked todo | integration + manual | `check-phase19.6.sh --demo`, `--bm`, `--fonoteka` at the final pins | ✅ | ⬜ pending (after Task 5) | +| 19.6-04-05 | 04 | 4 | D-17 | T-19.6-37 | Go-ahead before releasing post-release fixes | manual (checkpoint) | reply recorded in 19.6-04-SUMMARY.md ("release"; F-1 deferred) | ✅ | ✅ green | +| 19.6-04-06 | 04 | 4 | D-17 | T-19.6-37 | Each post-release fix reaches every pin (`merge-base --is-ancestor`) or is a tracked todo | integration + manual | `check-phase19.6.sh --demo`, `--bm`, `--fonoteka` at the final pins; merge-base results in 19.6-04-SUMMARY.md | ✅ | ✅ green | *Status: ⬜ pending · ✅ green · ❌ red · ⚠️ flaky* @@ -95,4 +95,4 @@ Gate stages are `scripts/check-phase19.6.sh --`; a test named under a sta - [x] Feedback latency < 60s - [x] `nyquist_compliant` set to true in the frontmatter -**Approval:** validated 2026-10-26 (plan 19.6-04, `check-phase19.6.sh --all`); Tasks 5 and 6 of plan 04 await the release decision. +**Approval:** validated 2026-10-26 (plan 19.6-04, `check-phase19.6.sh --all`; post-release fix released as sm-mfa-plugin v0.1.1). diff --git a/.planning/phases/19.6-two-factor-auth-and-device-flow-in-sm-user-plugin/deferred-items.md b/.planning/phases/19.6-two-factor-auth-and-device-flow-in-sm-user-plugin/deferred-items.md index 39db2834..fc74b6e9 100644 --- a/.planning/phases/19.6-two-factor-auth-and-device-flow-in-sm-user-plugin/deferred-items.md +++ b/.planning/phases/19.6-two-factor-auth-and-device-flow-in-sm-user-plugin/deferred-items.md @@ -18,7 +18,7 @@ status: open **What:** the module resolves through `replace => ./plugins/golem15/user`, so builds and tests use the v0.1.12 checkout. 19.6-03 Task 6 staged only the paths the plan names. Precedent `06bc9d0` raised the require together with the pointer. Raise it to v0.1.12 (and tidy) in a follow-up `chore:` commit. - sm-mfa-plugin: the v0.1.0 tag message says the two-factor mode is "optional, required or disabled" - status: open + status: resolved (2026-10-26: the v0.1.1 tag message names the modes disabled, optional and enforced) **What:** the config values are `disabled`, `optional` and `enforced`. The tag is pushed and is not rewritten (that would need a force-push). The README and `config/config.yaml` are correct. The next release's tag message should name the right values. - sm-demo-app: the release build needs read access to the private sm-mfa-plugin repository status: resolved (2026-10-26: the user made golem15/sm-mfa-plugin public; anonymous HTTPS `git ls-remote` of v0.1.0 works) @@ -33,5 +33,5 @@ status: open (observation) **What:** found by 19.6-04 `TestPHPErrorBodies`. Bodies built from Go maps (the login challenge, `2fa/status`, `totp/setup`, `email/send`, the WebAuthn register answer, the device initiate and status bodies) list their keys alphabetically, where PHP's `response()->json([...])` kept insertion order. Values and key sets match PHP; error bodies and the 422 bodies are byte-identical. JSON clients do not depend on key order, so nothing was changed; recorded in 19.6-SECURITY-REVIEW.md "Residual risks" for a decision if a client ever compares raw bytes. - sm-user-plugin: activate-by-code mints a session without the second factor when `require_activation` is off (finding F-1) - status: open (decision at plan 04 Task 5) - **What:** see 19.6-SECURITY-REVIEW.md "Open findings". + status: deferred (user decision at plan 04 Task 5) + **What:** see 19.6-SECURITY-REVIEW.md "Deferred findings" and the todo `.planning/todos/pending/2026-10-26-activate-by-code-second-factor.md`. diff --git a/.planning/todos/pending/2026-10-26-activate-by-code-second-factor.md b/.planning/todos/pending/2026-10-26-activate-by-code-second-factor.md new file mode 100644 index 00000000..0eb3bacb --- /dev/null +++ b/.planning/todos/pending/2026-10-26-activate-by-code-second-factor.md @@ -0,0 +1,18 @@ +--- +title: activate-by-code mints a session without the second factor when require_activation is off (finding F-1) +date: 2026-10-26 +priority: low +area: sm-user-plugin +--- + +sm-user-plugin's `POST /_user/api/v1/activate-by-code` signs the account in (`mintFor`) without asking the login gates. Phase 19.6 plan 01 accepted that path because "a new or not-yet-activated account cannot have 2FA yet". That reason holds only with the default `golem15.user.activation.require_activation: true`. With `require_activation: false` and `activate_mode: user`, a not-activated account signs in with its password, can enable TOTP (sm-mfa-plugin), and a later activate-by-code with the mailed activation code answers `{"message":"Account activated","token":…}` with no challenge. The mailbox alone then stands in for the second factor, which a password reset does not allow (a reset mints no token). + +Found and reproduced by 19.6-04 (security review finding F-1, 19.6-SECURITY-REVIEW.md "Open findings"): register in mode user, password login, TOTP setup and confirm, the next password login is challenged, `IssueActivationCode`, activate-by-code answers 200 with a token. + +The user deferred it at 19.6-04 Task 5 (2026-10-26). Impact is low: it needs the non-default configuration, a user who enabled a second factor before activating, and the user's unexpired activation code. + +To fix later: +- In `ActivateByCode`, after `VerifyActivationCode` succeeds, call `CheckLoginGates` for the user (a source the mfa gate challenges, for example `LoginSourcePassword`, or a new `LoginSourceActivation` together with an sm-mfa-plugin gate change) and answer the gate's body instead of the token; the account stays activated. +- Add a test in sm-user-plugin (a gate that challenges, then activate-by-code answers the challenge) and an end-to-end case in sm-mfa-plugin's `TestEveryLoginPathIsGated`, and update its `tokenMints` reason. +- Release sm-user-plugin as a patch and bump sm-demo-app, sm-bm-app and fonoteka.go in separate commits (D-17 order). +- Until then, `TestEveryLoginPathIsGated/mint_sites` lists `ActivateByCode` with a pointer to F-1, so any change to the mint sites fails until the list is updated.