Fix session timeouts in overlay session #18648

sgiehl · 2022-01-18T16:17:23Z

Description:

As commented in #18569:

I tried to reproduce the issue for a while, but wasn't able to with my normal setup. So I started to have some deeper thoughts about this issue. I excluded a possible browser bug, so it needed to be something within Matomo that let's the browser struggle with the session cookie.

So I took a closer look at the requests that are sent by the Overlay. All requests that are sent to index.php are actually setting the session cookie. But there is one big difference those requests are having.
We have a special handling for the samesite cookie attribute implemented for all requests that are going to the Overlay module:

matomo/core/Session.php

Lines 166 to 183 in 9873cb7

public static function getSameSiteCookieValue()

{

$config = Config::getInstance();

$general = $config->General;

$module = Piwik::getModule();

$action = Piwik::getAction();

$isOptOutRequest = $module == 'CoreAdminHome' && $action == 'optOut';

$isOverlay = $module == 'Overlay';

$shouldUseNone = !empty($general['enable_framed_pages']) || $isOptOutRequest || $isOverlay;

if ($shouldUseNone && ProxyHttp::isHttps()) {

return 'None';

}

return 'Lax';

}

The overlay page is sending multiple requests to the API. Such requests are actually setting the value to lax, while the requests going to the overlay module are setting the value to none.

That wasn't a problem in my setup as the tracked url and matomo were both running on the same domain. So I set up another vhost to have different urls for matomo and the tracked site. Using this set up it was easy to reproduce the problem.

So the problem actually is that depending on the request that last finished, before the notifyParentIframe request is sent, the samesite attribute is set to lax or none. With none everything works fine, but if the attribute is lax the iframe can't access the cookie, so it is not sent with the request, causing a new session to be started.

This fix actually checks if the referrer contains module=Overlay to set the samesite=none for those requests as well. This should properly work unless the referrer gets blocked for some reason.
An alternative would be to send some kind of magic parameter with all requests that are sent by the overlay, so we can check for that parameter.

While testing that I also saw, that the Overlay was sending a request to API.getPagesComparisonsDisabledFor, which is not needed, so I added the Overlay module to the exclude list for that.

@tsteur I have targeted the next_release branch as it might be nice to include that fix maybe. If you think it's to risky, feel free to change the milestone and I'll rebase the branch to 4.x-dev instead.

fixes #18538
replaces #18569

Review

core/Session.php

bx80

Makes sense now that the sequence of the multiple API calls was the race condition combined with different same site values eventually affecting iframe cookie availability, great spot! 🤯 👍

This is a much cleaner fix than my approach of knobbling the new session cookie.

I've been doing all testing with a separate vhost/domain - with this fix applied I did 100 page overlay link clicks without issue, then switched the the 4.x-dev branch and got a session expired error within three clicks, back to the fix branch and another 50 link clicks without issue. So looks like it solves the issue well.

tsteur

Added another comment to check. Will also create an issue that we document how Overlay works technically as it's always a bit confusing and this might help understand things better.

plugins/Overlay/Overlay.php

tsteur

Looks good. Be great though to add some tests to make sure eg when passing the Overlay parameter twice in the URL, that it behaves correctly as this is something that could break again in the future. Be great to add a few tests for isOverlayRequest (eg could make it isOverlayRequest($module, $action, $method, $referrer) for better testability )

tsteur · 2022-01-24T22:20:22Z

plugins/Overlay/Overlay.php

+
+        $isOverlay = $module == 'Overlay';
+        $referrerUrlQuery = parse_url(Url::getReferrer() ?? '', PHP_URL_QUERY);
+        parse_str($referrerUrlQuery, $referrerUrlQueryParams);


not sure if it makes sense to use UrlHelper::getArrayFromQueryString() which we also use in other places? Probably not too important as it might do the same, just wondering for consistency

I guess it doesn't do the exactly same for parameter arrays, but should be fine to use that one as well. will change that

sgiehl · 2022-01-31T13:10:29Z

@tsteur I've added a bunch of tests for the new method.

tsteur · 2022-01-31T19:45:40Z

@sgiehl seems various other commits are included here? As the release might be today have to maybe target 4.x-dev after all?

sgiehl · 2022-01-31T20:17:24Z

@tsteur Sorry I merged in 4.x-dev without thinking much about it. Rebased the branch so the changes aren't included anymore. Feel free to merge if needed or change the milestone and I can rebase to 4.x-dev again

tsteur · 2022-01-31T21:35:10Z

@sgiehl I'll change the milestone 👍

… from overlay session

sgiehl · 2022-02-01T13:45:48Z

@tsteur changed the base to 4.x-dev. Should be ready for a last review now....

tsteur · 2022-02-01T18:38:23Z

Looks good to me from a code perspective. @bx80 not sure if you can still easily reproduce the issue locally. If so, could you maybe also confirm it works?

bx80

I retested clicking around the overlay repeatedly without encountering the original issue which is quickly apparent on 4.x-dev. The unit tests all pass.

Might be outside the scope of this change, but on PHP8.1 the tests show a deprecation warning PHP Deprecated: strlen(): Passing null to parameter #1 ($string) of type string is deprecated in /var/www/matomo/core/UrlHelper.php on line 205 which is easily fixed by adding a null check to that line in UrlHelper:

if ($urlQuery === null || strlen($urlQuery) == 0) {

sgiehl added not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Needs Review PRs that need a code review labels Jan 18, 2022

sgiehl added this to the 4.7.0 milestone Jan 18, 2022

sgiehl requested review from tsteur and bx80 January 18, 2022 16:17

sgiehl linked an issue Jan 18, 2022 that may be closed by this pull request

Fix page overlay sidebar authentication failure #18538

Closed

tsteur reviewed Jan 18, 2022

View reviewed changes

core/Session.php Outdated Show resolved Hide resolved

bx80 approved these changes Jan 18, 2022

View reviewed changes

tsteur reviewed Jan 19, 2022

View reviewed changes

plugins/Overlay/Overlay.php Outdated Show resolved Hide resolved

plugins/Overlay/Overlay.php Show resolved Hide resolved

tsteur reviewed Jan 24, 2022

View reviewed changes

sgiehl force-pushed the fixoverlaysession branch from 9ed704f to 78b45fb Compare January 31, 2022 20:14

tsteur modified the milestones: 4.7.0, 4.8.0 Jan 31, 2022

sgiehl added 7 commits February 1, 2022 14:44

Ensure samesite cookie attribute is set correctly for requests coming…

ef86089

… from overlay session

Don't fetch API.getPagesComparisonsDisabledFor on Overlay page

d4b8ca5

Check for explicit overlay requests

fb1c32d

Adds valid host check

d49a0a1

parse referer query

0932704

use UrlHelper::getArrayFromQueryString

a9dcad6

Adds some tests for Overlay::isOverlayRequest

2e9148a

sgiehl force-pushed the fixoverlaysession branch from 78b45fb to 2e9148a Compare February 1, 2022 13:44

sgiehl changed the base branch from next_release to 4.x-dev February 1, 2022 13:44

sgiehl requested a review from bx80 February 2, 2022 15:57

bx80 approved these changes Feb 8, 2022

View reviewed changes

bx80 removed the Needs Review PRs that need a code review label Feb 10, 2022

sgiehl and others added 3 commits February 14, 2022 17:09

apply review feedback

aa91196

Merge branch '4.x-dev' into fixoverlaysession

c7e58cc

built vue files

e1ecc15

sgiehl merged commit 34e2265 into 4.x-dev Feb 14, 2022

sgiehl deleted the fixoverlaysession branch February 14, 2022 16:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix session timeouts in overlay session #18648

Fix session timeouts in overlay session #18648

sgiehl commented Jan 18, 2022

bx80 left a comment

tsteur left a comment

tsteur left a comment

tsteur Jan 24, 2022

sgiehl Jan 26, 2022

sgiehl commented Jan 31, 2022

tsteur commented Jan 31, 2022

sgiehl commented Jan 31, 2022

tsteur commented Jan 31, 2022

sgiehl commented Feb 1, 2022

tsteur commented Feb 1, 2022

bx80 left a comment

	public static function getSameSiteCookieValue()
	{
	$config = Config::getInstance();
	$general = $config->General;

	$module = Piwik::getModule();
	$action = Piwik::getAction();

	$isOptOutRequest = $module == 'CoreAdminHome' && $action == 'optOut';
	$isOverlay = $module == 'Overlay';
	$shouldUseNone = !empty($general['enable_framed_pages']) \|\| $isOptOutRequest \|\| $isOverlay;

	if ($shouldUseNone && ProxyHttp::isHttps()) {
	return 'None';
	}

	return 'Lax';
	}

Fix session timeouts in overlay session #18648

Fix session timeouts in overlay session #18648

Conversation

sgiehl commented Jan 18, 2022

Description:

Review

bx80 left a comment

Choose a reason for hiding this comment

tsteur left a comment

Choose a reason for hiding this comment

tsteur left a comment

Choose a reason for hiding this comment

tsteur Jan 24, 2022

Choose a reason for hiding this comment

sgiehl Jan 26, 2022

Choose a reason for hiding this comment

sgiehl commented Jan 31, 2022

tsteur commented Jan 31, 2022

sgiehl commented Jan 31, 2022

tsteur commented Jan 31, 2022

sgiehl commented Feb 1, 2022

tsteur commented Feb 1, 2022

bx80 left a comment

Choose a reason for hiding this comment