Warn when a system settings read fails in router settings paths

A LoadSystemSettings error in configureMonitorDependencies silently
skipped the tenant-monitor inherit block, leaving the monitor on
deny-all-private webhook CIDRs with nothing in the logs to explain why
private webhook targets fail SSRF validation. reloadSystemSettings
fails closed on the same error (embedding off) deliberately, but was
equally silent, so a persistent read failure looked identical to
embedding being switched off on purpose.

Both paths now log a warning with the error, mirroring the pattern from
cf5b86e62 in internal/config. A missing system.json (fresh install)
still stays quiet. Tests cover the warning on read failure, the quiet
fresh-install path, and that the reload still fails closed.
This commit is contained in:
rcourtman
2026-08-20 14:49:36 +01:00
parent cf5b86e624
commit ffe47fa4b7
2 changed files with 95 additions and 0 deletions
+8
View File
@@ -1903,6 +1903,8 @@ func (r *Router) configureMonitorDependencies(m *monitoring.Monitor) {
nm.SetPublicURL(settings.PublicURL)
}
}
} else if err != nil {
log.Warn().Err(err).Str("org", m.GetOrgID()).Msg("Failed to load system settings for tenant monitor; webhook private CIDR allowlist not applied")
}
}
@@ -4365,6 +4367,12 @@ func (r *Router) reloadSystemSettings() {
})
}
} else {
if err != nil {
// Failing closed (embedding off) is deliberate, but the error
// must still be observable — a persistent read failure otherwise
// looks identical to embedding being switched off on purpose.
log.Warn().Err(err).Msg("Failed to load system settings during reload; using safe defaults")
}
// On error, use safe defaults
r.cachedAllowEmbedding = false
r.cachedAllowedOrigins = ""
@@ -1,10 +1,15 @@
package api
import (
"os"
"path/filepath"
"strings"
"testing"
"github.com/rcourtman/pulse-go-rewrite/internal/config"
"github.com/rcourtman/pulse-go-rewrite/internal/monitoring"
"github.com/rs/zerolog"
"github.com/rs/zerolog/log"
)
// TestReloadSystemSettings_AppliesWebhookCIDRsToNewMonitor verifies the fix
@@ -93,3 +98,85 @@ func TestReloadSystemSettings_AppliesWebhookCIDRsToNewMonitor(t *testing.T) {
t.Fatal("after ReloadSystemSettings: expected 172.16.0.1 to be rejected (not in allowlist)")
}
}
// captureRouterSettingsLogs swaps the global logger for a locked buffer so
// tests can assert on warnings emitted by system-settings load failures.
func captureRouterSettingsLogs(t *testing.T) *lockedLogBuffer {
t.Helper()
var buf lockedLogBuffer
prevLogger := log.Logger
prevLevel := zerolog.GlobalLevel()
log.Logger = zerolog.New(&buf).Level(zerolog.DebugLevel)
zerolog.SetGlobalLevel(zerolog.DebugLevel)
t.Cleanup(func() {
log.Logger = prevLogger
zerolog.SetGlobalLevel(prevLevel)
})
return &buf
}
// unreadableSystemSettings returns a ConfigPersistence whose LoadSystemSettings
// fails: a directory named system.json produces a read error that is not
// os.IsNotExist — the same shape as a transient read failure.
func unreadableSystemSettings(t *testing.T) *config.ConfigPersistence {
t.Helper()
dataDir := t.TempDir()
if err := os.Mkdir(filepath.Join(dataDir, "system.json"), 0o700); err != nil {
t.Fatalf("mkdir system.json: %v", err)
}
return config.NewConfigPersistence(dataDir)
}
// A failed settings read while wiring a tenant monitor means the webhook
// private CIDR allowlist silently never applies and private webhook targets
// fail SSRF validation with no org-side fix — the failure must be logged.
func TestConfigureMonitorDependencies_WarnsWhenSettingsLoadFails(t *testing.T) {
logOutput := captureRouterSettingsLogs(t)
router := &Router{persistence: unreadableSystemSettings(t)}
router.configureMonitorDependencies(&monitoring.Monitor{})
if !strings.Contains(logOutput.String(), "Failed to load system settings for tenant monitor") {
t.Fatalf("expected settings-load failure warning, got logs: %s", logOutput.String())
}
}
// A missing system.json is a normal fresh install, not a failure — the tenant
// monitor wiring must stay quiet so fresh setups don't log spurious warnings.
func TestConfigureMonitorDependencies_NoWarningWhenSettingsFileMissing(t *testing.T) {
logOutput := captureRouterSettingsLogs(t)
router := &Router{persistence: config.NewConfigPersistence(t.TempDir())}
router.configureMonitorDependencies(&monitoring.Monitor{})
if strings.Contains(logOutput.String(), "Failed to load system settings") {
t.Fatalf("missing system.json must not warn, got logs: %s", logOutput.String())
}
}
// reloadSystemSettings deliberately fails closed (embedding off) on a settings
// read error, but the error must be observable: without a log line a
// persistent read failure looks identical to embedding being switched off on
// purpose.
func TestReloadSystemSettings_WarnsWhenLoadFailsAndFailsClosed(t *testing.T) {
logOutput := captureRouterSettingsLogs(t)
router := &Router{
config: &config.Config{},
persistence: unreadableSystemSettings(t),
}
router.cachedAllowEmbedding = true
router.cachedAllowedOrigins = "https://example.com"
router.ReloadSystemSettings()
if router.cachedAllowEmbedding || router.cachedAllowedOrigins != "" {
t.Fatal("a failed settings read must fail closed (embedding off, origins cleared)")
}
if !strings.Contains(logOutput.String(), "Failed to load system settings during reload") {
t.Fatalf("expected settings-load failure warning, got logs: %s", logOutput.String())
}
}