@c960657 opened this Pull Request on April 28th 2018 Contributor

All modern browsers support rel=noreferrer. This seems like a better and easier way to link to external sites.

When browsers eventually get support for Referer-Policy, all these rel=noreferrer can be replaced with a single HTTP header.

@tsteur commented on April 29th 2018 Member

FYI: We support IE 10 which doesn't support noreferrer. However, we have been using noreferrer couple of times recently instead of the proxy.

@c960657 commented on April 29th 2018 Contributor

Oh, I was not aware that we want to support IE 10. That's ambitious :-)

But you seem to imply that it is acceptable to ignore IE 10 in this case? The links will still work, of course.

@tsteur commented on April 29th 2018 Member

I would say for matomo.org links it should be fine, for external links I would probably still use the proxy until IE support is dropped. Any thoughts @mattab ?

@mattab commented on April 29th 2018 Member

For external links to non matomo domains, would be best to use the proxy as long as we support IE10 :+1:

@c960657 commented on May 1st 2018 Contributor

Tests still need some work. Stay tuned.

@robocoder commented on May 2nd 2018 Contributor

Just a thought, but couldn't you add a shim for IE10 to do to proxy redirect, and that way use rel=noreferrer through-out?

@robocoder commented on May 2nd 2018 Contributor

Something like the following pseudo-code:

if (navigator.appVersion.indexOf("MSIE 10") !== -1) {
    $('a[rel="noreferrer"]').click(function (evt) {
        var element = evt.target;
        var url = '?module=Proxy&action=redirect&url=' + element.getAttribute('href');
        window.open(url, element.getAttribute('target'));
        return false;
@tsteur commented on May 2nd 2018 Member

good idea @robocoder @c960657

@tsteur commented on May 8th 2018 Member

@c960657 would you be keen on checking the idea @robocoder suggested?

@c960657 commented on May 9th 2018 Contributor

Sure :-) I have been busy the past week, but I'll get back to it shortly.

@robocoder commented on May 10th 2018 Contributor

TIL window.open doesn't send the Referer header. Sweet.

@c960657 commented on May 10th 2018 Contributor

Yes, apparently so. I haven't been able to find an official source for this, but my own tests as well as various reports on Stack Overflow etc. seem to confirm it.

For this reason, I have restored the changes to transitions.js. The existing code that uses window.open() together with action=redirect does not work in MSIE 10.

Travis seems to be mostly happy now. There is one failing test in the CustomDimensions plugin that is a Git submodule. I don't know the procedure for dealing with such changes. A separate PR to the CustomDimensions repo after this PR has been merged?

@tsteur commented on May 23rd 2018 Member

Thanks for this @c960657 very appreciated 👍 Left only 2 comments or so.

Maybe at some point @sgiehl or @diosmosis could have a look as well in case I missed something.

Otherwise looks good 👍

@c960657 commented on May 23rd 2018 Contributor

Hmm, I don't understand why the Visitor log UI tests are failing. Any idea?

@diosmosis commented on June 22nd 2018 Member

Two things:

  • I've noticed several links are missing either noreferrer or noopener.
  • Might be useful to have some code in the UI tests that check that all links to external domains have this rel. The check would run right after a screenshot test passes. Thoughts (cc @tsteur)?
@diosmosis commented on July 24th 2018 Member

Hi @c960657 will you be able to make the requested changes? I still see some links missing either noopener/noreferrer.

@tsteur commented on July 24th 2018 Member

I reckon we can otherwise also just merge eventually as it'll be still better than before. IMO it be fine if not all are changed at this stage.

@diosmosis commented on July 24th 2018 Member

Moved my suggestion for an automated test to a new issue: https://github.com/matomo-org/matomo/issues/13204

This Pull Request was closed on July 25th 2018
Powered by GitHub Issue Mirror