diff --git a/models/auth/oauth2.go b/models/auth/oauth2.go index 846c386a20c..0bc2b517111 100644 --- a/models/auth/oauth2.go +++ b/models/auth/oauth2.go @@ -627,7 +627,7 @@ func GetActiveOAuth2SourceByAuthName(ctx context.Context, name string) (*Source, } if !has { - return nil, fmt.Errorf("oauth2 source not found, name: %q", name) + return nil, util.NewNotExistErrorf("oauth2 source not found, name: %q", name) } return authSource, nil diff --git a/modules/templates/helper_test.go b/modules/templates/helper_test.go index cf1db324768..c21c20efff9 100644 --- a/modules/templates/helper_test.go +++ b/modules/templates/helper_test.go @@ -5,6 +5,7 @@ package templates import ( "html/template" + "net/url" "strings" "testing" @@ -169,9 +170,21 @@ func TestQueryBuild(t *testing.T) { }) } +const queryNonASCII = " !\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~" // all non-letter & non-number chars + func TestQueryEscape(t *testing.T) { // this test is a reference for "urlQueryEscape" in JS - in := "!\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~" // all non-letter & non-number chars - 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~" - assert.Equal(t, expected, string(queryEscape(in))) + // Special case for space encoding: + // * RFC 3986: Uniform Resource Identifier (URI): %20 + // * WHATWG HTML: application/x-www-form-urlencoded: + + // * JavaScript: encodeURIComponent() uses "%20". URLSearchParams uses "+" + // * Golang: QueryEscape uses "+" + 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~" + assert.Equal(t, expected, url.QueryEscape(queryNonASCII)) +} + +func TestPathEscape(t *testing.T) { + // this test is a reference for "pathEscape" in JS + expected := "%20%21%22%23$%25&%27%28%29%2A+%2C-.%2F:%3B%3C=%3E%3F@%5B%5C%5D%5E_%60%7B%7C%7D~" + assert.Equal(t, expected, url.PathEscape(queryNonASCII)) } diff --git a/modules/util/util_test.go b/modules/util/util_test.go index ec8b738e543..9decf90f676 100644 --- a/modules/util/util_test.go +++ b/modules/util/util_test.go @@ -184,3 +184,10 @@ func TestOptionalArg(t *testing.T) { assert.Equal(t, 42, bar(nil)) assert.Equal(t, 100, bar(nil, 100)) } + +func TestPathEscapeSegments(t *testing.T) { + assert.Equal(t, "a", PathEscapeSegments("a")) + assert.Equal(t, "a/b", PathEscapeSegments("a/b")) + assert.Equal(t, "a/b%20c", PathEscapeSegments("a/b c")) + assert.Equal(t, "a/b+c", PathEscapeSegments("a/b+c")) +} diff --git a/routers/web/auth/auth.go b/routers/web/auth/auth.go index eb2045781dd..1baa0225217 100644 --- a/routers/web/auth/auth.go +++ b/routers/web/auth/auth.go @@ -230,7 +230,7 @@ func performAutoLoginOAuth2(ctx *context.Context, data *preparedSignInData) bool return false } - skipToOAuthURL := setting.AppSubURL + "/user/oauth2/" + url.QueryEscape(data.oauth2Providers[0].DisplayName()) + skipToOAuthURL := setting.AppSubURL + "/user/oauth2/" + url.PathEscape(data.oauth2Providers[0].DisplayName()) if redirectTo := ctx.FormString("redirect_to"); redirectTo != "" { skipToOAuthURL += "?redirect_to=" + url.QueryEscape(redirectTo) } diff --git a/routers/web/auth/auth_test.go b/routers/web/auth/auth_test.go index 943085a9635..a06e209e4f4 100644 --- a/routers/web/auth/auth_test.go +++ b/routers/web/auth/auth_test.go @@ -4,6 +4,7 @@ package auth import ( + "html/template" "net/http" "net/http/httptest" "net/url" @@ -67,15 +68,15 @@ func TestWebAuthOAuth2(t *testing.T) { defer test.MockVariableValue(&setting.OAuth2Client.EnableAutoRegistration, true)() _ = oauth2.Init(t.Context()) - addOAuth2Source(t, "dummy-auth-source", oauth2.Source{}) + addOAuth2Source(t, "dummy+auth's source", oauth2.Source{}) t.Run("OAuth2MissingField", func(t *testing.T) { defer test.MockVariableValue(&gothic.CompleteUserAuth, func(res http.ResponseWriter, req *http.Request) (goth.User, error) { - return goth.User{Provider: "dummy-auth-source", UserID: "dummy-user"}, nil + return goth.User{Provider: "dummy+auth's source", UserID: "dummy-user"}, nil })() mockOpt := contexttest.MockContextOption{SessionStore: session.NewMockMemStore("dummy-sid")} - ctx, resp := contexttest.MockContext(t, "/user/oauth2/dummy-auth-source/callback?code=dummy-code", mockOpt) - ctx.SetPathParam("provider", "dummy-auth-source") + ctx, resp := contexttest.MockContext(t, "/user/oauth2/..../callback?code=dummy-code", mockOpt) + ctx.SetPathParamRaw("provider", "dummy+auth%27s%20source") SignInOAuthCallback(ctx) assert.Equal(t, http.StatusSeeOther, resp.Code) assert.Equal(t, "/user/link_account", test.RedirectURL(resp)) @@ -83,13 +84,13 @@ func TestWebAuthOAuth2(t *testing.T) { // then the user will be redirected to the link account page, and see a message about the missing fields ctx, _ = contexttest.MockContext(t, "/user/link_account", mockOpt) LinkAccount(ctx) - assert.EqualValues(t, "auth.oauth_callback_unable_auto_reg:dummy-auth-source,email", ctx.Data["AutoRegistrationFailedPrompt"]) + assert.Equal(t, template.HTML("auth.oauth_callback_unable_auto_reg:dummy+auth's source,email"), ctx.Data["AutoRegistrationFailedPrompt"]) }) t.Run("OAuth2CallbackError", func(t *testing.T) { mockOpt := contexttest.MockContextOption{SessionStore: session.NewMockMemStore("dummy-sid")} - ctx, resp := contexttest.MockContext(t, "/user/oauth2/dummy-auth-source/callback", mockOpt) - ctx.SetPathParam("provider", "dummy-auth-source") + ctx, resp := contexttest.MockContext(t, "/user/oauth2/...../callback", mockOpt) + ctx.SetPathParamRaw("provider", "dummy+auth%27s%20source") SignInOAuthCallback(ctx) assert.Equal(t, http.StatusSeeOther, resp.Code) assert.Equal(t, "/user/login", test.RedirectURL(resp)) @@ -112,8 +113,8 @@ func TestWebAuthOAuth2(t *testing.T) { assert.Equal(t, expectedRedirect, test.RedirectURL(resp)) } } - testSignIn(t, "/user/login", http.StatusSeeOther, "/user/oauth2/dummy-auth-source") - testSignIn(t, "/user/login?redirect_to=/", http.StatusSeeOther, "/user/oauth2/dummy-auth-source?redirect_to=%2F") + testSignIn(t, "/user/login", http.StatusSeeOther, "/user/oauth2/dummy+auth%27s%20source") + testSignIn(t, "/user/login?redirect_to=/", http.StatusSeeOther, "/user/oauth2/dummy+auth%27s%20source?redirect_to=%2F") *enablePassword, *enableOpenID, *enablePasskey = true, false, false testSignIn(t, "/user/login", http.StatusOK, "") diff --git a/routers/web/auth/oauth.go b/routers/web/auth/oauth.go index e3f7e382424..8645aedbdee 100644 --- a/routers/web/auth/oauth.go +++ b/routers/web/auth/oauth.go @@ -36,9 +36,7 @@ import ( // SignInOAuth handles the OAuth2 login buttons func SignInOAuth(ctx *context.Context) { - // the provider is escaped by backend QueryEscape and frontend urlQueryEscape - // so always use QueryUnescape to decode it - authName, _ := url.QueryUnescape(ctx.PathParamRaw("provider")) + authName := ctx.PathParam("provider") authSource, err := auth.GetActiveOAuth2SourceByAuthName(ctx, authName) if err != nil { ctx.ServerError("SignIn", err) diff --git a/services/context/base_path.go b/services/context/base_path.go index 63e60c8654e..8c353f7acaf 100644 --- a/services/context/base_path.go +++ b/services/context/base_path.go @@ -44,9 +44,9 @@ func (b *Base) PathParamInt(p string) int { // SetPathParam set request path params into routes func (b *Base) SetPathParam(name, value string) { - if strings.HasPrefix(name, ":") { - setting.PanicInDevOrTesting("path param should not start with ':'") - name = name[1:] - } chi.RouteContext(b).URLParams.Add(name, url.PathEscape(value)) } + +func (b *Base) SetPathParamRaw(name, value string) { + chi.RouteContext(b).URLParams.Add(name, value) +} diff --git a/services/context/context_response.go b/services/context/context_response.go index d057f8d41eb..1fd7a0b447f 100644 --- a/services/context/context_response.go +++ b/services/context/context_response.go @@ -22,6 +22,7 @@ import ( "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/structs" "code.gitea.io/gitea/modules/templates" + "code.gitea.io/gitea/modules/util" "code.gitea.io/gitea/modules/web/middleware" ) @@ -143,11 +144,9 @@ func (ctx *Context) NotFound(logErr error) { } func (ctx *Context) notFoundInternal(logMsg string, logErr error) { + // TODO: it's safe to show the error message to end users if the error is fully controlled by our error system if logErr != nil { log.Log(2, log.DEBUG, "%s: %v", logMsg, logErr) - if !setting.IsProd { - ctx.Data["ErrorMsg"] = logErr - } } // response simple message if Accept isn't text/html @@ -166,11 +165,17 @@ func (ctx *Context) notFoundInternal(logMsg string, logErr error) { ctx.Data["IsRepo"] = ctx.Repo.Repository != nil ctx.Data["Title"] = "Page Not Found" + ctx.Data["ErrorMsg"] = "" // FIXME: the template never renders this message, need to fix in the future (and show safe messages to end users) ctx.HTML(http.StatusNotFound, "status/404") } // ServerError displays a 500 (Internal Server Error) page and prints the given error, if any. +// If the error is controlled by our error system, a related 404 page can be displayed instead. func (ctx *Context) ServerError(logMsg string, logErr error) { + if errors.Is(logErr, util.ErrNotExist) { + ctx.notFoundInternal(logMsg, logErr) + return + } ctx.serverErrorInternal(logMsg, logErr) } diff --git a/templates/user/auth/external_auth_methods.tmpl b/templates/user/auth/external_auth_methods.tmpl index c23cab65657..119be95d8bb 100644 --- a/templates/user/auth/external_auth_methods.tmpl +++ b/templates/user/auth/external_auth_methods.tmpl @@ -1,7 +1,6 @@
{{range $provider := .OAuth2Providers}} - {{/* use QueryEscape for consistent with frontend urlQueryEscape, it is right for a path component */}} - + {{$provider.IconHTML 24}} {{ctx.Locale.Tr "sign_in_with_provider" $provider.DisplayName}} {{end}} diff --git a/templates/user/settings/security/accountlinks.tmpl b/templates/user/settings/security/accountlinks.tmpl index 89228e1ae80..2fb1784b415 100644 --- a/templates/user/settings/security/accountlinks.tmpl +++ b/templates/user/settings/security/accountlinks.tmpl @@ -9,7 +9,7 @@
{{range $key := .OrderedOAuth2Names}} {{$provider := index $.OAuth2Providers $key}} - + {{$provider.IconHTML 20}} {{$provider.DisplayName}} diff --git a/tests/integration/oauth_test.go b/tests/integration/oauth_test.go index b28f81a0483..2b10f5f61d2 100644 --- a/tests/integration/oauth_test.go +++ b/tests/integration/oauth_test.go @@ -28,6 +28,7 @@ import ( "code.gitea.io/gitea/services/oauth2_provider" "code.gitea.io/gitea/tests" + "github.com/PuerkitoBio/goquery" "github.com/markbates/goth" "github.com/markbates/goth/gothic" "github.com/stretchr/testify/assert" @@ -44,7 +45,7 @@ func TestOAuth2Provider(t *testing.T) { t.Run("AuthorizeLoginRedirect", testAuthorizeLoginRedirect) t.Run("OAuth2WellKnown", testOAuth2WellKnown) - t.Run("OAuthSourceWithSpace", testOAuthSourceWithSpace) + t.Run("OAuthSourceSpecialChars", testOAuthSourceSpecialChars) // TODO: move more tests as sub-tests here, avoid unnecessary PrepareTestEnv } @@ -1098,19 +1099,44 @@ func TestSignInOauthCallbackSyncSSHKeys(t *testing.T) { // Checks if an OAuth provider with spaces within the name does work, // with the encoding of its names in the URL (PR#37327) -func testOAuthSourceWithSpace(t *testing.T) { +func testOAuthSourceSpecialChars(t *testing.T) { mockServer := createMockServer() defer mockServer.Close() - authName := "oauth test with spaces" - oauth2Source := oauth2.Source{ + addOAuth2Source(t, "test space", oauth2.Source{ Provider: "openidConnect", OpenIDConnectAutoDiscoveryURL: mockServer.URL + "/.well-known/openid-configuration", - } - addOAuth2Source(t, authName, oauth2Source) + }) + addOAuth2Source(t, "test+plus", oauth2.Source{ + Provider: "openidConnect", + OpenIDConnectAutoDiscoveryURL: mockServer.URL + "/.well-known/openid-configuration", + }) - session := emptyTestSession(t) - req := NewRequest(t, "GET", "/user/oauth2/"+url.QueryEscape(authName)) - resp := session.MakeRequest(t, req, http.StatusTemporaryRedirect) - assert.Contains(t, resp.Header().Get("Location"), mockServer.URL+"/authorize") + testOAuth2 := func(t *testing.T, uri string, statusCode int) { + req := NewRequest(t, "GET", uri) + resp := MakeRequest(t, req, statusCode) + if statusCode == http.StatusTemporaryRedirect { + assert.NotEmpty(t, resp.Header().Get("Location")) + } else { + assert.Empty(t, resp.Header().Get("Location")) + } + } + + req := MakeRequest(t, NewRequest(t, "GET", "/user/login"), http.StatusOK) + doc := NewHTMLParser(t, req.Body) + var oauth2Links []string + doc.Find(".external-login-link").Each(func(i int, s *goquery.Selection) { + oauth2Links = append(oauth2Links, s.AttrOr("href", "")) + }) + assert.Equal(t, []string{ + "/user/oauth2/test%20space", + "/user/oauth2/test+plus", + }, oauth2Links) + + testOAuth2(t, "/user/oauth2/test%20space", http.StatusTemporaryRedirect) + testOAuth2(t, "/user/oauth2/test+space", http.StatusNotFound) + + testOAuth2(t, "/user/oauth2/test+plus", http.StatusTemporaryRedirect) + testOAuth2(t, "/user/oauth2/test%2Bplus", http.StatusTemporaryRedirect) + testOAuth2(t, "/user/oauth2/test%20plus", http.StatusNotFound) } diff --git a/web_src/js/features/admin/common.ts b/web_src/js/features/admin/common.ts index 0f457e2e1ac..38c9418330d 100644 --- a/web_src/js/features/admin/common.ts +++ b/web_src/js/features/admin/common.ts @@ -2,7 +2,7 @@ import {checkAppUrl} from '../common-page.ts'; import {hideElem, queryElems, showElem, toggleElem} from '../../utils/dom.ts'; import {POST} from '../../modules/fetch.ts'; import {fomanticQuery} from '../../modules/fomantic/base.ts'; -import {urlQueryEscape} from '../../utils.ts'; +import {pathEscape} from '../../utils/url.ts'; const {appSubUrl} = window.config; @@ -232,7 +232,7 @@ function initAdminAuthentication() { const elAuthName = document.querySelector('#auth_name')!; const onAuthNameChange = function () { // appSubUrl is either empty or is a path that starts with `/` and doesn't have a trailing slash. - document.querySelector('#oauth2-callback-url')!.textContent = `${window.location.origin}${appSubUrl}/user/oauth2/${urlQueryEscape(elAuthName.value)}/callback`; + document.querySelector('#oauth2-callback-url')!.textContent = `${window.location.origin}${appSubUrl}/user/oauth2/${pathEscape(elAuthName.value)}/callback`; }; elAuthName.addEventListener('input', onAuthNameChange); onAuthNameChange(); diff --git a/web_src/js/utils.test.ts b/web_src/js/utils.test.ts index 507334c0b41..c72f55e773d 100644 --- a/web_src/js/utils.test.ts +++ b/web_src/js/utils.test.ts @@ -2,7 +2,6 @@ import { dirname, basename, extname, formatBytes, isObject, stripTags, parseIssueHref, translateMonth, translateDay, blobToDataURI, toAbsoluteUrl, encodeURLEncodedBase64, decodeURLEncodedBase64, isImageFile, isVideoFile, parseRepoOwnerPathInfo, - urlQueryEscape, } from './utils.ts'; test('dirname', () => { @@ -34,12 +33,6 @@ test('stripTags', () => { expect(stripTags('test')).toEqual('test'); }); -test('urlQueryEscape', () => { - const input = "!\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~"; - const 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~'; - expect(urlQueryEscape(input)).toEqual(expected); -}); - test('parseIssueHref', () => { expect(parseIssueHref('/owner/repo/issues/1')).toEqual({ownerName: 'owner', repoName: 'repo', pathType: 'issues', indexString: '1'}); expect(parseIssueHref('/owner/repo/pulls/1?query')).toEqual({ownerName: 'owner', repoName: 'repo', pathType: 'pulls', indexString: '1'}); diff --git a/web_src/js/utils.ts b/web_src/js/utils.ts index d802dd8e084..feeb4d3b6bd 100644 --- a/web_src/js/utils.ts +++ b/web_src/js/utils.ts @@ -51,15 +51,6 @@ export function stripTags(text: string): string { return text; } -export function urlQueryEscape(s: string) { - // See "TestQueryEscape" in backend - // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/encodeURIComponent#encoding_for_rfc3986 - return encodeURIComponent(s).replace( - /[!'()*]/g, - (c) => `%${c.charCodeAt(0).toString(16).toUpperCase()}`, - ); -} - export function parseIssueHref(href: string): IssuePathInfo { // FIXME: it should use pathname and trim the appSubUrl ahead const path = (href || '').replace(/[#?].*$/, ''); diff --git a/web_src/js/utils/url.test.ts b/web_src/js/utils/url.test.ts index 3a4323e88f7..a2e73393753 100644 --- a/web_src/js/utils/url.test.ts +++ b/web_src/js/utils/url.test.ts @@ -1,8 +1,22 @@ -import {linkifyURLs, pathEscapeSegments, toOriginUrl} from './url.ts'; +import {linkifyURLs, pathEscape, pathEscapeSegments, toOriginUrl, urlQueryEscape} from './url.ts'; -test('pathEscapeSegments', () => { - expect(pathEscapeSegments('a/b/c')).toEqual('a/b/c'); - expect(pathEscapeSegments('a/b/ c')).toEqual('a/b/%20c'); +describe('escape', () => { + const queryNonAscii = " !\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~"; + test('urlQueryEscape', () => { + const 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~'; + expect(urlQueryEscape(queryNonAscii)).toEqual(expected); + }); + + test('pathEscape', () => { + const expected = '%20%21%22%23$%25&%27%28%29%2A+%2C-.%2F:%3B%3C=%3E%3F@%5B%5C%5D%5E_%60%7B%7C%7D~'; + expect(pathEscape(queryNonAscii)).toEqual(expected); + }); + + test('pathEscapeSegments', () => { + expect(pathEscapeSegments('a/b/c')).toEqual('a/b/c'); + expect(pathEscapeSegments('a/b/ c')).toEqual('a/b/%20c'); + expect(pathEscapeSegments('a/b+c')).toEqual('a/b+c'); + }); }); test('linkifyURLs', () => { diff --git a/web_src/js/utils/url.ts b/web_src/js/utils/url.ts index 469693373aa..927f0937a4f 100644 --- a/web_src/js/utils/url.ts +++ b/web_src/js/utils/url.ts @@ -1,5 +1,33 @@ +export function urlQueryEscape(s: string) { + // See "TestQueryEscape" in backend + // https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/encodeURIComponent#encoding_for_rfc3986 + return encodeURIComponent(s).replace( + /[!'()*]/g, + (c) => `%${c.charCodeAt(0).toString(16).toUpperCase()}`, + ).replaceAll('%20', '+'); +} + +export function pathEscape(s: string): string { + // See "TestPathEscape" in backend + return encodeURIComponent(s).replace( + /[!'()*]/g, + (c) => `%${c.charCodeAt(0).toString(16).toUpperCase()}`, + ).replaceAll(/%(\w\w)/g, (v) => { + switch (v) { + case '%24': return '$'; + case '%26': return '&'; + case '%2B': return '+'; + case '%3A': return ':'; + case '%3D': return '='; + case '%40': return '@'; + default: return v; + } + }); +} + export function pathEscapeSegments(s: string): string { - return s.split('/').map(encodeURIComponent).join('/'); + // The same as backend's PathEscapeSegments + return s.split('/').map(pathEscape).join('/'); } // Match HTML tags (to skip) or URLs (to linkify) in HTML content