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 {