From 0489dd39cb007f22aeb693051bf8a16a80bc7c7a Mon Sep 17 00:00:00 2001 From: Tobias Gesellchen Date: Wed, 29 Jul 2026 18:59:46 +0200 Subject: [PATCH] fix(admin): stop Save Settings from dropping fields the UI doesn't manage HandleUpdateSettings and HandleUpdateLoggingSettings each built a fresh datastore.Settings{} from scratch before saving, so any field not covered by that handler's own DTO (e.g. hand-edited trust_forwarded_headers / trusted_proxy_cidrs) was silently reset to its zero value on every save. Load the persisted settings first and overlay only the fields each handler actually owns, matching the pattern already used elsewhere (addMargeHostToTLSFix). Fixes #589 Co-Authored-By: Claude Sonnet 5 --- pkg/service/handlers/handlers_setup.go | 139 ++++++++++++-------- pkg/service/handlers/handlers_setup_test.go | 97 ++++++++++++++ 2 files changed, 183 insertions(+), 53 deletions(-) diff --git a/pkg/service/handlers/handlers_setup.go b/pkg/service/handlers/handlers_setup.go index 515e52e..fc2e2b6 100644 --- a/pkg/service/handlers/handlers_setup.go +++ b/pkg/service/handlers/handlers_setup.go @@ -277,6 +277,25 @@ func (s *Server) HandleGetSettings(w http.ResponseWriter, _ *http.Request) { } } +// parseDNSUpstreamList splits a comma-separated DNS upstream list into its +// trimmed, non-empty entries. Returns nil for an empty input. +func parseDNSUpstreamList(dnsUpstream string) []string { + if dnsUpstream == "" { + return nil + } + + var upstreamList []string + + for _, u := range strings.Split(dnsUpstream, ",") { + u = strings.TrimSpace(u) + if u != "" { + upstreamList = append(upstreamList, u) + } + } + + return upstreamList +} + // HandleUpdateSettings updates the service settings. func (s *Server) HandleUpdateSettings(w http.ResponseWriter, r *http.Request) { var settings struct { @@ -360,20 +379,7 @@ func (s *Server) HandleUpdateSettings(w http.ResponseWriter, r *http.Request) { } s.dnsEnabled = settings.DNSEnabled - - // Handle comma-separated upstream DNS servers - var upstreamList []string - - if settings.DNSUpstream != "" { - for _, u := range strings.Split(settings.DNSUpstream, ",") { - u = strings.TrimSpace(u) - if u != "" { - upstreamList = append(upstreamList, u) - } - } - } - - s.dnsUpstream = upstreamList + s.dnsUpstream = parseDNSUpstreamList(settings.DNSUpstream) s.dnsBindAddr = settings.DNSBindAddr s.internalPaths = settings.InternalPaths @@ -407,42 +413,55 @@ func (s *Server) HandleUpdateSettings(w http.ResponseWriter, r *http.Request) { // HTTPS URL keeps following the Target Domain across restarts. currentHTTPS := s.httpsOverride + // Load the persisted settings first and overlay only the fields this + // handler owns, instead of building a fresh struct from scratch. Fields + // with no in-memory counterpart on Server (e.g. TrustForwardedHeaders, + // TrustedProxyCIDRs, TuneInStreamFormats) are only ever set by hand-editing + // settings.json; overwriting with a fresh struct would silently drop them + // (issue #589). + persisted, err := s.ds.GetSettings() + if err != nil { + s.mu.Unlock() + http.Error(w, "Failed to load existing settings: "+err.Error(), http.StatusInternalServerError) + + return + } + // Resolve TLS extra hosts: nil pointer means "field omitted, preserve existing"; // non-nil (even empty) means "replace with this list". - resolvedTLSExtraHosts := s.persistedTLSExtraHosts() + resolvedTLSExtraHosts := persisted.TLSExtraHosts if settings.TLSExtraHosts != nil { resolvedTLSExtraHosts = normaliseTLSExtraHosts(*settings.TLSExtraHosts) } log.Printf("Saving updated settings to %s/settings.json", s.ds.DataDir) - err = s.ds.SaveSettings(datastore.Settings{ - ServerURL: s.serverURL, - HTTPServerURL: currentHTTPS, - RedactLogs: currentRedact, - LogBodies: currentLogBody, - RecordInteractions: currentRecord, - DiscoveryInterval: s.discoveryInterval.String(), - DiscoveryEnabled: s.discoveryEnabled, - DNSEnabled: s.dnsEnabled, - DNSUpstream: s.dnsUpstream, - DNSBindAddr: s.dnsBindAddr, - InternalPaths: s.internalPaths, - Shortcuts: s.shortcuts, - SpotifyClientID: s.spotifyClientID, - SpotifyClientSecret: s.spotifyClientSecret, - SpotifyRedirectURI: s.spotifyRedirectURI, - AmazonClientID: s.amazonClientID, - AmazonClientSecret: s.amazonClientSecret, - AmazonRedirectURI: s.amazonRedirectURI, - TTSProvider: s.ttsProvider, - TTSGoogleAPIKey: s.ttsGoogleAPIKey, - TTSAppKey: s.ttsAppKey, - TTSLanguage: s.ttsLanguage, - TTSVoice: s.ttsVoice, - TTSVolume: s.ttsVolume, - TLSExtraHosts: resolvedTLSExtraHosts, - DefaultLanding: defaultLanding, - }) + persisted.ServerURL = s.serverURL + persisted.HTTPServerURL = currentHTTPS + persisted.RedactLogs = currentRedact + persisted.LogBodies = currentLogBody + persisted.RecordInteractions = currentRecord + persisted.DiscoveryInterval = s.discoveryInterval.String() + persisted.DiscoveryEnabled = s.discoveryEnabled + persisted.DNSEnabled = s.dnsEnabled + persisted.DNSUpstream = s.dnsUpstream + persisted.DNSBindAddr = s.dnsBindAddr + persisted.InternalPaths = s.internalPaths + persisted.Shortcuts = s.shortcuts + persisted.SpotifyClientID = s.spotifyClientID + persisted.SpotifyClientSecret = s.spotifyClientSecret + persisted.SpotifyRedirectURI = s.spotifyRedirectURI + persisted.AmazonClientID = s.amazonClientID + persisted.AmazonClientSecret = s.amazonClientSecret + persisted.AmazonRedirectURI = s.amazonRedirectURI + persisted.TTSProvider = s.ttsProvider + persisted.TTSGoogleAPIKey = s.ttsGoogleAPIKey + persisted.TTSAppKey = s.ttsAppKey + persisted.TTSLanguage = s.ttsLanguage + persisted.TTSVoice = s.ttsVoice + persisted.TTSVolume = s.ttsVolume + persisted.TLSExtraHosts = resolvedTLSExtraHosts + persisted.DefaultLanding = defaultLanding + err = s.ds.SaveSettings(persisted) dnsEnabled := s.dnsEnabled dnsUpstreamStr := strings.Join(s.dnsUpstream, ",") @@ -1021,17 +1040,31 @@ func (s *Server) HandleUpdateLoggingSettings(w http.ResponseWriter, r *http.Requ discoveryInterval := s.discoveryInterval.String() discoveryEnabled := s.discoveryEnabled + // Load the persisted settings first and overlay only the fields this + // handler owns, instead of building a fresh struct from scratch. This + // handler's DTO only ever covers 3 of ~25 fields, so a from-scratch + // struct used to reset everything else (credentials, DNS config, + // TrustForwardedHeaders/TrustedProxyCIDRs, ...) to its zero value on + // every save (issue #589). + persisted, err := s.ds.GetSettings() + if err != nil { + s.mu.Unlock() + http.Error(w, "Failed to load existing settings: "+err.Error(), http.StatusInternalServerError) + + return + } + + persisted.ServerURL = serverURL + persisted.HTTPServerURL = httpsOverride + persisted.RedactLogs = s.redactLogs + persisted.LogBodies = s.logBodies + persisted.RecordInteractions = s.recordEnabled + persisted.DiscoveryInterval = discoveryInterval + persisted.DiscoveryEnabled = discoveryEnabled + persisted.Shortcuts = s.shortcuts + log.Printf("Saving updated proxy settings to %s/settings.json", s.ds.DataDir) - err := s.ds.SaveSettings(datastore.Settings{ - ServerURL: serverURL, - HTTPServerURL: httpsOverride, - RedactLogs: s.redactLogs, - LogBodies: s.logBodies, - RecordInteractions: s.recordEnabled, - DiscoveryInterval: discoveryInterval, - DiscoveryEnabled: discoveryEnabled, - Shortcuts: s.shortcuts, - }) + err = s.ds.SaveSettings(persisted) s.mu.Unlock() if err != nil { diff --git a/pkg/service/handlers/handlers_setup_test.go b/pkg/service/handlers/handlers_setup_test.go index f9ecf18..aa9c891 100644 --- a/pkg/service/handlers/handlers_setup_test.go +++ b/pkg/service/handlers/handlers_setup_test.go @@ -162,6 +162,103 @@ func TestProxySettingsAPI(t *testing.T) { } } +// TestSettingsSavePreservesUnmanagedFields is the regression test for +// issue #589: saving settings via either the main settings form or the +// logging/proxy panel must not drop fields that have no counterpart in +// their respective request DTOs (e.g. hand-edited trust_forwarded_headers / +// trusted_proxy_cidrs, or the other handler's owned fields). +func TestSettingsSavePreservesUnmanagedFields(t *testing.T) { + tempDir, err := os.MkdirTemp("", "settings-preserve-test") + if err != nil { + t.Fatalf("Failed to create temp dir: %v", err) + } + defer os.RemoveAll(tempDir) + + ds := datastore.NewDataStore(tempDir) + _ = ds.Initialize() + + // Seed settings.json with fields neither handler's DTO exposes. + seeded := datastore.Settings{ + ServerURL: "http://127.0.0.1:8000", + TrustForwardedHeaders: true, + TrustedProxyCIDRs: []string{"10.42.0.0/16"}, + } + if err := ds.SaveSettings(seeded); err != nil { + t.Fatalf("Failed to seed settings: %v", err) + } + + r, server := setupRouter("http://127.0.0.1:8000", ds) + ts := httptest.NewServer(r) + defer ts.Close() + + // Simulate a credential already loaded into the running server (as + // main.go's startup wiring does) but not managed by the logging panel's + // DTO, to catch HandleUpdateLoggingSettings resetting fields it doesn't + // own back to their zero value. + server.spotifyClientID = "seeded-spotify-client-id" + + // Saving the main settings form (which knows nothing about + // trust_forwarded_headers / trusted_proxy_cidrs) must not drop them. + sysUpdate := map[string]string{"server_url": "http://127.0.0.1:8000"} + sysBody, err := json.Marshal(sysUpdate) + if err != nil { + t.Fatalf("Failed to marshal update: %v", err) + } + + res, err := http.Post(ts.URL+"/setup/settings", "application/json", bytes.NewBuffer(sysBody)) + if err != nil { + t.Fatal(err) + } + res.Body.Close() + + if res.StatusCode != http.StatusOK { + t.Fatalf("POST /setup/settings: expected status OK, got %v", res.Status) + } + + persisted, err := ds.GetSettings() + if err != nil { + t.Fatalf("Failed to reload settings: %v", err) + } + if !persisted.TrustForwardedHeaders { + t.Errorf("POST /setup/settings dropped TrustForwardedHeaders: %+v", persisted) + } + if len(persisted.TrustedProxyCIDRs) != 1 || persisted.TrustedProxyCIDRs[0] != "10.42.0.0/16" { + t.Errorf("POST /setup/settings dropped TrustedProxyCIDRs: %+v", persisted) + } + + // Saving the logging/proxy panel (which only knows redact/log_body/record) + // must not drop these fields, or the SpotifyClientID it also doesn't manage. + logUpdate := map[string]bool{"redact": true, "log_body": true, "record": false} + logBody, err := json.Marshal(logUpdate) + if err != nil { + t.Fatalf("Failed to marshal logging update: %v", err) + } + + res, err = http.Post(ts.URL+"/setup/logging-settings", "application/json", bytes.NewBuffer(logBody)) + if err != nil { + t.Fatal(err) + } + res.Body.Close() + + if res.StatusCode != http.StatusOK { + t.Fatalf("POST /setup/logging-settings: expected status OK, got %v", res.Status) + } + + persisted, err = ds.GetSettings() + if err != nil { + t.Fatalf("Failed to reload settings: %v", err) + } + if !persisted.TrustForwardedHeaders { + t.Errorf("POST /setup/logging-settings dropped TrustForwardedHeaders: %+v", persisted) + } + if len(persisted.TrustedProxyCIDRs) != 1 || persisted.TrustedProxyCIDRs[0] != "10.42.0.0/16" { + t.Errorf("POST /setup/logging-settings dropped TrustedProxyCIDRs: %+v", persisted) + } + if persisted.SpotifyClientID != "seeded-spotify-client-id" { + t.Errorf("POST /setup/logging-settings dropped SpotifyClientID: %+v", persisted) + } +} + func TestMigrationAndCA(t *testing.T) { tempDir, err := os.MkdirTemp("", "handlers-test") if err != nil {