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 <noreply@anthropic.com>

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 {

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 {