Fix critical security vulnerability in amp-restrict-management-to-localhost feature where attackers could bypass localhost restriction by spoofing X-Forwarded-For headers. Changes: - Use RemoteAddr (actual TCP connection) instead of ClientIP() in localhostOnlyMiddleware to prevent header spoofing attacks - Add comprehensive test coverage for spoofing prevention (6 test cases) - Update documentation with reverse proxy deployment guidance and limitations of the RemoteAddr approach The fix prevents attacks like: curl -H "X-Forwarded-For: 127.0.0.1" https://server/api/user Trade-off: Users behind reverse proxies will need to disable the feature and use alternative security measures (firewall rules, proxy ACLs). Addresses security review feedback from PR #287.
Showing the changes to internal/api/modules (2 of 3 files). Show all
Unified · Split
| @@ -16,14 +16,28 @@ import ( · 15 unmodified lines | ||
| 16 | 16 | |
| 17 | 17 | // localhostOnlyMiddleware restricts access to localhost (127.0.0.1, ::1) only. |
| 18 | 18 | // Returns 403 Forbidden for non-localhost clients. |
| 19 | +// | |
| 20 | +// Security: Uses RemoteAddr (actual TCP connection) instead of ClientIP() to prevent | |
| 21 | +// header spoofing attacks via X-Forwarded-For or similar headers. This means the | |
| 22 | +// middleware will not work correctly behind reverse proxies - users deploying behind | |
| 23 | +// nginx/Cloudflare should disable this feature and use firewall rules instead. | |
| 19 | 24 | func localhostOnlyMiddleware() gin.HandlerFunc { |
| 20 | 25 | return func(c *gin.Context) { |
| 21 | − clientIP := c.ClientIP() | |
| 26 | + // Use actual TCP connection address (RemoteAddr) to prevent header spoofing | |
| 27 | + // This cannot be forged by X-Forwarded-For or other client-controlled headers | |
| 28 | + remoteAddr := c.Request.RemoteAddr | |
| 29 | + | |
| 30 | + // RemoteAddr format is "IP:port" or "[IPv6]:port", extract just the IP | |
| 31 | + host, _, err := net.SplitHostPort(remoteAddr) | |
| 32 | + if err != nil { | |
| 33 | + // Try parsing as raw IP (shouldn't happen with standard HTTP, but be defensive) | |
| 34 | + host = remoteAddr | |
| 35 | + } | |
| 22 | 36 | |
| 23 | 37 | // Parse the IP to handle both IPv4 and IPv6 |
| 24 | − ip := net.ParseIP(clientIP) | |
| 38 | + ip := net.ParseIP(host) | |
| 25 | 39 | if ip == nil { |
| 26 | − log.Warnf("Amp management: invalid client IP %s, denying access", clientIP) | |
| 40 | + log.Warnf("Amp management: invalid RemoteAddr %s, denying access", remoteAddr) | |
| 27 | 41 | c.AbortWithStatusJSON(403, gin.H{ |
| 28 | 42 | "error": "Access denied: management routes restricted to localhost", |
| 29 | 43 | }) |
| @@ -32,7 +46,7 @@ func localhostOnlyMiddleware() gin.HandlerFunc { · 2 unmodified lines | ||
| 32 | 46 | |
| 33 | 47 | // Check if IP is loopback (127.0.0.1 or ::1) |
| 34 | 48 | if !ip.IsLoopback() { |
| 35 | − log.Warnf("Amp management: non-localhost IP %s attempted access, denying", clientIP) | |
| 49 | + log.Warnf("Amp management: non-localhost connection from %s attempted access, denying", remoteAddr) | |
| 36 | 50 | c.AbortWithStatusJSON(403, gin.H{ |
| 37 | 51 | "error": "Access denied: management routes restricted to localhost", |
| 38 | 52 | }) |
| @@ -220,3 +220,82 @@ func TestRegisterProviderAliases_NoAuthMiddleware(t *testing.T) { · 219 unmodified lines | ||
| 220 | 220 | t.Fatal("routes should register even without auth middleware") |
| 221 | 221 | } |
| 222 | 222 | } |
| 223 | + | |
| 224 | +func TestLocalhostOnlyMiddleware_PreventsSpoofing(t *testing.T) { | |
| 225 | + gin.SetMode(gin.TestMode) | |
| 226 | + r := gin.New() | |
| 227 | + | |
| 228 | + // Apply localhost-only middleware | |
| 229 | + r.Use(localhostOnlyMiddleware()) | |
| 230 | + r.GET("/test", func(c *gin.Context) { | |
| 231 | + c.String(http.StatusOK, "ok") | |
| 232 | + }) | |
| 233 | + | |
| 234 | + tests := []struct { | |
| 235 | + name string | |
| 236 | + remoteAddr string | |
| 237 | + forwardedFor string | |
| 238 | + expectedStatus int | |
| 239 | + description string | |
| 240 | + }{ | |
| 241 | + { | |
| 242 | + name: "spoofed_header_remote_connection", | |
| 243 | + remoteAddr: "192.168.1.100:12345", | |
| 244 | + forwardedFor: "127.0.0.1", | |
| 245 | + expectedStatus: http.StatusForbidden, | |
| 246 | + description: "Spoofed X-Forwarded-For header should be ignored", | |
| 247 | + }, | |
| 248 | + { | |
| 249 | + name: "real_localhost_ipv4", | |
| 250 | + remoteAddr: "127.0.0.1:54321", | |
| 251 | + forwardedFor: "", | |
| 252 | + expectedStatus: http.StatusOK, | |
| 253 | + description: "Real localhost IPv4 connection should work", | |
| 254 | + }, | |
| 255 | + { | |
| 256 | + name: "real_localhost_ipv6", | |
| 257 | + remoteAddr: "[::1]:54321", | |
| 258 | + forwardedFor: "", | |
| 259 | + expectedStatus: http.StatusOK, | |
| 260 | + description: "Real localhost IPv6 connection should work", | |
| 261 | + }, | |
| 262 | + { | |
| 263 | + name: "remote_ipv4", | |
| 264 | + remoteAddr: "203.0.113.42:8080", | |
| 265 | + forwardedFor: "", | |
| 266 | + expectedStatus: http.StatusForbidden, | |
| 267 | + description: "Remote IPv4 connection should be blocked", | |
| 268 | + }, | |
| 269 | + { | |
| 270 | + name: "remote_ipv6", | |
| 271 | + remoteAddr: "[2001:db8::1]:9090", | |
| 272 | + forwardedFor: "", | |
| 273 | + expectedStatus: http.StatusForbidden, | |
| 274 | + description: "Remote IPv6 connection should be blocked", | |
| 275 | + }, | |
| 276 | + { | |
| 277 | + name: "spoofed_localhost_ipv6", | |
| 278 | + remoteAddr: "203.0.113.42:8080", | |
| 279 | + forwardedFor: "::1", | |
| 280 | + expectedStatus: http.StatusForbidden, | |
| 281 | + description: "Spoofed X-Forwarded-For with IPv6 localhost should be ignored", | |
| 282 | + }, | |
| 283 | + } | |
| 284 | + | |
| 285 | + for _, tt := range tests { | |
| 286 | + t.Run(tt.name, func(t *testing.T) { | |
| 287 | + req := httptest.NewRequest(http.MethodGet, "/test", nil) | |
| 288 | + req.RemoteAddr = tt.remoteAddr | |
| 289 | + if tt.forwardedFor != "" { | |
| 290 | + req.Header.Set("X-Forwarded-For", tt.forwardedFor) | |
| 291 | + } | |
| 292 | + | |
| 293 | + w := httptest.NewRecorder() | |
| 294 | + r.ServeHTTP(w, req) | |
| 295 | + | |
| 296 | + if w.Code != tt.expectedStatus { | |
| 297 | + t.Errorf("%s: expected status %d, got %d", tt.description, tt.expectedStatus, w.Code) | |
| 298 | + } | |
| 299 | + }) | |
| 300 | + } | |
| 301 | +} | |