laravel / socialite

Laravel wrapper around OAuth 1 & OAuth 2 libraries.
https://laravel.com/docs/socialite
MIT License
5.58k stars 940 forks source link

v5.2.0 generate fatal error when used in REST endpoints #522

Closed aprokopenko closed 3 years ago

aprokopenko commented 3 years ago

Description:

We have a stateless application, which provides the only API enpoints. This means we don't have any sessions inside. After update to 5.2.0 and using Google auth we now get the fatal error on the endpoint, which returns auth URLs for social networks.

Laravel fatal response: image

Sentry debug backtrace: image

Steps To Reproduce:

driesvints commented 3 years ago

This package is only meant for web based sessions and not API authentication.

aprokopenko commented 3 years ago

Previous version 5.1.3 worked great.

aprokopenko commented 3 years ago

I think the correct behaviour will be to add config for enabling PCKE if it is supported by any provider.

In this case anyone can use this package

driesvints commented 3 years ago

@aprokopenko PCKE support was added in the latest release: https://github.com/laravel/socialite/pull/518

driesvints commented 3 years ago

I now see more clearly what you mean. @aaronpk do you know why your PR has broken things for use cases like @aprokopenko's one?

aprokopenko commented 3 years ago

Yes, and this new addon for PCKE generates a fatal error because you use a session to store code_verifier and code_challenge. In the API you don't need this, because codes client secret/key are in safe (you can't get codes like in mobile APP or SPA). So PCKE is not needed in this case.

So more friendly is to have config for using PCKE, it can be ON by default, but should be the ability to turn it off.

I can't even override anything, because you create providers as new $provider and I can't bind my own class and set pcke property to false to make it works again.

So, unfortunately, I need to downgrade right now and stop updating this package to the latest versions.

driesvints commented 3 years ago

I agree, it should have been opt-in. Not on by default. That should have been done in a new major version.

aprokopenko commented 3 years ago

Cool, thanks.

driesvints commented 3 years ago

Actually, I found a better way to check this. We'll just use the usesState check since that already indicates if the app uses state or not. Furthermore my comment about Socialite not being usable in an API was incorrect, sorry.

driesvints commented 3 years ago

Sent in a PR here: https://github.com/laravel/socialite/pull/523. Thanks for reporting this 👍

aaronpk commented 3 years ago

We'll just use the usesState check since that already indicates if the app uses state or not.

This may have been my misunderstanding of the usesState function. I thought that was referring to the OAuth parameter state, not the concept of state. The docs aren't super clear on this, especially because the usesState and isStateless functions are immediately followed by getState which does refer to the OAuth state parameter.

In any case, I am confused how the OP is handling OAuth in this case at all, as doing the authorization code flow with Google does require maintaining application state to check the state parameter in the redirect. If the answer is "we're not checking the state parameter" then you'll definitely need to solve that as the application would be open to some tricky attacks without that. If the stateless application is proxying these calls to a frontend application that does maintain state, we'll need to add methods to retrieve the state and code_verifier values from the OAuth class in order for the frontend to use them in the final access token request.

driesvints commented 3 years ago

@aprokopenko can you please shed some light on @aaronpk's remarks?

driesvints commented 3 years ago

We've released v5.2.1 to make PKCE opt-in instead. We'd appreciate any other insights into why this broke things for @aprokopenko and @fabien44300.

aprokopenko commented 3 years ago

Hi,

Thanks for the quick fix.

I checked the PR, so to correctly use PCKE I need to do something like this:

Socialite::driver($driverName)->enablePKCE();

(right now we use Socialite::driver($driverName)->stateless();)

I think you should document this somewhere :)

aprokopenko commented 3 years ago

Anyway, the new release is working fine now. Thanks.

driesvints commented 3 years ago

@aprokopenko can you please answer @aaronpk's questions? https://github.com/laravel/socialite/issues/522#issuecomment-781361745