Merge pull request #2119 from binwiederhier/auth-limiter-race

Do not read the shared visitor's user in the auth and account limiters

server/visitor.go

@@ -223,12 +223,10 @@ func (v *visitor) TopicCreationAllowed() bool {
 }
 
 // AuthAllowed returns true if an auth request can be attempted (a token is available)
+// It does not look at v.user: a concurrent request may set it on a shared IP-keyed visitor (#1051)
 func (v *visitor) AuthAllowed() bool {
 	v.mu.RLock() // limits could be replaced!
 	defer v.mu.RUnlock()
-	if v.user != nil {
-		return true // Users are already logged in, no need to limit requests
-	}
 	return v.limiter.Tokens(v.key, v.authRule()) >= 1
 }
 
@@ -236,19 +234,14 @@ func (v *visitor) AuthAllowed() bool {
 func (v *visitor) AuthFailed() {
 	v.mu.RLock() // limits could be replaced!
 	defer v.mu.RUnlock()
-	if v.user == nil {
-		v.limiter.Allow(v.key, v.authRule(), 1)
-	}
+	v.limiter.Allow(v.key, v.authRule(), 1)
 }
 
 // AccountActionAllowed returns true if a rate-limited account action (signup or password-reset
-// request) is currently allowed for this visitor
+// request) is currently allowed for this visitor. Like AuthAllowed, it does not look at v.user.
 func (v *visitor) AccountActionAllowed() bool {
 	v.mu.RLock() // limits could be replaced!
 	defer v.mu.RUnlock()
-	if v.user != nil {
-		return false // Users cannot create accounts when logged in
-	}
 	return v.limiter.Tokens(v.key, v.accountRule()) >= 1
 }
 
@@ -257,9 +250,7 @@ func (v *visitor) AccountActionAllowed() bool {
 func (v *visitor) AccountActionPerformed() {
 	v.mu.RLock() // limits could be replaced!
 	defer v.mu.RUnlock()
-	if v.user == nil {
-		v.limiter.Allow(v.key, v.accountRule(), 1)
-	}
+	v.limiter.Allow(v.key, v.accountRule(), 1)
 }
 
 func (v *visitor) BandwidthAllowed(bytes int64) bool {

server/visitor_test.go

@@ -229,3 +229,39 @@ func TestVisitor_ZeroLimitsAdmitNothing(t *testing.T) {
 		require.False(t, v.BandwidthLimiter().AllowN(1))
 	})
 }
+
+func TestVisitor_AuthLimiter_NotBypassedByConcurrentLogin(t *testing.T) {
+	// A user without a tier shares the IP-keyed visitor with anonymous requests from the same IP.
+	// Their login must not switch off the auth failure limiter for a concurrent guesser (#1051).
+	c := newTestConfigWithAuthFile(t, "")
+	c.VisitorAuthFailureLimitBurst = 3
+	s := newTestServer(t, c)
+	require.Nil(t, s.userManager.AddUser("phil", "phil", user.RoleUser, false))
+	u, err := s.userManager.User("phil")
+	require.Nil(t, err)
+	ip := netip.MustParseAddr("9.9.9.9")
+	vip := s.visitor(ip, nil) // The guesser's anonymous visitor (see maybeAuthenticate)
+
+	// The user logs in between the guesser's lookup and each failed guess
+	for i := 0; i < 3; i++ {
+		s.visitor(ip, u)
+		require.True(t, vip.AuthAllowed())
+		vip.AuthFailed()
+	}
+	s.visitor(ip, u)
+	require.False(t, vip.AuthAllowed())
+}
+
+func TestVisitor_AccountLimiter_NotDeniedByConcurrentLogin(t *testing.T) {
+	// The other direction: a concurrent login on the same IP must not deny an anonymous signup
+	c := newTestConfigWithAuthFile(t, "")
+	s := newTestServer(t, c)
+	require.Nil(t, s.userManager.AddUser("phil", "phil", user.RoleUser, false))
+	u, err := s.userManager.User("phil")
+	require.Nil(t, err)
+	ip := netip.MustParseAddr("9.9.9.9")
+	vip := s.visitor(ip, nil)
+
+	s.visitor(ip, u)
+	require.True(t, vip.AccountActionAllowed())
+}