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

Add isSameSite to schema #142

Merged
merged 3 commits into from Feb 17, 2017
Merged

Add isSameSite to schema #142

merged 3 commits into from Feb 17, 2017

Conversation

@arnivuo
Copy link
Contributor

arnivuo commented Sep 11, 2016

Fixes #141.

arnivuo added 3 commits Sep 10, 2016
Statehood in older version wasn't supporting isSameSite.
@goddyZhao

This comment has been minimized.

Copy link

goddyZhao commented Oct 27, 2016

This is really helpful, but are there any workaround to make it with hapi-auth-cookie so far ? This PR has not yet been merged

@dahjelle

This comment has been minimized.

Copy link

dahjelle commented Oct 28, 2016

Since isSameSite now defaults to true, not having this option means there is no way to get the previous behavior (i.e. not setting the SameSite attribute at all).

@arnivuo

This comment has been minimized.

Copy link
Contributor Author

arnivuo commented Nov 4, 2016

@goddyZhao not sure but I guess you could use npm Git URL as dependency to my fork. Or you could just copy lib/index.js into your project.

@dahjelle are you meaning that default should be false and not Strict. Because maybe there's point. My thought was that because statehood has Strict as default it would be good to reflect that here as well.

@dahjelle

This comment has been minimized.

Copy link

dahjelle commented Nov 7, 2016

@arnivuo

are you meaning that default should be false and not Strict. Because maybe there's point.

No, I meant that with the default to isSameSite = 'Strict' and not allowing the user to change the default, there is no way to get the previous default behavior of hapi-auth-cookie which is isSameSite = false.

This PR allows the user to allow the previous behavior if desired, which, I think, is pretty important! :-D

Did that clarify?

@arnivuo

This comment has been minimized.

Copy link
Contributor Author

arnivuo commented Nov 16, 2016

@dahjelle Umh...yah clear know. So but you are happy with that user won't get previous behavior by default?

@arnivuo

This comment has been minimized.

Copy link
Contributor Author

arnivuo commented Nov 16, 2016

@jaw187 any change for merging this PR?

@kanongil

This comment has been minimized.

Copy link
Member

kanongil commented Nov 16, 2016

The new behaviour won't work for me. Please allow me to configure it.

@dahjelle

This comment has been minimized.

Copy link

dahjelle commented Nov 16, 2016

@dahjelle Umh...yah clear know. So but you are happy with that user won't get previous behavior by default?

@arnivuo Yes, I'm glad isSameSite defaults to true, even though that isn't the previous default. It's a good security improvement, and it's nice to be secure-by-default.

@arnivuo

This comment has been minimized.

Copy link
Contributor Author

arnivuo commented Nov 16, 2016

@kanongil I added you as a collaborator to my fork.

@kanongil

This comment has been minimized.

Copy link
Member

kanongil commented Nov 16, 2016

@arnivuo Thanks, but I would prefer to get it sorted here. I have the power to merge this PR, but really @jaw187 should handle it.

@jaw187 Are you still maintaining this package?

@arnivuo

This comment has been minimized.

Copy link
Contributor Author

arnivuo commented Nov 21, 2016

@kanongil So what do you need me to do? Or that request wasn't for me.

@eliascodes

This comment has been minimized.

Copy link

eliascodes commented Jan 17, 2017

@jaw187 @kanongil will there be any movement on this? I ran into this problem too. I managed to workaround it using

const server = new Hapi.Server({ connections: { state: { isSameSite: 'Lax' } } });

but it would be nice to handle this directly in the plugin.

Thank you 👍

@kanongil kanongil mentioned this pull request Jan 25, 2017
@jaw187 jaw187 merged commit 4368d97 into hapijs:master Feb 17, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nlf nlf added the feature label Mar 28, 2017
@nlf nlf added this to the 7.0.0 milestone Mar 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.