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

Middleware that sets cookies to samesite=none #375

Closed
wants to merge 4 commits into from

Conversation

jedimdan
Copy link
Contributor

@jedimdan jedimdan commented Jan 20, 2020

Attempt to fix #367.

What I'm not sure is the strategy to insert this middleware. Should this be included in the install docs for developers to include into the global middleware list? The consideration is that this only applies for requests that are within an embedded app.

Also, the other part I'm not sure is if you are okay with a new dependency, seeing that this library has been very clean from other dependencies so far.

Based on Shopify's implementation: Shopify/shopify_app#851

@jedimdan jedimdan requested a review from gnikyt January 20, 2020 09:13
@coveralls
Copy link

coveralls commented Jan 20, 2020

Coverage Status

Coverage decreased (-0.8%) to 98.976% when pulling 45a5365 on jedimdan:bugfix-samesite-none-cookies into d565fc8 on ohmybrew:master.

@jedimdan
Copy link
Contributor Author

@darrynten I've got this working middleware that I already use in my production. I just need help figuring out what's the most appropriate way to make it available in this package. Do you have any suggestions?

@darrynten
Copy link
Contributor

darrynten commented Jan 31, 2020

I've been looking at this and there is no need to add any code to fix this issue.

The change you mentioned in #277 works perfectly.

For others who may need this info, the change needs to be done in the host laravel app that is using this.

You make the following changes in config/session.php

Do not make the changes I suggested before this edit (or in the linked issue). It would break older browsers. See #382

@jedimdan
Copy link
Contributor Author

Hi @darrynten the additional changes are necessary because of the browser compatibility issues. It's not sufficient to simply change the cookie settings.

I've explained this in detail in #367. The challenge is that the browsers that have issues are quite recent (iOS 12 and macOS 10.14) so it's quite critical that the compatibility issues are covered. This code is based on Shopify's changes to their own Rails library as well.

@jedimdan
Copy link
Contributor Author

jedimdan commented Jan 31, 2020

Regarding compatibility issues please read this article. This cookie completely breaks the app on Safari for iOS 12 and macOS 10.14 so it is critical that the cookie stays as the default settings for those browsers.

@darrynten
Copy link
Contributor

I'm actively working on this, I will update as soon as possible

@jedimdan
Copy link
Contributor Author

I'm actively working on this, I will update as soon as possible

What's your game plan?

@jedimdan
Copy link
Contributor Author

@darrynten I also recommend editing your recommendation above on changing the sessions config as doing so will cause the app to unexpectedly break on Safari for macOS 10.14, which I believe is still heavily used.

@darrynten darrynten mentioned this pull request Jan 31, 2020
@darrynten
Copy link
Contributor

Please see #382 it resolves the problem cleanly on the Service level instead of on the Middleware level

@jedimdan
Copy link
Contributor Author

Closing this PR in favour of #382

@jedimdan jedimdan closed this Jan 31, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cookies on embedded apps blocked by Chrome 80
3 participants