Clean up various use of escape/unescape functions for URL generation (#6334)

* Use PathUnescape instead of QueryUnescape when working with branch names

Currently branch names with a '+' fail in certain situations because
QueryUnescape replaces the + character with a blank space.

Using PathUnescape should be better since it is defined as:

// PathUnescape is identical to QueryUnescape except that it does not
// unescape '+' to ' ' (space).

Fixes #6333

* Change error to match new function name

* Add new util function PathEscapeSegments

This function simply runs PathEscape on each segment of a path without
touching the forward slash itself. We want to use this instead of
PathEscape/QueryEscape in most cases because a forward slash is a valid name for a
branch etc... and we don't want that escaped in a URL.

Putting this in new file url.go and also moving a couple similar
functions into that file as well.

* Use EscapePathSegments where appropriate

Replace various uses of EscapePath/EscapeQuery with new
EscapePathSegments. Also remove uncessary uses of various
escape/unescape functions when the text had already been escaped or was
not escaped.

* Reformat comment to make drone build happy

* Remove no longer used url library

* Requested code changes

cmd/hook.go

@@ -8,7 +8,6 @@ import (
 	"bufio"
 	"bytes"
 	"fmt"
-	"net/url"
 	"os"
 	"strconv"
 	"strings"
@@ -18,6 +17,7 @@ import (
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/private"
 	"code.gitea.io/gitea/modules/setting"
+	"code.gitea.io/gitea/modules/util"
 
 	"github.com/urfave/cli"
 )
@@ -239,7 +239,7 @@ func runHookPostReceive(c *cli.Context) error {
 					branch = fmt.Sprintf("%s:%s", repo.OwnerName, branch)
 				}
 				fmt.Fprintf(os.Stderr, "Create a new pull request for '%s':\n", branch)
-				fmt.Fprintf(os.Stderr, "  %s/compare/%s...%s\n", baseRepo.HTMLURL(), url.QueryEscape(baseRepo.DefaultBranch), url.QueryEscape(branch))
+				fmt.Fprintf(os.Stderr, "  %s/compare/%s...%s\n", baseRepo.HTMLURL(), util.PathEscapeSegments(baseRepo.DefaultBranch), util.PathEscapeSegments(branch))
 			} else {
 				fmt.Fprint(os.Stderr, "Visit the existing pull request:\n")
 				fmt.Fprintf(os.Stderr, "  %s/pulls/%d\n", baseRepo.HTMLURL(), pr.Index)

integrations/internal_test.go

@@ -8,17 +8,17 @@ import (
 	"encoding/json"
 	"fmt"
 	"net/http"
-	"net/url"
 	"testing"
 
 	"code.gitea.io/gitea/models"
 	"code.gitea.io/gitea/modules/setting"
+	"code.gitea.io/gitea/modules/util"
 
 	"github.com/stretchr/testify/assert"
 )
 
 func assertProtectedBranch(t *testing.T, repoID int64, branchName string, isErr, canPush bool) {
-	reqURL := fmt.Sprintf("/api/internal/branch/%d/%s", repoID, url.QueryEscape(branchName))
+	reqURL := fmt.Sprintf("/api/internal/branch/%d/%s", repoID, util.PathEscapeSegments(branchName))
 	req := NewRequest(t, "GET", reqURL)
 	t.Log(reqURL)
 	req.Header.Add("Authorization", fmt.Sprintf("Bearer %s", setting.InternalToken))

models/repo.go

@@ -838,7 +838,7 @@ type CloneLink struct {
 
 // ComposeHTTPSCloneURL returns HTTPS clone URL based on given owner and repository name.
 func ComposeHTTPSCloneURL(owner, repo string) string {
-	return fmt.Sprintf("%s%s/%s.git", setting.AppURL, url.QueryEscape(owner), url.QueryEscape(repo))
+	return fmt.Sprintf("%s%s/%s.git", setting.AppURL, url.PathEscape(owner), url.PathEscape(repo))
 }
 
 func (repo *Repository) cloneLink(e Engine, isWiki bool) *CloneLink {

modules/context/auth.go

@@ -5,8 +5,6 @@
 package context
 
 import (
-	"net/url"
-
 	"code.gitea.io/gitea/modules/auth"
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/setting"
@@ -48,7 +46,7 @@ func Toggle(options *ToggleOptions) macaron.Handler {
 				if ctx.Req.URL.Path != "/user/settings/change_password" {
 					ctx.Data["Title"] = ctx.Tr("auth.must_change_password")
 					ctx.Data["ChangePasscodeLink"] = setting.AppSubURL + "/user/change_password"
-					ctx.SetCookie("redirect_to", url.QueryEscape(setting.AppSubURL+ctx.Req.RequestURI), 0, setting.AppSubURL)
+					ctx.SetCookie("redirect_to", setting.AppSubURL+ctx.Req.RequestURI, 0, setting.AppSubURL)
 					ctx.Redirect(setting.AppSubURL + "/user/settings/change_password")
 					return
 				}
@@ -82,7 +80,7 @@ func Toggle(options *ToggleOptions) macaron.Handler {
 					return
 				}
 
-				ctx.SetCookie("redirect_to", url.QueryEscape(setting.AppSubURL+ctx.Req.RequestURI), 0, setting.AppSubURL)
+				ctx.SetCookie("redirect_to", setting.AppSubURL+ctx.Req.RequestURI, 0, setting.AppSubURL)
 				ctx.Redirect(setting.AppSubURL + "/user/login")
 				return
 			} else if !ctx.User.IsActive && setting.Service.RegisterEmailConfirm {
@@ -95,7 +93,7 @@ func Toggle(options *ToggleOptions) macaron.Handler {
 		// Redirect to log in page if auto-signin info is provided and has not signed in.
 		if !options.SignOutRequired && !ctx.IsSigned && !auth.IsAPIPath(ctx.Req.URL.Path) &&
 			len(ctx.GetCookie(setting.CookieUserName)) > 0 {
-			ctx.SetCookie("redirect_to", url.QueryEscape(setting.AppSubURL+ctx.Req.RequestURI), 0, setting.AppSubURL)
+			ctx.SetCookie("redirect_to", setting.AppSubURL+ctx.Req.RequestURI, 0, setting.AppSubURL)
 			ctx.Redirect(setting.AppSubURL + "/user/login")
 			return
 		}

modules/context/context.go

@@ -19,6 +19,7 @@ import (
 	"code.gitea.io/gitea/modules/base"
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/setting"
+	"code.gitea.io/gitea/modules/util"
 	"github.com/Unknwon/com"
 	"github.com/go-macaron/cache"
 	"github.com/go-macaron/csrf"
@@ -211,7 +212,7 @@ func Contexter() macaron.Handler {
 			if err == nil && len(repo.DefaultBranch) > 0 {
 				branchName = repo.DefaultBranch
 			}
-			prefix := setting.AppURL + path.Join(url.QueryEscape(ownerName), url.QueryEscape(repoName), "src", "branch", branchName)
+			prefix := setting.AppURL + path.Join(url.PathEscape(ownerName), url.PathEscape(repoName), "src", "branch", util.PathEscapeSegments(branchName))
 			c.Header().Set("Content-Type", "text/html")
 			c.WriteHeader(http.StatusOK)
 			c.Write([]byte(com.Expand(`<!doctype html>

modules/context/repo.go

@@ -172,7 +172,7 @@ func RetrieveBaseRepo(ctx *Context, repo *models.Repository) {
 
 // ComposeGoGetImport returns go-get-import meta content.
 func ComposeGoGetImport(owner, repo string) string {
-	return path.Join(setting.Domain, setting.AppSubURL, url.QueryEscape(owner), url.QueryEscape(repo))
+	return path.Join(setting.Domain, setting.AppSubURL, url.PathEscape(owner), url.PathEscape(repo))
 }
 
 // EarlyResponseForGoGetMeta responses appropriate go-get meta with status 200

modules/private/branch.go

@@ -7,17 +7,17 @@ package private
 import (
 	"encoding/json"
 	"fmt"
-	"net/url"
 
 	"code.gitea.io/gitea/models"
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/setting"
+	"code.gitea.io/gitea/modules/util"
 )
 
 // GetProtectedBranchBy get protected branch information
 func GetProtectedBranchBy(repoID int64, branchName string) (*models.ProtectedBranch, error) {
 	// Ask for running deliver hook and test pull request tasks.
-	reqURL := setting.LocalURL + fmt.Sprintf("api/internal/branch/%d/%s", repoID, url.PathEscape(branchName))
+	reqURL := setting.LocalURL + fmt.Sprintf("api/internal/branch/%d/%s", repoID, util.PathEscapeSegments(branchName))
 	log.GitLogger.Trace("GetProtectedBranchBy: %s", reqURL)
 
 	resp, err := newInternalRequest(reqURL, "GET").Response()

modules/util/url.go

@@ -0,0 +1,59 @@
+// Copyright 2019 The Gitea Authors. All rights reserved.
+// Use of this source code is governed by a MIT-style
+// license that can be found in the LICENSE file.
+
+package util
+
+import (
+	"net/url"
+	"path"
+	"strings"
+
+	"code.gitea.io/gitea/modules/log"
+	"code.gitea.io/gitea/modules/setting"
+)
+
+// PathEscapeSegments escapes segments of a path while not escaping forward slash
+func PathEscapeSegments(path string) string {
+	slice := strings.Split(path, "/")
+	for index := range slice {
+		slice[index] = url.PathEscape(slice[index])
+	}
+	escapedPath := strings.Join(slice, "/")
+	return escapedPath
+}
+
+// URLJoin joins url components, like path.Join, but preserving contents
+func URLJoin(base string, elems ...string) string {
+	if !strings.HasSuffix(base, "/") {
+		base += "/"
+	}
+	baseURL, err := url.Parse(base)
+	if err != nil {
+		log.Error(4, "URLJoin: Invalid base URL %s", base)
+		return ""
+	}
+	joinedPath := path.Join(elems...)
+	argURL, err := url.Parse(joinedPath)
+	if err != nil {
+		log.Error(4, "URLJoin: Invalid arg %s", joinedPath)
+		return ""
+	}
+	joinedURL := baseURL.ResolveReference(argURL).String()
+	if !baseURL.IsAbs() && !strings.HasPrefix(base, "/") {
+		return joinedURL[1:] // Removing leading '/' if needed
+	}
+	return joinedURL
+}
+
+// IsExternalURL checks if rawURL points to an external URL like http://example.com
+func IsExternalURL(rawURL string) bool {
+	parsed, err := url.Parse(rawURL)
+	if err != nil {
+		return true
+	}
+	if len(parsed.Host) != 0 && strings.Replace(parsed.Host, "www.", "", 1) != strings.Replace(setting.Domain, "www.", "", 1) {
+		return true
+	}
+	return false
+}

modules/util/util.go

@@ -5,12 +5,7 @@
 package util
 
 import (
-	"net/url"
-	"path"
 	"strings"
-
-	"code.gitea.io/gitea/modules/log"
-	"code.gitea.io/gitea/modules/setting"
 )
 
 // OptionalBool a boolean that can be "null"
@@ -56,41 +51,6 @@ func Max(a, b int) int {
 	return a
 }
 
-// URLJoin joins url components, like path.Join, but preserving contents
-func URLJoin(base string, elems ...string) string {
-	if !strings.HasSuffix(base, "/") {
-		base += "/"
-	}
-	baseURL, err := url.Parse(base)
-	if err != nil {
-		log.Error(4, "URLJoin: Invalid base URL %s", base)
-		return ""
-	}
-	joinedPath := path.Join(elems...)
-	argURL, err := url.Parse(joinedPath)
-	if err != nil {
-		log.Error(4, "URLJoin: Invalid arg %s", joinedPath)
-		return ""
-	}
-	joinedURL := baseURL.ResolveReference(argURL).String()
-	if !baseURL.IsAbs() && !strings.HasPrefix(base, "/") {
-		return joinedURL[1:] // Removing leading '/' if needed
-	}
-	return joinedURL
-}
-
-// IsExternalURL checks if rawURL points to an external URL like http://example.com
-func IsExternalURL(rawURL string) bool {
-	parsed, err := url.Parse(rawURL)
-	if err != nil {
-		return true
-	}
-	if len(parsed.Host) != 0 && strings.Replace(parsed.Host, "www.", "", 1) != strings.Replace(setting.Domain, "www.", "", 1) {
-		return true
-	}
-	return false
-}
-
 // Min min of two ints
 func Min(a, b int) int {
 	if a > b {

routers/home.go

@@ -7,7 +7,6 @@ package routers
 
 import (
 	"bytes"
-	"net/url"
 	"strings"
 
 	"code.gitea.io/gitea/models"
@@ -48,7 +47,7 @@ func Home(ctx *context.Context) {
 		} else if ctx.User.MustChangePassword {
 			ctx.Data["Title"] = ctx.Tr("auth.must_change_password")
 			ctx.Data["ChangePasscodeLink"] = setting.AppSubURL + "/user/change_password"
-			ctx.SetCookie("redirect_to", url.QueryEscape(setting.AppSubURL+ctx.Req.RequestURI), 0, setting.AppSubURL)
+			ctx.SetCookie("redirect_to", setting.AppSubURL+ctx.Req.RequestURI, 0, setting.AppSubURL)
 			ctx.Redirect(setting.AppSubURL + "/user/settings/change_password")
 		} else {
 			user.Dashboard(ctx)

routers/private/repository.go

@@ -6,7 +6,6 @@ package private
 
 import (
 	"net/http"
-	"net/url"
 
 	"code.gitea.io/gitea/models"
 
@@ -56,18 +55,18 @@ func GetRepository(ctx *macaron.Context) {
 func GetActivePullRequest(ctx *macaron.Context) {
 	baseRepoID := ctx.QueryInt64("baseRepoID")
 	headRepoID := ctx.QueryInt64("headRepoID")
-	baseBranch, err := url.QueryUnescape(ctx.QueryTrim("baseBranch"))
+	baseBranch := ctx.QueryTrim("baseBranch")
-	if err != nil {
+	if len(baseBranch) == 0 {
 		ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
-			"err": err.Error(),
+			"err": "QueryTrim failed",
 		})
 		return
 	}
 
-	headBranch, err := url.QueryUnescape(ctx.QueryTrim("headBranch"))
+	headBranch := ctx.QueryTrim("headBranch")
-	if err != nil {
+	if len(headBranch) == 0 {
 		ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
-			"err": err.Error(),
+			"err": "QueryTrim failed",
 		})
 		return
 	}

routers/repo/pull.go

@@ -10,7 +10,6 @@ import (
 	"container/list"
 	"fmt"
 	"io"
-	"net/url"
 	"path"
 	"strings"
 
@@ -633,10 +632,7 @@ func ParseCompareInfo(ctx *context.Context) (*models.User, *models.Repository, *
 		infoPath   string
 		err        error
 	)
-	infoPath, err = url.QueryUnescape(ctx.Params("*"))
+	infoPath = ctx.Params("*")
-	if err != nil {
-		ctx.NotFound("QueryUnescape", err)
-	}
 	infos := strings.Split(infoPath, "...")
 	if len(infos) != 2 {
 		log.Trace("ParseCompareInfo[%d]: not enough compared branches information %s", baseRepo.ID, infos)