Improve OAuth2 provider (correct Issuer, respect ENABLED) (#34966)

wxiaoguang · web-flow · commit 3533263ced8a · 2025-07-06T13:36:45.000+08:00
1. Make "Issuer" strictly follow the spec (see comment)
2. Make "/.well-known/openid-configuration" respond 404 if the OAuth2
provider is not enabled.

Then by the way, remove the JSEscape template helper because it is not
needed any more.
diff --git a/modules/templates/helper.go b/modules/templates/helper.go
@@ -40,7 +40,6 @@ func NewFuncMap() template.FuncMap {
 		"HTMLFormat":   htmlFormat,
 		"QueryEscape":  queryEscape,
 		"QueryBuild":   QueryBuild,
-		"JSEscape":     jsEscapeSafe,
 		"SanitizeHTML": SanitizeHTML,
 		"URLJoin":      util.URLJoin,
 		"DotEscape":    dotEscape,
@@ -181,10 +180,6 @@ func htmlFormat(s any, args ...any) template.HTML {
 	panic(fmt.Sprintf("unexpected type %T", s))
 }
 
-func jsEscapeSafe(s string) template.HTML {
-	return template.HTML(template.JSEscapeString(s))
-}
-
 func queryEscape(s string) template.URL {
 	return template.URL(url.QueryEscape(s))
 }
diff --git a/modules/templates/helper_test.go b/modules/templates/helper_test.go
@@ -57,10 +57,6 @@ func TestSubjectBodySeparator(t *testing.T) {
 		"Insufficient\n--\nSeparators")
 }
 
-func TestJSEscapeSafe(t *testing.T) {
-	assert.EqualValues(t, `\u0026\u003C\u003E\'\"`, jsEscapeSafe(`&<>'"`))
-}
-
 func TestSanitizeHTML(t *testing.T) {
 	assert.Equal(t, template.HTML(`<a href="/" rel="nofollow">link</a> xss <div>inline</div>`), SanitizeHTML(`<a href="/">link</a> <a href="javascript:">xss</a> <div style="dangerous">inline</div>`))
 }
diff --git a/routers/web/auth/oauth2_provider.go b/routers/web/auth/oauth2_provider.go
@@ -10,6 +10,7 @@ import (
 	"net/http"
 	"net/url"
 	"strconv"
+	"strings"
 
 	"code.gitea.io/gitea/models/auth"
 	user_model "code.gitea.io/gitea/models/user"
@@ -161,9 +162,7 @@ func IntrospectOAuth(ctx *context.Context) {
 			if err == nil && app != nil {
 				response.Active = true
 				response.Scope = grant.Scope
-				response.Issuer = setting.AppURL
-				response.Audience = []string{app.ClientID}
-				response.Subject = strconv.FormatInt(grant.UserID, 10)
+				response.RegisteredClaims = oauth2_provider.NewJwtRegisteredClaimsFromUser(app.ClientID, grant.UserID, nil /*exp*/)
 			}
 			if user, err := user_model.GetUserByID(ctx, grant.UserID); err == nil {
 				response.Username = user.Name
@@ -423,7 +422,14 @@ func GrantApplicationOAuth(ctx *context.Context) {
 
 // OIDCWellKnown generates JSON so OIDC clients know Gitea's capabilities
 func OIDCWellKnown(ctx *context.Context) {
-	ctx.Data["SigningKey"] = oauth2_provider.DefaultSigningKey
+	if !setting.OAuth2.Enabled {
+		http.NotFound(ctx.Resp, ctx.Req)
+		return
+	}
+	jwtRegisteredClaims := oauth2_provider.NewJwtRegisteredClaimsFromUser("well-known", 0, nil)
+	ctx.Data["OidcIssuer"] = jwtRegisteredClaims.Issuer // use the consistent issuer from the JWT registered claims
+	ctx.Data["OidcBaseUrl"] = strings.TrimSuffix(setting.AppURL, "/")
+	ctx.Data["SigningKeyMethodAlg"] = oauth2_provider.DefaultSigningKey.SigningMethod().Alg()
 	ctx.JSONTemplate("user/auth/oidc_wellknown")
 }
 
diff --git a/routers/web/swagger_json.go b/routers/web/swagger_json.go
@@ -4,10 +4,15 @@
 package web
 
 import (
+	"html/template"
+
+	"code.gitea.io/gitea/modules/setting"
 	"code.gitea.io/gitea/services/context"
 )
 
 // SwaggerV1Json render swagger v1 json
 func SwaggerV1Json(ctx *context.Context) {
+	ctx.Data["SwaggerAppVer"] = template.HTML(template.JSEscapeString(setting.AppVer))
+	ctx.Data["SwaggerAppSubUrl"] = setting.AppSubURL // it is JS-safe
 	ctx.JSONTemplate("swagger/v1_json")
 }
diff --git a/services/context/context_response.go b/services/context/context_response.go
@@ -92,7 +92,7 @@ func (ctx *Context) HTML(status int, name templates.TplName) {
 }
 
 // JSONTemplate renders the template as JSON response
-// keep in mind that the template is processed in HTML context, so JSON-things should be handled carefully, eg: by JSEscape
+// keep in mind that the template is processed in HTML context, so JSON things should be handled carefully, e.g.: use JSEscape
 func (ctx *Context) JSONTemplate(tmpl templates.TplName) {
 	t, err := ctx.Render.TemplateLookup(string(tmpl), nil)
 	if err != nil {
diff --git a/services/oauth2_provider/access_token.go b/services/oauth2_provider/access_token.go
@@ -106,6 +106,20 @@ func GrantAdditionalScopes(grantScopes string) auth.AccessTokenScope {
 	return auth.AccessTokenScopeAll
 }
 
+func NewJwtRegisteredClaimsFromUser(clientID string, grantUserID int64, exp *jwt.NumericDate) jwt.RegisteredClaims {
+	// https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderConfig
+	// The issuer value returned MUST be identical to the Issuer URL that was used as the prefix to /.well-known/openid-configuration
+	// to retrieve the configuration information. This MUST also be identical to the "iss" Claim value in ID Tokens issued from this Issuer.
+	// * https://accounts.google.com/.well-known/openid-configuration
+	// * https://github.com/login/oauth/.well-known/openid-configuration
+	return jwt.RegisteredClaims{
+		Issuer:    strings.TrimSuffix(setting.AppURL, "/"),
+		Audience:  []string{clientID},
+		Subject:   strconv.FormatInt(grantUserID, 10),
+		ExpiresAt: exp,
+	}
+}
+
 func NewAccessTokenResponse(ctx context.Context, grant *auth.OAuth2Grant, serverKey, clientKey JWTSigningKey) (*AccessTokenResponse, *AccessTokenError) {
 	if setting.OAuth2.InvalidateRefreshTokens {
 		if err := grant.IncreaseCounter(ctx); err != nil {
@@ -176,13 +190,8 @@ func NewAccessTokenResponse(ctx context.Context, grant *auth.OAuth2Grant, server
 		}
 
 		idToken := &OIDCToken{
-			RegisteredClaims: jwt.RegisteredClaims{
-				ExpiresAt: jwt.NewNumericDate(expirationDate.AsTime()),
-				Issuer:    setting.AppURL,
-				Audience:  []string{app.ClientID},
-				Subject:   strconv.FormatInt(grant.UserID, 10),
-			},
-			Nonce: grant.Nonce,
+			RegisteredClaims: NewJwtRegisteredClaimsFromUser(app.ClientID, grant.UserID, jwt.NewNumericDate(expirationDate.AsTime())),
+			Nonce:            grant.Nonce,
 		}
 		if grant.ScopeContains("profile") {
 			idToken.Name = user.DisplayName()
diff --git a/templates/swagger/v1_input.json b/templates/swagger/v1_input.json
@@ -1,6 +1,6 @@
 {
   "info": {
-    "version": "{{AppVer | JSEscape}}"
+    "version": "{{.SwaggerAppVer}}"
   },
-  "basePath": "{{AppSubUrl | JSEscape}}/api/v1"
+  "basePath": "{{.SwaggerAppSubUrl}}/api/v1"
 }
diff --git a/templates/swagger/v1_json.tmpl b/templates/swagger/v1_json.tmpl
diff --git a/templates/user/auth/oidc_wellknown.tmpl b/templates/user/auth/oidc_wellknown.tmpl
@@ -1,16 +1,16 @@
 {
-    "issuer": "{{AppUrl | JSEscape}}",
-    "authorization_endpoint": "{{AppUrl | JSEscape}}login/oauth/authorize",
-    "token_endpoint": "{{AppUrl | JSEscape}}login/oauth/access_token",
-    "jwks_uri": "{{AppUrl | JSEscape}}login/oauth/keys",
-    "userinfo_endpoint": "{{AppUrl | JSEscape}}login/oauth/userinfo",
-    "introspection_endpoint": "{{AppUrl | JSEscape}}login/oauth/introspect",
+    "issuer": "{{.OidcIssuer}}",
+    "authorization_endpoint": "{{.OidcBaseUrl}}/login/oauth/authorize",
+    "token_endpoint": "{{.OidcBaseUrl}}/login/oauth/access_token",
+    "jwks_uri": "{{.OidcBaseUrl}}/login/oauth/keys",
+    "userinfo_endpoint": "{{.OidcBaseUrl}}/login/oauth/userinfo",
+    "introspection_endpoint": "{{.OidcBaseUrl}}/login/oauth/introspect",
     "response_types_supported": [
         "code",
         "id_token"
     ],
     "id_token_signing_alg_values_supported": [
-        "{{.SigningKey.SigningMethod.Alg | JSEscape}}"
+        "{{.SigningKeyMethodAlg}}"
     ],
     "subject_types_supported": [
         "public"
diff --git a/tests/integration/oauth_test.go b/tests/integration/oauth_test.go
@@ -19,31 +19,41 @@ import (
 	"code.gitea.io/gitea/modules/json"
 	"code.gitea.io/gitea/modules/setting"
 	api "code.gitea.io/gitea/modules/structs"
+	"code.gitea.io/gitea/modules/test"
 	"code.gitea.io/gitea/services/oauth2_provider"
 	"code.gitea.io/gitea/tests"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
-func TestAuthorizeNoClientID(t *testing.T) {
+func TestOAuth2Provider(t *testing.T) {
 	defer tests.PrepareTestEnv(t)()
+
+	t.Run("AuthorizeNoClientID", testAuthorizeNoClientID)
+	t.Run("AuthorizeUnregisteredRedirect", testAuthorizeUnregisteredRedirect)
+	t.Run("AuthorizeUnsupportedResponseType", testAuthorizeUnsupportedResponseType)
+	t.Run("AuthorizeUnsupportedCodeChallengeMethod", testAuthorizeUnsupportedCodeChallengeMethod)
+	t.Run("AuthorizeLoginRedirect", testAuthorizeLoginRedirect)
+
+	t.Run("OAuth2WellKnown", testOAuth2WellKnown)
+}
+
+func testAuthorizeNoClientID(t *testing.T) {
 	req := NewRequest(t, "GET", "/login/oauth/authorize")
 	ctx := loginUser(t, "user2")
 	resp := ctx.MakeRequest(t, req, http.StatusBadRequest)
 	assert.Contains(t, resp.Body.String(), "Client ID not registered")
 }
 
-func TestAuthorizeUnregisteredRedirect(t *testing.T) {
-	defer tests.PrepareTestEnv(t)()
+func testAuthorizeUnregisteredRedirect(t *testing.T) {
 	req := NewRequest(t, "GET", "/login/oauth/authorize?client_id=da7da3ba-9a13-4167-856f-3899de0b0138&redirect_uri=UNREGISTERED&response_type=code&state=thestate")
 	ctx := loginUser(t, "user1")
 	resp := ctx.MakeRequest(t, req, http.StatusBadRequest)
 	assert.Contains(t, resp.Body.String(), "Unregistered Redirect URI")
 }
 
-func TestAuthorizeUnsupportedResponseType(t *testing.T) {
-	defer tests.PrepareTestEnv(t)()
+func testAuthorizeUnsupportedResponseType(t *testing.T) {
 	req := NewRequest(t, "GET", "/login/oauth/authorize?client_id=da7da3ba-9a13-4167-856f-3899de0b0138&redirect_uri=a&response_type=UNEXPECTED&state=thestate")
 	ctx := loginUser(t, "user1")
 	resp := ctx.MakeRequest(t, req, http.StatusSeeOther)
@@ -53,8 +63,7 @@ func TestAuthorizeUnsupportedResponseType(t *testing.T) {
 	assert.Equal(t, "Only code response type is supported.", u.Query().Get("error_description"))
 }
 
-func TestAuthorizeUnsupportedCodeChallengeMethod(t *testing.T) {
-	defer tests.PrepareTestEnv(t)()
+func testAuthorizeUnsupportedCodeChallengeMethod(t *testing.T) {
 	req := NewRequest(t, "GET", "/login/oauth/authorize?client_id=da7da3ba-9a13-4167-856f-3899de0b0138&redirect_uri=a&response_type=code&state=thestate&code_challenge_method=UNEXPECTED")
 	ctx := loginUser(t, "user1")
 	resp := ctx.MakeRequest(t, req, http.StatusSeeOther)
@@ -64,8 +73,7 @@ func TestAuthorizeUnsupportedCodeChallengeMethod(t *testing.T) {
 	assert.Equal(t, "unsupported code challenge method", u.Query().Get("error_description"))
 }
 
-func TestAuthorizeLoginRedirect(t *testing.T) {
-	defer tests.PrepareTestEnv(t)()
+func testAuthorizeLoginRedirect(t *testing.T) {
 	req := NewRequest(t, "GET", "/login/oauth/authorize")
 	assert.Contains(t, MakeRequest(t, req, http.StatusSeeOther).Body.String(), "/user/login")
 }
@@ -903,3 +911,23 @@ func TestOAuth_GrantScopesClaimAllGroups(t *testing.T) {
 		assert.Contains(t, userinfoParsed.Groups, group)
 	}
 }
+
+func testOAuth2WellKnown(t *testing.T) {
+	urlOpenidConfiguration := "/.well-known/openid-configuration"
+
+	defer test.MockVariableValue(&setting.AppURL, "https://try.gitea.io/")()
+	req := NewRequest(t, "GET", urlOpenidConfiguration)
+	resp := MakeRequest(t, req, http.StatusOK)
+	var respMap map[string]any
+	DecodeJSON(t, resp, &respMap)
+	assert.Equal(t, "https://try.gitea.io", respMap["issuer"])
+	assert.Equal(t, "https://try.gitea.io/login/oauth/authorize", respMap["authorization_endpoint"])
+	assert.Equal(t, "https://try.gitea.io/login/oauth/access_token", respMap["token_endpoint"])
+	assert.Equal(t, "https://try.gitea.io/login/oauth/keys", respMap["jwks_uri"])
+	assert.Equal(t, "https://try.gitea.io/login/oauth/userinfo", respMap["userinfo_endpoint"])
+	assert.Equal(t, "https://try.gitea.io/login/oauth/introspect", respMap["introspection_endpoint"])
+	assert.Equal(t, []any{"RS256"}, respMap["id_token_signing_alg_values_supported"])
+
+	defer test.MockVariableValue(&setting.OAuth2.Enabled, false)()
+	MakeRequest(t, NewRequest(t, "GET", urlOpenidConfiguration), http.StatusNotFound)
+}

Original file line number	Diff line number	Diff line change
`@@ -57,10 +57,6 @@ func TestSubjectBodySeparator(t *testing.T) {`
`57`	`57`	`"Insufficient\n--\nSeparators")`
`58`	`58`	`}`
`59`	`59`
`60`		`-func TestJSEscapeSafe(t *testing.T) {`
`61`		- assert.EqualValues(t, `\u0026\u003C\u003E\'\"`, jsEscapeSafe(`&<>'"`))
`62`		`-}`
`63`		`-`
`64`	`60`	`func TestSanitizeHTML(t *testing.T) {`
`65`	`61`	assert.Equal(t, template.HTML(`<a href="/" rel="nofollow">link</a> xss <div>inline</div>`), SanitizeHTML(`<a href="/">link</a> <a href="javascript:">xss</a> <div style="dangerous">inline</div>`))
`66`	`62`	`}`
Original file line number	Diff line number	Diff line change
`@@ -4,10 +4,15 @@`
`4`	`4`	`package web`
`5`	`5`
`6`	`6`	`import (`
	`7`	`+ "html/template"`
	`8`	`+`
	`9`	`+ "code.gitea.io/gitea/modules/setting"`
`7`	`10`	`"code.gitea.io/gitea/services/context"`
`8`	`11`	`)`
`9`	`12`
`10`	`13`	`// SwaggerV1Json render swagger v1 json`
`11`	`14`	`func SwaggerV1Json(ctx *context.Context) {`
	`15`	`+ ctx.Data["SwaggerAppVer"] = template.HTML(template.JSEscapeString(setting.AppVer))`
	`16`	`+ ctx.Data["SwaggerAppSubUrl"] = setting.AppSubURL // it is JS-safe`
`12`	`17`	`ctx.JSONTemplate("swagger/v1_json")`
`13`	`18`	`}`
Original file line number	Diff line number	Diff line change
`@@ -92,7 +92,7 @@ func (ctx *Context) HTML(status int, name templates.TplName) {`
`92`	`92`	`}`
`93`	`93`
`94`	`94`	`// JSONTemplate renders the template as JSON response`
`95`		`-// keep in mind that the template is processed in HTML context, so JSON-things should be handled carefully, eg: by JSEscape`
	`95`	`+// keep in mind that the template is processed in HTML context, so JSON things should be handled carefully, e.g.: use JSEscape`
`96`	`96`	`func (ctx *Context) JSONTemplate(tmpl templates.TplName) {`
`97`	`97`	`t, err := ctx.Render.TemplateLookup(string(tmpl), nil)`
`98`	`98`	`if err != nil {`
Original file line number	Diff line number	Diff line change
`@@ -1,6 +1,6 @@`
`1`	`1`	`{`
`2`	`2`	`"info": {`
`3`		`- "version": "{{AppVer \| JSEscape}}"`
	`3`	`+ "version": "{{.SwaggerAppVer}}"`
`4`	`4`	`},`
`5`		`- "basePath": "{{AppSubUrl \| JSEscape}}/api/v1"`
	`5`	`+ "basePath": "{{.SwaggerAppSubUrl}}/api/v1"`
`6`	`6`	`}`