fix: improve robustness and error handling across codebase

Security and stability improvements:
- Fix potential panic in isIPBlacklisted: use netip.ParseAddr instead of MustParseAddr
- Fix type assertion panic in processRuleMatch: use getLogID helper function
- Add HTTP client timeout (30s) in tor.go to prevent hanging requests
- Improve extractIP to handle empty/malformed input gracefully
- Improve getClientIP to validate X-Forwarded-For IPs before use

Input validation improvements:
- Add comprehensive validation in Validate() method for:
  - Negative anomaly_threshold
  - Invalid rate limit configuration
  - Negative max_request_body_size
  - Negative log_buffer
- Add validation in NewRateLimiter for requests, window, cleanup_interval

All tests pass with race detector enabled.

Author: fabriziosalmi

blacklist.go

@@ -68,10 +68,17 @@ func (m *Middleware) isIPBlacklisted(addr string) bool {
 	ip := extractIP(addr)
 
 	if m.ipBlacklist == nil {
-		m.logger.Error("blacklist", zap.String("IP blacklist", "is nil"))
+		m.logger.Debug("IP blacklist not initialized, skipping check")
+		return false
+	}
+
+	parsedIP, err := netip.ParseAddr(ip)
+	if err != nil {
+		m.logger.Debug("Failed to parse IP address for blacklist check", zap.String("ip", ip), zap.Error(err))
+		return false
 	}
 
-	if m.ipBlacklist.Contains(netip.MustParseAddr(ip)) {
+	if m.ipBlacklist.Contains(parsedIP) {
 		m.muIPBlacklistMetrics.Lock()                            // Acquire lock before accessing shared counter
 		m.IPBlacklistBlockCount++                                // Increment the counter
 		m.muIPBlacklistMetrics.Unlock()                          // Release lock after accessing counter

caddywaf.go

@@ -608,5 +608,31 @@ func (m *Middleware) Validate() error {
 	if m.logLevel == 0 {
 		m.logLevel = zapcore.InfoLevel // Default log level
 	}
+
+	// Validate anomaly threshold
+	if m.AnomalyThreshold < 0 {
+		return fmt.Errorf("anomaly_threshold cannot be negative: %d", m.AnomalyThreshold)
+	}
+
+	// Validate rate limit configuration if enabled
+	if m.RateLimit.Requests > 0 {
+		if m.RateLimit.Window <= 0 {
+			return fmt.Errorf("rate_limit window must be positive when rate limiting is enabled")
+		}
+		if m.RateLimit.CleanupInterval <= 0 {
+			return fmt.Errorf("rate_limit cleanup_interval must be positive when rate limiting is enabled")
+		}
+	}
+
+	// Validate max request body size
+	if m.MaxRequestBodySize < 0 {
+		return fmt.Errorf("max_request_body_size cannot be negative: %d", m.MaxRequestBodySize)
+	}
+
+	// Validate log buffer
+	if m.LogBuffer < 0 {
+		return fmt.Errorf("log_buffer cannot be negative: %d", m.LogBuffer)
+	}
+
 	return nil
 }

helpers.go

@@ -39,20 +39,35 @@ func appendCIDR(ip string) string {
 }
 
 // extractIP extracts the IP address from a remote address string.
+// Returns the original input if parsing fails, which allows upstream
+// code to handle invalid IPs gracefully.
 func extractIP(remoteAddr string) string {
+	if remoteAddr == "" {
+		return ""
+	}
 	host, _, err := net.SplitHostPort(remoteAddr)
 	if err != nil {
-		return remoteAddr // Assume the input is already an IP address
+		// Could be an IP without port, validate it
+		if ip := net.ParseIP(remoteAddr); ip != nil {
+			return remoteAddr
+		}
+		// Return as-is for upstream handling
+		return remoteAddr
 	}
 	return host
 }
 
 // getClientIP returns the real client IP, checking X-Forwarded-For header first.
+// Falls back to RemoteAddr if X-Forwarded-For is empty or contains invalid data.
 func getClientIP(r *http.Request) string {
 	if xff := r.Header.Get("X-Forwarded-For"); xff != "" {
 		ips := strings.Split(xff, ",")
 		if len(ips) > 0 {
-			return strings.TrimSpace(ips[0])
+			clientIP := strings.TrimSpace(ips[0])
+			// Validate the IP before returning
+			if clientIP != "" && net.ParseIP(extractIP(clientIP)) != nil {
+				return clientIP
+			}
 		}
 	}
 	return r.RemoteAddr

ratelimiter.go

@@ -37,6 +37,17 @@ type RateLimiter struct {
 
 // NewRateLimiter creates a new RateLimiter instance.
 func NewRateLimiter(config RateLimit) (*RateLimiter, error) {
+	// Validate configuration
+	if config.Requests <= 0 {
+		return nil, fmt.Errorf("rate limit requests must be positive, got %d", config.Requests)
+	}
+	if config.Window <= 0 {
+		return nil, fmt.Errorf("rate limit window must be positive, got %v", config.Window)
+	}
+	if config.CleanupInterval <= 0 {
+		return nil, fmt.Errorf("rate limit cleanup interval must be positive, got %v", config.CleanupInterval)
+	}
+
 	// Compile path regexes if paths are provided
 	if len(config.Paths) > 0 {
 		config.PathRegexes = make([]*regexp.Regexp, len(config.Paths))

rules.go

@@ -15,7 +15,7 @@ import (
 )
 
 func (m *Middleware) processRuleMatch(w http.ResponseWriter, r *http.Request, rule *Rule, target, value string, state *WAFState) bool {
-	logID := r.Context().Value(ContextKeyLogId("logID")).(string)
+	logID := getLogID(r.Context())
 
 	redactedValue := m.requestValueExtractor.RedactValueIfSensitive(target, value)
 

tor.go

@@ -48,7 +48,12 @@ func (t *TorConfig) updateTorExitNodes() error {
 		url = t.CustomTORExitNodeURL
 	}
 
-	resp, err := http.Get(url)
+	// Create HTTP client with timeout to avoid hanging requests
+	client := &http.Client{
+		Timeout: 30 * time.Second,
+	}
+
+	resp, err := client.Get(url)
 	if err != nil {
 		return fmt.Errorf("http get failed for %s: %w", url, err) // Improved error message with URL
 	}