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

Jump to bottom

Fix for page overlay sidebar authentication failure #18569

Closed
wants to merge 2 commits into from
Closed

Fix for page overlay sidebar authentication failure #18569

wants to merge 2 commits into from

Conversation

bx80
Copy link
Contributor

@bx80 bx80 commented Jan 4, 2022 •
edited

Description:

Fixes #18538

When using the page overlay feature, the notifyParentIframe call intermittently gets sent without a session cookie. This is quite random and only occurs approximately once in every 20-30 calls. It appears to be a race condition related iframe access to cookies.

Because there is no session cookie then a new session is started and the response to the notifyParentIframe sets a new session cookie which invalidates the session, this is apparent when the loadSidebar call is made shortly after and the token_auth is rejected and the user is forcibly logged out.

Without a solution to the race condition issue this PR is a workaround to prevent the session breaking. It's not possible to check the session cookie via javascript so a server side check is implemented which will remove the set-cookie header when responding to a notifyParentIframe without any cookies.

To recreate:

  1. Create some visits to a test site clicking between one or two pages.
  2. Open the page overlay view
  3. Set the browser console to view network requested filtered by notifyParentIframe
  4. Click back and forth between the pages that were tracked
  5. Eventually the sidebar will show a 'session expired' error.
  6. Check the last notifyParentIframe call in the console, it will be missing a request session cookie and will receive a set cookie for a new session

With this fix the notifyParentIframe call will still be made without a session cookie, but no set cookie will be returned in the response and the session will not break.

See also L3-197

Review

@bx80 bx80 added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Jan 4, 2022
@bx80 bx80 added this to the 4.7.0 milestone Jan 4, 2022
@bx80 bx80 self-assigned this Jan 4, 2022
@bx80 bx80 added the Needs Review PRs that need a code review label Jan 18, 2022
@sgiehl
Copy link
Member

sgiehl commented Jan 18, 2022

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.

Your workaround would actually solve that, but it's not really a proper solution. I'll close this PR and create a new one with a better fix soon.

@sgiehl sgiehl closed this Jan 18, 2022
@sgiehl sgiehl mentioned this pull request Jan 18, 2022
Merged
11 tasks
@sgiehl sgiehl deleted the m-18538-page-overlay-auth branch April 5, 2023 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@bx80 bx80

Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Milestone
4.7.0
Development

Successfully merging this pull request may close these issues.

Fix page overlay sidebar authentication failure
2 participants
@bx80 @sgiehl