fix: harden GNOME settings recovery

internal/gnome/gnome.go

@@ -9,6 +9,7 @@ import (
 	"encoding/json"
 	"errors"
 	"fmt"
+	"io"
 	"io/fs"
 	"net/url"
 	"os"
@@ -17,6 +18,7 @@ import (
 	"strconv"
 	"strings"
 	"syscall"
+	"unicode/utf8"
 
 	"proxyctl/internal/profile"
 	"proxyctl/internal/securefs"
@@ -102,6 +104,7 @@ type StateStore interface {
 	Load() ([]byte, error)
 	Save([]byte) error
 	Delete() error
+	Lock() (io.Closer, error)
 }
 
 type fileState struct {
@@ -119,6 +122,9 @@ func NewFileState(path string, uid, gid int, trustedRoot string) StateStore {
 func (s *fileState) Load() ([]byte, error)  { return s.store.ReadFile(s.path) }
 func (s *fileState) Save(data []byte) error { return s.store.AtomicWrite(s.path, data, 0o600) }
 func (s *fileState) Delete() error          { return s.store.Remove(s.path) }
+func (s *fileState) Lock() (io.Closer, error) {
+	return s.store.LockFile(s.path + ".lock")
+}
 
 type Configuration struct {
 	CommandName string
@@ -322,7 +328,11 @@ func (a *Adapter) SetDesired(ctx context.Context, enabled bool, active *profile.
 	if enabled && active == nil {
 		return target.OperationResult{Inspection: a.Inspect(ctx, active), Err: errors.New("no Active Profile; open Profile management to create and activate one")}
 	}
-	var err error
+	lock, err := a.state.Lock()
+	if err != nil {
+		return target.OperationResult{Inspection: a.Inspect(ctx, active), Err: redactForProfile(fmt.Errorf("lock GNOME mutation: %w", err), active)}
+	}
+	defer func() { _ = lock.Close() }() // best effort: release the process-shared advisory lock
 	if enabled {
 		err = a.enable(ctx, *active)
 	} else {
@@ -373,7 +383,17 @@ func (a *Adapter) disable(ctx context.Context) error {
 	if err != nil {
 		return fmt.Errorf("verify managed GNOME settings before restore: %w", err)
 	}
-	if fingerprint(current) != metadata.LastFingerprint {
+	currentFingerprint := fingerprint(current)
+	if currentFingerprint == fingerprint(metadata.Original) {
+		// The restore completed previously, but metadata cleanup did not. It is
+		// safe to retry only the cleanup while the restored snapshot still
+		// matches exactly.
+		if err := a.state.Delete(); err != nil && !errors.Is(err, fs.ErrNotExist) {
+			return fmt.Errorf("remove GNOME recovery metadata after restoring settings: %w", err)
+		}
+		return nil
+	}
+	if currentFingerprint != metadata.LastFingerprint {
 		return errors.New("GNOME settings no longer match proxyctl's last applied values; refusing restore because ownership evidence was lost")
 	}
 	if err := a.writeSettings(ctx, metadata.Original); err != nil {
@@ -593,7 +613,41 @@ func fingerprint(value settings) string {
 }
 
 func quote(value string) string {
-	return "'" + strings.ReplaceAll(strings.ReplaceAll(value, "\\", "\\\\"), "'", "\\'") + "'"
+	delimiter := byte('\'')
+	if strings.ContainsRune(value, '\'') {
+		// This matches GVariant's canonical printer: apostrophes select a
+		// double-quoted literal rather than an escaped single-quoted one.
+		delimiter = '"'
+	}
+	var b strings.Builder
+	b.WriteByte(delimiter)
+	for _, r := range value {
+		switch r {
+		case '\a':
+			b.WriteString(`\a`)
+		case '\b':
+			b.WriteString(`\b`)
+		case '\f':
+			b.WriteString(`\f`)
+		case '\n':
+			b.WriteString(`\n`)
+		case '\r':
+			b.WriteString(`\r`)
+		case '\t':
+			b.WriteString(`\t`)
+		case '\v':
+			b.WriteString(`\v`)
+		case '\\':
+			b.WriteString(`\\`)
+		default:
+			if r == rune(delimiter) {
+				b.WriteByte('\\')
+			}
+			b.WriteRune(r)
+		}
+	}
+	b.WriteByte(delimiter)
+	return b.String()
 }
 func quoteStrings(values []string) string {
 	quoted := make([]string, len(values))
@@ -604,74 +658,103 @@ func quoteStrings(values []string) string {
 }
 func parseString(raw string) (string, error) {
 	raw = strings.TrimSpace(raw)
-	if len(raw) < 2 || raw[0] != '\'' || raw[len(raw)-1] != '\'' {
+	value, consumed, err := parseQuoted(raw)
+	if err != nil || consumed != len(raw) {
 		return "", fmt.Errorf("invalid gsettings string %q", raw)
 	}
-	var b strings.Builder
+	return value, nil
-	escaped := false
-	for _, r := range raw[1 : len(raw)-1] {
-		if escaped {
-			b.WriteRune(r)
-			escaped = false
-		} else if r == '\\' {
-			escaped = true
-		} else {
-			b.WriteRune(r)
-		}
-	}
-	if escaped {
-		return "", fmt.Errorf("invalid gsettings escape %q", raw)
-	}
-	return b.String(), nil
 }
+
+func parseQuoted(raw string) (string, int, error) {
+	if len(raw) < 2 || (raw[0] != '\'' && raw[0] != '"') {
+		return "", 0, errors.New("missing GVariant string delimiter")
+	}
+	delimiter := raw[0]
+	var b strings.Builder
+	for i := 1; i < len(raw); {
+		if raw[i] == delimiter {
+			return b.String(), i + 1, nil
+		}
+		if raw[i] != '\\' {
+			r, size := utf8.DecodeRuneInString(raw[i:])
+			if r == utf8.RuneError && size == 1 {
+				return "", 0, errors.New("invalid UTF-8 in GVariant string")
+			}
+			b.WriteRune(r)
+			i += size
+			continue
+		}
+		i++
+		if i >= len(raw) {
+			return "", 0, errors.New("trailing GVariant escape")
+		}
+		switch raw[i] {
+		case 'a':
+			b.WriteByte('\a')
+		case 'b':
+			b.WriteByte('\b')
+		case 'f':
+			b.WriteByte('\f')
+		case 'n':
+			b.WriteByte('\n')
+		case 'r':
+			b.WriteByte('\r')
+		case 't':
+			b.WriteByte('\t')
+		case 'v':
+			b.WriteByte('\v')
+		case '\\', '\'', '"':
+			b.WriteByte(raw[i])
+		case 'u', 'U':
+			digits := 4
+			if raw[i] == 'U' {
+				digits = 8
+			}
+			if i+digits >= len(raw) {
+				return "", 0, errors.New("short GVariant Unicode escape")
+			}
+			code, err := strconv.ParseUint(raw[i+1:i+1+digits], 16, 32)
+			if err != nil || !utf8.ValidRune(rune(code)) {
+				return "", 0, errors.New("invalid GVariant Unicode escape")
+			}
+			b.WriteRune(rune(code))
+			i += digits
+		default:
+			return "", 0, errors.New("unsupported GVariant escape")
+		}
+		i++
+	}
+	return "", 0, errors.New("unterminated GVariant string")
+}
+
 func parseStrings(raw string) ([]string, error) {
 	raw = strings.TrimSpace(raw)
-	if raw == "[]" {
-		return []string{}, nil
-	}
 	if len(raw) < 2 || raw[0] != '[' || raw[len(raw)-1] != ']' {
 		return nil, fmt.Errorf("invalid gsettings string array %q", raw)
 	}
 	inner := strings.TrimSpace(raw[1 : len(raw)-1])
+	if inner == "" {
+		return []string{}, nil
+	}
 	var result []string
-	for len(inner) > 0 {
+	for {
-		if inner[0] != '\'' {
+		value, consumed, err := parseQuoted(inner)
-			return nil, fmt.Errorf("invalid gsettings string array %q", raw)
-		}
-		end := -1
-		escaped := false
-		for i := 1; i < len(inner); i++ {
-			if escaped {
-				escaped = false
-				continue
-			}
-			if inner[i] == '\\' {
-				escaped = true
-				continue
-			}
-			if inner[i] == '\'' {
-				end = i
-				break
-			}
-		}
-		if end < 0 {
-			return nil, fmt.Errorf("invalid gsettings string array %q", raw)
-		}
-		value, err := parseString(inner[:end+1])
 		if err != nil {
-			return nil, err
+			return nil, fmt.Errorf("invalid gsettings string array %q", raw)
 		}
 		result = append(result, value)
-		inner = strings.TrimSpace(inner[end+1:])
+		inner = strings.TrimSpace(inner[consumed:])
 		if inner == "" {
-			break
+			return result, nil
 		}
 		if inner[0] != ',' {
 			return nil, fmt.Errorf("invalid gsettings string array %q", raw)
 		}
 		inner = strings.TrimSpace(inner[1:])
+		if inner == "" {
+			return nil, fmt.Errorf("invalid gsettings string array %q", raw)
+		}
 	}
-	return result, nil
 }
 
 func redactForProfile(err error, active *profile.Profile) error {

internal/gnome/gnome_test.go

@@ -4,11 +4,14 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"io"
 	"os"
+	"os/exec"
 	"path/filepath"
 	"reflect"
 	"strings"
 	"testing"
+	"time"
 
 	"proxyctl/internal/gnome"
 	"proxyctl/internal/profile"
@@ -89,6 +92,11 @@ func (s *memoryState) Delete() error {
 	s.data = nil
 	return nil
 }
+func (s *memoryState) Lock() (io.Closer, error) { return nopCloser{}, nil }
+
+type nopCloser struct{}
+
+func (nopCloser) Close() error { return nil }
 
 func adapter(r *fakeRunner, state *memoryState) *gnome.Adapter {
 	return gnome.New(gnome.Configuration{CommandName: "gsettings", UID: 1000, GID: 1001, HomeDir: "/home/alice", ConfigDir: "/home/alice/.config", DataDir: "/home/alice/.local/share", CacheDir: "/home/alice/.cache", RuntimeDir: "/run/user/1000", StateFile: "/state/gnome.json"}, r, state)
@@ -188,6 +196,30 @@ func TestDescriptionStatesDesktopScopeWithoutUniversalCoverage(t *testing.T) {
 	}
 }
 
+func TestGVariantSingleAndDoubleQuotedValuesRestoreCanonically(t *testing.T) {
+	r, s := newRunner(), &memoryState{}
+	r.values["org.gnome.system.proxy mode"] = "'auto'"
+	r.values["org.gnome.system.proxy autoconfig-url"] = `"https://config.example/don't\\path"`
+	r.values["org.gnome.system.proxy ignore-hosts"] = `[
+		"don't.example",
+		'back\\slash'
+	]`
+
+	a := adapter(r, s)
+	if got := a.SetDesired(context.Background(), true, active()); got.Err != nil {
+		t.Fatalf("enable: %#v", got)
+	}
+	if got := a.SetDesired(context.Background(), false, nil); got.Err != nil {
+		t.Fatalf("disable: %#v", got)
+	}
+	if got := r.values["org.gnome.system.proxy autoconfig-url"]; got != `"https://config.example/don't\\path"` {
+		t.Fatalf("canonical autoconfig URL = %s", got)
+	}
+	if got := r.values["org.gnome.system.proxy ignore-hosts"]; got != `["don't.example", 'back\\slash']` {
+		t.Fatalf("canonical ignore hosts = %s", got)
+	}
+}
+
 func TestEnableSnapshotsAppliesInspectsAndRestoresExternalSettings(t *testing.T) {
 	r, s := newRunner(), &memoryState{}
 	r.values["org.gnome.system.proxy mode"] = "'auto'"
@@ -309,6 +341,143 @@ func TestCommandContractCarriesInvokingUserIdentityAndSessionEnvironment(t *test
 	}
 }
 
+func TestMidOperationSetFailuresExposePartialStateAndRemainRecoverable(t *testing.T) {
+	t.Run("enable", func(t *testing.T) {
+		r, s := newRunner(), &memoryState{}
+		r.values["org.gnome.system.proxy mode"] = "'auto'"
+		r.values["org.gnome.system.proxy autoconfig-url"] = "'https://old.example/proxy.pac'"
+		setCount := 0
+		r.fail = func(c gnome.Command) error {
+			if len(c.Args) > 0 && c.Args[0] == "set" {
+				setCount++
+				if setCount == 4 {
+					return os.ErrPermission
+				}
+			}
+			return nil
+		}
+		a := adapter(r, s)
+		got := a.SetDesired(context.Background(), true, active())
+		if got.Err == nil || !errors.Is(got.Err, os.ErrPermission) || got.Inspection.State != target.Drifted {
+			t.Fatalf("partial enable = %#v", got)
+		}
+		if setCount != 4 || r.values["org.gnome.system.proxy autoconfig-url"] != "''" || r.values["org.gnome.system.proxy ignore-hosts"] == "[]" || r.values["org.gnome.system.proxy.http host"] != "''" || s.data == nil {
+			t.Fatalf("failure was not genuinely partial: sets=%d values=%#v metadata=%q", setCount, r.values, s.data)
+		}
+		r.fail = nil
+		if got := a.SetDesired(context.Background(), true, active()); got.Err != nil || got.Inspection.State != target.Enabled {
+			t.Fatalf("retry enable = %#v", got)
+		}
+		if got := a.SetDesired(context.Background(), false, nil); got.Err != nil || got.Inspection.State != target.External {
+			t.Fatalf("restore after retry = %#v", got)
+		}
+	})
+
+	t.Run("restore", func(t *testing.T) {
+		r, s := newRunner(), &memoryState{}
+		r.values["org.gnome.system.proxy mode"] = "'auto'"
+		r.values["org.gnome.system.proxy autoconfig-url"] = "'https://old.example/proxy.pac'"
+		a := adapter(r, s)
+		if got := a.SetDesired(context.Background(), true, active()); got.Err != nil {
+			t.Fatal(got.Err)
+		}
+		setCount := 0
+		r.fail = func(c gnome.Command) error {
+			if len(c.Args) > 0 && c.Args[0] == "set" {
+				setCount++
+				if setCount == 5 {
+					return errors.New("session disappeared")
+				}
+			}
+			return nil
+		}
+		got := a.SetDesired(context.Background(), false, nil)
+		if got.Err == nil || !strings.Contains(got.Err.Error(), "session disappeared") || got.Inspection.State != target.Drifted || setCount != 5 || s.data == nil {
+			t.Fatalf("partial restore = %#v sets=%d metadata=%q", got, setCount, s.data)
+		}
+		r.fail = nil
+		// A partial restore loses ownership evidence and must not overwrite the
+		// mixed settings on retry.
+		before := len(r.calls)
+		got = a.SetDesired(context.Background(), false, nil)
+		if got.Err == nil || !strings.Contains(got.Err.Error(), "ownership") {
+			t.Fatalf("unsafe retry = %#v", got)
+		}
+		for _, c := range r.calls[before:] {
+			if len(c.Args) > 0 && c.Args[0] == "set" {
+				t.Fatalf("retry mutated partial external state: %#v", c)
+			}
+		}
+	})
+}
+
+func TestRestoreMetadataDeleteFailureCanBeSafelyRetried(t *testing.T) {
+	r, s := newRunner(), &memoryState{}
+	r.values["org.gnome.system.proxy mode"] = "'auto'"
+	r.values["org.gnome.system.proxy autoconfig-url"] = "'https://old.example/proxy.pac'"
+	a := adapter(r, s)
+	if got := a.SetDesired(context.Background(), true, active()); got.Err != nil {
+		t.Fatal(got.Err)
+	}
+	s.deleteErr = os.ErrPermission
+	first := a.SetDesired(context.Background(), false, nil)
+	if first.Err == nil || !errors.Is(first.Err, os.ErrPermission) || r.values["org.gnome.system.proxy mode"] != "'auto'" || s.data == nil {
+		t.Fatalf("first disable = %#v values=%#v metadata=%q", first, r.values, s.data)
+	}
+	s.deleteErr = nil
+	before := len(r.calls)
+	retry := a.SetDesired(context.Background(), false, nil)
+	if retry.Err != nil || retry.Inspection.State != target.External || s.data != nil {
+		t.Fatalf("retry disable = %#v metadata=%q", retry, s.data)
+	}
+	for _, c := range r.calls[before:] {
+		if len(c.Args) > 0 && c.Args[0] == "set" {
+			t.Fatalf("cleanup retry rewrote restored settings: %#v", c)
+		}
+	}
+}
+
+func TestFileStateLockSerializesGNOMEOperationsAcrossProcesses(t *testing.T) {
+	if path := os.Getenv("PROXYCTL_GNOME_LOCK_HELPER_PATH"); path != "" {
+		r := newRunner()
+		state := gnome.NewFileState(path, -1, -1, filepath.Dir(filepath.Dir(path)))
+		a := gnome.New(gnome.Configuration{UID: os.Getuid(), GID: os.Getgid(), HomeDir: t.TempDir(), StateFile: path}, r, state)
+		got := a.SetDesired(context.Background(), true, active())
+		if got.Err != nil {
+			t.Fatal(got.Err)
+		}
+		return
+	}
+
+	path := filepath.Join(t.TempDir(), "proxyctl", "gnome.json")
+	state := gnome.NewFileState(path, -1, -1, filepath.Dir(filepath.Dir(path)))
+	lock, err := state.Lock()
+	if err != nil {
+		t.Fatal(err)
+	}
+	command := exec.Command(os.Args[0], "-test.run=^TestFileStateLockSerializesGNOMEOperationsAcrossProcesses$")
+	command.Env = append(os.Environ(), "PROXYCTL_GNOME_LOCK_HELPER_PATH="+path)
+	done := make(chan error, 1)
+	go func() { done <- command.Run() }()
+	select {
+	case err := <-done:
+		t.Fatalf("child operation did not wait for lock: %v", err)
+	case <-time.After(100 * time.Millisecond):
+	}
+	if err := lock.Close(); err != nil {
+		t.Fatal(err)
+	}
+	select {
+	case err := <-done:
+		if err != nil {
+			t.Fatalf("child operation after unlock: %v", err)
+		}
+	case <-time.After(5 * time.Second):
+		_ = command.Process.Kill()
+		t.Fatal("child operation remained blocked after unlock")
+	}
+}
+
 func TestFailuresAreImmediateTruthfulAndPreserveOwnership(t *testing.T) {
 	t.Run("permission before mutation", func(t *testing.T) {
 		r, s := newRunner(), &memoryState{}

internal/securefs/securefs.go

@@ -55,6 +55,50 @@ type Store struct {
 	Owner       *Owner
 }
 
+type fileLock struct{ file *os.File }
+
+func (l *fileLock) Close() error {
+	unlockErr := unix.Flock(int(l.file.Fd()), unix.LOCK_UN)
+	closeErr := l.file.Close()
+	if unlockErr != nil {
+		return unlockErr
+	}
+	return closeErr
+}
+
+// LockFile acquires an exclusive advisory lock without following symbolic
+// links. The returned closer releases the lock and closes its descriptor.
+func (s Store) LockFile(path string) (io.Closer, error) {
+	directory, base, err := s.openParent(path, true)
+	if err != nil {
+		return nil, err
+	}
+	defer directory.Close()
+	file, err := OpenAt(directory, base, unix.O_CREAT|unix.O_RDWR, 0o600)
+	if err != nil {
+		return nil, err
+	}
+	if err := regular(file); 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
+		}
+	}
+	if err := file.Chmod(0o600); err != nil {
+		_ = file.Close()
+		return nil, err
+	}
+	if err := unix.Flock(int(file.Fd()), unix.LOCK_EX); err != nil {
+		_ = file.Close()
+		return nil, err
+	}
+	return &fileLock{file: file}, nil
+}
+
 func (s Store) ReadFile(path string) ([]byte, error) {
 	directory, base, err := s.openParent(path, false)
 	if err != nil {