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

[4.0] Serverside enforcement of rel=noopener for all external links #24337

Closed
SniperSister opened this issue Mar 24, 2019 · 31 comments
Closed

[4.0] Serverside enforcement of rel=noopener for all external links #24337

SniperSister opened this issue Mar 24, 2019 · 31 comments

Comments

@SniperSister
Copy link
Contributor

Is your feature request related to a problem? Please describe.

Not having the rel=noopener attribute applied to outgoing links is a potential security issue. So far, Joomla has tackled this by enforcing the attribute in various places like i.e. the WYSWIYG editor config, JHTML::link, layouts etc. - however, links generated outside of those places still depend on the user adding the attribute because it's not enforced by us. A typical example would be an article created without WYSIWYG editor support.

Describe the solution

A potential solution would be a system plugin listening to the onAfterRender event, dynamically adding the rel=noopener attribute after rendering the site.

Additional context

JSST has discussed this internally as a potential issue for 3.x, however considering the rather limitied potential securty impact and associated b/c break, we moved it to the public tracker to be discussed for Joomla 4.

@mbabker
Copy link
Contributor

mbabker commented Mar 24, 2019

Just going to re-iterate that this needs to remain an option and not an arbitrary platform behavior. Also keep in mind that this will have performance implications as it is going to require crawling the entire HTML document and parsing all <a> elements, with the only balance to this being to use the page cache plugin.

@brianteeman
Copy link
Contributor

I am not in favour of ever changing the user generated content.

Adding noopener is not enough - you also have to add noreferer

NOTE: This was rejected as a security issue three years ago by the JSST
NOTE 2: When I introduced the current noopener links there was massive pullback to this change from the SEO team

@SniperSister
Copy link
Contributor Author

NOTE: This was rejected as a security issue three years ago by the JSST

Well, nothing stops us from getting smarter :)

@brianteeman
Copy link
Contributor

Shame I was on the end of so much abuse at the time but glad to see the JSST is finally acknowledging th8is

@mbabker
Copy link
Contributor

mbabker commented Mar 24, 2019

Another thing to consider is this being an all-or-nothing implementation will have unwelcome side effects on some sites. Having this plugin run on joomla.org would walk through the mega menu and footer links and add unwanted rel attributes to those links; even though they are external in the context of the Joomla installation they are not external in the context of building a multi-site network, which means this would introduce an unwelcome SEO impact to our domain. So without making this a configurable thing (which means not using a regex to find all <a> elements in the document but using some tool that can read the DOM, which probably is a bit more performance intensive than a straight regex for everything), it is actually borderline unusable for potentially a large number of sites.

I think it's best this work be left in the content editor and a full on server side solution NOT introduced, but if people are going to adamantly pursue this then it needs to be really well scoped out.

@brianteeman
Copy link
Contributor

So far, Joomla has tackled this by enforcing the attribute in various places like i.e. the WYSWIYG editor config

Where - I dont see that

@SniperSister
Copy link
Contributor Author

which means this would introduce an unwelcome SEO impact to our domain

Maybe I miss something obvious, but how does noopener and/or noreferrer impact SEO?

@mbabker
Copy link
Contributor

mbabker commented Mar 24, 2019

Doesn't noreferrer prevent passing referral data between domains? Meaning referrals from www.joomla.org to developer.joomla.org would not be trackable.

@brianteeman
Copy link
Contributor

Yes that's correct

@brianteeman
Copy link
Contributor

There is only an issue anyway if the link is target blank and iirc Https negates it as well

@SniperSister
Copy link
Contributor Author

Solution: don’t add norefferer and use a referrer-policy header instead, because that’s more flexible

@brianteeman
Copy link
Contributor

That's overkill to address the security concerns as it would apply to all links not just those in a target blank

@SniperSister
Copy link
Contributor Author

Checking google's recommendation, it seems like that noopener actually is enough to kill the security issue - noreferrer is just an additional recommendation on top to improve privacy.

So, just applying noopener should fix the issue without affecting tracking.

@mbabker
Copy link
Contributor

mbabker commented Mar 24, 2019

I still have reservations because of the performance hit but if the scope is contained to only noopener then the plugin doesn't need to be as complex and is less likely to be something people immediately disable.

@brianteeman
Copy link
Contributor

Can't find the link right now but some older browsers don't support noopener

@brianteeman
Copy link
Contributor

@ggppdk
Copy link
Contributor

ggppdk commented Mar 24, 2019

Starting the regular expression with a fixed text greatly reduces the performance impact
in this case it would be <a\s

Also this should be much lighter than the performance impact of emailcloak plugin which is handling more cases ?

@mbabker
Copy link
Contributor

mbabker commented Mar 24, 2019

The mail cloaking code only runs on smaller snippets of HTML (most commonly the com_content article body), though may also run multiple times in a request cycle. Unless someone's got an engineering level genius idea, this would be something that runs on the fully completed HTML document and would inherently process every <a> element (those from user input such as the article body, the links generated by mod_menu, or again using a practical example a large segment of template level HTML such as the mega menu in the joomla.org template).

@brianteeman
Copy link
Contributor

and I repeat it should only run when you have target=_blank

@C-Lodder
Copy link
Member

Note that despite using noreferrer for browsers that don't support noopener, this won't work on IE11 for Windows 7 or earlier versions of 10. I'm not sure what Joomla's policy is on OS versions

@zero-24
Copy link
Contributor

zero-24 commented Mar 26, 2019

Well what does mean "won't work" here? Just nothing happens? Or something breaks? I think the first case can be accepted. When the general issue can be fixed. :)

@C-Lodder
Copy link
Member

C-Lodder commented Mar 26, 2019

@zero-24 I'm not sure as I don't use Win 7 or an older version of Win 10. This will be something for the security team to research

@zero-24
Copy link
Contributor

zero-24 commented Mar 26, 2019

Well when not implemented / interpreted by the browser i expect it to be ignored. But I see nothing we can do against that. By your comment i was thinking about special problems you know about ;)

@ggppdk
Copy link
Contributor

ggppdk commented Mar 26, 2019

question
target="somerandomframename" despite HTML5 deprecated still works in all browsers and will effectively behave as target="_blank" thus it needs noopener ?

@brianteeman
Copy link
Contributor

answer
yes

See a demo of the problem here https://mathiasbynens.github.io/rel-noopener/

@ggppdk
Copy link
Contributor

ggppdk commented Mar 30, 2019

About doing this Content Security Policy

It looks like disown-opener CSP directive was dropped from webappsec-csp
(anyway no browser added support of it)
w3c/webappsec-csp#327

but something new is planned
Adding a new HTTP header called Cross-Origin-Window-Policy

i do not link to it below, so that you try to read it,
just referencing saying that something is planned

whatwg/html#3740

So in future (J5 ?), (yes i know it will not be an option for long time),
we may give websites an alternative of not running this regex replacement,
and instead using the new CSP "related" header

@ghost ghost added the J4 Issue label Apr 4, 2019
@brianteeman
Copy link
Contributor

@SniperSister is this something you will be working on or should it be closed?

@SniperSister
Copy link
Contributor Author

It's on the agenda for 4.x

@brianteeman
Copy link
Contributor

@bembelimen I guess this should get a 4.1 tag of some sort

@brianteeman
Copy link
Contributor

4 years later is there really any point in keeping this open

@SniperSister
Copy link
Contributor Author

Agreed.

We have widespread adoption of the Cross-Origin-Window-Policy header that's configurable in the HTTP header plugin.

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

No branches or pull requests

8 participants