forked from molecule-ai/molecule-core
fix(router): call SetTrustedProxies(nil) to close IP-spoofing bypass (#179)
Without this call Gin's default trusts all X-Forwarded-For headers, letting any caller rotate their effective IP and bypass per-IP rate limiting. SetTrustedProxies(nil) forces c.ClientIP() to always return the real TCP RemoteAddr. Adds two regression tests: one documenting the pre-fix bypass, one asserting the spoofed header is ignored after the fix. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
parent
762b2f14a2
commit
06e02a310c
@ -45,6 +45,107 @@ func TestRateLimit_HeadersPresentOnAllowedRequest(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// TestRateLimit_XFF_BypassDocumented shows that WITHOUT SetTrustedProxies(nil)
|
||||
// a spoofed X-Forwarded-For header can rotate an attacker's effective IP and
|
||||
// bypass per-IP rate limiting (documents the issue #179 vulnerability).
|
||||
func TestRateLimit_XFF_BypassDocumented(t *testing.T) {
|
||||
gin.SetMode(gin.TestMode)
|
||||
ctx, cancel := context.WithCancel(context.Background())
|
||||
t.Cleanup(cancel)
|
||||
rl := NewRateLimiter(2, 5*time.Second, ctx)
|
||||
|
||||
r := gin.New()
|
||||
// Intentionally NOT calling r.SetTrustedProxies(nil) — replicates the
|
||||
// pre-fix behaviour where Gin trusts all proxies by default.
|
||||
r.Use(rl.Middleware())
|
||||
r.GET("/x", func(c *gin.Context) { c.String(http.StatusOK, "ok") })
|
||||
|
||||
// Exhaust both tokens for the real IP 10.0.0.1.
|
||||
for i := 0; i < 2; i++ {
|
||||
req := httptest.NewRequest(http.MethodGet, "/x", nil)
|
||||
req.RemoteAddr = "10.0.0.1:1234"
|
||||
w := httptest.NewRecorder()
|
||||
r.ServeHTTP(w, req)
|
||||
if w.Code != http.StatusOK {
|
||||
t.Fatalf("setup request %d: want 200, got %d", i+1, w.Code)
|
||||
}
|
||||
}
|
||||
// Third request without XFF must be rate-limited.
|
||||
{
|
||||
req := httptest.NewRequest(http.MethodGet, "/x", nil)
|
||||
req.RemoteAddr = "10.0.0.1:1234"
|
||||
w := httptest.NewRecorder()
|
||||
r.ServeHTTP(w, req)
|
||||
if w.Code != http.StatusTooManyRequests {
|
||||
t.Fatalf("3rd request (no XFF): want 429, got %d", w.Code)
|
||||
}
|
||||
}
|
||||
// With default proxy trust, spoofing X-Forwarded-For rotates the effective
|
||||
// IP → new bucket → bypass succeeds (returns 200).
|
||||
{
|
||||
req := httptest.NewRequest(http.MethodGet, "/x", nil)
|
||||
req.RemoteAddr = "10.0.0.1:1234"
|
||||
req.Header.Set("X-Forwarded-For", "20.0.0.1")
|
||||
w := httptest.NewRecorder()
|
||||
r.ServeHTTP(w, req)
|
||||
if w.Code != http.StatusOK {
|
||||
t.Skipf("bypass no longer works without trusted-proxy config (Gin version changed?): got %d", w.Code)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// TestRateLimit_XFF_NoBypassWithTrustedProxiesNil is the regression test for
|
||||
// issue #179: after r.SetTrustedProxies(nil) is added to router.Setup(), a
|
||||
// spoofed X-Forwarded-For header is ignored and the real RemoteAddr is used,
|
||||
// so the bypass no longer works.
|
||||
func TestRateLimit_XFF_NoBypassWithTrustedProxiesNil(t *testing.T) {
|
||||
gin.SetMode(gin.TestMode)
|
||||
ctx, cancel := context.WithCancel(context.Background())
|
||||
t.Cleanup(cancel)
|
||||
rl := NewRateLimiter(2, 5*time.Second, ctx)
|
||||
|
||||
r := gin.New()
|
||||
// Fix for issue #179 — mirror what router.Setup() now does.
|
||||
if err := r.SetTrustedProxies(nil); err != nil {
|
||||
t.Fatalf("SetTrustedProxies: %v", err)
|
||||
}
|
||||
r.Use(rl.Middleware())
|
||||
r.GET("/x", func(c *gin.Context) { c.String(http.StatusOK, "ok") })
|
||||
|
||||
// Exhaust both tokens for RemoteAddr 10.0.0.2.
|
||||
for i := 0; i < 2; i++ {
|
||||
req := httptest.NewRequest(http.MethodGet, "/x", nil)
|
||||
req.RemoteAddr = "10.0.0.2:9999"
|
||||
w := httptest.NewRecorder()
|
||||
r.ServeHTTP(w, req)
|
||||
if w.Code != http.StatusOK {
|
||||
t.Fatalf("setup request %d: want 200, got %d", i+1, w.Code)
|
||||
}
|
||||
}
|
||||
// Third plain request must be rate-limited.
|
||||
{
|
||||
req := httptest.NewRequest(http.MethodGet, "/x", nil)
|
||||
req.RemoteAddr = "10.0.0.2:9999"
|
||||
w := httptest.NewRecorder()
|
||||
r.ServeHTTP(w, req)
|
||||
if w.Code != http.StatusTooManyRequests {
|
||||
t.Fatalf("3rd plain request: want 429, got %d", w.Code)
|
||||
}
|
||||
}
|
||||
// Spoofed XFF must NOT rotate the bucket — still 429 because
|
||||
// SetTrustedProxies(nil) forces c.ClientIP() to return RemoteAddr.
|
||||
{
|
||||
req := httptest.NewRequest(http.MethodGet, "/x", nil)
|
||||
req.RemoteAddr = "10.0.0.2:9999"
|
||||
req.Header.Set("X-Forwarded-For", "99.99.99.99")
|
||||
w := httptest.NewRecorder()
|
||||
r.ServeHTTP(w, req)
|
||||
if w.Code != http.StatusTooManyRequests {
|
||||
t.Errorf("XFF bypass still works after fix: want 429, got %d — SetTrustedProxies(nil) not effective", w.Code)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// TestRateLimit_RetryAfterOn429 — throttled responses must carry Retry-After
|
||||
// per RFC 6585, so curl/fetch clients back off the exact required window.
|
||||
func TestRateLimit_RetryAfterOn429(t *testing.T) {
|
||||
|
||||
@ -25,6 +25,14 @@ import (
|
||||
func Setup(hub *ws.Hub, broadcaster *events.Broadcaster, prov *provisioner.Provisioner, platformURL, configsDir string, wh *handlers.WorkspaceHandler, channelMgr *channels.Manager) *gin.Engine {
|
||||
r := gin.Default()
|
||||
|
||||
// Issue #179 — trust no reverse-proxy headers. Without this call Gin's
|
||||
// default is to trust ALL X-Forwarded-For values, which lets any caller
|
||||
// spoof their IP and bypass per-IP rate limiting. With nil, c.ClientIP()
|
||||
// always returns the real TCP RemoteAddr.
|
||||
if err := r.SetTrustedProxies(nil); err != nil {
|
||||
panic("router: SetTrustedProxies: " + err.Error())
|
||||
}
|
||||
|
||||
// CORS origins — configurable via CORS_ORIGINS env var (comma-separated)
|
||||
corsOrigins := []string{"http://localhost:3000", "http://localhost:3001"}
|
||||
if v := os.Getenv("CORS_ORIGINS"); v != "" {
|
||||
|
||||
Loading…
Reference in New Issue
Block a user