Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 13 commits into from
Mar 18, 2019
Merged
4 changes: 2 additions & 2 deletions cmd/hook.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import (
"bufio"
"bytes"
"fmt"
"net/url"
"os"
"strconv"
"strings"
Expand All @@ -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"
)
Expand Down Expand Up @@ -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)
Expand Down
4 changes: 2 additions & 2 deletions integrations/internal_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down
3 changes: 1 addition & 2 deletions models/repo.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import (
"fmt"
"html/template"
"io/ioutil"
"net/url"
"os"
"os/exec"
"path"
Expand Down Expand Up @@ -838,7 +837,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, util.PathEscapeSegments(owner), util.PathEscapeSegments(repo))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one should be url.PathEscape - although we don't allow / in names at present we might in future

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

}

func (repo *Repository) cloneLink(e Engine, isWiki bool) *CloneLink {
Expand Down
8 changes: 3 additions & 5 deletions modules/context/auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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 {
Expand All @@ -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
}
Expand Down
3 changes: 2 additions & 1 deletion modules/context/context.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -210,7 +211,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(util.PathEscapeSegments(ownerName), util.PathEscapeSegments(repoName), "src", "branch", branchName)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ownerName and repoName should be url.PathEscape and branchName should be escaped with util.PathEscapeSegments

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

c.Header().Set("Content-Type", "text/html")
c.WriteHeader(http.StatusOK)
c.Write([]byte(com.Expand(`<!doctype html>
Expand Down
4 changes: 2 additions & 2 deletions modules/context/repo.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ package context
import (
"fmt"
"io/ioutil"
"net/url"
"path"
"strings"

Expand All @@ -17,6 +16,7 @@ import (
"code.gitea.io/gitea/modules/cache"
"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/setting"
"code.gitea.io/gitea/modules/util"

"github.com/Unknwon/com"
"gopkg.in/editorconfig/editorconfig-core-go.v1"
Expand Down Expand Up @@ -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, util.PathEscapeSegments(owner), util.PathEscapeSegments(repo))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

url.PathEscape

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

}

// EarlyResponseForGoGetMeta responses appropriate go-get meta with status 200
Expand Down
4 changes: 2 additions & 2 deletions modules/private/branch.go
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just check that this doesn't break. I put the PathEscape in recently and it worked for branches with subpaths.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this still works the same and as a bonus makes the logging a tiny bit more clear (though both should work fine here). With just PathEscape, you end up with a log like:

GetProtectedBranchBy: http://localhost:3000/api/internal/branch/3/refs%2Ftags%2Fdebian%2F1%251.6.0-4

And with PathEscapeSegments you end up with:

GetProtectedBranchBy: http://localhost:3000/api/internal/branch/3/refs/tags/debian/1%251.6.0-3

Since we tolerate the forward slashes in a URL anyway the second choice seems preferable for clarity and consistency with how we escape the branchName in other places now.

log.GitLogger.Trace("GetProtectedBranchBy: %s", reqURL)

resp, err := newInternalRequest(reqURL, "GET").Response()
Expand Down
4 changes: 2 additions & 2 deletions modules/private/internal.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,12 @@ import (
"fmt"
"net"
"net/http"
"net/url"

"code.gitea.io/gitea/models"
"code.gitea.io/gitea/modules/httplib"
"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/setting"
"code.gitea.io/gitea/modules/util"
)

func newRequest(url, method string) *httplib.Request {
Expand Down Expand Up @@ -77,7 +77,7 @@ func CheckUnitUser(userID, repoID int64, isAdmin bool, unitType models.UnitType)

// GetRepositoryByOwnerAndName returns the repository by given ownername and reponame.
func GetRepositoryByOwnerAndName(ownerName, repoName string) (*models.Repository, error) {
reqURL := setting.LocalURL + fmt.Sprintf("api/internal/repo/%s/%s", url.PathEscape(ownerName), url.PathEscape(repoName))
reqURL := setting.LocalURL + fmt.Sprintf("api/internal/repo/%s/%s", util.PathEscapeSegments(ownerName), util.PathEscapeSegments(repoName))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

url.PathEscape

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

log.GitLogger.Trace("GetRepositoryByOwnerAndName: %s", reqURL)

resp, err := newInternalRequest(reqURL, "GET").Response()
Expand Down
4 changes: 2 additions & 2 deletions modules/private/repository.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,11 @@ 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"
)

// GetRepository return the repository by its ID and a bool about if it's allowed to have PR
Expand Down Expand Up @@ -44,7 +44,7 @@ func GetRepository(repoID int64) (*models.Repository, bool, error) {

// ActivePullRequest returns an active pull request if it exists
func ActivePullRequest(baseRepoID int64, headRepoID int64, baseBranch, headBranch string) (*models.PullRequest, error) {
reqURL := setting.LocalURL + fmt.Sprintf("api/internal/active-pull-request?baseRepoID=%d&headRepoID=%d&baseBranch=%s&headBranch=%s", baseRepoID, headRepoID, url.QueryEscape(baseBranch), url.QueryEscape(headBranch))
reqURL := setting.LocalURL + fmt.Sprintf("api/internal/active-pull-request?baseRepoID=%d&headRepoID=%d&baseBranch=%s&headBranch=%s", baseRepoID, headRepoID, util.PathEscapeSegments(baseBranch), util.PathEscapeSegments(headBranch))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These ones are actually in a query so url.QueryEscape is the correct thing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

log.GitLogger.Trace("ActivePullRequest: %s", reqURL)

resp, err := newInternalRequest(reqURL, "GET").Response()
Expand Down
59 changes: 59 additions & 0 deletions modules/util/url.go
Original file line number Diff line number Diff line change
@@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A package named util is not a good idea maybe we could change this and begin from this PR?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

util does feel right for something like this but I assume the advice is to not re-use common stdlib names in general? What namespace would be preferred here?

I'm not very good at naming things, perhaps just a generic 'gitea' for these utility type functions that are local to this project

gitea.PathEscapeSegments(myString)

Not very clever but at least obvious to where it comes from. Lemme know what makes sense and I can update this PR.

If the name needs more thought/feedback, I think it would be better to add this PR 'as is' and create a second PR for changing the name of the currently used modules/util since this will fix a few existing bugs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lunny I think it's reasonable to continue using the util module here. We should refactor the module later as a separate pr.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could create a new package named module/url for all url related functions. But It's OK for another PR to do that.


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
}
40 changes: 0 additions & 40 deletions modules/util/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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 {
Expand Down
3 changes: 1 addition & 2 deletions routers/home.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ package routers

import (
"bytes"
"net/url"
"strings"

"code.gitea.io/gitea/models"
Expand Down Expand Up @@ -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)
Expand Down
4 changes: 2 additions & 2 deletions routers/private/repository.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,15 +56,15 @@ 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, err := url.PathUnescape(ctx.QueryTrim("baseBranch"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ctx.QueryTrim will already unescape its contents so you don't actually need to unescape these.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed that call. Included a check if baseBranch is empty to preserve the error handling at this point.

if err != nil {
ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
"err": err.Error(),
})
return
}

headBranch, err := url.QueryUnescape(ctx.QueryTrim("headBranch"))
headBranch, err := url.PathUnescape(ctx.QueryTrim("headBranch"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ctx.QueryTrim will already unescape its contents so you don't actually need to unescape these.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

if err != nil {
ctx.JSON(http.StatusInternalServerError, map[string]interface{}{
"err": err.Error(),
Expand Down
6 changes: 1 addition & 5 deletions routers/repo/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import (
"container/list"
"fmt"
"io"
"net/url"
"path"
"strings"

Expand Down Expand Up @@ -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)
Expand Down