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

New visitor ID generated per action in iframes (SameSite cookie issue) #16161

Closed
promediadd opened this issue Jun 30, 2020 · 4 comments · Fixed by #16733
Closed

New visitor ID generated per action in iframes (SameSite cookie issue) #16161

promediadd opened this issue Jun 30, 2020 · 4 comments · Fixed by #16733
Labels
Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc.

Comments

@promediadd
Copy link

Summary

  • Tracking inside an iframe when that iframe is on a third-party site is creating a new visitorId for every page visit/action taken by a visitor inside that iframe
  • When the site you want to track with Matomo is embedded in an iframe on a third-party site, the getCookie() function cannot find the visitorId cookie inside of the documentAlias.cookie string.
  • In our (limited) testing, changing the SameSite param in setCookie() function from SameSite=Lax to SameSite=None resolves the issue on most browsers (iOS Safari excluded) (i.e. the documentAlias.cookie string now contains our visitorId cookie and things work as expected)

Replicating the issue

  • We first noticed this issue because we saw a large spike in traffic/actions in our Matomo dashboards for a particular site
  • We determined this site was being iframed into a third-party site and any visits occuring inside this iframe were being tracked as new visitors for every page visit and action taken (blowing out the tracking statistics)
  • To replicate the bug and test/prove our theory we created a test page and set the tracking JS to console log the visitorId every 5 seconds:
setInterval(function(){
    _paq.push([function () {
        var piwikVisitor = this.getVisitorId();
        console.log('the visitor ID is: ' + piwikVisitor)
    }]);
},5000)
  • We then opened up the test page without using an iframe and we saw the same visitorId being spat out every 5 seconds (as expected):
    image
  • We then embedded this test page as an iframe on the same domain and we saw the same visitorId being spat out every 5 seconds (as expected)
  • We then embedded this test page as an iframe on a third-party domain and we saw a new visitorId being spat out every 5 seconds (as expected, but not wanted, and thus proving our theory)
    image

Our "almost" fix

  • After trying a bunch of things (see other things we tried below) and pouring over the unminified piwik.js file we found that the "almost" fix for us was setting SameSite=None instead of SameSite=Lax on the setCookie() function
  • This change allowed getCookie() to find the visitorId cookie inside of the documentAlias.cookie string which meant the same visitorId could be used
  • Except this did not work for iOS Safari, which is still producing the same problem of a new visitor ID for every page visit/action, e.g:
    image

Other things we tried

Presumably SameSite=Lax is set for a reason. And as noted SameSite=None does not resolve the issue on iOS Safari. So we have tried other things, such as:

  • We did try setting _paq.push(['setSecureCookie', location.protocol === 'https:']);
  • We did try enabling crossDomainLinking:
_paq.push(["setDomains", ["*.domain1.com", "*.domain2.com"]]);
_paq.push(["enableCrossDomainLinking"]);
  • We have made sure we've upgraded to the latest version of Matomo, both in our core installation and our JS
  • We tried on both secure and non-secure connections, on servers and local development environments

Potentially related...

  • I note this issue from last year is still Open: Meta: Update Cookie Implementations #14904
  • It lists some cookie shortcomings based on SameSite
  • I couldn't see anything here that explicitly related, but there may be some crossover

Is it just us?

  • I couldn't find any explicit mention of this issue on any other forum or git issue
  • Can others replicate this issue using my steps to reproduce above?
  • If it is limited to us then maybe there's something else we're doing incorrectly?
@tsteur
Copy link
Member

tsteur commented Jun 30, 2020

Not sure I can contribute much to this @promediadd

sounds like you almost found a solution except for Safari.

I think when _paq.push(['setSecureCookie', location.protocol === 'https:']); is set and we are on a secure site then Matomo could set SameSite=None. I think this isn't done maybe because it could cause issues when you have HTTP and HTTPS traffic as each would create a different cookie. Although the secure flag would cause different cookies to be set anyway so this might be a quick win.

Re Safari: They did have some SameSite issues but it looks they might be resolved now so it might work in the future: https://bugs.webkit.org/show_bug.cgi?id=198181

@promediadd
Copy link
Author

Thanks @tsteur — so is the expectation at this stage that we should maintain our own version of matomo.js (piwik.js) with these changes we've made for the foreseeable future? i.e. the findings posted here won't be integrated with official future releases?

All good if that is the case, just would like to plan for that.

An update re: iOS Safari

Thanks for the link to that Safari/Webkit bug. While we await all devices in the wild to receive that fix we have (perhaps against best practice re: privacy etc?) found a workaround that allows us to use localStorage instead of cookies if the cookieMatch in the getCookie() function doesn't return any data.

For posterity, in case it helps the core or others who want to maintain their own solution, our (briefly tested) implementation is as follows:

setCookie() function

function setCookie(cookieName, value, msToExpire, path, domain, isSecure) {
  // leave all the existing code
  // and then at the end of the function include:
  localStorage.setItem(cookieName, value);
}

getCookie() function

function getCookie(cookieName) {
  // leave all the existing code
  // but replace the last line 
  // i.e: return cookieMatch ? decodeWrapper(cookieMatch[2]) : 0;
  // with the following:
  if (cookieMatch) {
    cookieMatch = decodeWrapper(cookieMatch[2]);
  }
  else if (localStorage.getItem(cookieName)) {
    cookieMatch = localStorage.getItem(cookieName)
  }
  return cookieMatch ? cookieMatch : 0;
}

deleteCookie() function

function deleteCookie(cookieName, path, domain) {
  setCookie(cookieName, '', -86400, path, domain);
  // keep the existing line above, but also include this new line:
  localStorage.removeItem(cookieName);
}

@tsteur
Copy link
Member

tsteur commented Jul 1, 2020

@promediadd ideally you wouldn't maintain your own tracker file. In best case you could create a pull request for this file so we can review and merge it. Eg a PR that sets SameSite=None when secure flag is set.

If that's not possible to create PR I'd rename the issue to make it more clear on what needs to be done and maybe at some point we'd work on it. It's not clear though while milestone this would go.

The localStorage solution we would however likely not use eg since it's not trivial to delete this data easily after a certain amount of time etc.

We'd then also document this and create an FAQ advising people who track iframes to set the secure flag so they don't run into the same issue.

@tsteur tsteur added the Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. label Jul 1, 2020
beger added a commit to UuMacher/matomo that referenced this issue Oct 1, 2020
tsteur pushed a commit that referenced this issue Nov 17, 2020
@tsteur
Copy link
Member

tsteur commented Nov 17, 2020

fyi this should be fixed as part of the Matomo 4 release see https://matomo.org/faq/how-to/how-do-i-track-a-website-within-an-iframe/

Thanks to @beger and @fealXX

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants