fix: harden maintenance operations

cmd/proxyctl/main.go

@@ -51,10 +51,16 @@ func (processEnvironment) LookupEnv(name string) (string, bool) { return os.Look
 func runWithEnvironment(args []string, input io.Reader, output io.Writer, environment profile.IdentityEnvironment, files shell.FileSystem, executable string) error {
 	profileConfiguration, err := profile.ResolveConfiguration(environment)
 	if err != nil {
+		if len(args) == 1 && args[0] == "doctor" {
+			fmt.Fprintf(output, "[FAIL] identity resolution — %s\n", profile.RedactText(err.Error()))
+		}
 		return err
 	}
 	shellConfiguration, err := shell.ResolveConfiguration(environment)
 	if err != nil {
+		if len(args) == 1 && args[0] == "doctor" {
+			fmt.Fprintf(output, "[FAIL] Shell Target configuration — %s\n", profile.RedactText(err.Error()))
+		}
 		return err
 	}
 	shellTarget := shell.New(shellConfiguration, files, environment)
@@ -129,6 +135,9 @@ func runMaintenanceCommand(args []string, input io.Reader, output io.Writer, sto
 		checks = append([]target.Check{profileCheck}, checks...)
 		report := app.NewMaintenance(state, targets, nil, nil).Doctor(context.Background(), checks)
 		fmt.Fprint(output, report.String())
+		if !report.Healthy() {
+			return errors.New("doctor found configuration problems")
+		}
 		return nil
 	}
 	if args[0] != "reset" {
@@ -141,11 +150,19 @@ func runMaintenanceCommand(args []string, input io.Reader, output io.Writer, sto
 		fmt.Fprintln(output, "Reset cancelled; no Targets changed.")
 		return nil
 	}
+	if mutationLock == nil {
+		return errors.New("mutation lock is not configured")
+	}
+	unlock, err := mutationLock.Lock(context.Background())
+	if err != nil {
+		return fmt.Errorf("lock proxyctl mutation: %w", err)
+	}
+	defer unlock()
 	state, err := store.Load()
 	if err != nil {
 		return err
 	}
-	result := app.NewMaintenance(state, targets, mutationLock, logger).Reset(context.Background(), true)
+	result := app.NewMaintenance(state, targets, heldMutationLock{}, logger).Reset(context.Background(), true)
 	for _, item := range result.Targets {
 		if item.Skipped {
 			fmt.Fprintf(output, "%s: skipped (%s) — %s\n", item.TargetID, item.Inspection.State, profile.RedactText(item.Inspection.Evidence))
@@ -160,6 +177,10 @@ func runMaintenanceCommand(args []string, input io.Reader, output io.Writer, sto
 	return result.Err
 }
 
+type heldMutationLock struct{}
+
+func (heldMutationLock) Lock(context.Context) (func(), error) { return func() {}, nil }
+
 func diagnosticChecks(configuration profile.Configuration) []target.Check {
 	identity := target.Check{Name: "identity resolution", OK: true, Evidence: fmt.Sprintf("configuration owner %s (uid %d, gid %d)", configuration.Owner.Username, configuration.Owner.UID, configuration.Owner.GID)}
 	ok, evidence := storagePermissions(configuration)

cmd/proxyctl/maintenance_test.go

@@ -5,6 +5,7 @@ import (
 	"context"
 	"errors"
 	"os"
+	"os/exec"
 	"path/filepath"
 	"strings"
 	"testing"
@@ -15,6 +16,16 @@ import (
 	"proxyctl/internal/target"
 )
 
+type failingIdentityEnvironment struct{}
+
+func (failingIdentityEnvironment) CurrentUser() (profile.UserIdentity, error) {
+	return profile.UserIdentity{}, errors.New("identity lookup failed")
+}
+func (failingIdentityEnvironment) LookupUser(string) (profile.UserIdentity, error) {
+	return profile.UserIdentity{}, os.ErrNotExist
+}
+func (failingIdentityEnvironment) LookupEnv(string) (string, bool) { return "", false }
+
 type processFakeTarget struct {
 	id    string
 	state target.State
@@ -66,6 +77,24 @@ func TestDoctorCommandUsesFakeTargetsAndIsolatedXDGWithoutMutation(t *testing.T)
 	}
 }
 
+func TestDoctorReportsIdentityResolutionFailureStructurally(t *testing.T) {
+	var output bytes.Buffer
+	err := runWithEnvironment([]string{"doctor"}, strings.NewReader(""), &output, failingIdentityEnvironment{}, nil, "/test/proxyctl")
+	if err == nil || !strings.Contains(output.String(), "[FAIL] identity resolution") || !strings.Contains(output.String(), "identity lookup failed") {
+		t.Fatalf("doctor identity failure err=%v output=%q", err, output.String())
+	}
+}
+
+func TestDoctorReturnsFailureWhenAnyCheckFails(t *testing.T) {
+	store := profile.NewFileStore(filepath.Join(t.TempDir(), "profiles.json"))
+	fake := &processFakeTarget{id: "fake", state: target.Drifted}
+	var output bytes.Buffer
+	err := runMaintenanceCommand([]string{"doctor"}, strings.NewReader(""), &output, store, []target.Target{fake}, nil, nil, []target.Check{{Name: "identity", OK: true, Evidence: "ok"}})
+	if err == nil || !strings.Contains(output.String(), "[FAIL]") {
+		t.Fatalf("unhealthy doctor err=%v output=%q", err, output.String())
+	}
+}
+
 func TestDoctorStoragePermissionCheckIsReadOnly(t *testing.T) {
 	path := filepath.Join(t.TempDir(), "proxyctl", "profiles.json")
 	writeConfig(t, path, `{"profiles":[]}`)
@@ -86,6 +115,59 @@ func TestDoctorStoragePermissionCheckIsReadOnly(t *testing.T) {
 	}
 }
 
+func TestMaintenanceCommandsRunThroughBuiltBinaryWithIsolatedXDG(t *testing.T) {
+	root := t.TempDir()
+	binary := filepath.Join(root, "proxyctl")
+	build := exec.Command("go", "build", "-o", binary, ".")
+	if output, err := build.CombinedOutput(); err != nil {
+		t.Fatalf("build proxyctl: %v\n%s", err, output)
+	}
+	environment := isolatedProcessEnvironment(root)
+
+	doctor := exec.Command(binary, "doctor")
+	doctor.Env = environment
+	doctorOutput, doctorErr := doctor.CombinedOutput()
+	if doctorErr == nil || !strings.Contains(string(doctorOutput), "[FAIL]") || !strings.Contains(string(doctorOutput), "Target availability") {
+		t.Fatalf("doctor process err=%v output=%q", doctorErr, doctorOutput)
+	}
+
+	cancel := exec.Command(binary, "reset")
+	cancel.Env = environment
+	cancel.Stdin = strings.NewReader("no\n")
+	cancelOutput, err := cancel.CombinedOutput()
+	if err != nil || !strings.Contains(string(cancelOutput), "Reset cancelled") {
+		t.Fatalf("cancel process err=%v output=%q", err, cancelOutput)
+	}
+
+	confirmed := exec.Command(binary, "reset")
+	confirmed.Env = environment
+	confirmed.Stdin = strings.NewReader("yes\n")
+	confirmedOutput, err := confirmed.CombinedOutput()
+	if err != nil || !strings.Contains(string(confirmedOutput), "shell: skipped (Disabled)") {
+		t.Fatalf("confirmed process err=%v output=%q", err, confirmedOutput)
+	}
+	if _, err := os.Stat(filepath.Join(root, "home", ".bashrc")); !os.IsNotExist(err) {
+		t.Fatalf("process smoke test touched a Shell startup file: %v", err)
+	}
+}
+
+func isolatedProcessEnvironment(root string) []string {
+	blocked := map[string]bool{"HOME": true, "XDG_CONFIG_HOME": true, "XDG_STATE_HOME": true, "SHELL": true, "SUDO_USER": true, "HTTP_PROXY": true, "http_proxy": true, "HTTPS_PROXY": true, "https_proxy": true, "ALL_PROXY": true, "all_proxy": true, "NO_PROXY": true, "no_proxy": true}
+	environment := make([]string, 0, len(os.Environ())+4)
+	for _, item := range os.Environ() {
+		name, _, _ := strings.Cut(item, "=")
+		if !blocked[name] {
+			environment = append(environment, item)
+		}
+	}
+	return append(environment,
+		"HOME="+filepath.Join(root, "home"),
+		"XDG_CONFIG_HOME="+filepath.Join(root, "config"),
+		"XDG_STATE_HOME="+filepath.Join(root, "state"),
+		"SHELL=/bin/fish",
+	)
+}
+
 func TestResetCommandCancellationAndTruthfulPartialFailureWithFakeTargets(t *testing.T) {
 	root := t.TempDir()
 	config := filepath.Join(root, "xdg-config", "proxyctl", "profiles.json")

internal/operationlock/lock.go

@@ -5,7 +5,6 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"os"
 	"path/filepath"
 	"time"
 
@@ -30,39 +29,13 @@ func (l *Lock) Lock(ctx context.Context) (func(), error) {
 	if !filepath.IsAbs(l.path) {
 		return nil, errors.New("operation lock path must be absolute")
 	}
-	directory := filepath.Dir(l.path)
-	if err := os.MkdirAll(directory, 0o700); err != nil {
-		return nil, fmt.Errorf("create operation lock directory: %w", err)
+	store := securefs.Store{TrustedRoot: securefs.ExistingTrustedRoot(l.path)}
+	if l.owned {
+		store.Owner = &securefs.Owner{UID: l.uid, GID: l.gid}
 	}
-	if err := os.Chmod(directory, 0o700); err != nil {
-		return nil, fmt.Errorf("secure operation lock directory: %w", err)
-	}
-	if l.owned && (os.Geteuid() != l.uid || os.Getegid() != l.gid) {
-		if err := os.Chown(directory, l.uid, l.gid); err != nil {
-			return nil, fmt.Errorf("set operation lock directory owner: %w", err)
-		}
-	}
-	file, err := securefs.OpenNoFollow(l.path, unix.O_CREAT|unix.O_RDWR|unix.O_NONBLOCK, 0o600)
+	file, err := store.OpenRegular(l.path, unix.O_CREAT|unix.O_RDWR|unix.O_NONBLOCK, 0o600)
 	if err != nil {
-		return nil, fmt.Errorf("open operation lock: %w", err)
-	}
-	info, err := file.Stat()
-	if err != nil || !info.Mode().IsRegular() {
-		file.Close()
-		if err != nil {
-			return nil, fmt.Errorf("inspect operation lock: %w", err)
-		}
-		return nil, errors.New("operation lock is not a regular file")
-	}
-	if err := file.Chmod(0o600); err != nil {
-		file.Close()
-		return nil, fmt.Errorf("secure operation lock: %w", err)
-	}
-	if l.owned && (os.Geteuid() != l.uid || os.Getegid() != l.gid) {
-		if err := file.Chown(l.uid, l.gid); err != nil {
-			file.Close()
-			return nil, fmt.Errorf("set operation lock owner: %w", err)
-		}
+		return nil, fmt.Errorf("open operation lock securely: %w", err)
 	}
 	for {
 		err = unix.Flock(int(file.Fd()), unix.LOCK_EX|unix.LOCK_NB)

internal/operationlock/lock_test.go

@@ -2,12 +2,36 @@ package operationlock_test
 
 import (
 	"context"
+	"os"
+	"path/filepath"
 	"testing"
 	"time"
 
 	"proxyctl/internal/operationlock"
 )
 
+func TestLockRejectsSymlinkedStateAncestor(t *testing.T) {
+	root := t.TempDir()
+	victim := filepath.Join(root, "victim")
+	if err := os.Mkdir(victim, 0o700); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.Symlink(victim, filepath.Join(root, "state")); err != nil {
+		t.Fatal(err)
+	}
+	lock := operationlock.New(filepath.Join(root, "state", "proxyctl", "operation.lock"))
+	if _, err := lock.Lock(context.Background()); err == nil {
+		t.Fatal("lock followed a symlinked state ancestor")
+	}
+	entries, err := os.ReadDir(victim)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(entries) != 0 {
+		t.Fatalf("victim changed: %v", entries)
+	}
+}
+
 func TestLockSerializesIndependentInstancesAndHonorsContext(t *testing.T) {
 	path := t.TempDir() + "/proxyctl.lock"
 	first := operationlock.New(path)

internal/oplog/logger.go

@@ -2,16 +2,15 @@
 package oplog
 
 import (
+	"errors"
 	"fmt"
+	"io/fs"
 	"os"
-	"path/filepath"
 	"strings"
 	"time"
 
 	"proxyctl/internal/app"
 	"proxyctl/internal/securefs"
-
-	"golang.org/x/sys/unix"
 )
 
 type Logger struct {
@@ -34,18 +33,6 @@ func (l *Logger) Write(entry app.LogEntry) error {
 	if l.maxBytes <= 0 {
 		return nil
 	}
-	directory := filepath.Dir(l.path)
-	if err := os.MkdirAll(directory, 0o700); err != nil {
-		return fmt.Errorf("create log directory: %w", err)
-	}
-	if err := os.Chmod(directory, 0o700); err != nil {
-		return fmt.Errorf("secure log directory: %w", err)
-	}
-	if l.owned && (os.Geteuid() != l.uid || os.Getegid() != l.gid) {
-		if err := os.Chown(directory, l.uid, l.gid); err != nil {
-			return fmt.Errorf("set log directory owner: %w", err)
-		}
-	}
 	detail := strings.NewReplacer("\n", " ", "\r", " ").Replace(app.RedactLogDetail(entry.Detail))
 	if entry.Time.IsZero() {
 		entry.Time = time.Now()
@@ -58,56 +45,65 @@ func (l *Logger) Write(entry app.LogEntry) error {
 		}
 		line = string(truncated)
 	}
-	if info, err := os.Stat(l.path); err == nil && info.Size()+int64(len(line)) > l.maxBytes {
-		if err := l.rotate(); err != nil {
+	store := l.store()
+	current, err := readOptional(store, l.path)
+	if err != nil {
+		return fmt.Errorf("read operation log securely: %w", err)
+	}
+	if int64(len(current)+len(line)) > l.maxBytes {
+		if err := l.rotate(store, current); err != nil {
 			return err
 		}
-	} else if err != nil && !os.IsNotExist(err) {
-		return fmt.Errorf("inspect operation log: %w", err)
+		current = nil
 	}
-	file, err := securefs.OpenNoFollow(l.path, unix.O_CREAT|unix.O_APPEND|unix.O_WRONLY|unix.O_NONBLOCK, 0o600)
-	if err != nil {
-		return fmt.Errorf("open operation log: %w", err)
-	}
-	defer file.Close()
-	fileInfo, err := file.Stat()
-	if err != nil {
-		return fmt.Errorf("inspect operation log: %w", err)
-	}
-	if !fileInfo.Mode().IsRegular() {
-		return fmt.Errorf("operation log is not a regular file")
-	}
-	if err := file.Chmod(0o600); err != nil {
-		return fmt.Errorf("secure operation log: %w", err)
-	}
-	if l.owned && (os.Geteuid() != l.uid || os.Getegid() != l.gid) {
-		if err := file.Chown(l.uid, l.gid); err != nil {
-			return fmt.Errorf("set operation log owner: %w", err)
-		}
-	}
-	if _, err := file.WriteString(line); err != nil {
-		return fmt.Errorf("write operation log: %w", err)
+	current = append(current, line...)
+	if err := store.AtomicWrite(l.path, current, 0o600); err != nil {
+		return fmt.Errorf("write operation log atomically: %w", err)
 	}
 	return nil
 }
 
+func (l *Logger) store() securefs.Store {
+	store := securefs.Store{TrustedRoot: securefs.ExistingTrustedRoot(l.path)}
+	if l.owned {
+		store.Owner = &securefs.Owner{UID: l.uid, GID: l.gid}
+	}
+	return store
+}
+
 func field(value string) string {
 	return strings.NewReplacer(" ", "_", "\n", "_", "\r", "_").Replace(value)
 }
 
-func (l *Logger) rotate() error {
+func (l *Logger) rotate(store securefs.Store, current []byte) error {
 	if l.rotations <= 0 {
-		return os.Remove(l.path)
+		return nil
 	}
-	_ = os.Remove(fmt.Sprintf("%s.%d", l.path, l.rotations))
-	for i := l.rotations - 1; i >= 1; i-- {
-		oldPath, newPath := fmt.Sprintf("%s.%d", l.path, i), fmt.Sprintf("%s.%d", l.path, i+1)
-		if err := os.Rename(oldPath, newPath); err != nil && !os.IsNotExist(err) {
+	for i := l.rotations; i >= 2; i-- {
+		previous := fmt.Sprintf("%s.%d", l.path, i-1)
+		data, err := readOptional(store, previous)
+		if err != nil {
+			return fmt.Errorf("read rotated operation log: %w", err)
+		}
+		if data == nil {
+			continue
+		}
+		if err := store.AtomicWrite(fmt.Sprintf("%s.%d", l.path, i), data, 0o600); err != nil {
 			return fmt.Errorf("rotate operation log: %w", err)
 		}
 	}
-	if err := os.Rename(l.path, l.path+".1"); err != nil && !os.IsNotExist(err) {
-		return fmt.Errorf("rotate operation log: %w", err)
+	if current != nil {
+		if err := store.AtomicWrite(l.path+".1", current, 0o600); err != nil {
+			return fmt.Errorf("rotate operation log: %w", err)
+		}
 	}
 	return nil
 }
+
+func readOptional(store securefs.Store, path string) ([]byte, error) {
+	data, err := store.ReadFile(path)
+	if errors.Is(err, fs.ErrNotExist) || errors.Is(err, os.ErrNotExist) {
+		return nil, nil
+	}
+	return data, err
+}

internal/oplog/logger_test.go

@@ -11,6 +11,28 @@ import (
 	"proxyctl/internal/oplog"
 )
 
+func TestLoggerRejectsSymlinkedStateAncestor(t *testing.T) {
+	root := t.TempDir()
+	victim := filepath.Join(root, "victim")
+	if err := os.Mkdir(victim, 0o700); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.Symlink(victim, filepath.Join(root, "state")); err != nil {
+		t.Fatal(err)
+	}
+	logger := oplog.New(filepath.Join(root, "state", "proxyctl", "operations.log"), 1024, 1)
+	if err := logger.Write(app.LogEntry{Operation: "reset", Result: "failure"}); err == nil {
+		t.Fatal("logger followed a symlinked state ancestor")
+	}
+	entries, err := os.ReadDir(victim)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(entries) != 0 {
+		t.Fatalf("victim changed: %v", entries)
+	}
+}
+
 func TestLoggerRotatesAtBoundAndNeverStoresAuthenticatedURL(t *testing.T) {
 	path := filepath.Join(t.TempDir(), "state", "proxyctl.log")
 	logger := oplog.New(path, 180, 2)

internal/securefs/securefs.go

@@ -18,6 +18,22 @@ import (
 
 type Owner struct{ UID, GID int }
 
+// ExistingTrustedRoot returns the nearest existing ancestor without resolving
+// symlinks. Descriptor traversal will subsequently reject a symlink ancestor.
+func ExistingTrustedRoot(path string) string {
+	candidate := filepath.Dir(filepath.Clean(path))
+	for {
+		if _, err := os.Lstat(candidate); err == nil || !errors.Is(err, fs.ErrNotExist) {
+			return candidate
+		}
+		parent := filepath.Dir(candidate)
+		if parent == candidate {
+			return candidate
+		}
+		candidate = parent
+	}
+}
+
 func OpenNoFollow(path string, flags int, mode uint32) (*os.File, error) {
 	fd, err := unix.Open(path, flags|unix.O_CLOEXEC|unix.O_NOFOLLOW, mode)
 	if err != nil {
@@ -55,6 +71,45 @@ type Store struct {
 	Owner       *Owner
 }
 
+// OpenRegular opens a regular file through a descriptor-anchored parent path.
+// Parent components and the final file are never followed through symlinks.
+func (s Store) OpenRegular(path string, flags int, mode os.FileMode) (*os.File, error) {
+	directory, base, err := s.openParent(path, flags&unix.O_CREAT != 0)
+	if err != nil {
+		return nil, err
+	}
+	if err := directory.Chmod(0o700); err != nil {
+		_ = directory.Close()
+		return nil, err
+	}
+	if s.Owner != nil && (s.Owner.UID != os.Geteuid() || s.Owner.GID != os.Getegid()) {
+		if err := directory.Chown(s.Owner.UID, s.Owner.GID); err != nil {
+			_ = directory.Close()
+			return nil, err
+		}
+	}
+	file, err := OpenAt(directory, base, flags, uint32(mode.Perm()))
+	_ = directory.Close()
+	if err != nil {
+		return nil, err
+	}
+	if err := regular(file); err != nil {
+		_ = file.Close()
+		return nil, err
+	}
+	if err := file.Chmod(mode.Perm()); err != nil {
+		_ = file.Close()
+		return nil, err
+	}
+	if s.Owner != nil && (s.Owner.UID != os.Geteuid() || s.Owner.GID != os.Getegid()) {
+		if err := file.Chown(s.Owner.UID, s.Owner.GID); err != nil {
+			_ = file.Close()
+			return nil, err
+		}
+	}
+	return file, nil
+}
+
 func (s Store) ReadFile(path string) ([]byte, error) {
 	directory, base, err := s.openParent(path, false)
 	if err != nil {

internal/shell/shell.go

@@ -256,7 +256,9 @@ func (a *Adapter) Inspect(_ context.Context, active *profile.Profile) target.Ins
 }
 
 func (a *Adapter) SetDesired(ctx context.Context, enabled bool, active *profile.Profile) target.OperationResult {
-	if !a.Availability(ctx).Available {
+	// Enabling requires a supported current Shell, but disabling must remain
+	// available so reset can remove managed state after the user's Shell changes.
+	if enabled && !a.Availability(ctx).Available {
 		return target.OperationResult{Inspection: a.Inspect(ctx, active), Err: errors.New("Shell Target is unavailable: Bash or Zsh was not detected")}
 	}
 	if enabled && active == nil {

internal/shell/shell_test.go

@@ -472,6 +472,21 @@ func TestRecoveryAndGeneratedFilesAreAtomicOwnerOnly(t *testing.T) {
 	}
 }
 
+func TestDisableManagedShellAfterCurrentShellBecomesUnsupported(t *testing.T) {
+	root := t.TempDir()
+	configuration := shell.Configuration{Shell: "/bin/bash", StartupFile: filepath.Join(root, ".bashrc"), EnvironmentFile: filepath.Join(root, "config", "shell.env"), RecoveryFile: filepath.Join(root, "state", "shell.json")}
+	enabled := shell.New(configuration, shell.OSFileSystem{}, shell.MapEnvironment{})
+	if result := enabled.SetDesired(context.Background(), true, activeProfile()); result.Err != nil {
+		t.Fatal(result.Err)
+	}
+	configuration.Shell = "/bin/fish"
+	unsupported := shell.New(configuration, shell.OSFileSystem{}, shell.MapEnvironment{})
+	result := unsupported.SetDesired(context.Background(), false, nil)
+	if result.Err != nil || result.Inspection.State != target.Disabled {
+		t.Fatalf("disable from unsupported Shell = %#v", result)
+	}
+}
+
 func activeProfile() *profile.Profile {
 	return &profile.Profile{Name: "office", Mode: profile.AdvancedMode, HTTPURL: "http://alice:secret@proxy.example:8080", HTTPSURL: "https://proxy.example:8443", AllURL: "socks5h://proxy.example:1080", Bypass: []string{"localhost", ".example.com", "10.0.0.0/8"}}
 }