Closed somprabhsharma closed 8 years ago
If you don't pass a state
parameter then Passport's default state management will be used. Usually, you don't need to set your own state
parameter.
However, if you want to pass your own state
parameter nonetheless, please refer to Passport's OAuth2Strategy
since passport-pinterest
only passes the parameter through.
@analog-nico but if I use any other passport strategy like passport-linkedin
or passport-soundcloud
with the same code then I am not getting any error while using state
param. But I am getting error in passport-pinterest
. So issue is not with Passport's OAuth2Strategy .
Sorry, buddy, I can't help you with this because I currently have no test setup. But anyhow it really is not caused by passport-pinterest
itself because there is just no code other than setting state = true
as the default.
The most likely causes are:
state
optionIt is unlikely that the OAuth2Strategy
contains a bug. But I am pointing you to this because the OAuth2Strategy
is the one actually processing your state
parameter value. What you basically need to find out is what happens under the hood with your state
parameter value and how the Pinterest handles it.
@analog-nico I did some changes in passport-pinterest
library in my local system. and It worked. I first analysed all other passport strategy
and find out that no one is using state
param in the strategy.js
file. So I removed it and it worked. should I submit a PR ?
Yes, a PR would be great. Please also tell me which state
parameter value you are using. Thanks!
@analog-nico I submitted a PR. please review.
Judging from PR #4 you want to pass for state
either a falsy value or an empty string, right? What actual value do you want to pass?
@analog-nico As per the oauth2 documentation state
param contains a custom string value to maintain state of the client. and after successfull authentication client gets the same state
value. So I want to pass some custom string param in state
param. e.g. I want to use the below code in /pinterest
route.
passport.authenticate('pinterest', {
state: 'dGVzdGVuY29kZWR2YWx1ZQ'
})(req, res)
@analog-nico Also below are link to popular passport-strategys. No one is using state in their code. https://github.com/mjpearson/passport-wordpress/blob/master/lib/passport-wordpress/strategy.js#L48
https://github.com/jaredhanson/passport-tumblr/blob/master/lib/passport-tumblr/strategy.js#L50
https://github.com/jaredhanson/passport-facebook/blob/master/lib/strategy.js#L54
https://github.com/mstade/passport-google-oauth2/blob/master/lib/oauth2.js#L48
Sorry, I am not trying to reject your issue or anything.
But did you debug your code? At least by looking at the passport-pinterest
code your state
parameter should be forwarded to the OAuth2Strategy
as expected. There is no need to remove the lines in PR #4 .
Btw, the line options.state = options.state || true
makes sure that a state
value is always passed to the Pinterest API even if the user doesn't set the state
option. This is for added security that the other strategies just not employ.
@analog-nico you are doing it wrong. You are setting options.state = options.state||true
while creating Pinterest Strategy. But Pinterest Strategy is a part of middleware and runs only at the time of starting the server. So at that time options.state is undefined so it sets its value to true
and if passport-oauth2
sees state param's value as true then it handles the state by itself. Hence the state param I am sending explicitly is replaced by passport-oauth2
and is of no use. To use it you have to remove those lines from your code. Please test your code nicely. It does not work unless you remove those lines of code. It works only in one scenario when you do not pass state param explicitly.
Ok, I see. That means setting state
to true
as the default has to happen here.
Could you do me a favor and update your PR and test both scenarios - with and without setting the state
option?
As I said I have to test set up and it would be important to not introduce breaking changes.
Thanks!
Yes, I will test all possible scenarios and update the PR.
Awesome, thank you very much!
@analog-nico : I have got clear understanding of how state works in passport. Let me explain it to you. There are two state
params -
app.get('/auth/pinterest',
passport.authenticate('pinterest', {
state: 'asfds3UGVUSAFsad'
})
);
Let's call this state
as stateParam
. It is a String value. this is state param which is used by all platforms to stop CSRF and is mentioned in Oauth2 specification.
passport.use(new PinterestStrategy({
clientID: PINTEREST_APP_ID,
clientSecret: PINTEREST_APP_SECRET,
scope: ['read_public', 'read_relationships'],
callbackURL: "https://localhost:3000/auth/pinterest/callback",
state: true
},
function(accessToken, refreshToken, profile, done) {
done();
}
));
Let's call this state
as strategyState
. It is a boolean value (default value: false
if user does not override it in constructer) used in passport-oauth2 to decide if user wants passport to handle stateParam
or want to handle stateParam
by himself.
Now based on this strategyState
and stateParam
there are Two Main cases.
strategyState
value is true
. It means that user wants passport to handle the stateParam
. In this case the value of stateParam
will be generated by passport and verified by passport. User does not have to do anything. In this case user can not supply stateParam
value explicitly. [The current scenario happening with your code as you are overriding state as true in constructor.]false
or not overridden. It means that user wants to handle stateParam
by himself and does not want passport to interfere. (this is my case). Here can be two more cases - a. If user add custom stateParam
, then it is secure and does not allow CSRF to happen.
b. If user does not add any stateParam
, then it is not secure and CSRF can happen.
Now if your code remains same then it works fine for case 1
and case 2.b
but does not work for case 2.a
as the strategyValue
will always be true and if user add custom stateParam
then it will not match with the stateParam
generated by passport.
And now if you merge the PR submitted by me then it works fine for all 3 cases. But there is security risk in case 2.b
that is totally fault of the user who is implementing. He can simple add strategyState
as true and ignore the case2.b
to make it secure. So my suggestions is to merge the PR.
I hope, I cleared everything to you. Please be wise and make decision.
Thanks for the details @somprabhsharma ! The main reason why our discussion turns out to be so complicated is that I am solely responding based on looking at the code. If I would do some testing it would be resolved easily. However, I am not using passport-pinterest
myself anymore and thus have no test set up.
I was still thinking there was a way to keep the default behavior of state = true
even with PR #4 merged. But after checking the passport-oauth2
code I realized there isn't.
Ok, I'll merge PR #4 and make a semver major version bump because it - as you point out - introduces a breaking change for case 2.b
.
Thanks for working on this! Btw, would you like to take over the project and continue its maintenance?
I just published version 1.0.0. Thanks again!
@analog-nico Yes, I would love to be a part of this project and ready for its maintenance. Thank you for asking.
@somprabhsharma I am glad you wanna join in! Since you are actually using this lib this is a big help! You should just have received an invite for push rights. Is @somprabhsharma you handle on npm as well?
@analog-nico yes @somprabhsharma is my handle on npm as well.
@somprabhsharma Thank you so much. You explanation helped many who are struggling with oauth state
Whenever I pass custom state param, i get following error
Unable to verify authorization request state.
, If I remove state param then everything works fine.