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

Fix open redirect vulnerability on login screen #4312

Merged
merged 4 commits into from
Jun 26, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions modules/util/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"strings"

"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/setting"
)

// OptionalBool a boolean that can be "null"
Expand Down Expand Up @@ -78,6 +79,18 @@ func URLJoin(base string, elems ...string) string {
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) {
Copy link
Member

@sapk sapk Jun 28, 2018

Choose a reason for hiding this comment

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

return true
}
return false
}

// Min min of two ints
func Min(a, b int) int {
if a > b {
Expand Down
35 changes: 35 additions & 0 deletions modules/util/util_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ package util
import (
"testing"

"code.gitea.io/gitea/modules/setting"

"github.com/stretchr/testify/assert"
)

Expand Down Expand Up @@ -42,3 +44,36 @@ func TestURLJoin(t *testing.T) {
assert.Equal(t, test.Expected, URLJoin(test.Base, test.Elements...))
}
}

func TestIsExternalURL(t *testing.T) {
setting.Domain = "try.gitea.io"
type test struct {
Expected bool
RawURL string
}
newTest := func(expected bool, rawURL string) test {
return test{Expected: expected, RawURL: rawURL}
}
for _, test := range []test{
newTest(false,
"https://try.gitea.io"),
newTest(true,
"https://example.com/"),
newTest(true,
"//example.com"),
newTest(true,
"http://example.com"),
newTest(false,
"a/"),
Copy link
Member

Choose a reason for hiding this comment

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

Why is this false?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because it is not external

newTest(false,
"https://try.gitea.io/test?param=false"),
newTest(false,
Copy link
Member

Choose a reason for hiding this comment

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

Same here

Copy link
Member Author

Choose a reason for hiding this comment

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

Because it is not external

"test?param=false"),
newTest(false,
"//try.gitea.io/test?param=false"),
newTest(false,
Copy link
Member

Choose a reason for hiding this comment

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

And here

Copy link
Member Author

Choose a reason for hiding this comment

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

Because it is not external

"/hey/hey/hey#3244"),
} {
assert.Equal(t, test.Expected, IsExternalURL(test.RawURL))
}
}
3 changes: 2 additions & 1 deletion routers/user/auth.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import (
"code.gitea.io/gitea/modules/context"
"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/setting"
"code.gitea.io/gitea/modules/util"

"github.com/go-macaron/captcha"
"github.com/markbates/goth"
Expand Down Expand Up @@ -474,7 +475,7 @@ func handleSignInFull(ctx *context.Context, u *models.User, remember bool, obeyR
return setting.AppSubURL + "/"
}

if redirectTo, _ := url.QueryUnescape(ctx.GetCookie("redirect_to")); len(redirectTo) > 0 {
if redirectTo, _ := url.QueryUnescape(ctx.GetCookie("redirect_to")); len(redirectTo) > 0 && !util.IsExternalURL(redirectTo) {
ctx.SetCookie("redirect_to", "", -1, setting.AppSubURL)
if obeyRedirect {
ctx.RedirectToFirst(redirectTo)
Expand Down