fix(oauthserver): make PKCE optional for confidential clients on /oauth/authorize#2588
Open
tsushanth wants to merge 1 commit into
Open
Conversation
…th/authorize `validatePKCEParams` unconditionally required `code_challenge` + `code_challenge_method` on the authorization endpoint, so confidential clients whose OAuth consumers don't send PKCE were rejected with `invalid_request — PKCE flow requires both code_challenge and code_challenge_method`. Reporter @gormster hit this with Microsoft Power Platform Connectors; the original discussion (#44326) was about Shopify. Both connector-style consumers authenticate to the token endpoint with the registered client_secret and never include PKCE parameters on the authorize redirect — by design. OAuth 2.1 §4.1.1 only mandates PKCE for public clients. For confidential clients the client_secret presented at /oauth/token already authenticates the code exchange, so PKCE is OPTIONAL. The fix threads the resolved `client` through `validateRemainingAuthorizeParams` so `validatePKCEParams` can branch on `client.ClientType`: - Public client, no PKCE → reject (unchanged, OAuth 2.1 mandate). - Confidential client, no PKCE → accept (the bug being fixed). - Either client type, *partial* PKCE (only one of the two fields) → reject. Half-supplied PKCE is always wrong. - Either client type, full PKCE → validate format as before (S256/plain, 43–128 char challenge). `/oauth/token`'s `OAuthServerAuthorization.VerifyPKCE` already handles the "no challenge stored" case (returns `nil`), so the token exchange side needs no changes — a confidential client that opted out at /authorize will simply skip the code_verifier check at /token. Adds three regression tests on the OAuth server authorize suite: - `TestAuthorize_ConfidentialClientWithoutPKCEAccepted` — the reporter's exact failure shape now redirects to `?authorization_id=…` instead of `?error=invalid_request&error_description=PKCE…`. - `TestAuthorize_PublicClientWithoutPKCERejected` — verifies the OAuth 2.1 mandate is still enforced for public clients. - `TestAuthorize_ConfidentialClientWithPartialPKCERejected` — verifies the half-supplied PKCE guard. Fixes supabase#2585
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2585.
What
validatePKCEParamsininternal/api/oauthserver/authorize.gounconditionally requiredcode_challenge+code_challenge_method, so confidential clients whose consumers don't send PKCE were rejected at/oauth/authorizewith:@gormster hit this with Microsoft Power Platform Connectors; the original discussion (supabase/supabase#44326) was about Shopify. Both connector-style consumers authenticate to
/oauth/tokenwith the registeredclient_secretand don't include PKCE on the authorize redirect — by design.Why this is spec-compliant
OAuth 2.1 §4.1.1 mandates PKCE for public clients only. For confidential clients the
client_secretpresented at the token exchange already authenticates the code → token swap, so PKCE is OPTIONAL. The current code is stricter than the spec and incompatible with major IdP-style consumers.How
Threaded the resolved
*models.OAuthServerClientthroughvalidateRemainingAuthorizeParamssovalidatePKCEParamscan branch onclient.ClientType:/oauth/token'sOAuthServerAuthorization.VerifyPKCEalready handles the no challenge stored case (returnsnilearly atinternal/models/oauth_authorization.go:216-218), so the token endpoint needs no changes — a confidential client that opted out at/authorizesimply skips thecode_verifiercheck at/token.Tests
Three regression tests on the OAuth server authorize suite (the suite already covered the success-with-PKCE path):
TestAuthorize_ConfidentialClientWithoutPKCEAccepted— the reporter's exact failure shape now redirects to?authorization_id=…instead of the PKCE error.TestAuthorize_PublicClientWithoutPKCERejected— verifies the OAuth 2.1 mandate is still enforced for public clients.TestAuthorize_ConfidentialClientWithPartialPKCERejected— verifies the half-supplied PKCE guard.go build ./internal/api/oauthserver/clean.go test -count=1 -run TestNothing ./internal/api/oauthserver/confirms the suite compiles. I did not stand up the local Postgres dev container to run the full integration suite — flagging that explicitly. Happy to also add a token-endpoint test asserting that a code minted without PKCE swaps successfully withclient_secretand nocode_verifierif you want broader coverage on the same PR.Notes
code_challengepresent is identical to before.internal/api/pkce.go::validatePKCEParamsused by signup/sso/resend/otp/magic_link — different code path with different semantics (single-page login flows where PKCE is genuinely required).