diff --git a/modules/httplib/url.go b/modules/httplib/url.go index 14b95898f5b5e..ebd34db21524b 100644 --- a/modules/httplib/url.go +++ b/modules/httplib/url.go @@ -8,20 +8,35 @@ import ( "strings" "code.gitea.io/gitea/modules/setting" + "code.gitea.io/gitea/modules/util" ) -// IsRiskyRedirectURL returns true if the URL is considered risky for redirects -func IsRiskyRedirectURL(s string) bool { +func detectRelativeURL(s string, u *url.URL) bool { // Unfortunately browsers consider a redirect Location with preceding "//", "\\", "/\" and "\/" as meaning redirect to "http(s)://REST_OF_PATH" // Therefore we should ignore these redirect locations to prevent open redirects if len(s) > 1 && (s[0] == '/' || s[0] == '\\') && (s[1] == '/' || s[1] == '\\') { - return true + return false + } + if u.Path != "" { + u.Path = "/" + util.PathJoinRelX(u.Path) } + return u != nil && u.Scheme == "" && u.Host == "" && + (u.Path == "" || strings.HasPrefix(strings.ToLower(u.Path+"/"), strings.ToLower(setting.AppSubURL+"/"))) +} + +// IsRelativeURL detects if a URL is relative (no scheme or host) +func IsRelativeURL(s string) bool { + u, err := url.Parse(s) + return err == nil && detectRelativeURL(s, u) +} +func IsCurrentGiteaSiteURL(s string) bool { u, err := url.Parse(s) - if err != nil || ((u.Scheme != "" || u.Host != "") && !strings.HasPrefix(strings.ToLower(s), strings.ToLower(setting.AppURL))) { + if err != nil { + return false + } + if detectRelativeURL(s, u) { return true } - - return false + return strings.HasPrefix(strings.ToLower(u.String()+"/"), strings.ToLower(setting.AppURL)) } diff --git a/modules/httplib/url_test.go b/modules/httplib/url_test.go index 72033b1208cc4..a5c1b6d66bfc9 100644 --- a/modules/httplib/url_test.go +++ b/modules/httplib/url_test.go @@ -7,32 +7,59 @@ import ( "testing" "code.gitea.io/gitea/modules/setting" + "code.gitea.io/gitea/modules/test" "github.com/stretchr/testify/assert" ) -func TestIsRiskyRedirectURL(t *testing.T) { - setting.AppURL = "http://localhost:3000/" - tests := []struct { - input string - want bool - }{ - {"", false}, - {"foo", false}, - {"/", false}, - {"/foo?k=%20#abc", false}, +func TestIsRelativeURL(t *testing.T) { + rel := []string{ + "", + "foo", + "/", + "/foo?k=%20#abc", + } + for _, s := range rel { + assert.True(t, IsRelativeURL(s), "rel = %q", s) + } + abs := []string{ + "//", + "\\\\", + "/\\", + "\\/", + "mailto:a@b.com", + "https://test.com", + } + for _, s := range abs { + assert.False(t, IsRelativeURL(s), "abs = %q", s) + } +} - {"//", true}, - {"\\\\", true}, - {"/\\", true}, - {"\\/", true}, - {"mail:a@b.com", true}, - {"https://test.com", true}, - {setting.AppURL + "/foo", false}, - } - for _, tt := range tests { - t.Run(tt.input, func(t *testing.T) { - assert.Equal(t, tt.want, IsRiskyRedirectURL(tt.input)) - }) +func TestIsCurrentGiteaSiteURL(t *testing.T) { + defer test.MockVariableValue(&setting.AppURL, "http://localhost:3000/sub/")() + defer test.MockVariableValue(&setting.AppSubURL, "/sub")() + good := []string{ + "?key=val", + "/sub", + "/sub/", + "/sub/foo", + "/sub/foo/", + "http://localhost:3000/sub", + "http://localhost:3000/sub/", + } + for _, s := range good { + assert.True(t, IsCurrentGiteaSiteURL(s), "good = %q", s) + } + bad := []string{ + "/", + "//", + "\\\\", + "/foo", + "http://localhost:3000/sub/..", + "http://localhost:3000/other", + "http://other/", + } + for _, s := range bad { + assert.False(t, IsCurrentGiteaSiteURL(s), "bad = %q", s) } } diff --git a/routers/common/redirect.go b/routers/common/redirect.go index 9bf2025e19e7b..34044e814bc61 100644 --- a/routers/common/redirect.go +++ b/routers/common/redirect.go @@ -17,7 +17,7 @@ func FetchRedirectDelegate(resp http.ResponseWriter, req *http.Request) { // The typical page is "issue comment" page. The backend responds "/owner/repo/issues/1#comment-2", // then frontend needs this delegate to redirect to the new location with hash correctly. redirect := req.PostFormValue("redirect") - if httplib.IsRiskyRedirectURL(redirect) { + if !httplib.IsCurrentGiteaSiteURL(redirect) { resp.WriteHeader(http.StatusBadRequest) return } diff --git a/services/context/context_response.go b/services/context/context_response.go index 372b4cb38b6f6..838f42954a707 100644 --- a/services/context/context_response.go +++ b/services/context/context_response.go @@ -51,7 +51,7 @@ func (ctx *Context) RedirectToFirst(location ...string) { continue } - if httplib.IsRiskyRedirectURL(loc) { + if !httplib.IsCurrentGiteaSiteURL(loc) { continue }