diff --git a/docs/release-control/v6/internal/subsystems/notifications.md b/docs/release-control/v6/internal/subsystems/notifications.md index 1a9889e91..290aa0369 100644 --- a/docs/release-control/v6/internal/subsystems/notifications.md +++ b/docs/release-control/v6/internal/subsystems/notifications.md @@ -749,3 +749,27 @@ method suffixes, query URLs, fragments, transport errors and rate-limit logs. This is diagnostic containment, not evidence of customer exposure or recipient delivery. Arbitrary path secrets and unrecognised query credentials remain outside this bounded change. + +### Bounded diagnostic confidentiality: query representations and bypass callers + +Recognised query names are exactly token, apikey, api_key, key, secret and +password after one URL query decode. Every repeated occurrence is masked, +including mixed literal/escaped names. Unrelated names, ordering and values +remain intact; invalid name escapes fail closed. This is diagnostic projection, +not mutation of configured destinations or a claim to recognise arbitrary secrets. + +Resolved ntfy must apply the same transport-error projection before both its +error log and returned error. Common HTTP execution preserves payload bytes, +event identity and error causes; URLs containing userinfo remain rejected by +outbound validation even though historical diagnostic userinfo is masked. + +The caller matrix and retained Delivery regression tests exercise URL/message +helpers, actual rate-limit logs, common transport and resolved-ntfy transport +errors/logs with synthetic secrets. HTTP delivery-log regression verifies encoded +and repeated query credentials while retaining diagnostic context and entry +identity. Existing exact-output tables bound Slack/GovSlack/legacy, Discord, +Telegram/local paths, malformed URLs and non-secret lookalikes. Earlier proof +missed decoded query representations and a separate ntfy transport caller: +provider-only helper examples were not sufficient sink coverage. This contract +does not assert arbitrary response-body/third-party error secrecy, installed +recipient delivery, candidate qualification or historical customer exposure. diff --git a/internal/api/alerting/notifications_health_test.go b/internal/api/alerting/notifications_health_test.go index 436c71f22..7d4114044 100644 --- a/internal/api/alerting/notifications_health_test.go +++ b/internal/api/alerting/notifications_health_test.go @@ -333,6 +333,7 @@ func containsAny(value string, needles ...string) bool { func TestGetDeliveryLogDiagnosticContext(t *testing.T) { for _, tc := range []struct{ name, input, want string }{ {"plain", "connection refused", "connection refused"}, + {"encoded repeated query", "Post https://example.test/hook?%74oken=secret&token=secret&channel=ops failed", "Post https://example.test/hook?%74oken=REDACTED&token=REDACTED&channel=ops failed"}, {"userinfo", "Post https://user:password@example.test/hook: timeout", "Post https://REDACTED@example.test/hook: timeout"}, {"malformed", "Post https://user:password@example.test/%zz: timeout", "[invalid webhook URL]"}, } { diff --git a/internal/notifications/notifications.go b/internal/notifications/notifications.go index c043a5c58..49d879617 100644 --- a/internal/notifications/notifications.go +++ b/internal/notifications/notifications.go @@ -2776,6 +2776,7 @@ func (n *NotificationManager) sendResolvedWebhookNtfy(webhook WebhookConfig, ale resp, err := n.webhookClient.Do(req) if err != nil { + err = redactWebhookTransportError(err) log.Error(). Err(err). Str("webhook", webhook.Name). diff --git a/internal/notifications/webhook_confidentiality_contract_test.go b/internal/notifications/webhook_confidentiality_contract_test.go new file mode 100644 index 000000000..3daf14ce9 --- /dev/null +++ b/internal/notifications/webhook_confidentiality_contract_test.go @@ -0,0 +1,158 @@ +package notifications + +import ( + "bytes" + "errors" + "fmt" + "github.com/rs/zerolog" + "github.com/rs/zerolog/log" + "io" + "net/http" + "net/url" + "strings" + "testing" + "time" +) + +// Bounded Delivery assessment. All credentials are synthetic; no network sends. +func TestDeliveryEncodedQueryConfidentiality(t *testing.T) { + for _, key := range []string{"token", "apikey", "api_key", "key", "secret", "password"} { + t.Run(key, func(t *testing.T) { + encoded := "%" + "74" + key[1:] + // Encode the first byte of each already-supported query key. + switch key[0] { + case 'a': + encoded = "%61" + key[1:] + case 'k': + encoded = "%6b" + key[1:] + case 's': + encoded = "%73" + key[1:] + case 'p': + encoded = "%70" + key[1:] + } + target := "https://example.test/hook?" + encoded + "=delivery-fixture-secret&channel=ops" + parsed, err := url.Parse(target) + if err != nil || parsed.Query().Get(key) != "delivery-fixture-secret" { + t.Fatal("invalid fixture") + } + if strings.Contains(RedactWebhookURLSecrets(target), "delivery-fixture-secret") { + t.Error("URL helper exposes encoded-key credential") + } + if strings.Contains(RedactWebhookDiagnosticSecrets("Post "+target+" failed"), "delivery-fixture-secret") { + t.Error("diagnostic helper exposes encoded-key credential") + } + var captured bytes.Buffer + original := log.Logger + log.Logger = zerolog.New(&captured) + defer func() { log.Logger = original }() + manager := &NotificationManager{webhookRateLimits: make(map[string]*webhookRateLimit)} + for range WebhookRateLimitMax + 2 { + manager.checkWebhookRateLimit(target) + } + if !strings.Contains(captured.String(), "rate limit exceeded") { + t.Fatal("log path not reached") + } + if strings.Contains(captured.String(), "delivery-fixture-secret") { + t.Error("rate-limit log exposes encoded-key credential") + } + }) + } +} + +type deliveryFailTransport struct{ cause error } + +func (d deliveryFailTransport) RoundTrip(*http.Request) (*http.Response, error) { return nil, d.cause } +func TestDeliveryResolvedNtfyConfidentiality(t *testing.T) { + for _, key := range []string{"token", "apikey", "api_key", "key", "secret", "password"} { + t.Run(key, func(t *testing.T) { testResolvedNtfyConfidentiality(t, key) }) + } +} +func testResolvedNtfyConfidentiality(t *testing.T, key string) { + manager := &NotificationManager{webhookRateLimits: make(map[string]*webhookRateLimit)} + if err := manager.UpdateAllowedPrivateCIDRs("127.0.0.1/32"); err != nil { + t.Fatal(err) + } + cause := errors.New("synthetic transport failure") + manager.webhookClient = &http.Client{Transport: deliveryFailTransport{cause}} + var captured bytes.Buffer + original := log.Logger + log.Logger = zerolog.New(&captured) + defer func() { log.Logger = original }() + err := manager.sendResolvedWebhookNtfy(WebhookConfig{Name: "fixture", Service: "ntfy", URL: fmt.Sprintf("http://127.0.0.1/topic?%s=delivery-fixture-secret&%%%02x%s=delivery-fixture-secret", key, key[0], key[1:])}, nil, time.Now()) + if !errors.Is(err, cause) { + t.Fatalf("transport not reached: %v", err) + } + if strings.Contains(err.Error(), "delivery-fixture-secret") { + t.Error("resolved ntfy transport error exposes literal token credential") + } + if !strings.Contains(captured.String(), "failed to send resolved ntfy webhook") { + t.Fatal("error log not reached") + } + if strings.Contains(captured.String(), "delivery-fixture-secret") { + t.Error("resolved ntfy error log exposes literal token credential") + } +} + +// A finite cross-sink matrix: no network sends or persistent queue. +func TestWebhookConfidentialityCallerMatrix(t *testing.T) { + targets := []string{ + "https://fixture-user:fixture-secret@example.test/hook", + "https://hooks.slack.com/services/team/id/fixture-secret", + "https://hooks.slack-gov.com/legacy/fixture-secret", + "https://discord.com/api/v10/webhooks/id/fixture-secret", + "https://discordapp.com/api/webhooks/id/fixture-secret", + "https://api.telegram.org/%62otfixture-secret/sendMessage", + "http://127.0.0.1:8081/botfixture-secret/sendMessage", + } + for _, key := range []string{"token", "apikey", "api_key", "key", "secret", "password"} { + encoded := fmt.Sprintf("%%%02x%s", key[0], key[1:]) + targets = append(targets, "https://example.test/hook?"+key+"=fixture-secret&"+encoded+"=fixture-secret&channel=ops") + } + for _, target := range targets { + t.Run(target, func(t *testing.T) { + want := RedactWebhookURLSecrets(target) + if strings.Contains(want, "fixture-secret") || strings.Contains(want, "fixture-user") { + t.Fatal("unsafe URL") + } + if got := RedactWebhookDiagnosticSecrets("Post " + target + " failed"); got != "Post "+want+" failed" { + t.Fatalf("context lost: %s", got) + } + var captured bytes.Buffer + original := log.Logger + log.Logger = zerolog.New(&captured) + defer func() { log.Logger = original }() + manager := &NotificationManager{webhookRateLimits: make(map[string]*webhookRateLimit)} + for range WebhookRateLimitMax + 2 { + manager.checkWebhookRateLimit(target) + } + if strings.Contains(captured.String(), "fixture-secret") || !strings.Contains(captured.String(), "rate limit exceeded") { + t.Fatal("unsafe/missing rate-limit log") + } + cause := errors.New("synthetic transport failure") + payload := []byte(`{"event":"unchanged"}`) + sent := false + manager.webhookClient = &http.Client{Transport: confidentialityTransport(func(req *http.Request) (*http.Response, error) { + sent = true + body, err := io.ReadAll(req.Body) + if err != nil || !bytes.Equal(body, payload) || req.URL.String() != target || req.Header.Get("X-Pulse-Event-ID") != "event-1" { + t.Error("request identity changed") + } + return nil, cause + })} + _, err := manager.executeWebhookRequest(WebhookConfig{URL: target}, payload, webhookRequestOptions{eventID: "event-1"}) + if strings.Contains(target, "fixture-user") { + if sent || err == nil || !strings.Contains(err.Error(), "URL userinfo is not allowed") || strings.Contains(err.Error(), "fixture-secret") { + t.Fatalf("userinfo must be safely rejected: %v", err) + } + return + } + if !sent || !errors.Is(err, cause) || strings.Contains(err.Error(), "fixture-secret") || !strings.Contains(err.Error(), "synthetic transport failure") { + t.Fatalf("unsafe/missing transport failure: %v", err) + } + }) + } +} + +type confidentialityTransport func(*http.Request) (*http.Response, error) + +func (f confidentialityTransport) RoundTrip(req *http.Request) (*http.Response, error) { return f(req) } diff --git a/internal/notifications/webhook_url_redaction.go b/internal/notifications/webhook_url_redaction.go index 41e8dc609..9a8fa77b2 100644 --- a/internal/notifications/webhook_url_redaction.go +++ b/internal/notifications/webhook_url_redaction.go @@ -60,39 +60,29 @@ func RedactWebhookURLSecrets(urlString string) string { urlString = parsed.String() } - 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 + // Decode names exactly once, as net/url does, but retain the original + // spelling, order and unrelated values in diagnostic URLs. Inspect every + // occurrence rather than Query().Get(), which would miss repeated keys. + parts := strings.Split(parsed.RawQuery, "&") + changed := false + for i, part := range parts { + name, _, hasValue := strings.Cut(part, "=") + decoded, err := url.QueryUnescape(name) + if err != nil { + return invalidWebhookURLDiagnostic + } + switch decoded { + case "token", "apikey", "api_key", "key", "secret", "password": + if hasValue { + parts[i] = name + "=REDACTED" + changed = true } - 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") } } - + if changed { + parsed.RawQuery = strings.Join(parts, "&") + return parsed.String() + } return urlString } diff --git a/internal/notifications/webhook_url_redaction_test.go b/internal/notifications/webhook_url_redaction_test.go index b6ddfe018..6f6babd5c 100644 --- a/internal/notifications/webhook_url_redaction_test.go +++ b/internal/notifications/webhook_url_redaction_test.go @@ -16,6 +16,8 @@ func TestRedactWebhookURLSecrets(t *testing.T) { input string want string }{ + "malformed query name": {input: "https://example.test/hook?%zz=secret", want: invalidWebhookURLDiagnostic}, + "encoded lookalike": {input: "https://example.test/hook?extra_%74oken=visible&channel=ops#fragment", want: "https://example.test/hook?extra_%74oken=visible&channel=ops#fragment"}, "discord": {input: "https://discord.com/api/webhooks/123/discord-secret", want: "https://discord.com/api/webhooks/REDACTED"}, "discord versioned": {input: "https://discord.com/api/v10/webhooks/123/discord-secret", want: "https://discord.com/api/v10/webhooks/REDACTED"}, "discord legacy": {input: "https://discordapp.com/api/webhooks/123/discord-secret", want: "https://discordapp.com/api/webhooks/REDACTED"},