Mirrors/gitea
0
0
Fork 0
mirror of https://github.com/go-gitea/gitea.git synced 2026-05-15 17:11:10 +02:00
Code Issues Activity

Fix URL related escaping for oauth2 (#37334) (#37340)

Browse Source 
Backport #37334 by wxiaoguang

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
This commit is contained in:
Giteabot 2026-04-22 01:11:19 +08:00 committed by GitHub
parent 657ea10cf1
commit fc4296a21a
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
16 changed files with 135 additions and 60 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
models/auth/oauth2.go
View File

@ -633,7 +633,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

19
modules/templates/helper_test.go
View File

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

7
modules/util/util_test.go
View File

@ -192,3 +192,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"))
}

2
routers/web/auth/auth.go
View File

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

19
routers/web/auth/auth_test.go
View File

@ -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&#39;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, "")

4
routers/web/auth/oauth.go
View File

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

8
services/context/base_path.go
View File

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

11
services/context/context_response.go
View File

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

3
templates/user/auth/external_auth_methods.tmpl
View File

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

2
templates/user/settings/security/accountlinks.tmpl
View File

@ -9,7 +9,7 @@
<div class="menu">
{{range $key := .OrderedOAuth2Names}}
{{$provider := index $.OAuth2Providers $key}}
<a class="item" href="{{AppSubUrl}}/user/oauth2/{{$key}}">
<a class="item" href="{{AppSubUrl}}/user/oauth2/{{$key | PathEscape}}">
{{$provider.IconHTML 20}}
{{$provider.DisplayName}}
</a>

46
tests/integration/oauth_test.go
View File

@ -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
}
@ -1102,19 +1103,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)
}

4
web_src/js/features/admin/common.ts
View File

@ -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;
@ -231,7 +231,7 @@ function initAdminAuthentication() {
const elAuthName = document.querySelector<HTMLInputElement>('#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();

7
web_src/js/utils.test.ts
View File

@ -2,7 +2,6 @@ import {
dirname, basename, extname, isObject, stripTags, parseIssueHref,
parseUrl, translateMonth, translateDay, blobToDataURI,
toAbsoluteUrl, encodeURLEncodedBase64, decodeURLEncodedBase64, isImageFile, isVideoFile, parseRepoOwnerPathInfo,
urlQueryEscape,
} from './utils.ts';
test('dirname', () => {
@ -34,12 +33,6 @@ test('stripTags', () => {
expect(stripTags('<a>test</a>')).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'});

9
web_src/js/utils.ts
View File

@ -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(/[#?].*$/, '');

22
web_src/js/utils/url.test.ts
View File

@ -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', () => {

30
web_src/js/utils/url.ts
View File

@ -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