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

Set isSameSite to false and add code to support Hapi 13.5+ #265

Merged
merged 1 commit into from Sep 21, 2016

Conversation

@ldesplat
Copy link
Contributor

ldesplat commented Sep 20, 2016

Resolves #264 in a non-breaking manner.

Had to turn off code coverage for the Hapi 13 support, due to multiple reasons:

  1. Don't want to run code coverage using hapi 13
  2. Hapi 14 and up, don't error on server.state with cookie passwords less than 32 characters like Hapi 13 did so I don't know how to force an error here either.
  3. The way Hapi is implemented makes it hard to stub out the server.state method to force those scenarios.

Plan is to remove this code in next breaking release and stop support for Hapi 13 at that time.

@ldesplat ldesplat added the bug label Sep 20, 2016
@ldesplat ldesplat added this to the 8.2.1 milestone Sep 20, 2016
@ldesplat ldesplat merged commit 4077318 into hapijs:master Sep 21, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@AdriVanHoudt

This comment has been minimized.

Copy link
Contributor

AdriVanHoudt commented on lib/index.js in 705e1d0 Sep 22, 2016

Why not 'Lax'?

This comment has been minimized.

Copy link
Contributor Author

ldesplat replied Sep 22, 2016

It's documented here #264

The gist of it, is that it did not work with Chrome in non-incognito mode even after cleaning all the cache and cookies and such. For some reason, chrome refused to send the cookie. Only browser that did in my tests. I don't mind somebody trying to replicate that and tell me I missed something obvious.

This comment has been minimized.

Copy link
Contributor Author

ldesplat replied Sep 22, 2016

Maybe it saw my previous 'Strict' cookie and I did not reset properly? I would love to put it to Lax since it worked great with the 5 providers and browsers I tested.

This comment has been minimized.

Copy link
Contributor

AdriVanHoudt replied Sep 22, 2016

I was somehow not watching this repo :P will not miss an issue again ;)
Let me try to put it to Lax tomorrow. I have another cookie that didn't work with Strict but does with Lax.

@ldesplat

This comment has been minimized.

Copy link
Contributor Author

ldesplat commented Sep 22, 2016

Believe it or not, thanks to some great work from others, Strict did work with every browser except Chrome. It would fail the first time, but then we initiate a redirect to ourselves (only once) and all the browsers would properly send the Strict cookie :) except Chrome in my tests

@AdriVanHoudt

This comment has been minimized.

Copy link
Contributor

AdriVanHoudt commented Sep 22, 2016

So it would fail on every browser but then you do a redirect to yourself and it works? I assume this is because then you become the initiator and then it is ok to send the strict cookie along since it is now the same domain. But Chrome did not do this?

@ldesplat

This comment has been minimized.

Copy link
Contributor Author

ldesplat commented Sep 22, 2016

Correct. Chrome just refused to send the cookie when we became the initiator for Strict.
For Lax it should just work from the beginning and it did with all browsers but Chrome again! Could not work on my machine.

@AdriVanHoudt

This comment has been minimized.

Copy link
Contributor

AdriVanHoudt commented Sep 23, 2016

I just tried to put it to Lax and it does seem to work on Chrome Version 53.0.2785.116 (64-bit) on macOS,
changing this would be at least a minor (if not major) since this can have some implications. Could you test again maybe it was a issue in your version of chrome?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.