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
Various fixes for Samesite handling in sessions, iframes #15561
Conversation
…start has correct value, write header where needed, and use None for opt out frame so the session ID is sent when embedding the iframe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looked through the changes and if I unterstand everything correct it looks mostly good so far. But I'm wondering if those lines need an adjustment as well:
Lines 438 to 458 in 109926d
*/ | |
private static function getSameSiteValueForBrowser($default) | |
{ | |
$sameSite = ucfirst(strtolower($default)); | |
if ($sameSite == 'None') { | |
$userAgent = Http::getUserAgent(); | |
$ddFactory = StaticContainer::get(\Piwik\DeviceDetector\DeviceDetectorFactory::class); | |
$deviceDetector = $ddFactory->makeInstance($userAgent); | |
$deviceDetector->parse(); | |
$browser = $deviceDetector->getClient(); | |
if (is_array($browser)) { | |
$browser = $browser['name']; | |
} | |
if ((!ProxyHttp::isHttps()) && $browser === 'Chrome') { | |
$sameSite = 'Lax'; | |
} else if ($browser === 'Safari') { | |
$sameSite = ''; | |
} | |
} |
First point here is that Chrome is split up into Chrome
, Chrome Mobile
, Chrome Webview
, ... and many browsers are based on Chrome/Chromium and if they update their base to Chrome 80, they will require the correct SameSite values. So basically it's not good to only check for Browser with the name Chrome
here. It would be better to check the browser family instead. So something like:
$browserFamily = \DeviceDetector\Parser\Client\Browser::getBrowserFamily($deviceDetector->getClient('short_name'))
$isChrome = $browserFamily === 'Chrome';
Also Firefox and Edge announced to support the SameSite cookies. Won't they restrict SameSite=None
without Secure
as well? Not sure if the check for Chrome
makes sense here at all.
js/piwik.js
Outdated
@@ -3290,7 +3290,8 @@ if (typeof window.Piwik !== 'object') { | |||
(msToExpire ? ';expires=' + expiryDate.toGMTString() : '') + | |||
';path=' + (path || '/') + | |||
(domain ? ';domain=' + domain : '') + | |||
(isSecure ? ';secure' : ''); | |||
(isSecure ? ';secure' : '') + | |||
(isSecure ? ';SameSite=None' : ';SameSite=Lax'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually that might maybe cause problems in some specific browser versions that do not accept cookies with SameSite=None
or treat them as SameSite=Strict
instead. There's a list of incompatible browser versions. See https://www.chromium.org/updates/same-site/incompatible-clients
great points @sgiehl , I'll them asap. |
Just fyi they support samesite already for a while. What changes for Chrome is the default when no samesite attribute is specified, then Lax is applied, and when it says None it must be secure. AFAIK Firefox etc that's not the case just yet. We could probably apply same behaviour across browsers already though. Just means opt out might be broken for few more sites on http but might not be too many. |
Updated to always add SameSite=Lax in JS. Seems to work for chrome, firefox & safari. Didn't use old versions of safari of course, though those have issues w/ None only. |
Fix couple issues w/ samesite handling in session, make sure session start has correct value, write header where needed, and use None for opt out frame so the session ID is sent when embedding the iframe.
fix #15513