From c2d59d2747598e22e8669c52433668ca5f44c1c3 Mon Sep 17 00:00:00 2001 From: "pulse-triage[bot]" <249995291+pulse-triage[bot]@users.noreply.github.com> Date: Tue, 6 Oct 2026 10:40:18 +0100 Subject: [PATCH 1/2] Honour origin trust clears and unify webhook placeholder resolution Treat explicit empty origins as a durable same-origin policy, preserve omitted fields and deployment overrides, and expose the effective value. Resolve webhook masks consistently across Create, Update and Test; reject unresolved or ambiguous identities before mutation or delivery while retaining masked responses, per-key edits, clears and encrypted persistence. Include real middleware/reload and HTTP Test/edit/delivery controls with all affected and shared dependent contracts. Change-source: pulse-maintainer --- .../v6/internal/subsystems/agent-lifecycle.md | 13 ++ .../v6/internal/subsystems/api-contracts.md | 33 +++ .../v6/internal/subsystems/notifications.md | 22 ++ .../internal/subsystems/security-privacy.md | 20 ++ .../internal/subsystems/storage-recovery.md | 16 ++ internal/api/alerting/notifications.go | 119 +++++----- internal/api/alerting/notifications_test.go | 196 ++++++++++++++-- internal/api/system_settings.go | 22 +- .../api/system_settings_telemetry_test.go | 218 ++++++++++++++++++ 9 files changed, 581 insertions(+), 78 deletions(-) diff --git a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md index 4aedb323f2..78b114e937 100644 --- a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md +++ b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md @@ -15,6 +15,19 @@ ## Purpose +### Shared origin and webhook edit boundaries + +The referenced `internal/api/` settings boundary treats explicit empty +`allowedOrigins` as a durable clear and omitted values as unchanged. Effective +GET values and deployment-owned overrides remain consistent; failed/rejected +saves cannot change live trust. This does not change agent identity, enrolment, +credential scope, execution permission, installer transport or TLS policy. +Notification Create/Update/Test consistently reject unresolved saved masks and +conflicting HTTP header identities before mutation or sending; literal edits, +masked responses and persist-before-publish remain intact. Source verification +uses `TestAllowedOrigins*` and `TestWebhookPlaceholderBoundaryAgreement` plus +the existing settings/notification authority and ordinary-delivery controls. + ### Cluster installation keyboard continuity `ClusterAgentDeployDialog` keeps one mounted dismissal button while loading, diff --git a/docs/release-control/v6/internal/subsystems/api-contracts.md b/docs/release-control/v6/internal/subsystems/api-contracts.md index 02781fb449..ae1493c62a 100644 --- a/docs/release-control/v6/internal/subsystems/api-contracts.md +++ b/docs/release-control/v6/internal/subsystems/api-contracts.md @@ -20,6 +20,39 @@ ## Purpose +### Origin-trust reversal and consistent webhook placeholders + +`POST /api/system/settings/update` treats the presence of a string +`allowedOrigins` as a patch: an explicit empty string removes saved and runtime +CORS trust, while omission changes neither. Null is not an empty-string clear. +Persist first; a failed save changes no runtime policy or settings cache. +`GET /api/system/settings` reports the effective value. Deployment-owned origins +remain authoritative: changing them returns `409 env_locked` before any save; +the form's unchanged locked value does not replace the underlying saved value +or prevent other settings edits. Authentication, settings scopes, CSRF, TLS, +comma-separated exact-origin matching and embedding policy stay independent. +`TestAllowedOriginsPatchBoundaries`, `TestAllowedOriginsSavedEffectiveLifecycle`, +`TestAllowedOriginsSaveFailurePreservesPolicy` and +`TestAllowedOriginsGetUsesDeploymentPolicy` bind these boundaries to the real +handlers, middleware, persisted settings, settings reload and production config +loader. This is source acceptance, not an installed deployment observation. + +Webhook Create, Update and saved-form Test use one masked-value resolver. +`***REDACTED***` means keep the corresponding saved value, not omit the field: +missing, masked or conflicting saved identities return `400` before saving, +live publication or a Test send. HTTP header names share case-insensitive +identity, including aliases alongside an exact match; custom-field keys remain +case-sensitive. Conflicting submitted header aliases are also rejected. +A masked signing secret needs a non-empty, non-placeholder saved secret. +Explicit additions/changes, omitted-map replacement/removals and blank-header +normalization retain the existing semantics; list/create/update responses stay +masked. Unsaved forms must supply literal values rather than saved masks. +`TestWebhookPlaceholderBoundaryAgreement` and +`TestWebhookSavedFormTestMatchesEncryptedEdit` exercise every entry point and a +real Test request plus encrypted edit/readback. Existing ordinary-delivery +integration retains grouped/ungrouped firing, resolution and restart controls; +these do not establish native Telegram acceptance or alter old queued jobs. + ### Existing disk History keeps native thermal fallback reachable The authenticated disk History endpoint supplements a missing or shallow diff --git a/docs/release-control/v6/internal/subsystems/notifications.md b/docs/release-control/v6/internal/subsystems/notifications.md index c0b64a6bca..81b2176a01 100644 --- a/docs/release-control/v6/internal/subsystems/notifications.md +++ b/docs/release-control/v6/internal/subsystems/notifications.md @@ -15,6 +15,28 @@ ## Purpose +### Saved webhook identity is consistent for editing and Test + +Create, Update and Test share one resolver for masked header/custom-field and +signing-secret values. Reject missing or ambiguous saved placeholders before +persisting, changing live destinations or sending a test request. HTTP header +identity is case-insensitive across the complete saved map, even where an exact +spelling is present; conflicting submitted aliases cannot depend on map order. +Custom fields retain exact, case-sensitive identity. A saved signing secret +must be non-empty and not itself a placeholder to satisfy a signing mask. + +The replacement map still honours explicit add/change/delete and omitted keys; +cleared headers are normalized away so the sender's JSON default survives. +List/create/update responses remain masked. Create and unsaved Test reject masks +instead of silently dropping credentials; a valid literal create still returns +masked values. Saved-form Test uses the same resolved edit as Update but does +not change live configuration or encrypted persistence. Persist-before-publish, +SSRF/TLS policy, routing/scopes, notification pause honesty and already-admitted +queue snapshots remain unchanged. Source controls +`TestWebhookPlaceholderBoundaryAgreement` and +`TestWebhookSavedFormTestMatchesEncryptedEdit` complement the existing real +ordinary-delivery/restart/grouping controls; none proves native #2540 relief. + ### Delivery health reconciliation follows the complete send transition The queue's automatic send worker records the retry/dead-letter outcome and diff --git a/docs/release-control/v6/internal/subsystems/security-privacy.md b/docs/release-control/v6/internal/subsystems/security-privacy.md index d87a1a3121..28c5431305 100644 --- a/docs/release-control/v6/internal/subsystems/security-privacy.md +++ b/docs/release-control/v6/internal/subsystems/security-privacy.md @@ -18,6 +18,26 @@ ## Purpose +### Removing browser-origin trust is a durable explicit patch + +The system settings handler accepts an explicit empty string for +`allowedOrigins` and applies it only after the durable save succeeds. Omitted +origins do not reapply a different saved value to the runtime. Null and +non-string values are invalid requests. GET presents the effective runtime +policy, not a saved value concealed by a deployment override. + +Either existing override marker (`ALLOWED_ORIGINS` or `allowedOrigins`) keeps +the deployment policy authoritative. A requested different value is rejected +with `409 env_locked` before any part of the patch is persisted. The unchanged +value sent by a disabled form field leaves underlying disk policy untouched +and permits other settings to be saved. Clearing origins grants no CORS headers; +production config reload does not revive removed trust. No change to the +existing development localhost fallback or deployment environment precedence +is made. CORS never replaces authentication, `settings:write`, CSRF, TLS or +iframe policy. The `TestAllowedOrigins*` controls in +`internal/api/system_settings_telemetry_test.go` cover real middleware responses, +exact-list matching, protected writes, rejected/failed saves and reload. + Assistant retained-history wiring uses the store belonging to its current monitor and read-only metric queries. Canonical identity resolution precedes store lookup, and ambiguous names fail with a request for canonical identity. diff --git a/docs/release-control/v6/internal/subsystems/storage-recovery.md b/docs/release-control/v6/internal/subsystems/storage-recovery.md index 254bd34b54..f14af8d245 100644 --- a/docs/release-control/v6/internal/subsystems/storage-recovery.md +++ b/docs/release-control/v6/internal/subsystems/storage-recovery.md @@ -21,6 +21,22 @@ ## Purpose +### Persisted trust edits and restart consistency + +The shared `internal/api/` settings path durably saves an explicit empty +`allowedOrigins` before clearing live CORS policy. An omitted field does not +replace a different effective value; a failed or deployment-locked patch does +not partially change disk/runtime/cache state. Production config reload retains +the saved clear while existing deployment overrides stay authoritative. +Webhook edits resolve every saved placeholder before persistence or live +publication, and saved-form Test uses the same resolution without changing +configuration. Unknown/ambiguous placeholders fail rather than silently lose a +credential. `TestAllowedOriginsSavedEffectiveLifecycle`, +`TestAllowedOriginsSaveFailurePreservesPolicy` and +`TestWebhookSavedFormTestMatchesEncryptedEdit` cover these shared boundaries. +No database/driver/schema, backup recovery, retention or old queue snapshot +policy changes are part of this repair. + ### Guest-memory read state stays independent of backup disk state The existing guest drawer retains Core's selected memory provenance through diff --git a/internal/api/alerting/notifications.go b/internal/api/alerting/notifications.go index ccf64de6dc..2c30e52232 100644 --- a/internal/api/alerting/notifications.go +++ b/internal/api/alerting/notifications.go @@ -422,41 +422,26 @@ func maskedWebhookResponse(webhook notifications.WebhookConfig) map[string]inter return whMap } -// restoreMaskedWebhookSecrets replaces each masked value in an incoming -// webhook with the saved value of the same header, custom field or signing -// secret. Restoring per key keeps any value the user typed in the same edit; -// a masked key with no saved value is dropped, so the mask is never stored or -// sent as a real value. -func restoreMaskedWebhookSecrets(incoming, saved notifications.WebhookConfig) notifications.WebhookConfig { - incoming.Headers = restoreMaskedWebhookValues(incoming.Headers, saved.Headers) - incoming.CustomFields = restoreMaskedWebhookValues(incoming.CustomFields, saved.CustomFields) +// restoreMaskedWebhookSecrets is the shared Create/Update/Test policy. A mask +// means keep this saved value, not discard the field or store the placeholder. +// Reject missing/ambiguous saved identities before persistence or any test send. +func restoreMaskedWebhookSecrets(incoming, saved notifications.WebhookConfig) (notifications.WebhookConfig, error) { + var err error + incoming.Headers, err = restoreStrictMaskedWebhookValues(incoming.Headers, saved.Headers, true) + if err != nil { + return notifications.WebhookConfig{}, err + } + incoming.CustomFields, err = restoreStrictMaskedWebhookValues(incoming.CustomFields, saved.CustomFields, false) + if err != nil { + return notifications.WebhookConfig{}, err + } if incoming.SigningSecret == maskedWebhookSecret { + if saved.SigningSecret == "" || saved.SigningSecret == maskedWebhookSecret { + return notifications.WebhookConfig{}, fmt.Errorf("no saved value for masked signing secret") + } incoming.SigningSecret = saved.SigningSecret } - return incoming -} - -func restoreMaskedWebhookValues(incoming, saved map[string]string) map[string]string { - if len(incoming) == 0 { - return incoming - } - restored := make(map[string]string, len(incoming)) - for key, value := range incoming { - if value != maskedWebhookSecret { - restored[key] = value - continue - } - if savedValue, ok := saved[key]; ok { - restored[key] = savedValue - } - } - return restored -} - -// stripMaskedWebhookSecrets drops masked values from a webhook that has no -// saved counterpart, such as a create or an unsaved form test. -func stripMaskedWebhookSecrets(webhook notifications.WebhookConfig) notifications.WebhookConfig { - return restoreMaskedWebhookSecrets(webhook, notifications.WebhookConfig{}) + return notifications.NormalizeWebhookConfig(incoming), nil } // CreateWebhook creates a new webhook @@ -479,7 +464,11 @@ func (h *NotificationHandlers) CreateWebhook(w http.ResponseWriter, r *http.Requ http.Error(w, err.Error(), http.StatusBadRequest) return } - webhook = notifications.NormalizeWebhookConfig(stripMaskedWebhookSecrets(webhook)) + webhook, err = restoreMaskedWebhookSecrets(webhook, notifications.WebhookConfig{}) + if err != nil { + http.Error(w, err.Error(), http.StatusBadRequest) + return + } monitor := h.getMonitor(r.Context()) manager := monitor.GetNotificationManager() @@ -518,25 +507,42 @@ func restoreStrictMaskedWebhookValues(incoming, existing map[string]string, head if incoming == nil { return nil, nil } - resolved := make(map[string]string, len(incoming)) - for key, value := range incoming { - if value == "***REDACTED***" { - stored, found := existing[key] - if !found && headers { - for oldKey, oldValue := range existing { - if strings.EqualFold(oldKey, key) { - if found && stored != oldValue { - return nil, fmt.Errorf("ambiguous saved value for masked header %q", key) - } - stored, found = oldValue, true - } - } + // Build the full HTTP identity index, even if an exact spelling exists: + // a legacy case-varied alias must not override or conceal a conflicting value. + savedHeaders := make(map[string]string) + ambiguousHeaders := make(map[string]bool) + if headers { + for key, value := range existing { + identity := strings.ToLower(key) + if previous, found := savedHeaders[identity]; found && previous != value { + ambiguousHeaders[identity] = true } - if !found || stored == "***REDACTED***" { + savedHeaders[identity] = value + } + } + resolved := make(map[string]string, len(incoming)) + incomingHeaders := make(map[string]string) + for key, value := range incoming { + identity := strings.ToLower(key) + if value == maskedWebhookSecret { + stored, found := existing[key] + if headers { + if ambiguousHeaders[identity] { + return nil, fmt.Errorf("ambiguous saved value for masked header %q", key) + } + stored, found = savedHeaders[identity] + } + if !found || stored == maskedWebhookSecret { return nil, fmt.Errorf("no saved value for masked field %q", key) } value = stored } + if headers { + if previous, found := incomingHeaders[identity]; found && previous != value { + return nil, fmt.Errorf("ambiguous submitted values for header %q", key) + } + incomingHeaders[identity] = value + } resolved[key] = value } return resolved, nil @@ -578,7 +584,6 @@ func (h *NotificationHandlers) UpdateWebhook(w http.ResponseWriter, r *http.Requ MinimumSeverity *string `json:"minimumSeverity"` } _ = json.Unmarshal(bodyBytes, &routingPresence) - webhook = notifications.NormalizeWebhookConfig(webhook) monitor := h.getMonitor(r.Context()) manager := monitor.GetNotificationManager() @@ -598,20 +603,11 @@ func (h *NotificationHandlers) UpdateWebhook(w http.ResponseWriter, r *http.Requ if routingPresence.MinimumSeverity == nil { webhook.MinimumSeverity = existing.MinimumSeverity } - webhook.Headers, err = restoreStrictMaskedWebhookValues(webhook.Headers, existing.Headers, true) + webhook, err = restoreMaskedWebhookSecrets(webhook, existing) if err != nil { http.Error(w, err.Error(), http.StatusBadRequest) return } - webhook.CustomFields, err = restoreStrictMaskedWebhookValues(webhook.CustomFields, existing.CustomFields, false) - if err != nil { - http.Error(w, err.Error(), http.StatusBadRequest) - return - } - // Preserve the signing secret if the incoming value is redacted - if webhook.SigningSecret == "***REDACTED***" { - webhook.SigningSecret = existing.SigningSecret - } break } } @@ -619,7 +615,6 @@ func (h *NotificationHandlers) UpdateWebhook(w http.ResponseWriter, r *http.Requ http.Error(w, fmt.Sprintf("webhook not found: %s", webhookID), http.StatusNotFound) return } - webhook = notifications.NormalizeWebhookConfig(webhook) // Validate webhook URL if err := manager.ValidateWebhookURL(webhook.URL); err != nil { @@ -962,7 +957,11 @@ func (h *NotificationHandlers) TestWebhook(w http.ResponseWriter, r *http.Reques } } } - basicWebhook = notifications.NormalizeWebhookConfig(restoreMaskedWebhookSecrets(basicWebhook, saved)) + basicWebhook, err = restoreMaskedWebhookSecrets(basicWebhook, saved) + if err != nil { + http.Error(w, err.Error(), http.StatusBadRequest) + return + } // Try to extract service from body if present var serviceCheck struct { diff --git a/internal/api/alerting/notifications_test.go b/internal/api/alerting/notifications_test.go index d16f2204ed..fd2c682938 100644 --- a/internal/api/alerting/notifications_test.go +++ b/internal/api/alerting/notifications_test.go @@ -1603,6 +1603,8 @@ func TestWebhookMaskedEditPersistsAndDelivers(t *testing.T) { if edit.Headers["Authorization"] != "***REDACTED***" || edit.CustomFields["token"] != "***REDACTED***" { t.Fatal("list disclosed credentials") } + edit.Headers["authorization"] = edit.Headers["Authorization"] + delete(edit.Headers, "Authorization") edit.Headers["Content-Type"], edit.Headers["X-Route"] = "application/json", "new" delete(edit.Headers, "X-Remove") edit.CustomFields["channel"] = "new" @@ -1624,7 +1626,7 @@ func TestWebhookMaskedEditPersistsAndDelivers(t *testing.T) { monitor.manager = m saved := m.GetWebhooks()[0] if saved.Service != service || saved.Template != hook.Template || saved.URL != hook.URL || - saved.Headers["Authorization"] != "synthetic-auth" || saved.Headers["Content-Type"] != "application/json" || + saved.Headers["authorization"] != "synthetic-auth" || saved.Headers["Content-Type"] != "application/json" || saved.Headers["X-Route"] != "new" || len(saved.Headers) != 3 || saved.CustomFields["token"] != "synthetic-token" || saved.CustomFields["channel"] != "new" || len(saved.CustomFields) != 2 { t.Fatal("masked edit did not survive encrypted reload with explicit edits/removals intact") @@ -1819,7 +1821,7 @@ func TestWebhookMaskedValuesResolvePerKey(t *testing.T) { assert.Equal(t, "", published.SigningSecret, "an omitted signing secret is removed, as before") }) - t.Run("create drops masked values and responds masked", func(t *testing.T) { + t.Run("create keeps literal values and responds masked", func(t *testing.T) { h, manager, persistence := newHandlers() var added notifications.WebhookConfig manager.On("ValidateWebhookURL", "https://hooks.example.com/new").Return(nil).Once() @@ -1831,7 +1833,7 @@ func TestWebhookMaskedValuesResolvePerKey(t *testing.T) { body, _ := json.Marshal(map[string]interface{}{ "name": "New", "url": "https://hooks.example.com/new", "enabled": true, - "headers": map[string]string{"Authorization": "***REDACTED***", "X-Api-Key": "typed-key"}, + "headers": map[string]string{"X-Api-Key": "typed-key"}, "signingSecret": "typed-signing-secret", }) rec := httptest.NewRecorder() @@ -1872,25 +1874,187 @@ func TestWebhookMaskedValuesResolvePerKey(t *testing.T) { assert.Equal(t, "saved-signing-secret", tested.SigningSecret) }) - t.Run("form test of an unsaved webhook never sends the mask", func(t *testing.T) { + t.Run("form test of an unsaved webhook rejects the mask before sending", func(t *testing.T) { h, manager, _ := newHandlers() - var tested notifications.EnhancedWebhookConfig - manager.On("TestEnhancedWebhook", mock.Anything).Run(func(args mock.Arguments) { - tested = args.Get(0).(notifications.EnhancedWebhookConfig) - }).Return(200, "OK", nil).Once() - manager.On("IsEnabled").Return(true).Once() - body, _ := json.Marshal(map[string]interface{}{ "url": "https://hooks.example.com/new", - "headers": map[string]string{"Authorization": "***REDACTED***", "X-Team": "platform"}, + "headers": map[string]string{"Authorization": maskedWebhookSecret, "X-Team": "platform"}, }) rec := httptest.NewRecorder() h.TestWebhook(rec, httptest.NewRequest(http.MethodPost, "/api/notifications/webhooks/test", bytes.NewReader(body))) - - assert.Equal(t, http.StatusOK, rec.Code) - _, hasAuthorization := tested.Headers["Authorization"] - assert.False(t, hasAuthorization, "a mask with no saved value must not be sent") - assert.Equal(t, "platform", tested.Headers["X-Team"]) + assert.Equal(t, http.StatusBadRequest, rec.Code) + manager.AssertNotCalled(t, "TestEnhancedWebhook", mock.Anything) manager.AssertNotCalled(t, "GetWebhooks") }) } + +// A masked form must fail before persistence or a Test send if its saved +// identity is missing or ambiguous. Test and Update must resolve the same map. +func TestWebhookPlaceholderBoundaryAgreement(t *testing.T) { + for _, route := range []string{"update", "test", "create"} { + for _, tc := range []struct { + name string + savedHeaders, incomingHeaders, savedFields, incomingFields map[string]string + savedSigning, incomingSigning string + valid bool + }{ + {"case varied saved header", map[string]string{"Authorization": "synthetic-auth"}, map[string]string{"authorization": maskedWebhookSecret}, nil, nil, "", "", true}, + {"unknown header", nil, map[string]string{"X-Missing": maskedWebhookSecret}, nil, nil, "", "", false}, + {"stored header is a mask", map[string]string{"Authorization": maskedWebhookSecret}, map[string]string{"Authorization": maskedWebhookSecret}, nil, nil, "", "", false}, + {"exact match also has conflicting alias", map[string]string{"Authorization": "synthetic-one", "authorization": "synthetic-two"}, map[string]string{"Authorization": maskedWebhookSecret}, nil, nil, "", "", false}, + {"incoming aliases conflict", nil, map[string]string{"X-Route": "one", "x-route": "two"}, nil, nil, "", "", false}, + {"custom fields are case sensitive", nil, nil, map[string]string{"Token": "synthetic-token"}, map[string]string{"token": maskedWebhookSecret}, "", "", false}, + {"missing signing secret", nil, nil, nil, nil, "", maskedWebhookSecret, false}, + {"saved signing secret is a mask", nil, nil, nil, nil, maskedWebhookSecret, maskedWebhookSecret, false}, + } { + if route == "create" && tc.valid { + continue + } + t.Run(route+"/"+tc.name, func(t *testing.T) { + manager := new(MockNotificationManager) + persistence := new(MockNotificationConfigPersistence) + monitor := new(MockNotificationMonitor) + monitor.On("GetNotificationManager").Return(manager) + monitor.On("GetConfigPersistence").Return(persistence) + saved := notifications.WebhookConfig{ID: "saved", URL: "https://hooks.example.test/alerts", Headers: tc.savedHeaders, CustomFields: tc.savedFields, SigningSecret: tc.savedSigning} + manager.On("GetWebhooks").Return([]notifications.WebhookConfig{saved}).Maybe() + manager.On("ValidateWebhookURL", mock.Anything).Return(nil).Maybe() + manager.On("IsEnabled").Return(true).Maybe() + manager.On("UpdateWebhook", mock.Anything, mock.Anything).Return(nil).Maybe() + manager.On("AddWebhook", mock.Anything).Return().Maybe() + persistence.On("SaveWebhooks", mock.Anything).Return(nil).Maybe() + var tested notifications.EnhancedWebhookConfig + manager.On("TestEnhancedWebhook", mock.Anything).Run(func(a mock.Arguments) { tested = a.Get(0).(notifications.EnhancedWebhookConfig) }).Return(200, "OK", nil).Maybe() + incoming := notifications.WebhookConfig{ID: "saved", URL: saved.URL, Headers: tc.incomingHeaders, CustomFields: tc.incomingFields, SigningSecret: tc.incomingSigning} + body, err := json.Marshal(incoming) + if err != nil { + t.Fatal(err) + } + rec := httptest.NewRecorder() + h := NewNotificationHandlers(nil, monitor) + switch route { + case "create": + h.CreateWebhook(rec, httptest.NewRequest(http.MethodPost, "/api/notifications/webhooks", bytes.NewReader(body))) + case "update": + h.UpdateWebhook(rec, httptest.NewRequest(http.MethodPut, "/api/notifications/webhooks/saved", bytes.NewReader(body))) + case "test": + h.TestWebhook(rec, httptest.NewRequest(http.MethodPost, "/api/notifications/webhooks/test", bytes.NewReader(body))) + } + if tc.valid { + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200", rec.Code) + } + if route == "test" && tested.Headers["authorization"] != "synthetic-auth" { + t.Fatal("Test silently dropped the case-varied saved credential") + } + } else { + if rec.Code != http.StatusBadRequest { + t.Errorf("status = %d, want 400", rec.Code) + } + manager.AssertNotCalled(t, "TestEnhancedWebhook", mock.Anything) + manager.AssertNotCalled(t, "UpdateWebhook", mock.Anything, mock.Anything) + manager.AssertNotCalled(t, "AddWebhook", mock.Anything) + persistence.AssertNotCalled(t, "SaveWebhooks", mock.Anything) + } + for _, secret := range []string{"synthetic-auth", "synthetic-one", "synthetic-two", "synthetic-token"} { + if strings.Contains(rec.Body.String(), secret) { + t.Error("response disclosed a synthetic saved value") + } + } + }) + } + } +} + +func TestWebhookSavedFormTestMatchesEncryptedEdit(t *testing.T) { + type observation struct { + header http.Header + payload string + } + var mu sync.Mutex + var observed []observation + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + var payload json.RawMessage + if err := json.NewDecoder(r.Body).Decode(&payload); err != nil { + w.WriteHeader(400) + return + } + mu.Lock() + observed = append(observed, observation{r.Header.Clone(), string(payload)}) + mu.Unlock() + fmt.Fprint(w, "OK") + })) + defer server.Close() + dir := t.TempDir() + persistence := config.NewConfigPersistence(dir) + saved := notifications.WebhookConfig{ID: "saved-form", Name: "Ops", URL: server.URL, Enabled: true, Service: "generic", Headers: map[string]string{"Authorization": "synthetic-auth", "Content-Type": "text/plain", "X-Delete": "remove"}, CustomFields: map[string]string{"token": "synthetic-token", "channel": "old", "delete": "remove"}, SigningSecret: "synthetic-signing"} + if err := persistence.SaveWebhooks([]notifications.WebhookConfig{saved}); err != nil { + t.Fatal(err) + } + manager := notifications.NewNotificationManagerWithDeferredQueue("", dir) + defer manager.Stop() + if err := manager.UpdateAllowedPrivateCIDRs("127.0.0.1/32,::1/128"); err != nil { + t.Fatal(err) + } + manager.AddWebhook(saved) + liveBefore := manager.GetWebhooks()[0] + h := NewNotificationHandlers(nil, &webhookEditMonitor{manager, persistence}) + listed := httptest.NewRecorder() + h.GetWebhooks(listed, httptest.NewRequest(http.MethodGet, "/api/notifications/webhooks", nil)) + var editable []notifications.WebhookConfig + if err := json.Unmarshal(listed.Body.Bytes(), &editable); err != nil || len(editable) != 1 { + t.Fatal("masked saved form unavailable") + } + form := editable[0] + form.Headers["authorization"] = form.Headers["Authorization"] + delete(form.Headers, "Authorization") + form.Headers["Content-Type"] = "" + delete(form.Headers, "X-Delete") + form.Headers["X-New"] = "new" + form.CustomFields["channel"] = "new" + delete(form.CustomFields, "delete") + body, err := json.Marshal(form) + if err != nil { + t.Fatal(err) + } + before, err := os.ReadFile(filepath.Join(dir, "webhooks.enc")) + if err != nil { + t.Fatal(err) + } + tested := httptest.NewRecorder() + h.TestWebhook(tested, httptest.NewRequest(http.MethodPost, "/api/notifications/webhooks/test", bytes.NewReader(body))) + if tested.Code != http.StatusOK { + t.Fatalf("saved form test: %d", tested.Code) + } + after, err := os.ReadFile(filepath.Join(dir, "webhooks.enc")) + if err != nil || !bytes.Equal(before, after) || !reflect.DeepEqual(manager.GetWebhooks()[0], liveBefore) { + t.Fatal("form Test mutated durable or live configuration") + } + mu.Lock() + got := append([]observation(nil), observed...) + mu.Unlock() + if len(got) != 1 || got[0].header.Get("Authorization") != "synthetic-auth" || got[0].header.Get("Content-Type") != "application/json" || got[0].header.Get("X-New") != "new" || got[0].header.Get("X-Delete") != "" || strings.Contains(got[0].payload, maskedWebhookSecret) { + t.Fatal("real Test did not honour saved identities, explicit edits/removals and JSON default") + } + updated := httptest.NewRecorder() + h.UpdateWebhook(updated, httptest.NewRequest(http.MethodPut, "/api/notifications/webhooks/saved-form", bytes.NewReader(body))) + if updated.Code != http.StatusOK { + t.Fatalf("saved form update: %d", updated.Code) + } + loaded, err := persistence.LoadWebhooks() + if err != nil || len(loaded) != 1 { + t.Fatal("encrypted edit unavailable after reload") + } + if loaded[0].Headers["authorization"] != "synthetic-auth" || loaded[0].Headers["X-New"] != "new" || len(loaded[0].Headers) != 2 || loaded[0].CustomFields["token"] != "synthetic-token" || loaded[0].CustomFields["channel"] != "new" || len(loaded[0].CustomFields) != 2 || loaded[0].SigningSecret != "synthetic-signing" { + t.Fatal("Test and persisted edit resolved different identities") + } + for _, response := range []string{listed.Body.String(), updated.Body.String(), tested.Body.String()} { + for _, secret := range []string{"synthetic-auth", "synthetic-token", "synthetic-signing"} { + if strings.Contains(response, secret) { + t.Fatal("configuration response disclosed a synthetic saved secret") + } + } + } + // The existing ordinary-delivery integration separately covers restart, + // grouped/un-grouped firing and resolution using the saved destination. +} diff --git a/internal/api/system_settings.go b/internal/api/system_settings.go index 77b991047e..c5951c71c1 100644 --- a/internal/api/system_settings.go +++ b/internal/api/system_settings.go @@ -380,6 +380,12 @@ func applyDiscoveryConfigOverrides(current config.DiscoveryConfig, cfgMap map[st // validateSystemSettings validates settings before applying them func validateSystemSettings(_ *config.SystemSettings, rawRequest map[string]interface{}) error { + if value, present := rawRequest["allowedOrigins"]; present { + if _, ok := value.(string); !ok { + return fmt.Errorf("allowedOrigins must be a string (use an empty string for same-origin only)") + } + } + if val, ok := rawRequest["reportBranding"]; ok { settings, ok := val.(map[string]interface{}) if !ok { @@ -713,6 +719,8 @@ func (h *SystemSettingsHandler) HandleGetSystemSettings(w http.ResponseWriter, r Msg("Loaded system settings for API response") settings.UpdateChannel = config.EffectiveUpdateChannel(settings.UpdateChannel, h.config.UpdateChannel) + // Expose the effective policy, including deployment overrides and clears. + settings.AllowedOrigins = h.config.AllowedOrigins // Always expose effective backup polling configuration settings.PVEPollingInterval = int(h.config.PVEPollingInterval.Seconds()) settings.BackupPollingInterval = int(h.config.BackupPollingInterval.Seconds()) @@ -866,6 +874,16 @@ func (h *SystemSettingsHandler) HandleUpdateSystemSettings(w http.ResponseWriter return } + // A disabled deployment-owned field is still included by the settings form. + // Accept an unchanged value without copying the override into system.json; + // reject attempts to change it, before persisting any part of the patch. + _, allowedOriginsUpdated := rawRequest["allowedOrigins"] + allowedOriginsLocked := h.config.EnvOverrides["ALLOWED_ORIGINS"] || h.config.EnvOverrides["allowedOrigins"] + if allowedOriginsUpdated && allowedOriginsLocked && updates.AllowedOrigins != h.config.AllowedOrigins { + writeErrorResponse(w, http.StatusConflict, "env_locked", "allowedOrigins is locked by the ALLOWED_ORIGINS environment variable", nil) + return + } + // Start with existing settings settings := *existingSettings discoveryConfigUpdated := false @@ -886,7 +904,7 @@ func (h *SystemSettingsHandler) HandleUpdateSystemSettings(w http.ResponseWriter if _, ok := rawRequest["backupPollingInterval"]; ok { settings.BackupPollingInterval = updates.BackupPollingInterval } - if updates.AllowedOrigins != "" { + if allowedOriginsUpdated && !allowedOriginsLocked { settings.AllowedOrigins = updates.AllowedOrigins } if _, ok := rawRequest["connectionTimeout"]; ok { @@ -1026,7 +1044,7 @@ func (h *SystemSettingsHandler) HandleUpdateSystemSettings(w http.ResponseWriter } h.config.PVEPollingInterval = newInterval } - if settings.AllowedOrigins != "" { + if allowedOriginsUpdated && !allowedOriginsLocked { h.config.AllowedOrigins = settings.AllowedOrigins } if settings.ConnectionTimeout > 0 { diff --git a/internal/api/system_settings_telemetry_test.go b/internal/api/system_settings_telemetry_test.go index b0fa5d75cc..1d720765c5 100644 --- a/internal/api/system_settings_telemetry_test.go +++ b/internal/api/system_settings_telemetry_test.go @@ -748,3 +748,221 @@ func TestIssue1638SettingsSaveResetsSSHFailureBackoff(t *testing.T) { t.Fatalf("settings save called ResetSSHFailureBackoff %d times, want 1", monitor.resetSSHFailureBackoffCalls) } } + +// Clearing browser trust must be an explicit patch, not a falsy-value no-op. +func TestAllowedOriginsPatchBoundaries(t *testing.T) { + for _, tc := range []struct { + name, saved, effective, body, wantSaved, wantEffective string + override string + wantStatus int + }{ + {"clear wildcard", "*", "*", `{"allowedOrigins":""}`, "", "", "", http.StatusOK}, + {"clear exact origins", "https://old.example", "https://old.example", `{"allowedOrigins":""}`, "", "", "", http.StatusOK}, + {"omission keeps effective policy", "https://saved.example", "https://effective.example", `{"theme":"dark"}`, "https://saved.example", "https://effective.example", "", http.StatusOK}, + {"null is not a clear", "*", "*", `{"allowedOrigins":null}`, "*", "*", "", http.StatusBadRequest}, + {"deployment lock uppercase", "https://saved.example", "https://deploy.example", `{"allowedOrigins":""}`, "https://saved.example", "https://deploy.example", "ALLOWED_ORIGINS", http.StatusConflict}, + {"deployment lock field name", "https://saved.example", "https://deploy.example", `{"allowedOrigins":"*"}`, "https://saved.example", "https://deploy.example", "allowedOrigins", http.StatusConflict}, + {"unchanged locked form saves other fields", "https://saved.example", "https://deploy.example", `{"allowedOrigins":"https://deploy.example","theme":"dark"}`, "https://saved.example", "https://deploy.example", "allowedOrigins", http.StatusOK}, + } { + t.Run(tc.name, func(t *testing.T) { + dir := t.TempDir() + cfg := &config.Config{DataPath: dir, ConfigPath: dir, AllowedOrigins: tc.effective, EnvOverrides: map[string]bool{}} + if tc.override != "" { + cfg.EnvOverrides[tc.override] = true + } + h, persistence, token := setupTelemetryTest(t, cfg) + initial := config.DefaultSystemSettings() + initial.AllowedOrigins = tc.saved + if err := persistence.SaveSystemSettings(*initial); err != nil { + t.Fatal(err) + } + reloads := 0 + h.reloadSystemSettingsFunc = func() { reloads++ } + req := httptest.NewRequest(http.MethodPost, "/api/system/settings/update", strings.NewReader(tc.body)) + req.Header.Set("X-API-Token", token) + rec := httptest.NewRecorder() + h.HandleUpdateSystemSettings(rec, req) + if rec.Code != tc.wantStatus { + t.Fatalf("status = %d, want %d", rec.Code, tc.wantStatus) + } + saved, err := persistence.LoadSystemSettings() + if err != nil || saved == nil { + t.Fatalf("read saved policy: %v", err) + } + if saved.AllowedOrigins != tc.wantSaved || cfg.AllowedOrigins != tc.wantEffective { + t.Fatal("saved/effective policy did not honour field presence and deployment precedence") + } + if tc.wantStatus != http.StatusOK && reloads != 0 { + t.Fatal("rejected patch reloaded runtime state") + } + if tc.wantStatus == http.StatusOK && reloads != 1 { + t.Fatal("successful patch did not reload settings") + } + if tc.name == "unchanged locked form saves other fields" && saved.Theme != "dark" { + t.Fatal("locked value prevented an unrelated settings save") + } + }) + } +} + +// Exercise the production middleware and handlers, disk, settings-cache reload +// and the production config loader; no simulated setter stands in for policy. +func TestAllowedOriginsSavedEffectiveLifecycle(t *testing.T) { + for _, original := range []string{"*", "https://old.example"} { + t.Run(original, func(t *testing.T) { + dir := t.TempDir() + t.Setenv("PULSE_DATA_DIR", dir) + t.Setenv("ALLOWED_ORIGINS", "") + t.Setenv("PULSE_DEV", "false") + t.Setenv("NODE_ENV", "production") + cfg := &config.Config{DataPath: dir, ConfigPath: dir, AllowedOrigins: original, PublicURL: "https://pulse.example", AuthUser: "admin", AuthPass: "synthetic-unused-hash", EnvOverrides: map[string]bool{}, TLSCertFile: "synthetic-cert", TLSKeyFile: "synthetic-key"} + h, persistence, token := setupTelemetryTest(t, cfg) + initial := config.DefaultSystemSettings() + initial.AllowedOrigins = original + initial.AllowedEmbedOrigins = "https://frame.example" + if err := persistence.SaveSystemSettings(*initial); err != nil { + t.Fatal(err) + } + router := &Router{config: cfg, persistence: persistence, mux: http.NewServeMux()} + h.reloadSystemSettingsFunc = router.reloadSystemSettings + router.mux.HandleFunc("/api/system/settings", RequireAdmin(cfg, RequireScope(config.ScopeSettingsRead, h.HandleGetSystemSettings))) + router.mux.HandleFunc("/api/system/settings/update", RequireAdmin(cfg, RequireScope(config.ScopeSettingsWrite, h.HandleUpdateSystemSettings))) + router.reloadSystemSettings() + request := func(method, path, origin, body, credential string) *httptest.ResponseRecorder { + req := httptest.NewRequest(method, path, strings.NewReader(body)) + if origin != "" { + req.Header.Set("Origin", origin) + } + if credential != "" { + req.Header.Set("X-API-Token", credential) + } + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + return rec + } + assertOrigin := func(origin, want string) { + t.Helper() + rec := request(http.MethodGet, "/api/system/settings", origin, "", token) + if rec.Code != http.StatusOK || rec.Header().Get("Access-Control-Allow-Origin") != want { + t.Fatal("effective CORS response does not match saved policy") + } + if rec.Header().Get("Access-Control-Allow-Credentials") != "" && want == "" { + t.Fatal("removed trust still enables credentialed CORS") + } + if want != "" && want != "*" && (rec.Header().Get("Access-Control-Allow-Credentials") != "true" || !strings.Contains(rec.Header().Get("Vary"), "Origin")) { + t.Fatal("exact-origin credential/Vary contract changed") + } + if want == "*" && rec.Header().Get("Access-Control-Allow-Credentials") != "" { + t.Fatal("wildcard gained credentialed browser access") + } + } + origin := "https://old.example" + assertOrigin(origin, original) + rec := request(http.MethodPost, "/api/system/settings/update", "", `{"allowedOrigins":""}`, token) + if rec.Code != http.StatusOK { + t.Fatalf("clear: status %d", rec.Code) + } + for _, candidate := range []string{origin, "https://untrusted.example", ""} { + assertOrigin(candidate, "") + } + preflight := request(http.MethodOptions, "/api/system/settings", origin, "", "") + if preflight.Code != http.StatusOK || preflight.Header().Get("Access-Control-Allow-Origin") != "" { + t.Fatal("cleared policy still grants preflight access") + } + get := request(http.MethodGet, "/api/system/settings", "", "", token) + var visible SystemSettingsResponse + if err := json.Unmarshal(get.Body.Bytes(), &visible); err != nil || visible.AllowedOrigins != "" { + t.Fatal("GET does not expose the effective clear") + } + saved, err := persistence.LoadSystemSettings() + if err != nil || saved == nil || saved.AllowedOrigins != "" || saved.AllowedEmbedOrigins != initial.AllowedEmbedOrigins || saved.AllowEmbedding { + t.Fatal("clear lost saved policy or widened embedding") + } + if cfg.TLSCertFile != "synthetic-cert" || cfg.TLSKeyFile != "synthetic-key" { + t.Fatal("CORS edit altered TLS configuration") + } + restarted, err := config.LoadWithoutLoggingInit() + if err != nil || restarted.AllowedOrigins != "" { + t.Fatal("production config reload revived removed trust") + } + if rec := request(http.MethodGet, "/api/system/settings", origin, "", ""); rec.Code != http.StatusUnauthorized { + t.Fatal("same-origin policy bypassed authentication") + } + // CORS grants never replace the session mutation's CSRF requirement. + session := generateSessionToken() + GetSessionStore().CreateSession(session, time.Hour, "cors-fixture", "192.0.2.1", "admin") + t.Cleanup(func() { GetSessionStore().DeleteSession(session) }) + req := httptest.NewRequest(http.MethodPost, "/api/system/settings/update", strings.NewReader(`{"allowedOrigins":"*"}`)) + req.Header.Set("User-Agent", "cors-fixture") + req.AddCookie(&http.Cookie{Name: sessionCookieName(false), Value: session}) + csrf := httptest.NewRecorder() + router.ServeHTTP(csrf, req) + if csrf.Code != http.StatusForbidden || cfg.AllowedOrigins != "" { + t.Fatal("CORS clear weakened CSRF") + } + const readToken = "cors-read-synthetic.12345678" + cfg.APITokens = append(cfg.APITokens, config.APITokenRecord{ID: "read-only", Hash: internalauth.HashAPIToken(readToken), Scopes: []string{config.ScopeSettingsRead}}) + if rec := request(http.MethodPost, "/api/system/settings/update", "", `{"allowedOrigins":"*"}`, readToken); rec.Code != http.StatusForbidden || cfg.AllowedOrigins != "" { + t.Fatal("settings:read token widened trust") + } + // An explicit new exact list works; prefixes and paths do not match. + rec = request(http.MethodPost, "/api/system/settings/update", "", `{"allowedOrigins":"https://trusted.example, https://other.example"}`, token) + if rec.Code != http.StatusOK { + t.Fatalf("set exact list: %d", rec.Code) + } + assertOrigin("https://trusted.example", "https://trusted.example") + assertOrigin("https://other.example", "https://other.example") + assertOrigin("https://trusted.example.evil", "") + assertOrigin("https://trusted.example/", "") + assertOrigin("", "") + }) + } +} + +func TestAllowedOriginsSaveFailurePreservesPolicy(t *testing.T) { + dir := t.TempDir() + cfg := &config.Config{DataPath: dir, ConfigPath: dir, AllowedOrigins: "https://trusted.example"} + h, persistence, token := setupTelemetryTest(t, cfg) + initial := config.DefaultSystemSettings() + initial.AllowedOrigins = cfg.AllowedOrigins + if err := persistence.SaveSystemSettings(*initial); err != nil { + t.Fatal(err) + } + before, err := os.ReadFile(filepath.Join(dir, "system.json")) + if err != nil { + t.Fatal(err) + } + persistence.SetFileSystem(failingWriteFileSystem{}) + reloads := 0 + h.reloadSystemSettingsFunc = func() { reloads++ } + req := httptest.NewRequest(http.MethodPost, "/api/system/settings/update", strings.NewReader(`{"allowedOrigins":""}`)) + req.Header.Set("X-API-Token", token) + rec := httptest.NewRecorder() + h.HandleUpdateSystemSettings(rec, req) + after, err := os.ReadFile(filepath.Join(dir, "system.json")) + if err != nil { + t.Fatal(err) + } + if rec.Code != http.StatusInternalServerError || cfg.AllowedOrigins != initial.AllowedOrigins || reloads != 0 || !bytes.Equal(before, after) { + t.Fatal("failed save changed durable/effective browser trust") + } +} + +func TestAllowedOriginsGetUsesDeploymentPolicy(t *testing.T) { + dir := t.TempDir() + cfg := &config.Config{DataPath: dir, ConfigPath: dir, AllowedOrigins: "https://deploy.example", EnvOverrides: map[string]bool{"allowedOrigins": true}} + h, persistence, token := setupTelemetryTest(t, cfg) + initial := config.DefaultSystemSettings() + initial.AllowedOrigins = "https://saved.example" + if err := persistence.SaveSystemSettings(*initial); err != nil { + t.Fatal(err) + } + req := httptest.NewRequest(http.MethodGet, "/api/system/settings", nil) + req.Header.Set("X-API-Token", token) + rec := httptest.NewRecorder() + h.HandleGetSystemSettings(rec, req) + var response SystemSettingsResponse + if err := json.Unmarshal(rec.Body.Bytes(), &response); err != nil || rec.Code != http.StatusOK || response.AllowedOrigins != cfg.AllowedOrigins || !response.EnvOverrides["allowedOrigins"] { + t.Fatal("GET misrepresented deployment-owned browser trust") + } +} From 890887363cb037b76c1142086cfa8856173f84c5 Mon Sep 17 00:00:00 2001 From: "pulse-triage[bot]" <249995291+pulse-triage[bot]@users.noreply.github.com> Date: Tue, 6 Oct 2026 11:05:34 +0100 Subject: [PATCH 2/2] Initialize persistent auth in origin lifecycle fixture The connected origin test uses real session and CSRF middleware. Initialise and clean up its temporary stores rather than depend on unrelated test order. Contract-Neutral: Test fixture setup only; runtime behaviour and subsystem contracts are unchanged. Change-source: pulse-maintainer --- internal/api/system_settings_telemetry_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/internal/api/system_settings_telemetry_test.go b/internal/api/system_settings_telemetry_test.go index 1d720765c5..bc955e59c7 100644 --- a/internal/api/system_settings_telemetry_test.go +++ b/internal/api/system_settings_telemetry_test.go @@ -815,6 +815,9 @@ func TestAllowedOriginsSavedEffectiveLifecycle(t *testing.T) { t.Setenv("ALLOWED_ORIGINS", "") t.Setenv("PULSE_DEV", "false") t.Setenv("NODE_ENV", "production") + InitPersistentAuthStores(dir) + t.Cleanup(resetSessionStoreForTests) + t.Cleanup(resetCSRFStoreForTests) cfg := &config.Config{DataPath: dir, ConfigPath: dir, AllowedOrigins: original, PublicURL: "https://pulse.example", AuthUser: "admin", AuthPass: "synthetic-unused-hash", EnvOverrides: map[string]bool{}, TLSCertFile: "synthetic-cert", TLSKeyFile: "synthetic-key"} h, persistence, token := setupTelemetryTest(t, cfg) initial := config.DefaultSystemSettings()