diff --git a/server/visitor.go b/server/visitor.go index 3a766be8..631832ca 100644 --- a/server/visitor.go +++ b/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 { diff --git a/server/visitor_test.go b/server/visitor_test.go index d8241cae..37869293 100644 --- a/server/visitor_test.go +++ b/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()) +}