Skip to content

Commit ca46385

Browse files
mrsdizzietechknowlogick
mrsdizzie
authored and
techknowlogick
committed
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
1 parent c151682 commit ca46385

File tree

12 files changed

+80
-68
lines changed

12 files changed

+80
-68
lines changed

‎cmd/hook.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@ import (
88
"bufio"
99
"bytes"
1010
"fmt"
11-
"net/url"
1211
"os"
1312
"strconv"
1413
"strings"
@@ -18,6 +17,7 @@ import (
1817
"code.gitea.io/gitea/modules/log"
1918
"code.gitea.io/gitea/modules/private"
2019
"code.gitea.io/gitea/modules/setting"
20+
"code.gitea.io/gitea/modules/util"
2121

2222
"github.com/urfave/cli"
2323
)
@@ -239,7 +239,7 @@ func runHookPostReceive(c *cli.Context) error {
239239
branch = fmt.Sprintf("%s:%s", repo.OwnerName, branch)
240240
}
241241
fmt.Fprintf(os.Stderr, "Create a new pull request for '%s':\n", branch)
242-
fmt.Fprintf(os.Stderr, " %s/compare/%s...%s\n", baseRepo.HTMLURL(), url.QueryEscape(baseRepo.DefaultBranch), url.QueryEscape(branch))
242+
fmt.Fprintf(os.Stderr, " %s/compare/%s...%s\n", baseRepo.HTMLURL(), util.PathEscapeSegments(baseRepo.DefaultBranch), util.PathEscapeSegments(branch))
243243
} else {
244244
fmt.Fprint(os.Stderr, "Visit the existing pull request:\n")
245245
fmt.Fprintf(os.Stderr, " %s/pulls/%d\n", baseRepo.HTMLURL(), pr.Index)

‎integrations/internal_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,17 +8,17 @@ import (
88
"encoding/json"
99
"fmt"
1010
"net/http"
11-
"net/url"
1211
"testing"
1312

1413
"code.gitea.io/gitea/models"
1514
"code.gitea.io/gitea/modules/setting"
15+
"code.gitea.io/gitea/modules/util"
1616

1717
"github.com/stretchr/testify/assert"
1818
)
1919

2020
func assertProtectedBranch(t *testing.T, repoID int64, branchName string, isErr, canPush bool) {
21-
reqURL := fmt.Sprintf("/api/internal/branch/%d/%s", repoID, url.QueryEscape(branchName))
21+
reqURL := fmt.Sprintf("/api/internal/branch/%d/%s", repoID, util.PathEscapeSegments(branchName))
2222
req := NewRequest(t, "GET", reqURL)
2323
t.Log(reqURL)
2424
req.Header.Add("Authorization", fmt.Sprintf("Bearer %s", setting.InternalToken))

‎models/repo.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -838,7 +838,7 @@ type CloneLink struct {
838838

839839
// ComposeHTTPSCloneURL returns HTTPS clone URL based on given owner and repository name.
840840
func ComposeHTTPSCloneURL(owner, repo string) string {
841-
return fmt.Sprintf("%s%s/%s.git", setting.AppURL, url.QueryEscape(owner), url.QueryEscape(repo))
841+
return fmt.Sprintf("%s%s/%s.git", setting.AppURL, url.PathEscape(owner), url.PathEscape(repo))
842842
}
843843

844844
func (repo *Repository) cloneLink(e Engine, isWiki bool) *CloneLink {

‎modules/context/auth.go

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,8 +5,6 @@
55
package context
66

77
import (
8-
"net/url"
9-
108
"code.gitea.io/gitea/modules/auth"
119
"code.gitea.io/gitea/modules/log"
1210
"code.gitea.io/gitea/modules/setting"
@@ -48,7 +46,7 @@ func Toggle(options *ToggleOptions) macaron.Handler {
4846
if ctx.Req.URL.Path != "/user/settings/change_password" {
4947
ctx.Data["Title"] = ctx.Tr("auth.must_change_password")
5048
ctx.Data["ChangePasscodeLink"] = setting.AppSubURL + "/user/change_password"
51-
ctx.SetCookie("redirect_to", url.QueryEscape(setting.AppSubURL+ctx.Req.RequestURI), 0, setting.AppSubURL)
49+
ctx.SetCookie("redirect_to", setting.AppSubURL+ctx.Req.RequestURI, 0, setting.AppSubURL)
5250
ctx.Redirect(setting.AppSubURL + "/user/settings/change_password")
5351
return
5452
}
@@ -82,7 +80,7 @@ func Toggle(options *ToggleOptions) macaron.Handler {
8280
return
8381
}
8482

85-
ctx.SetCookie("redirect_to", url.QueryEscape(setting.AppSubURL+ctx.Req.RequestURI), 0, setting.AppSubURL)
83+
ctx.SetCookie("redirect_to", setting.AppSubURL+ctx.Req.RequestURI, 0, setting.AppSubURL)
8684
ctx.Redirect(setting.AppSubURL + "/user/login")
8785
return
8886
} else if !ctx.User.IsActive && setting.Service.RegisterEmailConfirm {
@@ -95,7 +93,7 @@ func Toggle(options *ToggleOptions) macaron.Handler {
9593
// Redirect to log in page if auto-signin info is provided and has not signed in.
9694
if !options.SignOutRequired && !ctx.IsSigned && !auth.IsAPIPath(ctx.Req.URL.Path) &&
9795
len(ctx.GetCookie(setting.CookieUserName)) > 0 {
98-
ctx.SetCookie("redirect_to", url.QueryEscape(setting.AppSubURL+ctx.Req.RequestURI), 0, setting.AppSubURL)
96+
ctx.SetCookie("redirect_to", setting.AppSubURL+ctx.Req.RequestURI, 0, setting.AppSubURL)
9997
ctx.Redirect(setting.AppSubURL + "/user/login")
10098
return
10199
}

‎modules/context/context.go

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import (
1919
"code.gitea.io/gitea/modules/base"
2020
"code.gitea.io/gitea/modules/log"
2121
"code.gitea.io/gitea/modules/setting"
22+
"code.gitea.io/gitea/modules/util"
2223
"github.com/Unknwon/com"
2324
"github.com/go-macaron/cache"
2425
"github.com/go-macaron/csrf"
@@ -211,7 +212,7 @@ func Contexter() macaron.Handler {
211212
if err == nil && len(repo.DefaultBranch) > 0 {
212213
branchName = repo.DefaultBranch
213214
}
214-
prefix := setting.AppURL + path.Join(url.QueryEscape(ownerName), url.QueryEscape(repoName), "src", "branch", branchName)
215+
prefix := setting.AppURL + path.Join(url.PathEscape(ownerName), url.PathEscape(repoName), "src", "branch", util.PathEscapeSegments(branchName))
215216
c.Header().Set("Content-Type", "text/html")
216217
c.WriteHeader(http.StatusOK)
217218
c.Write([]byte(com.Expand(`<!doctype html>

‎modules/context/repo.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -172,7 +172,7 @@ func RetrieveBaseRepo(ctx *Context, repo *models.Repository) {
172172

173173
// ComposeGoGetImport returns go-get-import meta content.
174174
func ComposeGoGetImport(owner, repo string) string {
175-
return path.Join(setting.Domain, setting.AppSubURL, url.QueryEscape(owner), url.QueryEscape(repo))
175+
return path.Join(setting.Domain, setting.AppSubURL, url.PathEscape(owner), url.PathEscape(repo))
176176
}
177177

178178
// EarlyResponseForGoGetMeta responses appropriate go-get meta with status 200

‎modules/private/branch.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,17 +7,17 @@ package private
77
import (
88
"encoding/json"
99
"fmt"
10-
"net/url"
1110

1211
"code.gitea.io/gitea/models"
1312
"code.gitea.io/gitea/modules/log"
1413
"code.gitea.io/gitea/modules/setting"
14+
"code.gitea.io/gitea/modules/util"
1515
)
1616

1717
// GetProtectedBranchBy get protected branch information
1818
func GetProtectedBranchBy(repoID int64, branchName string) (*models.ProtectedBranch, error) {
1919
// Ask for running deliver hook and test pull request tasks.
20-
reqURL := setting.LocalURL + fmt.Sprintf("api/internal/branch/%d/%s", repoID, url.PathEscape(branchName))
20+
reqURL := setting.LocalURL + fmt.Sprintf("api/internal/branch/%d/%s", repoID, util.PathEscapeSegments(branchName))
2121
log.GitLogger.Trace("GetProtectedBranchBy: %s", reqURL)
2222

2323
resp, err := newInternalRequest(reqURL, "GET").Response()

‎modules/util/url.go

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
// Copyright 2019 The Gitea Authors. All rights reserved.
2+
// Use of this source code is governed by a MIT-style
3+
// license that can be found in the LICENSE file.
4+
5+
package util
6+
7+
import (
8+
"net/url"
9+
"path"
10+
"strings"
11+
12+
"code.gitea.io/gitea/modules/log"
13+
"code.gitea.io/gitea/modules/setting"
14+
)
15+
16+
// PathEscapeSegments escapes segments of a path while not escaping forward slash
17+
func PathEscapeSegments(path string) string {
18+
slice := strings.Split(path, "/")
19+
for index := range slice {
20+
slice[index] = url.PathEscape(slice[index])
21+
}
22+
escapedPath := strings.Join(slice, "/")
23+
return escapedPath
24+
}
25+
26+
// URLJoin joins url components, like path.Join, but preserving contents
27+
func URLJoin(base string, elems ...string) string {
28+
if !strings.HasSuffix(base, "/") {
29+
base += "/"
30+
}
31+
baseURL, err := url.Parse(base)
32+
if err != nil {
33+
log.Error(4, "URLJoin: Invalid base URL %s", base)
34+
return ""
35+
}
36+
joinedPath := path.Join(elems...)
37+
argURL, err := url.Parse(joinedPath)
38+
if err != nil {
39+
log.Error(4, "URLJoin: Invalid arg %s", joinedPath)
40+
return ""
41+
}
42+
joinedURL := baseURL.ResolveReference(argURL).String()
43+
if !baseURL.IsAbs() && !strings.HasPrefix(base, "/") {
44+
return joinedURL[1:] // Removing leading '/' if needed
45+
}
46+
return joinedURL
47+
}
48+
49+
// IsExternalURL checks if rawURL points to an external URL like http://example.com
50+
func IsExternalURL(rawURL string) bool {
51+
parsed, err := url.Parse(rawURL)
52+
if err != nil {
53+
return true
54+
}
55+
if len(parsed.Host) != 0 && strings.Replace(parsed.Host, "www.", "", 1) != strings.Replace(setting.Domain, "www.", "", 1) {
56+
return true
57+
}
58+
return false
59+
}

‎modules/util/util.go

Lines changed: 0 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -5,12 +5,7 @@
55
package util
66

77
import (
8-
"net/url"
9-
"path"
108
"strings"
11-
12-
"code.gitea.io/gitea/modules/log"
13-
"code.gitea.io/gitea/modules/setting"
149
)
1510

1611
// OptionalBool a boolean that can be "null"
@@ -56,41 +51,6 @@ func Max(a, b int) int {
5651
return a
5752
}
5853

59-
// URLJoin joins url components, like path.Join, but preserving contents
60-
func URLJoin(base string, elems ...string) string {
61-
if !strings.HasSuffix(base, "/") {
62-
base += "/"
63-
}
64-
baseURL, err := url.Parse(base)
65-
if err != nil {
66-
log.Error(4, "URLJoin: Invalid base URL %s", base)
67-
return ""
68-
}
69-
joinedPath := path.Join(elems...)
70-
argURL, err := url.Parse(joinedPath)
71-
if err != nil {
72-
log.Error(4, "URLJoin: Invalid arg %s", joinedPath)
73-
return ""
74-
}
75-
joinedURL := baseURL.ResolveReference(argURL).String()
76-
if !baseURL.IsAbs() && !strings.HasPrefix(base, "/") {
77-
return joinedURL[1:] // Removing leading '/' if needed
78-
}
79-
return joinedURL
80-
}
81-
82-
// IsExternalURL checks if rawURL points to an external URL like http://example.com
83-
func IsExternalURL(rawURL string) bool {
84-
parsed, err := url.Parse(rawURL)
85-
if err != nil {
86-
return true
87-
}
88-
if len(parsed.Host) != 0 && strings.Replace(parsed.Host, "www.", "", 1) != strings.Replace(setting.Domain, "www.", "", 1) {
89-
return true
90-
}
91-
return false
92-
}
93-
9454
// Min min of two ints
9555
func Min(a, b int) int {
9656
if a > b {

‎routers/home.go

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ package routers
77

88
import (
99
"bytes"
10-
"net/url"
1110
"strings"
1211

1312
"code.gitea.io/gitea/models"
@@ -48,7 +47,7 @@ func Home(ctx *context.Context) {
4847
} else if ctx.User.MustChangePassword {
4948
ctx.Data["Title"] = ctx.Tr("auth.must_change_password")
5049
ctx.Data["ChangePasscodeLink"] = setting.AppSubURL + "/user/change_password"
51-
ctx.SetCookie("redirect_to", url.QueryEscape(setting.AppSubURL+ctx.Req.RequestURI), 0, setting.AppSubURL)
50+
ctx.SetCookie("redirect_to", setting.AppSubURL+ctx.Req.RequestURI, 0, setting.AppSubURL)
5251
ctx.Redirect(setting.AppSubURL + "/user/settings/change_password")
5352
} else {
5453
user.Dashboard(ctx)

‎routers/private/repository.go

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@ package private
66

77
import (
88
"net/http"
9-
"net/url"
109

1110
"code.gitea.io/gitea/models"
1211

@@ -56,18 +55,18 @@ func GetRepository(ctx *macaron.Context) {
5655
func GetActivePullRequest(ctx *macaron.Context) {
5756
baseRepoID := ctx.QueryInt64("baseRepoID")
5857
headRepoID := ctx.QueryInt64("headRepoID")
59-
baseBranch, err := url.QueryUnescape(ctx.QueryTrim("baseBranch"))
60-
if err != nil {
58+
baseBranch := ctx.QueryTrim("baseBranch")
59+
if len(baseBranch) == 0 {
6160
ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
62-
"err": err.Error(),
61+
"err": "QueryTrim failed",
6362
})
6463
return
6564
}
6665

67-
headBranch, err := url.QueryUnescape(ctx.QueryTrim("headBranch"))
68-
if err != nil {
66+
headBranch := ctx.QueryTrim("headBranch")
67+
if len(headBranch) == 0 {
6968
ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
70-
"err": err.Error(),
69+
"err": "QueryTrim failed",
7170
})
7271
return
7372
}

‎routers/repo/pull.go

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@ import (
1010
"container/list"
1111
"fmt"
1212
"io"
13-
"net/url"
1413
"path"
1514
"strings"
1615

@@ -633,10 +632,7 @@ func ParseCompareInfo(ctx *context.Context) (*models.User, *models.Repository, *
633632
infoPath string
634633
err error
635634
)
636-
infoPath, err = url.QueryUnescape(ctx.Params("*"))
637-
if err != nil {
638-
ctx.NotFound("QueryUnescape", err)
639-
}
635+
infoPath = ctx.Params("*")
640636
infos := strings.Split(infoPath, "...")
641637
if len(infos) != 2 {
642638
log.Trace("ParseCompareInfo[%d]: not enough compared branches information %s", baseRepo.ID, infos)

0 commit comments

Comments
 (0)