diff --git a/.superpowers/findings-ledger.json b/.superpowers/findings-ledger.json index 8680db6a..76ae2c53 100644 --- a/.superpowers/findings-ledger.json +++ b/.superpowers/findings-ledger.json @@ -8528,7 +8528,7 @@ "why": "handleVerifyTOTP calls partialStore.Consume before issueSession. When CreateSession fails the handler answers 500 \"failed to create session\", but the challenge is already gone (and the code is marked used by VerifyTOTPCodeOnce), so the only way forward is a fresh POST /login with the password. A verified second factor is discarded because of a persistence hiccup that has nothing to do with the credential. Same shape as OC-0376 on the register path.", "repro": "cd Server && go test -count=1 -run 'TestAuthCharacterization_VerifyTOTPFailurePaths/session_insert_fails' ./api/ — the row installs a BEFORE INSERT ON sessions trigger, verifies a valid code, pins the 500, drops the trigger and pins that the same partial token is now refused with 401.", "evidence": "Server/api/totp_handler.go:107 if _, ok := partialStore.Consume(partialToken); !ok { // challenge gone here\nServer/api/totp_handler.go:115 token, err := issueSession(r.Context(), database, user.ID, challenge.Device, challenge.IP) // fails after it", - "suggestedFix": "Keep the claim atomic and first: Consume the challenge before issuing the session (as today), then on CreateSession failure re-issue or restore the challenge for the same user/device/IP so the verified second factor is not discarded. Do NOT issue the session before Consume: two concurrent requests holding the same partial token can pass Lookup with different valid codes from the +/-1 step window (VerifyTOTPCodeOnce tracks (user, code), not the token), both would create sessions, and the losing Consume would leave an unreturned bearer session in the database. If the order must change, the loser has to revoke the session it created. Belongs to the AuthService in B3-2/B3-9.", + "suggestedFix": "Keep the claim atomic and first: Consume the challenge before issuing the session (as today), then on CreateSession failure re-issue or restore the challenge for the same user/device/IP so the verified second factor is not discarded. The restore must also keep the accepted verification usable: VerifyTOTPCodeOnce has already recorded (user, code) in UsedTOTPCodeStore for 90 s, so an immediate retry with the authenticator's still-current code would be refused as a replay - either carry the verified state on the restored challenge (retry issues the session without a new code) or roll back that MarkUsed claim together with the challenge. Do NOT issue the session before Consume: two concurrent requests holding the same partial token can pass Lookup with different valid codes from the +/-1 step window (the used-code store keys on (user, code), not the token), both would create sessions, and the losing Consume would leave an unreturned bearer session in the database; if the order must change, the loser has to revoke the session it created. Belongs to the AuthService in B3-2/B3-9.", "status": "open", "found": "2026-08-29", "hunt": "b3-1-auth-characterization-2026-08-29", diff --git a/Server/api/auth_characterization_test.go b/Server/api/auth_characterization_test.go index bc1fea9f..ae15ca72 100644 --- a/Server/api/auth_characterization_test.go +++ b/Server/api/auth_characterization_test.go @@ -224,25 +224,27 @@ func totpCode(t *testing.T, secret string) string { // wrongTOTPCode returns a six-digit code the verifier rejects for secret: // it differs from the codes of the previous, current and next 30-second -// steps, which are the three VerifyTOTPCode accepts. A constant such as -// "000000" collides with one of them once in ~333k runs. +// steps, which are the three VerifyTOTPCode accepts, and from the step after +// that — the verifier samples the clock later than this helper, so a request +// that crosses a step boundary in between is checked against {0,+1,+2}. A +// constant such as "000000" collides with one of them once in ~333k runs. func wrongTOTPCode(t *testing.T, secret string) string { t.Helper() now := time.Now().UTC() valid := map[string]bool{} - for _, d := range []time.Duration{-30 * time.Second, 0, 30 * time.Second} { + for _, d := range []time.Duration{-30 * time.Second, 0, 30 * time.Second, 60 * time.Second} { code, err := auth.GenerateTOTPCode(secret, now.Add(d)) if err != nil { t.Fatalf("GenerateTOTPCode: %v", err) } valid[code] = true } - for i := range 4 { + for i := range 5 { if code := fmt.Sprintf("%06d", i); !valid[code] { return code } } - t.Fatal("unreachable: four candidates, at most three valid codes") + t.Fatal("unreachable: five candidates, at most four valid codes") return "" } diff --git a/docs/plans/b3-server-architecture-guardrails-2026-08-29.md b/docs/plans/b3-server-architecture-guardrails-2026-08-29.md index ff50626a..d57a5a4b 100644 --- a/docs/plans/b3-server-architecture-guardrails-2026-08-29.md +++ b/docs/plans/b3-server-architecture-guardrails-2026-08-29.md @@ -31,19 +31,19 @@ surface to it. ## Steps at a glance -| Step | What | Size | Parallel with | -| -------- | ---------------------------------------------------------------------------------------------------------------------------------- | -------- | ---------------------- | -| **B3-0** | Boundary inventory: every upper-layer `db` import with a disposition; hub lifecycle; before-graph — **DONE 2026-08-29 (PR #1448)** | 1–2 days | B3-6, B3-7 | -| **B3-1** | Auth characterization tests — enumeration, sentinels, sessions, TOTP, rate limits, failure paths | 1 day | B3-6, B3-7 | -| **B3-2** | The auth vertical slice (S-10): route → `service.AuthService` → `db`, behaviour-neutral | 2–3 days | B3-6, B3-7 | -| **HP-3** | First vertical-slice review — scorecard | — | — | -| **B3-3** | Lifecycle extraction: `main.go` → `internal/app/` with one composite close contract | 1–2 days | B3-4 | -| **B3-4** | Hub constructor options (S-11): required collaborators validated at construction | 1 day | after B3-3 | -| **B3-5** | `ws` in-package split (S-08): responsibilities into named files, pure moves + adjacent rewrites | 2–3 days | after B3-3/B3-4 | -| **B3-6** | Guardrails: coverage floor (S-06), hub simulation + fault transport + fuzz seeds, benchmarks, rules | 3–4 days | B3-0..B3-2 | -| **B3-7** | Alpha-shaped test dataset: seed profile + anonymised `v1.2.0-alpha.4` snapshot | 1–2 days | B3-0..B3-2 | -| **B3-8** | Remaining domain families behind services (S-09), one PR each; S-03/S-04 fold into the channel family | spread | after HP-3, per-family | -| **B3-9** | The B3-tagged findings: OC-0323, OC-0345, OC-0346 + B3-1's OC-0376, OC-0377, OC-0378 (test-first, `bughunt-fix` shape) | 1 day | any | +| Step | What | Size | Parallel with | +| -------- | ---------------------------------------------------------------------------------------------------------------------------------- | -------- | ---------------------------------------------------------------- | +| **B3-0** | Boundary inventory: every upper-layer `db` import with a disposition; hub lifecycle; before-graph — **DONE 2026-08-29 (PR #1448)** | 1–2 days | B3-6, B3-7 | +| **B3-1** | Auth characterization tests — enumeration, sentinels, sessions, TOTP, rate limits, failure paths | 1 day | B3-6, B3-7 | +| **B3-2** | The auth vertical slice (S-10): route → `service.AuthService` → `db`, behaviour-neutral | 2–3 days | B3-6, B3-7 | +| **HP-3** | First vertical-slice review — scorecard | — | — | +| **B3-3** | Lifecycle extraction: `main.go` → `internal/app/` with one composite close contract | 1–2 days | B3-4 | +| **B3-4** | Hub constructor options (S-11): required collaborators validated at construction | 1 day | after B3-3 | +| **B3-5** | `ws` in-package split (S-08): responsibilities into named files, pure moves + adjacent rewrites | 2–3 days | after B3-3/B3-4 | +| **B3-6** | Guardrails: coverage floor (S-06), hub simulation + fault transport + fuzz seeds, benchmarks, rules | 3–4 days | B3-0..B3-2 | +| **B3-7** | Alpha-shaped test dataset: seed profile + anonymised `v1.2.0-alpha.4` snapshot | 1–2 days | B3-0..B3-2 | +| **B3-8** | Remaining domain families behind services (S-09), one PR each; S-03/S-04 fold into the channel family | spread | after HP-3, per-family | +| **B3-9** | The B3-tagged findings: OC-0323, OC-0345, OC-0346 + B3-1's OC-0376, OC-0377, OC-0378 (test-first, `bughunt-fix` shape) | 1 day | OC-0345/0346: any; OC-0323: with B3-8; OC-0376..0378: after B3-2 | Order: B3-0 → B3-1 → B3-2 → **HP-3** → B3-3 → B3-4 → B3-5 → B3-8. B3-6, B3-7 and B3-9 run beside the slice (roadmap "Safe parallelism": guardrail tooling