From cf4554b82c18b0bc1107a24e6b910cbae96fecab Mon Sep 17 00:00:00 2001 From: Evan Date: Thu, 11 Jun 2026 19:52:11 +0200 Subject: [PATCH] fix(idp): apply PKCE when building OAuth and OIDC providers (#12247) # Which Problems Are Solved - IDP intent authorization redirects used by Login v2 ignored the configured `UsePKCE` setting for Generic OAuth providers - This caused providers such as X/Twitter OAuth2 to receive authorization requests without `code_challenge` and `code_challenge_method` - OIDC provider construction had the same gap, even though PKCE is already part of the provider configuration model - Existing IDP intent redirect tests expected non-PKCE OAuth URLs and failed once PKCE was applied correctly # How the Problems Are Solved - Updated `OAuthIDPWriteModel.ToProvider` in internal/command/idp_model.go to pass `rp.WithPKCE(nil)` when `UsePKCE` is enabled - Updated `OIDCIDPWriteModel.ToProvider` to apply the same PKCE relying-party option for OIDC providers # Additional Changes - Added focused provider-construction tests covering: - OAuth provider redirects include code_challenge - OIDC provider redirects include code_challenge - both providers use code_challenge_method=S256 - both persist the generated codeVerifier for token exchange - Updated `TestCommands_AuthFromProvider` in internal/command/idp_intent_test.go so OAuth redirect assertions verify PKCE structurally instead of hard-coding the generated challenge value # Additional Context - Reproduced with X/Twitter OAuth2 where the generated authorization URL was missing PKCE parameters despite `usePkce: true` - Verified both the focused PKCE tests and the full unit test suite run successfully with the fix - Closes #12036 - Closes/supersedes #12054: - Tests are included - No slice re-allocation on `opts` append # Result ### Before `authUrl` in `StartIdentityProviderIntent` response is missing `code_challenge` and `code_challenge_method` for Generic OAuth IDP with PKCE enabled: ```json { "details": { "sequence": "1", "changeDate": "2026-06-07T18:48:16.921317Z", "resourceOwner": "376298239768395779" }, "authUrl": "https://x.com/i/oauth2/authorize?client_id=&prompt=select_account&redirect_uri=http%3A%2F%2Flocalhost%3A8080%2Fidps%2Fcallback&response_type=code&scope=tweet.read+users.email+users.read+offline.access&state=376417136207200259" } ``` ### After `code_challenge` and `code_challenge_method` are correctly included into `authUrl`: ```json { "details": { "sequence": "1", "changeDate": "2026-06-07T18:49:39.488941Z", "resourceOwner": "376298239768395779" }, "authUrl": "https://x.com/i/oauth2/authorize?client_id=&code_challenge=8G4vN8QNgSsbvGSHKwYPEc2qUYU2BK5L0fsr992duTA&code_challenge_method=S256&prompt=select_account&redirect_uri=http%3A%2F%2Flocalhost%3A8080%2Fidps%2Fcallback&response_type=code&scope=tweet.read+users.email+users.read+offline.access&state=376417274736672771" } ``` --- internal/command/idp_intent_test.go | 44 +++++++++++-- internal/command/idp_model.go | 10 ++- internal/command/idp_model_test.go | 99 +++++++++++++++++++++++++++++ 3 files changed, 146 insertions(+), 7 deletions(-) diff --git a/internal/command/idp_intent_test.go b/internal/command/idp_intent_test.go index 9503548ed..3e01048d3 100644 --- a/internal/command/idp_intent_test.go +++ b/internal/command/idp_intent_test.go @@ -439,8 +439,9 @@ func TestCommands_AuthFromProvider(t *testing.T) { samlRootURL string } type res struct { - auth idp.Auth - err error + auth idp.Auth + expectPKCE bool + err error } tests := []struct { name string @@ -585,7 +586,8 @@ func TestCommands_AuthFromProvider(t *testing.T) { callbackURL: "url", }, res{ - auth: &idp.RedirectAuth{RedirectURL: "auth?client_id=clientID&prompt=select_account&redirect_uri=url&response_type=code&state=id"}, + auth: &idp.RedirectAuth{RedirectURL: "auth?client_id=clientID&prompt=select_account&redirect_uri=url&response_type=code&state=id"}, + expectPKCE: true, }, }, { @@ -647,7 +649,8 @@ func TestCommands_AuthFromProvider(t *testing.T) { loginHint: "user@example.com", }, res{ - auth: &idp.RedirectAuth{RedirectURL: "auth?client_id=clientID&login_hint=user%40example.com&redirect_uri=url&response_type=code&state=id"}, + auth: &idp.RedirectAuth{RedirectURL: "auth?client_id=clientID&login_hint=user%40example.com&redirect_uri=url&response_type=code&state=id"}, + expectPKCE: true, }, }, { @@ -755,13 +758,44 @@ func TestCommands_AuthFromProvider(t *testing.T) { var got idp.Auth if err == nil { got, err = session.GetAuth(tt.args.ctx) - assert.Equal(t, tt.res.auth, got) assert.NoError(t, err) + if tt.res.expectPKCE { + assertRedirectAuthWithPKCE(t, tt.res.auth, got) + assert.NotEmpty(t, session.PersistentParameters()[oauth.CodeVerifier]) + } else { + assert.Equal(t, tt.res.auth, got) + } } }) } } +func assertRedirectAuthWithPKCE(t *testing.T, expected, actual idp.Auth) { + t.Helper() + + expectedRedirect, ok := expected.(*idp.RedirectAuth) + require.True(t, ok) + actualRedirect, ok := actual.(*idp.RedirectAuth) + require.True(t, ok) + + expectedURL, err := url.Parse(expectedRedirect.RedirectURL) + require.NoError(t, err) + actualURL, err := url.Parse(actualRedirect.RedirectURL) + require.NoError(t, err) + + actualQuery := actualURL.Query() + assert.NotEmpty(t, actualQuery.Get("code_challenge")) + assert.Equal(t, "S256", actualQuery.Get("code_challenge_method")) + + actualQuery.Del("code_challenge") + actualQuery.Del("code_challenge_method") + + assert.Equal(t, expectedURL.Scheme, actualURL.Scheme) + assert.Equal(t, expectedURL.Host, actualURL.Host) + assert.Equal(t, expectedURL.Path, actualURL.Path) + assert.Equal(t, expectedURL.Query(), actualQuery) +} + func TestCommands_AuthFromProvider_SAML(t *testing.T) { type fields struct { eventstore func(t *testing.T) *eventstore.Eventstore diff --git a/internal/command/idp_model.go b/internal/command/idp_model.go index fdfd9e21c..c8f73defe 100644 --- a/internal/command/idp_model.go +++ b/internal/command/idp_model.go @@ -179,7 +179,10 @@ func (wm *OAuthIDPWriteModel) ToProvider(callbackURL string, idpAlg crypto.Encry RedirectURL: callbackURL, Scopes: wm.Scopes, } - opts := make([]oauth.ProviderOpts, 0, 4) + opts := make([]oauth.ProviderOpts, 0, 5) + if wm.UsePKCE { + opts = append(opts, oauth.WithRelyingPartyOption(rp.WithPKCE(nil))) + } if wm.IsCreationAllowed { opts = append(opts, oauth.WithCreationAllowed()) } @@ -382,11 +385,14 @@ func (wm *OIDCIDPWriteModel) ToProvider(callbackURL string, idpAlg crypto.Encryp if err != nil { return nil, err } - opts := make([]oidc.ProviderOpts, 1, 6) + opts := make([]oidc.ProviderOpts, 1, 7) opts[0] = oidc.WithSelectAccount() if wm.IsIDTokenMapping { opts = append(opts, oidc.WithIDTokenMapping()) } + if wm.UsePKCE { + opts = append(opts, oidc.WithRelyingPartyOption(rp.WithPKCE(nil))) + } if wm.IsCreationAllowed { opts = append(opts, oidc.WithCreationAllowed()) } diff --git a/internal/command/idp_model_test.go b/internal/command/idp_model_test.go index 423bff39b..d4deba371 100644 --- a/internal/command/idp_model_test.go +++ b/internal/command/idp_model_test.go @@ -1,12 +1,22 @@ package command import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "net/url" + "sync/atomic" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + oidc_pkg "github.com/zitadel/oidc/v3/pkg/oidc" + "github.com/zitadel/zitadel/internal/crypto" "github.com/zitadel/zitadel/internal/domain" + providers "github.com/zitadel/zitadel/internal/idp" + "github.com/zitadel/zitadel/internal/idp/providers/oauth" "github.com/zitadel/zitadel/internal/zerrors" ) @@ -353,3 +363,92 @@ func TestCommands_AllIDPWriteModel(t *testing.T) { }) } } + +func TestOAuthIDPWriteModel_ToProvider_WithPKCE(t *testing.T) { + wm := &OAuthIDPWriteModel{ + Name: "OAuth", + ClientID: "clientID", + ClientSecret: &crypto.CryptoValue{Algorithm: "plain", Crypted: []byte("clientSecret"), KeyID: "keyID"}, + AuthorizationEndpoint: "https://idp.example.com/authorize", + TokenEndpoint: "https://idp.example.com/token", + UserEndpoint: "https://idp.example.com/user", + Scopes: []string{"user"}, + IDAttribute: "id", + UsePKCE: true, + } + + provider, err := wm.ToProvider("https://zitadel.example.com/idps/callback", plainTextEncryption{}) + require.NoError(t, err) + + assertProviderUsesPKCE(t, provider) +} + +func TestOIDCIDPWriteModel_ToProvider_WithPKCE(t *testing.T) { + var ( + issuer string + discoveryRequested atomic.Bool + ) + handler := func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != oidc_pkg.DiscoveryEndpoint { + http.NotFound(w, r) + return + } + discoveryRequested.Store(true) + w.Header().Set("Content-Type", "application/json") + // assert, not require: FailNow must not be called from the server's handler goroutine + assert.NoError(t, json.NewEncoder(w).Encode(&oidc_pkg.DiscoveryConfiguration{ + Issuer: issuer, + AuthorizationEndpoint: issuer + "/authorize", + TokenEndpoint: issuer + "/token", + UserinfoEndpoint: issuer + "/userinfo", + })) + } + + server := httptest.NewServer(http.HandlerFunc(handler)) + t.Cleanup(server.Close) + issuer = server.URL + + wm := &OIDCIDPWriteModel{ + Name: "OIDC", + Issuer: issuer, + ClientID: "clientID", + ClientSecret: &crypto.CryptoValue{Algorithm: "plain", Crypted: []byte("clientSecret"), KeyID: "keyID"}, + Scopes: []string{"openid"}, + UsePKCE: true, + } + + provider, err := wm.ToProvider("https://zitadel.example.com/idps/callback", plainTextEncryption{}) + require.NoError(t, err) + require.True(t, discoveryRequested.Load(), "expected OIDC discovery to be requested") + + assertProviderUsesPKCE(t, provider) +} + +func assertProviderUsesPKCE(t *testing.T, provider providers.Provider) { + t.Helper() + + session, err := provider.BeginAuth(context.Background(), "state") + require.NoError(t, err) + auth, err := session.GetAuth(context.Background()) + require.NoError(t, err) + redirect, ok := auth.(*providers.RedirectAuth) + require.True(t, ok) + authURL, err := url.Parse(redirect.RedirectURL) + require.NoError(t, err) + + query := authURL.Query() + assert.NotEmpty(t, query.Get("code_challenge")) + assert.Equal(t, "S256", query.Get("code_challenge_method")) + assert.NotEmpty(t, session.PersistentParameters()[oauth.CodeVerifier]) +} + +type plainTextEncryption struct{} + +func (plainTextEncryption) Algorithm() string { return "plain" } +func (plainTextEncryption) EncryptionKeyID() string { return "keyID" } +func (plainTextEncryption) DecryptionKeyIDs() []string { return []string{"keyID"} } +func (plainTextEncryption) Encrypt(value []byte) ([]byte, error) { return value, nil } +func (plainTextEncryption) Decrypt(hashed []byte, _ string) ([]byte, error) { return hashed, nil } +func (plainTextEncryption) DecryptString(hashed []byte, _ string) (string, error) { + return string(hashed), nil +}