Replace proxy redirect with rel=noreferrer #12780

c960657 · 2018-04-28T14:32:10Z

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 · 2018-04-29T03:13:29Z

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

tsteur · 2018-04-29T03:14:08Z

core/ErrorHandler.php

@@ -123,7 +123,7 @@ private static function getHtmlMessage($errno, $errstr, $errfile, $errline, $tra
        $message = ErrorHandler::getErrNoString($errno) . ' - ' . $errstr;

        $html = "<p>There is an error. Please report the message (Matomo " . (class_exists('Piwik\Version') ? Version::VERSION : '') . ")
-        and full backtrace in the <a href='?module=Proxy&action=redirect&url=https://forum.matomo.org' target='_blank'>Matomo forums</a> (please do a search first as it might have been reported already!).</p>";
+        and full backtrace in the <a target='_blank' rel='noreferrer' href='https://forum.matomo.org'>Matomo forums</a> (please do a search first as it might have been reported already!).</p>";


I reckon for security be also good to always add rel=noopener so rel="noreferrer noopener" but not needed.

Good idea. I'll look into that.

c960657 · 2018-04-29T10:31:41Z

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 · 2018-04-29T19:14:14Z

plugins/MobileMessaging/SMSProvider/ASPSMS.php

@@ -38,17 +38,17 @@ public function getId()

    public function getDescription()
    {
-        return 'You can use <a target="_blank" href="?module=Proxy&action=redirect&url=http://www.aspsms.com/en/?REF=227830"><img src="plugins/MobileMessaging/images/ASPSMS.png"/></a> to send SMS Reports from Piwik.<br/>
+        return 'You can use <a target="_blank" rel="noreferrer" href="http://www.aspsms.com/en/?REF=227830"><img src="plugins/MobileMessaging/images/ASPSMS.png"/></a> to send SMS Reports from Piwik.<br/>


For external links we would for now probably continue to use the proxy.

tsteur · 2018-04-29T19:15:27Z

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 · 2018-04-29T21:16:36Z

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

tsteur · 2018-04-30T19:19:39Z

core/testMinimumPhpVersion.php

                    "\n\n<br/><br/>Note: if for some reasons you cannot install composer, instead install the latest Matomo release from ".
-                    "<a href='https://builds.matomo.org/piwik.zip'>builds.matomo.org</a>.</p>";
+                    "<a href='https://builds.matomo.org/piwik.zip' rel='noreferrer noopener' target='_blank'>builds.matomo.org</a>.</p>";


I think the target _blank is not needed here?

tsteur · 2018-05-01T20:00:32Z

plugins/CoreHome/templates/_donate.twig

@@ -37,7 +37,7 @@
                title="{{ 'CoreHome_SubscribeAndBecomePiwikSupporter'|translate }}"/>
 			<a class="donate-spacer">{{ 'CoreHome_MakeOneTimeDonation'|translate }}</a>
            <a href="index.php?module=CoreHome&action=redirectToPaypal&idSite=1&cmd=_s-xclick&hosted_button_id=RPL23NJURMTFA&bb2_screener_=1357583494+83.233.186.82"
-               rel="noreferrer"  target="_blank" class="donate-one-time">{{ 'CoreHome_MakeOneTimeDonation'|translate }}</a>
+               rel="noopener" target="_blank" class="donate-one-time">{{ 'CoreHome_MakeOneTimeDonation'|translate }}</a>


any reason noreferrer was removed here? is the redirectToPaypal removing the referrer? may be good to still have it.

Hmm, no, seems like a casualty from my search-and-replace rampage :) I'll revert it.

tsteur · 2018-05-01T20:12:31Z

plugins/Proxy/Controller.php

-        if (!self::isPiwikUrl($url)) {
-            Piwik::checkUserHasSomeViewAccess();
-        }
+        Piwik::checkUserHasSomeViewAccess();


I was thinking it will be still good to have previous logic but makes sense to always check now.

tsteur · 2018-05-01T20:13:57Z

plugins/SEO/templates/getRank.twig

@@ -20,15 +20,15 @@
                {{ 'General_Error'|translate }}
            {% else %}
                {% set cleanUrl %}
-                    <a href="http://{{ urlToRank }}" rel="noreferrer"  target="_blank">{{ urlToRank }}</a>
+                    <a href="?module=Proxy&action=redirect&url=http://{{ urlToRank|url_encode }}" rel="noopener" target="_blank">{{ urlToRank }}</a>


does http:// need to be url encoded?

No (we don't do it elsewhere in hard-coded URLs).

tsteur · 2018-05-01T20:16:22Z

plugins/Transitions/javascripts/transitions.js

-        .data('text', title)
-        .data('maxLines', 3);
-
+    var h2 = self.centerBox.find('h2');


Can you maybe explain these changes?

Instead of using window.open(), links are created using regular tags, so they use the same mechanism for handling noreferrer noopener and action=redirect&url=... as everywhere else.

This allows right-click on links and selecting “Copy URL” from the context menu etc.

It was relevant in my first version of this PR before I reverted to use action=redirect&url=... (due to IE 10). I still think it is a good change (it streamlines noreferrer handling and avoids the UI problems with window.open()), but I guess it is not directly related to this PR. So I'll revert the changes from the PR and perhaps create a separate PR instead.

c960657 · 2018-05-01T20:31:35Z

Tests still need some work. Stay tuned.

robocoder · 2018-05-02T15:30:21Z

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 · 2018-05-02T15:39:31Z

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 · 2018-05-02T19:34:56Z

good idea @robocoder @c960657

tsteur · 2018-05-08T19:13:28Z

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

c960657 · 2018-05-09T06:31:03Z

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

robocoder · 2018-05-10T12:13:56Z

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

c960657 · 2018-05-10T13:43:44Z

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?

diosmosis · 2018-07-24T21:39:47Z

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

tsteur · 2018-07-24T22:17:09Z

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 · 2018-07-24T23:35:36Z

Moved my suggestion for an automated test to a new issue: #13204

* Replace proxy redirect with rel=noreferrer * Add noopener * Restore action=redirect for non-Matomo links * Wrap referring URLs * NO target on download link * Fix Github links * Fix whitespace * Fix tests * Revert change * Revert changes * Fix tests * Add noreferrer shim for MSIE 10 * Remove all action=redirect links * Restore noreferrer * Restore test * Fix one more occurrence * Update changelog * Combine if's * Fix changelog wording * Fix stray whitespace

c960657 force-pushed the proxy-redirect branch from 747b3d5 to c8fc441 Compare April 28, 2018 14:42

tsteur reviewed Apr 29, 2018

View reviewed changes

tsteur reviewed Apr 30, 2018

View reviewed changes

tsteur reviewed May 1, 2018

View reviewed changes

c960657 force-pushed the proxy-redirect branch 2 times, most recently from 1d64056 to e338f1a Compare May 2, 2018 14:37

Findus23 mentioned this pull request May 8, 2018

Add rel="noopener noreferrer" to a link #12861

Merged

c960657 force-pushed the proxy-redirect branch 3 times, most recently from c548b45 to 1fcf3ba Compare May 10, 2018 08:18

c960657 force-pushed the proxy-redirect branch from 5356142 to c90a208 Compare May 10, 2018 14:42

c960657 force-pushed the proxy-redirect branch 3 times, most recently from 79409e9 to 1bba0b1 Compare May 22, 2018 20:33

c960657 added 17 commits July 23, 2018 18:55

Restore action=redirect for non-Matomo links

04c08f6

Wrap referring URLs

81fd3d1

NO target on download link

b0b9dfd

Fix Github links

44fde7b

Fix whitespace

c8272d1

Fix tests

1d3c248

Revert change

ce753d8

Revert changes

0b3b52c

Fix tests

9bd45a6

Add noreferrer shim for MSIE 10

1263ea5

Remove all action=redirect links

b6c8f0d

Restore noreferrer

490d146

Restore test

87a0c03

Fix one more occurrence

8abe40c

Update changelog

c8b7c24

Combine if's

68bd47e

Fix changelog wording

47c320a

c960657 force-pushed the proxy-redirect branch from 65dbb46 to 47c320a Compare July 23, 2018 16:56

Fix stray whitespace

897488f

diosmosis mentioned this pull request Jul 24, 2018

Make sure all outgoing links have noreferrer #13204

Closed

Merge branch '3.x-dev' into proxy-redirect

d2e2ffe

diosmosis merged commit e09383e into matomo-org:3.x-dev Jul 25, 2018

mattab added the c: Privacy For issues that impact or improve the privacy. label Aug 28, 2018

Findus23 mentioned this pull request Sep 25, 2018

don't send referrer to plugin authors website #13479

Merged

Findus23 mentioned this pull request May 25, 2019

send Referrer-Policy header #14482

Merged

sgiehl mentioned this pull request Jul 22, 2020

Adds missing rel attribute for external link #16234

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace proxy redirect with rel=noreferrer #12780

Replace proxy redirect with rel=noreferrer #12780

c960657 commented Apr 28, 2018

tsteur commented Apr 29, 2018

tsteur Apr 29, 2018 •

edited

c960657 Apr 29, 2018

c960657 commented Apr 29, 2018

tsteur Apr 29, 2018

tsteur commented Apr 29, 2018

mattab commented Apr 29, 2018

tsteur Apr 30, 2018

tsteur May 1, 2018

c960657 May 2, 2018

tsteur May 1, 2018

tsteur May 1, 2018

tsteur May 1, 2018

c960657 May 2, 2018

tsteur May 1, 2018

c960657 May 2, 2018

c960657 commented May 1, 2018

robocoder commented May 2, 2018

robocoder commented May 2, 2018

tsteur commented May 2, 2018

tsteur commented May 8, 2018

c960657 commented May 9, 2018

robocoder commented May 10, 2018

c960657 commented May 10, 2018

diosmosis commented Jul 24, 2018

tsteur commented Jul 24, 2018

diosmosis commented Jul 24, 2018

Replace proxy redirect with rel=noreferrer #12780

Replace proxy redirect with rel=noreferrer #12780

Conversation

c960657 commented Apr 28, 2018

tsteur commented Apr 29, 2018

tsteur Apr 29, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

c960657 commented Apr 29, 2018

Choose a reason for hiding this comment

tsteur commented Apr 29, 2018

mattab commented Apr 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

c960657 commented May 1, 2018

robocoder commented May 2, 2018

robocoder commented May 2, 2018

tsteur commented May 2, 2018

tsteur commented May 8, 2018

c960657 commented May 9, 2018

robocoder commented May 10, 2018

c960657 commented May 10, 2018

diosmosis commented Jul 24, 2018

tsteur commented Jul 24, 2018

diosmosis commented Jul 24, 2018

tsteur Apr 29, 2018 •

edited