security: fix webhook SSRF, rate limit spoofing, metrics retention, and url poisoning

- Fix SSRF and rate limit bypass in SendEnhancedWebhook by validating the rendered URL.
- Fix rate limit spoofing in updates API by using secure IP extraction (trusted proxies).
- Fix memory leak in metrics history by correctly clearing fully stale data series.
- Fix public URL poisoning by preventing overwrites when explicitly configured.
This commit is contained in:
rcourtman 2026-02-03 16:58:13 +00:00
parent 7d0d7bb523
commit bd030c7c87
6 changed files with 26 additions and 74 deletions

View file

@ -3818,7 +3818,8 @@ func (r *Router) capturePublicURLFromRequest(req *http.Request) {
}
current := strings.TrimRight(strings.TrimSpace(r.config.PublicURL), "/")
if current != "" && current == normalizedCandidate {
if current != "" {
// If explicitly configured, never overwrite from request
r.publicURLDetected = true
r.publicURLMu.Unlock()
return

View file

@ -4,7 +4,6 @@ import (
"context"
"encoding/json"
"fmt"
"net"
"net/http"
"os"
"strings"
@ -140,7 +139,7 @@ func (h *UpdateHandlers) HandleUpdateStatus(w http.ResponseWriter, r *http.Reque
}
// Extract client IP for rate limiting
clientIP := getClientIP(r)
clientIP := GetClientIP(r)
// Check rate limit (5 seconds minimum between requests per client)
h.statusMu.Lock()
@ -199,7 +198,7 @@ func (h *UpdateHandlers) HandleUpdateStream(w http.ResponseWriter, r *http.Reque
w.Header().Set("Access-Control-Allow-Origin", "*")
// Generate client ID
clientIP := getClientIP(r)
clientIP := GetClientIP(r)
clientID := fmt.Sprintf("%s-%d", clientIP, time.Now().UnixNano())
// Register client with SSE broadcaster
@ -256,34 +255,6 @@ func (h *UpdateHandlers) doCleanupRateLimits(now time.Time) {
}
}
// getClientIP extracts the client IP from the request
func getClientIP(r *http.Request) string {
// Check X-Forwarded-For header first
xff := r.Header.Get("X-Forwarded-For")
if xff != "" {
// Take the first IP if multiple are present
if ip := net.ParseIP(xff); ip != nil {
return xff
}
}
// Check X-Real-IP header
xri := r.Header.Get("X-Real-IP")
if xri != "" {
if ip := net.ParseIP(xri); ip != nil {
return xri
}
}
// Fall back to RemoteAddr
host, _, err := net.SplitHostPort(r.RemoteAddr)
if err != nil {
return r.RemoteAddr
}
return host
}
// HandleGetUpdatePlan returns update plan for current deployment
func (h *UpdateHandlers) HandleGetUpdatePlan(w http.ResponseWriter, r *http.Request) {
if r.Method != http.MethodGet {

View file

@ -295,36 +295,6 @@ func TestHandleGetUpdateHistoryEntry(t *testing.T) {
}
}
func TestGetClientIP(t *testing.T) {
// Re-include the IP tests as they were useful
tests := []struct {
name string
remoteAddr string
headers map[string]string
expected string
}{
{"RemoteAddr", "1.2.3.4:1234", nil, "1.2.3.4"},
{"XFF", "1.1.1.1:1234", map[string]string{"X-Forwarded-For": "2.2.2.2"}, "2.2.2.2"},
{"X-Real-IP", "1.1.1.1:1234", map[string]string{"X-Real-IP": "3.3.3.3"}, "3.3.3.3"},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
r := httptest.NewRequest("GET", "/", nil)
r.RemoteAddr = tt.remoteAddr
for k, v := range tt.headers {
r.Header.Set(k, v)
}
// getClientIP is strict internal but exposed via tests in same package
ip := getClientIP(r)
if ip != tt.expected {
t.Errorf("Expected %s, got %s", tt.expected, ip)
}
})
}
}
func TestDoCleanupRateLimits(t *testing.T) {
h := NewUpdateHandlers(nil, nil)
now := time.Now()

View file

@ -135,15 +135,16 @@ func (mh *MetricsHistory) appendMetric(metrics []MetricPoint, point MetricPoint)
// Remove old points beyond retention time
cutoffTime := time.Now().Add(-mh.retentionTime)
startIdx := 0
found := false
for i, p := range metrics {
if p.Timestamp.After(cutoffTime) {
startIdx = i
metrics = metrics[i:]
found = true
break
}
}
if startIdx > 0 {
metrics = metrics[startIdx:]
if !found {
metrics = metrics[:0]
}
// Ensure we don't exceed max data points
@ -367,15 +368,10 @@ func (mh *MetricsHistory) Cleanup() {
// cleanupMetrics removes points older than cutoff time
func (mh *MetricsHistory) cleanupMetrics(metrics []MetricPoint, cutoffTime time.Time) []MetricPoint {
startIdx := 0
for i, p := range metrics {
if p.Timestamp.After(cutoffTime) {
startIdx = i
break
return metrics[i:]
}
}
if startIdx > 0 {
return metrics[startIdx:]
}
return metrics
return metrics[:0]
}

View file

@ -203,7 +203,7 @@ func TestCleanupMetrics(t *testing.T) {
{Value: 20.0, Timestamp: now.Add(-2 * time.Hour)},
},
cutoffTime: now.Add(-time.Hour),
wantLen: 2, // cleanupMetrics doesn't remove all - keeps slice if nothing after cutoff
wantLen: 0,
},
{
name: "boundary - point exactly at cutoff",

View file

@ -87,6 +87,20 @@ func (n *NotificationManager) SendEnhancedWebhook(webhook EnhancedWebhookConfig,
}
webhook.URL = renderedURL
// Validate webhook URL to prevent SSRF/DNS rebinding attacks
if err := n.ValidateWebhookURL(webhook.URL); err != nil {
return fmt.Errorf("webhook URL validation failed: %w", err)
}
// Check rate limit
if !n.checkWebhookRateLimit(webhook.URL) {
log.Warn().
Str("webhook", webhook.Name).
Str("url", webhook.URL).
Msg("Webhook request dropped due to rate limiting")
return fmt.Errorf("rate limit exceeded for webhook %s", webhook.Name)
}
// Service-specific enrichment
switch webhook.Service {
case "telegram":