diff --git a/internal/app/app.go b/internal/app/app.go index 8df8445..520213f 100644 --- a/internal/app/app.go +++ b/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 } } diff --git a/internal/app/storage_test.go b/internal/app/storage_test.go index 5d903fd..bc0b512 100644 --- a/internal/app/storage_test.go +++ b/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) diff --git a/internal/profile/profile.go b/internal/profile/profile.go index 44d85dc..5af630c 100644 --- a/internal/profile/profile.go +++ b/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) { diff --git a/internal/profile/store.go b/internal/profile/store.go index 80efd9c..68b304f 100644 --- a/internal/profile/store.go +++ b/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 } diff --git a/internal/profile/store_test.go b/internal/profile/store_test.go new file mode 100644 index 0000000..9274b08 --- /dev/null +++ b/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) + } +} diff --git a/internal/tui/model.go b/internal/tui/model.go index 7512af7..856fb09 100644 --- a/internal/tui/model.go +++ b/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 } diff --git a/internal/tui/model_test.go b/internal/tui/model_test.go index a55cd70..5ee51f4 100644 --- a/internal/tui/model_test.go +++ b/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)