fix(sso): launch the SSO browser through the launcher abstraction

pkg/assume/assume.go

@@ -36,19 +36,6 @@ import (
 	"gopkg.in/ini.v1"
 )
 
-// Launchers give a command that we need to run in order to launch a browser, such as
-// 'open <URL>' or 'firefox --new-tab <URL'. The returned command is a string slice,
-// with each element being an argument. (e.g. []string{"firefox", "--new-tab", "<URL>"})
-type Launcher interface {
-	LaunchCommand(url string, profile string) ([]string, error)
-	// UseForkProcess returns true if the launcher implementation should call
-	// the forkprocess library.
-	//
-	// For launchers that use 'open' commands, this should be false,
-	// as the forkprocess library causes the following error to appear:
-	// 	fork/exec open: no such file or directory
-	UseForkProcess() bool
-}
 type execConfig struct {
 	Cmd  string
 	Args []string
@@ -363,43 +350,22 @@ func AssumeCommand(c *cli.Context) error {
 			return errors.New("default browser not configured. run `granted browser set` to configure")
 		}
 
-		var l Launcher
-		switch cfg.DefaultBrowser {
-		case browser.ChromeKey, browser.BraveKey, browser.EdgeKey, browser.ChromiumKey, browser.VivaldiKey:
-			l = launcher.ChromeProfile{
-				BrowserType:    cfg.DefaultBrowser,
-				ExecutablePath: browserPath,
-			}
-		case browser.FirefoxKey, browser.WaterfoxKey:
-			l = launcher.Firefox{
-				ExecutablePath: browserPath,
-			}
-		case browser.SafariKey:
-			l = launcher.Safari{}
-		case browser.ArcKey:
-			l = launcher.Arc{}
-		case browser.ZenKey:
-			l = launcher.Zen{
-				ExecutablePath: browserPath,
-			}
-		case browser.FirefoxDevEditionKey:
-			l = launcher.FirefoxDevEdition{
-				ExecutablePath: browserPath,
-			}
-		case browser.FirefoxNightlyKey:
-			l = launcher.FirefoxNightly{
-				ExecutablePath: browserPath,
-			}
-		case browser.CustomKey:
+		var l launcher.Launcher
+		if cfg.DefaultBrowser == browser.CustomKey {
 			l, err = launcher.CustomFromLaunchTemplate(cfg.AWSConsoleBrowserLaunchTemplate, c.StringSlice("browser-launch-template-arg"))
 			if err == launcher.ErrLaunchTemplateNotConfigured {
 				return errors.New("error configuring custom browser, ensure that [AWSConsoleBrowserLaunchTemplate] is specified in your Granted config file")
 			}
 			if err != nil {
 				return err
 			}
-		default:
-			l = launcher.Open{}
+		} else {
+			l, err = launcher.ForBrowser(cfg.DefaultBrowser, browserPath, containerProfile)
+			if errors.Is(err, launcher.ErrUnsupportedBrowser) {
+				l = launcher.Open{}
+			} else if err != nil {
+				return err
+			}
 		}
 
 		printFlagUsage(con.Region, con.Service)

pkg/browser/detect.go

@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"os"
 	"os/exec"
+	"path/filepath"
 	"runtime"
 
 	"strings"
@@ -99,7 +100,19 @@ func Find() (string, error) {
 	return outcome, nil
 }
 
+// GetBrowserKey identifies a browser from a display name, from a vendor
+// identifier as returned by Find (a bundle ID on macOS, a .desktop file on
+// Linux, a registry ProgId on Windows), or from the path to its executable.
+// It returns StdoutKey for anything it does not recognise.
+//
+// Only the last path component is considered. Matching is by unanchored
+// substring, which is what lets one implementation handle every identifier
+// format, but the directory components of a path are chosen by the user and
+// routinely contain browser names. Matching those would let a home directory
+// decide which browser gets launched.
 func GetBrowserKey(b string) string {
+	b = filepath.Base(b)
+
 	if strings.Contains(strings.ToLower(b), "chrome") {
 		return ChromeKey
 	}

pkg/browser/detect_test.go

@@ -0,0 +1,67 @@
+package browser
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestGetBrowserKey(t *testing.T) {
+	tests := []struct {
+		name  string
+		input string
+		want  string
+	}{
+		// Display names.
+		{"display_name", "Chrome", ChromeKey},
+		{"display_name_multiword", "Firefox Developer Edition", FirefoxDevEditionKey},
+		{"display_name_stdout", "Stdout", StdoutKey},
+		{"display_name_custom", "Custom", CustomKey},
+
+		// Vendor identifiers, as returned by Find.
+		{"macos_bundle_id", "com.google.chrome", ChromeKey},
+		{"macos_bundle_id_safari", "com.apple.Safari", SafariKey},
+		{"linux_desktop_file", "firefox.desktop\n", FirefoxKey},
+		{"windows_progid", "ChromeHTML", ChromeKey},
+		{"windows_progid_edge", "MSEdgeHTM", EdgeKey},
+
+		// Executable paths.
+		{"path_safari", "/Applications/Safari.app/Contents/MacOS/Safari", SafariKey},
+		{"path_arc", "/Applications/Arc.app/Contents/MacOS/Arc", ArcKey},
+		{"path_chrome_mac", "/Applications/Google Chrome.app/Contents/MacOS/Google Chrome", ChromeKey},
+		{"path_firefox_linux", "/usr/bin/firefox", FirefoxKey},
+		{"path_waterfox", "/usr/bin/waterfox", WaterfoxKey},
+		{"path_zen_linux", "/usr/bin/zen-browser", ZenKey},
+		{"path_edge_windows", `\Program Files (x86)\Microsoft\Edge\Application\msedge.exe`, EdgeKey},
+
+		// Installed outside the default location.
+		{"path_homebrew", "/opt/homebrew/bin/firefox", FirefoxKey},
+		{"path_snap", "/snap/bin/chromium", ChromiumKey},
+
+		{"unknown", "/opt/some-browser/bin/browser", StdoutKey},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			assert.Equal(t, tt.want, GetBrowserKey(tt.input))
+		})
+	}
+}
+
+// Directory components are user-chosen and must not select a browser.
+func TestGetBrowserKey_IgnoresDirectoryComponents(t *testing.T) {
+	paths := []string{
+		"/Users/marcus/bin/browser",   // contains "arc"
+		"/Users/zenon/bin/browser",    // contains "zen"
+		"/opt/edge-cases/bin/browser", // contains "edge"
+		"/opt/research/bin/browser",   // contains "arc"
+		"/home/safarista/bin/browser", // contains "safari"
+		"/srv/chrome-testing/browser", // contains "chrome"
+	}
+
+	for _, path := range paths {
+		t.Run(path, func(t *testing.T) {
+			assert.Equal(t, StdoutKey, GetBrowserKey(path), "directory components must not select a browser")
+		})
+	}
+}

pkg/granted/console.go

@@ -8,7 +8,6 @@ import (
 
 	"github.com/common-fate/clio"
 	"github.com/common-fate/clio/clierr"
-	"github.com/fwdcloudsec/granted/pkg/assume"
 	"github.com/fwdcloudsec/granted/pkg/browser"
 	"github.com/fwdcloudsec/granted/pkg/cfaws"
 	"github.com/fwdcloudsec/granted/pkg/config"
@@ -66,53 +65,27 @@ var ConsoleCommand = cli.Command{
 			return nil
 		}
 
-		var l assume.Launcher
+		var l launcher.Launcher
 		if cfg.CustomBrowserPath == "" && cfg.DefaultBrowser != "" {
 			l = launcher.Open{}
 		} else if cfg.CustomBrowserPath == "" && cfg.AWSConsoleBrowserLaunchTemplate == nil {
 			return errors.New("default browser not configured. run `granted browser set` to configure")
 		} else {
-			switch cfg.DefaultBrowser {
-			case browser.ChromeKey:
-				l = launcher.ChromeProfile{
-					ExecutablePath: cfg.CustomBrowserPath,
-				}
-			case browser.BraveKey:
-				l = launcher.ChromeProfile{
-					ExecutablePath: cfg.CustomBrowserPath,
-				}
-			case browser.EdgeKey:
-				l = launcher.ChromeProfile{
-					ExecutablePath: cfg.CustomBrowserPath,
-				}
-			case browser.ChromiumKey:
-				l = launcher.ChromeProfile{
-					ExecutablePath: cfg.CustomBrowserPath,
-				}
-			case browser.VivaldiKey:
-				l = launcher.ChromeProfile{
-					ExecutablePath: cfg.CustomBrowserPath,
-				}
-			case browser.FirefoxKey:
-				l = launcher.Firefox{
-					ExecutablePath: cfg.CustomBrowserPath,
-				}
-			case browser.ZenKey:
-				l = launcher.Zen{
-					ExecutablePath: cfg.CustomBrowserPath,
-				}
-			case browser.SafariKey:
-				l = launcher.Safari{}
-			case browser.CustomKey:
+			if cfg.DefaultBrowser == browser.CustomKey {
 				l, err = launcher.CustomFromLaunchTemplate(cfg.AWSConsoleBrowserLaunchTemplate, c.StringSlice("browser-launch-template-arg"))
 				if err == launcher.ErrLaunchTemplateNotConfigured {
 					return errors.New("error configuring custom browser, ensure that [AWSConsoleBrowserLaunchTemplate] is specified in your Granted config file")
 				}
 				if err != nil {
 					return err
 				}
-			default:
-				l = launcher.Open{}
+			} else {
+				l, err = launcher.ForBrowser(cfg.DefaultBrowser, cfg.CustomBrowserPath, con.Profile)
+				if errors.Is(err, launcher.ErrUnsupportedBrowser) {
+					l = launcher.Open{}
+				} else if err != nil {
+					return err
+				}
 			}
 		}
 		// now build the actual command to run - e.g. 'firefox --new-tab <URL>'

pkg/idclogin/browser.go

@@ -8,19 +8,17 @@ import (
 
 	"github.com/common-fate/clio"
 	"github.com/common-fate/clio/clierr"
+	"github.com/fwdcloudsec/granted/pkg/browser"
 	grantedConfig "github.com/fwdcloudsec/granted/pkg/config"
 	"github.com/fwdcloudsec/granted/pkg/forkprocess"
 	"github.com/fwdcloudsec/granted/pkg/launcher"
-	"github.com/pkg/browser"
+	defaultbrowser "github.com/pkg/browser"
 )
 
 // openBrowser opens the given URL in the user's configured browser,
 // respecting Granted's custom browser settings. If the browser fails to open,
 // it returns an error.
 func openBrowser(url string, browserProfile string) error {
-	// nosemgrep: go.lang.security.audit.dangerous-exec-command.dangerous-exec-command
-	// The browser path comes from the user's local Granted configuration file,
-	// not from untrusted input.
 	config, err := grantedConfig.Load()
 	if err != nil {
 		return err
@@ -31,10 +29,10 @@ func openBrowser(url string, browserProfile string) error {
 	}
 
 	if config.CustomSSOBrowserPath != "" {
-		return openWithCustomPath(config.CustomSSOBrowserPath, url)
+		return openWithCustomPath(config.CustomSSOBrowserPath, url, browserProfile)
 	}
 
-	return browser.OpenURL(url)
+	return defaultbrowser.OpenURL(url)
 }
 
 func openWithLaunchTemplate(config *grantedConfig.Config, url string, browserProfile string) error {
@@ -46,6 +44,41 @@ func openWithLaunchTemplate(config *grantedConfig.Config, url string, browserPro
 		return err
 	}
 
+	return launch(l, url, browserProfile)
+}
+
+// openWithCustomPath launches the browser configured as CustomSSOBrowserPath.
+//
+// Browsers that Granted recognises are launched through their own Launcher,
+// because not all of them accept a URL as a bare argument. Safari in particular
+// resolves one as a file path relative to its sandbox container, which silently
+// opens the wrong page. Anything Granted does not recognise keeps being executed
+// directly, which is the behaviour those configurations already have.
+func openWithCustomPath(browserPath, url, browserProfile string) error {
+	l, err := launcherForPath(browserPath, browserProfile)
+	if err != nil {
+		return err
+	}
+	return launch(l, url, browserProfile)
+}
+
+func launcherForPath(browserPath, browserProfile string) (launcher.Launcher, error) {
+	key := browser.GetBrowserKey(browserPath)
+
+	l, err := launcher.ForBrowser(key, browserPath, browserProfile)
+	if errors.Is(err, launcher.ErrUnsupportedBrowser) {
+		clio.Debugf("no known browser at %s, launching it directly", browserPath)
+		return launcher.Direct{ExecutablePath: browserPath}, nil
+	}
+	if err != nil {
+		return nil, err
+	}
+	return l, nil
+}
+
+// launch runs a Launcher's command, detaching the browser from Granted so that
+// it outlives the CLI process.
+func launch(l launcher.Launcher, url, browserProfile string) error {
 	args, err := l.LaunchCommand(url, browserProfile)
 	if err != nil {
 		return fmt.Errorf("error building browser launch command: %w", err)
@@ -62,22 +95,15 @@ func openWithLaunchTemplate(config *grantedConfig.Config, url string, browserPro
 
 	clio.Debugf("running command without forkprocess: %s", args)
 	// nosemgrep: go.lang.security.audit.dangerous-exec-command.dangerous-exec-command
+	// The browser command is built from the user's local Granted configuration
+	// file, not from untrusted input.
 	cmd := exec.Command(args[0], args[1:]...)
 	cmd.Stdout = os.Stderr
 	cmd.Stderr = os.Stderr
-	return cmd.Start()
-}
-
-func openWithCustomPath(browserPath, url string) error {
-	// nosemgrep: go.lang.security.audit.dangerous-exec-command.dangerous-exec-command
-	cmd := exec.Command(browserPath, url)
-	cmd.Stdout = os.Stderr
-	cmd.Stderr = os.Stderr
 	if err := cmd.Start(); err != nil {
 		return err
 	}
-	_ = cmd.Process.Release()
-	return nil
+	return cmd.Process.Release()
 }
 
 // OpenBrowserWithFallbackMessage opens the browser and logs a helpful message if it fails.

pkg/idclogin/browser_test.go

@@ -0,0 +1,55 @@
+package idclogin
+
+import (
+	"testing"
+
+	"github.com/fwdcloudsec/granted/pkg/browser"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestLauncherForPath(t *testing.T) {
+	const url = "https://example.awsapps.com/start/#/device?user_code=ABCD-EFGH"
+
+	// 'open' on macOS, 'xdg-open' on Linux.
+	open := browser.OpenCommand()
+
+	tests := []struct {
+		name    string
+		path    string
+		profile string
+		want    []string
+	}{
+		{
+			name: "safari_is_launched_with_open",
+			path: "/Applications/Safari.app/Contents/MacOS/Safari",
+			want: []string{open, "-a", "Safari", url},
+		},
+		{
+			name: "firefox_keeps_its_own_launcher",
+			path: "/usr/bin/firefox",
+			want: []string{"/usr/bin/firefox", "--new-tab", url},
+		},
+		{
+			name: "unknown_browser_is_executed_directly",
+			path: "/opt/some-browser/bin/browser",
+			want: []string{"/opt/some-browser/bin/browser", url},
+		},
+		{
+			name: "chrome_without_sso_browser_profile",
+			path: "/usr/bin/google-chrome",
+			want: []string{"/usr/bin/google-chrome", "--no-first-run", "--no-default-browser-check", url},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			l, err := launcherForPath(tt.path, tt.profile)
+			require.NoError(t, err)
+
+			got, err := l.LaunchCommand(url, tt.profile)
+			require.NoError(t, err)
+			assert.Equal(t, tt.want, got)
+		})
+	}
+}