From 3533263ced8a6f3aaedaacedf411f7f392bf23ec Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Sun, 6 Jul 2025 13:36:45 +0800 Subject: [PATCH] Improve OAuth2 provider (correct Issuer, respect ENABLED) (#34966) 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. --- modules/templates/helper.go | 5 --- modules/templates/helper_test.go | 4 --- routers/web/auth/oauth2_provider.go | 14 +++++--- routers/web/swagger_json.go | 5 +++ services/context/context_response.go | 2 +- services/oauth2_provider/access_token.go | 23 ++++++++---- templates/swagger/v1_input.json | 4 +-- templates/swagger/v1_json.tmpl | 4 +-- templates/user/auth/oidc_wellknown.tmpl | 14 ++++---- tests/integration/oauth_test.go | 46 +++++++++++++++++++----- 10 files changed, 80 insertions(+), 41 deletions(-) diff --git a/modules/templates/helper.go b/modules/templates/helper.go index ff3f7cfda1..e454bce4bd 100644 --- 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 index 81f8235bd2..7e3a952e7b 100644 --- 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(`link xss
inline
`), SanitizeHTML(`link xss
inline
`)) } diff --git a/routers/web/auth/oauth2_provider.go b/routers/web/auth/oauth2_provider.go index dc9f34fd44..79989d8fbe 100644 --- 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 index fc39b504a9..52f6beaf59 100644 --- 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 index 4e11e29b69..3f64fc7352 100644 --- 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 index e01777031b..5a190d8616 100644 --- 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 index 1979febebb..e74c8fc9c4 100644 --- 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 index 879f59df2e..1cc3735b2b 100644 --- a/templates/swagger/v1_json.tmpl +++ b/templates/swagger/v1_json.tmpl @@ -19,9 +19,9 @@ "name": "MIT", "url": "http://opensource.org/licenses/MIT" }, - "version": "{{AppVer | JSEscape}}" + "version": "{{.SwaggerAppVer}}" }, - "basePath": "{{AppSubUrl | JSEscape}}/api/v1", + "basePath": "{{.SwaggerAppSubUrl}}/api/v1", "paths": { "/activitypub/user-id/{user-id}": { "get": { diff --git a/templates/user/auth/oidc_wellknown.tmpl b/templates/user/auth/oidc_wellknown.tmpl index 54bb4a763d..52c9a1b788 100644 --- 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 index d2228bae79..f8bc33c32a 100644 --- a/tests/integration/oauth_test.go +++ b/tests/integration/oauth_test.go @@ -19,6 +19,7 @@ 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" @@ -26,24 +27,33 @@ import ( "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) +}