fix(auth): andenfaktoren kunne gættes igennem, og et token kunne tilmelde sin egen

To huller fra sikkerhedsgennemgangen, og de er samme historie fortalt to gange:
den ene kontrol, der findes til når adgangskoden allerede er tabt, kunne omgås af
netop den, der havde adgangskoden.

**H1 — spanden blev fyldt af adgangskoden alene.** `handleLogin` nulstillede
ratebegrænseren ved hver korrekt adgangskode, *før* den så på, om der var en
andenfaktor. Det gav én, der havde kodeordet, ubegrænsede portioner på ti gæt:
log ind, få en frisk spand og en frisk ventende session, brug ti gæt på koden, log
ind igen. Med tre gyldige koder i ethvert øjeblik og tyve forespørgsler i
sekundet er det lige odds inden for en eftermiddag.

Spanden ryddes nu først, når login er *færdigt*, og der er kommet en spand mere:
koder tælles pr. konto og ikke kun pr. adresse. Adressespanden er slået af enhver
med mere end én adresse, hvilket er det almindelige — en telefon og en bærbar er
allerede to. Det, der gør en sekscifret kode sikker, er at gæt er en knap
ressource, og knapheden skal tælles imod det, der bliver angrebet.

Den ventende session får lov at leve. At afslutte den ved første forkerte ciffer
blev overvejet og valgt fra: det ville koste en, der taster ét tal forkert, sin
adgangskode igen, og det køber næsten ingenting oven i loftet pr. konto.

**M3 — et lækket token kunne tilmelde sin egen andenfaktor.** `/auth/totp/setup`
lå kun bag `requireAuth` og spurgte om ingenting, mens `disable` altid har
krævet adgangskoden: døren åbnede med et token og lukkede kun med et kodeord. Et
stjålet token kunne hente hemmeligheden, lægge den i sin egen godkender, bekræfte
den og beholde den eneste kopi af gendannelseskoderne. Ejerens næste login ville
stoppe ved en kode, de ikke havde, og de kunne ikke slå den fra uden at logge ind
først. Et tyveri bliver til et lejemål — den sætning står allerede i filen ved
siden af om passkeys, og det er den samme regel.

Nu bag `requireSession` sammen med passkeys og sessionerne, og `setup` spørger om
adgangskoden ligesom `disable`. Symmetrien er pointen: at slå den til er mindst
lige så indgribende som at slå den fra.

Og en note kan oprettes inde fra et projekt. At ligge i et projekt *er* måden en
note deles på, så det er én handling frem for to — man skulle før oprette noten
under Noter og bagefter finde projektet frem i "Del i". Ruden findes nu, også før
der er en eneste note, for ellers er der ingen knap dér, hvor man opdager at der
skal skrives noget ned.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

internal/httpapi/auth_handlers.go

@@ -154,7 +154,21 @@ func (s *Server) handleLogin(w http.ResponseWriter, r *http.Request) {
 		}
 	}
 
-	s.loginLimiter.reset(clientIP(r))
+	// The bucket is cleared only when the login is *finished*.
+	//
+	// It used to be cleared here, on the strength of the password alone — before
+	// the second factor had been asked for at all. That handed somebody who
+	// already had the password an unlimited supply of ten-guess batches: log in,
+	// get a fresh bucket and a fresh pending session, spend ten guesses on the
+	// code, log in again. At three valid codes in any moment and twenty requests a
+	// second, that is even odds inside an afternoon — against the one control that
+	// exists precisely for when the password is already gone.
+	//
+	// So an account with a second factor clears nothing yet. handleLoginTOTP does
+	// it, once the code is right.
+	if !user.TOTPEnabled {
+		s.loginLimiter.reset(clientIP(r))
+	}
 
 	// A session that still needs a code gets a short life of its own: long enough
 	// to fetch a phone, not long enough to be worth stealing.
@@ -191,6 +205,19 @@ func (s *Server) handleLoginTOTP(w http.ResponseWriter, r *http.Request) {
 	user := userFrom(r.Context())
 	session := sessionFrom(r.Context())
 
+	// Counted per account as well as per address.
+	//
+	// The address bucket alone is defeated by anybody with more than one, which is
+	// the ordinary case — a phone and a laptop is already two. A code is six
+	// digits: the only thing that makes it safe is that guesses are scarce, and
+	// scarcity has to be counted against the thing being attacked.
+	if ok, retry := s.totpLimiter.allow(user.ID); !ok {
+		w.Header().Set("Retry-After", retryAfterSeconds(retry))
+		writeError(w, http.StatusTooManyRequests, CodeRateLimited,
+			"too many verification codes; wait a moment and try again")
+		return
+	}
+
 	err := auth.VerifyTOTP(user.TOTPSecret, req.Code, time.Now())
 	if err != nil {
 		// A recovery code is accepted in the same field. Making somebody find a
@@ -198,6 +225,15 @@ func (s *Server) handleLoginTOTP(w http.ResponseWriter, r *http.Request) {
 		// introduce a second concept.
 		used, rerr := s.db.UseRecoveryCode(r.Context(), user.ID, req.Code)
 		if rerr != nil || !used {
+			// The pending session is left alive on purpose.
+			//
+			// Ending it on the first wrong digit was considered and rejected: it
+			// would make somebody who mistyped one number type their password again,
+			// and it buys almost nothing. What makes a six-digit code safe is that
+			// guesses are scarce, and the per-account bucket above is what makes
+			// them scarce — ten an hour, however many times the password is
+			// re-entered. The attack this closes was never "one session, many
+			// guesses"; it was "a fresh bucket for every password check".
 			writeError(w, http.StatusUnauthorized, CodeUnauthorized,
 				"that verification code is not valid")
 			return
@@ -205,6 +241,11 @@ func (s *Server) handleLoginTOTP(w http.ResponseWriter, r *http.Request) {
 		s.log.Info("recovery code used", "user", user.ID)
 	}
 
+	// Finished. Now the buckets may be cleared — both of them, so somebody who
+	// fumbled the code twice is not still waiting once they get it right.
+	s.loginLimiter.reset(clientIP(r))
+	s.totpLimiter.reset(user.ID)
+
 	if err := s.db.PromoteSession(r.Context(), session.ID, s.cfg.SessionTTL); err != nil {
 		s.internal(w, r, "promote session", err)
 		return

internal/httpapi/auth_test.go

@@ -301,7 +301,11 @@ func TestTOTPEnrolmentAndTwoStepLogin(t *testing.T) {
 
 	// Begin: a secret exists, but 2FA is not on yet — closing the tab here must
 	// not lock the account.
-	resp, body := ts.do(t, "POST", "/api/v1/auth/totp/setup", nil)
+	// Med adgangskoden: at slå andenfaktoren til er mindst lige så indgribende som
+	// at slå den fra, og den har altid spurgt.
+	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)
 	}
@@ -368,7 +372,9 @@ func TestRecoveryCodeCompletesLoginOnce(t *testing.T) {
 	ts := newTestServer(t)
 	ts.bootstrap(t)
 
-	_, body := ts.do(t, "POST", "/api/v1/auth/totp/setup", nil)
+	_, body := ts.do(t, "POST", "/api/v1/auth/totp/setup", map[string]string{
+		"password": "et langt kodeord",
+	})
 	secret := body["secret"].(string)
 	_, body = ts.do(t, "POST", "/api/v1/auth/totp/confirm", map[string]string{
 		"code": codeFor(t, secret),
@@ -897,3 +903,65 @@ func TestUserAgentsAreSummarisedForAPerson(t *testing.T) {
 		}
 	}
 }
+
+// Andenfaktoren kan ikke slås til uden adgangskoden.
+//
+// Døren åbnede før med et token og lukkede kun med et kodeord: `disable` har
+// altid spurgt, `setup` spurgte om ingenting. Et stjålet token kunne derfor hente
+// hemmeligheden, lægge den i sin egen godkender, bekræfte den og beholde den
+// eneste kopi af gendannelseskoderne — hvorefter ejerens næste login stoppede ved
+// en kode, de ikke havde, og de ikke kunne slå den fra uden at logge ind først.
+func TestTOTPCannotBeSwitchedOnWithoutThePassword(t *testing.T) {
+	ts := newTestServer(t)
+	ts.bootstrap(t)
+
+	resp, _ := ts.do(t, "POST", "/api/v1/auth/totp/setup", map[string]string{
+		"password": "det forkerte kodeord",
+	})
+	if resp.StatusCode != http.StatusUnprocessableEntity {
+		t.Fatalf("status %d, ventede at en forkert adgangskode blev afvist", resp.StatusCode)
+	}
+
+	resp, _ = ts.do(t, "POST", "/api/v1/auth/totp/setup", map[string]string{})
+	if resp.StatusCode == http.StatusOK {
+		t.Fatal("en tom adgangskode blev accepteret")
+	}
+}
+
+// Ratebegrænseren må ikke nulstilles af adgangskoden alene.
+//
+// Den blev det, og det gav én, der havde kodeordet, ubegrænsede portioner på ti
+// gæt: log ind, få en frisk spand og en frisk ventende session, brug ti gæt på
+// koden, log ind igen. Andenfaktoren findes præcis til det tilfælde, hvor
+// kodeordet allerede er tabt, så den portion skal betales for hver gang.
+func TestAPasswordAloneDoesNotRefillTheCodeAttempts(t *testing.T) {
+	ts := newTestServer(t)
+	ts.bootstrap(t)
+
+	_, body := ts.do(t, "POST", "/api/v1/auth/totp/setup", map[string]string{
+		"password": "et langt kodeord",
+	})
+	secret := body["secret"].(string)
+	ts.do(t, "POST", "/api/v1/auth/totp/confirm", map[string]string{"code": codeFor(t, secret)})
+	ts.do(t, "POST", "/api/v1/auth/logout", nil)
+
+	// Kodeordet er rigtigt hver gang; kun koden er forkert. Uden loftet ville det
+	// her kunne blive ved i det uendelige.
+	refused := 0
+	for i := 0; i < 14; i++ {
+		resp, _ := ts.do(t, "POST", "/api/v1/auth/login", map[string]string{
+			"email": "kristian@example.dk", "password": "et langt kodeord",
+		})
+		if resp.StatusCode == http.StatusTooManyRequests {
+			refused++
+			continue
+		}
+		resp, _ = ts.do(t, "POST", "/api/v1/auth/login/totp", map[string]string{"code": "000000"})
+		if resp.StatusCode == http.StatusTooManyRequests {
+			refused++
+		}
+	}
+	if refused == 0 {
+		t.Fatal("fjorten runder med forkerte koder blev aldrig afvist; spanden fyldes stadig af adgangskoden")
+	}
+}

internal/httpapi/server.go

@@ -64,6 +64,11 @@ type Server struct {
 	// than the rest of the API.
 	loginLimiter *limiter
 	resetLimiter *limiter
+	// Second factors are counted against the *account*, not the address. An
+	// address bucket is defeated by anybody with more than one address, which is
+	// the ordinary case — and a six-digit code is only safe while guesses are
+	// scarce, so the scarcity has to be counted against the thing being attacked.
+	totpLimiter *limiter
 
 	// Computed once from the built index.html; see csp.go.
 	csp string
@@ -81,6 +86,10 @@ func New(cfg *config.Config, db *store.DB, log *slog.Logger, web fs.FS) *Server
 		hub:          realtime.NewHub(log),
 		loginLimiter: newLimiter(10, 15*time.Minute),
 		resetLimiter: newLimiter(5, time.Hour),
+		// Tighter than the password bucket, and for a whole hour. Somebody typing
+		// their own code gets it right on the first or second try; ten in an hour is
+		// generous for a person and hopeless for a search through a million.
+		totpLimiter: newLimiter(10, time.Hour),
 	}
 	s.csp = contentSecurityPolicy(scriptHashes(web))
 	if w, err := auth.NewWebAuthn(cfg.BaseURL, "verdande"); err == nil {
@@ -200,11 +209,7 @@ func New(cfg *config.Config, db *store.DB, log *slog.Logger, web fs.FS) *Server
 				r.Put("/nav-order", s.handleSetNavOrder)
 				r.Post("/logout", s.handleLogout)
 				r.Post("/password/change", s.handleChangePassword)
-				r.Post("/totp/setup", s.handleTOTPSetup)
-				r.Post("/totp/confirm", s.handleTOTPConfirm)
-				r.Post("/totp/disable", s.handleTOTPDisable)
 				r.Get("/recovery-codes", s.handleRecoveryCodesCount)
-				r.Post("/recovery-codes", s.handleRecoveryCodesRegenerate)
 
 				// Behind requireSession, like the API tokens and for the same
 				// reason: a leaked token must not be able to end the sessions of
@@ -214,6 +219,23 @@ func New(cfg *config.Config, db *store.DB, log *slog.Logger, web fs.FS) *Server
 					r.Get("/sessions", s.handleListSessions)
 					r.Delete("/sessions/{sessionID}", s.handleDeleteSession)
 
+					// Turning the second factor on, and the codes that get you back
+					// in. Sessions only, and setup asks for the password.
+					//
+					// These sat outside this group, which meant the door opened with
+					// a token and closed only with a password: `disable` has always
+					// asked for the password, `setup` asked for nothing. A stolen
+					// token could therefore fetch the secret, load it into its own
+					// authenticator, confirm it, and keep the only copy of the
+					// recovery codes. The owner's next login would stop at a code
+					// they do not have, and they could not turn it off without
+					// logging in first. A theft becomes a tenancy — the same
+					// sentence written next door about passkeys, and the same rule.
+					r.Post("/totp/setup", s.handleTOTPSetup)
+					r.Post("/totp/confirm", s.handleTOTPConfirm)
+					r.Post("/totp/disable", s.handleTOTPDisable)
+					r.Post("/recovery-codes", s.handleRecoveryCodesRegenerate)
+
 					// Registering a key, and the list of them. Sessions only, for
 					// the same reason: a leaked token must not be able to add its
 					// own way in — that would turn a theft into a tenancy.

internal/httpapi/totp_handlers.go

@@ -20,7 +20,24 @@ type totpSetupResponse struct {
 }
 
 func (s *Server) handleTOTPSetup(w http.ResponseWriter, r *http.Request) {
+	// The password, the same as disable has always asked for.
+	//
+	// Symmetry is the point. Turning the second factor on is at least as
+	// consequential as turning it off — it decides which device gets to say yes
+	// from now on — and a door that opens more easily than it closes is not a
+	// door. Together with the move behind requireSession, this is what stops a
+	// stolen credential from enrolling its own authenticator and keeping the only
+	// copy of the recovery codes.
+	var req totpDisableRequest
+	if err := decodeJSON(w, r, &req); err != nil {
+		return
+	}
 	user := userFrom(r.Context())
+	if err := auth.VerifyPassword(user.PasswordHash, req.Password); err != nil {
+		writeFieldErrors(w, map[string]string{"password": "not correct"})
+		return
+	}
+
 	if user.TOTPEnabled {
 		writeError(w, http.StatusConflict, CodeConflict,
 			"two-factor authentication is already switched on")