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

Always add rel="noopener noreferrer" to link with target="_blank" #147

Closed
markkap opened this Issue Oct 16, 2018 · 4 comments

Comments

Projects
None yet
1 participant
@markkap
Copy link

markkap commented Oct 16, 2018

WordPress core added a solution for it as part of https://core.trac.wordpress.org/ticket/43187 but it is done the wrong way around, changing the input instead of changing the output.

Proper solution should look for all target="_blank" on the page just before output and modify them. This should happen both in front end and admin context.

@markkap markkap added the security label Oct 16, 2018

@markkap markkap added this to the 1.0.0 milestone Oct 16, 2018

@markkap

This comment has been minimized.

Copy link
Author

markkap commented Dec 12, 2018

The original solution was reverted out of 5.0, but this should be addressed.

@markkap

This comment has been minimized.

Copy link
Author

markkap commented Dec 12, 2018

Based on this article https://developers.google.com/web/tools/lighthouse/audits/noopener google audit will be happy when detecting either noopener or norefferer, and as noopener is the standard oriented way to avoid the related security issue, will just implement it (people that use old unsecure browsers.... well time to upgrade if they care about their security. As this is an edge case security problem which is hard to exploit, not going to complicate and bloat the code for them )

One thing we are going to do differently from the google check and more in line with how links inserted in the editor work, is to add noopener to all links that open new tabs/windows regardless of their destination as it is going to be hard to know in a multisite setting which urls are safe and which not.

(This actually should be ideal and easier to implement in JS, but since we will want compliance with google's security audit tool, it has to be done at the server side)

@markkap

This comment has been minimized.

Copy link
Author

markkap commented Mar 5, 2019

Inherited the implementation from 5.1. It seems like it is missing the functionality for the text and HTML widgets https://core.trac.wordpress.org/ticket/46421.

markkap added a commit that referenced this issue Mar 8, 2019

Remove the filter handling as it is not needed since we check for bla…
…nk target on the full HTML when it is generated #147

markkap added a commit that referenced this issue Mar 8, 2019

markkap added a commit that referenced this issue Mar 8, 2019

markkap added a commit that referenced this issue Mar 8, 2019

@markkap

This comment has been minimized.

Copy link
Author

markkap commented Mar 9, 2019

Implementation details:

  • Fully reject the partial WordPress approach, partially because it does not cover all content and old content, and you should never manipulate the content which a user added without his consent as long as you have other options. Will still use the WordPress code handling detection and manipulation of the rel attribute.
  • There are 3 contexts in which the transformation is carried out, front end HTML generation, admin pages, and plugin readme type content served from wordpress.org.
  • Non HTML content is not manipulated, which means RSS feed, AJAX responses, rest-api responses, and XML-RPC serve the raw content without manipulation. It is up to the client to fix the issue if it applies to the context in which it operates.
  • Implementation prefers the ease of understanding of the code over 100% completeness. As it is now links that are injected at the shutdown handler are not going to be checked. The assumption here is that content added at shutdown is an extreme edge case. It is also technically harder to write code that will under all conditions executed last.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.