Skip to content

Commit ca4107d

Browse files
wxiaoguangwxiaoguang
authored
Refactor external URL detection (#29973)
Follow #29960, `IsExternalURL` is not needed anymore. Add some tests for `RedirectToCurrentSite`
1 parent bfa160f commit ca4107d

File tree

7 files changed

+40
-62
lines changed

7 files changed

+40
-62
lines changed

‎modules/httplib/url.go

+5-3
Original file line numberDiff line numberDiff line change
@@ -32,9 +32,11 @@ func IsCurrentGiteaSiteURL(s string) bool {
3232
return false
3333
}
3434
if u.Path != "" {
35-
u.Path = "/" + util.PathJoinRelX(u.Path)
36-
if !strings.HasSuffix(u.Path, "/") {
37-
u.Path += "/"
35+
cleanedPath := util.PathJoinRelX(u.Path)
36+
if cleanedPath == "" || cleanedPath == "." {
37+
u.Path = "/"
38+
} else {
39+
u.Path += "/" + cleanedPath + "/"
3840
}
3941
}
4042
if urlIsRelative(s, u) {

‎modules/httplib/url_test.go

+5
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,8 @@ func TestIsCurrentGiteaSiteURL(t *testing.T) {
5353
assert.True(t, IsCurrentGiteaSiteURL(s), "good = %q", s)
5454
}
5555
bad := []string{
56+
".",
57+
"foo",
5658
"/",
5759
"//",
5860
"\\\\",
@@ -67,5 +69,8 @@ func TestIsCurrentGiteaSiteURL(t *testing.T) {
6769

6870
setting.AppURL = "http://localhost:3000/"
6971
setting.AppSubURL = ""
72+
assert.False(t, IsCurrentGiteaSiteURL("//"))
73+
assert.False(t, IsCurrentGiteaSiteURL("\\\\"))
74+
assert.False(t, IsCurrentGiteaSiteURL("http://localhost"))
7075
assert.True(t, IsCurrentGiteaSiteURL("http://localhost:3000?key=val"))
7176
}

‎routers/utils/utils.go

-16
Original file line numberDiff line numberDiff line change
@@ -5,26 +5,10 @@ package utils
55

66
import (
77
"html"
8-
"net/url"
98
"strings"
10-
11-
"code.gitea.io/gitea/modules/setting"
129
)
1310

1411
// SanitizeFlashErrorString will sanitize a flash error string
1512
func SanitizeFlashErrorString(x string) string {
1613
return strings.ReplaceAll(html.EscapeString(x), "\n", "<br>")
1714
}
18-
19-
// IsExternalURL checks if rawURL points to an external URL like http://example.com
20-
func IsExternalURL(rawURL string) bool {
21-
parsed, err := url.Parse(rawURL)
22-
if err != nil {
23-
return true
24-
}
25-
appURL, _ := url.Parse(setting.AppURL)
26-
if len(parsed.Host) != 0 && strings.Replace(parsed.Host, "www.", "", 1) != strings.Replace(appURL.Host, "www.", "", 1) {
27-
return true
28-
}
29-
return false
30-
}

‎routers/utils/utils_test.go

-39
Original file line numberDiff line numberDiff line change
@@ -5,47 +5,8 @@ package utils
55

66
import (
77
"testing"
8-
9-
"code.gitea.io/gitea/modules/setting"
10-
11-
"github.com/stretchr/testify/assert"
128
)
139

14-
func TestIsExternalURL(t *testing.T) {
15-
setting.AppURL = "https://try.gitea.io/"
16-
type test struct {
17-
Expected bool
18-
RawURL string
19-
}
20-
newTest := func(expected bool, rawURL string) test {
21-
return test{Expected: expected, RawURL: rawURL}
22-
}
23-
for _, test := range []test{
24-
newTest(false,
25-
"https://try.gitea.io"),
26-
newTest(true,
27-
"https://example.com/"),
28-
newTest(true,
29-
"//example.com"),
30-
newTest(true,
31-
"http://example.com"),
32-
newTest(false,
33-
"a/"),
34-
newTest(false,
35-
"https://try.gitea.io/test?param=false"),
36-
newTest(false,
37-
"test?param=false"),
38-
newTest(false,
39-
"//try.gitea.io/test?param=false"),
40-
newTest(false,
41-
"/hey/hey/hey#3244"),
42-
newTest(true,
43-
"://missing protocol scheme"),
44-
} {
45-
assert.Equal(t, test.Expected, IsExternalURL(test.RawURL))
46-
}
47-
}
48-
4910
func TestSanitizeFlashErrorString(t *testing.T) {
5011
tests := []struct {
5112
name string

‎routers/web/auth/auth.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import (
1717
"code.gitea.io/gitea/modules/auth/password"
1818
"code.gitea.io/gitea/modules/base"
1919
"code.gitea.io/gitea/modules/eventsource"
20+
"code.gitea.io/gitea/modules/httplib"
2021
"code.gitea.io/gitea/modules/log"
2122
"code.gitea.io/gitea/modules/optional"
2223
"code.gitea.io/gitea/modules/session"
@@ -25,7 +26,6 @@ import (
2526
"code.gitea.io/gitea/modules/util"
2627
"code.gitea.io/gitea/modules/web"
2728
"code.gitea.io/gitea/modules/web/middleware"
28-
"code.gitea.io/gitea/routers/utils"
2929
auth_service "code.gitea.io/gitea/services/auth"
3030
"code.gitea.io/gitea/services/auth/source/oauth2"
3131
"code.gitea.io/gitea/services/context"
@@ -368,7 +368,7 @@ func handleSignInFull(ctx *context.Context, u *user_model.User, remember, obeyRe
368368
return setting.AppSubURL + "/"
369369
}
370370

371-
if redirectTo := ctx.GetSiteCookie("redirect_to"); len(redirectTo) > 0 && !utils.IsExternalURL(redirectTo) {
371+
if redirectTo := ctx.GetSiteCookie("redirect_to"); redirectTo != "" && httplib.IsCurrentGiteaSiteURL(redirectTo) {
372372
middleware.DeleteRedirectToCookie(ctx.Resp)
373373
if obeyRedirect {
374374
ctx.RedirectToCurrentSite(redirectTo)

‎routers/web/auth/password.go

+1-2
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@ import (
1818
"code.gitea.io/gitea/modules/timeutil"
1919
"code.gitea.io/gitea/modules/web"
2020
"code.gitea.io/gitea/modules/web/middleware"
21-
"code.gitea.io/gitea/routers/utils"
2221
"code.gitea.io/gitea/services/context"
2322
"code.gitea.io/gitea/services/forms"
2423
"code.gitea.io/gitea/services/mailer"
@@ -312,7 +311,7 @@ func MustChangePasswordPost(ctx *context.Context) {
312311

313312
log.Trace("User updated password: %s", ctx.Doer.Name)
314313

315-
if redirectTo := ctx.GetSiteCookie("redirect_to"); len(redirectTo) > 0 && !utils.IsExternalURL(redirectTo) {
314+
if redirectTo := ctx.GetSiteCookie("redirect_to"); redirectTo != "" {
316315
middleware.DeleteRedirectToCookie(ctx.Resp)
317316
ctx.RedirectToCurrentSite(redirectTo)
318317
return

‎services/context/context_test.go

+27
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,11 @@ package context
66
import (
77
"net/http"
88
"net/http/httptest"
9+
"net/url"
910
"testing"
1011

1112
"code.gitea.io/gitea/modules/setting"
13+
"code.gitea.io/gitea/modules/test"
1214

1315
"github.com/stretchr/testify/assert"
1416
)
@@ -22,3 +24,28 @@ func TestRemoveSessionCookieHeader(t *testing.T) {
2224
assert.Len(t, w.Header().Values("Set-Cookie"), 1)
2325
assert.Contains(t, "other=bar", w.Header().Get("Set-Cookie"))
2426
}
27+
28+
func TestRedirectToCurrentSite(t *testing.T) {
29+
defer test.MockVariableValue(&setting.AppURL, "http://localhost:3000/sub/")()
30+
defer test.MockVariableValue(&setting.AppSubURL, "/sub")()
31+
cases := []struct {
32+
location string
33+
want string
34+
}{
35+
{"/", "/sub/"},
36+
{"http://localhost:3000/sub?k=v", "http://localhost:3000/sub?k=v"},
37+
{"http://other", "/sub/"},
38+
}
39+
for _, c := range cases {
40+
t.Run(c.location, func(t *testing.T) {
41+
req := &http.Request{URL: &url.URL{Path: "/"}}
42+
resp := httptest.NewRecorder()
43+
base, baseCleanUp := NewBaseContext(resp, req)
44+
defer baseCleanUp()
45+
ctx := NewWebContext(base, nil, nil)
46+
ctx.RedirectToCurrentSite(c.location)
47+
redirect := test.RedirectURL(resp)
48+
assert.Equal(t, c.want, redirect)
49+
})
50+
}
51+
}

0 commit comments

Comments
 (0)