thepaperpilot/forgejo
1
0
Fork 0
Code Issues Pull requests Releases Activity Actions

[PORT] drop utils.IsExternalURL (and expand IsRiskyRedirectURL tests) (#3167)

Browse source 
Related to  #2773
Related to Refactor URL detection [gitea#29960](https://github.com/go-gitea/gitea/pull/29960)
Related to Refactor external URL detection [gitea#29973](https://github.com/go-gitea/gitea/pull/29973)

I added a bunch of tests to `httplib.TestIsRiskyRedirectURL` and some cases should be better handled (however it is not an easy task).

I also ported the removal of `utils.IsExternalURL`, since it prevents duplicated (subtle) code.

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/3167
Reviewed-by: Earl Warren <earl-warren@noreply.codeberg.org>
Reviewed-by: Gusted <gusted@noreply.codeberg.org>
Co-authored-by: oliverpool <git@olivier.pfad.fr>
Co-committed-by: oliverpool <git@olivier.pfad.fr>
This commit is contained in:
oliverpool 2024-04-15 13:03:08 +00:00 committed by Earl Warren
parent 20c0292b5c
commit 16879b07d2
6 changed files with 104 additions and 75 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

91
modules/httplib/url_test.go
View file

@ -7,20 +7,39 @@ 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/"
defer test.MockVariableValue(&setting.AppURL, "http://localhost:3000/sub/")()
defer test.MockVariableValue(&setting.AppSubURL, "/sub")()
tests := []struct {
input string
want bool
}{
{"", false},
{"foo", false},
{"/", false},
{"./", false},
{"?key=val", false},
{"/sub/", false},
{"http://localhost:3000/sub/", false},
{"/sub/foo", false},
{"http://localhost:3000/sub/foo", false},
{"http://localhost:3000/sub/test?param=false", false},
// FIXME: should probably be true (would requires resolving references using setting.appURL.ResolveReference(u))
{"/sub/../", false},
{"http://localhost:3000/sub/../", false},
{"/sUb/", false},
{"http://localhost:3000/sUb/foo", false},
{"/sub", false},
{"/foo?k=%20#abc", false},
{"/", false},
{"a/", false},
{"test?param=false", false},
{"/hey/hey/hey#3244", false},
{"//", true},
{"\\\\", true},
@ -28,7 +47,73 @@ func TestIsRiskyRedirectURL(t *testing.T) {
{"\\/", true},
{"mail:a@b.com", true},
{"https://test.com", true},
{setting.AppURL + "/foo", false},
{"http://localhost:3000/foo", true},
{"http://localhost:3000/sub", true},
{"http://localhost:3000/sub?key=val", true},
{"https://example.com/", true},
{"//example.com", true},
{"http://example.com", true},
{"http://localhost:3000/test?param=false", true},
{"//localhost:3000/test?param=false", true},
{"://missing protocol scheme", true},
// FIXME: should probably be false
{"//localhost:3000/sub/test?param=false", true},
}
for _, tt := range tests {
t.Run(tt.input, func(t *testing.T) {
assert.Equal(t, tt.want, IsRiskyRedirectURL(tt.input))
})
}
}
func TestIsRiskyRedirectURLWithoutSubURL(t *testing.T) {
defer test.MockVariableValue(&setting.AppURL, "https://next.forgejo.org/")()
defer test.MockVariableValue(&setting.AppSubURL, "")()
tests := []struct {
input string
want bool
}{
{"", false},
{"foo", false},
{"./", false},
{"?key=val", false},
{"/sub/", false},
{"https://next.forgejo.org/sub/", false},
{"/sub/foo", false},
{"https://next.forgejo.org/sub/foo", false},
{"https://next.forgejo.org/sub/test?param=false", false},
{"https://next.forgejo.org/sub/../", false},
{"/sub/../", false},
{"/sUb/", false},
{"https://next.forgejo.org/sUb/foo", false},
{"/sub", false},
{"/foo?k=%20#abc", false},
{"/", false},
{"a/", false},
{"test?param=false", false},
{"/hey/hey/hey#3244", false},
{"https://next.forgejo.org/test?param=false", false},
{"https://next.forgejo.org/foo", false},
{"https://next.forgejo.org/sub", false},
{"https://next.forgejo.org/sub?key=val", false},
{"//", true},
{"\\\\", true},
{"/\\", true},
{"\\/", true},
{"mail:a@b.com", true},
{"https://test.com", true},
{"https://example.com/", true},
{"//example.com", true},
{"http://example.com", true},
{"://missing protocol scheme", true},
{"https://forgejo.org", true},
{"https://example.org?url=https://next.forgejo.org", true},
// FIXME: should probably be false
{"https://next.forgejo.org", true},
{"//next.forgejo.org/test?param=false", true},
{"//next.forgejo.org/sub/test?param=false", true},
}
for _, tt := range tests {
t.Run(tt.input, func(t *testing.T) {

16
routers/utils/utils.go
View file

@ -5,26 +5,10 @@ package utils
import (
"html"
"net/url"
"strings"
"code.gitea.io/gitea/modules/setting"
)
// SanitizeFlashErrorString will sanitize a flash error string
func SanitizeFlashErrorString(x string) string {
return strings.ReplaceAll(html.EscapeString(x), "\n", "<br>")
}
// 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
}
appURL, _ := url.Parse(setting.AppURL)
if len(parsed.Host) != 0 && strings.Replace(parsed.Host, "www.", "", 1) != strings.Replace(appURL.Host, "www.", "", 1) {
return true
}
return false
}

39
routers/utils/utils_test.go
View file

@ -5,47 +5,8 @@ package utils
import (
"testing"
"code.gitea.io/gitea/modules/setting"
"github.com/stretchr/testify/assert"
)
func TestIsExternalURL(t *testing.T) {
setting.AppURL = "https://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/"),
newTest(false,
"https://try.gitea.io/test?param=false"),
newTest(false,
"test?param=false"),
newTest(false,
"//try.gitea.io/test?param=false"),
newTest(false,
"/hey/hey/hey#3244"),
newTest(true,
"://missing protocol scheme"),
} {
assert.Equal(t, test.Expected, IsExternalURL(test.RawURL))
}
}
func TestSanitizeFlashErrorString(t *testing.T) {
tests := []struct {
name string

15
routers/web/auth/auth.go
View file

@ -18,6 +18,7 @@ import (
"code.gitea.io/gitea/modules/auth/password"
"code.gitea.io/gitea/modules/base"
"code.gitea.io/gitea/modules/eventsource"
"code.gitea.io/gitea/modules/httplib"
"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/optional"
"code.gitea.io/gitea/modules/session"
@ -26,7 +27,6 @@ import (
"code.gitea.io/gitea/modules/util"
"code.gitea.io/gitea/modules/web"
"code.gitea.io/gitea/modules/web/middleware"
"code.gitea.io/gitea/routers/utils"
auth_service "code.gitea.io/gitea/services/auth"
"code.gitea.io/gitea/services/auth/source/oauth2"
"code.gitea.io/gitea/services/context"
@ -372,17 +372,16 @@ func handleSignInFull(ctx *context.Context, u *user_model.User, remember, obeyRe
return setting.AppSubURL + "/"
}
if redirectTo := ctx.GetSiteCookie("redirect_to"); len(redirectTo) > 0 && !utils.IsExternalURL(redirectTo) {
redirectTo := ctx.GetSiteCookie("redirect_to")
if redirectTo != "" {
middleware.DeleteRedirectToCookie(ctx.Resp)
if obeyRedirect {
ctx.RedirectToFirst(redirectTo)
}
if obeyRedirect {
return ctx.RedirectToFirst(redirectTo)
}
if !httplib.IsRiskyRedirectURL(redirectTo) {
return redirectTo
}
if obeyRedirect {
ctx.Redirect(setting.AppSubURL + "/")
}
return setting.AppSubURL + "/"
}

9
routers/web/auth/password.go
View file

@ -18,7 +18,6 @@ import (
"code.gitea.io/gitea/modules/timeutil"
"code.gitea.io/gitea/modules/web"
"code.gitea.io/gitea/modules/web/middleware"
"code.gitea.io/gitea/routers/utils"
"code.gitea.io/gitea/services/context"
"code.gitea.io/gitea/services/forms"
"code.gitea.io/gitea/services/mailer"
@ -312,11 +311,9 @@ func MustChangePasswordPost(ctx *context.Context) {
log.Trace("User updated password: %s", ctx.Doer.Name)
if redirectTo := ctx.GetSiteCookie("redirect_to"); len(redirectTo) > 0 && !utils.IsExternalURL(redirectTo) {
redirectTo := ctx.GetSiteCookie("redirect_to")
if redirectTo != "" {
middleware.DeleteRedirectToCookie(ctx.Resp)
ctx.RedirectToFirst(redirectTo)
return
}
ctx.Redirect(setting.AppSubURL + "/")
ctx.RedirectToFirst(redirectTo)
}

9
services/context/context_response.go
View file

@ -44,8 +44,10 @@ func RedirectToUser(ctx *Base, userName string, redirectUserID int64) {
ctx.Redirect(path.Join(setting.AppSubURL, redirectPath), http.StatusTemporaryRedirect)
}
// RedirectToFirst redirects to first not empty URL
func (ctx *Context) RedirectToFirst(location ...string) {
// RedirectToFirst redirects to first not empty URL which likely belongs to current site.
// If no suitable redirection is found, it redirects to the home.
// It returns the location it redirected to.
func (ctx *Context) RedirectToFirst(location ...string) string {
for _, loc := range location {
if len(loc) == 0 {
continue
@ -56,10 +58,11 @@ func (ctx *Context) RedirectToFirst(location ...string) {
}
ctx.Redirect(loc)
return
return loc
}
ctx.Redirect(setting.AppSubURL + "/")
return setting.AppSubURL + "/"
}
const tplStatus500 base.TplName = "status/500"