diff --git a/TODO.md b/TODO.md index 4fb2e39..60e6c82 100644 --- a/TODO.md +++ b/TODO.md @@ -19,6 +19,8 @@ milestone: https://git.eeqj.de/sneak/dnswatcher/milestone/7 # Completed Steps +- 2026-10-15: the log names a notification endpoint (`slack`, `mattermost`, + `ntfy`) and never shows its URL's path or query (closes #288). - 2026-10-13: a hostname or a domain whose zone's nameservers all stop answering gets an NS Failure for each, then an NS Recovery (closes #284). - 2026-10-13: an IPv6 address is written in brackets before its port, as in diff --git a/internal/notify/delivery_test.go b/internal/notify/delivery_test.go index 064af7b..8d699d7 100644 --- a/internal/notify/delivery_test.go +++ b/internal/notify/delivery_test.go @@ -6,10 +6,12 @@ import ( "encoding/json" "errors" "io" + "log/slog" "maps" "net/http" "net/http/httptest" "net/url" + "slices" "strings" "sync" "testing" @@ -38,6 +40,13 @@ const ( prioUrgent = "urgent" ) +// Endpoint names, as the log gives them, used across multiple tests. +const ( + endpointNtfy = "ntfy" + endpointSlack = "slack" + endpointMattermost = "mattermost" +) + // testHost is the hostname used in request construction tests. const testHost = "example.com" @@ -1094,6 +1103,172 @@ func TestSendNotificationErrorNamesEndpoint(t *testing.T) { } } +// ── webhook URLs kept out of the log ────────────────────── + +// webhookPathAndQuery follows the server's address in every +// endpoint's URL in the tests below. A webhook URL's path or query +// lets anyone post to the channel or topic, so neither word marked +// secret in it may reach the log. +const webhookPathAndQuery = "/hooks/secret-in-path?token=secret-in-query" + +// assertNoWebhookSecret fails the test if text holds either word +// marked secret in webhookPathAndQuery. +func assertNoWebhookSecret(t *testing.T, text string) { + t.Helper() + + for _, secret := range []string{"secret-in-path", "secret-in-query"} { + if strings.Contains(text, secret) { + t.Errorf("%q from the webhook URL is in:\n%s", secret, text) + } + } +} + +// logRecord is the part of a JSON log line the tests below read. +type logRecord struct { + Level string `json:"level"` + Msg string `json:"msg"` + Endpoint string `json:"endpoint"` + Title string `json:"title"` + Error string `json:"error"` +} + +// parseLog decodes each line of a JSON log. +func parseLog(t *testing.T, logs string) []logRecord { + t.Helper() + + var records []logRecord + + for line := range strings.Lines(logs) { + var record logRecord + + err := json.Unmarshal([]byte(line), &record) + if err != nil { + t.Fatalf("decoding log line %q: %v", line, err) + } + + records = append(records, record) + } + + return records +} + +// sendToEachEndpoint sends one notification with the given title to +// ntfy, Slack and Mattermost, each at baseURL followed by +// webhookPathAndQuery, and retries a failed delivery once. When +// every delivery has finished it returns the JSON log written, +// debug lines included. +func sendToEachEndpoint(t *testing.T, baseURL, title string) string { + t.Helper() + + target, err := url.Parse(baseURL + webhookPathAndQuery) + if err != nil { + t.Fatalf("parsing webhook URL: %v", err) + } + + svc, logs := newLoggingService(http.DefaultTransport) + svc.SetNtfyURL(target) + svc.SetSlackWebhookURL(target) + svc.SetMattermostWebhookURL(target) + svc.SetSleepFunc(instantSleep) + svc.SetRetryConfig(notify.RetryConfig{ + MaxRetries: 1, + BaseDelay: time.Millisecond, + MaxDelay: time.Millisecond, + }) + + svc.SendNotification(context.Background(), title, "m", prioInfo) + + waitForCondition(t, func() bool { + return svc.OutstandingDeliveries() == 0 + }) + + return logs.String() +} + +// TestSendNotificationDebugLogOmitsWebhookURL checks that, with debug +// logging on, a delivery to each endpoint logs the endpoint's name and +// the title, and no part of the URL's path or query. +func TestSendNotificationDebugLogOmitsWebhookURL(t *testing.T) { + t.Parallel() + + const title = "Port Change" + + srv := httptest.NewServer( + http.HandlerFunc( + func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + }), + ) + defer srv.Close() + + logs := sendToEachEndpoint(t, srv.URL, title) + + assertNoWebhookSecret(t, logs) + + got := map[string]string{} + + for _, record := range parseLog(t, logs) { + if record.Level == slog.LevelDebug.String() { + got[record.Endpoint] = record.Title + } + } + + want := map[string]string{ + endpointNtfy: title, + endpointSlack: title, + endpointMattermost: title, + } + + if !maps.Equal(got, want) { + t.Errorf("debug lines' endpoint and title = %v, want %v", got, want) + } +} + +// TestSendNotificationErrorLogOmitsWebhookURL checks that a delivery +// to each endpoint that cannot connect logs the transport's error, +// which names the refused connection, on its retry line and on its +// final failure line, and that no line holds any part of the URL's +// path or query. +func TestSendNotificationErrorLogOmitsWebhookURL(t *testing.T) { + t.Parallel() + + // Closed before anything is sent, so every attempt fails to + // connect. + srv := httptest.NewServer(http.NotFoundHandler()) + srv.Close() + + logs := sendToEachEndpoint(t, srv.URL, "t") + + assertNoWebhookSecret(t, logs) + + // For each endpoint, the messages of its lines whose error names + // the refused connection, in the order they were logged. + got := map[string][]string{} + + for _, record := range parseLog(t, logs) { + if strings.Contains(record.Error, "connection refused") { + got[record.Endpoint] = append( + got[record.Endpoint], record.Msg, + ) + } + } + + lines := []string{ + "notification delivery failed, retrying", + "failed to send notification after retries", + } + want := map[string][]string{ + endpointNtfy: lines, + endpointSlack: lines, + endpointMattermost: lines, + } + + if !maps.EqualFunc(got, want, slices.Equal) { + t.Errorf("lines whose error names the refused connection = %v, "+ + "want %v", got, want) + } +} + // ── SlackPayload JSON marshaling ────────────────────────── func TestSlackPayloadJSON(t *testing.T) { diff --git a/internal/notify/export_test.go b/internal/notify/export_test.go index 52c3b3d..1324e3c 100644 --- a/internal/notify/export_test.go +++ b/internal/notify/export_test.go @@ -81,7 +81,8 @@ func (svc *Service) SendNtfy( return svc.sendNtfy(ctx, topicURL, title, message, priority) } -// SendSlack exports sendSlack for testing. +// SendSlack exports sendSlack for testing. It sends as the +// "slack" endpoint. func (svc *Service) SendSlack( ctx context.Context, webhookURL *url.URL, @@ -89,7 +90,7 @@ func (svc *Service) SendSlack( title, message, priority string, ) error { return svc.sendSlack( - ctx, webhookURL, failed, title, message, priority, + ctx, webhookURL, "slack", failed, title, message, priority, ) } diff --git a/internal/notify/notify.go b/internal/notify/notify.go index ff9026a..6ed09ac 100644 --- a/internal/notify/notify.go +++ b/internal/notify/notify.go @@ -56,6 +56,15 @@ func IsAllowedScheme(scheme string) bool { func ValidateWebhookURL(raw string) (*url.URL, error) { u, err := url.ParseRequestURI(raw) if err != nil { + // A *url.Error quotes the whole URL, and this error is + // logged at startup. A webhook URL's path or query lets + // anyone post to the channel or topic, so keep only the + // reason. + var urlErr *url.Error + if errors.As(err, &urlErr) { + err = urlErr.Err + } + return nil, fmt.Errorf("invalid URL: %w", err) } @@ -277,7 +286,7 @@ func (svc *Service) dispatchSlack( svc.dispatch(ctx, "slack", func(c context.Context) error { return svc.sendSlack( - c, svc.slackWebhookURL, ErrSlackFailed, + c, svc.slackWebhookURL, "slack", ErrSlackFailed, title, message, priority, ) }) @@ -295,8 +304,8 @@ func (svc *Service) dispatchMattermost( ctx, "mattermost", func(c context.Context) error { return svc.sendSlack( - c, svc.mattermostWebhookURL, ErrMattermostFailed, - title, message, priority, + c, svc.mattermostWebhookURL, "mattermost", + ErrMattermostFailed, title, message, priority, ) }, ) @@ -307,9 +316,11 @@ func (svc *Service) sendNtfy( topicURL *url.URL, title, message, priority string, ) error { + // The topic URL is left out of the log: it lets anyone who has + // it read and post to the topic. svc.log.Debug( "sending ntfy notification", - "topic", topicURL.String(), + "endpoint", "ntfy", "title", title, ) @@ -372,12 +383,15 @@ type SlackAttachment struct { } // sendSlack posts to a Slack or Mattermost incoming webhook, which -// take the same payload. An HTTP error status is returned wrapped in -// failed, ErrSlackFailed or ErrMattermostFailed, so the error names -// the endpoint. +// take the same payload. endpoint, "slack" or "mattermost", names +// the webhook in the log in place of its URL, which lets anyone who +// has it post to the channel. An HTTP error status is returned +// wrapped in failed, ErrSlackFailed or ErrMattermostFailed, so the +// error names the endpoint. func (svc *Service) sendSlack( ctx context.Context, webhookURL *url.URL, + endpoint string, failed error, title, message, priority string, ) error { @@ -388,7 +402,7 @@ func (svc *Service) sendSlack( svc.log.Debug( "sending webhook notification", - "url", webhookURL.String(), + "endpoint", endpoint, "title", title, ) diff --git a/internal/notify/notify_test.go b/internal/notify/notify_test.go index 3ac34dd..d0e49a0 100644 --- a/internal/notify/notify_test.go +++ b/internal/notify/notify_test.go @@ -79,6 +79,23 @@ func TestValidateWebhookURLInvalid(t *testing.T) { } } +// TestValidateWebhookURLErrorOmitsURL checks that the error for a +// webhook URL that does not parse, here because a carriage return +// was left at its end, leaves out the URL's path and query: the +// error is logged at startup. +func TestValidateWebhookURLErrorOmitsURL(t *testing.T) { + t.Parallel() + + _, err := notify.ValidateWebhookURL( + "https://hooks.example.com" + webhookPathAndQuery + "\r", + ) + if err == nil { + t.Fatal("expected an error for a URL ending in a carriage return") + } + + assertNoWebhookSecret(t, err.Error()) +} + func TestIsAllowedScheme(t *testing.T) { t.Parallel()