From bf173480e88047cb94ed5375315237604aada252 Mon Sep 17 00:00:00 2001 From: vlnst Date: Wed, 7 Oct 2026 01:21:45 +0300 Subject: [PATCH 1/2] Fix ratelimit crash under concurrent requests --- src/middleware/ratelimit.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/middleware/ratelimit.go b/src/middleware/ratelimit.go index d83c901..aba7864 100644 --- a/src/middleware/ratelimit.go +++ b/src/middleware/ratelimit.go @@ -56,7 +56,12 @@ func Ratelimit() gin.HandlerFunc { // subtract 1 from the value after 1 minute if the value exists and is greater than 0 time.AfterFunc(time.Minute, func() { val, ok := ipMap.Load(ip) - if ok && val.(int) > 0 { + // Concurrent requests can lose increments, so the entry may already + // have been deleted by an earlier timer. + if !ok { + return + } + if val.(int) > 0 { ipMap.Store(ip, val.(int)-1) } From 080e07c056660d78b4a4b08cabd4b3807fe8e4f0 Mon Sep 17 00:00:00 2001 From: vlnst Date: Wed, 4 Nov 2026 19:45:00 +0300 Subject: [PATCH 2/2] fix: rate limit in fixed one-minute windows --- src/middleware/ratelimit.go | 71 +++++++++----------- src/middleware/ratelimit_test.go | 111 +++++++++++++++++++++++++++++++ 2 files changed, 144 insertions(+), 38 deletions(-) create mode 100644 src/middleware/ratelimit_test.go diff --git a/src/middleware/ratelimit.go b/src/middleware/ratelimit.go index aba7864..6f5b629 100644 --- a/src/middleware/ratelimit.go +++ b/src/middleware/ratelimit.go @@ -10,9 +10,39 @@ import ( "github.com/gin-gonic/gin" ) -var ipMap = sync.Map{} +const ( + ratelimitWindow = time.Minute + ratelimitRequests = 30 +) + +type limiter struct { + mu sync.Mutex + now func() time.Time + windowStart time.Time + counts map[string]int +} + +func newLimiter(now func() time.Time) *limiter { + return &limiter{now: now, windowStart: now(), counts: map[string]int{}} +} + +func (l *limiter) allow(ip string) bool { + l.mu.Lock() + defer l.mu.Unlock() + + if now := l.now(); now.Sub(l.windowStart) >= ratelimitWindow { + l.windowStart = now + l.counts = map[string]int{} + } + l.counts[ip]++ + return l.counts[ip] <= ratelimitRequests +} func Ratelimit() gin.HandlerFunc { + return ratelimit(newLimiter(time.Now)) +} + +func ratelimit(l *limiter) gin.HandlerFunc { return func(c *gin.Context) { if os.Getenv("DISABLE_RATELIMIT") == "true" { @@ -25,26 +55,7 @@ func Ratelimit() gin.HandlerFunc { return } - ip := c.ClientIP() - - // log request count as the value, ip as key - // if the ip is not in the map, create a new entry with a value of 1 - // if they exceed 30 requests in 1 minute, return a 429 - - // get the value from the map - val, ok := ipMap.Load(ip) - if !ok { - // if the ip is not in the map, create a new entry with a value of 1 - ipMap.Store(ip, 1) - c.Next() - return - } - - // if the ip is in the map, increment the value - ipMap.Store(ip, val.(int)+1) - - // if they exceed 30 requests in 1 minute, return a 429 - if val.(int) > 30 { + if !l.allow(c.ClientIP()) { c.HTML(429, "home.html", gin.H{ "errorMessage": "You have exceeded the request limit. Please try again in a minute.", "version": config.Version, @@ -53,22 +64,6 @@ func Ratelimit() gin.HandlerFunc { return } - // subtract 1 from the value after 1 minute if the value exists and is greater than 0 - time.AfterFunc(time.Minute, func() { - val, ok := ipMap.Load(ip) - // Concurrent requests can lose increments, so the entry may already - // have been deleted by an earlier timer. - if !ok { - return - } - if val.(int) > 0 { - ipMap.Store(ip, val.(int)-1) - } - - // if the value is 0, delete the entry from the map - if val.(int) == 0 { - ipMap.Delete(ip) - } - }) + c.Next() } } diff --git a/src/middleware/ratelimit_test.go b/src/middleware/ratelimit_test.go new file mode 100644 index 0000000..cdd6f1b --- /dev/null +++ b/src/middleware/ratelimit_test.go @@ -0,0 +1,111 @@ +package middleware + +import ( + "fmt" + "html/template" + "net/http" + "net/http/httptest" + "sync" + "testing" + "time" + + "github.com/gin-gonic/gin" +) + +type fakeClock struct { + mu sync.Mutex + t time.Time +} + +func (f *fakeClock) now() time.Time { + f.mu.Lock() + defer f.mu.Unlock() + return f.t +} + +func (f *fakeClock) advance(d time.Duration) { + f.mu.Lock() + f.t = f.t.Add(d) + f.mu.Unlock() +} + +func limitedEngine(l *limiter) *gin.Engine { + gin.SetMode(gin.TestMode) + r := gin.New() + r.SetHTMLTemplate(template.Must(template.New("home.html").Parse("limited"))) + r.Use(ratelimit(l)) + r.GET("/version", func(c *gin.Context) { c.String(http.StatusOK, "ok") }) + return r +} + +func request(r *gin.Engine, ip string) int { + req := httptest.NewRequest(http.MethodGet, "/version", nil) + req.RemoteAddr = ip + ":1234" + w := httptest.NewRecorder() + r.ServeHTTP(w, req) + return w.Code +} + +func TestRatelimitLimitsAndRecovers(t *testing.T) { + clock := &fakeClock{t: time.Unix(1_700_000_000, 0)} + r := limitedEngine(newLimiter(clock.now)) + + for i := 1; i <= ratelimitRequests; i++ { + if code := request(r, "192.0.2.1"); code != http.StatusOK { + t.Fatalf("request %d: status %d, want 200", i, code) + } + } + // Overshoot well past the limit, as a script or a shared IP would. + for i := 0; i < 70; i++ { + if code := request(r, "192.0.2.1"); code != http.StatusTooManyRequests { + t.Fatalf("over-limit request: status %d, want 429", code) + } + } + if code := request(r, "192.0.2.2"); code != http.StatusOK { + t.Fatalf("other client limited too: status %d", code) + } + + clock.advance(ratelimitWindow) + if code := request(r, "192.0.2.1"); code != http.StatusOK { + t.Fatalf("client still limited after the window: status %d", code) + } +} + +func TestRatelimitForgetsOldClients(t *testing.T) { + clock := &fakeClock{t: time.Unix(1_700_000_000, 0)} + l := newLimiter(clock.now) + r := limitedEngine(l) + + for i := 0; i < 5000; i++ { + request(r, fmt.Sprintf("198.51.%d.%d", i/256, i%256)) + } + l.mu.Lock() + seen := len(l.counts) + l.mu.Unlock() + if seen != 5000 { + t.Fatalf("limiter tracked %d clients, want 5000", seen) + } + + clock.advance(ratelimitWindow) + request(r, "192.0.2.1") + + l.mu.Lock() + n := len(l.counts) + l.mu.Unlock() + if n != 1 { + t.Fatalf("limiter holds %d clients after the window, want 1", n) + } +} + +func TestRatelimitConcurrent(t *testing.T) { + r := limitedEngine(newLimiter(time.Now)) + var wg sync.WaitGroup + for i := 0; i < 200; i++ { + wg.Add(1) + go func() { + defer wg.Done() + request(r, "192.0.2.9") + }() + } + wg.Wait() +}