fix: validate "scope" for OAuth2, "state" for OpenID (#39642)

1. Fixes: https://github.com/go-gitea/gitea/issues/39641
* The OAuth2 authorize/grant endpoint accepted and persisted arbitrary
scope strings without validation, unlike personal access token creation
which rejects unknown scopes. Normalize and validate form.Scope before
creating or updating a grant, ignore invalid scopes
* `// TODO: at the moment invalid tokens are treated as "all" to avoid
breaking 1.22 behavior` is fixed.
2. To avoid CSRF attack for OpenID endpoint, add a state check

---------

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>

routers/web/auth/oauth2_provider.go

@@ -197,7 +197,7 @@ func IntrospectOAuth(ctx *context.Context) {
 	}
 
 	response.Active = true
-	response.Scope = grant.Scope
+	response.Scope = oauth2_provider.NormalizeGrantScope(grant.Scope)
 	response.RegisteredClaims = oauth2_provider.NewJwtRegisteredClaimsFromUser(introspectingApp.ClientID, grant.UserID, nil /*exp*/)
 	user, err := user_model.GetUserByID(ctx, grant.UserID)
 	if err != nil {
@@ -323,12 +323,15 @@ func AuthorizeOAuth(ctx *context.Context) {
 		return
 	}
 
+	if grant != nil && form.Scope == "" {
+		form.Scope = grant.Scope
+	}
+	form.Scope = oauth2_provider.NormalizeGrantScope(form.Scope)
+
 	var addedScopes, removedScopes []string
 	if grant != nil {
-		if form.Scope == "" {
-			form.Scope = grant.Scope
-		}
-		addedScopes, removedScopes = util.DiffSlice(strings.Fields(grant.Scope), strings.Fields(form.Scope))
+		grantedScope := oauth2_provider.NormalizeGrantScope(grant.Scope)
+		addedScopes, removedScopes = util.DiffSlice(strings.Fields(grantedScope), strings.Fields(form.Scope))
 	}
 	scopeChanged := len(addedScopes) > 0 || len(removedScopes) > 0
 
@@ -431,6 +434,8 @@ func GrantApplicationOAuth(ctx *context.Context) {
 		handleServerError(ctx, form.State, form.RedirectURI)
 		return
 	}
+	form.Scope = oauth2_provider.NormalizeGrantScope(form.Scope)
+
 	if grant == nil {
 		grant, err = app.CreateGrant(ctx, ctx.Doer.ID, form.Scope)
 		if err != nil {

routers/web/auth/oauth_test.go

@@ -6,6 +6,7 @@ package auth
 import (
 	"net/http"
 	"net/http/httptest"
+	"net/url"
 	"sync/atomic"
 	"testing"
 
@@ -32,6 +33,7 @@ func createAndParseToken(t *testing.T, grant *auth.OAuth2Grant) *oauth2_provider
 	response, terr := oauth2_provider.NewAccessTokenResponse(t.Context(), grant, signingKey, signingKey)
 	assert.Nil(t, terr)
 	assert.NotNil(t, response)
+	assert.Equal(t, grant.Scope, response.Scope)
 
 	parsedToken, err := jwt.ParseWithClaims(response.IDToken, &oauth2_provider.OIDCToken{}, func(token *jwt.Token) (any, error) {
 		assert.NotNil(t, token.Method)
@@ -115,21 +117,56 @@ func TestOAuth2ScopeChange(t *testing.T) {
 	app := unittest.AssertExistsAndLoadBean(t, &auth.OAuth2Application{ID: 1})
 	doer := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 1})
 	mockOpt := contexttest.MockContextOption{SessionStore: session.NewMockMemStore("oauth2-scope-change")}
-	authorize := func(scope string) int {
+	authorize := func(scope string) *httptest.ResponseRecorder {
 		ctx, resp := contexttest.MockContext(t, "/login/oauth/authorize", mockOpt)
 		ctx.Doer = doer
 		web.SetForm(ctx, &forms.AuthorizationForm{ResponseType: "code", ClientID: app.ClientID, RedirectURI: app.RedirectURIs[0], State: "state", Scope: scope})
 		AuthorizeOAuth(ctx)
-		return resp.Code
+		return resp
 	}
-	assert.Equal(t, http.StatusSeeOther, authorize(""))
-	assert.Equal(t, http.StatusSeeOther, authorize("profile openid"))
-	assert.Equal(t, http.StatusOK, authorize("openid profile email"))
+	assert.Equal(t, http.StatusSeeOther, authorize("").Code)
+	assert.Equal(t, http.StatusSeeOther, authorize("profile openid").Code)
+	grant := unittest.AssertExistsAndLoadBean(t, &auth.OAuth2Grant{ID: 1})
+	require.NoError(t, auth.UpdateGrantScope(t.Context(), grant, "openid profile read:user write:user"))
+	assert.Equal(t, http.StatusSeeOther, authorize("openid profile write:user").Code)
+	require.NoError(t, auth.UpdateGrantScope(t.Context(), grant, "openid invalid:scope"))
+	assert.Equal(t, http.StatusOK, authorize("openid profile").Code)
+	require.NoError(t, auth.UpdateGrantScope(t.Context(), grant, "openid profile"))
+	assert.Equal(t, http.StatusOK, authorize("openid profile email").Code)
 
-	ctx, resp := contexttest.MockContext(t, "/login/oauth/grant", mockOpt)
-	ctx.Doer = doer
-	web.SetForm(ctx, &forms.GrantApplicationForm{ClientID: app.ClientID, Granted: true, RedirectURI: app.RedirectURIs[0], State: "state", Scope: "openid profile email"})
-	GrantApplicationOAuth(ctx)
-	assert.Equal(t, http.StatusSeeOther, resp.Code)
-	unittest.AssertExistsAndLoadBean(t, &auth.OAuth2Grant{ID: 1, Scope: "openid profile email"})
+	for _, tt := range []struct {
+		name, scope, wantScope string
+	}{
+		{"only invalid scopes", "invalid:scope unsupported", "no-token-scope"},
+		{"OIDC with invalid scope", "openid invalid:scope", "openid no-token-scope"},
+		{"comma-only scope", ",", "no-token-scope"},
+		{"OIDC with comma-only scope", "openid ,,", "openid no-token-scope"},
+		{"OIDC scopes", "openid profile email", "openid profile email"},
+		{"mixed valid and invalid scopes", "openid groups read:user,invalid:scope public-only unsupported", "openid groups public-only read:user"},
+		{"normalized access scopes", " read:user  openid profile email read:user ", "openid profile email read:user"},
+		{"redundant read scope", "openid read:user write:user", "openid write:user"},
+		{"redundant scope with all", "openid all read:user", "openid all"},
+	} {
+		t.Run(tt.name, func(t *testing.T) {
+			resp := authorize(tt.scope)
+			require.Equal(t, http.StatusOK, resp.Code)
+			ctx, resp := contexttest.MockContext(t, "/login/oauth/grant", mockOpt)
+			ctx.Doer = doer
+			web.SetForm(ctx, &forms.GrantApplicationForm{ClientID: app.ClientID, Granted: true, RedirectURI: app.RedirectURIs[0], State: "state", Scope: tt.scope})
+			GrantApplicationOAuth(ctx)
+			assert.Equal(t, http.StatusSeeOther, resp.Code)
+			redirect, err := url.Parse(resp.Header().Get("Location"))
+			require.NoError(t, err)
+			assert.Empty(t, redirect.Query().Get("error"))
+			assert.Equal(t, "state", redirect.Query().Get("state"))
+			unittest.AssertExistsAndLoadBean(t, &auth.OAuth2Grant{ID: 1, Scope: tt.wantScope})
+			assert.NotEmpty(t, redirect.Query().Get("code"))
+			resp = authorize(tt.scope)
+			assert.Equal(t, http.StatusSeeOther, resp.Code)
+			redirect, err = url.Parse(resp.Header().Get("Location"))
+			require.NoError(t, err)
+			assert.NotEmpty(t, redirect.Query().Get("code"))
+			assert.Empty(t, redirect.Query().Get("error"))
+		})
+	}
 }

routers/web/user/setting/security/openid.go

@@ -5,11 +5,14 @@ package security
 
 import (
 	"errors"
+	"fmt"
 	"net/http"
+	"time"
 
 	audit_model "gitea.dev/models/audit"
 	user_model "gitea.dev/models/user"
 	"gitea.dev/modules/auth/openid"
+	"gitea.dev/modules/base"
 	"gitea.dev/modules/log"
 	"gitea.dev/modules/setting"
 	"gitea.dev/modules/util"
@@ -19,6 +22,8 @@ import (
 	"gitea.dev/services/forms"
 )
 
+const openIDAssociationCodeMinutes = 10
+
 // OpenIDPost response for change user's openid
 func OpenIDPost(ctx *context.Context) {
 	if user_model.IsFeatureDisabledWithLoginType(ctx.Doer, setting.UserFeatureManageCredentials) {
@@ -70,7 +75,8 @@ func OpenIDPost(ctx *context.Context) {
 		}
 	}
 
-	redirectTo := setting.AppURL + "user/settings/security"
+	state := base.CreateTimeLimitCode(fmt.Sprintf("openid_association:%d", ctx.Doer.ID), openIDAssociationCodeMinutes, time.Now(), nil)
+	redirectTo := setting.AppURL + "user/settings/security?state=" + state
 	url, err := openid.RedirectURL(id, redirectTo, setting.AppURL)
 	if err != nil {
 		loadSecurityData(ctx)
@@ -82,6 +88,11 @@ func OpenIDPost(ctx *context.Context) {
 }
 
 func settingsOpenIDVerify(ctx *context.Context) {
+	if !base.VerifyTimeLimitCode(time.Now(), fmt.Sprintf("openid_association:%d", ctx.Doer.ID), openIDAssociationCodeMinutes, ctx.FormString("state")) {
+		ctx.HTTPError(http.StatusForbidden)
+		return
+	}
+
 	log.Trace("Incoming call to: " + ctx.Req.URL.String())
 
 	fullURL := setting.AppURL + ctx.Req.URL.String()[1:]

routers/web/user/setting/security/openid_test.go

@@ -4,15 +4,103 @@
 package security
 
 import (
+	"fmt"
 	"net/http"
+	"net/http/httptest"
+	"net/url"
 	"testing"
+	"time"
 
 	"gitea.dev/models/unittest"
+	user_model "gitea.dev/models/user"
+	"gitea.dev/modules/base"
+	"gitea.dev/modules/setting"
+	"gitea.dev/modules/test"
+	"gitea.dev/modules/web"
 	"gitea.dev/services/contexttest"
+	"gitea.dev/services/forms"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
+func TestOpenIDAssociation(t *testing.T) {
+	unittest.PrepareTestEnv(t)
+	defer test.MockVariableValue(&setting.AppURL, "https://gitea.example/")()
+	defer test.MockVariableValue(&setting.AppSubURL, "")()
+	defer test.MockVariableValue(&setting.Security.AllowedHostList, "127.0.0.1:*")()
+
+	provider := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if r.Method == http.MethodPost {
+			assert.Equal(t, "check_authentication", r.FormValue("openid.mode"))
+			w.Header().Set("Content-Type", "text/plain")
+			fmt.Fprint(w, "ns:http://specs.openid.net/auth/2.0\nis_valid:true\n")
+			return
+		}
+		w.Header().Set("Content-Type", "text/html")
+		fmt.Fprintf(w, `<html><head><link rel="openid2.provider" href="http://%s/provider"></head></html>`, r.Host)
+	}))
+	defer provider.Close()
+	identity := provider.URL + "/identity"
+	ctx, resp := contexttest.MockContext(t, "POST /user/settings/security/openid")
+	contexttest.LoadUser(t, ctx, 2)
+	web.SetForm(ctx, &forms.AddOpenIDForm{Openid: identity})
+	OpenIDPost(ctx)
+	require.Equal(t, http.StatusSeeOther, resp.Code)
+	redirect, err := url.Parse(resp.Header().Get("Location"))
+	require.NoError(t, err)
+	callback, err := url.Parse(redirect.Query().Get("openid.return_to"))
+	require.NoError(t, err)
+	state := callback.Query().Get("state")
+	require.NotEmpty(t, state)
+
+	params := url.Values{
+		"openid.ns":             {"http://specs.openid.net/auth/2.0"},
+		"openid.mode":           {"id_res"},
+		"openid.op_endpoint":    {provider.URL + "/provider"},
+		"openid.claimed_id":     {identity},
+		"openid.identity":       {identity},
+		"openid.return_to":      {callback.String()},
+		"openid.response_nonce": {time.Now().UTC().Format("2006-01-02T15:04:05Z") + "association"},
+		"openid.assoc_handle":   {"test-association"},
+		"openid.signed":         {"op_endpoint,claimed_id,identity,return_to,response_nonce,assoc_handle"},
+		"openid.sig":            {"test-signature"},
+	}
+	for _, tc := range []struct {
+		name  string
+		uid   int64
+		state string
+	}{
+		{"unsolicited", 2, ""},
+		{"wrong state", 2, "wrong"},
+		{"other user", 1, state},
+		{"expired", 2, base.CreateTimeLimitCode("openid_association:2", openIDAssociationCodeMinutes, time.Now().Add(-time.Hour), nil)},
+		{"other purpose", 2, base.CreateTimeLimitCode("reset_password:2", openIDAssociationCodeMinutes, time.Now(), nil)},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			params.Set("state", tc.state)
+			ctx, resp := contexttest.MockContext(t, "GET /user/settings/security?"+params.Encode())
+			contexttest.LoadUser(t, ctx, tc.uid)
+			Security(ctx)
+			assert.Equal(t, http.StatusForbidden, resp.Code)
+			unittest.AssertNotExistsBean(t, &user_model.UserOpenID{URI: identity})
+		})
+	}
+
+	params.Set("state", state)
+	ctx, resp = contexttest.MockContext(t, "GET /user/settings/security?"+params.Encode())
+	contexttest.LoadUser(t, ctx, 2)
+	Security(ctx)
+	require.Equal(t, http.StatusSeeOther, resp.Code)
+	unittest.AssertExistsAndLoadBean(t, &user_model.UserOpenID{UID: 2, URI: identity})
+
+	ctx, resp = contexttest.MockContext(t, "GET /user/settings/security?"+params.Encode())
+	contexttest.LoadUser(t, ctx, 2)
+	Security(ctx)
+	assert.Equal(t, http.StatusOK, resp.Code)
+	assert.Equal(t, "Nonce already used", ctx.Flash.ErrorMsg)
+}
+
 func TestDeleteOpenIDReturnsNotFoundForOtherUsersAddress(t *testing.T) {
 	unittest.PrepareTestEnv(t)
 	ctx, _ := contexttest.MockContext(t, "POST /user/settings/security?id=1")