Skip to content

Commit a08820a

Browse files
BagToadBagToad
authored
Merge commit from fork
Initial fix for browsing URLs
2 parents 61bf393 + 1ecf6c4 commit a08820a

File tree

2 files changed

+241
-8
lines changed

2 files changed

+241
-8
lines changed

‎pkg/browser/browser.go‎

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,9 @@
22
package browser
33

44
import (
5+
"fmt"
56
"io"
7+
"net/url"
68
"os"
79
"os/exec"
810

@@ -45,9 +47,20 @@ func (b *Browser) Browse(url string) error {
4547
}
4648

4749
func (b *Browser) browse(url string, env []string) error {
50+
// Ensure the URL is supported including the scheme,
51+
// overwrite `url` for use within the function.
52+
urlParsed, err := isPossibleProtocol(url)
53+
if err != nil {
54+
return err
55+
}
56+
57+
url = urlParsed.String()
58+
59+
// Use default `gh` browsing module for opening URL if not customized.
4860
if b.launcher == "" {
4961
return cliBrowser.OpenURL(url)
5062
}
63+
5164
launcherArgs, err := shlex.Split(b.launcher)
5265
if err != nil {
5366
return err
@@ -78,3 +91,49 @@ func resolveLauncher() string {
7891
}
7992
return os.Getenv("BROWSER")
8093
}
94+
95+
func isSupportedScheme(scheme string) bool {
96+
switch scheme {
97+
case "http", "https", "vscode", "vscode-insiders":
98+
return true
99+
default:
100+
return false
101+
}
102+
}
103+
104+
func isPossibleProtocol(u string) (*url.URL, error) {
105+
// Parse URL for known supported schemes before handling unknown cases.
106+
urlParsed, err := url.Parse(u)
107+
if err != nil {
108+
return nil, fmt.Errorf("opening unparsable URL is unsupported: %s", u)
109+
}
110+
111+
if isSupportedScheme(urlParsed.Scheme) {
112+
return urlParsed, nil
113+
}
114+
115+
// Disallow any unrecognized URL schemes if explicitly present.
116+
if urlParsed.Scheme != "" {
117+
return nil, fmt.Errorf("opening unsupport URL scheme: %s", u)
118+
}
119+
120+
// Disallow URLs that match existing files or directories on the filesystem
121+
// as these could be executables or executed by the launcher browser due to
122+
// the file extension and/or associated application.
123+
//
124+
// Symlinks should not be resolved in order to avoid broken links or other
125+
// vulnerabilities trying to resolve them.
126+
if fileInfo, _ := os.Lstat(u); fileInfo != nil {
127+
return nil, fmt.Errorf("opening files or directories is unsupported: %s", u)
128+
}
129+
130+
// Disallow URLs that match executables found in the user path.
131+
exec, _ := safeexec.LookPath(u)
132+
if exec != "" {
133+
return nil, fmt.Errorf("opening executables is unsupported: %s", u)
134+
}
135+
136+
// Otherwise, assume HTTP URL using `https` to ensure secure browsing.
137+
urlParsed.Scheme = "https"
138+
return urlParsed, nil
139+
}

‎pkg/browser/browser_test.go‎

Lines changed: 182 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,13 @@ import (
44
"bytes"
55
"fmt"
66
"os"
7+
"path/filepath"
8+
"runtime"
79
"testing"
810

911
"github.com/cli/go-gh/v2/pkg/config"
1012
"github.com/stretchr/testify/assert"
13+
"github.com/stretchr/testify/require"
1114
)
1215

1316
func TestHelperProcess(t *testing.T) {
@@ -18,15 +21,186 @@ func TestHelperProcess(t *testing.T) {
1821
os.Exit(0)
1922
}
2023

24+
// TestBrowse ensures supported URLs are opened by the browser launcher.
25+
// Running package tests in VSCode will cause this to fail due to use of
26+
// `-coverageprofile` flag without `GOCOVERDIR` env var.
2127
func TestBrowse(t *testing.T) {
22-
launcher := fmt.Sprintf("%q -test.run=TestHelperProcess -- chrome", os.Args[0])
23-
stdout := &bytes.Buffer{}
24-
stderr := &bytes.Buffer{}
25-
b := Browser{launcher: launcher, stdout: stdout, stderr: stderr}
26-
err := b.browse("github.com", []string{"GH_WANT_HELPER_PROCESS=1"})
27-
assert.NoError(t, err)
28-
assert.Equal(t, "[chrome github.com]", stdout.String())
29-
assert.Equal(t, "", stderr.String())
28+
type browseTest struct {
29+
name string
30+
url string
31+
launcher string
32+
expected string
33+
setup func(*testing.T) error
34+
wantErr bool
35+
}
36+
37+
tests := []browseTest{
38+
{
39+
name: "Explicit `http` URL works",
40+
url: "http://github.com",
41+
launcher: fmt.Sprintf("%q -test.run=TestHelperProcess -- explicit http", os.Args[0]),
42+
expected: "[explicit http http://github.com]",
43+
},
44+
{
45+
name: "Explicit `https` URL works",
46+
url: "https://github.com",
47+
launcher: fmt.Sprintf("%q -test.run=TestHelperProcess -- explicit https", os.Args[0]),
48+
expected: "[explicit https https://github.com]",
49+
},
50+
{
51+
name: "Explicit `HTTPS` URL works",
52+
url: "HTTPS://github.com",
53+
launcher: fmt.Sprintf("%q -test.run=TestHelperProcess -- explicit HTTPS", os.Args[0]),
54+
expected: "[explicit HTTPS https://github.com]",
55+
},
56+
{
57+
name: "Explicit `vscode` URL works",
58+
url: "vscode:extension/GitHub.copilot",
59+
launcher: fmt.Sprintf("%q -test.run=TestHelperProcess -- explicit vscode", os.Args[0]),
60+
expected: "[explicit vscode vscode:extension/GitHub.copilot]",
61+
},
62+
{
63+
name: "Explicit `vscode-insiders` URL works",
64+
url: "vscode-insiders:extension/GitHub.copilot",
65+
launcher: fmt.Sprintf("%q -test.run=TestHelperProcess -- explicit vscode-insiders", os.Args[0]),
66+
expected: "[explicit vscode-insiders vscode-insiders:extension/GitHub.copilot]",
67+
},
68+
{
69+
name: "Implicit `https` URL works",
70+
url: "github.com",
71+
launcher: fmt.Sprintf("%q -test.run=TestHelperProcess -- implicit https", os.Args[0]),
72+
expected: "[implicit https https://github.com]",
73+
},
74+
{
75+
name: "Explicit absolute `file://` URL errors",
76+
url: "file:///System/Applications/Calculator.app",
77+
wantErr: true,
78+
},
79+
}
80+
81+
// Setup additional test scenarios covering OS-specific executables and directories
82+
// that should be installed on maintainer workstations and GitHub hosted runners.
83+
switch runtime.GOOS {
84+
case "windows":
85+
tests = append(tests, []browseTest{
86+
{
87+
name: "Explicit absolute Windows file URL errors",
88+
url: `C:\Windows\System32\cmd.exe`,
89+
wantErr: true,
90+
},
91+
{
92+
name: "Explicit absolute Windows directory URL errors",
93+
url: `C:\Windows\System32`,
94+
wantErr: true,
95+
},
96+
}...)
97+
// Default should handle common Unix/Linux scenarios including Mac OS.
98+
default:
99+
tests = append(tests, []browseTest{
100+
{
101+
name: "Implicit absolute Unix/Linux file URL errors",
102+
url: "/bin/bash",
103+
wantErr: true,
104+
},
105+
{
106+
name: "Implicit absolute Unix/Linux directory URL errors",
107+
url: "/bin",
108+
wantErr: true,
109+
},
110+
{
111+
name: "Implicit relative Unix/Linux file URL errors",
112+
url: "poc.command",
113+
setup: func(t *testing.T) error {
114+
// Setup a temporary directory to stage content and execute the test within,
115+
// ensure the test's original working directory is restored after.
116+
cwd, err := os.Getwd()
117+
if err != nil {
118+
return err
119+
}
120+
121+
tempDir := t.TempDir()
122+
err = os.Chdir(tempDir)
123+
if err != nil {
124+
return err
125+
}
126+
127+
t.Cleanup(func() {
128+
_ = os.Chdir(cwd)
129+
})
130+
131+
// Create content for local file URL testing
132+
path := filepath.Join(tempDir, "poc.command")
133+
err = os.WriteFile(path, []byte("#!/bin/bash\necho hello"), 0755)
134+
if err != nil {
135+
return err
136+
}
137+
138+
return nil
139+
},
140+
wantErr: true,
141+
},
142+
{
143+
name: "Implicit relative Unix/Linux directory URL errors",
144+
url: "Fake.app",
145+
setup: func(t *testing.T) error {
146+
// Setup a temporary directory to stage content and execute the test within,
147+
// ensure the test's original working directory is restored after.
148+
cwd, err := os.Getwd()
149+
if err != nil {
150+
return err
151+
}
152+
153+
tempDir := t.TempDir()
154+
err = os.Chdir(tempDir)
155+
if err != nil {
156+
return err
157+
}
158+
159+
t.Cleanup(func() {
160+
_ = os.Chdir(cwd)
161+
})
162+
163+
// Create content for local directory URL testing
164+
path := filepath.Join(tempDir, "Fake.app")
165+
err = os.Mkdir(path, 0755)
166+
if err != nil {
167+
return err
168+
}
169+
170+
path = filepath.Join(path, "poc.command")
171+
err = os.WriteFile(path, []byte("#!/bin/bash\necho hello"), 0755)
172+
if err != nil {
173+
return err
174+
}
175+
176+
return nil
177+
},
178+
wantErr: true,
179+
},
180+
}...)
181+
}
182+
183+
for _, tt := range tests {
184+
t.Run(tt.name, func(t *testing.T) {
185+
if tt.setup != nil {
186+
err := tt.setup(t)
187+
require.NoError(t, err)
188+
}
189+
190+
stdout := &bytes.Buffer{}
191+
stderr := &bytes.Buffer{}
192+
b := Browser{launcher: tt.launcher, stdout: stdout, stderr: stderr}
193+
err := b.browse(tt.url, []string{"GH_WANT_HELPER_PROCESS=1"})
194+
195+
if tt.wantErr {
196+
require.Error(t, err)
197+
} else {
198+
require.NoError(t, err)
199+
assert.Equal(t, tt.expected, stdout.String())
200+
assert.Equal(t, "", stderr.String())
201+
}
202+
})
203+
}
30204
}
31205

32206
func TestResolveLauncher(t *testing.T) {

0 commit comments

Comments
 (0)