replace the custom ratelimiter strategy with a fixed window

CHANGELOG.md

@@ -1,9 +1,12 @@
 ## v0.36.7 (WIP)
 
-- Fixes high memory usage with large file uploads ([#7572](https://github.com/pocketbase/pocketbase/discussions/7572)).
+- Fixed high memory usage with large file uploads ([#7572](https://github.com/pocketbase/pocketbase/discussions/7572)).
 
-- (@todo) Updated `modernc.org/sqlite` to v1.47.0 (SQLite v3.52.0).
+- Updated the rate limiter reset rules to follow a more traditional fixed window strategy _(aka. to be more close to how it is presented in the UI - allow max X user requests under Ys)_ since several users complained that the older algorithm was not intuitive and not suitable for large intervals.
-    _It fixes a [database corruption bug](https://sqlite.org/wal.html#walresetbug) that it is very difficult to trigger but still it is advised to upgrade._
+    _Approximated sliding window strategy was also suggested as a better compromise option to help minimize traffic spikes right after reset but the additional tracking could introduce some overhead and for now it is left aside until we have more tests._
+
+- (@todo) Updated `modernc.org/sqlite` to v1.47.0 and SQLite 3.52.0.
+    _⚠️ SQLite 3.52.0 fixed a [database corruption bug](https://sqlite.org/wal.html#walresetbug) that is very unlikely to happen (with PocketBase even more so because we queue on app level all writes and explicit transactions through a single db connection), but still it is advised to upgrade._
 
 
 ## v0.36.6

README.md

@@ -49,7 +49,7 @@ your own custom app specific business logic and still have a single portable exe
 
 Here is a minimal example:
 
-0. [Install Go 1.23+](https://go.dev/doc/install) (_if you haven't already_)
+0. [Install Go 1.24+](https://go.dev/doc/install) (_if you haven't already_)
 
 1. Create a new project directory with the following `main.go` file inside it:
     ```go

apis/middlewares_rate_limit.go

@@ -108,28 +108,6 @@ func checkCollectionRateLimit(e *core.RequestEvent, collection *core.Collection,
 
 // -------------------------------------------------------------------
 
-// @todo consider exporting as helper?
-//
-//nolint:unused
-func isClientRateLimited(e *core.RequestEvent, rtId string) bool {
-	rateLimiters, ok := e.App.Store().Get(rateLimitersStoreKey).(*store.Store[string, *rateLimiter])
-	if !ok || rateLimiters == nil {
-		return false
-	}
-
-	rt, ok := rateLimiters.GetOk(rtId)
-	if !ok || rt == nil {
-		return false
-	}
-
-	client, ok := rt.getClient(e.RealIP())
-	if !ok || client == nil {
-		return false
-	}
-
-	return client.available <= 0 && time.Now().Unix()-client.lastConsume < client.interval
-}
-
 // @todo consider exporting as helper?
 func checkRateLimit(e *core.RequestEvent, rtId string, rule core.RateLimitRule) error {
 	switch rule.Audience {
@@ -154,7 +132,7 @@ func checkRateLimit(e *core.RequestEvent, rtId string, rule core.RateLimitRule)
 	}
 
 	rt := rateLimiters.GetOrSet(rtId, func() *rateLimiter {
-		return newRateLimiter(rule.MaxRequests, rule.Duration, rule.Duration+1800)
+		return newRateLimiter(rule.MaxRequests, rule.Duration, 1800)
 	})
 	if rt == nil {
 		e.App.Logger().Warn("Failed to retrieve app rate limiter", "id", rtId)
@@ -311,30 +289,27 @@ func newRateClient(maxAllowed int, intervalInSec int64) *rateClient {
 	}
 }
 
-// @todo evaluate swiching to a more traditional fixed window or sliding window counter
+// @todo evaluate swiching to sliding window with approximation counter similar to Cloudflare.
-// implementations since some users complained that it is not intuitive (see #7329).
 //
-// rateClient is a mixture of token bucket and fixed window rate limit strategies
+// rateClient implements fixed window rate limit strategy.
-// that refills the allowance only after at least "interval" seconds
-// has elapsed since the last request.
 type rateClient struct {
 	// use plain Mutex instead of RWMutex since the operations are expected
 	// to be mostly writes (e.g. consume()) and it should perform better
 	sync.Mutex
 
-	maxAllowed  int   // the max allowed tokens per interval
+	maxAllowed int   // the max allowed tokens per interval
-	available   int   // the total available tokens
+	available  int   // the total available tokens
-	interval    int64 // in seconds
+	start      int64 // the start time of the current window
-	lastConsume int64 // the time of the last consume
+	interval   int64 // in seconds
 }
 
-// hasExpired checks whether it has been at least minElapsed seconds since the lastConsume time.
+// hasExpired checks whether it has been at least minElapsed seconds after the last active window.
 // (usually used to perform periodic cleanup of staled instances).
 func (l *rateClient) hasExpired(relativeNow int64, minElapsed int64) bool {
 	l.Lock()
 	defer l.Unlock()
 
-	return relativeNow-l.lastConsume > minElapsed
+	return relativeNow-(l.start+l.interval) > minElapsed
 }
 
 // consume decreases the current allowance with 1 (if not exhausted already).
@@ -347,15 +322,14 @@ func (l *rateClient) consume() bool {
 
 	nowUnix := time.Now().Unix()
 
-	// reset consumed counter
+	// reset
-	if nowUnix-l.lastConsume >= l.interval {
+	if nowUnix-l.start >= l.interval {
 		l.available = l.maxAllowed
+		l.start = nowUnix
 	}
 
 	if l.available > 0 {
 		l.available--
-		l.lastConsume = nowUnix
-
 		return true
 	}
 

apis/middlewares_rate_limit_test.go

@@ -74,7 +74,7 @@ func TestDefaultRateLimitMiddleware(t *testing.T) {
 
 	scenarios := []struct {
 		url            string
-		wait           float64
+		wait           float64 // ms
 		authenticated  bool
 		expectedStatus int
 	}{
@@ -85,10 +85,13 @@ func TestDefaultRateLimitMiddleware(t *testing.T) {
 		{"/norate", 0, false, 200},
 
 		{"/rate/a", 0, false, 200},
+		{"/rate/a", 800, false, 200}, // (fixed window check) wait enough to ensure that it can't fit 2 requests in 1s
+		{"/rate/a", 800, false, 200},
+		{"/rate/a", 800, false, 200},
 		{"/rate/a", 0, false, 200},
 		{"/rate/a", 0, false, 429},
 		{"/rate/a", 0, false, 429},
-		{"/rate/a", 1.1, false, 200},
+		{"/rate/a", 1000, false, 200},
 		{"/rate/a", 0, false, 200},
 		{"/rate/a", 0, false, 429},
 
@@ -96,7 +99,7 @@ func TestDefaultRateLimitMiddleware(t *testing.T) {
 		{"/rate/b", 0, false, 200},
 		{"/rate/b", 0, false, 200},
 		{"/rate/b", 0, false, 429},
-		{"/rate/b", 1.1, false, 200},
+		{"/rate/b", 1000, false, 200},
 		{"/rate/b", 0, false, 200},
 		{"/rate/b", 0, false, 200},
 		{"/rate/b", 0, false, 429},
@@ -118,7 +121,7 @@ func TestDefaultRateLimitMiddleware(t *testing.T) {
 		{"/rate/guest", 0, false, 429},
 
 		// "guest" rule with regular user (should fallback to the /rate/ rule)
-		{"/rate/guest", 1.1, true, 200},
+		{"/rate/guest", 1000, true, 200},
 		{"/rate/guest", 0, true, 200},
 		{"/rate/guest", 0, true, 429},
 		{"/rate/guest", 0, true, 429},
@@ -126,10 +129,6 @@ func TestDefaultRateLimitMiddleware(t *testing.T) {
 
 	for _, s := range scenarios {
 		t.Run(s.url, func(t *testing.T) {
-			if s.wait > 0 {
-				time.Sleep(time.Duration(s.wait) * time.Second)
-			}
-
 			rec := httptest.NewRecorder()
 			req := httptest.NewRequest("GET", s.url, nil)
 
@@ -147,6 +146,10 @@ func TestDefaultRateLimitMiddleware(t *testing.T) {
 				req.Header.Add("Authorization", token)
 			}
 
+			if s.wait > 0 {
+				time.Sleep(time.Duration(s.wait) * time.Millisecond)
+			}
+
 			mux.ServeHTTP(rec, req)
 
 			result := rec.Result()

tools/security/jwt.go

@@ -45,6 +45,8 @@ func ParseJWT(token string, verificationKey string) (jwt.MapClaims, error) {
 // NewJWT generates and returns new HS256 signed JWT.
 func NewJWT(payload jwt.MapClaims, signingKey string, duration time.Duration) (string, error) {
 	claims := jwt.MapClaims{
+		// @todo consider with the refactoring to either remove the
+		// duration argument or make it always take precedent?
 		"exp": time.Now().Add(duration).Unix(),
 	}