Skip to content

Commit 99f568d

Browse files
wxiaoguangwxiaoguang
committed
fix
1 parent caff989 commit 99f568d

15 files changed

Lines changed: 131 additions & 60 deletions

File tree

‎models/auth/oauth2.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -627,7 +627,7 @@ func GetActiveOAuth2SourceByAuthName(ctx context.Context, name string) (*Source,
627627
}
628628

629629
if !has {
630-
return nil, fmt.Errorf("oauth2 source not found, name: %q", name)
630+
return nil, util.NewNotExistErrorf("oauth2 source not found, name: %q", name)
631631
}
632632

633633
return authSource, nil

‎modules/templates/helper_test.go‎

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ package templates
55

66
import (
77
"html/template"
8+
"net/url"
89
"strings"
910
"testing"
1011

@@ -169,9 +170,21 @@ func TestQueryBuild(t *testing.T) {
169170
})
170171
}
171172

173+
const queryNonASCII = " !\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~" // all non-letter & non-number chars
174+
172175
func TestQueryEscape(t *testing.T) {
173176
// this test is a reference for "urlQueryEscape" in JS
174-
in := "!\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~" // all non-letter & non-number chars
175-
expected := "%21%22%23%24%25%26%27%28%29%2A%2B%2C-.%2F%3A%3B%3C%3D%3E%3F%40%5B%5C%5D%5E_%60%7B%7C%7D~"
176-
assert.Equal(t, expected, string(queryEscape(in)))
177+
// Special case for space encoding:
178+
// * RFC 3986: Uniform Resource Identifier (URI): %20
179+
// * WHATWG HTML: application/x-www-form-urlencoded: +
180+
// * JavaScript: encodeURIComponent() uses "%20". URLSearchParams uses "+"
181+
// * Golang: QueryEscape uses "+"
182+
expected := "+%21%22%23%24%25%26%27%28%29%2A%2B%2C-.%2F%3A%3B%3C%3D%3E%3F%40%5B%5C%5D%5E_%60%7B%7C%7D~"
183+
assert.Equal(t, expected, url.QueryEscape(queryNonASCII))
184+
}
185+
186+
func TestPathEscape(t *testing.T) {
187+
// this test is a reference for "pathEscape" in JS
188+
expected := "%20%21%22%23$%25&%27%28%29%2A+%2C-.%2F:%3B%3C=%3E%3F@%5B%5C%5D%5E_%60%7B%7C%7D~"
189+
assert.Equal(t, expected, url.PathEscape(queryNonASCII))
177190
}

‎routers/web/auth/auth.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -230,7 +230,7 @@ func performAutoLoginOAuth2(ctx *context.Context, data *preparedSignInData) bool
230230
return false
231231
}
232232

233-
skipToOAuthURL := setting.AppSubURL + "/user/oauth2/" + url.QueryEscape(data.oauth2Providers[0].DisplayName())
233+
skipToOAuthURL := setting.AppSubURL + "/user/oauth2/" + url.PathEscape(data.oauth2Providers[0].DisplayName())
234234
if redirectTo := ctx.FormString("redirect_to"); redirectTo != "" {
235235
skipToOAuthURL += "?redirect_to=" + url.QueryEscape(redirectTo)
236236
}

‎routers/web/auth/auth_test.go‎

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
package auth
55

66
import (
7+
"html/template"
78
"net/http"
89
"net/http/httptest"
910
"net/url"
@@ -67,29 +68,29 @@ func TestWebAuthOAuth2(t *testing.T) {
6768
defer test.MockVariableValue(&setting.OAuth2Client.EnableAutoRegistration, true)()
6869

6970
_ = oauth2.Init(t.Context())
70-
addOAuth2Source(t, "dummy-auth-source", oauth2.Source{})
71+
addOAuth2Source(t, "dummy+auth's source", oauth2.Source{})
7172

7273
t.Run("OAuth2MissingField", func(t *testing.T) {
7374
defer test.MockVariableValue(&gothic.CompleteUserAuth, func(res http.ResponseWriter, req *http.Request) (goth.User, error) {
74-
return goth.User{Provider: "dummy-auth-source", UserID: "dummy-user"}, nil
75+
return goth.User{Provider: "dummy+auth's source", UserID: "dummy-user"}, nil
7576
})()
7677
mockOpt := contexttest.MockContextOption{SessionStore: session.NewMockMemStore("dummy-sid")}
77-
ctx, resp := contexttest.MockContext(t, "/user/oauth2/dummy-auth-source/callback?code=dummy-code", mockOpt)
78-
ctx.SetPathParam("provider", "dummy-auth-source")
78+
ctx, resp := contexttest.MockContext(t, "/user/oauth2/..../callback?code=dummy-code", mockOpt)
79+
ctx.SetPathParamRaw("provider", "dummy+auth%27s%20source")
7980
SignInOAuthCallback(ctx)
8081
assert.Equal(t, http.StatusSeeOther, resp.Code)
8182
assert.Equal(t, "/user/link_account", test.RedirectURL(resp))
8283

8384
// then the user will be redirected to the link account page, and see a message about the missing fields
8485
ctx, _ = contexttest.MockContext(t, "/user/link_account", mockOpt)
8586
LinkAccount(ctx)
86-
assert.EqualValues(t, "auth.oauth_callback_unable_auto_reg:dummy-auth-source,email", ctx.Data["AutoRegistrationFailedPrompt"])
87+
assert.Equal(t, template.HTML("auth.oauth_callback_unable_auto_reg:dummy+auth&#39;s source,email"), ctx.Data["AutoRegistrationFailedPrompt"])
8788
})
8889

8990
t.Run("OAuth2CallbackError", func(t *testing.T) {
9091
mockOpt := contexttest.MockContextOption{SessionStore: session.NewMockMemStore("dummy-sid")}
91-
ctx, resp := contexttest.MockContext(t, "/user/oauth2/dummy-auth-source/callback", mockOpt)
92-
ctx.SetPathParam("provider", "dummy-auth-source")
92+
ctx, resp := contexttest.MockContext(t, "/user/oauth2/...../callback", mockOpt)
93+
ctx.SetPathParamRaw("provider", "dummy+auth%27s%20source")
9394
SignInOAuthCallback(ctx)
9495
assert.Equal(t, http.StatusSeeOther, resp.Code)
9596
assert.Equal(t, "/user/login", test.RedirectURL(resp))
@@ -112,8 +113,8 @@ func TestWebAuthOAuth2(t *testing.T) {
112113
assert.Equal(t, expectedRedirect, test.RedirectURL(resp))
113114
}
114115
}
115-
testSignIn(t, "/user/login", http.StatusSeeOther, "/user/oauth2/dummy-auth-source")
116-
testSignIn(t, "/user/login?redirect_to=/", http.StatusSeeOther, "/user/oauth2/dummy-auth-source?redirect_to=%2F")
116+
testSignIn(t, "/user/login", http.StatusSeeOther, "/user/oauth2/dummy+auth%27s%20source")
117+
testSignIn(t, "/user/login?redirect_to=/", http.StatusSeeOther, "/user/oauth2/dummy+auth%27s%20source?redirect_to=%2F")
117118

118119
*enablePassword, *enableOpenID, *enablePasskey = true, false, false
119120
testSignIn(t, "/user/login", http.StatusOK, "")

‎routers/web/auth/oauth.go‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -36,9 +36,7 @@ import (
3636

3737
// SignInOAuth handles the OAuth2 login buttons
3838
func SignInOAuth(ctx *context.Context) {
39-
// the provider is escaped by backend QueryEscape and frontend urlQueryEscape
40-
// so always use QueryUnescape to decode it
41-
authName, _ := url.QueryUnescape(ctx.PathParamRaw("provider"))
39+
authName := ctx.PathParam("provider")
4240
authSource, err := auth.GetActiveOAuth2SourceByAuthName(ctx, authName)
4341
if err != nil {
4442
ctx.ServerError("SignIn", err)

‎services/context/base_path.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -44,9 +44,9 @@ func (b *Base) PathParamInt(p string) int {
4444

4545
// SetPathParam set request path params into routes
4646
func (b *Base) SetPathParam(name, value string) {
47-
if strings.HasPrefix(name, ":") {
48-
setting.PanicInDevOrTesting("path param should not start with ':'")
49-
name = name[1:]
50-
}
5147
chi.RouteContext(b).URLParams.Add(name, url.PathEscape(value))
5248
}
49+
50+
func (b *Base) SetPathParamRaw(name, value string) {
51+
chi.RouteContext(b).URLParams.Add(name, value)
52+
}

‎services/context/context_response.go‎

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import (
2222
"code.gitea.io/gitea/modules/setting"
2323
"code.gitea.io/gitea/modules/structs"
2424
"code.gitea.io/gitea/modules/templates"
25+
"code.gitea.io/gitea/modules/util"
2526
"code.gitea.io/gitea/modules/web/middleware"
2627
)
2728

@@ -143,10 +144,17 @@ func (ctx *Context) NotFound(logErr error) {
143144
}
144145

145146
func (ctx *Context) notFoundInternal(logMsg string, logErr error) {
147+
// it's safe to show the error message to end users since the error is fully controlled by our error system
148+
var errorMsg string
146149
if logErr != nil {
147-
log.Log(2, log.DEBUG, "%s: %v", logMsg, logErr)
148-
if !setting.IsProd {
149-
ctx.Data["ErrorMsg"] = logErr
150+
log.Log(2, log.DEBUG, "NotFound: %s: %v", logMsg, logErr)
151+
errorMsg = logErr.Error()
152+
}
153+
if logMsg != "" {
154+
if errorMsg == "" {
155+
errorMsg = logMsg
156+
} else {
157+
errorMsg = logMsg + "; " + errorMsg
150158
}
151159
}
152160

@@ -160,17 +168,23 @@ func (ctx *Context) notFoundInternal(logMsg string, logErr error) {
160168
}
161169

162170
if !showHTML {
163-
ctx.plainTextInternal(3, http.StatusNotFound, []byte("Not found.\n"))
171+
errorMsg = util.IfZero(errorMsg, "Not found.")
172+
ctx.plainTextInternal(3, http.StatusNotFound, []byte(errorMsg+"\n"))
164173
return
165174
}
166175

167176
ctx.Data["IsRepo"] = ctx.Repo.Repository != nil
168177
ctx.Data["Title"] = "Page Not Found"
178+
ctx.Data["ErrorMsg"] = errorMsg
169179
ctx.HTML(http.StatusNotFound, "status/404")
170180
}
171181

172182
// ServerError displays a 500 (Internal Server Error) page and prints the given error, if any.
173183
func (ctx *Context) ServerError(logMsg string, logErr error) {
184+
if errors.Is(logErr, util.ErrNotExist) {
185+
ctx.notFoundInternal(logMsg, logErr)
186+
return
187+
}
174188
ctx.serverErrorInternal(logMsg, logErr)
175189
}
176190

‎templates/user/auth/external_auth_methods.tmpl‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
<div id="external-login-navigator" class="tw-py-1 tw-flex tw-flex-col tw-gap-3">
22
{{range $provider := .OAuth2Providers}}
3-
{{/* use QueryEscape for consistent with frontend urlQueryEscape, it is right for a path component */}}
4-
<a class="ui button external-login-link tw-gap-3" data-require-appurl-check="true" rel="nofollow" href="{{AppSubUrl}}/user/oauth2/{{$provider.DisplayName | QueryEscape}}">
3+
<a class="ui button external-login-link tw-gap-3" data-require-appurl-check="true" rel="nofollow" href="{{AppSubUrl}}/user/oauth2/{{$provider.DisplayName | PathEscape}}">
54
{{$provider.IconHTML 24}} {{ctx.Locale.Tr "sign_in_with_provider" $provider.DisplayName}}
65
</a>
76
{{end}}

‎templates/user/settings/security/accountlinks.tmpl‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@
99
<div class="menu">
1010
{{range $key := .OrderedOAuth2Names}}
1111
{{$provider := index $.OAuth2Providers $key}}
12-
<a class="item" href="{{AppSubUrl}}/user/oauth2/{{$key}}">
12+
<a class="item" href="{{AppSubUrl}}/user/oauth2/{{$key | PathEscape}}">
1313
{{$provider.IconHTML 20}}
1414
{{$provider.DisplayName}}
1515
</a>

‎tests/integration/oauth_test.go‎

Lines changed: 31 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ import (
2727
"code.gitea.io/gitea/services/auth/source/oauth2"
2828
"code.gitea.io/gitea/services/oauth2_provider"
2929
"code.gitea.io/gitea/tests"
30+
"github.com/PuerkitoBio/goquery"
3031

3132
"github.com/markbates/goth"
3233
"github.com/markbates/goth/gothic"
@@ -44,7 +45,7 @@ func TestOAuth2Provider(t *testing.T) {
4445
t.Run("AuthorizeLoginRedirect", testAuthorizeLoginRedirect)
4546

4647
t.Run("OAuth2WellKnown", testOAuth2WellKnown)
47-
t.Run("OAuthSourceWithSpace", testOAuthSourceWithSpace)
48+
t.Run("OAuthSourceSpecialChars", testOAuthSourceSpecialChars)
4849
// TODO: move more tests as sub-tests here, avoid unnecessary PrepareTestEnv
4950
}
5051

@@ -1098,19 +1099,40 @@ func TestSignInOauthCallbackSyncSSHKeys(t *testing.T) {
10981099

10991100
// Checks if an OAuth provider with spaces within the name does work,
11001101
// with the encoding of its names in the URL (PR#37327)
1101-
func testOAuthSourceWithSpace(t *testing.T) {
1102+
func testOAuthSourceSpecialChars(t *testing.T) {
11021103
mockServer := createMockServer()
11031104
defer mockServer.Close()
11041105

1105-
authName := "oauth test with spaces"
1106-
oauth2Source := oauth2.Source{
1106+
addOAuth2Source(t, "test space", oauth2.Source{
11071107
Provider: "openidConnect",
11081108
OpenIDConnectAutoDiscoveryURL: mockServer.URL + "/.well-known/openid-configuration",
1109+
})
1110+
addOAuth2Source(t, "test+plus", oauth2.Source{
1111+
Provider: "openidConnect",
1112+
OpenIDConnectAutoDiscoveryURL: mockServer.URL + "/.well-known/openid-configuration",
1113+
})
1114+
1115+
testOAuth2 := func(t *testing.T, uri string, statusCode int) {
1116+
req := NewRequest(t, "GET", uri)
1117+
resp := MakeRequest(t, req, statusCode)
1118+
assert.NotEmpty(t, resp.Header().Get("Location"))
11091119
}
1110-
addOAuth2Source(t, authName, oauth2Source)
11111120

1112-
session := emptyTestSession(t)
1113-
req := NewRequest(t, "GET", "/user/oauth2/"+url.QueryEscape(authName))
1114-
resp := session.MakeRequest(t, req, http.StatusTemporaryRedirect)
1115-
assert.Contains(t, resp.Header().Get("Location"), mockServer.URL+"/authorize")
1121+
req := MakeRequest(t, NewRequest(t, "GET", "/user/login"), http.StatusOK)
1122+
doc := NewHTMLParser(t, req.Body)
1123+
var oauth2Links []string
1124+
doc.Find(".external-login-link").Each(func(i int, s *goquery.Selection) {
1125+
oauth2Links = append(oauth2Links, s.AttrOr("href", ""))
1126+
})
1127+
assert.Equal(t, []string{
1128+
"/user/oauth2/test%20space",
1129+
"/user/oauth2/test+plus",
1130+
}, oauth2Links)
1131+
1132+
testOAuth2(t, "/user/oauth2/test%20space", http.StatusTemporaryRedirect)
1133+
testOAuth2(t, "/user/oauth2/test+space", http.StatusNotFound)
1134+
1135+
testOAuth2(t, "/user/oauth2/test+plus", http.StatusTemporaryRedirect)
1136+
testOAuth2(t, "/user/oauth2/test%2Bplus", http.StatusTemporaryRedirect)
1137+
testOAuth2(t, "/user/oauth2/test%20plus", http.StatusNotFound)
11161138
}

0 commit comments

Comments
 (0)