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

Various fixes for Samesite handling in sessions, iframes #15561

Merged
merged 9 commits into from Feb 20, 2020

Conversation

diosmosis
Copy link
Member

@diosmosis diosmosis commented Feb 13, 2020

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

…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.
@diosmosis diosmosis added the Needs Review PRs that need a code review label Feb 13, 2020
@diosmosis diosmosis added this to the 3.13.3 milestone Feb 13, 2020
core/Session.php Outdated Show resolved Hide resolved
Copy link
Member

@sgiehl sgiehl left a 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:

matomo/core/Cookie.php

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');
Copy link
Member

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

@diosmosis
Copy link
Member Author

great points @sgiehl , I'll them asap.

@tsteur
Copy link
Member

tsteur commented Feb 14, 2020

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.

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.

@diosmosis
Copy link
Member Author

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.

@tsteur tsteur modified the milestones: 3.13.3, 3.13.4 Feb 20, 2020
@tsteur tsteur merged commit 071f505 into 3.x-dev Feb 20, 2020
@tsteur tsteur deleted the session-sames-site branch February 20, 2020 03:05
@mattab mattab changed the title Fix couple issues w/ samesite handling in session, make sure session … Various fixes for Samesite handling Feb 24, 2020
@mattab mattab changed the title Various fixes for Samesite handling Various fixes for Samesite handling in sessions, iframes Feb 24, 2020
jbuget pushed a commit to 1024pix/pix-analytics that referenced this pull request Sep 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants