diff --git a/internal/backup/backup_test.go b/internal/backup/backup_test.go index 707a6e3c..f2823811 100644 --- a/internal/backup/backup_test.go +++ b/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() diff --git a/internal/gui/settings_keep_test.go b/internal/gui/settings_keep_test.go index 00b0a700..35bc2fd7 100644 --- a/internal/gui/settings_keep_test.go +++ b/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) { diff --git a/internal/settings/save_concurrent_test.go b/internal/settings/save_concurrent_test.go new file mode 100644 index 00000000..1bc0a40c --- /dev/null +++ b/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) + } + } +} diff --git a/internal/settings/save_test.go b/internal/settings/save_test.go new file mode 100644 index 00000000..894805e5 --- /dev/null +++ b/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) + } +} diff --git a/internal/settings/save_windows_test.go b/internal/settings/save_windows_test.go new file mode 100644 index 00000000..a504ede4 --- /dev/null +++ b/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) + } +} diff --git a/internal/settings/settings.go b/internal/settings/settings.go index fd8fbc6c..d47b70ec 100644 --- a/internal/settings/settings.go +++ b/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 { diff --git a/update_auto_cli_test.go b/update_auto_cli_test.go index 749fc7c8..720cb10a 100644 --- a/update_auto_cli_test.go +++ b/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) {