[Server] Fix rate limiter: disable semantics, draining window, bounded keys, pre-work login gate

- recordAttempt()/isBlocked() return false when maxAttempts <= 0 or windowSeconds <= 0, so the documented 'set to 0 to disable' actually disables instead of blocking every address permanently
- Keys at their limit are rejected without recording, so repeated attempts at the attacker's pace cannot keep sliding the window and holding the lockout open; a block now clears once the recorded attempts age out of windowSeconds
- Attempts map is bounded: an opportunistic prune (every 60s) erases keys whose attempts have fully aged out
- Login is throttled before the work: isLoginRateLimited() (a new virtual on Server, backed by isBlocked()) gates loginUser(), so a locked-out address no longer burns a database round trip and password verification per attempt
- recordAttemptAt()/isBlockedAt() time seams make the sliding-window behaviour deterministically testable; tests rewritten for the real semantics (no reliance on the maxAttempts=0 bug, no sleeps)

libcockatrice_network/libcockatrice/network/server/remote/server.h

@@ -180,6 +180,16 @@ public:
     {
         return false;
     }
+    /**
+     * @brief True if the given address is already locked out of logging in.
+     *
+     * Consulted before any authentication work (database round trip, password
+     * verification) so a blocked address cannot burn server CPU per attempt.
+     */
+    virtual bool isLoginRateLimited(const QString & /*ipAddress*/)
+    {
+        return false;
+    }
     /** @brief Clear any failed-login lockout for the given address, e.g. after a successful login. */
     virtual void clearFailedLogins(const QString & /*ipAddress*/)
     {

libcockatrice_network/libcockatrice/network/server/remote/server_protocolhandler.cpp

@@ -514,6 +514,11 @@ Response::ResponseCode Server_ProtocolHandler::cmdLogin(const Command_Login &cmd
     QString reasonStr;
     int banSecondsLeft = 0;
     QString connectionType = getConnectionType();
+    // Throttle before doing any work: a locked-out address must not get a full
+    // database round trip and password verification on every attempt.
+    if (server->isLoginRateLimited(getAddress())) {
+        return Response::RespTooManyRequests;
+    }
     AuthenticationResult res = server->loginUser(this, userName, password, needsHash, reasonStr, banSecondsLeft,
                                                  clientId, clientVersion, connectionType);
     switch (res) {

servatrice/src/ratelimiter.cpp

@@ -4,22 +4,47 @@
 
 bool RateLimiter::recordAttempt(const QString &key, int maxAttempts, int windowSeconds)
 {
-    QMutexLocker locker(&mutex);
+    return recordAttemptAt(key, maxAttempts, windowSeconds, QDateTime::currentSecsSinceEpoch());
-    const qint64 now = QDateTime::currentSecsSinceEpoch();
+}
 
-    QList<qint64> &timestamps = attempts[key];
+bool RateLimiter::recordAttemptAt(const QString &key, int maxAttempts, int windowSeconds, qint64 now)
-    timestamps.append(now);
+{
+    if (maxAttempts <= 0 || windowSeconds <= 0) {
+        return false; // rate limiting disabled for this endpoint
+    }
+
+    QMutexLocker locker(&mutex);
+    pruneLocked(now);
+
+    AttemptHistory &history = attempts[key];
+    history.windowSeconds = windowSeconds;
+    QList<qint64> &timestamps = history.timestamps;
     while (!timestamps.isEmpty() && timestamps.first() <= now - windowSeconds) {
         timestamps.removeFirst();
     }
+    if (timestamps.size() >= maxAttempts) {
+        // Already at the limit: reject without appending so repeated attempts
+        // at the attacker's own pace cannot keep sliding the window forward
+        // and hold the lockout open forever.
+        return true;
+    }
 
-    return timestamps.size() > maxAttempts;
+    timestamps.append(now);
+    return false;
 }
 
 bool RateLimiter::isBlocked(const QString &key, int maxAttempts, int windowSeconds) const
 {
+    return isBlockedAt(key, maxAttempts, windowSeconds, QDateTime::currentSecsSinceEpoch());
+}
+
+bool RateLimiter::isBlockedAt(const QString &key, int maxAttempts, int windowSeconds, qint64 now) const
+{
+    if (maxAttempts <= 0 || windowSeconds <= 0) {
+        return false;
+    }
+
     QMutexLocker locker(&mutex);
-    const qint64 now = QDateTime::currentSecsSinceEpoch();
 
     const auto it = attempts.constFind(key);
     if (it == attempts.constEnd()) {
@@ -27,12 +52,12 @@ bool RateLimiter::isBlocked(const QString &key, int maxAttempts, int windowSecon
     }
 
     int count = 0;
-    for (const qint64 &timestamp : it.value()) {
+    for (const qint64 &timestamp : it.value().timestamps) {
         if (timestamp > now - windowSeconds) {
             ++count;
         }
     }
-    return count > maxAttempts;
+    return count >= maxAttempts;
 }
 
 void RateLimiter::clearAttempts(const QString &key)
@@ -40,3 +65,24 @@ void RateLimiter::clearAttempts(const QString &key)
     QMutexLocker locker(&mutex);
     attempts.remove(key);
 }
+
+void RateLimiter::pruneLocked(qint64 now)
+{
+    if (lastPruneSecs + PruneIntervalSeconds > now) {
+        return;
+    }
+    lastPruneSecs = now;
+
+    auto it = attempts.begin();
+    while (it != attempts.end()) {
+        QList<qint64> &timestamps = it.value().timestamps;
+        while (!timestamps.isEmpty() && timestamps.first() <= now - it.value().windowSeconds) {
+            timestamps.removeFirst();
+        }
+        if (timestamps.isEmpty()) {
+            it = attempts.erase(it);
+        } else {
+            ++it;
+        }
+    }
+}

servatrice/src/ratelimiter.h

@@ -6,20 +6,69 @@
 #include <QMutex>
 #include <QString>
 
-/** @brief Thread-safe per-key attempt counter used to throttle abusive requests. */
+/** @brief Thread-safe per-key attempt limiter used to throttle abusive requests. */
 class RateLimiter
 {
 public:
-    /** @brief Record an attempt for the key and report whether the key is now over the limit. */
+    /**
+     * @brief Record an attempt for @p key and report whether the key is now blocked.
+     *
+     * A key that is already at its limit is not extended: the attempt is
+     * rejected without appending, so the lockout lifts once the recorded
+     * attempts age out of the window. Returns false immediately when
+     * @p maxAttempts <= 0 or @p windowSeconds <= 0 (rate limiting disabled),
+     * recording nothing.
+     *
+     * Delegates to recordAttemptAt() with the current wall-clock time.
+     */
     bool recordAttempt(const QString &key, int maxAttempts, int windowSeconds);
-    /** @brief True if the key already has more than maxAttempts attempts within windowSeconds. */
+
+    /**
+     * @brief Record an attempt as seen at epoch second @p now.
+     *
+     * Exposed for deterministic tests; production callers should use
+     * recordAttempt().
+     */
+    bool recordAttemptAt(const QString &key, int maxAttempts, int windowSeconds, qint64 now);
+
+    /**
+     * @brief True if @p key already has at least @p maxAttempts attempts within @p windowSeconds.
+     *
+     * Reads only; records nothing. Returns false when @p maxAttempts <= 0 or
+     * @p windowSeconds <= 0. Delegates to isBlockedAt().
+     */
     bool isBlocked(const QString &key, int maxAttempts, int windowSeconds) const;
+
+    /**
+     * @brief Whether @p key is blocked as measured at epoch second @p now.
+     *
+     * Exposed for deterministic tests; production callers should use
+     * isBlocked().
+     */
+    bool isBlockedAt(const QString &key, int maxAttempts, int windowSeconds, qint64 now) const;
+
     /** @brief Drop all recorded attempts for the key, e.g. after a successful login. */
     void clearAttempts(const QString &key);
 
 private:
+    struct AttemptHistory
+    {
+        QList<qint64> timestamps; // epoch seconds, oldest first
+        int windowSeconds;
+    };
+
+    /**
+     * @brief Drops keys whose last attempts have fully aged out of their window.
+     *
+     * Runs at most every PruneIntervalSeconds to stop the map from growing
+     * without bound when attackers rotate source addresses.
+     */
+    void pruneLocked(qint64 now);
+
     mutable QMutex mutex;
-    QMap<QString, QList<qint64>> attempts; // key -> attempt timestamps (epoch seconds)
+    QMap<QString, AttemptHistory> attempts; // key -> attempt history
+    qint64 lastPruneSecs = 0;
+    static constexpr int PruneIntervalSeconds = 60;
 };
 
-#endif
+#endif

servatrice/src/servatrice.cpp

@@ -1080,6 +1080,11 @@ bool Servatrice::recordFailedLogin(const QString &ipAddress)
     return rateLimiter.recordAttempt("login:" + ipAddress, getMaxLoginAttemptsPerIp(), getLoginAttemptWindowSeconds());
 }
 
+bool Servatrice::isLoginRateLimited(const QString &ipAddress)
+{
+    return rateLimiter.isBlocked("login:" + ipAddress, getMaxLoginAttemptsPerIp(), getLoginAttemptWindowSeconds());
+}
+
 void Servatrice::clearFailedLogins(const QString &ipAddress)
 {
     rateLimiter.clearAttempts("login:" + ipAddress);

servatrice/src/servatrice.h

@@ -284,6 +284,7 @@ public:
         return &rateLimiter;
     }
     bool recordFailedLogin(const QString &ipAddress) override;
+    bool isLoginRateLimited(const QString &ipAddress) override;
     void clearFailedLogins(const QString &ipAddress) override;
     int getMaxLoginAttemptsPerIp() const;
     int getLoginAttemptWindowSeconds() const;

tests/rate_limiter_test.cpp

@@ -8,36 +8,93 @@ namespace
 TEST(RateLimiterTest, AllowsAttemptsWithinLimit)
 {
     RateLimiter limiter;
-    ASSERT_FALSE(limiter.recordAttempt("ip", 3, 60));
+    ASSERT_FALSE(limiter.recordAttemptAt("ip", 3, 60, 1000));
-    ASSERT_FALSE(limiter.recordAttempt("ip", 3, 60));
+    ASSERT_FALSE(limiter.recordAttemptAt("ip", 3, 60, 1001));
-    ASSERT_FALSE(limiter.recordAttempt("ip", 3, 60));
+    // Still under the limit after two attempts, so the key is not blocked...
-    ASSERT_FALSE(limiter.isBlocked("ip", 3, 60));
+    ASSERT_FALSE(limiter.isBlockedAt("ip", 3, 60, 1002));
+    // ...and the third (the limit) is allowed through.
+    ASSERT_FALSE(limiter.recordAttemptAt("ip", 3, 60, 1002));
+    // The next attempt is blocked once three are on the books.
+    ASSERT_TRUE(limiter.isBlockedAt("ip", 3, 60, 1003));
+    ASSERT_TRUE(limiter.recordAttemptAt("ip", 3, 60, 1003));
 }
 
-TEST(RateLimiterTest, BlocksAttemptsOverLimit)
+TEST(RateLimiterTest, BlocksAttemptsAtTheLimit)
 {
     RateLimiter limiter;
-    ASSERT_FALSE(limiter.recordAttempt("ip", 2, 60));
+    ASSERT_FALSE(limiter.recordAttemptAt("ip", 2, 60, 1000));
-    ASSERT_FALSE(limiter.recordAttempt("ip", 2, 60));
+    ASSERT_FALSE(limiter.recordAttemptAt("ip", 2, 60, 1001));
-    ASSERT_TRUE(limiter.recordAttempt("ip", 2, 60));
+    ASSERT_TRUE(limiter.recordAttemptAt("ip", 2, 60, 1002));
-    ASSERT_TRUE(limiter.isBlocked("ip", 2, 60));
+    ASSERT_TRUE(limiter.isBlockedAt("ip", 2, 60, 1002));
+}
+
+TEST(RateLimiterTest, NonPositiveConfigDisablesLimiting)
+{
+    RateLimiter limiter;
+    // maxAttempts = 0 is the documented "set to 0 to disable"; negative or
+    // zero windows are config mistakes and must not lock everyone out either.
+    ASSERT_FALSE(limiter.recordAttempt("ip", 0, 60));
+    ASSERT_FALSE(limiter.isBlocked("ip", 0, 60));
+    ASSERT_FALSE(limiter.recordAttempt("ip", -1, 60));
+    ASSERT_FALSE(limiter.isBlocked("ip", -1, 60));
+    ASSERT_FALSE(limiter.recordAttempt("ip", 5, 0));
+    ASSERT_FALSE(limiter.isBlocked("ip", 5, 0));
 }
 
 TEST(RateLimiterTest, ClearAttempts)
 {
     RateLimiter limiter;
-    ASSERT_TRUE(limiter.recordAttempt("ip", 0, 60));
+    ASSERT_FALSE(limiter.recordAttemptAt("ip", 1, 60, 1000));
-    ASSERT_TRUE(limiter.isBlocked("ip", 0, 60));
+    ASSERT_TRUE(limiter.recordAttemptAt("ip", 1, 60, 1001));
+    ASSERT_TRUE(limiter.isBlockedAt("ip", 1, 60, 1001));
     limiter.clearAttempts("ip");
-    ASSERT_FALSE(limiter.isBlocked("ip", 0, 60));
+    ASSERT_FALSE(limiter.isBlockedAt("ip", 1, 60, 1002));
+    // The cleared key starts fresh and is allowed again.
+    ASSERT_FALSE(limiter.recordAttemptAt("ip", 1, 60, 1002));
 }
 
 TEST(RateLimiterTest, KeysAreIndependent)
 {
     RateLimiter limiter;
-    ASSERT_TRUE(limiter.recordAttempt("a", 0, 60));
+    ASSERT_FALSE(limiter.recordAttemptAt("a", 1, 60, 1000));
-    ASSERT_FALSE(limiter.isBlocked("b", 0, 60));
+    ASSERT_TRUE(limiter.recordAttemptAt("a", 1, 60, 1001));
-    ASSERT_TRUE(limiter.isBlocked("a", 0, 60));
+    ASSERT_FALSE(limiter.isBlockedAt("b", 1, 60, 1001));
+    ASSERT_TRUE(limiter.isBlockedAt("a", 1, 60, 1001));
+}
+
+TEST(RateLimiterTest, AttemptsAgeOutOfTheWindow)
+{
+    RateLimiter limiter;
+    const qint64 t = 1000;
+    const int window = 60;
+
+    ASSERT_FALSE(limiter.recordAttemptAt("ip", 2, window, t));
+    ASSERT_FALSE(limiter.recordAttemptAt("ip", 2, window, t + 1));
+    ASSERT_TRUE(limiter.recordAttemptAt("ip", 2, window, t + 2));
+    ASSERT_TRUE(limiter.isBlockedAt("ip", 2, window, t + 2));
+
+    // A blocked attempt is rejected without recording, so it cannot extend
+    // the lockout either.
+    ASSERT_TRUE(limiter.recordAttemptAt("ip", 2, window, t + 3));
+
+    // Once the recorded attempts fall outside the window the key clears by
+    // itself, with no manual unlock.
+    ASSERT_FALSE(limiter.isBlockedAt("ip", 2, window, t + 61));
+    ASSERT_FALSE(limiter.recordAttemptAt("ip", 2, window, t + 62));
+}
+
+TEST(RateLimiterTest, BlockedKeyIsUnlockedAfterSuccessfulClear)
+{
+    RateLimiter limiter;
+    const qint64 t = 5000;
+
+    ASSERT_FALSE(limiter.recordAttemptAt("ip", 2, 60, t));
+    ASSERT_FALSE(limiter.recordAttemptAt("ip", 2, 60, t + 1));
+    ASSERT_TRUE(limiter.recordAttemptAt("ip", 2, 60, t + 2));
+    ASSERT_TRUE(limiter.isBlockedAt("ip", 2, 60, t + 2));
+
+    limiter.clearAttempts("ip");
+    ASSERT_FALSE(limiter.recordAttemptAt("ip", 2, 60, t + 3));
 }
 
 } // namespace
@@ -46,4 +103,4 @@ int main(int argc, char **argv)
 {
     ::testing::InitGoogleTest(&argc, argv);
     return RUN_ALL_TESTS();
-}
+}