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

rel=Noopener #14881

Merged
merged 13 commits into from
Mar 26, 2017
Merged

rel=Noopener #14881

merged 13 commits into from
Mar 26, 2017

Conversation

brianteeman
Copy link
Contributor

@brianteeman brianteeman commented Mar 24, 2017

I raised this issue last year with the JSST but it was rejected as a non-issue and more of a browser bug than something Joomla should be doing anything about.

However as now the Google site auditing tool "lighthouse" (lighthouse report https://developers.google.com/web/tools/lighthouse/audits/noopener) will report this as both a performance and a security issue I believe there is no longer an excuse for not taking action in joomla.

It is a really simple change that will not have any visible effect on a website that uses target="_blank" for any of its menu links and the links generated by tinyMCE already do this even if you were not aware of it. This change will act to harden a Joomla website from malicious sites that trick you into creating links to their sites and has a claimed (by google) performance benefit.

Frontend Menus

Menu items that are links to External Urls with the target set to New Window aka target="_blank". There is no need to apply this to other menu links with the target set to New Window as they are to our own web site and presumably you are not going to attack your own web site.

To test this create two menu items. One an article set to open in a new window and the other an external link set to open in a new window. Only the external link menu will have a rel="noopener" attribute

Fields

We can also create links to external web site using the URL field type and this is hard coded to open in a new window with target="_blank" (that's probably wrong to hard code but it is beyond the scope of this PR)

To test this create a field of type URL for an article and then create an article that uses the field and check to see that the link is rendered with the rel="noopener" attribute

Article Links

With articles we have the option to create up to three links - Link A, Link B, Link C. This PR adds the rel="noopener" attribute to the three options new window, popup window, modal window

To test this enter a link for all three fields in an article using each of the changed options the three options new window, popup window, modal window and check to see that the link is rendered with the rel="noopener" attribute

Contact links

With contacts we have the option to complete a field called Website. This is hard coded to open in a new window with target="_blank" (that's probably wrong to hard code but it is beyond the scope of this PR)

To test this create a contact and complete the website field and check to see that the link is rendered with the rel="noopener" attribute

Com_installer

Every update site has a target="_blank" link to the external extension update site. This adds the rel="noopener" attribute to the link - note this is added to joomla.org links as well as its not possible to differentiate.
Every available update has an infourl that has a target="_blank" link to the external extension site. This adds the rel="noopener" attribute to the link.

External links to non Joomla properties

The rel="noopener" attribute has also been added to several hardcoded links within Joomla to non joomla owned properties.

  1. Two Factor Auth Plugin
  2. Debug Plugin
  3. Installation GPL link

External links to Joomla properties

I assumed we are not going to attack our own users sites but to prevent tools like limelight from reporting security errors I have also added the rel=noopener to the links to the joomla.org homepage that are publically visible on the admin login page

@C-Lodder
Copy link
Member

C-Lodder commented Mar 24, 2017

+1, but you need to be using rel="noopener noreferrer" cause Firefox doesn't support noopener

@AlexRed
Copy link
Contributor

AlexRed commented Mar 24, 2017

yes, also TinyMCE and JCE use: target="_blank" rel="noopener noreferrer"


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/14881.

@chriswagner0815
Copy link

Hi Brian,
I agree!

Chris

@brianteeman
Copy link
Contributor Author

brianteeman commented Mar 24, 2017 via email

@brianteeman
Copy link
Contributor Author

brianteeman commented Mar 24, 2017 via email

@brianteeman
Copy link
Contributor Author

Firefox does support it
https://bugzilla.mozilla.org/show_bug.cgi?id=1222516

@chriswagner0815
Copy link

Brian,
what do you mean with the position of nofollow? You have not mentioned this above. In general, website internal links should not be marked as nofollow. External links, according to Google guidelines, should be marked nofollow if it is a sort of paid ad or advertorial (which should therefor not pass pagerank). In gerneral, this is choice of the webmasters in my eyes, we should let them decide and not mark links nofollow by default.

If you were actually asking something else, do let me know! :)
Chris

@brianteeman
Copy link
Contributor Author

@chriswagner0815 Nothing in this PR is for internal links.

If you look at our existing code for the three links in articles you will see that they are hardcoded to nofollow
I changed those links in this pr to nofollow noopener

@C-Lodder said noopener should be changed to noopener noreferrer" cause Firefox doesn't support
noopener (although it does)

So I want to check that you are asking me to change in this case the nofollow noopener to nofollow noopener noreferrer

I only want to do the work once

@brianteeman
Copy link
Contributor Author

Deciding that lnk s which are hardcoded to nofollow should not be is beyond the scope of this pr

@chriswagner0815
Copy link

Hello Brian,
I have asked you on facebook to help me out with the nofollow and why you would include it. I raised with you why I think we should leave this to the users. You told me that everything here is in the ticket and that you will not reply on facebook to elaborate more.

I also do not see speficic links in "three articles" - what specific links and articles are you talking about?

So I am asking here: for what specific reason are you bringing up the nofollow-attribute and why do you want to add it and for what specific reason did you not bring up nofollow later instead of having it included in the 1st place.

Chris

@brianteeman
Copy link
Contributor Author

I also do not see speficic links in "three articles" - what specific links and articles are you talking about?

https://docs.joomla.org/Help36:Content_Article_Manager_Edit#Images_and_Links

So I am asking here: for what specific reason are you bringing up the nofollow-attribute and why do you want to add it and for what specific reason did you not bring up nofollow later instead of having it included in the 1st place.

I am not adding nofollow ever anywhere

@C-Lodder
Copy link
Member

@brianteeman rel="noopener noreferrer" should be this.

Later FF versions may support noopener but Joomla 3.x is supposed to support FF 13 and above.

@C-Lodder
Copy link
Member

C-Lodder commented Mar 24, 2017

@chriswagner0815 this PR fixes a security vulnerability. nofollow has nothing to do with it, so there's no reason for it to be included in this PR.

Feel free to make a separate PR for it.

@brianteeman
Copy link
Contributor Author

brianteeman commented Mar 24, 2017 via email

@C-Lodder
Copy link
Member

The attribute and attribute value order shouldn't matter

@brianteeman
Copy link
Contributor Author

I asked because your first post implied there was an order. Glad we cleared it up and i can finish the pr.

@chriswagner0815
Copy link

Hi Charlie,
I put the nofollow up because Brian worte:
"Tinymce seems to suggest it should be nofollow noopener noreferrer are you ok with that"

I have asked Brian about this in FB and asked for guidance in understanding. You now bring up, that this does not have anything to do with the pr here, Charlie.

I still do not understand the topic at hand but have been asked to share my thoughts. As long as they incorporate my ideas about the nofollow links, everything is fine and we do not have any issue. That is not to say that I now do understand why Brian brought up the incorporation of tinymce's suggestions and why it would be okay to use them.

Chris

@brianteeman
Copy link
Contributor Author

@chriswagner0815 as both Charlie and i have said there is nothing in this pr that changes anything related to do with nofollow. Dont believe me then read the code. If you want to change that then this is not the place.

@brianteeman
Copy link
Contributor Author

@C-Lodder updated as requested - quite correct I forgot all about supporting and protecting users with older browsers.

@C-Lodder
Copy link
Member

I have tested this item ✅ successfully on 0b78de0

thanks Brian


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/14881.

@dgrammatiko
Copy link
Contributor

I have tested this item ✅ successfully on 0b78de0


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/14881.

@dgrammatiko
Copy link
Contributor

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/14881.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Mar 24, 2017
@@ -48,20 +48,20 @@
{
case 1:
// Open in a new window
echo '<a href="' . htmlspecialchars($link, ENT_COMPAT, 'UTF-8') . '" target="_blank" rel="nofollow">' .
echo '<a href="' . htmlspecialchars($link, ENT_COMPAT, 'UTF-8') . '" target="_blank" rel="nofollow noopener noreferrer">' .
Copy link
Member

@C-Lodder C-Lodder Mar 24, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh just notice there are 2 spaces before the rel="...", but can be done in a separate PR as this wasn't your doing

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not anymore

@chriswagner0815
Copy link

Let it be stated that I still do not understand what you write about, Brian, and that I have quoted you as follows:
"Tinymce seems to suggest it should be nofollow noopener noreferrer are you ok with that"

I have requested, that you elaborate why this should be "are you ok with that" and I have not received an explanation. As responsible voice of the JOT / seo team, let it be said that I do not understand code and that, despite I have asked, did not receive and explanation in laymens terms.

I also quote what I wrote before:
"Brian, what do you mean with the position of nofollow? You have not mentioned this above. In general, website internal links should not be marked as nofollow. External links, according to Google guidelines, should be marked nofollow if it is a sort of paid ad or advertorial (which should therefor not pass pagerank). In gerneral, this is choice of the webmasters in my eyes, we should let them decide and not mark links nofollow by default.

If you were actually asking something else, do let me know! :)
Chris"

For that reason, hear my VETO hence I have not understood your mentioning of "nofollow" and your question why anyone should be "okay with that".
Chris

@brianteeman
Copy link
Contributor Author

Sorry I cant help it if you wont read the code. If you did you would realise you are not talking sense at all. Your veto is a joke

@chriswagner0815
Copy link

Hello Brian,
I have asked you to explain to me what you are going to change and why. And I quoted you and asked, why you said (quote again):
"Tinymce seems to suggest it should be nofollow noopener noreferrer are you ok with that"

I also requested your help in understanding the issue multiple times. I am not a coder :) - so I have to rely on what you tell me.

If you decide to not help, then my joke is your joke and then we all suddenly joke and then the joke becomes a conversation - and then I still am not let into it :-(

So again:

  1. What code part are you changing from what to what other?
  2. Why are you mentioning the "nofollow" attribute as quoted above and for what reason are you asking if readers would be okay with that?

Chris :)

@brianteeman
Copy link
Contributor Author

The code change is in the PR - you will have to read it - i am not typing it again here

I already answered the question about nofollow #14881 (comment)

@mbabker
Copy link
Contributor

mbabker commented Mar 24, 2017

Please review the PR, you don't need a coder to read what's on the "Files changed tab". You will find no additions or removals of nofollow in this PR and the explicit question related to the order of the values for the rel attribute where it already existed. That's all.

@brianteeman
Copy link
Contributor Author

Thanks @mbabker

@C-Lodder
Copy link
Member

Ok let me just clear some stuff up here. Firstly, there should be absolutely nothing to discuss about nofollow in this PR. nofollow is used to tell search engines not to follow links.

This PR is completely different. It has nothing to do with search engines. It fixes a security vulnerability that is quite uncommon among web masters, when using target="_blank" or opening a new window/tab using Javascript.

Even though I think there needs to be some form of an automated approach for content, this PR does the basics for 3RD party links in the backend.

So, anything regarding nofollow need to be discussed and submitted a a separate PR.

@chriswagner0815
Copy link

Hi Brian, hi Charlie, hi Michael,
I see a rel nofollow attribute in the code, and I am trying to discuss why this is the case. A rel="nofollow" is here: 6 components/com_content/views/article/tmpl/default_links.php, 2 plugins/fields/url/tmpl/url.php and I would like to know how this looks on a specific Joomla page and under what circumstances this is added.

A rel="nofollow" generally means that Google will FOLLOW this link but not pass pagerank. Passing of pagerank may be what we need. You can read about the pagerank patent online https://www.google.com/patents/US6285999 (essence: page rank of linking page = (page ranking of link receiveing page) -1). This is a method of trust and power flow used in ranking and I just would like to make sure we are considering what we do.

I still do not have answers to the fact (laymen's terms!) where we are doing this and why we are doing this in these cases. Do we, as mentioned above, presume, that the pages are advertorial pages where we should not pass page rank? Is that presumtion one that we, as a project, should make?

Brian: 10 minutes on the phone would have helped me understand this - and I still do not have answers and explanations I need to advise on this topic. I certainly will need your number because now I have about 40 minutes discussion time for the topic and I like to be efficient. Oh, and I will buy you come cigarette packs as well ;)

Charlie: see my remarks about how nofollow is handled - hope you have your knowledge enriched, what you wrote before suggests, it should have been.

Please answer the according questions! Happy to read of you!
Chris

@mbabker
Copy link
Contributor

mbabker commented Mar 24, 2017

Please log that as a separate issue then. Yes, there are existing nofollow uses in the code, but this item neither adds new ones or removes existing ones.

Issues and pull requests should typically be focused on one item. That is an item separate from this change. Happy to discuss the matter, but please let's not get wires crossed on tasks.

@wilsonge wilsonge merged commit 1f89fb0 into joomla:staging Mar 26, 2017
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Mar 26, 2017
@brianteeman brianteeman deleted the noopener branch March 26, 2017 16:36
@darnux
Copy link

darnux commented Jul 14, 2017

Adding noreferrer attribute to links with target=_blank leads to problems with tracking tools like Google Analytics hence backlinks are not shown anymore due to the fact that HTTP_REFERER does not get set.
As far as I read, only noopener is needed for security and performance, and adding noreferrer too was a workaround for Firefox which did not support noopener in the past. This should be fixed since Firefox 52 released on March 7, 2017 (see firefox bug report for details: https://bugzilla.mozilla.org/show_bug.cgi?id=1222516).

My suggestion is to remove noreferrer from wherever added in this PR so tracking tools will be able to see where the vistor came from again.

@C-Lodder
Copy link
Member

C-Lodder commented Jul 15, 2017

@darnux - We're not manipulating any links add by uses, such as those in articles, so tracking isn't an issue.

@darnux
Copy link

darnux commented Jul 17, 2017

@C-Lodder thank you for your reply. What about this?

modules/mod_menu/tmpl/default_url.php

 41 if ($item->browserNav == 1)
 42 {
 43   $attributes['target'] = '_blank';
 44   $attributes['rel'] = 'noopener noreferrer';
 45 }

It causes links in footer which open with a new tab to lose their referrer. That is a problem for affiliate sites or site networks which are using backlinks as a SEO strategy. Would be great if it was configurable in configuration.php.

One example where noreferrer is violating ToS of Amazon Associates Program is here:

https://affiliate-program.amazon.com/help/operating/policies#Associates%20Program%20Participation%20Requirements

In section 8

(v) You will not cloak, hide, spoof, or otherwise obscure the URL of your Site containing Special Links (including by use of Redirecting Links) or the user agent of the application in which Content is displayed or used such that we cannot reasonably determine the site or application from which a customer clicks through such Special Link to the Amazon Site.

@chriswagner0815
Copy link

Hi guys,
I have discussed that with colleagues who are affiliates and using wordpress. The information I got is that for amazon this is not a problem but for affiliate providers like affilinet. People request to put links in with additional plugins to avoid problems with norefferer.
I don't have a clever solution for that but the web is full of people complaining. I have not been able to find information from Amazon themselves.

Does anyone have more information?
Kind regards
Chris

@zero-24 zero-24 added this to the Joomla 3.7.0 milestone Jul 17, 2017
@brianteeman
Copy link
Contributor Author

If it is a specific issue for your site you can create a template override

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants