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

[5.x] Make PKCE opt-in #523

Merged
merged 1 commit into from
Feb 22, 2021
Merged

[5.x] Make PKCE opt-in #523

merged 1 commit into from
Feb 22, 2021

Conversation

driesvints
Copy link
Member

@driesvints driesvints commented Feb 18, 2021

This makes #518 opt-in since it seems to be breaking apps using the Google provider in API's.

Fixes #522

@aaronpk
Copy link
Contributor

aaronpk commented Feb 18, 2021

See my comments in #522 before merging this. Stateless apps can't use the OAuth authorization flow at all it requires maintaining application state in order to check the OAuth state parameter as well as the new PKCE parameters.

@driesvints driesvints marked this pull request as draft February 18, 2021 14:10
@driesvints
Copy link
Member Author

Converting to draft until we've gotten to the bottom of this.

@driesvints driesvints marked this pull request as ready for review February 22, 2021 09:30
@driesvints
Copy link
Member Author

@aaronpk I'm going to go ahead with this PR by disabling PKCE by default and making it opt-in since this seems to break apps using the Google Provider in an API: #524 #522

Until we've fully figured out what's causing this we should keep it opt-in.

@driesvints driesvints changed the title [5.x] Fix stateless and PCKE [5.x] Make PKCE opt-in Feb 22, 2021
@taylorotwell taylorotwell merged commit bdb6487 into 5.x Feb 22, 2021
@taylorotwell taylorotwell deleted the disable-pcke-by-default branch February 22, 2021 14:19
@aaronpk
Copy link
Contributor

aaronpk commented Feb 22, 2021

Okay I've reviewed everything again, and I'm guessing that people who are using this from a stateless API are just leaving out the OAuth state check entirely, since the hasInvalidState checks for that stateless property before checking the OAuth state value. Omitting the state check opens up CSRF attacks on the client, which was the reason for including the state parameter in OAuth in the first place. That said, PKCE prevents CSRF attacks in a cleaner way, by having the server enforce it. (it also prevents some other attacks as well).

The best way to resolve this in a future update would be to provide two ways for people to add PKCE when using it from an API without a session, either:

  • make the codeVerifier that the library generates available to the calling code so that it can provide it to the getAccessTokenResponse method
  • set the codeVerifier value when they make the method call to get the authURL, and also provide the codeVerifier value to the getAccessTokenResponse method

@driesvints
Copy link
Member Author

@aaronpk think it's best that you just attempt a PR with what you want to do so we can look at actual code.

@binotaliu
Copy link
Contributor

enablePKCE() method is declared as protected. How do I enable it without extending my own provider?

@Lukerayn3r
Copy link

Lukerayn3r commented Feb 3, 2023

@aaronpk Did you ever get chance to work on a PR for using PKCE on a stateless API? I have got it working by manually setting the $codeVerifier value, I now just need to get it working with a random $codeVerifier instead.

Is this possible and if so do you have any suggestions?

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.

v5.2.0 generate fatal error when used in REST endpoints
5 participants