Fix URL related escaping for oauth2 (#37334)

Follow up #37327. See the comments.

* Root problem: the design of OAuth2 providers is a mess, the display
name is used as provider's name and used in the URL directly
* The regressions:
* When trying to fix https://github.com/go-gitea/gitea/issues/36409 , it
introduced inconsistent URL escaping for the "path" part.
* This fix: always use "path escaping" for the path part, add more tests
to cover all escaping cases.

Now, frontend "pathEscape" and "pathEscapeSegments" generate exactly the
same result as backend.

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

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

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"))
+}

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

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, "")

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)

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

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

templates/user/auth/external_auth_methods.tmpl

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

templates/user/settings/security/accountlinks.tmpl

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

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

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<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();

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('<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'});

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

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

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