Merge pull request 'fix: PKCE only for OpenID Connect authentication sources' (#4094) from oliverpool/forgejo:pkce_goth_fix into forgejo

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/4094
Reviewed-by: Earl Warren <earl-warren@noreply.codeberg.org>

release-notes/8.0.0/feat/3307.md

@@ -1 +1 @@
-Support [Proof Key for Code Exchange (PKCE - RFC7636)](https://www.rfc-editor.org/rfc/rfc7636) for external login sources using the OAuth2 flow.
+Support [Proof Key for Code Exchange (PKCE - RFC7636)](https://www.rfc-editor.org/rfc/rfc7636) for external login using the OpenID Connect authentication source.

routers/web/auth/oauth.go

@@ -44,6 +44,9 @@ import (
 	"github.com/golang-jwt/jwt/v5"
 	"github.com/markbates/goth"
 	"github.com/markbates/goth/gothic"
+	"github.com/markbates/goth/providers/fitbit"
+	"github.com/markbates/goth/providers/openidConnect"
+	"github.com/markbates/goth/providers/zoom"
 	go_oauth2 "golang.org/x/oauth2"
 )
 
@@ -888,7 +891,7 @@ func SignInOAuth(ctx *context.Context) {
 		return
 	}
 
-	codeChallenge, err := generateCodeChallenge(ctx)
+	codeChallenge, err := generateCodeChallenge(ctx, provider)
 	if err != nil {
 		ctx.ServerError("SignIn", fmt.Errorf("could not generate code_challenge: %w", err))
 		return
@@ -1238,7 +1241,21 @@ func handleOAuth2SignIn(ctx *context.Context, source *auth.Source, u *user_model
 }
 
 // generateCodeChallenge stores a code verifier in the session and returns a S256 code challenge for PKCE
-func generateCodeChallenge(ctx *context.Context) (codeChallenge string, err error) {
+func generateCodeChallenge(ctx *context.Context, provider string) (codeChallenge string, err error) {
+	// the `code_verifier` is only forwarded by specific providers
+	// https://codeberg.org/forgejo/forgejo/issues/4033
+	p, ok := goth.GetProviders()[provider]
+	if !ok {
+		return "", nil
+	}
+	switch p.(type) {
+	default:
+		return "", nil
+	case *openidConnect.Provider, *fitbit.Provider, *zoom.Provider:
+		// those providers forward the `code_verifier`
+		// a code_challenge can be generated
+	}
+
 	codeVerifier, err := util.CryptoRandomString(43) // 256/log2(62) = 256 bits of entropy (each char having log2(62) of randomness)
 	if err != nil {
 		return "", err

tests/integration/integration_test.go

@@ -54,8 +54,8 @@ import (
 	gouuid "github.com/google/uuid"
 	"github.com/markbates/goth"
 	"github.com/markbates/goth/gothic"
-	goth_gitlab "github.com/markbates/goth/providers/github"
-	goth_github "github.com/markbates/goth/providers/gitlab"
+	goth_github "github.com/markbates/goth/providers/github"
+	goth_gitlab "github.com/markbates/goth/providers/gitlab"
 	"github.com/santhosh-tekuri/jsonschema/v5"
 	"github.com/stretchr/testify/assert"
 )
@@ -325,6 +325,13 @@ func authSourcePayloadOAuth2(name string) map[string]string {
 	}
 }
 
+func authSourcePayloadOpenIDConnect(name, appURL string) map[string]string {
+	payload := authSourcePayloadOAuth2(name)
+	payload["oauth2_provider"] = "openidConnect"
+	payload["open_id_connect_auto_discovery_url"] = appURL + ".well-known/openid-configuration"
+	return payload
+}
+
 func authSourcePayloadGitLab(name string) map[string]string {
 	payload := authSourcePayloadOAuth2(name)
 	payload["oauth2_provider"] = "gitlab"

tests/integration/oauth_test.go

@@ -532,7 +532,8 @@ func TestSignInOAuthCallbackSignIn(t *testing.T) {
 	assert.Greater(t, userAfterLogin.LastLoginUnix, userGitLab.LastLoginUnix)
 }
 
-func TestSignInOAuthCallbackPKCE(t *testing.T) {
+func TestSignInOAuthCallbackWithoutPKCEWhenUnsupported(t *testing.T) {
+	// https://codeberg.org/forgejo/forgejo/issues/4033
 	defer tests.PrepareTestEnv(t)()
 
 	// Setup authentication source
@@ -557,20 +558,12 @@ func TestSignInOAuthCallbackPKCE(t *testing.T) {
 	resp := session.MakeRequest(t, req, http.StatusTemporaryRedirect)
 	dest, err := url.Parse(resp.Header().Get("Location"))
 	assert.NoError(t, err)
-	assert.Equal(t, "S256", dest.Query().Get("code_challenge_method"))
-	codeChallenge := dest.Query().Get("code_challenge")
-	assert.NotEmpty(t, codeChallenge)
+	assert.Empty(t, dest.Query().Get("code_challenge_method"))
+	assert.Empty(t, dest.Query().Get("code_challenge"))
 
 	// callback (to check the initial code_challenge)
 	defer mockCompleteUserAuth(func(res http.ResponseWriter, req *http.Request) (goth.User, error) {
-		codeVerifier := req.URL.Query().Get("code_verifier")
-		assert.NotEmpty(t, codeVerifier)
-		assert.Greater(t, len(codeVerifier), 40, codeVerifier)
-
-		sha2 := sha256.New()
-		io.WriteString(sha2, codeVerifier)
-		assert.Equal(t, codeChallenge, base64.RawURLEncoding.EncodeToString(sha2.Sum(nil)))
-
+		assert.Empty(t, req.URL.Query().Get("code_verifier"))
 		return goth.User{
 			Provider: gitlabName,
 			UserID:   userGitLabUserID,
@@ -583,6 +576,57 @@ func TestSignInOAuthCallbackPKCE(t *testing.T) {
 	unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: userGitLab.ID})
 }
 
+func TestSignInOAuthCallbackPKCE(t *testing.T) {
+	onGiteaRun(t, func(t *testing.T, u *url.URL) {
+		// Setup authentication source
+		sourceName := "oidc"
+		authSource := addAuthSource(t, authSourcePayloadOpenIDConnect(sourceName, u.String()))
+		// Create a user as if it had been previously been created by the authentication source.
+		userID := "5678"
+		user := &user_model.User{
+			Name:        "oidc.user",
+			Email:       "oidc.user@example.com",
+			Passwd:      "oidc.userpassword",
+			Type:        user_model.UserTypeIndividual,
+			LoginType:   auth_model.OAuth2,
+			LoginSource: authSource.ID,
+			LoginName:   userID,
+		}
+		defer createUser(context.Background(), t, user)()
+
+		// initial redirection (to generate the code_challenge)
+		session := emptyTestSession(t)
+		req := NewRequest(t, "GET", fmt.Sprintf("/user/oauth2/%s", sourceName))
+		resp := session.MakeRequest(t, req, http.StatusTemporaryRedirect)
+		dest, err := url.Parse(resp.Header().Get("Location"))
+		assert.NoError(t, err)
+		assert.Equal(t, "S256", dest.Query().Get("code_challenge_method"))
+		codeChallenge := dest.Query().Get("code_challenge")
+		assert.NotEmpty(t, codeChallenge)
+
+		// callback (to check the initial code_challenge)
+		defer mockCompleteUserAuth(func(res http.ResponseWriter, req *http.Request) (goth.User, error) {
+			codeVerifier := req.URL.Query().Get("code_verifier")
+			assert.NotEmpty(t, codeVerifier)
+			assert.Greater(t, len(codeVerifier), 40, codeVerifier)
+
+			sha2 := sha256.New()
+			io.WriteString(sha2, codeVerifier)
+			assert.Equal(t, codeChallenge, base64.RawURLEncoding.EncodeToString(sha2.Sum(nil)))
+
+			return goth.User{
+				Provider: sourceName,
+				UserID:   userID,
+				Email:    user.Email,
+			}, nil
+		})()
+		req = NewRequest(t, "GET", fmt.Sprintf("/user/oauth2/%s/callback?code=XYZ&state=XYZ", sourceName))
+		resp = session.MakeRequest(t, req, http.StatusSeeOther)
+		assert.Equal(t, "/", test.RedirectURL(resp))
+		unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: user.ID})
+	})
+}
+
 func TestSignInOAuthCallbackRedirectToEscaping(t *testing.T) {
 	defer tests.PrepareTestEnv(t)()