Skip to content
This repository has been archived by the owner on Jan 13, 2022. It is now read-only.

FacebookRedirectLoginHelper - let's not regenerate CSRF token #613

Merged
merged 1 commit into from Aug 6, 2016

Conversation

KuceraMartin
Copy link
Contributor

When FacebookRedirectLoginHelper::getLoginUrl() is called it should first check if a CSRF token (called state in the source code) has already been generated and use the existing one if it has.
Right now any time the method is called, a new CSRF token is generated. That means if the method is called more than once, only the last URL will be working which is a problem.

What if the user opens up a webpage in multiple tabs at once? Then he will only be able to use Facebook login in the last tab. And if you forget to put favicon.ico on your website, then he will only be able to use Facebook login in IE :) (see #583 for details).

@ghost
Copy link

ghost commented Jun 14, 2016

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

@ghost
Copy link

ghost commented Jun 14, 2016

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@ghost ghost added the CLA Signed label Jun 14, 2016
@SammyK SammyK added this to the 5.2.1 milestone Aug 6, 2016
@ghost ghost added the CLA Signed label Aug 6, 2016
@SammyK
Copy link
Contributor

SammyK commented Aug 6, 2016

This is great stuff! Thanks! This should knock out most of the issues we've been having for a very long time with the CSRF (#583). I'm going to finish this one off by resetting the state after someone successfully obtains an access token so that the same CSRF token isn't being used every time.

@SammyK SammyK merged commit b44e2f4 into facebookarchive:master Aug 6, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants