fix: serialize persistent profile mutations

internal/app/app.go

@@ -59,99 +59,89 @@ func (a *Application) Snapshot() Snapshot {
 
 func (a *Application) Dispatch(intent Intent) Result {
 	var notice string
-	var err error
-	switch intent.Kind {
-	case CreateProfile:
-		notice, err = a.create(intent.Profile)
-	case ActivateProfile:
-		notice, err = a.activate(intent.Name)
-	case UpdateProfile:
-		notice, err = a.update(intent.Name, intent.Profile)
-	case DeleteProfile:
-		notice, err = a.delete(intent.Name, intent.Confirm)
-	default:
-		err = errors.New("unsupported Profile action")
+	next, err := a.store.Mutate(func(current profile.State) (profile.State, error) {
+		var mutationErr error
+		switch intent.Kind {
+		case CreateProfile:
+			current, notice, mutationErr = create(current, intent.Profile)
+		case ActivateProfile:
+			current, notice, mutationErr = activate(current, intent.Name)
+		case UpdateProfile:
+			current, notice, mutationErr = update(current, intent.Name, intent.Profile)
+		case DeleteProfile:
+			current, notice, mutationErr = deleteProfile(current, intent.Name, intent.Confirm)
+		default:
+			mutationErr = errors.New("unsupported Profile action")
+		}
+		return current, mutationErr
+	})
+	if err == nil {
+		a.state = next
 	}
 	return Result{Snapshot: a.Snapshot(), Notice: notice, Err: err}
 }
 
-func (a *Application) create(input profile.Profile) (string, error) {
+func create(state profile.State, input profile.Profile) (profile.State, string, error) {
 	p, err := profile.ValidateAndNormalize(input)
 	if err != nil {
-		return "", err
+		return state, "", err
 	}
-	if a.index(p.Name) >= 0 {
-		return "", fmt.Errorf("a Profile named %q already exists", p.Name)
+	if index(state, p.Name) >= 0 {
+		return state, "", fmt.Errorf("a Profile named %q already exists", p.Name)
 	}
-	next := cloneState(a.state)
+	next := cloneState(state)
 	next.Profiles = append(next.Profiles, p)
-	if err := a.store.Save(next); err != nil {
-		return "", err
-	}
-	a.state = next
-	return fmt.Sprintf("Created Profile %q", p.Name), nil
+	return next, fmt.Sprintf("Created Profile %q", p.Name), nil
 }
 
-func (a *Application) activate(name string) (string, error) {
-	if a.index(name) < 0 {
-		return "", fmt.Errorf("Profile %q does not exist", name)
+func activate(state profile.State, name string) (profile.State, string, error) {
+	if index(state, name) < 0 {
+		return state, "", fmt.Errorf("Profile %q does not exist", name)
 	}
-	next := cloneState(a.state)
+	next := cloneState(state)
 	next.ActiveProfile = name
-	if err := a.store.Save(next); err != nil {
-		return "", err
-	}
-	a.state = next
-	return fmt.Sprintf("Activated Profile %q", name), nil
+	return next, fmt.Sprintf("Activated Profile %q", name), nil
 }
 
-func (a *Application) update(originalName string, input profile.Profile) (string, error) {
-	index := a.index(originalName)
-	if index < 0 {
-		return "", fmt.Errorf("Profile %q does not exist", originalName)
+func update(state profile.State, originalName string, input profile.Profile) (profile.State, string, error) {
+	profileIndex := index(state, originalName)
+	if profileIndex < 0 {
+		return state, "", fmt.Errorf("Profile %q does not exist", originalName)
 	}
 	p, err := profile.ValidateAndNormalize(input)
 	if err != nil {
-		return "", err
+		return state, "", err
 	}
-	if other := a.index(p.Name); other >= 0 && other != index {
-		return "", fmt.Errorf("a Profile named %q already exists", p.Name)
+	if other := index(state, p.Name); other >= 0 && other != profileIndex {
+		return state, "", fmt.Errorf("a Profile named %q already exists", p.Name)
 	}
-	next := cloneState(a.state)
-	next.Profiles[index] = p
+	next := cloneState(state)
+	next.Profiles[profileIndex] = p
 	if next.ActiveProfile == originalName {
 		next.ActiveProfile = p.Name
 	}
-	if err := a.store.Save(next); err != nil {
-		return "", err
-	}
-	a.state = next
-	return fmt.Sprintf("Updated Profile %q", p.Name), nil
+	return next, fmt.Sprintf("Updated Profile %q", p.Name), nil
 }
 
-func (a *Application) delete(name string, confirm bool) (string, error) {
-	index := a.index(name)
-	if index < 0 {
-		return "", fmt.Errorf("Profile %q does not exist", name)
+func deleteProfile(state profile.State, name string, confirm bool) (profile.State, string, error) {
+	profileIndex := index(state, name)
+	if profileIndex < 0 {
+		return state, "", fmt.Errorf("Profile %q does not exist", name)
 	}
-	if len(a.state.Profiles) == 1 && !confirm {
-		return "", errors.New("deleting the final Profile requires confirmation")
+	if len(state.Profiles) == 1 && !confirm {
+		return state, "", errors.New("deleting the final Profile requires confirmation")
 	}
-	next := cloneState(a.state)
-	next.Profiles = append(next.Profiles[:index], next.Profiles[index+1:]...)
+	next := cloneState(state)
+	next.Profiles = append(next.Profiles[:profileIndex], next.Profiles[profileIndex+1:]...)
 	if next.ActiveProfile == name {
 		next.ActiveProfile = ""
 	}
-	if err := a.store.Save(next); err != nil {
-		return "", err
-	}
-	a.state = next
-	return fmt.Sprintf("Deleted Profile %q", name), nil
+	return next, fmt.Sprintf("Deleted Profile %q", name), nil
 }
 
-func (a *Application) index(name string) int {
-	for i := range a.state.Profiles {
-		if a.state.Profiles[i].Name == name {
+func index(state profile.State, name string) int {
+	for i := range state.Profiles {
+		if state.Profiles[i].Name == name {
 			return i
 		}
 	}

internal/app/storage_test.go

@@ -5,6 +5,7 @@ import (
 	"os"
 	"path/filepath"
 	"strings"
+	"sync"
 	"testing"
 
 	"proxyctl/internal/app"
@@ -70,6 +71,64 @@ func TestProfileConfigurationUsesIsolatedXDGPathOwnerOnlyAndAtomicReplacement(t
 	}
 }
 
+func TestConcurrentApplicationsMutateLatestStateWithoutLostUpdates(t *testing.T) {
+	path := filepath.Join(t.TempDir(), "proxyctl", "profiles.json")
+	first, err := app.New(profile.NewFileStore(path))
+	if err != nil {
+		t.Fatal(err)
+	}
+	second, err := app.New(profile.NewFileStore(path))
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	start := make(chan struct{})
+	results := make(chan app.Result, 2)
+	var workers sync.WaitGroup
+	for application, name := range map[*app.Application]string{first: "home", second: "office"} {
+		workers.Add(1)
+		go func() {
+			defer workers.Done()
+			<-start
+			results <- application.Dispatch(app.Intent{Kind: app.CreateProfile, Profile: profile.Profile{
+				Name: name, Mode: profile.SingleMode, SingleURL: "http://proxy.example:8080",
+			}})
+		}()
+	}
+	close(start)
+	workers.Wait()
+	close(results)
+	for result := range results {
+		if result.Err != nil {
+			t.Fatalf("concurrent mutation: %v", result.Err)
+		}
+	}
+
+	reloaded, err := app.New(profile.NewFileStore(path))
+	if err != nil {
+		t.Fatal(err)
+	}
+	if got := len(reloaded.Snapshot().Profiles); got != 2 {
+		t.Fatalf("persisted Profiles = %d, want both concurrent updates", got)
+	}
+	assertMode(t, path+".lock", 0o600)
+}
+
+func TestLoadSecuresExistingSecretBearingConfiguration(t *testing.T) {
+	path := filepath.Join(t.TempDir(), "proxyctl", "profiles.json")
+	if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil {
+		t.Fatal(err)
+	}
+	contents := `{"profiles":[{"name":"private","mode":"single","singleUrl":"http://alice:secret@proxy.example:8080"}]}`
+	if err := os.WriteFile(path, []byte(contents), 0o644); err != nil {
+		t.Fatal(err)
+	}
+	if _, err := app.New(profile.NewFileStore(path)); err != nil {
+		t.Fatalf("load existing configuration: %v", err)
+	}
+	assertMode(t, path, 0o600)
+}
+
 func assertMode(t *testing.T, path string, want os.FileMode) {
 	t.Helper()
 	info, err := os.Stat(path)

internal/profile/profile.go

@@ -34,7 +34,7 @@ type State struct {
 
 type Store interface {
 	Load() (State, error)
-	Save(State) error
+	Mutate(func(State) (State, error)) (State, error)
 }
 
 func ValidateAndNormalize(input Profile) (Profile, error) {

internal/profile/store.go

@@ -4,9 +4,13 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"io"
 	"io/fs"
 	"os"
+	"os/user"
 	"path/filepath"
+	"strings"
+	"syscall"
 )
 
 type FileStore struct {
@@ -17,22 +21,137 @@ func NewFileStore(path string) *FileStore {
 	return &FileStore{path: path}
 }
 
-func ConfigPath() (string, error) {
-	configDir, err := os.UserConfigDir()
+type UserIdentity struct {
+	Username string
+	HomeDir  string
+}
+
+type IdentityEnvironment interface {
+	CurrentUser() (UserIdentity, error)
+	LookupUser(string) (UserIdentity, error)
+	LookupEnv(string) (string, bool)
+}
+
+type systemIdentityEnvironment struct{}
+
+func (systemIdentityEnvironment) CurrentUser() (UserIdentity, error) {
+	value, err := user.Current()
 	if err != nil {
-		return "", fmt.Errorf("locate user configuration directory: %w", err)
+		return UserIdentity{}, err
+	}
+	return UserIdentity{Username: value.Username, HomeDir: value.HomeDir}, nil
+}
+
+func (systemIdentityEnvironment) LookupUser(name string) (UserIdentity, error) {
+	value, err := user.Lookup(name)
+	if err != nil {
+		return UserIdentity{}, err
+	}
+	return UserIdentity{Username: value.Username, HomeDir: value.HomeDir}, nil
+}
+
+func (systemIdentityEnvironment) LookupEnv(name string) (string, bool) {
+	return os.LookupEnv(name)
+}
+
+func ConfigPath() (string, error) {
+	return ConfigPathWith(systemIdentityEnvironment{})
+}
+
+func ConfigPathWith(environment IdentityEnvironment) (string, error) {
+	owner, err := environment.CurrentUser()
+	if err != nil {
+		return "", fmt.Errorf("identify configuration owner: %w", err)
+	}
+	invokingUser := false
+	if name, ok := environment.LookupEnv("SUDO_USER"); ok && name != "" && name != owner.Username {
+		owner, err = environment.LookupUser(name)
+		if err != nil {
+			return "", fmt.Errorf("identify invoking user %q: %w", name, err)
+		}
+		invokingUser = true
+	}
+	if owner.HomeDir == "" {
+		return "", errors.New("configuration owner has no home directory")
+	}
+
+	configDir := ""
+	if value, ok := environment.LookupEnv("XDG_CONFIG_HOME"); ok && filepath.IsAbs(value) {
+		if !invokingUser || pathWithin(value, owner.HomeDir) {
+			configDir = value
+		}
+	}
+	if configDir == "" {
+		configDir = filepath.Join(owner.HomeDir, ".config")
 	}
 	return filepath.Join(configDir, "proxyctl", "profiles.json"), nil
 }
 
+func pathWithin(path, parent string) bool {
+	relative, err := filepath.Rel(parent, path)
+	return err == nil && relative != ".." && !strings.HasPrefix(relative, ".."+string(filepath.Separator))
+}
+
 func (s *FileStore) Load() (State, error) {
-	data, err := os.ReadFile(s.path)
+	return s.load()
+}
+
+func (s *FileStore) Mutate(change func(State) (State, error)) (State, error) {
+	if err := s.secureDirectory(); err != nil {
+		return State{}, err
+	}
+	lock, err := os.OpenFile(s.path+".lock", os.O_CREATE|os.O_RDWR, 0o600)
+	if err != nil {
+		return State{}, fmt.Errorf("open Profile configuration lock: %w", err)
+	}
+	defer lock.Close()
+	if err := lock.Chmod(0o600); err != nil {
+		return State{}, fmt.Errorf("secure Profile configuration lock: %w", err)
+	}
+	if err := syscall.Flock(int(lock.Fd()), syscall.LOCK_EX); err != nil {
+		return State{}, fmt.Errorf("lock Profile configuration: %w", err)
+	}
+	defer syscall.Flock(int(lock.Fd()), syscall.LOCK_UN) //nolint:errcheck -- closing also releases the lock
+
+	current, err := s.load()
+	if err != nil {
+		return State{}, err
+	}
+	next, err := change(current)
+	if err != nil {
+		return current, err
+	}
+	if err := s.save(next); err != nil {
+		return current, err
+	}
+	return next, nil
+}
+
+func (s *FileStore) load() (State, error) {
+	file, err := os.Open(s.path)
 	if errors.Is(err, fs.ErrNotExist) {
 		return State{}, nil
 	}
 	if err != nil {
 		return State{}, fmt.Errorf("read Profile configuration: %w", err)
 	}
+	defer file.Close()
+	info, err := file.Stat()
+	if err != nil {
+		return State{}, fmt.Errorf("inspect Profile configuration: %w", err)
+	}
+	if !info.Mode().IsRegular() {
+		return State{}, errors.New("Profile configuration is not a regular file")
+	}
+	if info.Mode().Perm() != 0o600 {
+		if err := file.Chmod(0o600); err != nil {
+			return State{}, fmt.Errorf("secure Profile configuration: %w", err)
+		}
+	}
+	data, err := io.ReadAll(file)
+	if err != nil {
+		return State{}, fmt.Errorf("read Profile configuration: %w", err)
+	}
 	var state State
 	if err := json.Unmarshal(data, &state); err != nil {
 		return State{}, fmt.Errorf("parse Profile configuration: %w", err)
@@ -40,13 +159,21 @@ func (s *FileStore) Load() (State, error) {
 	return state, nil
 }
 
-func (s *FileStore) Save(state State) error {
-	if err := os.MkdirAll(filepath.Dir(s.path), 0o700); err != nil {
+func (s *FileStore) secureDirectory() error {
+	directory := filepath.Dir(s.path)
+	if err := os.MkdirAll(directory, 0o700); err != nil {
 		return fmt.Errorf("create Profile configuration directory: %w", err)
 	}
-	if err := os.Chmod(filepath.Dir(s.path), 0o700); err != nil {
+	if err := os.Chmod(directory, 0o700); err != nil {
 		return fmt.Errorf("secure Profile configuration directory: %w", err)
 	}
+	return nil
+}
+
+func (s *FileStore) save(state State) error {
+	if err := s.secureDirectory(); err != nil {
+		return err
+	}
 	data, err := json.MarshalIndent(state, "", "  ")
 	if err != nil {
 		return fmt.Errorf("encode Profile configuration: %w", err)
@@ -73,19 +200,10 @@ func (s *FileStore) Save(state State) error {
 	if err := temp.Close(); err != nil {
 		return fmt.Errorf("close temporary Profile configuration: %w", err)
 	}
+	// Rename is the commit point. There are deliberately no fallible operations
+	// afterwards, so callers never retain stale memory after a visible commit.
 	if err := os.Rename(tempName, s.path); err != nil {
 		return fmt.Errorf("replace Profile configuration: %w", err)
 	}
-	if err := os.Chmod(s.path, 0o600); err != nil {
-		return fmt.Errorf("secure Profile configuration: %w", err)
-	}
-	dir, err := os.Open(filepath.Dir(s.path))
-	if err == nil {
-		err = dir.Sync()
-		_ = dir.Close()
-	}
-	if err != nil {
-		return fmt.Errorf("sync Profile configuration directory: %w", err)
-	}
 	return nil
 }

internal/profile/store_test.go

@@ -0,0 +1,141 @@
+package profile
+
+import (
+	"errors"
+	"os"
+	"os/exec"
+	"path/filepath"
+	"syscall"
+	"testing"
+	"time"
+)
+
+type fakeIdentityEnvironment struct {
+	current UserIdentity
+	users   map[string]UserIdentity
+	env     map[string]string
+}
+
+func (f fakeIdentityEnvironment) CurrentUser() (UserIdentity, error) { return f.current, nil }
+func (f fakeIdentityEnvironment) LookupUser(name string) (UserIdentity, error) {
+	identity, ok := f.users[name]
+	if !ok {
+		return UserIdentity{}, errors.New("not found")
+	}
+	return identity, nil
+}
+func (f fakeIdentityEnvironment) LookupEnv(name string) (string, bool) {
+	value, ok := f.env[name]
+	return value, ok
+}
+
+func TestFileStoreLockSerializesAcrossProcesses(t *testing.T) {
+	if path := os.Getenv("PROXYCTL_LOCK_HELPER_PATH"); path != "" {
+		store := NewFileStore(path)
+		_, err := store.Mutate(func(state State) (State, error) {
+			state.Profiles = append(state.Profiles, Profile{Name: "child"})
+			return state, nil
+		})
+		if err != nil {
+			t.Fatal(err)
+		}
+		return
+	}
+
+	path := filepath.Join(t.TempDir(), "proxyctl", "profiles.json")
+	store := NewFileStore(path)
+	if _, err := store.Mutate(func(state State) (State, error) { return state, nil }); err != nil {
+		t.Fatal(err)
+	}
+	lock, err := os.OpenFile(path+".lock", os.O_RDWR, 0o600)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer lock.Close()
+	if err := syscall.Flock(int(lock.Fd()), syscall.LOCK_EX); err != nil {
+		t.Fatal(err)
+	}
+
+	command := exec.Command(os.Args[0], "-test.run=^TestFileStoreLockSerializesAcrossProcesses$")
+	command.Env = append(os.Environ(), "PROXYCTL_LOCK_HELPER_PATH="+path)
+	done := make(chan error, 1)
+	go func() { done <- command.Run() }()
+	select {
+	case err := <-done:
+		t.Fatalf("child mutation did not wait for lock: %v", err)
+	case <-time.After(100 * time.Millisecond):
+	}
+	if err := syscall.Flock(int(lock.Fd()), syscall.LOCK_UN); err != nil {
+		t.Fatal(err)
+	}
+	select {
+	case err := <-done:
+		if err != nil {
+			t.Fatalf("child mutation after unlock: %v", err)
+		}
+	case <-time.After(5 * time.Second):
+		_ = command.Process.Kill()
+		t.Fatal("child mutation remained blocked after unlock")
+	}
+	state, err := store.Load()
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(state.Profiles) != 1 || state.Profiles[0].Name != "child" {
+		t.Fatalf("state after serialized child mutation = %#v", state)
+	}
+}
+
+func TestConfigPathUsesInvokingUserIdentityUnderSudo(t *testing.T) {
+	environment := fakeIdentityEnvironment{
+		current: UserIdentity{Username: "root", HomeDir: "/root"},
+		users:   map[string]UserIdentity{"alice": {Username: "alice", HomeDir: "/home/alice"}},
+		env: map[string]string{
+			"SUDO_USER":       "alice",
+			"XDG_CONFIG_HOME": "/home/alice/.xdg/config",
+		},
+	}
+	path, err := ConfigPathWith(environment)
+	if err != nil {
+		t.Fatal(err)
+	}
+	want := filepath.Join("/home/alice/.xdg/config", "proxyctl", "profiles.json")
+	if path != want {
+		t.Fatalf("configuration path = %q, want invoking user's XDG path %q", path, want)
+	}
+}
+
+func TestConfigPathDoesNotReuseRootXDGForInvokingUser(t *testing.T) {
+	environment := fakeIdentityEnvironment{
+		current: UserIdentity{Username: "root", HomeDir: "/root"},
+		users:   map[string]UserIdentity{"alice": {Username: "alice", HomeDir: "/home/alice"}},
+		env: map[string]string{
+			"SUDO_USER":       "alice",
+			"XDG_CONFIG_HOME": "/root/.config",
+		},
+	}
+	path, err := ConfigPathWith(environment)
+	if err != nil {
+		t.Fatal(err)
+	}
+	want := filepath.Join("/home/alice/.config", "proxyctl", "profiles.json")
+	if path != want {
+		t.Fatalf("configuration path = %q, want invoking user's fallback %q", path, want)
+	}
+}
+
+func TestConfigPathUsesRootForDirectRootLogin(t *testing.T) {
+	environment := fakeIdentityEnvironment{
+		current: UserIdentity{Username: "root", HomeDir: "/root"},
+		users:   map[string]UserIdentity{},
+		env:     map[string]string{"XDG_CONFIG_HOME": "/root/custom-config"},
+	}
+	path, err := ConfigPathWith(environment)
+	if err != nil {
+		t.Fatal(err)
+	}
+	want := filepath.Join("/root/custom-config", "proxyctl", "profiles.json")
+	if path != want {
+		t.Fatalf("configuration path = %q, want direct root path %q", path, want)
+	}
+}

internal/tui/model.go

@@ -141,11 +141,9 @@ func (m Model) updateForm(key tea.KeyMsg) (tea.Model, tea.Cmd) {
 	case "ctrl+t":
 		if m.form.profile.Mode == profile.SingleMode {
 			m.form.profile.Mode = profile.AdvancedMode
-			if m.form.profile.HTTPURL == "" && m.form.profile.HTTPSURL == "" && m.form.profile.AllURL == "" {
-				m.form.profile.HTTPURL = m.form.profile.SingleURL
-				m.form.profile.HTTPSURL = m.form.profile.SingleURL
-				m.form.profile.AllURL = m.form.profile.SingleURL
-			}
+			m.form.profile.HTTPURL = m.form.profile.SingleURL
+			m.form.profile.HTTPSURL = m.form.profile.SingleURL
+			m.form.profile.AllURL = m.form.profile.SingleURL
 		} else {
 			m.form.profile.Mode = profile.SingleMode
 		}

internal/tui/model_test.go

@@ -81,6 +81,33 @@ func TestProfileFormMapsKeyboardInputToCreateIntent(t *testing.T) {
 	}
 }
 
+func TestEditingSingleURLExpandsCurrentValueWhenSwitchingToAdvanced(t *testing.T) {
+	const oldURL = "http://old.example:8080"
+	const newURL = "socks5h://new.example:1080"
+	module := &recordingModule{snapshot: app.Snapshot{Profiles: []profile.Profile{{
+		Name: "office", Mode: profile.SingleMode, SingleURL: oldURL,
+		HTTPURL: oldURL, HTTPSURL: oldURL, AllURL: oldURL,
+	}}}}
+	model := NewModel(module)
+	model = updateKey(model, "p")
+	model = updateKey(model, "e")
+	model = updateKey(model, "tab")
+	for range oldURL {
+		model = updateKey(model, "backspace")
+	}
+	model = updateRunes(model, newURL)
+	model = updateKey(model, "ctrl+t")
+	model = updateKey(model, "ctrl+s")
+
+	if len(module.intents) != 1 {
+		t.Fatalf("edit form dispatched %d intents", len(module.intents))
+	}
+	got := module.intents[0].Profile
+	if got.HTTPURL != newURL || got.HTTPSURL != newURL || got.AllURL != newURL {
+		t.Fatalf("advanced URLs = %q, %q, %q; want current Single URL %q", got.HTTPURL, got.HTTPSURL, got.AllURL, newURL)
+	}
+}
+
 func TestRenderedMalformedProxyURLStillObscuresPassword(t *testing.T) {
 	module := &recordingModule{snapshot: app.Snapshot{Profiles: []profile.Profile{{
 		Name: "manual", Mode: profile.SingleMode, SingleURL: "http://alice:do-not-show@proxy.example:bad",
@@ -107,6 +134,10 @@ func updateKey(model Model, key string) Model {
 		msg = tea.KeyMsg{Type: tea.KeyTab}
 	case "ctrl+s":
 		msg = tea.KeyMsg{Type: tea.KeyCtrlS}
+	case "ctrl+t":
+		msg = tea.KeyMsg{Type: tea.KeyCtrlT}
+	case "backspace":
+		msg = tea.KeyMsg{Type: tea.KeyBackspace}
 	}
 	updated, _ := model.Update(msg)
 	return updated.(Model)