Option to specify supported PKCE code_challenge_methods supported

[ThisIsMissEm]

In OAuth 2.0 Security Best Current Practices, the PKCE code_challenge_method of plain MUST NOT be used, as it leaks the code_verifier in the authorization request:

When using PKCE, clients SHOULD use PKCE code challenge methods that do not expose the PKCE verifier in the authorization request. Otherwise, attackers that can read the authorization request (cf. Attacker A4 in Section 3) can break the security provided by PKCE. Currently, S256 is the only such method.

Doorkeeper's PKCE support currently supports both plain and S256 and there is no easy way to disable 'plain' to be inline with security best current practices.

Potential Resolution

Add a code_challenge_methods_supported option that is an array of methods, defaulting to ['plain', 'S256'] which is then used when validating the code_challenge_method.

Additionally this should be exposed via Doorkeeper.configuration.code_challenge_methods_supported, such that servers implementing OAuth Authorization Server Metadata (also now recommended, issue #1587) can return code_challenge_methods_supported property.

[ThisIsMissEm]

In this Mastodon pull request, I've worked around this by extending Doorkeeper's Doorkeeper::OAuth::PreAuthorization

[ransombriggs]

I am also interested in this feature since I did a similar, but less elegant workaround in my own codebase to disable plain

[ThisIsMissEm]

@nbulaj would you be interested in a pull request to add said code_challenge_methods_supported configuration option?

[nbulaj]

Yeah @ThisIsMissEm , all the above sounds reasonable to me :+1:

[ThisIsMissEm]

(Now I just need to figure out #1721 such that I can contribute this feature change)

[nbulaj]

TBH I don't use docker for local development so not sure which setup we have currently for it :smiley_cat: On my local machine I'm able to run the test suite without issues :thinking:

[kmayer]

I develop in an environment where I need multiple versions of Ruby, Rails, Gems and on and on and on. 17 year-old legacy systems are like that. I've found that the only way to get a predictable development environment is to isolate each one in devcontainer. It's saved me over and over again. It also saved colleagues who were reviewing my code, but weren't otherwise in active development of it. It was sooo much easier for them to "just" launch the container than install all of the things.

[ThisIsMissEm]

I'm saying I can't get the tests passing in either local environment OR devcontainer, that means I cannot work on the doorkeeper-gem codebase, because I cannot get the tests passing to ensure I'm not breaking anything. The only way I'd have to test is via CI when approved by the doorkeeper maintainers.

[ransombriggs]

@ThisIsMissEm I have a PR https://github.com/doorkeeper-gem/doorkeeper/pull/1732 to tweak the dockerfile that worked for me

[ThisIsMissEm]

Have now been able to open a PR for this: #1735