-
Notifications
You must be signed in to change notification settings - Fork 684
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
SameSiteCookieMiddleware breaks cookies in test environment #891
Comments
Nice! Sorry I missed #874 when I created this one. I have a concern that switching this middleware off for the entire test suite might create an issue where some tests might pass in the test environment but fail in other environments in certain situations. I don't have any specific examples, but anytime there is parity between production and the test environment it weakens the accuracy of the test suite. This may not be a huge concern in this case, but I did want to at least bring it up. Thanks for the fix! |
That is a valid concern however I think there is a disconnect in how sessions are handled in tests and production. This middleware should also be temporary until rails provides better handling of the samesite settings in the cookie store |
Pff this took us 2 whole development days to figure out. As it was breaking everything in our development env. (and it wasn't really clear it was related to shopify). So happy we found it! |
We updated from version
11.4.0
of the gem to12.0.0
. After that upgrade the majority of our system specs were breaking. I isolated the upgrade to ensure that it was only the shopify_app gem (not some other dependency) version change that caused the issue. Diff of 11.4.0 to 12.0.0The issue manifested as an inability to set the
flash
within Rails. Many of our specs were validating that certain flash messages were present in the page and those specs were failing. After digging in a little deeper I found that no session cookie was being set at all in the browser.I was able to work around this issue by disabling the
enable_same_site_none
configuration option for the test environment. This is not an ideal fix because it creates parity between the test environment and production, but it will get us by for now.The other environments do not seem to exhibit any issues with this middleware.
Workaround:
The text was updated successfully, but these errors were encountered: