diff --git a/.github/workflows/create-release.yml b/.github/workflows/create-release.yml index 4bead5286a..6e30774447 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,12 +185,12 @@ 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}" 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}" 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/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/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/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/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/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/docs/release-control/v6/internal/subsystems/notifications.md b/docs/release-control/v6/internal/subsystems/notifications.md index 3f0d77679f..6b14c76474 100644 --- a/docs/release-control/v6/internal/subsystems/notifications.md +++ b/docs/release-control/v6/internal/subsystems/notifications.md @@ -607,3 +607,57 @@ 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. + +### 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. + +### 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 cb795f58de..9a8fa77b2f 100644 --- a/internal/notifications/webhook_url_redaction.go +++ b/internal/notifications/webhook_url_redaction.go @@ -35,52 +35,54 @@ func RedactWebhookURLSecrets(urlString string) string { } 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 { - 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" + 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() } } - queryIndex := strings.Index(urlString, "?") - if queryIndex == -1 { - return urlString + // 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() } - 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 6208f8e317..6f6babd5c2 100644 --- a/internal/notifications/webhook_url_redaction_test.go +++ b/internal/notifications/webhook_url_redaction_test.go @@ -16,6 +16,15 @@ 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"}, + "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"}, @@ -46,6 +55,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", @@ -180,3 +196,52 @@ 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) + } +} + +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) + } +} 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) + } + } + }) + } +} 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()