Skip to content

Allow overriding PKCE verifier validation without forking the handler #885

Description

@shilps1583

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.

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions