From 4a981bc2ec0197044615150a9e8165a54f221ad7 Mon Sep 17 00:00:00 2001 From: "pulse-triage[bot]" <249995291+pulse-triage[bot]@users.noreply.github.com> Date: Sun, 6 Sep 2026 21:52:45 +0100 Subject: [PATCH] fix(notifications): prioritise HTTP rejection over retry body hints A provider 403 response mentioning an authentication timeout was treated as a transient network failure and sent again with unchanged credentials. Classify explicit HTTP status before diagnostic text, retaining existing transient status and network fallback behaviour. Add a status/body matrix and a queue-free loopback regression that verifies one request and terminal delivery history. Change-source: pulse-maintainer --- .../v6/internal/subsystems/notifications.md | 13 ++++- internal/notifications/webhook_enhanced.go | 14 ++--- internal/notifications/webhook_retry_test.go | 51 +++++++++++++++++++ 3 files changed, 66 insertions(+), 12 deletions(-) diff --git a/docs/release-control/v6/internal/subsystems/notifications.md b/docs/release-control/v6/internal/subsystems/notifications.md index 8e11b5413..e02bed3a8 100644 --- a/docs/release-control/v6/internal/subsystems/notifications.md +++ b/docs/release-control/v6/internal/subsystems/notifications.md @@ -247,7 +247,18 @@ That same ownership includes webhook retry classification. The canonical retry gate in `webhook_enhanced.go` must parse provider failures from both `status 429`-style and `HTTP 429`-style error strings before it decides whether to retry, so a non-retryable `HTTP 400` result cannot be retried just -because the transport changed its error wording. +because the transport changed its error wording. Explicit HTTP status also +wins over network-like diagnostic text in the response body: a terminal 403 +mentioning "authentication timeout" must stop transport retries, retain the +403 in delivery history, and record zero retries when rejected on the first +attempt. The existing transient HTTP exceptions and retryable fallback for +network or unclassified failures remain unchanged. +`TestIsRetryableWebhookError_StatusOverridesBody` and +`TestWebhookRetryRejectsForbiddenTimeoutBody` in +`internal/notifications/webhook_retry_test.go` pin this precedence with a +status/body matrix and a queue-free loopback receiver that checks request +count and delivery history. These are synthetic transport proofs, not installed +recipient receipts or a change to persistent-queue retry policy. That same notification transport boundary also owns outbound Apprise HTTP URL normalization. Server URLs must be validated as absolute HTTP(S) endpoints without userinfo before request construction, and the `/notify` plus optional diff --git a/internal/notifications/webhook_enhanced.go b/internal/notifications/webhook_enhanced.go index 3f6902cf4..17e89a83f 100644 --- a/internal/notifications/webhook_enhanced.go +++ b/internal/notifications/webhook_enhanced.go @@ -495,17 +495,9 @@ func parseRetryAfterBackoff(retryAfter string, now time.Time) (time.Duration, bo // isRetryableWebhookError determines if a webhook error should trigger a retry func isRetryableWebhookError(err error) bool { - errStr := strings.ToLower(err.Error()) - - // Network-related errors that should be retried - if strings.Contains(errStr, "timeout") || - strings.Contains(errStr, "connection refused") || - strings.Contains(errStr, "connection reset") || - strings.Contains(errStr, "no such host") || - strings.Contains(errStr, "network unreachable") { - return true - } - + // An explicit HTTP rejection takes precedence over diagnostic body text: + // a 403 mentioning "timeout" is not a transport timeout. Errors without + // a recognised status (including network failures) remain retryable below. if statusCode, ok := webhookErrorStatusCode(err); ok { switch statusCode { case http.StatusRequestTimeout, http.StatusMisdirectedRequest, http.StatusLocked, http.StatusTooEarly, http.StatusTooManyRequests: diff --git a/internal/notifications/webhook_retry_test.go b/internal/notifications/webhook_retry_test.go index 588ad54ea..42325bfbf 100644 --- a/internal/notifications/webhook_retry_test.go +++ b/internal/notifications/webhook_retry_test.go @@ -3,6 +3,8 @@ package notifications import ( "errors" "fmt" + "net/http" + "sync/atomic" "testing" ) @@ -320,3 +322,52 @@ func TestIsRetryableWebhookError_NetworkPatterns(t *testing.T) { }) } } + +// Provider diagnostics must not turn a terminal HTTP rejection into a network retry. +func TestIsRetryableWebhookError_StatusOverridesBody(t *testing.T) { + for _, code := range []int{400, 401, 403, 404, 422} { + for _, body := range []string{"authentication timeout", "connection refused", "connection reset", "no such host", "network unreachable"} { + t.Run(fmt.Sprintf("%d/%s", code, body), func(t *testing.T) { + err := fmt.Errorf("webhook returned HTTP %d: %s", code, body) + if isRetryableWebhookError(err) { + t.Fatalf("terminal HTTP %d was retried because of diagnostic %q", code, body) + } + }) + } + } +} + +// Exercise the HTTP transport without starting a persistent queue or workers. +func TestWebhookRetryRejectsForbiddenTimeoutBody(t *testing.T) { + var attempts atomic.Int32 + server := newIPv4HTTPServer(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + attempts.Add(1) + w.Header().Set("Retry-After", "0") + w.WriteHeader(http.StatusForbidden) + fmt.Fprint(w, "authentication timeout: replace credentials") + })) + defer server.Close() + nm := &NotificationManager{} + if err := nm.UpdateAllowedPrivateCIDRs("127.0.0.1"); err != nil { + t.Fatal(err) + } + nm.webhookClient = nm.createSecureWebhookClient(WebhookTimeout) + defer nm.webhookClient.CloseIdleConnections() + err := nm.sendWebhookWithRetry(EnhancedWebhookConfig{ + WebhookConfig: WebhookConfig{Name: "synthetic rejection", URL: server.URL}, + RetryEnabled: true, RetryCount: 1, + }, []byte(`{"test":true}`), "synthetic:alert") + if err == nil { + t.Fatal("expected forbidden delivery failure") + } + if got := attempts.Load(); got != 1 { + t.Errorf("HTTP attempts = %d, want 1", got) + } + history := nm.GetWebhookHistory() + if len(history) != 1 { + t.Fatalf("history length = %d, want 1", len(history)) + } + if history[0].StatusCode != http.StatusForbidden || history[0].Success || history[0].RetryAttempts != 0 { + t.Errorf("history lost terminal rejection: status=%d success=%v retries=%d", history[0].StatusCode, history[0].Success, history[0].RetryAttempts) + } +}