Skip to content

Commit 258949b

Browse files
andyfellerandyfeller
committed
Test refactor, refine errors, remove VSCode fix
This commit is focused on PR feedback: 1. Undoing the fix for running package tests within VSCode The "Run package tests" action within VSCode uses `-coverprofile` flag when invoking `go test`, which causes `TestBrowse` to error due to missing `GOCOVERDIR` env var. As this was called out as a problem in multiple tests, it felt like a larger problem to tackle outside of these changes. 2. Ensure browser test runs for Linux/Unix separately from Mac OS @babakks called out how the previous version of tests would fail on Linux when testing against Mac OS calculate app. This test has been reworked to separate Mac OS from Unix/Linux tests as well as consolidate the separate build specific test files back into a single file. 3. Improved `error` messages Rather than having a single, uniform error message from the various scenarios, this commit more nuanced error messages based on the different scenarios that are user-friendly. 4. Erring if URL is an executable on the path This is an additional defense in depth effort to guard against URLs that match executables found within the user's path. This leverages similar logic used elsewhere within `gh` for safely finding and executing commands.
1 parent 0f8a22f commit 258949b

4 files changed

Lines changed: 204 additions & 195 deletions

File tree

‎pkg/browser/browser.go‎

Lines changed: 16 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -47,8 +47,8 @@ func (b *Browser) Browse(url string) error {
4747
}
4848

4949
func (b *Browser) browse(url string, env []string) error {
50-
if !isPossibleProtocol(url) {
51-
return fmt.Errorf("cannot browse due to unsupported protocol: %s", url)
50+
if err := isPossibleProtocol(url); err != nil {
51+
return err
5252
}
5353

5454
if b.launcher == "" {
@@ -85,31 +85,35 @@ func resolveLauncher() string {
8585
return os.Getenv("BROWSER")
8686
}
8787

88-
func (b *Browser) IsURL(u string) bool {
89-
return isPossibleProtocol(u)
90-
}
91-
9288
func isSupportedProtocol(u string) bool {
9389
return strings.HasPrefix(u, "http:") ||
9490
strings.HasPrefix(u, "https:") ||
9591
strings.HasPrefix(u, "vscode:") ||
9692
strings.HasPrefix(u, "vscode-insiders:")
9793
}
9894

99-
func isPossibleProtocol(u string) bool {
95+
func isPossibleProtocol(u string) error {
10096
if isSupportedProtocol(u) {
101-
return true
97+
return nil
10298
}
10399

104-
// Disallow URLs using alternative `file:` protocol not located previously
100+
// Disallow URLs using alternative `file:` protocol
105101
if strings.HasPrefix(u, "file:") {
106-
return false
102+
return fmt.Errorf("opening files or directories is unsupported: %s", u)
107103
}
108104

109105
// Disallow URLs that match existing files or directories on the filesystem
106+
// as these could be executables or executed by the launcher browser due to
107+
// the file extension and/or associated application.
110108
if fileInfo, _ := os.Lstat(u); fileInfo != nil {
111-
return false
109+
return fmt.Errorf("opening files or directories is unsupported: %s", u)
110+
}
111+
112+
// Disallow URLs that match executables found in the user path.
113+
exec, _ := safeexec.LookPath(u)
114+
if exec != "" {
115+
return fmt.Errorf("opening executables is unsupported: %s", u)
112116
}
113117

114-
return true
118+
return nil
115119
}

‎pkg/browser/browser_other_test.go‎

Lines changed: 0 additions & 63 deletions
This file was deleted.

0 commit comments

Comments
 (0)