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
Changes from 3 commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -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"
@@ -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) {

This comment has been minimized.

Copy link
@sapk

sapk Jun 28, 2018

Member
return true
}
return false
}

// Min min of two ints
func Min(a, b int) int {
if a > b {
@@ -7,6 +7,8 @@ package util
import (
"testing"

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

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

@@ -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/"),

This comment has been minimized.

Copy link
@lafriks

lafriks Jun 25, 2018

Member

Why is this false?

This comment has been minimized.

Copy link
@jonasfranz

jonasfranz Jun 25, 2018

Author Member

Because it is not external

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

This comment has been minimized.

Copy link
@lafriks

lafriks Jun 25, 2018

Member

Same here

This comment has been minimized.

Copy link
@jonasfranz

jonasfranz Jun 25, 2018

Author Member

Because it is not external

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

This comment has been minimized.

Copy link
@lafriks

lafriks Jun 25, 2018

Member

And here

This comment has been minimized.

Copy link
@jonasfranz

jonasfranz Jun 25, 2018

Author Member

Because it is not external

"/hey/hey/hey#3244"),
} {
assert.Equal(t, test.Expected, IsExternalURL(test.RawURL))
}
}
@@ -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"
@@ -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)
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.