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

client config: make state parameter optional #284

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

balland
Copy link

@balland balland commented Sep 14, 2018

The state parameter is recommended but not mandatory so make this
configurable in case the server does not support it. By default this is
set to true in order to prevent cross-site request forgery.

The state parameter is recommended but not mandatory so make this
configurable in case the server does not support it. By default this is
set to true in order to prevent cross-site request forgery.
balland added a commit to balland/OAuth2 that referenced this pull request Sep 14, 2018
The state parameter is recommended but not mandatory so make this
configurable in case the server does not support it. By default this is
set to true in order to prevent cross-site request forgery.

There is a pending PR in the main repo p2#284
@p2
Copy link
Owner

p2 commented Sep 14, 2018

Do you know which servers don't support state? Not using state opens a gaping security hole and I would prefer if it has to be used.

@balland
Copy link
Author

balland commented Sep 17, 2018

In my case, this only happens when using QR codes and directly start the authentification process to the step of requesting the redirect URL. So in this case, there is no risk of cross-site request forgery. In the change I did, the state parameter is still checked if there is any.

@p2
Copy link
Owner

p2 commented Sep 17, 2018

The risk is in the library being able to be used without state parameter, which I'm not a fan of. How exactly are you starting the flow? Maybe it makes sense to create a subclass for this use case.

@balland
Copy link
Author

balland commented Oct 2, 2018

Our scenario is when users log in with QR codes. The QR code encodes directly the redirect url so there is not state to check here. What would be the proper way to add such feature to the library?

@p2
Copy link
Owner

p2 commented Oct 4, 2018

Subclass, take a look at e.g. OAuth2CodeGrantNoTokenType.

balland added a commit to balland/OAuth2 that referenced this pull request Sep 22, 2020
The state parameter is recommended but not mandatory so make this
configurable in case the server does not support it. By default this is
set to true in order to prevent cross-site request forgery.

There is a pending PR in the main repo p2#284
ajandrade pushed a commit to sensational/OAuth2 that referenced this pull request Dec 28, 2022
The state parameter is recommended but not mandatory so make this
configurable in case the server does not support it. By default this is
set to true in order to prevent cross-site request forgery.

There is a pending PR in the main repo p2#284
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.

2 participants