enableAutoOutboundTracking breaks target="_blank" and probably noopener security

[SimonBackx]

trafficstars

Versions

• 0.3.1

Describe the bug

When loading plausible with link tracking, links with target _blank are no longer opened in a new tab.

Expected behavior

Instead, the link should open in a new tab.

Steps to reproduce

Steps:

1. Install normally
2. Set it up
3. Enable enableAutoOutboundTracking

    import Plausible from 'plausible-tracker'
    const plausible = Plausible({
        domain: 'example.com',
        trackLocalhost: true
    })

    // This tracks the current page view and all future ones as well
    plausible.enableAutoPageviews()
    plausible.enableAutoOutboundTracking()

4. Add a button

<a rel="nofollow noopener" target="_blank" href="https://google.com">Test</a>

5. Click on the button. The page opens in the same tab, not in a new tab

Your Environment

• OS: Mac
• Browser: Safari, Chrome, (probably all)
• Versions: all

Additional context

Might also be interesting to check if the rel attributes are respected (noopener) from a security perspective.

[ukutaht]

Yes, the timeout in https://github.com/plausible/plausible-tracker/blob/a5f8e946263b4e9898c729293f74a095ef71cfcb/src/lib/tracker.ts#L286 should only trigger for links that are not target="_blank".

For target="_blank" links, browser default behaviour should be allowed to happen as we do in our main tracker script here: https://github.com/plausible/analytics/blob/master/tracker/src/plausible.js#L87

[ukutaht]

Also, an issue we had on the main tracker script was middle clicking on the mouse to open the link in a new tab. Outbound link tracking was messing with the default behaviour there which was fixed. Is this issue also present in this tracker script?

[SimonBackx]

I'm not sure but shouldn't you also replace the location.href = link.href; with something that respects the rel and target attributes? https://dev.to/dhilipkmr/why-should-you-use-noopener-beware-of-security-flaws-3i57

[ukutaht]

When using target="_blank", we just allow the default browser behaviour. This line:

location.href = link.href;

does not run when using target="_blank". So it already respects target="_blank" rel="noopener" links.

[SimonBackx]

Okay great! To prevent all the special cases for each browser, it might be an idea to update the way you delay the click event. If you redispatch the event, you don't need to check on shift keys, middle mouse etc.

I've made an example https://github.com/SimonBackx/link-delay-test

Demo at https://simonbackx.github.io/link-delay-test/

[SimonBackx]

BTW, the current behaviour in plausible does stop noopener when not using target="blank"

[ukutaht]

BTW, the current behaviour in plausible does stop noopener when not using target="blank"

Darn, yeah that's true.

I do like the idea of just falling back to browser defaults as much as possible. Your demo is great, thanks for that. However, middle clicking and shift-clicking does not open the link in a new tab for me :/ Running FireFox on Elementary OS.

[SimonBackx]

BTW, the current behaviour in plausible does stop noopener when not using target="blank"

Darn, yeah that's true.

I do like the idea of just falling back to browser defaults as much as possible. Your demo is great, thanks for that. However, middle clicking and shift-clicking does not open the link in a new tab for me :/ Running FireFox on Elementary OS.

Oh too bad, does work on Chrome and Safari on MacOS.

[ukutaht]

I tested again:

1. Firefox on Elementary: middle click nor ctrl-click work to open in a new tab
2. Chromium on Elmentary: middle click does not work, ctrl-click does work

The only thing I can think of is the second time the event is dispatched, it has isTrusted: false. I wonder if there's some security feature in the browser that doesn't allow JS to trigger a new tab link click programmatically. I didn't find much information about why it would be prevented.

Thanks @SimonBackx for making this demo, it's great. But I cannot merge it since it doesn't work consistently everywhere.

I think the best solution is switching to navigator.sendBeacon to send the event itself. That way we don't need to mess with the browser event at all, we can leave everything as default browser behaviour but still get the desired result of sending the event to our backend. What do you think?

[SimonBackx]

Yes, the sendBeacon approach seems the right one!

[maurice-g]

Not sure if this belongs here, but enableAutoOutboundTracking also broke an interactive site of ours: We had a simple anchor (without href) with onclick event handler attached. The event handler would trigger the opening of a modal. After clicking the link, however, the entire page would reload, killing the modal that was just opening again. Assume this line to be the culprit: https://github.com/plausible/plausible-tracker/blob/a5f8e946263b4e9898c729293f74a095ef71cfcb/src/lib/tracker.ts#L288

[ukutaht]

Thanks for reporting @maurice-g.

I can't see a way to fix it other than moving to a completely different method for tracking like sendBeacon or something else.

[maurice-g]

I understand. But I do think this would warrant an entry in the docs to warn users that this function might break functionality.

[Joelius300]

I do think this would warrant an entry in the docs to warn users that this function might break functionality.

I agree. It's very unfortunate that this doesn't work out of the box.

Edit: My PR adding a warning about this was just merged :tada:

[Joelius300]

I can't see a way to fix it other than moving to a completely different method for tracking like sendBeacon or something else.

So is it correct that #16 would solve this issue? Unfortunately I don't know this technology and don't have enough time to invest right now otherwise I'd try to contribute :)
Also there might be some issues with tracking blockers like uBlock (I'll add that to #16).

[ukutaht]

So is it correct that #16 would solve this issue? Unfortunately I don't know this technology and don't have enough time to invest right now otherwise I'd try to contribute :)

Yes, using sendBeacon would mean we don't have to hijack any default browser behaviour. We could allow the browser to just do it's own thing while the beacon sends data in the background.

[abett]

Had to dig for a good while to realize that my target="_top" links stopped working due to me using the outbound link tracking script, so I ended up here as well... :)

Unless you're planning to move to sendBeacon as a fix in the near future, is there any chance you can intercept the link target and, if that's set to _top, write window.top.location.href instead of the current window frame?

For now this is blocking us from migrating our production environment to plausible, because unfortunately we rely on shipping some content in iframes and we can't have outbound links break in these...

[metmarkosaric]

thanks @abett! there are no short term plans about the beacon api that we can tell you about. we are a small team with limited development resources so need to focus on most frequently requested features first. in the meanwhile you could use our default script and not the oubtound link click script. thanks for understanding!

[abett]

Thanks for the quick reply @metmarkosaric, completely understand your priorities.

I found this blog post of yours from a year ago - if I adjust the GA script you shared to send a plausible event instead, does it achieve the same outcome as plausible.outbound-links.js - or should I take something else into consideration when trying to replicate the functionality with just the default script?

[metmarkosaric]

you're welcome! could you try that please and let us know if it works? would be a nice workaround until we get the beacon api working

[abett]

Hoping I haven't included a typo here, yes it worked as following:

document.addEventListener('click', (event) => {
  let link = event.target;
  while (link && (typeof link.tagName === 'undefined' || link.tagName.toLowerCase() !== 'a' || !link.href)) {
    link = link.parentNode;
  }

  if (link && link.href && link.host && link.host !== location.host) {
    const props = { url: link.href };

    console.debug('tracking now', {props});
    window.plausible('Outbound Link: Click', { props });

    // Allow event to be sent before the page is unloaded
    if (!link.target || link.target.match(/^_(self|parent|top)$/i)) {
      setTimeout(function() {
        console.debug('navigating now');
        if (link.target === '_top') window.top.location.href = link.href;
        if (link.target === '_parent') window.parent.location.href = link.href;
        location.href = link.href;
      }, 150);
      event.preventDefault();
    }
  }
})

[metmarkosaric]

that's great news @abett, thanks for sharing! i've added a link to your comment in our docs

[Joelius300]

Thanks @abett @metmarkosaric would it make sense to add this logic to the current solution? I mean checking the target attribute instead of always just assigning to location.href.

https://github.com/plausible/plausible-tracker/blob/a837ff455a75b81177bbe0cef866fafd95e5c3ca/src/lib/tracker.ts#L292-L295

For target _blank it should just track the event but nothing more (browser handles that), for _top and _parent assign to the respective window properties and for _self (and no target as that's default) assign to location.href as it's doing right now.
IIRC this matches the snippet by @abett although I've never used target _top and _parent.

[metmarkosaric]

thanks @Joelius300! it's something we may be able to look at but i cannot make any promise. all of our development resources are focused on the data import and performance improvements at the moment.

[Joelius300]

No worries 👍🏼 in that case I might submit a PR for your consideration when you have time.

[Joelius300]

Opened #21 with a fix. It has no tests currently so it's not ready for merge yet but I hope it helps as a start.

For anyone needing this feature right now, I'm using my fixed fork in a project of mine with git submodule. Let yourself get inspired ((no warranties)) 😄

[Joelius300]

Yes, the timeout in

https://github.com/plausible/plausible-tracker/blob/master/src/lib/tracker.ts#L286 should only trigger for links that are not target="_blank".

For target="_blank" links, browser default behaviour should be allowed to happen as we do in our main tracker script here: https://github.com/plausible/analytics/blob/master/tracker/src/plausible.js#L87

Looking back at the comment by @ukutaht in April, I realized that there would now be a discrepancy between the linked script and the npm module. In the js script you're also sending events on ctrl, shift and middle click (at least that's what I can tell from the code). With this PR, ctrl and middle click purposefully aren't sent and I completely forgot about shift and meta, which probably should be treated like ctrl.
However, I also see in the js script that it always writes to location.href so _top and _parent wouldn't work when used with iframes AFAIK.

So I guess it would be good to define a behavior for all those cases and consistently implement it in the npm module and js script without breaking links.

Although out of scope of this PR, is there a reason you aren't using the npm module for the tracker in the main repo? I'm assuming it's about the size, right? As an outsider with not that much experience my first thought was just how hard it must be to keep those two in sync without accidentally breaking the same feature policy.

[maximedupre]

Hey guys! Sorry, I'm a bit confused... so what's the solution? 😁

If we want to have target="_blank" links, is the only solution to remove plausible.enableAutoOutboundTracking();?

[smuuf]

I stumbled upon this issue after our company deployed Plausible's outbound-tracking code and we immediately noticed almost exact same thing as https://github.com/plausible/plausible-tracker/issues/12#issuecomment-911588798.

Pardon my ignorance (I'm not entirely versed in these JS+browser API things), but why is the line:

https://github.com/plausible/plausible-tracker/blob/a5f8e946263b4e9898c729293f74a095ef71cfcb/src/lib/tracker.ts#L288

... even necessary? Why does the Plausible tracking code need to do the actual request? Why doesn't it let it just pass and let the browser deal with the navigation itself (which also gives our app a chance to handle the click event the way we need it, without Plausible tracking code force-navigating away)? 🤔

[Joelius300]

@smuuf The script prevents the browser from executing the default behavior (which is to immediately switch sites (as fast as possible)), to quickly send an analytics request. Then it waits for a small delay in which the analytics request can be completed. Afterwards, the default behavior has to be invoked manually because it was suppressed originally, therefore a write to location.href is necessary to actually do the navigation that one would expect from a link click by default.

@smuuf @maximedupre as mentioned in my previous comment, there is an open PR to fix this (#21), which you can use by getting inspired from the links in my comment.

@ everyonewhocares The only thing missing to allow #21 to be merged is tests as far as I understand. I do not have the time or interest to dive into the testing infrastructure used in this project but if you do, feel free to comment how to approach them or outright implement some tests on top of my PR (e.g. in your own fork, could then supersede my PR).