fix(b3-1): Codex round 3 — exclude the +2 TOTP step, sequence auth B3-9 after B3-2, keep the replay claim in OC-0378's remedy

- wrongTOTPCode excludes {-1,0,+1,+2}: the verifier samples the clock after
  the helper, so a step boundary in between shifts its window to {0,+1,+2}
- B3-9 "Parallel with": OC-0345/0346 any; OC-0323 with B3-8; OC-0376..0378
  after B3-2 (matches the safe-parallelism rule and the section text)
- OC-0378 suggestedFix: a restored challenge must carry the accepted
  verification or roll back the MarkUsed claim, or the retry is a replay

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Rg9QQWVN3E5UUgBD2dydtu
This commit is contained in:
J3vb
2026-08-29 21:05:39 +02:00
co-authored by Claude Fable 5
parent bf49453cc1
commit 8614603bba
3 changed files with 21 additions and 19 deletions
+1 -1
View File
@@ -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",
+7 -5
View File
@@ -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 ""
}
@@ -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)** | 12 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 | 23 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 | 12 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 | 23 days | after B3-3/B3-4 |
| **B3-6** | Guardrails: coverage floor (S-06), hub simulation + fault transport + fuzz seeds, benchmarks, rules | 34 days | B3-0..B3-2 |
| **B3-7** | Alpha-shaped test dataset: seed profile + anonymised `v1.2.0-alpha.4` snapshot | 12 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)** | 12 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 | 23 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 | 12 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 | 23 days | after B3-3/B3-4 |
| **B3-6** | Guardrails: coverage floor (S-06), hub simulation + fault transport + fuzz seeds, benchmarks, rules | 34 days | B3-0..B3-2 |
| **B3-7** | Alpha-shaped test dataset: seed profile + anonymised `v1.2.0-alpha.4` snapshot | 12 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