Fixed issue with incompatible same-site: none browsers

[semoal]

We were using same-site: :none, and secure: true on our session_store, but we found issues in some browsers, after some research we found:

• Versions of Chrome from Chrome 51 to Chrome 66 (inclusive on both ends) are affected
• Versions of UC Browser on Android prior to version 12.13.2
• Versions of Safari and embedded browsers on MacOS 10.14 and all browsers on iOS 12

All of them are checked on the dependency that i've installed, so when a request is coming if the user agent is not compatible, we remove the same_site cookie.

I'm not an expert ruby developer so i'm open to criticism.

Links

• https://www.chromium.org/updates/same-site/incompatible-clients

[tubbo]

Is this just to avoid people possibly configuring their session store incorrectly? Setting SameSite=None isn't something most people want to do, in my experience, so I'm a bit skeptical that this needs to be in redis-actionpack.

[semoal]

Is this just to avoid people possibly configuring their session store incorrectly? Setting SameSite=None isn't something most people want to do, in my experience, so I'm a bit skeptical that this needs to be in redis-actionpack.

This is to avoid, if someone uses SameSite=none, don't have issues with incompatible browsers with that cookie.

Probably this could be implemented outside redis-actionpack, but i'm not sure how.

Basically I found hard to know where to put that "custom logic", to don't send that cookie if the browser is incompatible. I saw we configured in our rails app, config.session_store with redis-action-pack so i've introduced that logic here.

[tubbo]

Not sure how I feel about this change. There are a few red flags here:

• It depends on an external library, which not only could change, but it could also not change when the conditions around setting SameSite=None on cookies also change. I have no real guarantee that the gem will remain up-to-date.
• This can be easily avoided by just not setting same_site: :none on your cookies, which I feel is the best approach for most sites that I've seen. Generally, you don't want your site's session to be accessible by any other party, even if that session is encrypted/signed. It just doesn't make sense to me why you'd want cookies in your Rails session to be sent in a 3rd-party context. If you really want to do this, you can just easily set a signed/encrypted cookie with same_site: :none set on it and avoid the session store altogether.

Also, we generally don't want to add features to this gem when they are not supported in the libraries that this serves as an adapter for. The purpose of redis-actionpack is to adapt the Rails session to be stored within Redis. If you really wanted to make a change here that would affect the maximum number of users, this behavior should be done higher up, in Rack::Session::Abstract::ID::Persisted.

Either way, I don't really see a place for this code in redis-actionpack itself.

[tubbo]

sorry didn't mean to close it :D

[semoal]

Not sure how I feel about this change. There are a few red flags here:

• It depends on an external library, which not only could change, but it could also not change when the conditions around setting SameSite=None on cookies also change. I have no real guarantee that the gem will remain up-to-date.
• This can be easily avoided by just not setting same_site: :none on your cookies, which I feel is the best approach for most sites that I've seen. Generally, you don't want your site's session to be accessible by any other party, even if that session is encrypted/signed. It just doesn't make sense to me why you'd want cookies in your Rails session to be sent in a 3rd-party context. If you really want to do this, you can just easily set a signed/encrypted cookie with same_site: :none set on it and avoid the session store altogether.

Also, we generally don't want to add features to this gem when they are not supported in the libraries that this serves as an adapter for. The purpose of redis-actionpack is to adapt the Rails session to be stored within Redis. If you really wanted to make a change here that would affect the maximum number of users, this behavior should be done higher up, in Rack::Session::Abstract::ID::Persisted.

Either way, I don't really see a place for this code in redis-actionpack itself.

You're absolutely right with the second point, probably "if this has sense to be somewhere", is in the rack:session class, to help the maxium number of users, this is not a dependent of redis, but yes of the session and cookies, so probably has more sense to be in the rack class.

About the second, well, I built that gem has a full suite with 100% code coverage and probably it's something to not change in the future, because the new browsers already has compatibility with it.

So I'll close this pull request, and i'll work on rack repository.

Thanks Tom 4 your help. 👍🏻

[tubbo]

Makes sense to me. I'll be happy to accept the rack update that makes this work! :)