From f744e0700e48a901b6d53b58379e57c77d9e87f8 Mon Sep 17 00:00:00 2001 From: rcourtman Date: Fri, 24 Jul 2026 11:56:30 +0100 Subject: [PATCH] Fix availability summaries and webhook secret handling --- .../components/Alerts/WebhookConfig.test.tsx | 1 + .../availabilitySettingsModel.test.ts | 35 +++++++++ .../Settings/availabilitySettingsModel.ts | 12 +++- internal/api/notifications.go | 65 ++--------------- internal/notifications/notifications.go | 12 ++-- internal/notifications/templates_test.go | 1 + .../notifications/webhook_enhanced_test.go | 40 +++++++++++ .../notifications/webhook_url_redaction.go | 72 +++++++++++++++++++ .../webhook_url_redaction_test.go | 56 +++++++++++++++ tests/migration/v5_to_v6_test.go | 45 ++++++++++++ 10 files changed, 271 insertions(+), 68 deletions(-) create mode 100644 internal/notifications/webhook_url_redaction.go create mode 100644 internal/notifications/webhook_url_redaction_test.go diff --git a/frontend-modern/src/components/Alerts/WebhookConfig.test.tsx b/frontend-modern/src/components/Alerts/WebhookConfig.test.tsx index 72a7d53e3..160894c27 100644 --- a/frontend-modern/src/components/Alerts/WebhookConfig.test.tsx +++ b/frontend-modern/src/components/Alerts/WebhookConfig.test.tsx @@ -1137,6 +1137,7 @@ describe('WebhookConfig', () => { { service: 'teams', expected: 'Microsoft Teams' }, { service: 'pagerduty', expected: 'PagerDuty' }, { service: 'telegram', expected: 'Telegram' }, + { service: 'gotify', expected: 'Gotify' }, { service: 'ntfy', expected: 'ntfy' }, ]; diff --git a/frontend-modern/src/components/Settings/__tests__/availabilitySettingsModel.test.ts b/frontend-modern/src/components/Settings/__tests__/availabilitySettingsModel.test.ts index c3fdf7605..85666fc77 100644 --- a/frontend-modern/src/components/Settings/__tests__/availabilitySettingsModel.test.ts +++ b/frontend-modern/src/components/Settings/__tests__/availabilitySettingsModel.test.ts @@ -105,6 +105,41 @@ describe('availabilitySettingsModel', () => { }), ]), ).toBe('1 down · 2 enabled'); + expect( + getAvailabilityTargetsSummary([ + target({ + protocol: 'udp', + port: 27015, + status: { + ...target(), + targetId: 'steam-server', + protocol: 'udp', + available: false, + outcome: 'indeterminate', + }, + }), + ]), + ).toBe('1 open or filtered · 1 enabled'); + expect( + getAvailabilityTargetsSummary([ + target({ + id: 'closed-port', + status: { ...target(), targetId: 'closed-port', available: false }, + }), + target({ + id: 'silent-port', + protocol: 'udp', + port: 27015, + status: { + ...target(), + targetId: 'silent-port', + protocol: 'udp', + available: false, + outcome: 'indeterminate', + }, + }), + ]), + ).toBe('1 down · 1 open or filtered · 2 enabled'); expect(getAvailabilityTargetStatusClass(target({ enabled: false }))).toBe( 'bg-surface-alt text-muted', ); diff --git a/frontend-modern/src/components/Settings/availabilitySettingsModel.ts b/frontend-modern/src/components/Settings/availabilitySettingsModel.ts index c62c86398..74ebc339b 100644 --- a/frontend-modern/src/components/Settings/availabilitySettingsModel.ts +++ b/frontend-modern/src/components/Settings/availabilitySettingsModel.ts @@ -125,10 +125,20 @@ export function getAvailabilityTargetStatusClass(target: AvailabilityTarget): st export function getAvailabilityTargetsSummary(targets: readonly AvailabilityTarget[]): string { const enabled = targets.filter((target) => target.enabled).length; + const indeterminate = targets.filter( + (target) => target.enabled && target.status?.outcome === 'indeterminate', + ).length; const down = targets.filter( - (target) => target.enabled && target.status?.available === false, + (target) => + target.enabled && + target.status?.available === false && + target.status.outcome !== 'indeterminate', ).length; if (targets.length === 0) return 'No availability checks configured'; + if (down > 0 && indeterminate > 0) { + return `${down} down · ${indeterminate} open or filtered · ${enabled} enabled`; + } if (down > 0) return `${down} down · ${enabled} enabled`; + if (indeterminate > 0) return `${indeterminate} open or filtered · ${enabled} enabled`; return `${enabled} enabled · ${targets.length} total`; } diff --git a/internal/api/notifications.go b/internal/api/notifications.go index 761f6deb9..6bf3479b5 100644 --- a/internal/api/notifications.go +++ b/internal/api/notifications.go @@ -707,67 +707,10 @@ func (h *NotificationHandlers) GetWebhookHistory(w http.ResponseWriter, r *http. json.NewEncoder(w).Encode(history) } -// redactSecretsFromURL masks tokens and credentials in URLs +// redactSecretsFromURL is retained as the API package boundary for delivery +// history while the canonical redaction policy lives with webhook execution. func redactSecretsFromURL(urlStr string) string { - // Redact common patterns like: - // - /bot123456:ABC-DEF1234ghIkl-zyx57W2v1u123ew11/sendMessage → /botXXX:REDACTED/sendMessage - // - ?token=abc123 → ?token=REDACTED - // - ?apikey=abc123 → ?apikey=REDACTED - - // Redact Telegram bot tokens - if idx := strings.Index(urlStr, "/bot"); idx != -1 { - // Search for next "/" after "/bot" (starting at idx+4) - if endIdx := strings.Index(urlStr[idx+4:], "/"); endIdx != -1 { - urlStr = urlStr[:idx+4] + "REDACTED" + urlStr[idx+4+endIdx:] - } else { - // No trailing slash - token extends to end of URL or query string - if qIdx := strings.Index(urlStr[idx+4:], "?"); qIdx != -1 { - urlStr = urlStr[:idx+4] + "REDACTED" + urlStr[idx+4+qIdx:] - } else { - urlStr = urlStr[:idx+4] + "REDACTED" - } - } - } - - // Redact query parameters with sensitive names - if qIdx := strings.Index(urlStr, "?"); qIdx != -1 { - sensitiveParams := []string{"token", "apikey", "api_key", "key", "secret", "password"} - for _, param := range sensitiveParams { - pattern := param + "=" - // Search for the pattern after the query string starts - searchStart := qIdx - for { - paramIdx := strings.Index(urlStr[searchStart:], pattern) - if paramIdx == -1 { - break - } - paramIdx += searchStart // Convert to absolute index - - // Check that we're at a parameter boundary (after ? or &) - if paramIdx > 0 { - prevChar := urlStr[paramIdx-1] - if prevChar != '?' && prevChar != '&' { - // Not at a boundary - this is part of another param name - // Move past this match and continue searching - searchStart = paramIdx + len(pattern) - continue - } - } - - // Valid match - redact the value - start := paramIdx + len(pattern) - end := start - for end < len(urlStr) && urlStr[end] != '&' && urlStr[end] != '#' { - end++ - } - urlStr = urlStr[:start] + "REDACTED" + urlStr[end:] - // After modification, continue from after the inserted REDACTED - searchStart = start + len("REDACTED") - } - } - } - - return urlStr + return notifications.RedactWebhookURLSecrets(urlStr) } // GetEmailProviders returns available email providers @@ -813,7 +756,7 @@ func (h *NotificationHandlers) TestWebhook(w http.ResponseWriter, r *http.Reques log.Info(). Str("service", webhook.Service). - Str("url", webhook.URL). + Str("url", notifications.RedactWebhookURLSecrets(webhook.URL)). Str("name", webhook.Name). Msg("Testing webhook") diff --git a/internal/notifications/notifications.go b/internal/notifications/notifications.go index d27095e87..8940235a2 100644 --- a/internal/notifications/notifications.go +++ b/internal/notifications/notifications.go @@ -2745,7 +2745,7 @@ func (n *NotificationManager) executeWebhookRequest(webhook WebhookConfig, paylo resp, err := client.Do(req) if err != nil { - return nil, fmt.Errorf("failed to send webhook: %w", err) + return nil, fmt.Errorf("failed to send webhook: %w", redactWebhookTransportError(err)) } defer resp.Body.Close() @@ -2790,7 +2790,7 @@ func (n *NotificationManager) sendWebhookRequest(webhook WebhookConfig, jsonData log.Error(). Err(err). Str("webhook", webhook.Name). - Str("url", webhook.URL). + Str("url", RedactWebhookURLSecrets(webhook.URL)). Msg("webhook URL validation failed at send time - possible DNS rebinding") return fmt.Errorf("webhook URL validation failed: %w", err) } @@ -2799,7 +2799,7 @@ func (n *NotificationManager) sendWebhookRequest(webhook WebhookConfig, jsonData if !n.checkWebhookRateLimit(webhook.URL) { log.Warn(). Str("webhook", webhook.Name). - Str("url", webhook.URL). + Str("url", RedactWebhookURLSecrets(webhook.URL)). Msg("Webhook request dropped due to rate limiting") return fmt.Errorf("rate limit exceeded for webhook %s", webhook.Name) } @@ -3160,7 +3160,7 @@ func (n *NotificationManager) ValidateWebhookURL(webhookURL string) error { } log.Debug(). Str("host", host). - Str("url", webhookURL). + Str("url", RedactWebhookURLSecrets(webhookURL)). Msg("localhost webhook URL allowed via allowlist") } @@ -3183,7 +3183,7 @@ func (n *NotificationManager) ValidateWebhookURL(webhookURL string) error { if n.isIPInAllowlist(ip) { log.Debug(). Str("ip", ip.String()). - Str("url", webhookURL). + Str("url", RedactWebhookURLSecrets(webhookURL)). Msg("webhook URL resolves to private IP in allowlist") } else { return fmt.Errorf("webhook URL resolves to private IP %s - private networks are not allowed for security (configure allowlist in System Settings)", ip.String()) @@ -3207,7 +3207,7 @@ func (n *NotificationManager) ValidateWebhookURL(webhookURL string) error { // This helps prevent SSRF attacks using numeric IPs to bypass filters if u.Scheme == "https" && isNumericIP(host) { log.Warn(). - Str("url", webhookURL). + Str("url", RedactWebhookURLSecrets(webhookURL)). Msg("webhook URL uses numeric IP with HTTPS - certificate validation may fail") } diff --git a/internal/notifications/templates_test.go b/internal/notifications/templates_test.go index 74fe26cf1..0d0f527f8 100644 --- a/internal/notifications/templates_test.go +++ b/internal/notifications/templates_test.go @@ -239,6 +239,7 @@ func TestGetWebhookTemplates_KnownServices(t *testing.T) { "slack", "teams", "pagerduty", + "gotify", "generic", } diff --git a/internal/notifications/webhook_enhanced_test.go b/internal/notifications/webhook_enhanced_test.go index cbbf358f1..e0167322e 100644 --- a/internal/notifications/webhook_enhanced_test.go +++ b/internal/notifications/webhook_enhanced_test.go @@ -1,6 +1,7 @@ package notifications import ( + "encoding/json" "fmt" "io" "net/http" @@ -37,6 +38,45 @@ func TestEnhancedWebhook(t *testing.T) { assert.Equal(t, "ok", resp) } +func TestGotifyPresetTestDelivery(t *testing.T) { + nm := NewNotificationManager("https://pulse.example") + require.NoError(t, nm.UpdateAllowedPrivateCIDRs("127.0.0.1")) + + server := newIPv4HTTPServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + assert.Equal(t, http.MethodPost, r.Method) + assert.Equal(t, "gotify-app-token", r.URL.Query().Get("token")) + assert.Equal(t, "application/json", r.Header.Get("Content-Type")) + + var payload struct { + Message string `json:"message"` + Title string `json:"title"` + Priority int `json:"priority"` + Extras map[string]interface{} `json:"extras"` + } + require.NoError(t, json.NewDecoder(r.Body).Decode(&payload)) + assert.NotEmpty(t, payload.Message) + assert.NotEmpty(t, payload.Title) + assert.Positive(t, payload.Priority) + assert.Contains(t, payload.Extras, "client::display") + assert.Contains(t, payload.Extras, "pulse::alert") + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"id":1}`)) + })) + defer server.Close() + + basic := WebhookConfig{ + Name: "Operations Gotify", + URL: server.URL + "/message?token=gotify-app-token", + Method: http.MethodPost, + Enabled: true, + Service: "gotify", + } + status, response, err := nm.TestEnhancedWebhook(BuildEnhancedWebhookTestConfig(basic, "gotify")) + require.NoError(t, err) + assert.Equal(t, http.StatusOK, status) + assert.JSONEq(t, `{"id":1}`, response) +} + func TestShouldSendWebhook(t *testing.T) { nm := &NotificationManager{} diff --git a/internal/notifications/webhook_url_redaction.go b/internal/notifications/webhook_url_redaction.go new file mode 100644 index 000000000..7f5f80d51 --- /dev/null +++ b/internal/notifications/webhook_url_redaction.go @@ -0,0 +1,72 @@ +package notifications + +import ( + "errors" + "net/url" + "strings" +) + +// RedactWebhookURLSecrets masks credentials commonly embedded in webhook URLs +// while preserving the URL shape needed for operator diagnostics. +func RedactWebhookURLSecrets(urlString string) string { + // Telegram bot credentials are path components rather than query values. + if idx := strings.Index(urlString, "/bot"); idx != -1 { + if endIdx := strings.Index(urlString[idx+4:], "/"); endIdx != -1 { + urlString = urlString[:idx+4] + "REDACTED" + urlString[idx+4+endIdx:] + } else if queryIdx := strings.Index(urlString[idx+4:], "?"); queryIdx != -1 { + urlString = urlString[:idx+4] + "REDACTED" + urlString[idx+4+queryIdx:] + } else { + urlString = urlString[:idx+4] + "REDACTED" + } + } + + queryIndex := strings.Index(urlString, "?") + if queryIndex == -1 { + return urlString + } + + for _, parameter := range []string{"token", "apikey", "api_key", "key", "secret", "password"} { + pattern := parameter + "=" + searchStart := queryIndex + for { + parameterIndex := strings.Index(urlString[searchStart:], pattern) + if parameterIndex == -1 { + break + } + parameterIndex += searchStart + + if parameterIndex > 0 { + previous := urlString[parameterIndex-1] + if previous != '?' && previous != '&' { + searchStart = parameterIndex + len(pattern) + continue + } + } + + valueStart := parameterIndex + len(pattern) + valueEnd := valueStart + for valueEnd < len(urlString) && urlString[valueEnd] != '&' && urlString[valueEnd] != '#' { + valueEnd++ + } + urlString = urlString[:valueStart] + "REDACTED" + urlString[valueEnd:] + searchStart = valueStart + len("REDACTED") + } + } + + return urlString +} + +func redactWebhookTransportError(err error) error { + if err == nil { + return nil + } + + var urlError *url.Error + if !errors.As(err, &urlError) { + return err + } + + redacted := *urlError + redacted.URL = RedactWebhookURLSecrets(urlError.URL) + return &redacted +} diff --git a/internal/notifications/webhook_url_redaction_test.go b/internal/notifications/webhook_url_redaction_test.go new file mode 100644 index 000000000..96bb084f7 --- /dev/null +++ b/internal/notifications/webhook_url_redaction_test.go @@ -0,0 +1,56 @@ +package notifications + +import ( + "errors" + "net/url" + "strings" + "testing" +) + +func TestRedactWebhookURLSecrets(t *testing.T) { + tests := map[string]struct { + input string + want string + }{ + "gotify token": { + input: "https://gotify.example/message?token=gotify-secret", + want: "https://gotify.example/message?token=REDACTED", + }, + "telegram path and query": { + input: "https://api.telegram.org/bot123:secret/send?token=query-secret", + want: "https://api.telegram.org/botREDACTED/send?token=REDACTED", + }, + "unrelated parameters": { + input: "https://example.com/hook?extra_token=visible&channel=ops", + want: "https://example.com/hook?extra_token=visible&channel=ops", + }, + } + + for name, test := range tests { + t.Run(name, func(t *testing.T) { + if got := RedactWebhookURLSecrets(test.input); got != test.want { + t.Fatalf("RedactWebhookURLSecrets() = %q, want %q", got, test.want) + } + }) + } +} + +func TestRedactWebhookTransportErrorPreservesBehaviorWithoutToken(t *testing.T) { + cause := errors.New("connection refused") + original := &url.Error{ + Op: "Post", + URL: "https://gotify.example/message?token=gotify-secret", + Err: cause, + } + + redacted := redactWebhookTransportError(original) + if strings.Contains(redacted.Error(), "gotify-secret") { + t.Fatalf("redacted transport error exposed token: %v", redacted) + } + if !strings.Contains(redacted.Error(), "token=REDACTED") { + t.Fatalf("redacted transport error omitted diagnostic URL shape: %v", redacted) + } + if !errors.Is(redacted, cause) { + t.Fatal("redacted transport error no longer unwraps to its original cause") + } +} diff --git a/tests/migration/v5_to_v6_test.go b/tests/migration/v5_to_v6_test.go index 7d0fa61ae..bfee9418a 100644 --- a/tests/migration/v5_to_v6_test.go +++ b/tests/migration/v5_to_v6_test.go @@ -4,6 +4,7 @@ package migration import ( + "bytes" "encoding/json" "os" "path/filepath" @@ -12,6 +13,7 @@ import ( "github.com/rcourtman/pulse-go-rewrite/internal/alerts" "github.com/rcourtman/pulse-go-rewrite/internal/config" "github.com/rcourtman/pulse-go-rewrite/internal/crypto" + "github.com/rcourtman/pulse-go-rewrite/internal/notifications" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -256,6 +258,49 @@ func TestV5DataDir_EncryptedConfigRoundtrip(t *testing.T) { assert.Len(t, nodesCfg3.PVEInstances, 4) } +func TestV5GotifyWebhookSurvivesV6LoadAndRestart(t *testing.T) { + dataDir, _, _, _, _ := buildV5DataDir(t) + const token = "v5-gotify-app-token" + + // v5.1.36 stored this exact WebhookConfig JSON shape in webhooks.enc with + // the installation's AES-GCM key. Build that fixture directly so this test + // does not accidentally rely on the current SaveWebhooks implementation. + v5Webhooks := []notifications.WebhookConfig{{ + ID: "gotify-v5", + Name: "Operations Gotify", + URL: "https://gotify.example/message?token=" + token, + Method: "POST", + Headers: map[string]string{"Content-Type": "application/json"}, + Enabled: true, + Service: "gotify", + }} + plaintext, err := json.Marshal(v5Webhooks) + require.NoError(t, err) + + cryptoManager, err := crypto.NewCryptoManagerAt(dataDir) + require.NoError(t, err) + encrypted, err := cryptoManager.Encrypt(plaintext) + require.NoError(t, err) + require.NoError(t, os.WriteFile(filepath.Join(dataDir, "webhooks.enc"), encrypted, 0o600)) + + v6Persistence := config.NewConfigPersistence(dataDir) + loaded, err := v6Persistence.LoadWebhooks() + require.NoError(t, err) + require.Len(t, loaded, 1) + assert.Equal(t, v5Webhooks[0], loaded[0]) + + // A fresh persistence instance models the post-upgrade process restart. + restarted := config.NewConfigPersistence(dataDir) + loadedAfterRestart, err := restarted.LoadWebhooks() + require.NoError(t, err) + require.Len(t, loadedAfterRestart, 1) + assert.Equal(t, v5Webhooks[0], loadedAfterRestart[0]) + + stored, err := os.ReadFile(filepath.Join(dataDir, "webhooks.enc")) + require.NoError(t, err) + assert.False(t, bytes.Contains(stored, []byte(token)), "Gotify token must remain encrypted at rest") +} + // TestV5DataDir_EmptyDataDir verifies that v6 starts cleanly against an // empty data directory (brand new installation). func TestV5DataDir_EmptyDataDir(t *testing.T) {