Ensure force PKCE always forces PKCE, fixes #1654

[ThisIsMissEm]

Summary

Previously force_pkce option could be bypassed if the client was confidential, that's no longer the case with the Security Practices BCP, which recommends PKCE for all clients.

Fixes #1654

This is technically a breaking change

Other Information

We could perhaps simplify the tests a little, but I'm not sure that's necessary right now.

[nbulaj]

LGTM, thanks

[gkemmey]

how does this affect implementations currently forcing public apps to use pkce, but not confidential ones? unless i'm misunderstanding, this is a breaking change i'd have to pass on to third-party integrators, or disable the force_pkce option entirely.

[ThisIsMissEm]

how does this affect implementations currently forcing public apps to use pkce, but not confidential ones? unless i'm misunderstanding, this is a breaking change i'd have to pass on to third-party integrators, or disable the force_pkce option entirely.

@gkemmey This would be a breaking change, in which case, I'd recommend disabling force_pkce and then introducing it again to your ecosystem. I'd be hesitant to add a block invocation for force_pkce when it should really be an "on" or "off" option. Keep in mind, that the OAuth 2.1 and OAuth Security Current Best Practices strongly recommend PKCE for all authorization code requests due to the volume of security issues that PKCE fixes.

Keep in mind this would not affect client credentials grant flow, only authorization code grant flow.

[molawson]

I'd be hesitant to add a block invocation for force_pkce when it should really be an "on" or "off" option.

@ThisIsMissEm Can you say a little more about this perspective? While I love the idea of moving more providers and clients in the direction of the most secure methods of operation available, having some in-between functionality (i.e. a "partially on" state as the code works prior to this change) seems hugely beneficial for enabling apps to move in this more secure direction incrementally. I understand that's not ideal and makes for more code to maintain (I love the simplicity of this change!), but there seems like a reasonable amount of upside to that incremental/layered approach.

[ThisIsMissEm]

If anything, we'd add a require_pkce_for block, but force_pkce should always be all clients or none, you can't partially "force" something.

So like:

require_pkce_for { |client, server|  ... }

Or something (you wouldn't have the grant type in here, because it's only authorization_code where this is relevant).

But you could probably also do this with a custom extension, I think, in like the PreAuth code?

[nbulaj]

Hey all. Thanks for the great insights! This indeed can affect existing projects & customers so I think if we need to add some warning message? Or at least a changelog entry with a breaking mark? And release it as a part of 6.x version. WDYT? We had some similar changes before and indeed reports were created.

[ThisIsMissEm]

@nbulaj yeah, we could ship this and my other changes as a 6.x and just indicate major breaking changes

[gkemmey]

if we ship this as is, there will be no doorkeeper-supported way to recover the existing behavior where pkce is only required for public clients. i'd like to continue forcing public clients to use pkce, but avoid forcing third-party integrators to update their confidential oauth apps to use pkce.

are we deciding that behavior will have to be patched in?

if not and we’re willing to make a breaking change to force_pkce, i'd rather we allow a block so it can be configured. as an example, here's a branch that replaces force_pkce with a require_pkce_for hook.

[ThisIsMissEm]

@gkemmey that looks like overall a good change, but I think we should continue having force_pkce, but also introduce require_pkce_for and just make the default implementation use the value of force_pkce — does that make sense?

[ThisIsMissEm]

@gkemmey if you want, open up that branch as a pull request against ThisIsMissEm:fix/allow-force-pkce-for-all-clients and I'll rebase it in, keeping authorship.

[gkemmey]

that makes perfect sense! opened the pr here. don't feel like you have to use it if it's easier to just make the changes yourself. thanks! 🙏

[paulRbr]

Thanks both of you @ThisIsMissEm and @gkemmey how can we help to make this change go forward? I've done a quick review and didn't see anything off (but I'm new to the gem so I'm trusting @nbulaj 's approval of April 2025 😊 )

[ThisIsMissEm]

Let me try to get to it today. The tldr on my end is that this work was primarily for the mastodon project, and my funding wasn't sustainable so I've had to focus on other things to earn money.

[ThisIsMissEm]

I started merging in that change, but the test coverage here doesn't make sense to me right now. see: https://github.com/ThisIsMissEm/doorkeeper/pull/1/changes#r2750112774