Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add pkce_code_challenge_methods option #1735

Conversation

ThisIsMissEm
Copy link
Contributor

@ThisIsMissEm ThisIsMissEm commented Oct 2, 2024

Summary

This fixes #1717, allowing a user to prevent using the plain method for PKCE code challenges, since using plain is deemed insecure (as noted in the PKCE RFC)

This is based off of #1732, I also have some local changes that pull in irb and debug gems for interactive debugging, which I found helpful when testing in the console via:

$ docker run -it --rm doorkeeper:test /srv/bin/console

Other Information

In validate_pkce_code_challenge_methods I did want to log an error if the database hadn't been configured for PKCE, however, due to how this code is instantiated, I wasn't able to get that working due to the model class not yet being loaded by bin/console (so I'm assuming issues may happen in actual usage too).

The error message does list the supported methods always as plain or S256 — to fix that, we'd need to parameterize the localisation message to receive the supported code challenge methods. I wasn't sure if I should make such a change here.

This also limits us to only ever supporting plain or S256, but to my knowledge no other PKCE code challenge methods exist, and doorkeeper wouldn't have support for them anyway due to how it hardcodes which ones it supports.

Copy link
Member

@nbulaj nbulaj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it 🙇 Thanks a lot!

@nbulaj nbulaj merged commit dd77155 into doorkeeper-gem:main Oct 9, 2024
23 checks passed
@ThisIsMissEm ThisIsMissEm deleted the feat/add-pkce_code_challenge_methods-option branch October 9, 2024 11:05
@ransombriggs
Copy link
Contributor

I pushed up #1747 to try and fix the error message when plain is passed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Option to specify supported PKCE code_challenge_methods supported
3 participants