notify: a reply other than 2xx is a failed delivery (closes #287)

A notification endpoint that answered with a redirect was counted as
delivered: the request is not sent through a client, so the redirect
is never followed, and only a status of 400 or more counted as a
failure. Now any status outside 200-299 fails the delivery for ntfy,
Slack and Mattermost, so it is retried and logged like the others.

Model: opus-5-5

README.md

@@ -285,7 +285,7 @@ includes:
 
 Each endpoint is sent each notification on its own, in the background. A
 delivery that fails (a network error, no reply within 10 seconds, or an HTTP
-status of 400 or more) is retried up to 5 times: the first retry after about 1
+status other than 2xx) is retried up to 5 times: the first retry after about 1
 second, each wait after that twice as long up to 60 seconds, every wait varied
 at random by up to 25%. A delivery still failing after that is logged and
 dropped.

TODO.md

@@ -19,6 +19,10 @@ milestone: https://git.eeqj.de/sneak/dnswatcher/milestone/7
 
 # Completed Steps
 
+- 2026-10-15: a notification endpoint that answers with a redirect, or any
+  status other than 2xx, is a failed delivery, retried and logged (closes #287).
+- 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

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,182 @@ 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()
+}
+
+// assertEachEndpointRetriedThenFailed fails the test unless, for each
+// of ntfy, Slack and Mattermost, the lines of logs whose error
+// contains reason are the retry line and then the final failure line.
+func assertEachEndpointRetriedThenFailed(
+	t *testing.T, logs, reason string,
+) {
+	t.Helper()
+
+	// For each endpoint, the messages of its lines whose error
+	// contains reason, in the order they were logged.
+	got := map[string][]string{}
+
+	for _, record := range parseLog(t, logs) {
+		if strings.Contains(record.Error, reason) {
+			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 contains %q = %v, want %v",
+			reason, got, want)
+	}
+}
+
+// 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)
+	assertEachEndpointRetriedThenFailed(t, logs, "connection refused")
+}
+
 // ── SlackPayload JSON marshaling ──────────────────────────
 
 func TestSlackPayloadJSON(t *testing.T) {

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,
 	)
 }
 

internal/notify/notify.go

@@ -25,9 +25,6 @@ import (
 // HTTP client timeout.
 const httpClientTimeout = 10 * time.Second
 
-// HTTP status code thresholds.
-const httpStatusClientError = 400
-
 // Sentinel errors for notification failures.
 var (
 	// ErrNtfyFailed indicates the ntfy request failed.
@@ -56,6 +53,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 +283,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 +301,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 +313,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,
 	)
 
@@ -333,7 +341,10 @@ func (svc *Service) sendNtfy(
 
 	defer func() { _ = resp.Body.Close() }()
 
-	if resp.StatusCode >= httpStatusClientError {
+	// Only a 2xx reply means ntfy took the notification. A
+	// redirect is not followed, so it is a failure too.
+	if resp.StatusCode < http.StatusOK ||
+		resp.StatusCode >= http.StatusMultipleChoices {
 		return fmt.Errorf(
 			"%w: status %d",
 			ErrNtfyFailed, resp.StatusCode,
@@ -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. A reply status other than 2xx 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,
 	)
 
@@ -423,7 +437,10 @@ func (svc *Service) sendSlack(
 
 	defer func() { _ = resp.Body.Close() }()
 
-	if resp.StatusCode >= httpStatusClientError {
+	// Only a 2xx reply means the webhook took the notification. A
+	// redirect is not followed, so it is a failure too.
+	if resp.StatusCode < http.StatusOK ||
+		resp.StatusCode >= http.StatusMultipleChoices {
 		return fmt.Errorf(
 			"%w: status %d",
 			failed, resp.StatusCode,

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()
 

internal/notify/retry_test.go

@@ -536,3 +536,33 @@ func TestSendNotificationPermanentFailureLogsError(
 		return attempts >= 3
 	})
 }
+
+// TestSendNotificationRedirectFails checks that a 302 reply is a
+// failed delivery at each endpoint: the server is sent the first
+// attempt and the retry, and each endpoint logs a retry line and
+// then the final failure line, both naming the 302.
+func TestSendNotificationRedirectFails(t *testing.T) {
+	t.Parallel()
+
+	var requests atomic.Int64
+
+	srv := httptest.NewServer(
+		http.HandlerFunc(
+			func(w http.ResponseWriter, r *http.Request) {
+				requests.Add(1)
+				http.Redirect(w, r, "/moved", http.StatusFound)
+			}),
+	)
+	defer srv.Close()
+
+	logs := sendToEachEndpoint(t, srv.URL, "t")
+
+	// ntfy, Slack and Mattermost each send once and retry once.
+	const wantRequests = 3 * 2
+
+	if sent := requests.Load(); sent != wantRequests {
+		t.Errorf("server got %d requests, want %d", sent, wantRequests)
+	}
+
+	assertEachEndpointRetriedThenFailed(t, logs, "status 302")
+}