@sgiehl opened this Pull Request on January 18th 2022 Member

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:
https://github.com/matomo-org/matomo/blob/9873cb71e66be0f80839c76e923c3b866dd23b46/core/Session.php#L166-L183

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

@sgiehl commented on January 31st 2022 Member

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

@tsteur commented on January 31st 2022 Member

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

@sgiehl commented on January 31st 2022 Member

@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 commented on January 31st 2022 Member

@sgiehl I'll change the milestone 👍

@sgiehl commented on February 1st 2022 Member

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

@tsteur commented on February 1st 2022 Member

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?

This Pull Request was closed on February 14th 2022
Powered by GitHub Issue Mirror