From ca463856377cf1ed40f8d8bd6f76739bf3711bb1 Mon Sep 17 00:00:00 2001 From: mrsdizzie Date: Mon, 18 Mar 2019 10:00:23 -0400 Subject: [PATCH] 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 | 4 +-- integrations/internal_test.go | 4 +-- models/repo.go | 2 +- modules/context/auth.go | 8 ++--- modules/context/context.go | 3 +- modules/context/repo.go | 2 +- modules/private/branch.go | 4 +-- modules/util/url.go | 59 +++++++++++++++++++++++++++++++++++ modules/util/util.go | 40 ------------------------ routers/home.go | 3 +- routers/private/repository.go | 13 ++++---- routers/repo/pull.go | 6 +--- 12 files changed, 80 insertions(+), 68 deletions(-) create mode 100644 modules/util/url.go diff --git a/cmd/hook.go b/cmd/hook.go index bde449ccdc..d3a1a56b01 100644 --- a/cmd/hook.go +++ b/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) diff --git a/integrations/internal_test.go b/integrations/internal_test.go index 3c8657e642..ee0c0d18f1 100644 --- a/integrations/internal_test.go +++ b/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)) diff --git a/models/repo.go b/models/repo.go index 2822f7d718..cafde29363 100644 --- a/models/repo.go +++ b/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 { diff --git a/modules/context/auth.go b/modules/context/auth.go index 9d9bd81c75..ca897de6ed 100644 --- a/modules/context/auth.go +++ b/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 } diff --git a/modules/context/context.go b/modules/context/context.go index 5d4a2cad54..5c7254de22 100644 --- a/modules/context/context.go +++ b/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(` diff --git a/modules/context/repo.go b/modules/context/repo.go index 8f2622fa82..e0ee802f7d 100644 --- a/modules/context/repo.go +++ b/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 diff --git a/modules/private/branch.go b/modules/private/branch.go index b6b119e871..bbd0d4b697 100644 --- a/modules/private/branch.go +++ b/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() diff --git a/modules/util/url.go b/modules/util/url.go new file mode 100644 index 0000000000..381e8b935b --- /dev/null +++ b/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 +} diff --git a/modules/util/util.go b/modules/util/util.go index b2067c8df6..4203b5eb51 100644 --- a/modules/util/util.go +++ b/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 { diff --git a/routers/home.go b/routers/home.go index 437c569a79..a9c4774867 100644 --- a/routers/home.go +++ 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) diff --git a/routers/private/repository.go b/routers/private/repository.go index 0769e1f250..9f451bcf1d 100644 --- a/routers/private/repository.go +++ b/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")) - if err != nil { + baseBranch := ctx.QueryTrim("baseBranch") + 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")) - if err != nil { + headBranch := ctx.QueryTrim("headBranch") + if len(headBranch) == 0 { ctx.JSON(http.StatusInternalServerError, map[string]interface{}{ - "err": err.Error(), + "err": "QueryTrim failed", }) return } diff --git a/routers/repo/pull.go b/routers/repo/pull.go index 043fc5c93a..c61ae905f7 100644 --- a/routers/repo/pull.go +++ b/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("*")) - if err != nil { - ctx.NotFound("QueryUnescape", err) - } + infoPath = ctx.Params("*") infos := strings.Split(infoPath, "...") if len(infos) != 2 { log.Trace("ParseCompareInfo[%d]: not enough compared branches information %s", baseRepo.ID, infos)