Encode segments of gh browse resulting URL (#4663)

- URLs are now always generated without a trailing slash
- Windows-style paths are normalized before processing
- Special characters in branch names are escaped

Co-authored-by: Mislav Marohnić <mislav@github.com>

internal/ghrepo/repo.go

@@ -111,7 +111,9 @@ func IsSame(a, b Interface) bool {
 func GenerateRepoURL(repo Interface, p string, args ...interface{}) string {
 	baseURL := fmt.Sprintf("%s%s/%s", ghinstance.HostPrefix(repo.RepoHost()), repo.RepoOwner(), repo.RepoName())
 	if p != "" {
-		return baseURL + "/" + fmt.Sprintf(p, args...)
+		if path := fmt.Sprintf(p, args...); path != "" {
+			return baseURL + "/" + path
+		}
 	}
 	return baseURL
 }

pkg/cmd/browse/browse.go

@@ -3,6 +3,8 @@ package browse
 import (
 	"fmt"
 	"net/http"
+	"net/url"
+	"path"
 	"path/filepath"
 	"strconv"
 	"strings"
@@ -131,7 +133,7 @@ func runBrowse(opts *BrowseOptions) error {
 	if err != nil {
 		return err
 	}
-	url := ghrepo.GenerateRepoURL(baseRepo, section)
+	url := ghrepo.GenerateRepoURL(baseRepo, "%s", section)
 
 	if opts.NoBrowserFlag {
 		_, err := fmt.Fprintln(opts.IO.Out, url)
@@ -186,9 +188,14 @@ func parseSection(baseRepo ghrepo.Interface, opts *BrowseOptions) (string, error
 		} else {
 			rangeFragment = fmt.Sprintf("L%d", rangeStart)
 		}
-		return fmt.Sprintf("blob/%s/%s?plain=1#%s", branchName, filePath, rangeFragment), nil
+		return fmt.Sprintf("blob/%s/%s?plain=1#%s", escapePath(branchName), escapePath(filePath), rangeFragment), nil
 	}
-	return fmt.Sprintf("tree/%s/%s", branchName, filePath), nil
+	return strings.TrimSuffix(fmt.Sprintf("tree/%s/%s", escapePath(branchName), escapePath(filePath)), "/"), nil
+}
+
+// escapePath URL-encodes special characters but leaves slashes unchanged
+func escapePath(p string) string {
+	return strings.ReplaceAll(url.PathEscape(p), "%2F", "/")
 }
 
 func parseFile(opts BrowseOptions, f string) (p string, start int, end int, err error) {
@@ -202,11 +209,9 @@ func parseFile(opts BrowseOptions, f string) (p string, start int, end int, err
 		return
 	}
 
-	p = parts[0]
-	if !filepath.IsAbs(p) {
-		p = filepath.Clean(filepath.Join(opts.PathFromRepoRoot(), p))
-		// Ensure that a path using \ can be used in a URL
-		p = strings.ReplaceAll(p, "\\", "/")
+	p = filepath.ToSlash(parts[0])
+	if !path.IsAbs(p) {
+		p = path.Join(opts.PathFromRepoRoot(), p)
 		if p == "." || strings.HasPrefix(p, "..") {
 			p = ""
 		}

pkg/cmd/browse/browse_test.go

@@ -2,6 +2,7 @@ package browse
 
 import (
 	"fmt"
+	"io/ioutil"
 	"net/http"
 	"os"
 	"path/filepath"
@@ -125,6 +126,8 @@ func TestNewCmdBrowse(t *testing.T) {
 			argv, err := shlex.Split(tt.cli)
 			assert.NoError(t, err)
 			cmd.SetArgs(argv)
+			cmd.SetOut(ioutil.Discard)
+			cmd.SetErr(ioutil.Discard)
 			_, err = cmd.ExecuteC()
 
 			if tt.wantsErr {
@@ -217,7 +220,7 @@ func Test_runBrowse(t *testing.T) {
 				Branch: "trunk",
 			},
 			baseRepo:    ghrepo.New("jlsestak", "CouldNotThinkOfARepoName"),
-			expectedURL: "https://github.com/jlsestak/CouldNotThinkOfARepoName/tree/trunk/",
+			expectedURL: "https://github.com/jlsestak/CouldNotThinkOfARepoName/tree/trunk",
 		},
 		{
 			name: "branch flag with file",
@@ -235,7 +238,7 @@ func Test_runBrowse(t *testing.T) {
 				PathFromRepoRoot: func() string { return "pkg/dir" },
 			},
 			baseRepo:    ghrepo.New("bstnc", "yeepers"),
-			expectedURL: "https://github.com/bstnc/yeepers/tree/feature-123/",
+			expectedURL: "https://github.com/bstnc/yeepers/tree/feature-123",
 		},
 		{
 			name: "branch flag within dir with .",
@@ -354,7 +357,7 @@ func Test_runBrowse(t *testing.T) {
 			},
 			baseRepo:    ghrepo.New("vilmibm", "gh-user-status"),
 			wantsErr:    false,
-			expectedURL: "https://github.com/vilmibm/gh-user-status/tree/6f1a2405cace1633d89a79c74c65f22fe78f9659/",
+			expectedURL: "https://github.com/vilmibm/gh-user-status/tree/6f1a2405cace1633d89a79c74c65f22fe78f9659",
 		},
 		{
 			name: "open last commit with a file",
@@ -392,6 +395,16 @@ func Test_runBrowse(t *testing.T) {
 			expectedURL:   "https://github.com/bchadwic/gh-graph/tree/trunk/pkg/cmd/pr",
 			wantsErr:      false,
 		},
+		{
+			name: "use special characters in selector arg",
+			opts: BrowseOptions{
+				SelectorArg: "?=hello world/ *:23-44",
+				Branch:      "branch/with spaces?",
+			},
+			baseRepo:    ghrepo.New("bchadwic", "test"),
+			expectedURL: "https://github.com/bchadwic/test/blob/branch/with%20spaces%3F/%3F=hello%20world/%20%2A?plain=1#L23-L44",
+			wantsErr:    false,
+		},
 	}
 
 	for _, tt := range tests {
@@ -439,60 +452,88 @@ func Test_runBrowse(t *testing.T) {
 }
 
 func Test_parsePathFromFileArg(t *testing.T) {
-	s := string(os.PathSeparator)
 	tests := []struct {
 		name         string
+		currentDir   string
 		fileArg      string
 		expectedPath string
 	}{
+		{
+			name:         "empty paths",
+			currentDir:   "",
+			fileArg:      "",
+			expectedPath: "",
+		},
+		{
+			name:         "root directory",
+			currentDir:   "",
+			fileArg:      ".",
+			expectedPath: "",
+		},
+		{
+			name:         "relative path",
+			currentDir:   "",
+			fileArg:      filepath.FromSlash("foo/bar.py"),
+			expectedPath: "foo/bar.py",
+		},
 		{
 			name:         "go to parent folder",
-			fileArg:      ".." + s,
+			currentDir:   "pkg/cmd/browse/",
+			fileArg:      filepath.FromSlash("../"),
 			expectedPath: "pkg/cmd",
 		},
 		{
 			name:         "current folder",
+			currentDir:   "pkg/cmd/browse/",
 			fileArg:      ".",
 			expectedPath: "pkg/cmd/browse",
 		},
 		{
 			name:         "current folder (alternative)",
-			fileArg:      "." + s,
+			currentDir:   "pkg/cmd/browse/",
+			fileArg:      filepath.FromSlash("./"),
 			expectedPath: "pkg/cmd/browse",
 		},
 		{
 			name:         "file that starts with '.'",
+			currentDir:   "pkg/cmd/browse/",
 			fileArg:      ".gitignore",
 			expectedPath: "pkg/cmd/browse/.gitignore",
 		},
 		{
 			name:         "file in current folder",
+			currentDir:   "pkg/cmd/browse/",
 			fileArg:      filepath.Join(".", "browse.go"),
 			expectedPath: "pkg/cmd/browse/browse.go",
 		},
 		{
 			name:         "file within parent folder",
+			currentDir:   "pkg/cmd/browse/",
 			fileArg:      filepath.Join("..", "browse.go"),
 			expectedPath: "pkg/cmd/browse.go",
 		},
 		{
 			name:         "file within parent folder uncleaned",
-			fileArg:      filepath.Join("..", ".") + s + s + s + "browse.go",
+			currentDir:   "pkg/cmd/browse/",
+			fileArg:      filepath.FromSlash(".././//browse.go"),
 			expectedPath: "pkg/cmd/browse.go",
 		},
 		{
 			name:         "different path from root directory",
+			currentDir:   "pkg/cmd/browse/",
 			fileArg:      filepath.Join("..", "..", "..", "internal/build/build.go"),
 			expectedPath: "internal/build/build.go",
 		},
 		{
 			name:         "go out of repository",
-			fileArg:      filepath.Join("..", "..", "..", "..", "..", "..") + s + "",
+			currentDir:   "pkg/cmd/browse/",
+			fileArg:      filepath.FromSlash("../../../../../../"),
 			expectedPath: "",
 		},
 		{
 			name:         "go to root of repository",
-			fileArg:      filepath.Join("..", "..", "..") + s + "",
+			currentDir:   "pkg/cmd/browse/",
+			fileArg:      filepath.Join("../../../"),
 			expectedPath: "",
 		},
 		{
@@ -504,7 +545,7 @@ func Test_parsePathFromFileArg(t *testing.T) {
 	for _, tt := range tests {
 		path, _, _, _ := parseFile(BrowseOptions{
 			PathFromRepoRoot: func() string {
-				return "pkg/cmd/browse/"
+				return tt.currentDir
 			}}, tt.fileArg)
 		assert.Equal(t, tt.expectedPath, path, tt.name)
 	}