diff --git a/docs/release-control/v6/internal/subsystems/notifications.md b/docs/release-control/v6/internal/subsystems/notifications.md index 212efbade3..bdb750c8bc 100644 --- a/docs/release-control/v6/internal/subsystems/notifications.md +++ b/docs/release-control/v6/internal/subsystems/notifications.md @@ -15,6 +15,33 @@ ## Purpose +### Webhook diagnostics withhold receiver-controlled text + +Ordinary and enhanced firing/recovery senders, including ntfy's plain-text +recovery path, drain at most the existing 1 MiB response cap without retaining +body bytes. Logs, delivery errors/history and the existing Test response carry +structured status, bounded byte counts and incomplete/limit-reached facts, not +arbitrary receiver text. Test keeps its status/string/error shape; a nonempty +response string now explains that the body was withheld. ResponseLogging +enables metadata only. Reaching the read cap does not claim the body exceeded it. + +HTTP status still owns rejection classification and retry decisions, Retry-After +and actual attempt/final-status accounting survive, and incomplete 2xx replies +remain failures. Body-read errors retain their original cause for errors.Is/As +and classification, without formatting its possibly private prose. Requests, +credentials, payload/occurrence identity, TLS/SSRF, destination scope, queue +policy/persistence and existing operator retry authority are unchanged. Existing +historical diagnostics are not rewritten or claimed to be scrubbed. + +`webhook_response_confidentiality_test.go` exercises the real ordinary/enhanced/ +ntfy/retry senders with in-memory HTTP responses: echoed credentials/alert text, +successful/rejected responses, error-prose disclosure, final delivery history, +unchanged requests and response closure. `webhook_response_metadata_test.go` +checks empty/incomplete/at-cap/over-cap bodies and the Test summary. Existing +Test/Gotify and ntfy HTTP-error controls retain status and request expectations +with the new withheld response. These are synthetic source controls, not native +provider acceptance or a cause/fix claim for reported notification failures. + ### Saved webhook identity is consistent for editing and Test Create, Update and Test share one resolver for masked header/custom-field and diff --git a/internal/notifications/notifications.go b/internal/notifications/notifications.go index cf75adfa22..67ea7fa95a 100644 --- a/internal/notifications/notifications.go +++ b/internal/notifications/notifications.go @@ -273,9 +273,11 @@ type NotificationManager struct { } type webhookHTTPResult struct { - statusCode int - headers http.Header - body string + statusCode int + headers http.Header + responseBytes int64 + responseLimitReached bool + responseIncomplete bool } type webhookRequestOptions struct { @@ -3148,25 +3150,8 @@ func (n *NotificationManager) sendResolvedWebhookNtfy(webhook WebhookConfig, ale } defer resp.Body.Close() - // Read response with size limit - limitedReader := io.LimitReader(resp.Body, WebhookMaxResponseSize) - var respBody bytes.Buffer - if _, err := respBody.ReadFrom(limitedReader); err != nil { - log.Warn(). - Err(err). - Str("webhook", webhook.Name). - Str("service", "ntfy"). - Msg("failed to read resolved ntfy webhook response body") - // A rejected recovery has the same authoritative HTTP verdict as a - // firing delivery, even if its diagnostic body is interrupted. Keep - // permanent refusals out of the connectivity retry path. - if resp.StatusCode < 200 || resp.StatusCode >= 300 { - return FailfWithClass(ClassFromHTTPStatus(resp.StatusCode), "ntfy webhook returned HTTP %d: failed to read ntfy webhook response: %w", resp.StatusCode, err) - } - return fmt.Errorf("failed to read ntfy webhook response: %w", err) - } - - if resp.StatusCode >= 200 && resp.StatusCode < 300 { + result, err := readWebhookResponse(resp) + if err == nil { log.Info(). Str("webhook", webhook.Name). Str("service", "ntfy"). @@ -3178,12 +3163,15 @@ func (n *NotificationManager) sendResolvedWebhookNtfy(webhook WebhookConfig, ale } log.Warn(). + Err(err). Str("webhook", webhook.Name). Str("service", "ntfy"). Int("status", resp.StatusCode). - Str("response", respBody.String()). - Msg("resolved ntfy webhook returned non-success status") - return FailfWithClass(ClassFromHTTPStatus(resp.StatusCode), "ntfy webhook returned HTTP %d: %s", resp.StatusCode, respBody.String()) + Int64("responseBytes", result.responseBytes). + Bool("responseLimitReached", result.responseLimitReached). + Bool("responseIncomplete", result.responseIncomplete). + Msg("resolved ntfy webhook response did not complete successfully") + return fmt.Errorf("ntfy %w", err) } // checkWebhookRateLimit checks if a webhook can be sent based on rate limits @@ -3322,46 +3310,25 @@ func (n *NotificationManager) executeWebhookRequest(webhook WebhookConfig, paylo } defer resp.Body.Close() - limitedReader := io.LimitReader(resp.Body, WebhookMaxResponseSize) - var respBody bytes.Buffer - bytesRead, err := respBody.ReadFrom(limitedReader) - if err != nil { - result := &webhookHTTPResult{statusCode: resp.StatusCode, headers: resp.Header.Clone()} - // Headers already establish a rejection even when its diagnostic body - // is interrupted. Preserve that verdict for transport and queue retries, - // rather than turning a terminal HTTP failure into a connectivity retry. - if resp.StatusCode < 200 || resp.StatusCode >= 300 { - return result, FailfWithClass(ClassFromHTTPStatus(resp.StatusCode), "webhook returned HTTP %d: failed to read webhook response: %w", resp.StatusCode, err) - } - return result, fmt.Errorf("failed to read webhook response: %w", err) - } - if bytesRead >= WebhookMaxResponseSize { + result, err := readWebhookResponse(resp) + if result.responseLimitReached { log.Warn(). Str("webhook", webhook.Name). - Int64("bytesRead", bytesRead). + Int64("responseBytes", result.responseBytes). Int("maxSize", WebhookMaxResponseSize). - Msg("webhook response exceeded size limit, truncated") - } - - result := &webhookHTTPResult{ - statusCode: resp.StatusCode, - headers: resp.Header.Clone(), - body: respBody.String(), + Msg("webhook response read limit reached; body withheld") } if opts.responseLogging || resp.StatusCode < 200 || resp.StatusCode >= 300 { log.Debug(). Str("webhook", webhook.Name). Int("status", resp.StatusCode). - Str("response", result.body). + Int64("responseBytes", result.responseBytes). + Bool("responseLimitReached", result.responseLimitReached). + Bool("responseIncomplete", result.responseIncomplete). Msg("webhook response") } - - if resp.StatusCode < 200 || resp.StatusCode >= 300 { - return result, FailfWithClass(ClassFromHTTPStatus(resp.StatusCode), "webhook returned HTTP %d: %s", resp.StatusCode, result.body) - } - - return result, nil + return result, err } func (n *NotificationManager) sendWebhookRequest(webhook WebhookConfig, jsonData []byte, alertType string, eventID string) error { @@ -3410,12 +3377,12 @@ func (n *NotificationManager) sendWebhookRequest(webhook WebhookConfig, jsonData Int("payloadSize", len(jsonData)). Msg("webhook notification sent successfully") - // Log response body only in debug mode for successful requests - if len(result.body) > 0 { + if result.responseBytes > 0 { log.Debug(). Str("webhook", webhook.Name). - Str("response", result.body). - Msg("webhook response body") + Int64("responseBytes", result.responseBytes). + Bool("responseLimitReached", result.responseLimitReached). + Msg("webhook response body withheld") } return nil } else { @@ -3424,9 +3391,8 @@ func (n *NotificationManager) sendWebhookRequest(webhook WebhookConfig, jsonData Str("service", webhook.Service). Str("type", alertType). Int("status", result.statusCode). - Str("response", result.body). Msg("webhook returned non-success status") - return FailfWithClass(ClassFromHTTPStatus(result.statusCode), "webhook returned HTTP %d: %s", result.statusCode, result.body) + return FailfWithClass(ClassFromHTTPStatus(result.statusCode), "webhook returned HTTP %d (response body withheld)", result.statusCode) } } diff --git a/internal/notifications/notifications_additional_test.go b/internal/notifications/notifications_additional_test.go index 4a2b08bf7b..db9ad5110e 100644 --- a/internal/notifications/notifications_additional_test.go +++ b/internal/notifications/notifications_additional_test.go @@ -547,8 +547,8 @@ func TestSendResolvedWebhookNtfyMultipleAlertsHTTPError(t *testing.T) { if !strings.Contains(err.Error(), "ntfy webhook returned HTTP 503") { t.Fatalf("expected HTTP status in error, got %v", err) } - if !strings.Contains(err.Error(), "temporarily unavailable") { - t.Fatalf("expected response body in error, got %v", err) + if strings.Contains(err.Error(), "temporarily unavailable") || !strings.Contains(err.Error(), "response body withheld") { + t.Fatalf("provider text must be withheld while retaining the HTTP verdict: %v", err) } if gotMethod != http.MethodPut { diff --git a/internal/notifications/webhook_enhanced.go b/internal/notifications/webhook_enhanced.go index c586c2e9fb..6b36cfcc49 100644 --- a/internal/notifications/webhook_enhanced.go +++ b/internal/notifications/webhook_enhanced.go @@ -25,7 +25,7 @@ type EnhancedWebhookConfig struct { RetryCount int `json:"retryCount"` FilterRules WebhookFilterRules `json:"filterRules"` CustomFields map[string]interface{} `json:"customFields"` // For template variables - ResponseLogging bool `json:"responseLogging"` // Log response for debugging + ResponseLogging bool `json:"responseLogging"` // Log structured response metadata, never provider text } // WebhookFilterRules defines filtering for this webhook @@ -577,10 +577,10 @@ func (n *NotificationManager) TestEnhancedWebhook(webhook EnhancedWebhookConfig) result, err := n.executeEnhancedWebhookRequest(webhook, payload, WebhookTestTimeout, "Pulse-Monitoring/2.0 (Test)", webhookEventID(testAlert.ID, "alert")) if err != nil { if result != nil { - return result.statusCode, result.body, err + return result.statusCode, result.responseSummary(), err } return 0, "", err } - return result.statusCode, result.body, nil + return result.statusCode, result.responseSummary(), nil } diff --git a/internal/notifications/webhook_enhanced_test.go b/internal/notifications/webhook_enhanced_test.go index 0f4000dad0..259ed11d1a 100644 --- a/internal/notifications/webhook_enhanced_test.go +++ b/internal/notifications/webhook_enhanced_test.go @@ -36,7 +36,7 @@ func TestEnhancedWebhook(t *testing.T) { status, resp, err := nm.TestEnhancedWebhook(webhook) assert.NoError(t, err) assert.Equal(t, http.StatusOK, status) - assert.Equal(t, "ok", resp) + assert.Equal(t, "Response body withheld (2 bytes read)", resp) } func TestGotifyPresetTestDelivery(t *testing.T) { @@ -75,7 +75,7 @@ func TestGotifyPresetTestDelivery(t *testing.T) { status, response, err := nm.TestEnhancedWebhook(BuildEnhancedWebhookTestConfig(basic, "gotify")) require.NoError(t, err) assert.Equal(t, http.StatusOK, status) - assert.JSONEq(t, `{"id":1}`, response) + assert.Equal(t, "Response body withheld (8 bytes read)", response) } func TestShouldSendWebhook(t *testing.T) { diff --git a/internal/notifications/webhook_response.go b/internal/notifications/webhook_response.go new file mode 100644 index 0000000000..751c4b0511 --- /dev/null +++ b/internal/notifications/webhook_response.go @@ -0,0 +1,60 @@ +package notifications + +import ( + "fmt" + "io" + "net/http" +) + +// Receivers can echo credentials, alert content or arbitrary provider text. +// Keep only bounded byte counts and structured delivery facts, not a copy of +// the body that another caller could put in logs, history or a Test response. +func readWebhookResponse(resp *http.Response) (*webhookHTTPResult, error) { + result := &webhookHTTPResult{statusCode: resp.StatusCode, headers: resp.Header.Clone()} + var err error + result.responseBytes, err = io.Copy(io.Discard, io.LimitReader(resp.Body, WebhookMaxResponseSize)) + result.responseLimitReached = result.responseBytes == WebhookMaxResponseSize + result.responseIncomplete = err != nil + if err != nil { + // Preserve identity and classification before hiding the error prose. + // A body-read error can itself contain the receiver's private text. + class := ClassifyNotificationFailureError(err) + if resp.StatusCode < 200 || resp.StatusCode >= 300 { + class = ClassFromHTTPStatus(resp.StatusCode) + } + return result, FailWithClass(class, &webhookResponseReadError{status: resp.StatusCode, cause: err}) + } + if resp.StatusCode < 200 || resp.StatusCode >= 300 { + return result, FailfWithClass(ClassFromHTTPStatus(resp.StatusCode), + "webhook returned HTTP %d: %s (response body withheld)", resp.StatusCode, http.StatusText(resp.StatusCode)) + } + return result, nil +} + +type webhookResponseReadError struct { + status int + cause error +} + +func (e *webhookResponseReadError) Error() string { + if e.status < 200 || e.status >= 300 { + return fmt.Sprintf("webhook returned HTTP %d: failed to read response body (details withheld)", e.status) + } + return "failed to read webhook response body (details withheld)" +} + +func (e *webhookResponseReadError) Unwrap() error { return e.cause } + +func (r *webhookHTTPResult) responseSummary() string { + if r.responseBytes == 0 && !r.responseIncomplete { + return "" + } + suffix := "" + if r.responseIncomplete { + suffix = "; read incomplete" + } else if r.responseLimitReached { + // Reaching the cap is not proof that the body exceeded it. + suffix = "; read limit reached" + } + return fmt.Sprintf("Response body withheld (%d bytes read%s)", r.responseBytes, suffix) +} diff --git a/internal/notifications/webhook_response_confidentiality_test.go b/internal/notifications/webhook_response_confidentiality_test.go new file mode 100644 index 0000000000..f7210bfda5 --- /dev/null +++ b/internal/notifications/webhook_response_confidentiality_test.go @@ -0,0 +1,176 @@ +package notifications + +import ( + "bytes" + "errors" + "fmt" + "io" + "net/http" + "strings" + "testing" + "time" + + "github.com/rs/zerolog" + "github.com/rs/zerolog/log" +) + +// The receiver may echo arbitrary headers, custom fields, URLs or alert text. +// None of these fixtures opens a listener, contacts a provider or uses a queue. +const webhookResponsePrivateText = "synthetic-header-token synthetic-payload-private synthetic-provider-private" + +type trackedWebhookResponse struct { + reader io.Reader + cause error + read int64 + closed bool +} + +func (r *trackedWebhookResponse) Read(p []byte) (int, error) { + n, err := r.reader.Read(p) + r.read += int64(n) + if err == io.EOF && r.cause != nil { + err = r.cause + } + return n, err +} + +func (r *trackedWebhookResponse) Close() error { r.closed = true; return nil } + +func captureWebhookResponseLogs(t *testing.T) *bytes.Buffer { + t.Helper() + var captured bytes.Buffer + previous := log.Logger + log.Logger = zerolog.New(&captured).Level(zerolog.DebugLevel) + t.Cleanup(func() { log.Logger = previous }) + return &captured +} + +func responseFixtureManager(t *testing.T, transport confidentialityTransport) *NotificationManager { + t.Helper() + n := &NotificationManager{webhookClient: &http.Client{Transport: transport}, + webhookRateLimits: make(map[string]*webhookRateLimit)} + if err := n.UpdateAllowedPrivateCIDRs("127.0.0.1/32"); err != nil { + t.Fatal(err) + } + return n +} + +func assertWithheldWebhookResponse(t *testing.T, captured *bytes.Buffer, err error) { + t.Helper() + for _, private := range strings.Fields(webhookResponsePrivateText) { + if strings.Contains(captured.String(), private) || (err != nil && strings.Contains(err.Error(), private)) { + t.Errorf("provider text reached diagnostics: %s", private) + } + } +} + +func TestWebhookResponseConfidentialitySendPaths(t *testing.T) { + for _, path := range []string{"ordinary", "enhanced", "ntfy-resolved"} { + for _, code := range []int{200, 400, 401, 503} { + t.Run(fmt.Sprintf("%s/%d", path, code), func(t *testing.T) { + captured := captureWebhookResponseLogs(t) + body := &trackedWebhookResponse{reader: strings.NewReader(webhookResponsePrivateText)} + requests := 0 + n := responseFixtureManager(t, func(r *http.Request) (*http.Response, error) { + requests++ + if r.URL.Query().Get("token") != "synthetic-header-token" || r.Header.Get("Authorization") != "Bearer synthetic-header-token" { + t.Error("withholding response text changed the request credential") + } + return &http.Response{StatusCode: code, Header: http.Header{"Retry-After": {"0"}}, Body: body}, nil + }) + webhook := WebhookConfig{Name: "response fixture", URL: "http://127.0.0.1/hook?token=synthetic-header-token", + Headers: map[string]string{"Authorization": "Bearer synthetic-header-token"}} + var err error + switch path { + case "ordinary": + err = n.sendWebhookRequest(webhook, []byte(`{"unchanged":true}`), "alert", "fixture:alert") + case "enhanced": + var result *webhookHTTPResult + result, err = n.executeEnhancedWebhookRequest(EnhancedWebhookConfig{WebhookConfig: webhook, ResponseLogging: true}, + []byte(`{"unchanged":true}`), WebhookTimeout, "response-fixture", "fixture:alert") + if result == nil || result.statusCode != code || result.headers.Get("Retry-After") != "0" { + t.Error("structured status or retry hint changed") + } + case "ntfy-resolved": + err = n.sendResolvedWebhookNtfy(webhook, nil, time.Unix(1, 0)) + } + if (err == nil) != (code == 200) || requests != 1 || !body.closed { + t.Fatalf("verdict/request/cleanup changed: error=%v requests=%d closed=%t", err, requests, body.closed) + } + if err != nil && ClassifyNotificationFailureError(err) != ClassFromHTTPStatus(code) { + t.Error("HTTP rejection lost its authoritative failure class") + } + assertWithheldWebhookResponse(t, captured, err) + }) + } + } +} + +func TestWebhookResponseConfidentialityRetryHistory(t *testing.T) { + for _, finalCode := range []int{http.StatusForbidden, http.StatusAccepted} { + t.Run(fmt.Sprint(finalCode), func(t *testing.T) { + captured := captureWebhookResponseLogs(t) + attempts := 0 + payload := []byte(`{"occurrence":"unchanged"}`) + n := responseFixtureManager(t, func(r *http.Request) (*http.Response, error) { + attempts++ + sent, err := io.ReadAll(r.Body) + if err != nil || !bytes.Equal(sent, payload) || r.Header.Get("X-Pulse-Event-ID") != "fixture:alert" { + t.Error("retry changed occurrence or payload") + } + code := http.StatusTooManyRequests + if attempts > 1 { + code = finalCode + } + return &http.Response{StatusCode: code, Header: http.Header{"Retry-After": {"0"}}, + Body: io.NopCloser(strings.NewReader(webhookResponsePrivateText))}, nil + }) + err := n.sendWebhookWithRetry(EnhancedWebhookConfig{ + WebhookConfig: WebhookConfig{Name: "response fixture", URL: "http://127.0.0.1/hook"}, RetryCount: 2, + }, payload, "fixture:alert") + if attempts != 2 || (err == nil) != (finalCode == http.StatusAccepted) { + t.Fatalf("retry verdict changed: attempts=%d error=%v", attempts, err) + } + history := n.GetWebhookHistory() + if len(history) != 1 || history[0].StatusCode != finalCode || history[0].RetryAttempts != 1 || + history[0].Success != (finalCode == http.StatusAccepted) || history[0].PayloadSize != len(payload) { + t.Fatalf("delivery history lost final status or attempt accounting: %+v", history) + } + assertWithheldWebhookResponse(t, captured, err) + assertWithheldWebhookResponse(t, captured, errors.New(history[0].ErrorMessage)) + }) + } +} + +func TestWebhookResponseConfidentialityReadFailure(t *testing.T) { + for _, path := range []string{"shared", "ntfy-resolved"} { + for _, code := range []int{200, 401, 503} { + t.Run(fmt.Sprintf("%s/%d", path, code), func(t *testing.T) { + captured := captureWebhookResponseLogs(t) + cause := fmt.Errorf("%s: %w", webhookResponsePrivateText, io.ErrUnexpectedEOF) + body := &trackedWebhookResponse{reader: strings.NewReader(webhookResponsePrivateText), cause: cause} + n := responseFixtureManager(t, func(*http.Request) (*http.Response, error) { + return &http.Response{StatusCode: code, Header: make(http.Header), Body: body}, nil + }) + webhook := WebhookConfig{Name: "response fixture", URL: "http://127.0.0.1/hook"} + var err error + if path == "shared" { + _, err = n.executeWebhookRequest(webhook, nil, webhookRequestOptions{}) + } else { + err = n.sendResolvedWebhookNtfy(webhook, nil, time.Unix(1, 0)) + } + if !errors.Is(err, cause) || !errors.Is(err, io.ErrUnexpectedEOF) || !body.closed { + t.Fatalf("incomplete reply became success or lost its cause/cleanup: %v closed=%t", err, body.closed) + } + wantClass := ClassFromHTTPStatus(code) + if code == 200 { + wantClass = NotificationFailureConnectivity + } + if ClassifyNotificationFailureError(err) != wantClass || isRetryableWebhookError(err) != (code != 401) { + t.Error("read error changed HTTP verdict or retry policy") + } + assertWithheldWebhookResponse(t, captured, err) + }) + } + } +} diff --git a/internal/notifications/webhook_response_metadata_test.go b/internal/notifications/webhook_response_metadata_test.go new file mode 100644 index 0000000000..5b99f374c6 --- /dev/null +++ b/internal/notifications/webhook_response_metadata_test.go @@ -0,0 +1,50 @@ +package notifications + +import ( + "errors" + "fmt" + "io" + "net/http" + "strings" + "testing" +) + +func TestWebhookResponseMetadataBoundAndSummary(t *testing.T) { + for _, size := range []int{0, 1, WebhookMaxResponseSize - 1, WebhookMaxResponseSize, WebhookMaxResponseSize + 1000} { + for _, code := range []int{200, 403} { + t.Run(fmt.Sprintf("%d/%d", code, size), func(t *testing.T) { + body := &trackedWebhookResponse{reader: strings.NewReader(strings.Repeat("x", size))} + result, err := readWebhookResponse(&http.Response{StatusCode: code, + Header: http.Header{"Retry-After": {"42"}}, Body: body}) + wantBytes := int64(min(size, WebhookMaxResponseSize)) + if (err == nil) != (code == 200) || body.read != wantBytes || result.responseBytes != wantBytes || + result.responseLimitReached != (size >= WebhookMaxResponseSize) || result.responseIncomplete || + result.statusCode != code || result.headers.Get("Retry-After") != "42" { + t.Fatalf("response metadata/verdict/bound changed: result=%+v err=%v bytes=%d", result, err, body.read) + } + wantSummary := "" + if size > 0 { + suffix := "" + if size >= WebhookMaxResponseSize { + suffix = "; read limit reached" + } + wantSummary = fmt.Sprintf("Response body withheld (%d bytes read%s)", wantBytes, suffix) + } + if summary := result.responseSummary(); summary != wantSummary { + t.Errorf("Test summary = %q; want %q", summary, wantSummary) + } + }) + } + } +} + +func TestWebhookResponseMetadataIncompleteSummary(t *testing.T) { + for _, text := range []string{"", webhookResponsePrivateText} { + body := &trackedWebhookResponse{reader: strings.NewReader(text), cause: io.ErrUnexpectedEOF} + result, err := readWebhookResponse(&http.Response{StatusCode: 200, Header: make(http.Header), Body: body}) + if !errors.Is(err, io.ErrUnexpectedEOF) || !result.responseIncomplete || result.responseBytes != int64(len(text)) || + result.responseSummary() != fmt.Sprintf("Response body withheld (%d bytes read; read incomplete)", len(text)) { + t.Fatalf("incomplete response lost cause or Test summary: result=%+v err=%v", result, err) + } + } +}