Open external links using rel="noopener" #293
Conversation
|
@jonschlinkert Do you think it is possible to merge and create a new release for this PR that is about performance and security? Please note I tried to run tests but there are missing dependencies and the |
There was a problem hiding this comment.
There's a typo, but other than that, I think this might be okay. The one question I have, is how would someone that expects to be able to use window.opener for an internal link be able to do that? This might need to be handled with an option so users have more control over when noopener is set.
lib/rules.js
Outdated
| @@ -157,7 +157,8 @@ rules.paragraph_close = function(tokens, idx /*, options, env */) { | |||
| rules.link_open = function(tokens, idx, options /* env */) { | |||
| var title = tokens[idx].title ? (' title="' + escapeHtml(replaceEntities(tokens[idx].title)) + '"') : ''; | |||
| var target = options.linkTarget ? (' target="' + options.linkTarget + '"') : ''; | |||
| return '<a href="' + escapeHtml(tokens[idx].href) + '"' + title + target + '>'; | |||
| var rel = options.linkTarget && options.linkTarget !== '_self' ? (' ref="noopener"') : ''; | |||
There was a problem hiding this comment.
This should be rel="noopener".
For performance and security reasons, this patch opens external links using `rel="noopener"` when target is defined and other than `_self`. More details about the reason for this patch are available below: https://developers.google.com/web/tools/lighthouse/audits/noopener
|
@doowb Thanks, the typo is fixed.
In that case, people can set the existing The fact that we do not have fine grain control over link attributes per link seems to be another problem that is not even yet addressed for the |
|
@lpellegr thanks! I'll leave this open for a few days to see if anyone else has any comments or suggestions about this change. |
|
I'll merge this and try to get it published this weekend. I think some other things have been merged since the last published version so I might have to backport the changes if this should be a patch bump. |
|
I didn't forget about this. There are some things that need to be done with the repository before this can be merged. |
|
|
|
Is there any update on this PR? |
|
FYI: since Remarkable doesn't support this, we've started using DomPurify to handle attaching Here's the plugin we wrote: https://github.com/swagger-api/swagger-ui/blob/59bd9f4988007a0e561a62831f444787b84f6f2c/src/core/components/providers/markdown.jsx#L7-L16 If you're handling user generated Markdown, you should be sanitizing your output anyway! DomPurify is by far the best sanitization library that I've come across 😄 |
|
@doowb any news? |
|
@doowb any news on this? 😸 |
For performance and security reasons, this patch opens external links using
rel="noopener"when target is defined and other than_self.More details about the reason for this patch are available below:
https://developers.google.com/web/tools/lighthouse/audits/noopener