Prevent GitHub CLI and extensions from executing arbitrary commands from compromised GitHub Enterprise Server

@@ -2,7 +2,9 @@
 package browser
 
 import (
+	"fmt"
 	"io"
+	"net/url"
 	"os"
 	"os/exec"
 
@@ -45,9 +47,20 @@ func (b *Browser) Browse(url string) error {
 }
 
 func (b *Browser) browse(url string, env []string) error {
+	// Ensure the URL is supported including the scheme,
+	// overwrite `url` for use within the function.
+	urlParsed, err := isPossibleProtocol(url)
+	if err != nil {
+		return err
+	}
+
+	url = urlParsed.String()
+
+	// Use default `gh` browsing module for opening URL if not customized.
 	if b.launcher == "" {
 		return cliBrowser.OpenURL(url)
 	}
+
 	launcherArgs, err := shlex.Split(b.launcher)
 	if err != nil {
 		return err
@@ -78,3 +91,49 @@ func resolveLauncher() string {
 	}
 	return os.Getenv("BROWSER")
 }
+
+func isSupportedScheme(scheme string) bool {
+	switch scheme {
+	case "http", "https", "vscode", "vscode-insiders":
+		return true
+	default:
+		return false
+	}
+}
+
+func isPossibleProtocol(u string) (*url.URL, error) {
+	// Parse URL for known supported schemes before handling unknown cases.
+	urlParsed, err := url.Parse(u)
+	if err != nil {
+		return nil, fmt.Errorf("opening unparsable URL is unsupported: %s", u)
+	}
+
+	if isSupportedScheme(urlParsed.Scheme) {
+		return urlParsed, nil
+	}
+
+	// Disallow any unrecognized URL schemes if explicitly present.
+	if urlParsed.Scheme != "" {
+		return nil, fmt.Errorf("opening unsupport URL scheme: %s", u)
+	}
+
+	// Disallow URLs that match existing files or directories on the filesystem
+	// as these could be executables or executed by the launcher browser due to
+	// the file extension and/or associated application.
+	//
+	// Symlinks should not be resolved in order to avoid broken links or other
+	// vulnerabilities trying to resolve them.
+	if fileInfo, _ := os.Lstat(u); fileInfo != nil {
+		return nil, fmt.Errorf("opening files or directories is unsupported: %s", u)
+	}
+
+	// Disallow URLs that match executables found in the user path.
+	exec, _ := safeexec.LookPath(u)
+	if exec != "" {
+		return nil, fmt.Errorf("opening executables is unsupported: %s", u)
+	}
+
+	// Otherwise, assume HTTP URL using `https` to ensure secure browsing.
+	urlParsed.Scheme = "https"
+	return urlParsed, nil
+}

@@ -4,10 +4,13 @@ import (
 	"bytes"
 	"fmt"
 	"os"
+	"path/filepath"
+	"runtime"
 	"testing"
 
 	"github.com/cli/go-gh/v2/pkg/config"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestHelperProcess(t *testing.T) {
@@ -18,15 +21,186 @@ func TestHelperProcess(t *testing.T) {
 	os.Exit(0)
 }
 
+// TestBrowse ensures supported URLs are opened by the browser launcher.
+// Running package tests in VSCode will cause this to fail due to use of
+// `-coverageprofile` flag without `GOCOVERDIR` env var.
 func TestBrowse(t *testing.T) {
-	launcher := fmt.Sprintf("%q -test.run=TestHelperProcess -- chrome", os.Args[0])
-	stdout := &bytes.Buffer{}
-	stderr := &bytes.Buffer{}
-	b := Browser{launcher: launcher, stdout: stdout, stderr: stderr}
-	err := b.browse("github.com", []string{"GH_WANT_HELPER_PROCESS=1"})
-	assert.NoError(t, err)
-	assert.Equal(t, "[chrome github.com]", stdout.String())
-	assert.Equal(t, "", stderr.String())
+	type browseTest struct {
+		name     string
+		url      string
+		launcher string
+		expected string
+		setup    func(*testing.T) error
+		wantErr  bool
+	}
+
+	tests := []browseTest{
+		{
+			name:     "Explicit `http` URL works",
+			url:      "http://github.com",
+			launcher: fmt.Sprintf("%q -test.run=TestHelperProcess -- explicit http", os.Args[0]),
+			expected: "[explicit http http://github.com]",
+		},
+		{
+			name:     "Explicit `https` URL works",
+			url:      "https://github.com",
+			launcher: fmt.Sprintf("%q -test.run=TestHelperProcess -- explicit https", os.Args[0]),
+			expected: "[explicit https https://github.com]",
+		},
+		{
+			name:     "Explicit `HTTPS` URL works",
+			url:      "HTTPS://github.com",
+			launcher: fmt.Sprintf("%q -test.run=TestHelperProcess -- explicit HTTPS", os.Args[0]),
+			expected: "[explicit HTTPS https://github.com]",
+		},
+		{
+			name:     "Explicit `vscode` URL works",
+			url:      "vscode:extension/GitHub.copilot",
+			launcher: fmt.Sprintf("%q -test.run=TestHelperProcess -- explicit vscode", os.Args[0]),
+			expected: "[explicit vscode vscode:extension/GitHub.copilot]",
+		},
+		{
+			name:     "Explicit `vscode-insiders` URL works",
+			url:      "vscode-insiders:extension/GitHub.copilot",
+			launcher: fmt.Sprintf("%q -test.run=TestHelperProcess -- explicit vscode-insiders", os.Args[0]),
+			expected: "[explicit vscode-insiders vscode-insiders:extension/GitHub.copilot]",
+		},
+		{
+			name:     "Implicit `https` URL works",
+			url:      "github.com",
+			launcher: fmt.Sprintf("%q -test.run=TestHelperProcess -- implicit https", os.Args[0]),
+			expected: "[implicit https https://github.com]",
+		},
+		{
+			name:    "Explicit absolute `file://` URL errors",
+			url:     "file:///System/Applications/Calculator.app",
+			wantErr: true,
+		},
+	}
+
+	// Setup additional test scenarios covering OS-specific executables and directories
+	// that should be installed on maintainer workstations and GitHub hosted runners.
+	switch runtime.GOOS {
+	case "windows":
+		tests = append(tests, []browseTest{
+			{
+				name:    "Explicit absolute Windows file URL errors",
+				url:     `C:\Windows\System32\cmd.exe`,
+				wantErr: true,
+			},
+			{
+				name:    "Explicit absolute Windows directory URL errors",
+				url:     `C:\Windows\System32`,
+				wantErr: true,
+			},
+		}...)
+	// Default should handle common Unix/Linux scenarios including Mac OS.
+	default:
+		tests = append(tests, []browseTest{
+			{
+				name:    "Implicit absolute Unix/Linux file URL errors",
+				url:     "/bin/bash",
+				wantErr: true,
+			},
+			{
+				name:    "Implicit absolute Unix/Linux directory URL errors",
+				url:     "/bin",
+				wantErr: true,
+			},
+			{
+				name: "Implicit relative Unix/Linux file URL errors",
+				url:  "poc.command",
+				setup: func(t *testing.T) error {
+					// Setup a temporary directory to stage content and execute the test within,
+					// ensure the test's original working directory is restored after.
+					cwd, err := os.Getwd()
+					if err != nil {
+						return err
+					}
+
+					tempDir := t.TempDir()
+					err = os.Chdir(tempDir)
+					if err != nil {
+						return err
+					}
+
+					t.Cleanup(func() {
+						_ = os.Chdir(cwd)
+					})
+
+					// Create content for local file URL testing
+					path := filepath.Join(tempDir, "poc.command")
+					err = os.WriteFile(path, []byte("#!/bin/bash\necho hello"), 0755)
+					if err != nil {
+						return err
+					}
+
+					return nil
+				},
+				wantErr: true,
+			},
+			{
+				name: "Implicit relative Unix/Linux directory URL errors",
+				url:  "Fake.app",
+				setup: func(t *testing.T) error {
+					// Setup a temporary directory to stage content and execute the test within,
+					// ensure the test's original working directory is restored after.
+					cwd, err := os.Getwd()
+					if err != nil {
+						return err
+					}
+
+					tempDir := t.TempDir()
+					err = os.Chdir(tempDir)
+					if err != nil {
+						return err
+					}
+
+					t.Cleanup(func() {
+						_ = os.Chdir(cwd)
+					})
+
+					// Create content for local directory URL testing
+					path := filepath.Join(tempDir, "Fake.app")
+					err = os.Mkdir(path, 0755)
+					if err != nil {
+						return err
+					}
+
+					path = filepath.Join(path, "poc.command")
+					err = os.WriteFile(path, []byte("#!/bin/bash\necho hello"), 0755)
+					if err != nil {
+						return err
+					}
+
+					return nil
+				},
+				wantErr: true,
+			},
+		}...)
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if tt.setup != nil {
+				err := tt.setup(t)
+				require.NoError(t, err)
+			}
+
+			stdout := &bytes.Buffer{}
+			stderr := &bytes.Buffer{}
+			b := Browser{launcher: tt.launcher, stdout: stdout, stderr: stderr}
+			err := b.browse(tt.url, []string{"GH_WANT_HELPER_PROCESS=1"})
+
+			if tt.wantErr {
+				require.Error(t, err)
+			} else {
+				require.NoError(t, err)
+				assert.Equal(t, tt.expected, stdout.String())
+				assert.Equal(t, "", stderr.String())
+			}
+		})
+	}
 }
 
 func TestResolveLauncher(t *testing.T) {