security: fix rate limiter bypass, error swallowing, race, window math (#2, #4, #17, #18)#587
Merged
Merged
Conversation
#2, #4, #17, #18) #2 — X-Forwarded-For bypass gin's c.ClientIP() trusts proxy headers by default and there was no SetTrustedProxies() call anywhere. Any client could spoof X-Forwarded-For to evade per-IP rate limiting and pollute audit logs with arbitrary IPs. Fix: - Add Config.TrustedProxies ([]string of CIDRs) - New CLI flag --trusted-proxies (empty default → trust no proxies → use RemoteAddr; safe out of the box) - NewRouter() now calls router.SetTrustedProxies(...). Operators behind a real reverse proxy must explicitly opt in. - AppConfig added to server.Dependencies so the router knows the trusted list at construction time. #4 — Redis rate limiter swallowed errors redis.go::Allow returned (true, nil) on any Redis error, "failing open" silently. The operator-controlled --rate-limit-fail-closed flag was a no-op because the error never reached the middleware that checks it. Fix: propagate the error to the caller so the middleware can apply fail-closed behaviour as configured. #17 — In-memory rate limiter data race on lastSeen The lastSeen field on the per-IP entry was a plain time.Time accessed concurrently by Allow() and the cleanup goroutine. Race detector tripped and updates were non-deterministic. Fix: convert lastSeen to atomic.Int64 (unix nanos) with touch() and lastSeenTime() helpers. cleanup() reads via lastSeenTime() so the lock- free fast path stays correct. #18 — Redis window math mismatch vs in-memory window := burst / rps used integer division and truncated to 0 when burst < rps, producing inconsistent enforcement across the two backends. Fix: use ceiling division (a + b - 1) / b so the redis sliding window is at least as long as the rps period, matching the effective window of the golang.org/x/time/rate limiter used by the in-memory backend.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Four related rate limiter fixes:
#2 — X-Forwarded-For bypass (High)
gin's
c.ClientIP()trusts proxy headers by default and there was noSetTrustedProxies()call anywhere. Any client could spoofX-Forwarded-Forto evade per-IP rate limiting and pollute audit logs with arbitrary IPs.#4 — Redis swallowed errors (High)
internal/rate_limit/redis.goreturned(true, nil)on any Redis error. The operator-controlled--rate-limit-fail-closedflag was a no-op because the error never reached the middleware.#17 — Data race on
lastSeen(Low)The race detector tripped on the per-IP entry's
lastSeenfield.#18 — Window math mismatch (Low)
window := burst / rpsused integer division and truncated to 0 whenburst < rps.Changes
Config.TrustedProxies([]string of CIDRs) and CLI flag--trusted-proxies(empty default → trust no proxies → useRemoteAddr).NewRouter()now callsrouter.SetTrustedProxies(...).AppConfigadded toserver.Dependenciesfor this.redis.go::Allowpropagates errors so the middleware can apply fail-closed as configured.lastSeenis nowatomic.Int64(unix nanos) withtouch()/lastSeenTime()helpers; cleanup reads via the atomic.(a + b - 1) / bso the redis sliding window is at least as long as the rps period.Migration
Operators behind a real reverse proxy must explicitly opt in via
--trusted-proxies=<proxy-cidr>or the rate limiter will key on the proxy IP. Document in CHANGELOG and../authorizer-docs.Test plan
go build ./...go test -race ./internal/rate_limit/...TEST_DBS=sqlite go test -run TestSignup ./internal/integration_tests/