Describe your problem
pkce.Handler.HandleTokenEndpointRequest inlines RFC 7636's code_verifier
length (43-128 chars), character-set, and S256/plain comparison rules
directly in one function, with no seam to change them.
Anyone migrating clients from (or needing to interoperate with) an
authorization server that predates or relaxes those rules -- for example,
one with no minimum verifier length -- currently has to fork the entire
handler just to change that one policy decision. That means also
re-implementing the PKCE session lifecycle and the
EnforcePKCE/EnforcePKCEForPublicClients handling that has nothing to do
with verifier policy, and silently drifting from upstream fixes to that
logic over time (we only noticed after review that our own fork had
dropped EnforcePKCEForPublicClients handling entirely by accident).
Describe your ideal solution
Extract the length/character-set check and the challenge comparison into
a small interface, CodeVerifierStrategy, with two methods --
ValidateVerifierFormat and ValidateChallenge -- and add it as an
optional field on Handler (Verifier), defaulting to
DefaultCodeVerifierStrategy, which preserves today's RFC 7636 behavior
exactly when left unset.
I have a behavior-preserving implementation with tests ready on my fork
(branch feat/pluggable-pkce-verifier-strategy) -- every existing
handler/pkce test passes unchanged, since the default behavior is
identical; the interface only adds an override seam. Happy to open the PR
right away if this direction looks reasonable, or adjust the shape (for
example, decomposing HandleTokenEndpointRequest into smaller exported
steps instead of adding a new interface) based on feedback here first.
Workarounds or alternatives
- Forking
pkce.Handler entirely, which is what we do today. Works, but
duplicates the whole session lifecycle and PKCE-enforcement logic to
change one policy decision, and risks silently losing fixes like the
EnforcePKCEForPublicClients handling mentioned above.
- A config flag to disable the length/charset check globally. Considered
and rejected: it would weaken PKCE's security guarantee by default for
every fosite consumer who sets it, not just ones needing legacy-provider
compatibility. A pluggable strategy with a secure default avoids that,
since nothing about the default behavior changes.
Additional Context
Filed alongside an independent PKCE session-lifecycle fix, #883, found
while working on this.
Describe your problem
pkce.Handler.HandleTokenEndpointRequestinlines RFC 7636's code_verifierlength (43-128 chars), character-set, and S256/plain comparison rules
directly in one function, with no seam to change them.
Anyone migrating clients from (or needing to interoperate with) an
authorization server that predates or relaxes those rules -- for example,
one with no minimum verifier length -- currently has to fork the entire
handler just to change that one policy decision. That means also
re-implementing the PKCE session lifecycle and the
EnforcePKCE/EnforcePKCEForPublicClients handling that has nothing to do
with verifier policy, and silently drifting from upstream fixes to that
logic over time (we only noticed after review that our own fork had
dropped EnforcePKCEForPublicClients handling entirely by accident).
Describe your ideal solution
Extract the length/character-set check and the challenge comparison into
a small interface,
CodeVerifierStrategy, with two methods --ValidateVerifierFormatandValidateChallenge-- and add it as anoptional field on
Handler(Verifier), defaulting toDefaultCodeVerifierStrategy, which preserves today's RFC 7636 behaviorexactly when left unset.
I have a behavior-preserving implementation with tests ready on my fork
(branch
feat/pluggable-pkce-verifier-strategy) -- every existinghandler/pkcetest passes unchanged, since the default behavior isidentical; the interface only adds an override seam. Happy to open the PR
right away if this direction looks reasonable, or adjust the shape (for
example, decomposing
HandleTokenEndpointRequestinto smaller exportedsteps instead of adding a new interface) based on feedback here first.
Workarounds or alternatives
pkce.Handlerentirely, which is what we do today. Works, butduplicates the whole session lifecycle and PKCE-enforcement logic to
change one policy decision, and risks silently losing fixes like the
EnforcePKCEForPublicClients handling mentioned above.
and rejected: it would weaken PKCE's security guarantee by default for
every fosite consumer who sets it, not just ones needing legacy-provider
compatibility. A pluggable strategy with a secure default avoids that,
since nothing about the default behavior changes.
Additional Context
Filed alongside an independent PKCE session-lifecycle fix, #883, found
while working on this.