Skip to content

fix(tollbooth): default IPLookup in LimitByRequest to avoid silent bypass - #118

Open
desperatee wants to merge 1 commit into
didip:masterfrom
desperatee:fix/limit-handler-default-iplookup
Open

fix(tollbooth): default IPLookup in LimitByRequest to avoid silent bypass#118
desperatee wants to merge 1 commit into
didip:masterfrom
desperatee:fix/limit-handler-default-iplookup

Conversation

@desperatee

@desperatee desperatee commented Jun 27, 2026

Copy link
Copy Markdown

Summary

The two entry points to the limiter — HTTPMiddleware and LimitHandler — behave inconsistently for the most common configuration (tollbooth.NewLimiter(N, nil) with no explicit SetIPLookup):

Entry point Default Name == "" handling Result
HTTPMiddleware Defaults to "RemoteAddr" at line 353 Limiter fires ✅
LimitHandler No default applied; falls through to skip Silent bypass

Root cause: limiter.New() leaves explicitIPLookup at its zero value (IPLookup{Name: ""}). RemoteIPFromIPLookup only knows four names; an empty string returns "", and ShouldSkipLimiter then returns true because the bucket cannot be addressed.

HTTPMiddleware has this guard:

if lmt.GetIPLookup().Name == "" {
    lmt.SetIPLookup(limiter.IPLookup{Name: "RemoteAddr"})
}

LimitByRequest (called by LimitHandler and the public API) does not.

Reproduction

lmt := tollbooth.NewLimiter(1, nil)
h := tollbooth.LimitHandler(lmt, myHandler)
// 100 rapid-fire requests all pass — limiter never fires.

Fix

Apply the same guard in LimitByRequest so both entry points behave identically.

Severity

HIGH — silent rate-limit bypass, no log/metric signal, and the broken pattern matches the simplest example in any rate-limiting blog post about this library. Users who switch from HTTPMiddleware to LimitHandler (e.g. to wrap a single endpoint) get a degradation with no warning.

Proof of Concept

limiter.New() does not set explicitIPLookup, leaving IPLookup{Name: ""} as the zero value. RemoteIPFromIPLookup only handles "RemoteAddr", "X-Forwarded-For", "X-Real-IP", and "CF-Connecting-IP" — the empty-string name falls through to return "". ShouldSkipLimiter then returns true (skip) because the per-key bucket cannot be addressed.

HTTPMiddleware defends against this with a Name == "" guard that defaults to "RemoteAddr". LimitHandler / LimitByRequest do not — so the most common documented usage (tollbooth.LimitHandler(tollbooth.NewLimiter(N, nil), handler)) silently lets every request through.

Test_Issue48 is a vacuous pass: its configured limit (2 req/s) equals its actual send rate (one request every 500ms), so the test passes whether the limiter fires or is bypassed entirely.

Steps to Reproduce

package tollbooth

import (
	"net/http"
	"net/http/httptest"
	"testing"
)

func TestLimitHandlerBypassWithoutIPLookup(t *testing.T) {
	lmt := NewLimiter(1, nil) // No SetIPLookup — typical README usage

	allowed := 0
	h := LimitHandler(lmt, http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
		allowed++
	}))

	for i := 0; i < 100; i++ {
		req, _ := http.NewRequest("GET", "/", nil)
		req.RemoteAddr = "1.2.3.4:1234"
		h.ServeHTTP(httptest.NewRecorder(), req)
	}

	if allowed == 100 {
		t.Fatalf("BYPASS: 100/100 requests passed limiter of 1 req/s")
	}
}

100/100 pass on master, ~1/100 pass after fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant