diff --git a/pkg/browser/browser.go b/pkg/browser/browser.go index d17951a5..a34201c8 100644 --- a/pkg/browser/browser.go +++ b/pkg/browser/browser.go @@ -61,15 +61,11 @@ func (b *Browser) browse(url string, env []string) error { return cliBrowser.OpenURL(url) } - launcherArgs, err := shlex.Split(b.launcher) + launcherExe, launcherArgs, err := splitLauncher(b.launcher) if err != nil { return err } - launcherExe, err := safeexec.LookPath(launcherArgs[0]) - if err != nil { - return err - } - args := append(launcherArgs[1:], url) + args := append(launcherArgs, url) cmd := exec.Command(launcherExe, args...) cmd.Stdout = b.stdout cmd.Stderr = b.stderr @@ -79,6 +75,32 @@ func (b *Browser) browse(url string, env []string) error { return cmd.Run() } +// splitLauncher resolves the launcher string to an executable path and +// any extra arguments. Try the literal path first so launchers stored +// in a directory with spaces, like +// `C:\Program Files\Google\Chrome\Application\chrome.exe`, work without +// the user having to quote them. Fall back to shell-style word +// splitting when the literal lookup fails, since that's how launchers +// with arguments (`firefox --private-window`) are conventionally +// configured. +func splitLauncher(launcher string) (string, []string, error) { + if exe, err := safeexec.LookPath(launcher); err == nil { + return exe, nil, nil + } + parts, err := shlex.Split(launcher) + if err != nil { + return "", nil, err + } + if len(parts) == 0 { + return "", nil, fmt.Errorf("empty launcher") + } + exe, err := safeexec.LookPath(parts[0]) + if err != nil { + return "", nil, err + } + return exe, parts[1:], nil +} + func resolveLauncher() string { if ghBrowser := os.Getenv("GH_BROWSER"); ghBrowser != "" { return ghBrowser diff --git a/pkg/browser/browser_test.go b/pkg/browser/browser_test.go index f0b90c70..fdbcaa5d 100644 --- a/pkg/browser/browser_test.go +++ b/pkg/browser/browser_test.go @@ -203,6 +203,38 @@ func TestBrowse(t *testing.T) { } } +// Regression for #174: a launcher path containing a space (a common +// case on Windows with `C:\Program Files\...\chrome.exe`) was being +// shell-split into garbage by shlex. splitLauncher should try the +// literal path first so it survives unquoted. +func TestSplitLauncherPathWithSpaces(t *testing.T) { + dir := t.TempDir() + subdir := filepath.Join(dir, "with space") + require.NoError(t, os.MkdirAll(subdir, 0o755)) + + name := "fake-browser" + if runtime.GOOS == "windows" { + name += ".exe" + } + path := filepath.Join(subdir, name) + require.NoError(t, os.WriteFile(path, []byte("#!/bin/sh\n"), 0o755)) + + exe, args, err := splitLauncher(path) + require.NoError(t, err) + assert.Equal(t, path, exe) + assert.Empty(t, args) +} + +// splitLauncher should still honor traditional `firefox --private` +// style launchers that pass extra args alongside the binary name. +func TestSplitLauncherWithArgs(t *testing.T) { + launcher := fmt.Sprintf("%q --private-window", os.Args[0]) + exe, args, err := splitLauncher(launcher) + require.NoError(t, err) + assert.Contains(t, exe, filepath.Base(os.Args[0])) + assert.Equal(t, []string{"--private-window"}, args) +} + func TestResolveLauncher(t *testing.T) { tests := []struct { name string