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

enableAutoOutboundTracking breaks target="_blank" and probably noopener security #12

Open
SimonBackx opened this issue Apr 11, 2021 · 38 comments · May be fixed by #21 or #54
Open

enableAutoOutboundTracking breaks target="_blank" and probably noopener security #12

SimonBackx opened this issue Apr 11, 2021 · 38 comments · May be fixed by #21 or #54
Labels
bug Something isn't working

Comments

@SimonBackx
Copy link

SimonBackx commented Apr 11, 2021

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()
  1. Add a button
<a rel="nofollow noopener" target="_blank" href="https://google.com">Test</a>
  1. 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.

@SimonBackx SimonBackx added the bug Something isn't working label Apr 11, 2021
@SimonBackx SimonBackx changed the title enableAutoOutboundTracking breaks target="_blank" enableAutoOutboundTracking breaks target="_blank" and probably noopener security Apr 11, 2021
@ukutaht
Copy link
Contributor

ukutaht commented Apr 12, 2021

Yes, the timeout in

setTimeout(() => {
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
Copy link
Contributor

ukutaht commented Apr 12, 2021

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
Copy link
Author

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
Copy link
Contributor

ukutaht commented Apr 13, 2021

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
Copy link
Author

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
Copy link
Author

SimonBackx commented Apr 13, 2021

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

@ukutaht
Copy link
Contributor

ukutaht commented Apr 13, 2021

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
Copy link
Author

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
Copy link
Contributor

ukutaht commented Apr 28, 2021

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
Copy link
Author

Yes, the sendBeacon approach seems the right one!

@maurice-g
Copy link

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:

location.href = this.href;

@ukutaht
Copy link
Contributor

ukutaht commented Sep 2, 2021

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
Copy link

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

@Joelius300
Copy link

Joelius300 commented Nov 5, 2021

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 🎉

@Joelius300
Copy link

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
Copy link
Contributor

ukutaht commented Nov 8, 2021

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
Copy link

abett commented Nov 16, 2021

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
Copy link
Contributor

metmarkosaric commented Nov 16, 2021

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
Copy link

abett commented Nov 16, 2021

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
Copy link
Contributor

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
Copy link

abett commented Nov 16, 2021

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
Copy link
Contributor

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

@Joelius300
Copy link

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.

setTimeout(() => {
// eslint-disable-next-line functional/immutable-data
location.href = this.href;
}, 150);

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
Copy link
Contributor

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
Copy link

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

Joelius300 added a commit to Joelius300/plausible-tracker that referenced this issue Nov 17, 2021
Joelius300 added a commit to Joelius300/plausible-tracker that referenced this issue Nov 17, 2021
@Joelius300 Joelius300 linked a pull request Nov 17, 2021 that will close this issue
9 tasks
@Joelius300
Copy link

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
Copy link

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
Copy link

maximedupre commented Jul 23, 2022

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
Copy link

smuuf commented Jan 18, 2023

I stumbled upon this issue after our company deployed Plausible's outbound-tracking code and we immediately noticed almost exact same thing as #12 (comment).

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

location.href = this.href;

... 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
Copy link

@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).

@long2ice
Copy link

So how about this problem?

@orvn
Copy link

orvn commented Sep 12, 2023

This is a pretty critical issue, is there any plan to assign it to someone?

@madebyfabian
Copy link

Can you please consider prioritizing this issue higher?
It's still a fairly critical issue today. Just got a complaint from one of my customers about this.

@Barbapapazes
Copy link

Hello,

Since this package have not been update for two years, I made a fork.

I rewrite the tracker to be more modular and I fix this issue.

https://github.com/Barbapapazes/plausible-tracker

@cschweda
Copy link

cschweda commented Mar 1, 2024

Any update on this besides @Barbapapazes new fork (and the fix above from @abett)?

The fix doesn't work for me (under Nuxt 3) -- and I'd like to stick with the canonical plausible instead of a fork.

@reatlat
Copy link

reatlat commented May 28, 2024

Hey @metmarkosaric! Since the package doesn't get updated regularly, do you think you can make @Barbapapazes the maintainer of this package and let him update it? It seems like his package is much better than the original one!

Well done @Barbapapazes!

@orvn
Copy link

orvn commented Jul 5, 2024

@Barbapapazes, have you considered making a PR into this repo?

@Barbapapazes
Copy link

Barbapapazes commented Jul 5, 2024

@orvn yes but actually, since there is no update, I have no real reason to do it. Also, the fork is a full rewrite so a PR will be difficult.

However, I've been approached by @ukutaht on Twitter but no response since a month so I don't really know what I have to do 🤷‍♂️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet