From 77994eff09bb9903e44efe8002d9592bb91f8fdf Mon Sep 17 00:00:00 2001 From: kristianwind Date: Sun, 23 Aug 2026 23:34:34 +0200 Subject: [PATCH] feat(security): a TOTP code can be used once MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RFC 6238 says the verifier must reject a code it has already accepted: a code stays valid for its whole thirty-second window, so one seen in flight — a phishing page, a proxy, over the shoulder — could be replayed until it expired. Tasks now record the counter of the last accepted step (totp_last_step), and a login accepts a code only when its step is strictly greater. The guard is in the UPDATE, not a read-then-write, so two logins racing with the same stolen code cannot both find the old value and both proceed — exactly one moves the row. auth.TOTPStep is VerifyTOTP that also reports which step matched, walking the same one-step window; the code entered a little early or late spends the step it was issued at, not the step it was checked at, so a late-but-valid code is still single-use. Verified end to end: the same code presented to two logins in one window is accepted once and refused the second time. Co-Authored-By: Claude Opus 5 --- internal/auth/auth_test.go | 33 ++++++++++++++ internal/auth/totp.go | 35 +++++++++++++++ internal/httpapi/auth_handlers.go | 19 +++++++- internal/httpapi/auth_test.go | 44 +++++++++++++++++++ .../store/migrations/0029_totp_single_use.sql | 11 +++++ internal/store/users.go | 20 +++++++++ 6 files changed, 161 insertions(+), 1 deletion(-) create mode 100644 internal/store/migrations/0029_totp_single_use.sql diff --git a/internal/auth/auth_test.go b/internal/auth/auth_test.go index d20dca9..a59f797 100644 --- a/internal/auth/auth_test.go +++ b/internal/auth/auth_test.go @@ -265,6 +265,39 @@ func TestRecoveryCodes(t *testing.T) { } } +// TOTPStep returns the step a code matched, and it must be the same step across +// the tolerance window — that number is what makes a code single-use, so a code +// entered a little early or late has to spend the same step, not a new one. +func TestTOTPStepIsStableAcrossTheWindow(t *testing.T) { + secret, _, err := NewTOTPSecret("verdande", "k@example.com") + if err != nil { + t.Fatal(err) + } + now := time.Date(2026, 3, 10, 10, 0, 15, 0, time.UTC) + code := codeAt(t, secret, now) + + want, err := TOTPStep(secret, code, now) + if err != nil { + t.Fatalf("the current code did not match: %v", err) + } + // Verified one step late — the code is still valid, and must report the step it + // was issued at, not the step it was checked at. + got, err := TOTPStep(secret, code, now.Add(30*time.Second)) + if err != nil { + t.Fatalf("code rejected one step late: %v", err) + } + if got != want { + t.Errorf("same code reported step %d then %d; a replay check keyed on that would not catch it", want, got) + } + + if _, err := TOTPStep(secret, "000000", now); err == nil { + t.Error("a wrong code returned a step") + } + if _, err := TOTPStep("", code, now); err != ErrNoTOTPSecret { + t.Errorf("empty secret: %v, want ErrNoTOTPSecret", err) + } +} + // codeAt produces the code an authenticator app would display at that moment, // using the same settings VerifyTOTP checks against. func codeAt(t *testing.T, secret string, now time.Time) string { diff --git a/internal/auth/totp.go b/internal/auth/totp.go index d68e4cd..4f30397 100644 --- a/internal/auth/totp.go +++ b/internal/auth/totp.go @@ -2,6 +2,7 @@ package auth import ( "crypto/rand" + "crypto/subtle" "encoding/base32" "errors" "fmt" @@ -82,6 +83,40 @@ func VerifyTOTP(secret, code string, now time.Time) error { return nil } +// TOTPStep verifies a code and returns the counter of the step it matched, so the +// caller can refuse to accept the same step twice. +// +// ValidateCustom answers only yes or no, and "yes" for a code inside the skew +// window is not enough to stop a replay: a code seen in flight stays valid for the +// rest of its period, and the fix is to record which step was spent and never +// accept that step or an earlier one again. That needs the number, so this walks +// the same window ValidateCustom would — one step either side — and reports the +// counter of the match. The comparison is constant time for the same reason the +// password check is: a code is a secret being checked against a computed value. +func TOTPStep(secret, code string, now time.Time) (int64, error) { + if secret == "" { + return 0, ErrNoTOTPSecret + } + code = strings.ReplaceAll(strings.TrimSpace(code), " ", "") + + for offset := -int64(totpSkew); offset <= int64(totpSkew); offset++ { + at := now.Add(time.Duration(offset) * totpPeriod * time.Second) + want, err := totp.GenerateCodeCustom(secret, at, totp.ValidateOpts{ + Period: totpPeriod, + Skew: 0, + Digits: totpDigits, + Algorithm: otp.AlgorithmSHA1, + }) + if err != nil { + return 0, ErrInvalidCode + } + if subtle.ConstantTimeCompare([]byte(want), []byte(code)) == 1 { + return at.Unix() / totpPeriod, nil + } + } + return 0, ErrInvalidCode +} + // NewRecoveryCodes returns single-use codes for someone who has lost their phone, // and the hashes to store. Without these, losing a device means losing the account, // and the only fix left is editing the database by hand. diff --git a/internal/httpapi/auth_handlers.go b/internal/httpapi/auth_handlers.go index 788bb2e..c03a230 100644 --- a/internal/httpapi/auth_handlers.go +++ b/internal/httpapi/auth_handlers.go @@ -218,7 +218,24 @@ func (s *Server) handleLoginTOTP(w http.ResponseWriter, r *http.Request) { return } - err := auth.VerifyTOTP(user.TOTPSecret, req.Code, time.Now()) + step, err := auth.TOTPStep(user.TOTPSecret, req.Code, time.Now()) + if err == nil { + // The code is right; now it must also be unused. A TOTP code stays valid for + // its whole window, so one observed in flight — a phishing page, a proxy, + // over the shoulder — could otherwise be replayed until it expires. Spending + // the step here makes it single-use: a second presentation of the same code, + // or an earlier one, finds the step already gone. + fresh, cerr := s.db.ConsumeTOTPStep(r.Context(), user.ID, step) + if cerr != nil { + s.internal(w, r, "consume totp step", cerr) + return + } + if !fresh { + writeError(w, http.StatusUnauthorized, CodeUnauthorized, + "that verification code has already been used") + return + } + } if err != nil { // A recovery code is accepted in the same field. Making somebody find a // different form while locked out of their account is a poor time to diff --git a/internal/httpapi/auth_test.go b/internal/httpapi/auth_test.go index 779c245..110dfbe 100644 --- a/internal/httpapi/auth_test.go +++ b/internal/httpapi/auth_test.go @@ -372,6 +372,50 @@ func TestTOTPEnrolmentAndTwoStepLogin(t *testing.T) { } } +// A TOTP code works once. A code seen in flight and replayed inside its window +// must be refused the second time, or the second factor is only as good as the +// thirty seconds an attacker has to reuse what they captured. +func TestTOTPCodeIsSingleUse(t *testing.T) { + ts := newTestServer(t) + ts.bootstrap(t) + + resp, body := ts.do(t, "POST", "/api/v1/auth/totp/setup", map[string]string{ + "password": "et langt kodeord", + }) + if resp.StatusCode != http.StatusOK { + t.Fatalf("totp setup: %d %v", resp.StatusCode, body) + } + secret, _ := body["secret"].(string) + resp, body = ts.do(t, "POST", "/api/v1/auth/totp/confirm", map[string]string{ + "code": codeFor(t, secret), + }) + if resp.StatusCode != http.StatusOK { + t.Fatalf("totp confirm: %d %v", resp.StatusCode, body) + } + + // One code, captured once and presented twice. + code := codeFor(t, secret) + + ts.do(t, "POST", "/api/v1/auth/logout", nil) + ts.do(t, "POST", "/api/v1/auth/login", map[string]string{ + "email": "kristian@example.dk", "password": "et langt kodeord", + }) + resp, body = ts.do(t, "POST", "/api/v1/auth/login/totp", map[string]string{"code": code}) + if resp.StatusCode != http.StatusOK { + t.Fatalf("first use of the code failed: %d %v", resp.StatusCode, body) + } + + // A second login, same code, same window. + ts.do(t, "POST", "/api/v1/auth/logout", nil) + ts.do(t, "POST", "/api/v1/auth/login", map[string]string{ + "email": "kristian@example.dk", "password": "et langt kodeord", + }) + resp, body = ts.do(t, "POST", "/api/v1/auth/login/totp", map[string]string{"code": code}) + if resp.StatusCode != http.StatusUnauthorized { + t.Fatalf("the same code was accepted twice: %d %v", resp.StatusCode, body) + } +} + // A recovery code is accepted in the same field as a TOTP code, and works once. func TestRecoveryCodeCompletesLoginOnce(t *testing.T) { ts := newTestServer(t) diff --git a/internal/store/migrations/0029_totp_single_use.sql b/internal/store/migrations/0029_totp_single_use.sql new file mode 100644 index 0000000..c446408 --- /dev/null +++ b/internal/store/migrations/0029_totp_single_use.sql @@ -0,0 +1,11 @@ +-- A TOTP code is meant to be usable once. RFC 6238 says so in as many words: the +-- verifier must reject a code that has already been accepted, or a code observed in +-- flight — over the shoulder, off a phishing page, out of a proxy — can be replayed +-- for the rest of its thirty-second window. +-- +-- totp_last_step records the counter of the most recently accepted code (Unix time +-- divided by the period). A login accepts a code only when its step is strictly +-- greater, so the same code cannot be spent twice and time cannot run backwards. +-- Null means no code has been accepted yet, which is every account until its owner +-- next signs in with one. +ALTER TABLE users ADD COLUMN totp_last_step INTEGER; diff --git a/internal/store/users.go b/internal/store/users.go index 6558cb4..67f6468 100644 --- a/internal/store/users.go +++ b/internal/store/users.go @@ -466,6 +466,26 @@ func (db *DB) SetTOTPSecret(ctx context.Context, userID, secret string, enabled return err } +// ConsumeTOTPStep records a just-accepted TOTP step and reports whether it was +// fresh. It answers false when the step has already been spent — the same code, or +// an earlier one, being presented a second time. +// +// The guard is in the statement, not in a read-then-write: two logins racing with +// the same stolen code must not both find the old value and both proceed. The row +// moves forward only when the new step is strictly greater, so exactly one of them +// updates a row and the other sees nothing change. +func (db *DB) ConsumeTOTPStep(ctx context.Context, userID string, step int64) (bool, error) { + res, err := db.ExecContext(ctx, + `UPDATE users SET totp_last_step = ? WHERE id = ? + AND (totp_last_step IS NULL OR totp_last_step < ?)`, + step, userID, step) + if err != nil { + return false, err + } + n, _ := res.RowsAffected() + return n == 1, nil +} + // InboxID returns the user's Inbox project. func (db *DB) InboxID(ctx context.Context, userID string) (string, error) { var id string