settings: save atomically and preserve unreadable configuration (#621)

* fix(settings): preserve unreadable configuration

* fix(settings): make saves atomic and recover empty files

internal/backup/backup_test.go

@@ -1,6 +1,7 @@
 package backup
 
 import (
+	"bytes"
 	"encoding/json"
 	"errors"
 	"os"
@@ -17,6 +18,32 @@ import (
 	"github.com/yetone/magpie/internal/settings"
 )
 
+func TestRestoreKeepsCorruptSettings(t *testing.T) {
+	for _, tc := range []struct{ name, body string }{
+		{"truncated", `{"theme":"dark","proxy":"direct","githubToken":"SYNTHETIC_PRIVATE_TOKEN"`},
+		{"theme type", `{"theme":7,"proxy":"direct","githubToken":"SYNTHETIC_PRIVATE_TOKEN"}`},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			home(t)
+			if err := os.MkdirAll(settings.Dir(), 0o755); err != nil {
+				t.Fatal(err)
+			}
+			original := []byte(tc.body)
+			if err := os.WriteFile(settings.Path(), original, 0o600); err != nil {
+				t.Fatal(err)
+			}
+			b := Bundle{Version: 1, Settings: &settings.Settings{Theme: "light"}}
+			result, err := Restore(b, Parts{Settings: true})
+			if err == nil || result.Settings {
+				t.Errorf("restore accepted corrupt local settings: %+v, %v", result, err)
+			}
+			if after, err := os.ReadFile(settings.Path()); err != nil || !bytes.Equal(after, original) {
+				t.Errorf("restore overwrote the corrupt settings: %v", err)
+			}
+		})
+	}
+}
+
 // home gives the test a machine of its own: no agents, no magpie files.
 func home(t *testing.T) {
 	t.Helper()

internal/gui/settings_keep_test.go

@@ -1,6 +1,7 @@
 package gui
 
 import (
+	"bytes"
 	"net/http"
 	"net/http/httptest"
 	"os"
@@ -10,10 +11,50 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/yetone/magpie/internal/agentenv"
 	"github.com/yetone/magpie/internal/redact"
 	"github.com/yetone/magpie/internal/settings"
 )
 
+func TestSettingsSaveKeepsCorruptFile(t *testing.T) {
+	for _, tc := range []struct{ path, body string }{
+		{"/api/settings", `{"theme":"light"}`},
+		{"/api/settings/quota-left", `{"on":true}`},
+	} {
+		t.Run(tc.path, func(t *testing.T) {
+			h := t.TempDir()
+			t.Setenv("HOME", h)
+			t.Setenv("USERPROFILE", h)
+			t.Setenv("XDG_CONFIG_HOME", filepath.Join(h, ".config"))
+			t.Setenv("XDG_CACHE_HOME", filepath.Join(h, ".cache"))
+			t.Setenv("APPDATA", filepath.Join(h, "AppData", "Roaming"))
+			t.Setenv("LOCALAPPDATA", filepath.Join(h, "AppData", "Local"))
+			for _, v := range agentenv.Vars {
+				t.Setenv(v, "")
+			}
+			if err := os.MkdirAll(settings.Dir(), 0o755); err != nil {
+				t.Fatal(err)
+			}
+			const token = "SYNTHETIC_PRIVATE_TOKEN"
+			original := []byte(`{"theme":"dark","githubToken":"` + token + `"`)
+			if err := os.WriteFile(settings.Path(), original, 0o600); err != nil {
+				t.Fatal(err)
+			}
+			rec := httptest.NewRecorder()
+			Handler(nil, nil).ServeHTTP(rec, httptest.NewRequest("POST", tc.path, strings.NewReader(tc.body)))
+			if rec.Code != http.StatusBadRequest {
+				t.Errorf("save returned %d, want a settings error", rec.Code)
+			}
+			if strings.Contains(rec.Body.String(), token) {
+				t.Error("the error response exposed the settings contents")
+			}
+			if after, err := os.ReadFile(settings.Path()); err != nil || !bytes.Equal(after, original) {
+				t.Errorf("the request overwrote the corrupt settings: %v", err)
+			}
+		})
+	}
+}
+
 // What the models' page keeps in settings — names, levels, whether a model
 // takes images — outlives a save of the Settings page, which never sends it.
 func TestSettingsSaveKeepsModelChoices(t *testing.T) {

internal/settings/save_concurrent_test.go

@@ -0,0 +1,103 @@
+package settings
+
+import (
+	"encoding/json"
+	"os"
+	"path/filepath"
+	"sync"
+	"sync/atomic"
+	"testing"
+
+	"github.com/yetone/magpie/internal/steady"
+)
+
+func TestSaveConcurrent(t *testing.T) {
+	testSaveConcurrent(t, false)
+}
+
+func TestSaveConcurrentHardLink(t *testing.T) {
+	testSaveConcurrent(t, true)
+}
+
+func testSaveConcurrent(t *testing.T, linked bool) {
+	t.Helper()
+	t.Setenv("XDG_CONFIG_HOME", t.TempDir())
+	const token = "SYNTHETIC_PRIVATE_TOKEN"
+	if err := Save(Settings{Theme: "dark", Lang: "zh", Proxy: "direct", GitHubToken: token}); err != nil {
+		t.Fatal(err)
+	}
+	other := filepath.Join(Dir(), "linked-settings.json")
+	if linked {
+		if err := os.Link(Path(), other); err != nil {
+			t.Skip("no hard links here:", err)
+		}
+	}
+	var saveErrors, lostFields, readErrors, reads atomic.Int64
+	check := func(s Settings) {
+		if s.Theme != "dark" || s.Lang != "zh" || s.Proxy != "direct" || s.GitHubToken != token {
+			lostFields.Add(1)
+		}
+	}
+	stop, readDone := make(chan struct{}), make(chan struct{})
+	if !linked {
+		// Read outside Load's synchronization too: a normal file must be
+		// published as a complete snapshot, not merely hidden by a lock.
+		go func() {
+			defer close(readDone)
+			for {
+				select {
+				case <-stop:
+					return
+				default:
+				}
+				b, err := steady.ReadFile(Path())
+				var s Settings
+				if err != nil || json.Unmarshal(b, &s) != nil {
+					readErrors.Add(1)
+				} else {
+					check(s)
+				}
+				reads.Add(1)
+			}
+		}()
+	} else {
+		close(readDone)
+	}
+	start := make(chan struct{})
+	var wg sync.WaitGroup
+	for range 8 {
+		wg.Go(func() {
+			<-start
+			for i := range 300 {
+				s := Load()
+				check(s)
+				s.NoAutoUpdate = i%2 == 0
+				if err := Save(s); err != nil {
+					saveErrors.Add(1)
+				}
+			}
+		})
+	}
+	close(start)
+	wg.Wait()
+	close(stop)
+	<-readDone
+	check(Load())
+	if saveErrors.Load() != 0 || lostFields.Load() != 0 || readErrors.Load() != 0 {
+		t.Errorf("concurrent saves: %d save errors, %d lost-field reads, %d incomplete-file reads",
+			saveErrors.Load(), lostFields.Load(), readErrors.Load())
+	}
+	if !linked && reads.Load() == 0 {
+		t.Fatal("the independent reader did not observe any saves")
+	}
+	if linked {
+		a, err := os.Stat(Path())
+		if err != nil {
+			t.Fatal(err)
+		}
+		b, err := os.Stat(other)
+		if err != nil || !os.SameFile(a, b) {
+			t.Fatalf("saving split the hard link: %v", err)
+		}
+	}
+}

internal/settings/save_test.go

@@ -0,0 +1,147 @@
+package settings
+
+import (
+	"bytes"
+	"errors"
+	"os"
+	"runtime"
+	"strings"
+	"testing"
+)
+
+// A default or partially decoded display must not become the new file.
+func TestSaveKeepsCorruptSettings(t *testing.T) {
+	const token = "SYNTHETIC_PRIVATE_TOKEN"
+	for _, tc := range []struct{ name, body string }{
+		{"truncated", `{"theme":"dark","githubToken":"` + token + `"`},
+		{"theme type", `{"theme":7,"githubToken":"` + token + `"}`},
+		{"proxy type", `{"proxy":7,"githubToken":"` + token + `"}`},
+		{"nested type", `{"otel":{"headers":7},"githubToken":"` + token + `"}`},
+		{"BOM truncated", "\ufeff" + `{"theme":"dark","githubToken":"` + token + `"`},
+		{"BOM type", "\ufeff" + `{"theme":7,"githubToken":"` + token + `"}`},
+		{"non-JSON whitespace", "\u00a0" + `{"theme":"dark","githubToken":"` + token + `"}`},
+		{"array", `[]`},
+		{"trailing JSON", `{"theme":"dark"} {}`},
+	} {
+		for _, loaded := range []bool{false, true} {
+			kind := "fresh"
+			if loaded {
+				kind = "loaded"
+			}
+			t.Run(tc.name+"/"+kind, func(t *testing.T) {
+				t.Setenv("XDG_CONFIG_HOME", t.TempDir())
+				if err := os.MkdirAll(Dir(), 0o755); err != nil {
+					t.Fatal(err)
+				}
+				original := []byte(tc.body)
+				if err := os.WriteFile(Path(), original, 0o640); err != nil {
+					t.Fatal(err)
+				}
+				before, err := os.Stat(Path())
+				if err != nil {
+					t.Fatal(err)
+				}
+				s := Settings{Theme: "light"}
+				if loaded {
+					s = Load()
+				}
+				s.NoAutoUpdate = true
+				if err := Save(s); err == nil {
+					t.Error("corrupt settings were accepted")
+				} else if !strings.Contains(err.Error(), Path()) || strings.Contains(err.Error(), token) {
+					t.Errorf("error must name the file without exposing its contents: %v", err)
+				}
+				after, err := os.ReadFile(Path())
+				if err != nil {
+					t.Fatal(err)
+				}
+				if !bytes.Equal(after, original) {
+					t.Error("the original settings were overwritten")
+				}
+				fi, err := os.Stat(Path())
+				if err != nil {
+					t.Fatal(err)
+				}
+				if fi.Mode().Perm() != before.Mode().Perm() {
+					t.Errorf("refused save changed permissions: %v, was %v", fi.Mode(), before.Mode())
+				}
+			})
+		}
+	}
+}
+
+func TestSaveRecoversEmptySettings(t *testing.T) {
+	for _, body := range []string{"", " \t\r\n", "\ufeff", "\ufeff \t\r\n"} {
+		t.Run(body, func(t *testing.T) {
+			t.Setenv("XDG_CONFIG_HOME", t.TempDir())
+			if err := os.MkdirAll(Dir(), 0o755); err != nil {
+				t.Fatal(err)
+			}
+			if err := os.WriteFile(Path(), []byte(body), 0o600); err != nil {
+				t.Fatal(err)
+			}
+			s := Load()
+			s.Theme, s.NoAutoUpdate = "light", true
+			if err := Save(s); err != nil {
+				t.Fatal(err)
+			}
+			if got := Load(); got.Theme != "light" || !got.NoAutoUpdate {
+				t.Fatal("the preference change was not saved to the empty file")
+			}
+		})
+	}
+}
+
+func TestSettingsBOM(t *testing.T) {
+	t.Setenv("XDG_CONFIG_HOME", t.TempDir())
+	if err := os.MkdirAll(Dir(), 0o755); err != nil {
+		t.Fatal(err)
+	}
+	const token = "SYNTHETIC_PRIVATE_TOKEN"
+	body := "\ufeff" + ` {"theme":"dark","proxy":"direct","lang":"zh","githubToken":"` + token + `"}`
+	if err := os.WriteFile(Path(), []byte(body), 0o600); err != nil {
+		t.Fatal(err)
+	}
+	s := Load()
+	if s.Theme != "dark" || s.Proxy != "direct" || s.Lang != "zh" || s.GitHubToken != token {
+		t.Error("loading a BOM file lost existing fields")
+	}
+	s.NoAutoUpdate = true
+	if err := Save(s); err != nil {
+		t.Fatal(err)
+	}
+	got := Load()
+	if !got.NoAutoUpdate || got.Theme != "dark" || got.Proxy != "direct" || got.Lang != "zh" || got.GitHubToken != token {
+		t.Fatal("editing a BOM file lost existing fields")
+	}
+}
+
+func TestSaveKeepsUnreadableSettings(t *testing.T) {
+	if runtime.GOOS == "windows" {
+		t.Skip("Windows read restrictions are covered by TestSaveKeepsReadBlockedSettings")
+	}
+	t.Setenv("XDG_CONFIG_HOME", t.TempDir())
+	if err := os.MkdirAll(Dir(), 0o755); err != nil {
+		t.Fatal(err)
+	}
+	original := []byte(`{"theme":"dark","githubToken":"SYNTHETIC_PRIVATE_TOKEN"}`)
+	if err := os.WriteFile(Path(), original, 0o200); err != nil {
+		t.Fatal(err)
+	}
+	if _, err := os.ReadFile(Path()); !errors.Is(err, os.ErrPermission) {
+		t.Skipf("this process can read a write-only file: %v", err)
+	}
+	err := Save(Settings{Theme: "light"})
+	if !errors.Is(err, os.ErrPermission) {
+		t.Errorf("Save returned %v, want the read permission error", err)
+	}
+	if fi, err := os.Stat(Path()); err != nil || fi.Mode().Perm() != 0o200 {
+		t.Errorf("refused save changed the write-only file's mode: %v, %v", fi, err)
+	}
+	if err := os.Chmod(Path(), 0o600); err != nil {
+		t.Fatal(err)
+	}
+	if after, err := os.ReadFile(Path()); err != nil || !bytes.Equal(after, original) {
+		t.Errorf("the unreadable settings were overwritten: %v", err)
+	}
+}

internal/settings/save_windows_test.go

@@ -0,0 +1,54 @@
+//go:build windows
+
+package settings
+
+import (
+	"bytes"
+	"errors"
+	"os"
+	"testing"
+
+	"golang.org/x/sys/windows"
+)
+
+func TestSaveKeepsReadBlockedSettings(t *testing.T) {
+	t.Setenv("XDG_CONFIG_HOME", t.TempDir())
+	if err := os.MkdirAll(Dir(), 0o755); err != nil {
+		t.Fatal(err)
+	}
+	original := []byte(`{"theme":"dark","githubToken":"SYNTHETIC_PRIVATE_TOKEN"}`)
+	if err := os.WriteFile(Path(), original, 0o600); err != nil {
+		t.Fatal(err)
+	}
+	name, err := windows.UTF16PtrFromString(Path())
+	if err != nil {
+		t.Fatal(err)
+	}
+	// Writes remain possible, but a reader cannot open this existing file.
+	h, err := windows.CreateFile(name, windows.GENERIC_WRITE,
+		windows.FILE_SHARE_WRITE|windows.FILE_SHARE_DELETE, nil,
+		windows.OPEN_EXISTING, windows.FILE_ATTRIBUTE_NORMAL, 0)
+	if err != nil {
+		t.Fatal(err)
+	}
+	closed := false
+	t.Cleanup(func() {
+		if !closed {
+			windows.CloseHandle(h)
+		}
+	})
+	if _, err := os.ReadFile(Path()); !errors.Is(err, windows.ERROR_SHARING_VIOLATION) {
+		t.Fatalf("the read restriction was not established: %v", err)
+	}
+	saved := Save(Settings{Theme: "light"})
+	if err := windows.CloseHandle(h); err != nil {
+		t.Fatal(err)
+	}
+	closed = true
+	if !errors.Is(saved, windows.ERROR_SHARING_VIOLATION) {
+		t.Errorf("Save returned %v, want the read-sharing error", saved)
+	}
+	if after, err := os.ReadFile(Path()); err != nil || !bytes.Equal(after, original) {
+		t.Errorf("the read-blocked settings were overwritten: %v", err)
+	}
+}

internal/settings/settings.go

@@ -8,6 +8,7 @@
 package settings
 
 import (
+	"bytes"
 	"encoding/json"
 	"fmt"
 	"math"
@@ -18,11 +19,14 @@ import (
 	"regexp"
 	"slices"
 	"strings"
+	"sync"
 	"time"
 
 	"github.com/yetone/magpie/internal/appdir"
 	"github.com/yetone/magpie/internal/catalog"
+	"github.com/yetone/magpie/internal/edit"
 	"github.com/yetone/magpie/internal/redact"
+	"github.com/yetone/magpie/internal/steady"
 )
 
 // Settings is what the user chose. "" and "system" both mean "follow the OS".
@@ -514,11 +518,17 @@ func Dir() string { return appdir.Config() }
 // Portable is the data folder of a portable magpie, or "" when installed.
 func Portable() string { return appdir.Portable() }
 
+// Serialize reads with saves too: the editor preserves hard links by
+// writing them in place rather than replacing their inode.
+var fileMu sync.RWMutex
+
 // Load reads the settings; anything missing or unreadable is the default.
 func Load() Settings {
+	fileMu.RLock()
+	defer fileMu.RUnlock()
 	var s Settings
-	if b, err := os.ReadFile(Path()); err == nil {
-		_ = json.Unmarshal(b, &s)
+	if b, err := steady.ReadFile(Path()); err == nil {
+		_ = json.Unmarshal(bytes.TrimPrefix(b, []byte("\xef\xbb\xbf")), &s)
 	}
 	return s.normal()
 }
@@ -545,6 +555,8 @@ func CheckProxy(p string) error {
 
 // Save validates and writes the settings.
 func Save(s Settings) error {
+	fileMu.Lock()
+	defer fileMu.Unlock()
 	s = s.normal()
 	if !slices.Contains(Themes, s.Theme) {
 		return fmt.Errorf("theme must be one of %v, not %q", Themes, s.Theme)
@@ -619,6 +631,19 @@ func Save(s Settings) error {
 	if len(s.TrayUsages) > 0 {
 		s.TrayUsage = s.TrayUsages[0]
 	}
+	// Load may have returned defaults or only part of an unreadable file.
+	// Do not replace it, including its permissions, with those values.
+	if b, err := steady.ReadFile(Path()); err == nil {
+		b = bytes.TrimPrefix(b, []byte("\xef\xbb\xbf"))
+		if len(bytes.TrimSpace(b)) != 0 {
+			var stored Settings
+			if err := json.Unmarshal(b, &stored); err != nil {
+				return fmt.Errorf("could not read settings at %s; repair or move that file aside before saving: %w", Path(), err)
+			}
+		}
+	} else if !os.IsNotExist(err) {
+		return fmt.Errorf("could not read settings at %s: %w", Path(), err)
+	}
 	if err := os.MkdirAll(Dir(), 0o755); err != nil {
 		return err
 	}
@@ -632,7 +657,16 @@ func Save(s Settings) error {
 			return err
 		}
 	}
-	return os.WriteFile(Path(), append(b, '\n'), 0o600)
+	// Open without truncating: read-only settings must still reject saves,
+	// and a new file must have the private mode WriteAtomic will preserve.
+	f, err := os.OpenFile(Path(), os.O_CREATE|os.O_WRONLY, 0o600)
+	if err != nil {
+		return err
+	}
+	if err := f.Close(); err != nil {
+		return err
+	}
+	return edit.WriteAtomic(Path(), append(b, '\n'))
 }
 
 func (s Settings) normal() Settings {

update_auto_cli_test.go

@@ -1,11 +1,76 @@
 package main
 
 import (
+	"bytes"
+	"os"
 	"testing"
 
 	"github.com/yetone/magpie/internal/settings"
 )
 
+func TestUpdateAutoKeepsCorruptSettings(t *testing.T) {
+	for _, tc := range []struct{ name, body string }{
+		{"truncated", `{"theme":"dark","githubToken":"SYNTHETIC_PRIVATE_TOKEN"`},
+		{"theme type", `{"theme":7,"githubToken":"SYNTHETIC_PRIVATE_TOKEN"}`},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			groupsHome(t)
+			if err := os.MkdirAll(settings.Dir(), 0o755); err != nil {
+				t.Fatal(err)
+			}
+			original := []byte(tc.body)
+			if err := os.WriteFile(settings.Path(), original, 0o600); err != nil {
+				t.Fatal(err)
+			}
+			if err := updateCmd([]string{"update", "auto", "off"}); err == nil {
+				t.Error("the command reported success with corrupt settings")
+			}
+			if after, err := os.ReadFile(settings.Path()); err != nil || !bytes.Equal(after, original) {
+				t.Errorf("the command overwrote the corrupt settings: %v", err)
+			}
+		})
+	}
+}
+
+func TestUpdateAutoRecoversEmptySettings(t *testing.T) {
+	for _, body := range []string{"", " \t\r\n", "\ufeff \t\r\n"} {
+		t.Run(body, func(t *testing.T) {
+			groupsHome(t)
+			if err := os.MkdirAll(settings.Dir(), 0o755); err != nil {
+				t.Fatal(err)
+			}
+			if err := os.WriteFile(settings.Path(), []byte(body), 0o600); err != nil {
+				t.Fatal(err)
+			}
+			if err := updateCmd([]string{"update", "auto", "off"}); err != nil {
+				t.Fatal(err)
+			}
+			if !settings.Load().NoAutoUpdate {
+				t.Fatal("the update preference was not saved")
+			}
+		})
+	}
+}
+
+func TestUpdateAutoWithBOMSettings(t *testing.T) {
+	groupsHome(t)
+	if err := os.MkdirAll(settings.Dir(), 0o755); err != nil {
+		t.Fatal(err)
+	}
+	const token = "SYNTHETIC_PRIVATE_TOKEN"
+	body := "\ufeff" + `{"theme":"dark","proxy":"direct","lang":"zh","githubToken":"` + token + `"}`
+	if err := os.WriteFile(settings.Path(), []byte(body), 0o600); err != nil {
+		t.Fatal(err)
+	}
+	if err := updateCmd([]string{"update", "auto", "off"}); err != nil {
+		t.Fatal(err)
+	}
+	got := settings.Load()
+	if !got.NoAutoUpdate || got.Theme != "dark" || got.Proxy != "direct" || got.Lang != "zh" || got.GitHubToken != token {
+		t.Fatal("the update command lost fields from the BOM file")
+	}
+}
+
 // magpie update auto turns the app's own update checks off and on and sets
 // how often they run, from the presets only (#472).
 func TestUpdateAutoCmd(t *testing.T) {