diff --git a/internal/gnome/gnome.go b/internal/gnome/gnome.go index b2a717f..59cbc1a 100644 --- a/internal/gnome/gnome.go +++ b/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 - 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 + return value, 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 { - if inner[0] != '\'' { - 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]) + for { + value, consumed, err := parseQuoted(inner) 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 { diff --git a/internal/gnome/gnome_test.go b/internal/gnome/gnome_test.go index b48c91e..4c0f306 100644 --- a/internal/gnome/gnome_test.go +++ b/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{} diff --git a/internal/securefs/securefs.go b/internal/securefs/securefs.go index ad882ac..5042011 100644 --- a/internal/securefs/securefs.go +++ b/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 {