From 77a8e4ee359dc1e2e82344b9c75d8d37c7f8942c Mon Sep 17 00:00:00 2001 From: "pulse-triage[bot]" <249995291+pulse-triage[bot]@users.noreply.github.com> Date: Mon, 7 Sep 2026 17:08:03 +0100 Subject: [PATCH] fix(notifications): mask Slack webhook paths in diagnostics Slack incoming webhook paths contain credentials, but existing diagnostic redaction retained them. Mask standard and legacy Slack/GovSlack paths while preserving host and error-cause diagnostics; cover encoded paths and actual rate-limit logs. Change-source: pulse-maintainer --- .../v6/internal/subsystems/notifications.md | 16 ++++++++++ .../notifications/webhook_url_redaction.go | 14 +++++++++ .../webhook_url_redaction_test.go | 31 +++++++++++++++++++ 3 files changed, 61 insertions(+) diff --git a/docs/release-control/v6/internal/subsystems/notifications.md b/docs/release-control/v6/internal/subsystems/notifications.md index 48e59e7ca..13423eb55 100644 --- a/docs/release-control/v6/internal/subsystems/notifications.md +++ b/docs/release-control/v6/internal/subsystems/notifications.md @@ -699,3 +699,19 @@ non-authority at signs, combined path/query redaction, error unwrapping and actual rate-limit log output. These queue-free tests establish local diagnostic redaction, not destination receipt, installed recovery or release qualification. No claim is made that arbitrary custom path/query secrets are recognised. + +### Slack webhook diagnostic path confidentiality + +The same helper masks paths on the exact `hooks.slack.com` and +`hooks.slack-gov.com` hosts. `/services/` remains as a diagnostic marker; legacy +paths become `/REDACTED`. Matching uses the parsed, case-insensitive hostname +and clears the encoded path representation, so ports and escaped path segments +do not bypass masking. Other hosts retain their diagnostic paths. Userinfo and +known query credentials remain redacted; configured destinations are unchanged. + +[Slack's incoming-webhook documentation](https://docs.slack.dev/messaging/sending-messages-using-incoming-webhooks/) +identifies the webhook URL as secret and documents GovSlack's separate domain. +Queue-free regression tests cover both hosts, encoded and legacy paths, +lookalike/unrelated hosts, transport errors and actual rate-limit log output. +This does not establish customer exposure, recipient receipt or recognition of +arbitrary custom webhook secrets. diff --git a/internal/notifications/webhook_url_redaction.go b/internal/notifications/webhook_url_redaction.go index 9786f4886..af202224e 100644 --- a/internal/notifications/webhook_url_redaction.go +++ b/internal/notifications/webhook_url_redaction.go @@ -21,6 +21,20 @@ func RedactWebhookURLSecrets(urlString string) string { urlString = parsed.String() } + // Slack incoming webhook paths are credentials, including legacy paths. + // Match the parsed host, not a substring, and discard RawPath so escaped + // credentials cannot survive URL.String(). Do not change the destination. + switch strings.ToLower(parsed.Hostname()) { + case "hooks.slack.com", "hooks.slack-gov.com": + if strings.HasPrefix(parsed.Path, "/services/") { + parsed.Path = "/services/REDACTED" + } else { + parsed.Path = "/REDACTED" + } + parsed.RawPath = "" + urlString = parsed.String() + } + // Telegram bot credentials are path components rather than query values. if idx := strings.Index(urlString, "/bot"); idx != -1 { if endIdx := strings.Index(urlString[idx+4:], "/"); endIdx != -1 { diff --git a/internal/notifications/webhook_url_redaction_test.go b/internal/notifications/webhook_url_redaction_test.go index 23f5263ed..edd7a1479 100644 --- a/internal/notifications/webhook_url_redaction_test.go +++ b/internal/notifications/webhook_url_redaction_test.go @@ -16,6 +16,12 @@ func TestRedactWebhookURLSecrets(t *testing.T) { input string want string }{ + "slack": {input: "https://hooks.slack.com/services/T-test/B-test/slack-secret", want: "https://hooks.slack.com/services/REDACTED"}, + "gov slack": {input: "https://hooks.slack-gov.com/services/T-test/B-test/slack-secret?token=query-secret&channel=ops", want: "https://hooks.slack-gov.com/services/REDACTED?token=REDACTED&channel=ops"}, + "slack encoded path and authority": {input: "https://user:password@HOOKS.SLACK.COM:443/serv%69ces/T-test/B-test/slack%2Dsecret", want: "https://REDACTED@HOOKS.SLACK.COM:443/services/REDACTED"}, + "slack legacy path": {input: "https://hooks.slack.com/T-test/B-test/slack-secret", want: "https://hooks.slack.com/REDACTED"}, + "unrelated services path": {input: "https://example.com/services/status", want: "https://example.com/services/status"}, + "slack lookalike": {input: "https://hooks.slack.com.example.org/services/status", want: "https://hooks.slack.com.example.org/services/status"}, "basic auth": { input: "https://hook-user:hook-password@example.com/hook", want: "https://REDACTED@example.com/hook", @@ -117,3 +123,28 @@ func TestWebhookRateLimitLogsRedactURLSecrets(t *testing.T) { t.Fatalf("expected redacted url in logs, got %q", out) } } + +func TestSlackWebhookDiagnosticsRedactPath(t *testing.T) { + const webhookURL = "https://hooks.slack.com/services/T-test/B-test/slack-secret" + cause := errors.New("connection refused") + original := &url.Error{Op: "Post", URL: webhookURL, Err: cause} + redacted := redactWebhookTransportError(original) + if strings.Contains(redacted.Error(), "slack-secret") || !strings.Contains(redacted.Error(), "/services/REDACTED") { + t.Fatalf("unsafe transport diagnostic: %v", redacted) + } + if original.URL != webhookURL || !errors.Is(redacted, cause) { + t.Fatal("transport error identity or cause changed") + } + var captured bytes.Buffer + logger := log.Logger + log.Logger = zerolog.New(&captured) + t.Cleanup(func() { log.Logger = logger }) + nm := &NotificationManager{webhookRateLimits: make(map[string]*webhookRateLimit)} + for range WebhookRateLimitMax + 2 { + nm.checkWebhookRateLimit(webhookURL) + } + out := captured.String() + if !strings.Contains(out, "rate limit exceeded") || !strings.Contains(out, "/services/REDACTED") || strings.Contains(out, "slack-secret") { + t.Fatalf("unsafe rate-limit diagnostic: %s", out) + } +}