From f3366a34ab9861768cdafa122f3c4b2ef62489ad Mon Sep 17 00:00:00 2001 From: "pulse-triage[bot]" <249995291+pulse-triage[bot]@users.noreply.github.com> Date: Mon, 7 Sep 2026 18:07:11 +0100 Subject: [PATCH 1/6] fix(notifications): backport Discord diagnostic credential masking Backport-of: 3c77ecb338ee3d68f1dd7d57a367b4727040a61c (code and tests only) Change-source: pulse-maintainer --- .../release-v6.4-discord-redaction/README.md | 23 ++++++++++++++ .../v6/internal/subsystems/notifications.md | 14 +++++++++ .../notifications/webhook_url_redaction.go | 9 ++++++ .../webhook_url_redaction_test.go | 31 +++++++++++++++++++ 4 files changed, 77 insertions(+) create mode 100644 docs/qualification/release-v6.4-discord-redaction/README.md diff --git a/docs/qualification/release-v6.4-discord-redaction/README.md b/docs/qualification/release-v6.4-discord-redaction/README.md new file mode 100644 index 0000000000..4424159fbd --- /dev/null +++ b/docs/qualification/release-v6.4-discord-redaction/README.md @@ -0,0 +1,23 @@ +# Discord diagnostic credential backport + +Security backport of Core commit 3c77ecb338ee3d68f1dd7d57a367b4727040a61c, +limited to the notification redactor and its regression tests. Eligible under +Release Train rules 2/4: synthetic tests on release-line base +acb841d3d667545a493033fad48a810db84586b5 exposed Discord webhook path tokens. +Discord documents secure webhook tokens and token-authorised operations: +https://docs.discord.com/developers/resources/webhook (read 7 September 2026). + +The change masks the suffix after /webhooks/ on exact Discord hosts, including +legacy/versioned paths and escaped credentials. It modifies diagnostics, not +request destinations, and preserves transport error causes. + +Validation: importing tests alone reproduced failures in helper output and +transport diagnostics. With the repair, focused race tests repeated 20 times +passed, covering helper cases, transport errors and actual rate-limit logs. +Logs are retained in the lane packet 20260907T170524Z-release-line. +No full suite, release qualification or installed delivery test was performed. + +This does not scrub historical delivery text or establish customer exposure. +Telegram parsing review remains separate. Protected PR checks, steward risk +assessment and exact-candidate qualification remain required; prior adverse +latency and excluded crash evidence are not cleared by these focused tests. diff --git a/docs/release-control/v6/internal/subsystems/notifications.md b/docs/release-control/v6/internal/subsystems/notifications.md index ea379e9ac6..e43b8786a4 100644 --- a/docs/release-control/v6/internal/subsystems/notifications.md +++ b/docs/release-control/v6/internal/subsystems/notifications.md @@ -603,3 +603,17 @@ 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. + +### Discord webhook diagnostic path confidentiality + +On exact `discord.com` and legacy `discordapp.com` hosts, the shared redactor +masks the suffix after `/webhooks/`, including the webhook ID and token. +Versioned API prefixes remain visible; encoded paths, host casing and ports +cannot bypass masking. Unrelated paths and lookalike hosts are unchanged. +Configured destinations and transport error causes are not modified. + +[Discord's webhook reference](https://docs.discord.com/developers/resources/webhook) +identifies the secure webhook token and token-authorised operations. Focused +synthetic regressions cover helper output, transport diagnostics and actual +rate-limit logs. This is not evidence of customer exposure, recipient receipt, +release qualification, or protection of arbitrary custom-host credentials. diff --git a/internal/notifications/webhook_url_redaction.go b/internal/notifications/webhook_url_redaction.go index af202224e7..26ba2f75e0 100644 --- a/internal/notifications/webhook_url_redaction.go +++ b/internal/notifications/webhook_url_redaction.go @@ -33,6 +33,15 @@ func RedactWebhookURLSecrets(urlString string) string { } parsed.RawPath = "" urlString = parsed.String() + case "discord.com", "discordapp.com": + // Discord webhook IDs and tokens follow /webhooks/ in both + // unversioned and versioned API paths. Mask the entire suffix, + // including escaped credentials and compatibility endpoint paths. + if idx := strings.Index(parsed.Path, "/webhooks/"); idx != -1 { + parsed.Path = parsed.Path[:idx] + "/webhooks/REDACTED" + parsed.RawPath = "" + urlString = parsed.String() + } } // Telegram bot credentials are path components rather than query values. diff --git a/internal/notifications/webhook_url_redaction_test.go b/internal/notifications/webhook_url_redaction_test.go index edd7a1479c..ec2ff8290b 100644 --- a/internal/notifications/webhook_url_redaction_test.go +++ b/internal/notifications/webhook_url_redaction_test.go @@ -16,6 +16,13 @@ func TestRedactWebhookURLSecrets(t *testing.T) { input string want string }{ + "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"}, + "discord encoded": {input: "https://user:password@DISCORD.COM:443/api/webhooks/123/discord%2Dsecret", want: "https://REDACTED@DISCORD.COM:443/api/webhooks/REDACTED"}, + "discord query": {input: "https://discord.com/api/webhooks/123/discord-secret?wait=true&token=query-secret", want: "https://discord.com/api/webhooks/REDACTED?wait=true&token=REDACTED"}, + "discord lookalike": {input: "https://discord.com.example.org/api/webhooks/status", want: "https://discord.com.example.org/api/webhooks/status"}, + "discord unrelated": {input: "https://discord.com/api/status", want: "https://discord.com/api/status"}, "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"}, @@ -148,3 +155,27 @@ func TestSlackWebhookDiagnosticsRedactPath(t *testing.T) { t.Fatalf("unsafe rate-limit diagnostic: %s", out) } } +func TestDiscordWebhookDiagnosticsRedactPath(t *testing.T) { + const webhookURL = "https://discord.com/api/webhooks/123/discord-secret" + cause := errors.New("connection refused") + original := &url.Error{Op: "Post", URL: webhookURL, Err: cause} + redacted := redactWebhookTransportError(original) + if strings.Contains(redacted.Error(), "discord-secret") || !strings.Contains(redacted.Error(), "/api/webhooks/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, "/api/webhooks/REDACTED") || strings.Contains(out, "discord-secret") { + t.Fatalf("unsafe rate-limit diagnostic: %s", out) + } +} From 71c721ccee8bea8a64a130fdbc0ea78292ff748b Mon Sep 17 00:00:00 2001 From: "pulse-triage[bot]" <249995291+pulse-triage[bot]@users.noreply.github.com> Date: Mon, 7 Sep 2026 18:32:34 +0100 Subject: [PATCH 2/6] fix(notifications): backport Telegram decoded-path credential masking Backport reviewed Core 192a72e05c06b2a2ff3a04bb2ef53ded78950e47 with candidate-specific before/after evidence. Change-source: pulse-maintainer Change-source: pulse-maintainer --- .../release-v6.4-telegram-redaction/README.md | 25 +++++++++++++++ .../release-v6.4-telegram-redaction/after.log | 1 + .../before.log | 18 +++++++++++ .../v6/internal/subsystems/notifications.md | 16 ++++++++++ .../notifications/webhook_url_redaction.go | 19 ++++++----- .../webhook_url_redaction_test.go | 32 +++++++++++++++++++ 6 files changed, 103 insertions(+), 8 deletions(-) create mode 100644 docs/qualification/release-v6.4-telegram-redaction/README.md create mode 100644 docs/qualification/release-v6.4-telegram-redaction/after.log create mode 100644 docs/qualification/release-v6.4-telegram-redaction/before.log diff --git a/docs/qualification/release-v6.4-telegram-redaction/README.md b/docs/qualification/release-v6.4-telegram-redaction/README.md new file mode 100644 index 0000000000..d6a9d59bc2 --- /dev/null +++ b/docs/qualification/release-v6.4-telegram-redaction/README.md @@ -0,0 +1,25 @@ +# Telegram diagnostic credential backport — 7 September 2026 + +Release Train rules 2/4 permit this named security repair on base +`dcf7e499613679c941abe49800f1294b5714fde1`. Backports only code, tests and +notification contract from reviewed Core `192a72e05c06b2a2ff3a04bb2ef53ded78950e47`. +Existing Discord masking and diagnostic context handling are preserved. + +Fresh independent primary evidence: https://core.telegram.org/bots/api#making-requests +identifies bot authentication tokens in request paths and supports local API +servers. This is diagnostic containment, not a new product surface. + +Tests imported alone failed (before.log, exit 1): +`go test ./internal/notifications -run '^Test(RedactWebhookURLSecrets|TelegramWebhookDiagnosticsRedactPath)$' -count=1`. +Synthetic escaped path credentials survived helper and transport diagnostics; +non-credential bot hostnames/query URLs also lost diagnostic context. + +Repair validation (after.log): +`go test -race ./internal/notifications -run '^Test(RedactWebhook|WebhookRateLimitLogsRedactURLSecrets|SlackWebhookDiagnosticsRedactPath|DiscordWebhookDiagnosticsRedactPath|TelegramWebhookDiagnosticsRedactPath)' -count=20`. + +No full qualification, installed change, customer exposure assertion, historical +stored-data cleanup or recipient receipt proof. Protected integration and all +eight enforced checks remain required. Existing adverse latency and excluded +crash evidence are not cleared. Delivery owns release maturity and publication. + +Result: exit 0, all selected tests passed with race detection (20 repeats). diff --git a/docs/qualification/release-v6.4-telegram-redaction/after.log b/docs/qualification/release-v6.4-telegram-redaction/after.log new file mode 100644 index 0000000000..fd6ae4f2bf --- /dev/null +++ b/docs/qualification/release-v6.4-telegram-redaction/after.log @@ -0,0 +1 @@ +ok github.com/rcourtman/pulse-go-rewrite/internal/notifications 1.142s diff --git a/docs/qualification/release-v6.4-telegram-redaction/before.log b/docs/qualification/release-v6.4-telegram-redaction/before.log new file mode 100644 index 0000000000..23283aefc4 --- /dev/null +++ b/docs/qualification/release-v6.4-telegram-redaction/before.log @@ -0,0 +1,18 @@ +--- FAIL: TestRedactWebhookURLSecrets (0.00s) + --- FAIL: TestRedactWebhookURLSecrets/telegram_escaped_prefix (0.00s) + webhook_url_redaction_test.go:76: RedactWebhookURLSecrets() = "https://api.telegram.org/%62ot123:telegram-secret/sendMessage", want "https://api.telegram.org/botREDACTED/sendMessage" + --- FAIL: TestRedactWebhookURLSecrets/telegram_local_escaped_prefix (0.00s) + webhook_url_redaction_test.go:76: RedactWebhookURLSecrets() = "http://localhost:8081/%62ot123:telegram-secret/sendMessage", want "http://localhost:8081/botREDACTED/sendMessage" + --- FAIL: TestRedactWebhookURLSecrets/telegram_no_method_with_URL_query (0.00s) + webhook_url_redaction_test.go:76: RedactWebhookURLSecrets() = "https://api.telegram.org/botREDACTED//example.org/status", want "https://api.telegram.org/botREDACTED?next=https://example.org/status" + --- FAIL: TestRedactWebhookURLSecrets/bot_query_is_not_a_path (0.00s) + webhook_url_redaction_test.go:76: RedactWebhookURLSecrets() = "https://example.org/hook?next=https://botREDACTED/status", want "https://example.org/hook?next=https://bot.example.org/status" + --- FAIL: TestRedactWebhookURLSecrets/telegram_fragment (0.00s) + webhook_url_redaction_test.go:76: RedactWebhookURLSecrets() = "https://api.telegram.org/botREDACTED", want "https://api.telegram.org/botREDACTED#diagnostic" + --- FAIL: TestRedactWebhookURLSecrets/bot_hostname_is_not_a_path (0.00s) + webhook_url_redaction_test.go:76: RedactWebhookURLSecrets() = "https://botREDACTED/hook?channel=ops", want "https://bot.example.org/hook?channel=ops" +--- FAIL: TestTelegramWebhookDiagnosticsRedactPath (0.00s) + webhook_url_redaction_test.go:228: unsafe transport diagnostic: Post "https://api.telegram.org/%62ot123:telegram-secret/sendMessage": connection refused +FAIL +FAIL github.com/rcourtman/pulse-go-rewrite/internal/notifications 0.005s +FAIL diff --git a/docs/release-control/v6/internal/subsystems/notifications.md b/docs/release-control/v6/internal/subsystems/notifications.md index bb5c4ecef8..670f8af00e 100644 --- a/docs/release-control/v6/internal/subsystems/notifications.md +++ b/docs/release-control/v6/internal/subsystems/notifications.md @@ -621,3 +621,19 @@ identifies the secure webhook token and token-authorised operations. Focused synthetic regressions cover helper output, transport diagnostics and actual rate-limit logs. This is not evidence of customer exposure, recipient receipt, release qualification, or protection of arbitrary custom-host credentials. + +### Telegram diagnostic path parsing + +Telegram bot-path masking operates on the parsed, decoded URL path and clears +RawPath after replacement. This covers percent-encoded bot prefixes without +mistaking a hostname or a URL inside a query for a bot path. Method suffixes, +query diagnostics and fragments remain intact; configured destinations and +transport-error causes are unchanged. Host-independent masking is retained for +local API servers, which are supported by the +[Telegram API documentation](https://core.telegram.org/bots/api#making-requests). + +Focused regression tests cover escaped prefixes/tokens, local servers, missing +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. diff --git a/internal/notifications/webhook_url_redaction.go b/internal/notifications/webhook_url_redaction.go index 019ec41f5f..41e8dc609e 100644 --- a/internal/notifications/webhook_url_redaction.go +++ b/internal/notifications/webhook_url_redaction.go @@ -46,15 +46,18 @@ func RedactWebhookURLSecrets(urlString string) 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 { - urlString = urlString[:idx+4] + "REDACTED" + urlString[idx+4+endIdx:] - } else if queryIdx := strings.Index(urlString[idx+4:], "?"); queryIdx != -1 { - urlString = urlString[:idx+4] + "REDACTED" + urlString[idx+4+queryIdx:] - } else { - urlString = urlString[:idx+4] + "REDACTED" + // Telegram also supports local API servers, so retain host-independent + // masking, but inspect only the decoded path. Searching the whole URL + // misses escaped prefixes and can mistake hostnames or query URLs for + // bot credentials. Clear RawPath to prevent escaped secrets resurfacing. + if idx := strings.Index(parsed.Path, "/bot"); idx != -1 { + end := len(parsed.Path) + if suffix := strings.Index(parsed.Path[idx+4:], "/"); suffix != -1 { + end = idx + 4 + suffix } + parsed.Path = parsed.Path[:idx+4] + "REDACTED" + parsed.Path[end:] + parsed.RawPath = "" + urlString = parsed.String() } queryIndex := strings.Index(urlString, "?") diff --git a/internal/notifications/webhook_url_redaction_test.go b/internal/notifications/webhook_url_redaction_test.go index 7f871ac30b..b6ddfe018f 100644 --- a/internal/notifications/webhook_url_redaction_test.go +++ b/internal/notifications/webhook_url_redaction_test.go @@ -53,6 +53,13 @@ func TestRedactWebhookURLSecrets(t *testing.T) { input: "https://gotify.example/message?token=gotify-secret", want: "https://gotify.example/message?token=REDACTED", }, + "telegram escaped prefix": {input: "https://api.telegram.org/%62ot123:telegram-secret/sendMessage", want: "https://api.telegram.org/botREDACTED/sendMessage"}, + "telegram escaped token": {input: "https://api.telegram.org/bot123:telegram%2Dsecret/sendMessage", want: "https://api.telegram.org/botREDACTED/sendMessage"}, + "telegram local escaped prefix": {input: "http://localhost:8081/%62ot123:telegram-secret/sendMessage", want: "http://localhost:8081/botREDACTED/sendMessage"}, + "telegram no method with URL query": {input: "https://api.telegram.org/bot123:telegram-secret?next=https://example.org/status", want: "https://api.telegram.org/botREDACTED?next=https://example.org/status"}, + "telegram fragment": {input: "https://api.telegram.org/bot123:telegram-secret#diagnostic", want: "https://api.telegram.org/botREDACTED#diagnostic"}, + "bot hostname is not a path": {input: "https://bot.example.org/hook?channel=ops", want: "https://bot.example.org/hook?channel=ops"}, + "bot query is not a path": {input: "https://example.org/hook?next=https://bot.example.org/status", want: "https://example.org/hook?next=https://bot.example.org/status"}, "telegram path and query": { input: "https://api.telegram.org/bot123:secret/send?token=query-secret", want: "https://api.telegram.org/botREDACTED/send?token=REDACTED", @@ -211,3 +218,28 @@ func TestDiscordWebhookDiagnosticsRedactPath(t *testing.T) { t.Fatalf("unsafe rate-limit diagnostic: %s", out) } } + +func TestTelegramWebhookDiagnosticsRedactPath(t *testing.T) { + const webhookURL = "https://api.telegram.org/%62ot123:telegram-secret/sendMessage" + cause := errors.New("connection refused") + original := &url.Error{Op: "Post", URL: webhookURL, Err: cause} + redacted := redactWebhookTransportError(original) + if strings.Contains(redacted.Error(), "telegram-secret") || !strings.Contains(redacted.Error(), "/botREDACTED/sendMessage") { + 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, "/botREDACTED/sendMessage") || strings.Contains(out, "telegram-secret") { + t.Fatalf("unsafe rate-limit diagnostic: %s", out) + } +} From e7a5b2c9072dd8bc498c44aa38dc7d294db1edbc Mon Sep 17 00:00:00 2001 From: "pulse-triage[bot]" <249995291+pulse-triage[bot]@users.noreply.github.com> Date: Mon, 7 Sep 2026 19:07:52 +0100 Subject: [PATCH 3/6] fix(notifications): contain encoded query and resolved ntfy diagnostics Literal-only query matching and the separate resolved ntfy transport caller leaked recognised URL credentials. Decode each query name once and project ntfy transport errors before logging or returning them, without changing destinations or error causes. Add synthetic sink-matrix and HTTP projection regressions plus the bounded notification contract in this commit. Change-source: pulse-maintainer (cherry picked from commit 21ed7a89270a5ee1850254d0289c013567871fa8) --- .../README.md | 32 ++++ .../v6/internal/subsystems/notifications.md | 24 +++ .../api/alerting/notifications_health_test.go | 36 ++++ internal/notifications/notifications.go | 1 + .../webhook_confidentiality_contract_test.go | 158 ++++++++++++++++++ .../notifications/webhook_url_redaction.go | 50 +++--- .../webhook_url_redaction_test.go | 2 + 7 files changed, 273 insertions(+), 30 deletions(-) create mode 100644 docs/qualification/release-v6.4-query-ntfy-redaction/README.md create mode 100644 internal/notifications/webhook_confidentiality_contract_test.go diff --git a/docs/qualification/release-v6.4-query-ntfy-redaction/README.md b/docs/qualification/release-v6.4-query-ntfy-redaction/README.md new file mode 100644 index 0000000000..6236659e4c --- /dev/null +++ b/docs/qualification/release-v6.4-query-ntfy-redaction/README.md @@ -0,0 +1,32 @@ +# Encoded-query and resolved-ntfy diagnostic security backport + +Release Train rules 2/4: named security defects reproduced on supplied candidate +71c721ccee8bea8a64a130fdbc0ea78292ff748b, not new product scope. +Source: Core 21ed7a89270a5ee1850254d0289c013567871fa8. +Only that correction is backported; mixed main is not imported. The HTTP test +context was absent here, so retain the complete diagnostic-context test from +that source when resolving the conflict. Production HTTP code is unchanged. + +Before repair, TestDeliveryEncodedQueryConfidentiality and +TestDeliveryResolvedNtfyConfidentiality fail using synthetic credentials and +in-memory transports. After repair, the focused notification/redaction tests +and TestGetDeliveryLog HTTP tests pass. Logs are retained in +/var/lib/pulse-maintainer/queue/staging/20260907T181017Z-release-line/ +(before.log, after.log, api.log; race.log records the separate repeated matrix). + +The repair decodes supported query names once and masks repeated occurrences; +resolved ntfy projects transport errors before logging and returning. The +matrix checks unchanged destination, payload, event identity, safe userinfo +rejection and error-cause unwrapping. This is bounded diagnostic protection, +not arbitrary-secret detection, evidence of customer exposure, recipient +receipt, full-suite qualification or release approval. + +Fresh external acceptance context: OWASP Logging Cheat Sheet, Data to exclude +and Verification, inspected 2026-09-07: +https://cheatsheetseries.owasp.org/cheatsheets/Logging_Cheat_Sheet.html +Credentials should not be logged; diagnostic failure paths need verification. + +Delivery still owns protected-PR integration and exact-candidate qualification. +The retained 940f788d latency failure is not cleared by these focused tests; +Benchmarks remains advisory for source landing. Stable needs exact RC soak +and founder packet approval. No excluded crash investigation was performed. diff --git a/docs/release-control/v6/internal/subsystems/notifications.md b/docs/release-control/v6/internal/subsystems/notifications.md index 670f8af00e..6b14c76474 100644 --- a/docs/release-control/v6/internal/subsystems/notifications.md +++ b/docs/release-control/v6/internal/subsystems/notifications.md @@ -637,3 +637,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 5321f1d14d..1c654a7b83 100644 --- a/internal/api/alerting/notifications_health_test.go +++ b/internal/api/alerting/notifications_health_test.go @@ -329,3 +329,39 @@ func containsAny(value string, needles ...string) bool { } return false } + +// These are response-boundary checks, not just URL-helper checks. +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]"}, + } { + t.Run(tc.name, func(t *testing.T) { + manager := new(MockNotificationManager) + monitor := new(MockNotificationMonitor) + monitor.On("GetNotificationManager").Return(manager).Once() + manager.On("GetDeliveryLog", mock.Anything, 0).Return([]notifications.DeliveryLogEntry{ + {NotificationID: "attempt-1", ErrorMessage: tc.input, FailureClass: "transport"}, + }, nil).Once() + rec := httptest.NewRecorder() + NewNotificationHandlers(nil, monitor).GetDeliveryLog(rec, httptest.NewRequest(http.MethodGet, "/api/notifications/delivery-log", nil)) + if rec.Code != http.StatusOK { + t.Fatalf("status = %d", rec.Code) + } + var response struct { + Entries []notifications.DeliveryLogEntry `json:"entries"` + } + if err := json.Unmarshal(rec.Body.Bytes(), &response); err != nil { + t.Fatal(err) + } + if len(response.Entries) != 1 || response.Entries[0].ErrorMessage != tc.want || + response.Entries[0].NotificationID != "attempt-1" || response.Entries[0].FailureClass != "transport" { + t.Fatalf("unexpected delivery projection: %#v", response) + } + manager.AssertExpectations(t) + monitor.AssertExpectations(t) + }) + } +} diff --git a/internal/notifications/notifications.go b/internal/notifications/notifications.go index 72e32c8489..19ead2dc24 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 0000000000..3daf14ce99 --- /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 41e8dc609e..9a8fa77b2f 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 - } - 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++ + // 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 } - 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 b6ddfe018f..6f6babd5c2 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"}, From bad8c4fc6bdc10e3399cc14534fc2981547a8a51 Mon Sep 17 00:00:00 2001 From: rcourtman <8825017+rcourtman@users.noreply.github.com> Date: Mon, 7 Sep 2026 19:30:27 +0100 Subject: [PATCH 4/6] Publish reviewed release snapshots independently of branch tips Continuous development must not change the source of an admitted release. Allow the workflow to run at the qualified preparation PR head after its normal merge, verifying exact source and workflow identity, canonical PR provenance, and ancestry in the governed release line. Later branch commits remain outside that release. Document the immutable-candidate contract and verify source workflow compatibility before qualification. Validation: snapshot identity and workflow contract tests passed, including wrong-head, wrong-base, fork, unmerged and unbound dispatch rejection. The existing release workflow promotion policy test also passed. (cherry picked from commit b64709e7b7ad174e9c94ad2a0d3d841678690935) Change-source: pulse-maintainer --- .github/workflows/create-release.yml | 23 +++- .gitignore | 2 + .../v6/internal/RELEASE_PROMOTION_POLICY.md | 15 ++- .../subsystems/deployment-installability.md | 19 ++++ .../release_promotion_policy_test.py | 8 ++ scripts/release_control/release_snapshot.py | 100 ++++++++++++++++++ .../release_control/release_snapshot_test.py | 85 +++++++++++++++ 7 files changed, 249 insertions(+), 3 deletions(-) create mode 100644 scripts/release_control/release_snapshot.py create mode 100644 scripts/release_control/release_snapshot_test.py diff --git a/.github/workflows/create-release.yml b/.github/workflows/create-release.yml index 4bead5286a..dd6db29434 100644 --- a/.github/workflows/create-release.yml +++ b/.github/workflows/create-release.yml @@ -13,6 +13,14 @@ on: description: 'Exact 40-character commit SHA admitted for this release' required: true type: string + release_source_branch: + description: 'Governed source branch for a merged immutable release snapshot' + required: false + type: string + release_pull_request: + description: 'Merged pull request that reviewed the immutable snapshot' + required: false + type: string release_notes: description: 'Release notes (markdown)' required: true @@ -85,6 +93,10 @@ permissions: jobs: # Combined version extraction and validation (saves a checkout) prepare: + permissions: + actions: read + contents: read + pull-requests: read # Stable releases use hosted runners regardless of their Windows-signing # decision. Prereleases retain the credential-free PVE acceleration path. runs-on: ${{ !contains(inputs.version, '-') && 'ubuntu-24.04' || fromJSON('["self-hosted","Linux","X64","pulse-pve-compile"]') }} @@ -134,11 +146,20 @@ jobs: persist-credentials: false fetch-depth: 0 + - name: Verify reviewed release snapshot + id: snapshot + env: + GH_TOKEN: ${{ github.token }} + RELEASE_SOURCE_BRANCH: ${{ inputs.release_source_branch }} + RELEASE_PULL_REQUEST: ${{ inputs.release_pull_request }} + run: python3 scripts/release_control/release_snapshot.py + - name: Extract version id: extract env: VERSION_INPUT: ${{ inputs.version }} HISTORICAL_ASSET_BACKFILL_INPUT: ${{ inputs.historical_asset_backfill_only }} + SNAPSHOT_SOURCE_BRANCH: ${{ steps.snapshot.outputs.source_branch }} run: | set -euo pipefail if [[ ! "${VERSION_INPUT}" =~ ^[0-9]+\.[0-9]+\.[0-9]+(-((rc|alpha|beta)\.[0-9]+))?$ ]]; then @@ -164,7 +185,7 @@ jobs: exit 1 fi - SOURCE_BRANCH="${GITHUB_REF_NAME}" + SOURCE_BRANCH="${SNAPSHOT_SOURCE_BRANCH}" HISTORICAL_ASSET_BACKFILL_ONLY="${HISTORICAL_ASSET_BACKFILL_INPUT}" python3 scripts/write_github_output.py tag "${TAG}" python3 scripts/write_github_output.py version "${VERSION}" diff --git a/.gitignore b/.gitignore index 99d5297ef3..a0852d8c19 100644 --- a/.gitignore +++ b/.gitignore @@ -230,6 +230,8 @@ scripts/release_control/* !scripts/release_control/contract_audit_test.py !scripts/release_control/customer_promotion_lease.sh !scripts/release_control/control_plane.py +!scripts/release_control/release_snapshot.py +!scripts/release_control/release_snapshot_test.py !scripts/release_control/generate_platform_support_frontend_module.py !scripts/release_control/control_plane_audit.py !scripts/release_control/control_plane_audit_test.py diff --git a/docs/release-control/v6/internal/RELEASE_PROMOTION_POLICY.md b/docs/release-control/v6/internal/RELEASE_PROMOTION_POLICY.md index e1aad0f8ae..b78ae25f8c 100644 --- a/docs/release-control/v6/internal/RELEASE_PROMOTION_POLICY.md +++ b/docs/release-control/v6/internal/RELEASE_PROMOTION_POLICY.md @@ -254,8 +254,19 @@ without the other lanes changing the candidate underneath it. every third train and records the decision here. 2. Each train has its own branch, `release/v6.N`, created from `main` at cut time and declared in `docs/release-control/control_plane.json` so the - release workflow refuses a dispatch from any other branch. `main` is never - frozen. A fix for something found in the candidate is backported to the + release workflow verifies that governed source line. `main` is never + frozen. A selected release is an immutable commit, not the current tip of + the train. Its preparation PR uses a fixed `release-candidate/` + ref and passes the normal protected review path into the governed line. + Qualification, builds and publication use that PR's exact head commit, + even when the merge or later train commits contain newer work. Snapshot + dispatch must verify the merged PR's head, canonical repository, source + line and continued ancestry in published history. The workflow and source + SHA must both equal the admitted snapshot. Later commits belong to another + release unless the maintainer explicitly rejects the selected candidate + for a concrete defect in that candidate. Merely finding newer work does + not invalidate qualification or restart a release. + A fix for something found in the candidate is backported to the release branch through a pull request; each backport produces the next `rc.N` and restarts the soak. After general availability the branch is the patch line for that train. diff --git a/docs/release-control/v6/internal/subsystems/deployment-installability.md b/docs/release-control/v6/internal/subsystems/deployment-installability.md index b6e71af380..f6b775bc37 100644 --- a/docs/release-control/v6/internal/subsystems/deployment-installability.md +++ b/docs/release-control/v6/internal/subsystems/deployment-installability.md @@ -15,6 +15,25 @@ ## Purpose +### Immutable release source + +Continuous development must not change an admitted release's source. The +preparation PR's qualified head stays fixed on `release-candidate/` +while its governed source branch continues receiving work. Dispatch verifies +that the canonical PR merged into the version's governed source line, that its +head and ref match the admitted snapshot, and that its merge remains in that +line's published history. Qualification, workflow execution, compiler dispatch +and published artifacts bind to that head, not a later merge or branch tip. +The source workflow must implement the snapshot input and provenance contract +before the maintainer spends an exact qualification run on it. Later changes +belong to the next candidate unless the maintainer explicitly rejects the +selected source for a concrete defect. Existing maturity, soak, failed-check +and publication-authority boundaries still apply. +`scripts/release_control/release_snapshot.py` owns snapshot identity validation. +Its executable identity cases are in `release_snapshot_test.py`, and the staged +workflow contract is verified in `release_promotion_policy_test.py`. + + ### Benchmark qualification evidence The Build and Test benchmark job retains `bench-metadata.txt` together with diff --git a/scripts/release_control/release_promotion_policy_test.py b/scripts/release_control/release_promotion_policy_test.py index 216cc45982..a2044537ed 100644 --- a/scripts/release_control/release_promotion_policy_test.py +++ b/scripts/release_control/release_promotion_policy_test.py @@ -310,6 +310,14 @@ def setUp(self) -> None: ): self.skipTest("staged governance inputs missing; see test_staged_governance_inputs_are_present") + def test_release_workflow_supports_reviewed_immutable_snapshots(self) -> None: + from release_snapshot import check_workflow + + with tempfile.TemporaryDirectory() as directory: + workflow = Path(directory) / "create-release.yml" + workflow.write_text(read(".github/workflows/create-release.yml")) + check_workflow(workflow) + def test_staged_governance_inputs_are_present(self) -> None: if STAGED_GOVERNANCE_INPUT_ERRORS: self.fail( diff --git a/scripts/release_control/release_snapshot.py b/scripts/release_control/release_snapshot.py new file mode 100644 index 0000000000..5d1f359fc2 --- /dev/null +++ b/scripts/release_control/release_snapshot.py @@ -0,0 +1,100 @@ +#!/usr/bin/env python3 +"""Bind release execution to a reviewed snapshot, independently of a moving train.""" +from __future__ import annotations + +import argparse +import json +import os +from pathlib import Path +import re +import subprocess + + +SHA = re.compile(r"[0-9a-f]{40}") +SNAPSHOT_REF = re.compile(r"release-candidate/[0-9A-Za-z._-]+") +SOURCE_BRANCH = re.compile(r"main|release/v[0-9]+\.[0-9]+") + + +def source_branch(ref: str, requested: str, pull_request: str) -> str: + if not ref.startswith("refs/heads/"): + raise ValueError("release dispatch must name a branch ref") + branch = ref.removeprefix("refs/heads/") + if SNAPSHOT_REF.fullmatch(branch): + if not SOURCE_BRANCH.fullmatch(requested) or not re.fullmatch(r"[1-9][0-9]*", pull_request): + raise ValueError("snapshot dispatch requires its governed source branch and merged pull request") + return requested + if requested or pull_request: + raise ValueError("snapshot provenance inputs require a reserved release-candidate ref") + if not SOURCE_BRANCH.fullmatch(branch): + raise ValueError("release dispatch is outside a governed source branch") + return branch + + +def reviewed_merge(pr: dict, *, repository: str, ref: str, branch: str, sha: str) -> str: + if not SHA.fullmatch(sha): + raise ValueError("source must be an exact commit") + head, base = pr.get("head", {}), pr.get("base", {}) + if pr.get("merged") is not True or pr.get("state") != "closed": + raise ValueError("release snapshot pull request is not merged") + if head.get("sha") != sha or head.get("ref") != ref.removeprefix("refs/heads/"): + raise ValueError("pull request does not identify the dispatched snapshot") + if base.get("ref") != branch: + raise ValueError("pull request belongs to another release line") + if any(part.get("repo", {}).get("full_name") != repository for part in (head, base)): + raise ValueError("release snapshot must come from the canonical repository") + merge = pr.get("merge_commit_sha", "") + if not isinstance(merge, str) or not SHA.fullmatch(merge): + raise ValueError("pull request has no exact merge commit") + return merge + + +def check_workflow(path: Path) -> None: + # BaseLoader preserves the YAML `on` key rather than treating it as a bool. + import yaml + + workflow = yaml.load(path.read_text(), Loader=yaml.BaseLoader) + inputs = workflow["on"]["workflow_dispatch"]["inputs"] + for name in ("expected_source_sha", "release_source_branch", "release_pull_request"): + if inputs.get(name, {}).get("type") != "string": + raise ValueError(f"release workflow lacks snapshot input {name}") + steps = workflow["jobs"]["prepare"]["steps"] + guard = next((step for step in steps if step.get("id") == "snapshot"), {}) + if "python3 scripts/release_control/release_snapshot.py" not in guard.get("run", ""): + raise ValueError("release workflow does not execute the snapshot provenance guard") + expected = {"RELEASE_SOURCE_BRANCH": "${{ inputs.release_source_branch }}", "RELEASE_PULL_REQUEST": "${{ inputs.release_pull_request }}"} + if any(guard.get("env", {}).get(key) != value for key, value in expected.items()): + raise ValueError("release workflow does not bind snapshot provenance inputs") + + +def main() -> None: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--check-workflow", type=Path) + args = parser.parse_args() + if args.check_workflow: + check_workflow(args.check_workflow) + return + + ref = os.environ["GITHUB_REF"] + requested = os.environ.get("RELEASE_SOURCE_BRANCH", "") + pr_number = os.environ.get("RELEASE_PULL_REQUEST", "") + branch = source_branch(ref, requested, pr_number) + if requested: + repository = os.environ["GITHUB_REPOSITORY"] + if repository != "rcourtman/Pulse": + raise ValueError("snapshot releases are restricted to the canonical Pulse repository") + sha = os.environ["GITHUB_SHA"] + if os.environ["GITHUB_WORKFLOW_SHA"] != sha: + raise ValueError("workflow and source must be the same immutable snapshot") + pr = json.loads(subprocess.check_output(["gh", "api", f"repos/{repository}/pulls/{pr_number}"], text=True)) + merge = reviewed_merge(pr, repository=repository, ref=ref, branch=branch, sha=sha) + subprocess.run(["git", "fetch", "--quiet", "--no-tags", f"https://github.com/{repository}.git", f"refs/heads/{branch}"], check=True) + # Later merges are allowed. A rewrite that removes the reviewed merge is not. + subprocess.run(["git", "merge-base", "--is-ancestor", sha, merge], check=True) + subprocess.run(["git", "merge-base", "--is-ancestor", merge, "FETCH_HEAD"], check=True) + with Path(os.environ["GITHUB_OUTPUT"]).open("a") as output: + output.write(f"source_branch={branch}\n") + print(f"Release source is bound to {os.environ['GITHUB_SHA']} from {branch}") + + +if __name__ == "__main__": + main() diff --git a/scripts/release_control/release_snapshot_test.py b/scripts/release_control/release_snapshot_test.py new file mode 100644 index 0000000000..cf50930f01 --- /dev/null +++ b/scripts/release_control/release_snapshot_test.py @@ -0,0 +1,85 @@ +"""Release provenance stays bound to the reviewed commit as its train advances.""" +from copy import deepcopy +import json +import os +from pathlib import Path +import subprocess +import tempfile +import unittest +from unittest.mock import patch + +import release_snapshot as snapshot + + +class SnapshotIdentityTest(unittest.TestCase): + def setUp(self): + self.sha = 'a' * 40 + self.merge = 'b' * 40 + self.ref = 'refs/heads/release-candidate/packet-1' + self.pr = { + 'state': 'closed', 'merged': True, 'merge_commit_sha': self.merge, + 'head': {'sha': self.sha, 'ref': 'release-candidate/packet-1', 'repo': {'full_name': 'rcourtman/Pulse'}}, + 'base': {'ref': 'release/v6.4', 'repo': {'full_name': 'rcourtman/Pulse'}}, + } + + def verify(self, pr): + return snapshot.reviewed_merge(pr, repository='rcourtman/Pulse', ref=self.ref, + branch='release/v6.4', sha=self.sha) + + def test_merged_snapshot_identity(self): + self.assertEqual(self.merge, self.verify(self.pr)) + self.assertEqual('release/v6.4', snapshot.source_branch(self.ref, 'release/v6.4', '42')) + self.assertEqual('main', snapshot.source_branch('refs/heads/main', '', '')) + + def test_unreviewed_or_retargeted_identity_is_rejected(self): + changes = [('merged', False), ('state', 'open'), ('merge_commit_sha', ''), + ('head.sha', 'c' * 40), ('head.ref', 'release-candidate/other'), + ('base.ref', 'main'), ('head.repo.full_name', 'someone/Pulse'), + ('base.repo.full_name', 'someone/Pulse')] + for path, value in changes: + with self.subTest(path=path): + pr = deepcopy(self.pr) + target = pr + fields = path.split('.') + for field in fields[:-1]: + target = target[field] + target[fields[-1]] = value + with self.assertRaises(ValueError): + self.verify(pr) + + def test_unbound_dispatch_inputs_are_rejected(self): + for args in [(self.ref, '', '42'), (self.ref, 'main', ''), + (self.ref, 'feature/other', '42'), (self.ref, 'main', '../42'), + ('refs/heads/main', 'main', '42'), ('refs/tags/v6.4.4-beta.1', '', '')]: + with self.subTest(args=args), self.assertRaises(ValueError): + snapshot.source_branch(*args) + + def test_source_workflow_supports_snapshot_inputs_before_qualification(self): + path = Path(__file__).resolve().parents[2] / '.github/workflows/create-release.yml' + snapshot.check_workflow(path) + with tempfile.TemporaryDirectory() as raw: + old = Path(raw) / 'workflow.yml' + old.write_text(path.read_text().replace('release_pull_request:', 'removed_input:', 1)) + with self.assertRaises(ValueError): + snapshot.check_workflow(old) + + def test_workflow_verifies_ancestry_without_requiring_current_tip(self): + with tempfile.TemporaryDirectory() as raw: + output = Path(raw) / 'output' + env = {'GITHUB_REF': self.ref, 'RELEASE_SOURCE_BRANCH': 'release/v6.4', + 'RELEASE_PULL_REQUEST': '42', 'GITHUB_REPOSITORY': 'rcourtman/Pulse', + 'GITHUB_SHA': self.sha, 'GITHUB_WORKFLOW_SHA': self.sha, + 'GITHUB_OUTPUT': str(output)} + with patch.dict(os.environ, env), patch('sys.argv', ['release_snapshot.py']), \ + patch.object(subprocess, 'check_output', return_value=json.dumps(self.pr)), \ + patch.object(subprocess, 'run') as run: + snapshot.main() + self.assertEqual('source_branch=release/v6.4\n', output.read_text()) + commands = [call.args[0] for call in run.call_args_list] + self.assertIn(['git', 'merge-base', '--is-ancestor', self.sha, self.merge], commands) + self.assertIn(['git', 'merge-base', '--is-ancestor', self.merge, 'FETCH_HEAD'], commands) + self.assertFalse(any('reset' in command or 'checkout' in command for command in commands)) + + +if __name__ == '__main__': + unittest.main() From 49a005005409389294a0c93721a9d7379cbd5421 Mon Sep 17 00:00:00 2001 From: rcourtman <8825017+rcourtman@users.noreply.github.com> Date: Mon, 7 Sep 2026 19:38:48 +0100 Subject: [PATCH 5/6] Encode the release source branch through the shared output helper The branch is validated by the snapshot guard, but its transfer between workflow steps must also use the canonical GitHub command-file encoder. Keep the source binding unchanged and satisfy the workflow trust audit. Validation: all 41 workflow trust tests and five snapshot tests pass. Contract-Neutral: Encode the already-validated release branch with the shared GitHub command-file helper without changing source identity or release authority (cherry picked from commit ab562c82aa73937c55e8432b8c0a6f8accfef59d) Change-source: pulse-maintainer --- .github/workflows/create-release.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/create-release.yml b/.github/workflows/create-release.yml index dd6db29434..6e30774447 100644 --- a/.github/workflows/create-release.yml +++ b/.github/workflows/create-release.yml @@ -190,7 +190,7 @@ jobs: python3 scripts/write_github_output.py tag "${TAG}" python3 scripts/write_github_output.py version "${VERSION}" echo "is_prerelease=${IS_PRERELEASE}" >> $GITHUB_OUTPUT - echo "source_branch=${SOURCE_BRANCH}" >> $GITHUB_OUTPUT + python3 scripts/write_github_output.py source_branch "${SOURCE_BRANCH}" python3 scripts/write_github_output.py historical_asset_backfill_only "${HISTORICAL_ASSET_BACKFILL_ONLY}" echo "Version: ${VERSION}, Tag: ${TAG}, Prerelease: ${IS_PRERELEASE}, Branch: ${SOURCE_BRANCH}, HistoricalBackfillOnly: ${HISTORICAL_ASSET_BACKFILL_ONLY}" From d2f7bd0f2522f3466e4003ab12a75612dd279937 Mon Sep 17 00:00:00 2001 From: "pulse-triage[bot]" <249995291+pulse-triage[bot]@users.noreply.github.com> Date: Mon, 7 Sep 2026 22:20:18 +0100 Subject: [PATCH 6/6] fix(discovery): preserve current records during suggestion backfill Backfill could save a stale List snapshot after manual discovery repaired a service, restoring unknown identity and dropping its URL and engine version. Derive and persist missing suggestions from the current record under the store lock instead, without holding it across monitor reads. Add a deterministic SetReadState/manual-refresh interleaving and encrypted restart assertions, plus coverage for current identity, dismissed proposals, deletion and persistence failure. The discovery package passes twenty race-enabled repetitions. Change-source: pulse-maintainer --- .../v6/internal/subsystems/monitoring.md | 17 ++++ internal/servicediscovery/service.go | 13 +-- internal/servicediscovery/service_test.go | 93 +++++++++++++++++++ internal/servicediscovery/store.go | 32 +++++++ internal/servicediscovery/store_test.go | 92 ++++++++++++++++++ 5 files changed, 239 insertions(+), 8 deletions(-) diff --git a/docs/release-control/v6/internal/subsystems/monitoring.md b/docs/release-control/v6/internal/subsystems/monitoring.md index 3a701921ee..1f36d8f29f 100644 --- a/docs/release-control/v6/internal/subsystems/monitoring.md +++ b/docs/release-control/v6/internal/subsystems/monitoring.md @@ -17,6 +17,23 @@ ## Purpose +**Availability backfill preserves concurrent discovery changes (7 September 2026)** + +The backfill List snapshot is a work list, not an authoritative record to save. +Read the current discovery, derive a missing suggestion from its current identity, +and persist under one store write lock. State-provider reads stay outside that +lock. Preserve newer identity, URL, engine version, user notes and existing +(including dismissed) proposals; never resurrect a discovery deleted after List. +A failed persistence attempt remains an error without installing the proposed +change in cache. `TestService_BackfillPreservesConcurrentManualRepair` in `service_test.go` +deterministically pauses SetReadState +backfill after List, completes manual ESPHome repair, then resumes backfill and +checks both cache and encrypted restart. +`TestStore_BackfillAvailabilitySuggestionUsesCurrentRecord` in `store_test.go` +covers current-identity +inference, dismissal, deletion, unsupported identity and persistence failure. +This is discovery state-integrity proof, not incident or notification acceptance. + ### Host-local network evidence exclusion Automatic PVE association must not treat loopback, unspecified, multicast or diff --git a/internal/servicediscovery/service.go b/internal/servicediscovery/service.go index 6761bcc675..fd3b7c0efc 100644 --- a/internal/servicediscovery/service.go +++ b/internal/servicediscovery/service.go @@ -3514,14 +3514,11 @@ func (s *Service) backfillAvailabilitySuggestions(ctx context.Context) { } externalIP := s.getResourceExternalIP(req) - suggestion := SuggestAvailabilityProbe(d, externalIP) - if suggestion != nil { - d.SuggestedAvailabilityProbe = suggestion - if err := s.store.Save(d); err != nil { - log.Warn().Err(err).Str("id", d.ID).Msg("Failed to save backfilled availability suggestion") - } else { - updated++ - } + changed, err := s.store.backfillAvailabilitySuggestion(d.ID, externalIP) + if err != nil { + log.Warn().Err(err).Str("id", d.ID).Msg("Failed to save backfilled availability suggestion") + } else if changed { + updated++ } } diff --git a/internal/servicediscovery/service_test.go b/internal/servicediscovery/service_test.go index 90a12b1f6c..ed9074e507 100644 --- a/internal/servicediscovery/service_test.go +++ b/internal/servicediscovery/service_test.go @@ -8,6 +8,7 @@ import ( "path/filepath" "strings" "sync" + "sync/atomic" "testing" "time" @@ -2667,3 +2668,95 @@ func TestService_ListDiscoveriesByTarget_DoesNotBridgeSharedHostnames(t *testing t.Fatalf("expected estate B to keep finding its own record, got %d", len(own)) } } + +// Pause the first snapshot read, which backfill performs after Store.List. +// Other reads remain available to the concurrent manual discovery. +type backfillBarrierState struct { + unifiedresources.ReadState + first atomic.Bool + listed chan struct{} + resume chan struct{} +} + +func (s *backfillBarrierState) VMs() []*unifiedresources.VMView { + if s.first.CompareAndSwap(false, true) { + close(s.listed) + <-s.resume + } + return s.ReadState.VMs() +} + +func TestService_BackfillPreservesConcurrentManualRepair(t *testing.T) { + store, err := NewStore(t.TempDir()) + if err != nil { + t.Fatal(err) + } + service := NewService(store, nil, DefaultConfig()) + rs := readStateFromSnapshot(StateSnapshot{Containers: []Container{ + {VMID: 102, Name: "esphome", Node: "pve1", Status: "running"}, + }}) + // Set up the fixture before starting the asynchronous backfill. + service.readState = rs + service.SetCommandScanningEnabled(true) + service.SetAIAnalyzer(&stubAnalyzer{response: `{}`}) + service.collectFingerprints(context.Background()) + id := MakeResourceID(ResourceTypeSystemContainer, "pve1", "102") + fp, err := store.GetFingerprint(id) + if err != nil || fp == nil { + t.Fatalf("fingerprint: %v, %v", fp, err) + } + if err := store.Save(&ResourceDiscovery{ + ID: id, ResourceType: ResourceTypeSystemContainer, TargetID: "pve1", ResourceID: "102", + Hostname: "esphome", ServiceType: "unknown", ServiceName: "Unknown Service", Category: CategoryUnknown, + Fingerprint: fp.Hash, FingerprintedAt: fp.GeneratedAt, FingerprintSchemaVersion: fp.SchemaVersion, + CLIAccessVersion: CLIAccessVersion, + }); err != nil { + t.Fatal(err) + } + barrier := &backfillBarrierState{ReadState: rs, listed: make(chan struct{}), resume: make(chan struct{})} + var release sync.Once + unblock := func() { release.Do(func() { close(barrier.resume) }) } + service.SetReadState(barrier) + t.Cleanup(func() { unblock(); service.backfillCancel(); <-service.backfillDone }) + select { + case <-barrier.listed: + case <-time.After(5 * time.Second): + t.Fatal("backfill did not reach snapshot barrier") + } + summary, err := service.RunManualDiscoveryRefresh(context.Background()) + if err != nil { + t.Fatal(err) + } + if summary.DiscoveredCount != 1 || summary.FailedCount != 0 { + t.Fatalf("refresh: %+v", summary) + } + repaired, err := store.Get(id) + if err != nil { + t.Fatal(err) + } + if repaired.ServiceType != "esphome" { + t.Fatalf("manual repair failed: %q", repaired.ServiceType) + } + unblock() + select { + case <-service.backfillDone: + case <-time.After(5 * time.Second): + t.Fatal("backfill did not finish") + } + // Read through a new encrypted store as well as the live cache. + restarted, err := NewStore(filepath.Dir(store.dataDir)) + if err != nil { + t.Fatal(err) + } + for name, reader := range map[string]*Store{"cache": store, "disk": restarted} { + got, err := reader.Get(id) + if err != nil || got == nil { + t.Fatalf("%s read: %v", name, err) + } + if got.ServiceType != repaired.ServiceType || got.ServiceName != repaired.ServiceName || + got.SuggestedURL != repaired.SuggestedURL || got.DiscoveryEngineVersion != repaired.DiscoveryEngineVersion { + t.Errorf("%s: backfill overwrote manual repair: type=%q name=%q URL=%q engine=%d", name, + got.ServiceType, got.ServiceName, got.SuggestedURL, got.DiscoveryEngineVersion) + } + } +} diff --git a/internal/servicediscovery/store.go b/internal/servicediscovery/store.go index ebf1b9b0d9..29c532f3d6 100644 --- a/internal/servicediscovery/store.go +++ b/internal/servicediscovery/store.go @@ -381,7 +381,11 @@ func (s *Store) marshalDiscoveryForStorage(discovery *ResourceDiscovery) ([]byte func (s *Store) Save(d *ResourceDiscovery) error { s.mu.Lock() defer s.mu.Unlock() + return s.saveLocked(d) +} +// saveLocked requires s.mu to be held for writing. +func (s *Store) saveLocked(d *ResourceDiscovery) error { if d.ID == "" { return fmt.Errorf("discovery ID is required") } @@ -433,7 +437,11 @@ func (s *Store) Get(id string) (*ResourceDiscovery, error) { s.mu.Lock() defer s.mu.Unlock() + return s.getLocked(id) +} +// getLocked requires s.mu to be held for writing (loads may migrate files). +func (s *Store) getLocked(id string) (*ResourceDiscovery, error) { filePath := s.getFilePath(id) activePath := filePath data, migratedPlaintext, err := s.loadDiscoveryFileData(filePath, maxDiscoveryFileReadBytes) @@ -481,6 +489,30 @@ func (s *Store) Get(id string) (*ResourceDiscovery, error) { return cloneResourceDiscovery(&discovery), nil } +// backfillAvailabilitySuggestion reads, derives and persists under one lock. +// A List snapshot is only a work list: never write its stale identity or resurrect +// a deleted discovery. State/monitor reads must happen before taking this lock. +func (s *Store) backfillAvailabilitySuggestion(id, externalIP string) (bool, error) { + s.mu.Lock() + defer s.mu.Unlock() + current, err := s.getLocked(id) + if err != nil || current == nil { + return false, err + } + if current.SuggestedAvailabilityProbe != nil { + return false, nil + } + suggestion := SuggestAvailabilityProbe(current, externalIP) + if suggestion == nil { + return false, nil + } + current.SuggestedAvailabilityProbe = suggestion + if err := s.saveLocked(current); err != nil { + return false, err + } + return true, nil +} + // GetByResource retrieves a discovery by resource type and ID. func (s *Store) GetByResource(resourceType ResourceType, targetID, resourceID string) (*ResourceDiscovery, error) { id := MakeResourceID(resourceType, targetID, resourceID) diff --git a/internal/servicediscovery/store_test.go b/internal/servicediscovery/store_test.go index 67fc64bdcf..043ee43d33 100644 --- a/internal/servicediscovery/store_test.go +++ b/internal/servicediscovery/store_test.go @@ -1234,3 +1234,95 @@ func TestStore_GetStaleResources(t *testing.T) { t.Fatalf("expected GetStaleResources to return list error") } } + +func TestStore_BackfillAvailabilitySuggestionUsesCurrentRecord(t *testing.T) { + for _, scenario := range []string{"new identity", "existing dismissed proposal", "deleted", "unsupported", "write failure"} { + t.Run(scenario, func(t *testing.T) { + dir := t.TempDir() + store, err := NewStore(dir) + if err != nil { + t.Fatal(err) + } + id := MakeResourceID(ResourceTypeSystemContainer, "pve1", "102") + old := &ResourceDiscovery{ID: id, Hostname: "esphome", ServiceType: "unknown"} + if err := store.Save(old); err != nil { + t.Fatal(err) + } + work, err := store.List() + if err != nil || len(work) != 1 { + t.Fatalf("List: %v, %v", work, err) + } + current := cloneResourceDiscovery(old) + current.ServiceType = "redis" + current.ServiceName = "New Redis" + current.UserNotes = "operator note after list" + if scenario == "existing dismissed proposal" { + current.SuggestedAvailabilityProbe = SuggestAvailabilityProbe(current, "192.0.2.20") + current.DismissedAvailabilityProbeFingerprint = current.SuggestedAvailabilityProbe.EvidenceFingerprint + } + if scenario == "unsupported" { + current.ServiceType = "unknown" + current.Hostname = "unidentified" + } + if err := store.Save(current); err != nil { + t.Fatal(err) + } + before, err := store.Get(id) + if err != nil { + t.Fatal(err) + } + if scenario == "deleted" { + if err := store.Delete(id); err != nil { + t.Fatal(err) + } + } + if scenario == "write failure" { + // A directory at the temporary file path causes a real persistence error. + if err := os.Mkdir(store.getFilePath(id)+".tmp", 0700); err != nil { + t.Fatal(err) + } + } + changed, err := store.backfillAvailabilitySuggestion(work[0].ID, "192.0.2.10") + if scenario == "write failure" { + if err == nil || changed { + t.Fatalf("expected failed write, got changed=%v err=%v", changed, err) + } + } else if err != nil { + t.Fatal(err) + } + if changed != (scenario == "new identity") { + t.Fatalf("unexpected changed=%v", changed) + } + restarted, err := NewStore(dir) + if err != nil { + t.Fatal(err) + } + for name, reader := range map[string]*Store{"cache": store, "disk": restarted} { + got, err := reader.Get(id) + if err != nil { + t.Fatal(err) + } + if scenario == "deleted" { + if got != nil { + t.Errorf("%s: deleted discovery resurrected", name) + } + continue + } + want := cloneResourceDiscovery(before) + if scenario == "new identity" { + want.SuggestedAvailabilityProbe = SuggestAvailabilityProbe(before, "192.0.2.10") + want.UpdatedAt = got.UpdatedAt + if got.SuggestedAvailabilityProbe == nil || got.SuggestedAvailabilityProbe.Port != 6379 { + t.Fatalf("%s: suggestion not derived from current Redis identity", name) + } + } + // JSON comparison ignores time.Time's process-local monotonic clock. + gotJSON, _ := json.Marshal(got) + wantJSON, _ := json.Marshal(want) + if string(gotJSON) != string(wantJSON) { + t.Errorf("%s: backfill changed unrelated/current fields", name) + } + } + }) + } +}