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.
This commit is contained in:
wxiaoguang 2025-07-06 13:36:45 +08:00 committed by GitHub
parent 429efc8b4f
commit 3533263ced
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
10 changed files with 80 additions and 41 deletions

View File

@ -40,7 +40,6 @@ func NewFuncMap() template.FuncMap {
"HTMLFormat": htmlFormat, "HTMLFormat": htmlFormat,
"QueryEscape": queryEscape, "QueryEscape": queryEscape,
"QueryBuild": QueryBuild, "QueryBuild": QueryBuild,
"JSEscape": jsEscapeSafe,
"SanitizeHTML": SanitizeHTML, "SanitizeHTML": SanitizeHTML,
"URLJoin": util.URLJoin, "URLJoin": util.URLJoin,
"DotEscape": dotEscape, "DotEscape": dotEscape,
@ -181,10 +180,6 @@ func htmlFormat(s any, args ...any) template.HTML {
panic(fmt.Sprintf("unexpected type %T", s)) 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 { func queryEscape(s string) template.URL {
return template.URL(url.QueryEscape(s)) return template.URL(url.QueryEscape(s))
} }

View File

@ -57,10 +57,6 @@ func TestSubjectBodySeparator(t *testing.T) {
"Insufficient\n--\nSeparators") "Insufficient\n--\nSeparators")
} }
func TestJSEscapeSafe(t *testing.T) {
assert.EqualValues(t, `\u0026\u003C\u003E\'\"`, jsEscapeSafe(`&<>'"`))
}
func TestSanitizeHTML(t *testing.T) { 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>`)) 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>`))
} }

View File

@ -10,6 +10,7 @@ import (
"net/http" "net/http"
"net/url" "net/url"
"strconv" "strconv"
"strings"
"code.gitea.io/gitea/models/auth" "code.gitea.io/gitea/models/auth"
user_model "code.gitea.io/gitea/models/user" user_model "code.gitea.io/gitea/models/user"
@ -161,9 +162,7 @@ func IntrospectOAuth(ctx *context.Context) {
if err == nil && app != nil { if err == nil && app != nil {
response.Active = true response.Active = true
response.Scope = grant.Scope response.Scope = grant.Scope
response.Issuer = setting.AppURL response.RegisteredClaims = oauth2_provider.NewJwtRegisteredClaimsFromUser(app.ClientID, grant.UserID, nil /*exp*/)
response.Audience = []string{app.ClientID}
response.Subject = strconv.FormatInt(grant.UserID, 10)
} }
if user, err := user_model.GetUserByID(ctx, grant.UserID); err == nil { if user, err := user_model.GetUserByID(ctx, grant.UserID); err == nil {
response.Username = user.Name response.Username = user.Name
@ -423,7 +422,14 @@ func GrantApplicationOAuth(ctx *context.Context) {
// OIDCWellKnown generates JSON so OIDC clients know Gitea's capabilities // OIDCWellKnown generates JSON so OIDC clients know Gitea's capabilities
func OIDCWellKnown(ctx *context.Context) { 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") ctx.JSONTemplate("user/auth/oidc_wellknown")
} }

View File

@ -4,10 +4,15 @@
package web package web
import ( import (
"html/template"
"code.gitea.io/gitea/modules/setting"
"code.gitea.io/gitea/services/context" "code.gitea.io/gitea/services/context"
) )
// SwaggerV1Json render swagger v1 json // SwaggerV1Json render swagger v1 json
func SwaggerV1Json(ctx *context.Context) { 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") ctx.JSONTemplate("swagger/v1_json")
} }

View File

@ -92,7 +92,7 @@ func (ctx *Context) HTML(status int, name templates.TplName) {
} }
// JSONTemplate renders the template as JSON response // 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) { func (ctx *Context) JSONTemplate(tmpl templates.TplName) {
t, err := ctx.Render.TemplateLookup(string(tmpl), nil) t, err := ctx.Render.TemplateLookup(string(tmpl), nil)
if err != nil { if err != nil {

View File

@ -106,6 +106,20 @@ func GrantAdditionalScopes(grantScopes string) auth.AccessTokenScope {
return auth.AccessTokenScopeAll 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) { func NewAccessTokenResponse(ctx context.Context, grant *auth.OAuth2Grant, serverKey, clientKey JWTSigningKey) (*AccessTokenResponse, *AccessTokenError) {
if setting.OAuth2.InvalidateRefreshTokens { if setting.OAuth2.InvalidateRefreshTokens {
if err := grant.IncreaseCounter(ctx); err != nil { if err := grant.IncreaseCounter(ctx); err != nil {
@ -176,12 +190,7 @@ func NewAccessTokenResponse(ctx context.Context, grant *auth.OAuth2Grant, server
} }
idToken := &OIDCToken{ idToken := &OIDCToken{
RegisteredClaims: jwt.RegisteredClaims{ RegisteredClaims: NewJwtRegisteredClaimsFromUser(app.ClientID, grant.UserID, jwt.NewNumericDate(expirationDate.AsTime())),
ExpiresAt: jwt.NewNumericDate(expirationDate.AsTime()),
Issuer: setting.AppURL,
Audience: []string{app.ClientID},
Subject: strconv.FormatInt(grant.UserID, 10),
},
Nonce: grant.Nonce, Nonce: grant.Nonce,
} }
if grant.ScopeContains("profile") { if grant.ScopeContains("profile") {

View File

@ -1,6 +1,6 @@
{ {
"info": { "info": {
"version": "{{AppVer | JSEscape}}" "version": "{{.SwaggerAppVer}}"
}, },
"basePath": "{{AppSubUrl | JSEscape}}/api/v1" "basePath": "{{.SwaggerAppSubUrl}}/api/v1"
} }

View File

@ -19,9 +19,9 @@
"name": "MIT", "name": "MIT",
"url": "http://opensource.org/licenses/MIT" "url": "http://opensource.org/licenses/MIT"
}, },
"version": "{{AppVer | JSEscape}}" "version": "{{.SwaggerAppVer}}"
}, },
"basePath": "{{AppSubUrl | JSEscape}}/api/v1", "basePath": "{{.SwaggerAppSubUrl}}/api/v1",
"paths": { "paths": {
"/activitypub/user-id/{user-id}": { "/activitypub/user-id/{user-id}": {
"get": { "get": {

View File

@ -1,16 +1,16 @@
{ {
"issuer": "{{AppUrl | JSEscape}}", "issuer": "{{.OidcIssuer}}",
"authorization_endpoint": "{{AppUrl | JSEscape}}login/oauth/authorize", "authorization_endpoint": "{{.OidcBaseUrl}}/login/oauth/authorize",
"token_endpoint": "{{AppUrl | JSEscape}}login/oauth/access_token", "token_endpoint": "{{.OidcBaseUrl}}/login/oauth/access_token",
"jwks_uri": "{{AppUrl | JSEscape}}login/oauth/keys", "jwks_uri": "{{.OidcBaseUrl}}/login/oauth/keys",
"userinfo_endpoint": "{{AppUrl | JSEscape}}login/oauth/userinfo", "userinfo_endpoint": "{{.OidcBaseUrl}}/login/oauth/userinfo",
"introspection_endpoint": "{{AppUrl | JSEscape}}login/oauth/introspect", "introspection_endpoint": "{{.OidcBaseUrl}}/login/oauth/introspect",
"response_types_supported": [ "response_types_supported": [
"code", "code",
"id_token" "id_token"
], ],
"id_token_signing_alg_values_supported": [ "id_token_signing_alg_values_supported": [
"{{.SigningKey.SigningMethod.Alg | JSEscape}}" "{{.SigningKeyMethodAlg}}"
], ],
"subject_types_supported": [ "subject_types_supported": [
"public" "public"

View File

@ -19,6 +19,7 @@ import (
"code.gitea.io/gitea/modules/json" "code.gitea.io/gitea/modules/json"
"code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/setting"
api "code.gitea.io/gitea/modules/structs" api "code.gitea.io/gitea/modules/structs"
"code.gitea.io/gitea/modules/test"
"code.gitea.io/gitea/services/oauth2_provider" "code.gitea.io/gitea/services/oauth2_provider"
"code.gitea.io/gitea/tests" "code.gitea.io/gitea/tests"
@ -26,24 +27,33 @@ import (
"github.com/stretchr/testify/require" "github.com/stretchr/testify/require"
) )
func TestAuthorizeNoClientID(t *testing.T) { func TestOAuth2Provider(t *testing.T) {
defer tests.PrepareTestEnv(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") req := NewRequest(t, "GET", "/login/oauth/authorize")
ctx := loginUser(t, "user2") ctx := loginUser(t, "user2")
resp := ctx.MakeRequest(t, req, http.StatusBadRequest) resp := ctx.MakeRequest(t, req, http.StatusBadRequest)
assert.Contains(t, resp.Body.String(), "Client ID not registered") assert.Contains(t, resp.Body.String(), "Client ID not registered")
} }
func TestAuthorizeUnregisteredRedirect(t *testing.T) { func testAuthorizeUnregisteredRedirect(t *testing.T) {
defer tests.PrepareTestEnv(t)()
req := NewRequest(t, "GET", "/login/oauth/authorize?client_id=da7da3ba-9a13-4167-856f-3899de0b0138&redirect_uri=UNREGISTERED&response_type=code&state=thestate") 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") ctx := loginUser(t, "user1")
resp := ctx.MakeRequest(t, req, http.StatusBadRequest) resp := ctx.MakeRequest(t, req, http.StatusBadRequest)
assert.Contains(t, resp.Body.String(), "Unregistered Redirect URI") assert.Contains(t, resp.Body.String(), "Unregistered Redirect URI")
} }
func TestAuthorizeUnsupportedResponseType(t *testing.T) { func testAuthorizeUnsupportedResponseType(t *testing.T) {
defer tests.PrepareTestEnv(t)()
req := NewRequest(t, "GET", "/login/oauth/authorize?client_id=da7da3ba-9a13-4167-856f-3899de0b0138&redirect_uri=a&response_type=UNEXPECTED&state=thestate") 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") ctx := loginUser(t, "user1")
resp := ctx.MakeRequest(t, req, http.StatusSeeOther) 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")) assert.Equal(t, "Only code response type is supported.", u.Query().Get("error_description"))
} }
func TestAuthorizeUnsupportedCodeChallengeMethod(t *testing.T) { func testAuthorizeUnsupportedCodeChallengeMethod(t *testing.T) {
defer tests.PrepareTestEnv(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") 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") ctx := loginUser(t, "user1")
resp := ctx.MakeRequest(t, req, http.StatusSeeOther) 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")) assert.Equal(t, "unsupported code challenge method", u.Query().Get("error_description"))
} }
func TestAuthorizeLoginRedirect(t *testing.T) { func testAuthorizeLoginRedirect(t *testing.T) {
defer tests.PrepareTestEnv(t)()
req := NewRequest(t, "GET", "/login/oauth/authorize") req := NewRequest(t, "GET", "/login/oauth/authorize")
assert.Contains(t, MakeRequest(t, req, http.StatusSeeOther).Body.String(), "/user/login") 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) 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)
}