fix

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))
 }

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)
  }
 }

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
  }

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
  }