From 7c1ebbecd58a43fa795b1ac757090244fa5b4754 Mon Sep 17 00:00:00 2001 From: rcourtman Date: Tue, 3 Feb 2026 22:41:44 +0000 Subject: [PATCH] fix(security): enhance webhook validation, enforce API scopes, and improve test coverage --- internal/api/host_agents.go | 4 +- .../api/router_handlers_additional_test.go | 8 +- internal/api/security_regression_test.go | 443 ++++++++++++++++++ internal/notifications/notifications.go | 7 +- internal/notifications/notifications_test.go | 30 ++ internal/notifications/webhook_client_test.go | 138 +++++- .../notifications/webhook_security_test.go | 24 + 7 files changed, 636 insertions(+), 18 deletions(-) create mode 100644 internal/api/security_regression_test.go create mode 100644 internal/notifications/webhook_security_test.go diff --git a/internal/api/host_agents.go b/internal/api/host_agents.go index 567669cde..b8f148024 100644 --- a/internal/api/host_agents.go +++ b/internal/api/host_agents.go @@ -311,9 +311,7 @@ func (h *HostAgentHandlers) resolveConfigHost(ctx context.Context, hostID string for _, candidate := range state.Hosts { if candidate.TokenID != "" && candidate.TokenID == record.ID { - if candidate.ID == hostID { - return candidate, true - } + return candidate, true } } diff --git a/internal/api/router_handlers_additional_test.go b/internal/api/router_handlers_additional_test.go index 4d760599a..b606508b1 100644 --- a/internal/api/router_handlers_additional_test.go +++ b/internal/api/router_handlers_additional_test.go @@ -209,7 +209,7 @@ func TestHandleSimpleStats(t *testing.T) { } func TestHandleSocketIO_RedirectsForJS(t *testing.T) { - router := &Router{} + router := &Router{config: &config.Config{}} req := httptest.NewRequest(http.MethodGet, "/socket.io/socket.io.js", nil) rec := httptest.NewRecorder() @@ -224,7 +224,7 @@ func TestHandleSocketIO_RedirectsForJS(t *testing.T) { } func TestHandleSocketIO_PollingHandshake(t *testing.T) { - router := &Router{} + router := &Router{config: &config.Config{}} req := httptest.NewRequest(http.MethodGet, "/socket.io/?transport=polling", nil) rec := httptest.NewRecorder() @@ -243,7 +243,7 @@ func TestHandleSocketIO_PollingHandshake(t *testing.T) { } func TestHandleSocketIO_PollingConnected(t *testing.T) { - router := &Router{} + router := &Router{config: &config.Config{}} req := httptest.NewRequest(http.MethodGet, "/socket.io/?transport=polling&sid=abc", nil) rec := httptest.NewRecorder() @@ -258,7 +258,7 @@ func TestHandleSocketIO_PollingConnected(t *testing.T) { } func TestHandleSocketIO_DefaultRedirect(t *testing.T) { - router := &Router{} + router := &Router{config: &config.Config{}} req := httptest.NewRequest(http.MethodGet, "/socket.io/?foo=bar", nil) rec := httptest.NewRecorder() diff --git a/internal/api/security_regression_test.go b/internal/api/security_regression_test.go new file mode 100644 index 000000000..2f6f3b0d2 --- /dev/null +++ b/internal/api/security_regression_test.go @@ -0,0 +1,443 @@ +package api + +import ( + "bytes" + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" + + "github.com/gorilla/websocket" + "github.com/rcourtman/pulse-go-rewrite/internal/agentexec" + "github.com/rcourtman/pulse-go-rewrite/internal/config" + pulsews "github.com/rcourtman/pulse-go-rewrite/internal/websocket" +) + +type wsRawMessage struct { + Type agentexec.MessageType `json:"type"` + Payload json.RawMessage `json:"payload,omitempty"` +} + +func newTestConfigWithTokens(t *testing.T, records ...config.APITokenRecord) *config.Config { + t.Helper() + tempDir := t.TempDir() + return &config.Config{ + DataPath: tempDir, + ConfigPath: tempDir, + APITokens: records, + } +} + +func newTokenRecord(t *testing.T, raw string, scopes []string, metadata map[string]string) config.APITokenRecord { + t.Helper() + record, err := config.NewAPITokenRecord(raw, "test-token", scopes) + if err != nil { + t.Fatalf("NewAPITokenRecord: %v", err) + } + if metadata != nil { + record.Metadata = metadata + } + return *record +} + +func readRegisteredPayload(t *testing.T, conn *websocket.Conn) agentexec.RegisteredPayload { + t.Helper() + conn.SetReadDeadline(time.Now().Add(2 * time.Second)) + _, data, err := conn.ReadMessage() + if err != nil { + t.Fatalf("ReadMessage: %v", err) + } + + var msg wsRawMessage + if err := json.Unmarshal(data, &msg); err != nil { + t.Fatalf("unmarshal message: %v", err) + } + if msg.Type != agentexec.MsgTypeRegistered { + t.Fatalf("message type = %q, want %q", msg.Type, agentexec.MsgTypeRegistered) + } + if msg.Payload == nil { + t.Fatalf("registered payload missing") + } + + var payload agentexec.RegisteredPayload + if err := json.Unmarshal(msg.Payload, &payload); err != nil { + t.Fatalf("unmarshal registered payload: %v", err) + } + return payload +} + +func TestSimpleStatsRequiresAuthInAPIMode(t *testing.T) { + rawToken := "stats-token-123.12345678" + record := newTokenRecord(t, rawToken, []string{config.ScopeMonitoringRead}, nil) + cfg := newTestConfigWithTokens(t, record) + router := NewRouter(cfg, nil, nil, nil, nil, "1.0.0") + + req := httptest.NewRequest(http.MethodGet, "/simple-stats", nil) + rec := httptest.NewRecorder() + router.Handler().ServeHTTP(rec, req) + if rec.Code != http.StatusUnauthorized { + t.Fatalf("expected 401 without token, got %d", rec.Code) + } + + req = httptest.NewRequest(http.MethodGet, "/simple-stats", nil) + req.Header.Set("X-API-Token", rawToken) + rec = httptest.NewRecorder() + router.Handler().ServeHTTP(rec, req) + if rec.Code != http.StatusOK { + t.Fatalf("expected 200 with token, got %d", rec.Code) + } + if !strings.Contains(rec.Body.String(), "Simple Pulse Stats") { + t.Fatalf("expected stats page HTML, got %q", rec.Body.String()) + } +} + +func TestSocketIORequiresAuthInAPIMode(t *testing.T) { + rawToken := "socket-token-123.12345678" + record := newTokenRecord(t, rawToken, []string{config.ScopeMonitoringRead}, nil) + cfg := newTestConfigWithTokens(t, record) + router := NewRouter(cfg, nil, nil, nil, nil, "1.0.0") + + req := httptest.NewRequest(http.MethodGet, "/socket.io/?transport=polling", nil) + rec := httptest.NewRecorder() + router.Handler().ServeHTTP(rec, req) + if rec.Code != http.StatusUnauthorized { + t.Fatalf("expected 401 without token, got %d", rec.Code) + } + + req = httptest.NewRequest(http.MethodGet, "/socket.io/?transport=polling", nil) + req.Header.Set("X-API-Token", rawToken) + rec = httptest.NewRecorder() + router.Handler().ServeHTTP(rec, req) + if rec.Code != http.StatusOK { + t.Fatalf("expected 200 with token, got %d", rec.Code) + } + if ct := rec.Header().Get("Content-Type"); ct != "text/plain; charset=UTF-8" { + t.Fatalf("expected text/plain content type, got %q", ct) + } + if body := rec.Body.String(); !strings.HasPrefix(body, "0{") { + t.Fatalf("unexpected polling handshake body: %q", body) + } +} + +func TestSocketIOWebSocketRequiresAuthInAPIMode(t *testing.T) { + rawToken := "socket-ws-token-123.12345678" + record := newTokenRecord(t, rawToken, []string{config.ScopeMonitoringRead}, nil) + cfg := newTestConfigWithTokens(t, record) + + hub := pulsews.NewHub(nil) + go hub.Run() + defer hub.Stop() + + router := NewRouter(cfg, nil, nil, hub, nil, "1.0.0") + ts := httptest.NewServer(router.Handler()) + defer ts.Close() + + wsURL := "ws" + strings.TrimPrefix(ts.URL, "http") + "/socket.io/?transport=websocket" + + conn, resp, err := websocket.DefaultDialer.Dial(wsURL, nil) + if err == nil { + conn.Close() + t.Fatalf("expected websocket auth failure without token") + } + if resp == nil { + t.Fatalf("expected HTTP response for failed websocket auth") + } + if resp.StatusCode != http.StatusUnauthorized { + t.Fatalf("expected 401 for missing token, got %d", resp.StatusCode) + } + + headers := http.Header{} + headers.Set("X-API-Token", rawToken) + conn, resp, err = websocket.DefaultDialer.Dial(wsURL, headers) + if err != nil { + t.Fatalf("expected websocket connection with token, got %v", err) + } + if resp == nil || resp.StatusCode != http.StatusSwitchingProtocols { + t.Fatalf("expected 101 switching protocols, got %v", resp) + } + conn.Close() +} + +func TestWebSocketRequiresMonitoringReadScope(t *testing.T) { + rawToken := "ws-token-123.12345678" + record := newTokenRecord(t, rawToken, []string{config.ScopeHostReport}, nil) + cfg := newTestConfigWithTokens(t, record) + router := NewRouter(cfg, nil, nil, nil, nil, "1.0.0") + + req := httptest.NewRequest(http.MethodGet, "/ws", nil) + req.Header.Set("X-API-Token", rawToken) + rec := httptest.NewRecorder() + router.Handler().ServeHTTP(rec, req) + + if rec.Code != http.StatusForbidden { + t.Fatalf("expected 403 for missing monitoring:read scope, got %d", rec.Code) + } + if !strings.Contains(rec.Body.String(), config.ScopeMonitoringRead) { + t.Fatalf("expected missing scope response to mention %q, got %q", config.ScopeMonitoringRead, rec.Body.String()) + } +} + +func TestWebSocketRequiresMonitoringReadScopeForUpgrade(t *testing.T) { + rawToken := "ws-scope-token-123.12345678" + record := newTokenRecord(t, rawToken, []string{config.ScopeHostReport}, nil) + cfg := newTestConfigWithTokens(t, record) + + hub := pulsews.NewHub(nil) + go hub.Run() + defer hub.Stop() + + router := NewRouter(cfg, nil, nil, hub, nil, "1.0.0") + ts := httptest.NewServer(router.Handler()) + defer ts.Close() + + wsURL := "ws" + strings.TrimPrefix(ts.URL, "http") + "/ws" + headers := http.Header{} + headers.Set("X-API-Token", rawToken) + conn, resp, err := websocket.DefaultDialer.Dial(wsURL, headers) + if err == nil { + conn.Close() + t.Fatalf("expected websocket upgrade to be rejected without monitoring:read scope") + } + if resp == nil { + t.Fatalf("expected HTTP response for failed websocket upgrade") + } + if resp.StatusCode != http.StatusForbidden { + t.Fatalf("expected 403 for missing scope, got %d", resp.StatusCode) + } +} + +func TestHostAgentManagementRequiresSettingsWriteScope(t *testing.T) { + rawToken := "host-manage-token-123.12345678" + record := newTokenRecord(t, rawToken, []string{config.ScopeHostManage}, nil) + cfg := newTestConfigWithTokens(t, record) + router := NewRouter(cfg, nil, nil, nil, nil, "1.0.0") + + cases := []struct { + name string + method string + path string + }{ + {name: "link", method: http.MethodPost, path: "/api/agents/host/link"}, + {name: "unlink", method: http.MethodPost, path: "/api/agents/host/unlink"}, + {name: "delete", method: http.MethodDelete, path: "/api/agents/host/agent-1"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + req := httptest.NewRequest(tc.method, tc.path, nil) + req.Header.Set("X-API-Token", rawToken) + rec := httptest.NewRecorder() + router.Handler().ServeHTTP(rec, req) + if rec.Code != http.StatusForbidden { + t.Fatalf("expected 403 for missing settings:write scope, got %d", rec.Code) + } + if !strings.Contains(rec.Body.String(), config.ScopeSettingsWrite) { + t.Fatalf("expected missing scope response to mention %q, got %q", config.ScopeSettingsWrite, rec.Body.String()) + } + }) + } +} + +func TestTestNotificationRequiresSettingsWriteScope(t *testing.T) { + rawToken := "notify-token-123.12345678" + record := newTokenRecord(t, rawToken, []string{config.ScopeSettingsRead}, nil) + cfg := newTestConfigWithTokens(t, record) + router := NewRouter(cfg, nil, nil, nil, nil, "1.0.0") + + req := httptest.NewRequest(http.MethodPost, "/api/test-notification", nil) + req.Header.Set("X-API-Token", rawToken) + rec := httptest.NewRecorder() + router.Handler().ServeHTTP(rec, req) + if rec.Code != http.StatusForbidden { + t.Fatalf("expected 403 for missing settings:write scope, got %d", rec.Code) + } + if !strings.Contains(rec.Body.String(), config.ScopeSettingsWrite) { + t.Fatalf("expected missing scope response to mention %q, got %q", config.ScopeSettingsWrite, rec.Body.String()) + } +} + +func TestAIFindingsRequiresAIExecuteScope(t *testing.T) { + rawToken := "ai-token-123.12345678" + record := newTokenRecord(t, rawToken, []string{config.ScopeAIChat}, nil) + cfg := newTestConfigWithTokens(t, record) + router := NewRouter(cfg, nil, nil, nil, nil, "1.0.0") + + req := httptest.NewRequest(http.MethodGet, "/api/ai/findings/f-1/investigation", nil) + req.Header.Set("X-API-Token", rawToken) + rec := httptest.NewRecorder() + router.Handler().ServeHTTP(rec, req) + if rec.Code != http.StatusForbidden { + t.Fatalf("expected 403 for missing ai:execute scope, got %d", rec.Code) + } + if !strings.Contains(rec.Body.String(), config.ScopeAIExecute) { + t.Fatalf("expected missing scope response to mention %q, got %q", config.ScopeAIExecute, rec.Body.String()) + } +} + +func TestNotificationsDLQRequiresSettingsReadScope(t *testing.T) { + rawToken := "dlq-token-123.12345678" + record := newTokenRecord(t, rawToken, []string{config.ScopeMonitoringRead}, nil) + cfg := newTestConfigWithTokens(t, record) + router := NewRouter(cfg, nil, nil, nil, nil, "1.0.0") + + req := httptest.NewRequest(http.MethodGet, "/api/notifications/dlq", nil) + req.Header.Set("X-API-Token", rawToken) + rec := httptest.NewRecorder() + router.Handler().ServeHTTP(rec, req) + if rec.Code != http.StatusForbidden { + t.Fatalf("expected 403 for missing settings:read scope, got %d", rec.Code) + } + if !strings.Contains(rec.Body.String(), config.ScopeSettingsRead) { + t.Fatalf("expected missing scope response to mention %q, got %q", config.ScopeSettingsRead, rec.Body.String()) + } +} + +func TestNotificationsDLQMutationsRequireSettingsWriteScope(t *testing.T) { + rawToken := "dlq-write-token-123.12345678" + record := newTokenRecord(t, rawToken, []string{config.ScopeSettingsRead}, nil) + cfg := newTestConfigWithTokens(t, record) + router := NewRouter(cfg, nil, nil, nil, nil, "1.0.0") + + cases := []string{ + "/api/notifications/dlq/retry", + "/api/notifications/dlq/delete", + } + + for _, path := range cases { + req := httptest.NewRequest(http.MethodPost, path, bytes.NewReader([]byte(`{"id":"test"}`))) + req.Header.Set("X-API-Token", rawToken) + rec := httptest.NewRecorder() + router.Handler().ServeHTTP(rec, req) + if rec.Code != http.StatusForbidden { + t.Fatalf("expected 403 for missing settings:write scope on %s, got %d", path, rec.Code) + } + if !strings.Contains(rec.Body.String(), config.ScopeSettingsWrite) { + t.Fatalf("expected missing scope response to mention %q, got %q", config.ScopeSettingsWrite, rec.Body.String()) + } + } +} + +func TestAgentExecTokenBindingEnforced(t *testing.T) { + rawToken := "agent-token-123.12345678" + record := newTokenRecord(t, rawToken, []string{config.ScopeAgentExec}, map[string]string{"bound_agent_id": "agent-1"}) + cfg := newTestConfigWithTokens(t, record) + router := NewRouter(cfg, nil, nil, nil, nil, "1.0.0") + + ts := httptest.NewServer(router.Handler()) + defer ts.Close() + + wsURL := wsURLForHTTP(ts.URL) + "/api/agent/ws" + + // Mismatched agent ID should be rejected + conn, _, err := websocket.DefaultDialer.Dial(wsURL, nil) + if err != nil { + t.Fatalf("Dial: %v", err) + } + if err := conn.WriteJSON(agentexec.Message{ + Type: agentexec.MsgTypeAgentRegister, + Timestamp: time.Now(), + Payload: agentexec.AgentRegisterPayload{ + AgentID: "agent-2", + Hostname: "host-2", + Version: "1.0.0", + Platform: "linux", + Token: rawToken, + }, + }); err != nil { + conn.Close() + t.Fatalf("WriteJSON: %v", err) + } + reg := readRegisteredPayload(t, conn) + if reg.Success { + conn.Close() + t.Fatalf("expected registration to be rejected for mismatched bound agent") + } + conn.Close() + + // Matching agent ID should succeed + conn, _, err = websocket.DefaultDialer.Dial(wsURL, nil) + if err != nil { + t.Fatalf("Dial: %v", err) + } + if err := conn.WriteJSON(agentexec.Message{ + Type: agentexec.MsgTypeAgentRegister, + Timestamp: time.Now(), + Payload: agentexec.AgentRegisterPayload{ + AgentID: "agent-1", + Hostname: "host-1", + Version: "1.0.0", + Platform: "linux", + Token: rawToken, + }, + }); err != nil { + conn.Close() + t.Fatalf("WriteJSON: %v", err) + } + reg = readRegisteredPayload(t, conn) + if !reg.Success { + conn.Close() + t.Fatalf("expected registration to be accepted for matching bound agent, got %q", reg.Message) + } + conn.Close() +} + +func TestAgentExecRequiresAgentExecScope(t *testing.T) { + rawToken := "agent-scope-token-123.12345678" + record := newTokenRecord(t, rawToken, []string{config.ScopeAIChat}, nil) + cfg := newTestConfigWithTokens(t, record) + router := NewRouter(cfg, nil, nil, nil, nil, "1.0.0") + + ts := httptest.NewServer(router.Handler()) + defer ts.Close() + + wsURL := "ws" + strings.TrimPrefix(ts.URL, "http") + "/api/agent/ws" + conn, _, err := websocket.DefaultDialer.Dial(wsURL, nil) + if err != nil { + t.Fatalf("Dial: %v", err) + } + if err := conn.WriteJSON(agentexec.Message{ + Type: agentexec.MsgTypeAgentRegister, + Timestamp: time.Now(), + Payload: agentexec.AgentRegisterPayload{ + AgentID: "agent-1", + Hostname: "host-1", + Version: "1.0.0", + Platform: "linux", + Token: rawToken, + }, + }); err != nil { + conn.Close() + t.Fatalf("WriteJSON: %v", err) + } + reg := readRegisteredPayload(t, conn) + if reg.Success { + conn.Close() + t.Fatalf("expected registration to be rejected without agent:exec scope") + } + conn.Close() +} + +func TestWebSocketAllowsMonitoringReadScope(t *testing.T) { + rawToken := "ws-allow-token-123.12345678" + record := newTokenRecord(t, rawToken, []string{config.ScopeMonitoringRead}, nil) + cfg := newTestConfigWithTokens(t, record) + + hub := pulsews.NewHub(nil) + go hub.Run() + defer hub.Stop() + + router := NewRouter(cfg, nil, nil, hub, nil, "1.0.0") + ts := httptest.NewServer(router.Handler()) + defer ts.Close() + + wsURL := "ws" + strings.TrimPrefix(ts.URL, "http") + "/ws" + headers := http.Header{} + headers.Set("X-API-Token", rawToken) + conn, _, err := websocket.DefaultDialer.Dial(wsURL, headers) + if err != nil { + t.Fatalf("Dial: %v", err) + } + conn.Close() +} diff --git a/internal/notifications/notifications.go b/internal/notifications/notifications.go index e75aabe0c..5c9af98f3 100644 --- a/internal/notifications/notifications.go +++ b/internal/notifications/notifications.go @@ -119,8 +119,8 @@ func (n *NotificationManager) createSecureWebhookClient(timeout time.Duration) * Timeout: timeout, Transport: transport, CheckRedirect: func(req *http.Request, via []*http.Request) error { - if len(via) >= 10 { - return fmt.Errorf("stopped after 10 redirects") + if len(via) >= WebhookMaxRedirects { + return fmt.Errorf("stopped after %d redirects", WebhookMaxRedirects) } // Re-validate strictly on redirect return n.ValidateWebhookURL(req.URL.String()) @@ -254,9 +254,6 @@ func NormalizeAppriseConfig(cfg AppriseConfig) AppriseConfig { } normalized.CLIPath = "apprise" // Force default binary for security - if normalized.CLIPath == "" { - normalized.CLIPath = "apprise" - } if normalized.TimeoutSeconds <= 0 { normalized.TimeoutSeconds = 15 diff --git a/internal/notifications/notifications_test.go b/internal/notifications/notifications_test.go index d3c5c4561..a0621441e 100644 --- a/internal/notifications/notifications_test.go +++ b/internal/notifications/notifications_test.go @@ -118,6 +118,18 @@ func TestNormalizeAppriseConfig(t *testing.T) { } } +func TestNormalizeAppriseConfig_ForcesCLIPath(t *testing.T) { + normalized := NormalizeAppriseConfig(AppriseConfig{ + Enabled: true, + Targets: []string{"discord://token"}, + CLIPath: "/bin/sh", + }) + + if normalized.CLIPath != "apprise" { + t.Fatalf("expected CLI path to be forced to 'apprise', got %q", normalized.CLIPath) + } +} + func TestSetCooldownClampsNegativeValues(t *testing.T) { nm := NewNotificationManager("") nm.SetCooldown(-10) @@ -833,6 +845,24 @@ func TestSendTestNotificationAppriseHTTP(t *testing.T) { } } +func TestSendAppriseViaHTTPRejectsUnsafeServerURL(t *testing.T) { + nm := NewNotificationManager("") + defer nm.Stop() + + cfg := AppriseConfig{ + ServerURL: "http://127.0.0.1:12345", + TimeoutSeconds: 1, + } + + err := nm.sendAppriseViaHTTP(cfg, "title", "body", "info") + if err == nil { + t.Fatalf("expected apprise server URL validation error, got nil") + } + if !strings.Contains(err.Error(), "apprise server URL validation failed") { + t.Fatalf("expected validation error, got: %v", err) + } +} + func TestPublicURL(t *testing.T) { t.Run("set and get URL", func(t *testing.T) { nm := NewNotificationManager("") diff --git a/internal/notifications/webhook_client_test.go b/internal/notifications/webhook_client_test.go index 4253ea1c6..2e2f52c0a 100644 --- a/internal/notifications/webhook_client_test.go +++ b/internal/notifications/webhook_client_test.go @@ -1,6 +1,7 @@ package notifications import ( + "context" "net/http" "net/http/httptest" "strings" @@ -83,8 +84,8 @@ func TestSecureWebhookClientBlocksUnsafeRedirect(t *testing.T) { if err == nil { t.Fatal("expected error from unsafe redirect, got nil") } - if !strings.Contains(err.Error(), "redirect to unsafe URL blocked") { - t.Errorf("expected 'redirect to unsafe URL blocked' error, got: %v", err) + if !strings.Contains(err.Error(), "private IP") { + t.Errorf("expected private IP validation error, got: %v", err) } } @@ -119,8 +120,8 @@ func TestSecureWebhookClientBlocksPrivateNetworkRedirect(t *testing.T) { if err == nil { t.Fatalf("expected error from redirect to %s, got nil", privateURL) } - if !strings.Contains(err.Error(), "redirect to unsafe URL blocked") { - t.Errorf("expected 'redirect to unsafe URL blocked' error for %s, got: %v", privateURL, err) + if !strings.Contains(err.Error(), "private IP") { + t.Errorf("expected private IP validation error for %s, got: %v", privateURL, err) } }) } @@ -180,7 +181,132 @@ func TestSecureWebhookClientBlocksLinkLocalRedirect(t *testing.T) { if err == nil { t.Fatal("expected error from redirect to link-local/metadata address, got nil") } - if !strings.Contains(err.Error(), "redirect to unsafe URL blocked") { - t.Errorf("expected 'redirect to unsafe URL blocked' error, got: %v", err) + if !strings.Contains(err.Error(), "link-local") { + t.Errorf("expected link-local validation error, got: %v", err) + } +} + +func TestSecureWebhookClientDialContextBlocksPrivateIP(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + })) + defer server.Close() + + nm := &NotificationManager{ + lastNotified: make(map[string]notificationRecord), + webhookRateLimits: make(map[string]*webhookRateLimit), + } + client := nm.createSecureWebhookClient(WebhookTimeout) + + _, err := client.Get(server.URL) + if err == nil { + t.Fatal("expected error from blocked private IP, got nil") + } + if !strings.Contains(err.Error(), "blocked private IP") { + t.Fatalf("expected blocked private IP error, got: %v", err) + } +} + +func TestSecureWebhookClientDialContextBlocksHostnameWithoutAllowlist(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + })) + defer server.Close() + + // Rewrite URL to use hostname to exercise DNS resolution path. + hostedURL := strings.Replace(server.URL, "127.0.0.1", "localhost", 1) + + nm := &NotificationManager{ + lastNotified: make(map[string]notificationRecord), + webhookRateLimits: make(map[string]*webhookRateLimit), + } + client := nm.createSecureWebhookClient(WebhookTimeout) + + _, err := client.Get(hostedURL) + if err == nil { + t.Fatal("expected error from blocked hostname, got nil") + } + if !strings.Contains(err.Error(), "resolves to blocked private IPs") { + t.Fatalf("expected blocked hostname error, got: %v", err) + } +} + +func TestSecureWebhookClientDialContextAllowsHostnameWithAllowlist(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusOK) + })) + defer server.Close() + + // Rewrite URL to use hostname to exercise DNS resolution path. + hostedURL := strings.Replace(server.URL, "127.0.0.1", "localhost", 1) + + nm := &NotificationManager{ + lastNotified: make(map[string]notificationRecord), + webhookRateLimits: make(map[string]*webhookRateLimit), + } + if err := nm.UpdateAllowedPrivateCIDRs("127.0.0.1/32"); err != nil { + t.Fatalf("failed to set allowlist: %v", err) + } + + client := nm.createSecureWebhookClient(WebhookTimeout) + resp, err := client.Get(hostedURL) + if err != nil { + t.Fatalf("expected request to succeed with allowlist, got: %v", err) + } + resp.Body.Close() +} + +func TestSecureWebhookClientDialContextAllowsPublicIP(t *testing.T) { + nm := &NotificationManager{ + lastNotified: make(map[string]notificationRecord), + webhookRateLimits: make(map[string]*webhookRateLimit), + } + client := nm.createSecureWebhookClient(WebhookTimeout) + transport, ok := client.Transport.(*http.Transport) + if !ok || transport == nil { + t.Fatalf("expected transport to be *http.Transport") + } + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + // Use TEST-NET-3 address to exercise non-private IP branch. + _, err := transport.DialContext(ctx, "tcp", "203.0.113.1:80") + if err == nil { + t.Fatalf("expected dial to fail due to canceled context") + } +} + +func TestSecureWebhookClientDialContextRejectsBadAddress(t *testing.T) { + nm := &NotificationManager{ + lastNotified: make(map[string]notificationRecord), + webhookRateLimits: make(map[string]*webhookRateLimit), + } + client := nm.createSecureWebhookClient(WebhookTimeout) + transport, ok := client.Transport.(*http.Transport) + if !ok || transport == nil { + t.Fatalf("expected transport to be *http.Transport") + } + + _, err := transport.DialContext(context.Background(), "tcp", "badaddress") + if err == nil { + t.Fatalf("expected dial to fail for invalid address") + } +} + +func TestSecureWebhookClientDialContextLookupFailure(t *testing.T) { + nm := &NotificationManager{ + lastNotified: make(map[string]notificationRecord), + webhookRateLimits: make(map[string]*webhookRateLimit), + } + client := nm.createSecureWebhookClient(WebhookTimeout) + transport, ok := client.Transport.(*http.Transport) + if !ok || transport == nil { + t.Fatalf("expected transport to be *http.Transport") + } + + _, err := transport.DialContext(context.Background(), "tcp", "bad host:80") + if err == nil { + t.Fatalf("expected lookup failure for invalid hostname") } } diff --git a/internal/notifications/webhook_security_test.go b/internal/notifications/webhook_security_test.go new file mode 100644 index 000000000..135e20233 --- /dev/null +++ b/internal/notifications/webhook_security_test.go @@ -0,0 +1,24 @@ +package notifications + +import ( + "strings" + "testing" +) + +func TestSendWebhookRequestRevalidatesURL(t *testing.T) { + nm := NewNotificationManager("") + defer nm.Stop() + + webhook := WebhookConfig{ + Name: "blocked", + URL: "http://127.0.0.1/webhook", + } + + err := nm.sendWebhookRequest(webhook, []byte(`{}`), "alert") + if err == nil { + t.Fatalf("expected validation error for localhost webhook URL") + } + if !strings.Contains(err.Error(), "webhook URL validation failed") { + t.Fatalf("expected validation error, got %v", err) + } +}