From 96001d134e1c4b50db306ecc95a003fc37dda719 Mon Sep 17 00:00:00 2001 From: rcourtman Date: Wed, 10 Jun 2026 11:37:39 +0100 Subject: [PATCH] Harden MSP tenant isolation: scope org-bound tokens away from default org, propagate webhook allowlist to all tenants Two gaps found by exercising the MSP pilot path live on a throwaway multi-tenant instance: 1. CheckAccess granted any authenticated principal access to the default org, so a token bound to a client org could read the provider's own default-org estate if it leaked from a client site. Org-bound tokens now fall through to the explicit binding check for the default org; authenticated users and legacy unbound tokens keep default-org access, and binding "default" explicitly still grants it. 2. The webhook private-target allowlist (instance-wide system setting) only ever reached the default org's notification manager on startup/reload, and only the request-context org on settings update. Tenant orgs' webhooks to private targets (per-client Gotify over VPN, the canonical MSP alert route) failed SSRF validation with no org-side remedy, and any allowlist died with a restart. Settings updates and reloads now fan out to every live tenant manager via the new MultiTenantMonitor.ForEachMonitor, and tenant monitors inherit the persisted allowlist and public URL at creation. Both fixes verified live: org-bound token vs default org returns 403; client-org webhooks to a private target succeed after restart and for orgs created after the allowlist was saved. MSP.md validation checklist gains the default-org probe and the allowlist guidance; MULTI_TENANT.md documents the binding semantics. Contracts updated for api-contracts, security-privacy, and monitoring with adjacency notes for agent-lifecycle, storage-recovery, and performance-and-scalability. --- docs/MSP.md | 17 ++++- docs/MULTI_TENANT.md | 2 +- .../v6/internal/subsystems/agent-lifecycle.md | 7 ++ .../v6/internal/subsystems/api-contracts.md | 12 ++++ .../v6/internal/subsystems/monitoring.md | 8 +++ .../subsystems/performance-and-scalability.md | 5 ++ .../internal/subsystems/security-privacy.md | 11 +++ .../internal/subsystems/storage-recovery.md | 5 ++ internal/api/authorization.go | 26 +++++-- internal/api/authorization_test.go | 25 +++++++ internal/api/contract_test.go | 25 +++++++ internal/api/router.go | 34 ++++++++- internal/api/security_regression_test.go | 71 +++++++++++++++++++ internal/api/system_settings.go | 46 +++++++++--- .../monitoring/canonical_guardrails_test.go | 30 ++++++++ internal/monitoring/multi_tenant_monitor.go | 20 ++++++ 16 files changed, 327 insertions(+), 17 deletions(-) diff --git a/docs/MSP.md b/docs/MSP.md index 34e27c3b4..7c210bfef 100644 --- a/docs/MSP.md +++ b/docs/MSP.md @@ -88,14 +88,23 @@ the tunnels and keep the management port out of them. ``` 4. **Cross-tenant isolation (shared-process mode only).** A token bound to one - organization must get `403` when targeting another: + organization must get `403` when targeting another organization AND when + targeting the default org (a leaked client-site token must not read the + provider's own estate): ```bash curl -sk -o /dev/null -w '%{http_code}\n' \ -H "X-API-Token: " -H "X-Pulse-Org-ID: org-b" \ https://pulse.internal:7655/api/alerts/active # 403 + curl -sk -o /dev/null -w '%{http_code}\n' \ + -H "X-API-Token: " -H "X-Pulse-Org-ID: default" \ + https://pulse.internal:7655/api/alerts/active # 403 ``` + Keep your own monitoring estate in its own organization too, rather than + in the default org, so every boundary in the instance is an explicit org + boundary. + ## Per-client alert routing Configure notification destinations inside each client's scope — the client @@ -103,6 +112,12 @@ runtime in the provider-hosted model, or the organization in shared-process mode. A per-client Gotify server, Slack channel, or PSA endpoint only ever sees that client's alerts. +Webhook targets on private IPs (a Gotify server reached over a VPN tunnel, +for example) are blocked by default for SSRF safety. Allow them once in +**Settings → System → Network → Webhook Security**; the allowlist is +instance-wide and applies to every organization, including ones created +later. + Alert webhook payloads carry the firing tenant's identity (`{{.TenantID}}`, `{{.TenantName}}`), so a single central PSA endpoint can also route by client. For ticket bridges (ConnectWise and similar), use the delivery contract — diff --git a/docs/MULTI_TENANT.md b/docs/MULTI_TENANT.md index 6aeddb6a1..885e46522 100644 --- a/docs/MULTI_TENANT.md +++ b/docs/MULTI_TENANT.md @@ -129,7 +129,7 @@ Use this for one company operating many internal sites, teams, departments, or e To onboard an internal estate: 1. **Create an organization for the estate** (see [Creating an Organization](#creating-an-organization)). -2. **Create an org-bound API token** with the `agent:report` scope, bound to that estate's organization (`orgId`). A token bound to a single organization automatically routes every agent that uses it into that organization, with no extra header required. +2. **Create an org-bound API token** with the `agent:report` scope, bound to that estate's organization (`orgId`). A token bound to a single organization automatically routes every agent that uses it into that organization, with no extra header required. Binding also scopes the token: an org-bound token cannot access other organizations, including the default org (bind `default` explicitly if a token genuinely needs it). Legacy unbound tokens keep their default-org access. 3. **Install the estate's agents** (Proxmox host, Docker, Kubernetes) using that token. Their telemetry lands in the selected organization. 4. **(Optional) Alias node names per estate.** If two estates both use the default `pve` hostname and you want them visually distinct, set `--hostname` (or the `PULSE_HOSTNAME` environment variable) on the agent, for example `--hostname "acme-pve1"`. See [UNIFIED_AGENT.md](UNIFIED_AGENT.md). 5. **(Optional) Isolate agent check-in on its own port.** When remote nodes reach the central server across the internet, enable [Split-Port Agent Ingest](CONFIGURATION.md#split-port-agent-ingest-network-isolation) so agents connect on a dedicated, firewalled port that exposes only `/api/agents/*` and never the web UI or management API. diff --git a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md index 99a8df606..3b16d33cb 100644 --- a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md +++ b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md @@ -1204,6 +1204,13 @@ profile and assignment columns, but embedded table framing must route through ## Current State +Default-org token scoping and notification-settings fan-out on shared +`internal/api/` handlers are likewise adjacent only: org-bound token denial +for the default org and instance-wide webhook allowlist propagation are +api-contract/security owned and change no install, enrollment, setup-token, +or fleet command authority. Agent tokens keep routing agents into their bound +org exactly as before. + Notification webhook management changes on shared `internal/api/` handlers are likewise adjacent only: the webhook `signingSecret` payload field, its masked list representation, and masked-echo preservation on update are diff --git a/docs/release-control/v6/internal/subsystems/api-contracts.md b/docs/release-control/v6/internal/subsystems/api-contracts.md index 6beca591a..3f401de3c 100644 --- a/docs/release-control/v6/internal/subsystems/api-contracts.md +++ b/docs/release-control/v6/internal/subsystems/api-contracts.md @@ -2024,6 +2024,18 @@ the canonical monitored-system blocked payload. ## Current State +That same shared API boundary now also owns the default-org token scoping +rule and instance-wide notification settings propagation. A token explicitly +bound to one or more organizations is scoped to those organizations only and +must not implicitly reach the default org's data; authenticated users and +legacy unbound tokens retain default-org access, and binding `default` +explicitly still grants it (pinned by +`TestContract_OrgBoundTokenIsScopedAwayFromDefaultOrg`). System settings that +shape notification transport (webhook private-target allowlist, public URL) +are stored instance-wide and must be applied to every live tenant monitor's +notification manager on update and reload, and inherited by tenant monitors at +creation, instead of reaching only the request's org or the default monitor. + That same notifications boundary also carries the webhook signing-secret payload contract: webhook management payloads may include an optional `signingSecret` that enables HMAC-signed deliveries, the list API must mask a diff --git a/docs/release-control/v6/internal/subsystems/monitoring.md b/docs/release-control/v6/internal/subsystems/monitoring.md index 8c7738f9d..dad486372 100644 --- a/docs/release-control/v6/internal/subsystems/monitoring.md +++ b/docs/release-control/v6/internal/subsystems/monitoring.md @@ -314,6 +314,14 @@ truth for live infrastructure data. ## Current State +That same reloadable multi-tenant monitor boundary also owns instance-wide +notification settings fan-out. `ForEachMonitor` visits every live tenant +monitor so callers can propagate the webhook security allowlist and public +URL to each org's notification manager, and tenant monitors inherit those +persisted settings at creation through the router's monitor initializer, so +an org created after the settings were saved (or after a restart) observes +the same allowlist as the default org. + This subsystem now sits under the dedicated core monitoring runtime lane so discovery, metrics-history correctness, and platform-specific runtime coverage can be governed as first-class product work instead of staying diluted inside diff --git a/docs/release-control/v6/internal/subsystems/performance-and-scalability.md b/docs/release-control/v6/internal/subsystems/performance-and-scalability.md index c767f0205..ec9f568a1 100644 --- a/docs/release-control/v6/internal/subsystems/performance-and-scalability.md +++ b/docs/release-control/v6/internal/subsystems/performance-and-scalability.md @@ -627,6 +627,11 @@ shell clickable behind another overlay. ## Current State +Default-org token scoping and notification-settings fan-out on shared +`internal/api/router.go` are likewise adjacent only: the tenant-monitor +settings inheritance hook runs once per tenant-monitor creation and changes +no chart transport, downsampling, or polling behavior. + The bars / sparklines toggle and its sparkline-range picker on the WorkloadsSurface support page-level ownership through four optional overrides exposed by diff --git a/docs/release-control/v6/internal/subsystems/security-privacy.md b/docs/release-control/v6/internal/subsystems/security-privacy.md index f6a3f43af..91903fd85 100644 --- a/docs/release-control/v6/internal/subsystems/security-privacy.md +++ b/docs/release-control/v6/internal/subsystems/security-privacy.md @@ -229,6 +229,17 @@ controls as normal product settings. ## Current State +The multi-tenant authorization boundary now also owns default-org token +scoping. An org-bound API token is a client-scoped credential: it must be +denied implicit access to the default org so a token that leaks from a client +site cannot read the provider's own estate, while authenticated users and +legacy unbound tokens keep default-org access for compatibility. The webhook +SSRF allowlist is the related instance-wide security setting: it must +propagate to every tenant org's notification manager (update, reload, and +tenant-monitor creation), because an allowlist that only the default org +observes silently denies legitimate per-client private webhook targets and +invites per-org security drift. + This subsystem now gives `L14` an explicit governed home for privacy guidance and telemetry disclosures instead of leaving those trust surfaces as lane-level evidence with no subsystem ownership. diff --git a/docs/release-control/v6/internal/subsystems/storage-recovery.md b/docs/release-control/v6/internal/subsystems/storage-recovery.md index 6cb77a38e..3e4a296d2 100644 --- a/docs/release-control/v6/internal/subsystems/storage-recovery.md +++ b/docs/release-control/v6/internal/subsystems/storage-recovery.md @@ -1180,6 +1180,11 @@ recovery scope, or a storage/recovery-owned secret source. ## Current State +Default-org token scoping and notification-settings fan-out on shared +`internal/api/` handlers are likewise adjacent only: they are +api-contract/security owned and create no storage, recovery-point, or +backup-surface semantics. + Notification webhook management changes on shared `internal/api/` handlers are likewise adjacent only: the webhook `signingSecret` payload field and its masking semantics are notifications/API-contract owned and create no storage, diff --git a/internal/api/authorization.go b/internal/api/authorization.go index 50479c1be..c5b8a5750 100644 --- a/internal/api/authorization.go +++ b/internal/api/authorization.go @@ -142,11 +142,27 @@ type AuthorizationResult struct { // CheckAccess performs a comprehensive authorization check for a request. func (c *DefaultAuthorizationChecker) CheckAccess(token *config.APITokenRecord, userID, orgID string) AuthorizationResult { - // The default organization is always accessible to any authenticated principal. - if orgID == "default" && (token != nil || userID != "") { - return AuthorizationResult{ - Allowed: true, - Reason: "Default organization is accessible to all authenticated users", + // The default organization stays accessible to authenticated users and to + // legacy unbound tokens. A token explicitly bound to organizations is + // scoped to those organizations only — binding expresses intent to limit, + // so it must not implicitly reach the default org's data (bind "default" + // explicitly to grant that). Without this, a client-bound MSP token that + // leaks from a client site could read the provider's own default-org + // estate. + if orgID == "default" { + if token != nil { + if token.IsLegacyToken() { + return AuthorizationResult{ + Allowed: true, + Reason: "Default organization is accessible to unbound tokens", + } + } + // Org-bound tokens fall through to the explicit binding check. + } else if userID != "" { + return AuthorizationResult{ + Allowed: true, + Reason: "Default organization is accessible to all authenticated users", + } } } diff --git a/internal/api/authorization_test.go b/internal/api/authorization_test.go index 689c5115f..50e50fe9f 100644 --- a/internal/api/authorization_test.go +++ b/internal/api/authorization_test.go @@ -144,4 +144,29 @@ func TestDefaultAuthorizationChecker_CheckAccess(t *testing.T) { assert.False(t, res.Allowed) assert.Equal(t, "No authentication context provided", res.Reason) }) + + t.Run("default org access by principal kind", func(t *testing.T) { + // Users keep implicit default-org access. + res := checker.CheckAccess(nil, "user1", "default") + assert.True(t, res.Allowed) + + // Legacy unbound tokens keep implicit default-org access. + res = checker.CheckAccess(&config.APITokenRecord{}, "", "default") + assert.True(t, res.Allowed) + + // A token bound to a client org must NOT implicitly reach the + // default org — binding expresses intent to limit. This is the MSP + // isolation boundary: a leaked client-site token cannot read the + // provider's own estate. + res = checker.CheckAccess(&config.APITokenRecord{OrgID: "client-acme"}, "", "default") + assert.False(t, res.Allowed) + res = checker.CheckAccess(&config.APITokenRecord{OrgIDs: []string{"client-acme", "client-beta"}}, "", "default") + assert.False(t, res.Allowed) + + // Binding "default" explicitly still grants it. + res = checker.CheckAccess(&config.APITokenRecord{OrgID: "default"}, "", "default") + assert.True(t, res.Allowed) + res = checker.CheckAccess(&config.APITokenRecord{OrgIDs: []string{"default", "client-acme"}}, "", "default") + assert.True(t, res.Allowed) + }) } diff --git a/internal/api/contract_test.go b/internal/api/contract_test.go index 804132d56..6b54f29e4 100644 --- a/internal/api/contract_test.go +++ b/internal/api/contract_test.go @@ -15480,3 +15480,28 @@ func TestContract_WebhookSigningSecretMaskedAndPreserved(t *testing.T) { } mockManager.AssertExpectations(t) } + +// TestContract_OrgBoundTokenIsScopedAwayFromDefaultOrg pins the MSP tenant +// isolation boundary: a token explicitly bound to client organizations must +// not implicitly reach the default org's data (a leaked client-site token +// must not read the provider's own estate), while users and legacy unbound +// tokens retain default-org access for backward compatibility. +func TestContract_OrgBoundTokenIsScopedAwayFromDefaultOrg(t *testing.T) { + checker := NewAuthorizationChecker(nil) + + if res := checker.CheckAccess(&config.APITokenRecord{OrgID: "client-acme"}, "", "default"); res.Allowed { + t.Fatal("org-bound token must not implicitly access the default org") + } + if res := checker.CheckAccess(&config.APITokenRecord{OrgIDs: []string{"client-a", "client-b"}}, "", "default"); res.Allowed { + t.Fatal("multi-org-bound token must not implicitly access the default org") + } + if res := checker.CheckAccess(&config.APITokenRecord{OrgID: "default"}, "", "default"); !res.Allowed { + t.Fatal("token explicitly bound to default must access the default org") + } + if res := checker.CheckAccess(&config.APITokenRecord{}, "", "default"); !res.Allowed { + t.Fatal("legacy unbound token must retain default-org access") + } + if res := checker.CheckAccess(nil, "admin", "default"); !res.Allowed { + t.Fatal("authenticated users must retain default-org access") + } +} diff --git a/internal/api/router.go b/internal/api/router.go index 099d3031f..992878692 100644 --- a/internal/api/router.go +++ b/internal/api/router.go @@ -1421,6 +1421,27 @@ func (r *Router) configureMonitorDependencies(m *monitoring.Monitor) { m.SetResourceStore(adapter) } + // Tenant monitors must inherit the persisted instance-wide notification + // settings. System settings are stored globally, so without this a tenant + // monitor created after the allowlist was saved (or after a restart) + // silently falls back to deny-all-private and per-client private webhook + // targets (e.g. MSP Gotify endpoints reached over VPN) fail SSRF + // validation with no org-side way to allow them. + if r.persistence != nil { + if settings, err := r.persistence.LoadSystemSettings(); err == nil && settings != nil { + if nm := m.GetNotificationManager(); nm != nil { + if settings.WebhookAllowedPrivateCIDRs != "" { + if err := nm.UpdateAllowedPrivateCIDRs(settings.WebhookAllowedPrivateCIDRs); err != nil { + log.Error().Err(err).Msg("Failed to apply webhook allowed private CIDRs to tenant monitor") + } + } + if settings.PublicURL != "" { + nm.SetPublicURL(settings.PublicURL) + } + } + } + } + if len(r.monitorSupplementalRecords) == 0 { return } @@ -3737,7 +3758,9 @@ func (r *Router) reloadSystemSettings() { r.config.HideLocalLogin = systemSettings.HideLocalLogin } - // Update webhook allowed private CIDRs in notification manager + // Update webhook allowed private CIDRs in notification managers. + // The setting is instance-wide, so every tenant monitor's manager + // must observe it, not just the default org's. if r.monitor != nil { if nm := r.monitor.GetNotificationManager(); nm != nil { if err := nm.UpdateAllowedPrivateCIDRs(systemSettings.WebhookAllowedPrivateCIDRs); err != nil { @@ -3745,6 +3768,15 @@ func (r *Router) reloadSystemSettings() { } } } + if r.mtMonitor != nil { + r.mtMonitor.ForEachMonitor(func(m *monitoring.Monitor) { + if nm := m.GetNotificationManager(); nm != nil { + if err := nm.UpdateAllowedPrivateCIDRs(systemSettings.WebhookAllowedPrivateCIDRs); err != nil { + log.Error().Err(err).Msg("Failed to update webhook allowed private CIDRs on tenant monitor during settings reload") + } + } + }) + } } else { // On error, use safe defaults r.cachedAllowEmbedding = false diff --git a/internal/api/security_regression_test.go b/internal/api/security_regression_test.go index 77de77df5..637a0ed5a 100644 --- a/internal/api/security_regression_test.go +++ b/internal/api/security_regression_test.go @@ -17,6 +17,7 @@ import ( "github.com/gorilla/websocket" "github.com/rcourtman/pulse-go-rewrite/internal/agentexec" "github.com/rcourtman/pulse-go-rewrite/internal/config" + "github.com/rcourtman/pulse-go-rewrite/internal/models" "github.com/rcourtman/pulse-go-rewrite/internal/monitoring" "github.com/rcourtman/pulse-go-rewrite/internal/servicediscovery" pulsews "github.com/rcourtman/pulse-go-rewrite/internal/websocket" @@ -4767,3 +4768,73 @@ func TestRequirePermissionUsesContextUsername(t *testing.T) { t.Fatalf("RBAC subject must come from context, got %q (header was %q)", observedSubject, "evil-spoofed-user") } } + +// Instance-wide webhook security settings must reach every tenant org's +// notification manager: the allowlist is stored globally, so applying it only +// to the request's org (or only to the default org on reload) leaves other +// tenants' webhook targets failing SSRF validation with no org-side remedy. +func TestSystemSettingsWebhookAllowlistPropagatesToAllTenantManagers(t *testing.T) { + t.Setenv("PULSE_MULTI_TENANT_ENABLED", "true") + defer SetMultiTenantEnabled(false) + SetMultiTenantEnabled(true) + + dataDir := t.TempDir() + tokenVal := "allowlist-propagation-token-123.12345678" + cfg := &config.Config{ + DataPath: dataDir, + APITokens: []config.APITokenRecord{ + {ID: "tok-allowlist", Hash: auth.HashAPIToken(tokenVal), Name: "Allowlist Test Token"}, + }, + } + persistence := config.NewConfigPersistence(dataDir) + mtp := config.NewMultiTenantPersistence(dataDir) + mtm := monitoring.NewMultiTenantMonitor(cfg, mtp, nil) + t.Cleanup(mtm.Stop) + + for _, org := range []struct{ id, name string }{ + {"org-a", "Org A"}, + {"org-b", "Org B"}, + } { + if err := mtp.SaveOrganization(&models.Organization{ID: org.id, DisplayName: org.name}); err != nil { + t.Fatalf("SaveOrganization(%s): %v", org.id, err) + } + if _, err := mtm.GetMonitor(org.id); err != nil { + t.Fatalf("GetMonitor(%s): %v", org.id, err) + } + } + + h := NewSystemSettingsHandler(cfg, persistence, nil, mtm, nil, func() {}, nil) + + // Both org managers must reject the private target before the update. + for _, orgID := range []string{"org-a", "org-b"} { + m, err := mtm.GetMonitor(orgID) + if err != nil { + t.Fatalf("GetMonitor(%s): %v", orgID, err) + } + if err := m.GetNotificationManager().ValidateWebhookURL("http://192.0.2.10:9999/hook"); err == nil { + t.Fatalf("%s: expected private webhook target to be rejected before allowlist update", orgID) + } + } + + body := bytes.NewBufferString(`{"webhookAllowedPrivateCIDRs":"192.0.2.0/24"}`) + req := httptest.NewRequest(http.MethodPost, "/api/system/settings/update", body) + req.Header.Set("Content-Type", "application/json") + req.Header.Set("X-API-Token", tokenVal) + rec := httptest.NewRecorder() + h.HandleUpdateSystemSettings(rec, req) + if rec.Code != http.StatusOK { + t.Fatalf("HandleUpdateSystemSettings responded %d: %s", rec.Code, rec.Body.String()) + } + + // Every live tenant manager must observe the new allowlist, regardless of + // which org the admin's request context pointed at. + for _, orgID := range []string{"org-a", "org-b"} { + m, err := mtm.GetMonitor(orgID) + if err != nil { + t.Fatalf("GetMonitor(%s): %v", orgID, err) + } + if err := m.GetNotificationManager().ValidateWebhookURL("http://192.0.2.10:9999/hook"); err != nil { + t.Fatalf("%s: expected private webhook target to be allowed after allowlist update, got %v", orgID, err) + } + } +} diff --git a/internal/api/system_settings.go b/internal/api/system_settings.go index add5e60b8..ed4f29791 100644 --- a/internal/api/system_settings.go +++ b/internal/api/system_settings.go @@ -134,6 +134,33 @@ func (h *SystemSettingsHandler) SetMultiTenantMonitor(mtm *monitoring.MultiTenan } } +// forEachNotificationManager applies fn to the notification manager of every +// live tenant monitor when multi-tenant iteration is available, falling back +// to the request's monitor otherwise. Instance-wide notification settings +// (webhook security allowlist, public URL) must reach every org's manager. +func (h *SystemSettingsHandler) forEachNotificationManager(r *http.Request, fn func(*notifications.NotificationManager)) { + h.stateMu.RLock() + mtMonitor := h.mtMonitor + h.stateMu.RUnlock() + + type monitorRanger interface { + ForEachMonitor(func(*monitoring.Monitor)) + } + if ranger, ok := mtMonitor.(monitorRanger); ok && ranger != nil { + ranger.ForEachMonitor(func(m *monitoring.Monitor) { + if nm := m.GetNotificationManager(); nm != nil { + fn(nm) + } + }) + return + } + if monitor := h.getMonitor(r.Context()); monitor != nil { + if nm := monitor.GetNotificationManager(); nm != nil { + fn(nm) + } + } +} + func (h *SystemSettingsHandler) getMonitor(ctx context.Context) SystemSettingsMonitor { h.stateMu.RLock() mtMonitor := h.mtMonitor @@ -1045,18 +1072,19 @@ func (h *SystemSettingsHandler) HandleUpdateSystemSettings(w http.ResponseWriter h.getMonitor(r.Context()).DisableTemperatureMonitoring() } } - if cidrUpdateRequested && h.getMonitor(r.Context()) != nil { - if nm := h.getMonitor(r.Context()).GetNotificationManager(); nm != nil { + // Webhook security allowlist and public URL are instance-wide settings: + // apply them to every live tenant monitor's notification manager, not + // just the org the admin happened to have selected. + if cidrUpdateRequested { + h.forEachNotificationManager(r, func(nm *notifications.NotificationManager) { nm.ApplyAllowedPrivateCIDRs(settings.WebhookAllowedPrivateCIDRs, parsedCIDRNets) - } + }) } if _, ok := rawRequest["publicURL"]; ok { - if h.getMonitor(r.Context()) != nil { - if nm := h.getMonitor(r.Context()).GetNotificationManager(); nm != nil { - nm.SetPublicURL(settings.PublicURL) - log.Info().Str("publicURL", settings.PublicURL).Msg("Updated notification public URL from settings") - } - } + h.forEachNotificationManager(r, func(nm *notifications.NotificationManager) { + nm.SetPublicURL(settings.PublicURL) + }) + log.Info().Str("publicURL", settings.PublicURL).Msg("Updated notification public URL from settings") } // Reload cached system settings after successful save diff --git a/internal/monitoring/canonical_guardrails_test.go b/internal/monitoring/canonical_guardrails_test.go index 6ca71e932..9fafa10cc 100644 --- a/internal/monitoring/canonical_guardrails_test.go +++ b/internal/monitoring/canonical_guardrails_test.go @@ -2281,3 +2281,33 @@ func TestTenantMonitorWiresOrgIdentityIntoNotifications(t *testing.T) { t.Fatalf("TenantIdentity() name after rename = %q, want %q", tenantName, "Acme Corp Renamed") } } + +// Instance-wide notification settings (webhook security allowlist, public +// URL) propagate through ForEachMonitor; it must visit every live tenant +// monitor so no org's manager is left observing stale security settings. +func TestForEachMonitorVisitsAllTenantMonitors(t *testing.T) { + mtm := &MultiTenantMonitor{ + monitors: map[string]*Monitor{ + "default": {}, + "org-a": {}, + "org-b": {}, + "nil-org": nil, + }, + } + + visited := 0 + mtm.ForEachMonitor(func(m *Monitor) { + if m == nil { + t.Fatal("ForEachMonitor must skip nil monitors") + } + visited++ + }) + if visited != 3 { + t.Fatalf("ForEachMonitor visited %d monitors, want 3", visited) + } + + // Nil receiver and nil callback are safe no-ops. + var nilMTM *MultiTenantMonitor + nilMTM.ForEachMonitor(func(*Monitor) { t.Fatal("nil receiver must not invoke callback") }) + mtm.ForEachMonitor(nil) +} diff --git a/internal/monitoring/multi_tenant_monitor.go b/internal/monitoring/multi_tenant_monitor.go index 3502cf992..2fc7dcf29 100644 --- a/internal/monitoring/multi_tenant_monitor.go +++ b/internal/monitoring/multi_tenant_monitor.go @@ -84,6 +84,26 @@ func (mtm *MultiTenantMonitor) SetMonitorInitializer(initializer func(*Monitor)) } } +// ForEachMonitor runs fn against every live tenant monitor. Use it to +// propagate instance-wide runtime settings (e.g. webhook security allowlist) +// that every org's monitor must observe. +func (mtm *MultiTenantMonitor) ForEachMonitor(fn func(*Monitor)) { + if mtm == nil || fn == nil { + return + } + mtm.mu.RLock() + monitors := make([]*Monitor, 0, len(mtm.monitors)) + for _, monitor := range mtm.monitors { + if monitor != nil { + monitors = append(monitors, monitor) + } + } + mtm.mu.RUnlock() + for _, monitor := range monitors { + fn(monitor) + } +} + // GetMonitor returns the monitor instance for a specific organization. // It lazily initializes the monitor if it doesn't exist. func (mtm *MultiTenantMonitor) GetMonitor(orgID string) (*Monitor, error) {