Add rel="noopener noreferrer" wherever target="_blank" is used #7680

bluefuton · 2016-08-25T20:38:54Z

To address the target="_blank" issue outlined here:

https://dev.to/ben/the-targetblank-vulnerability-by-example

...this PR adds rel="noopener noreferrer" to every external link using target="_blank" in the Calypso codebase.

We've also added an eslint rule (Automattic/eslint-config-wpcalypso#2) to flag any links using target="_blank" without the corresponding rel attribute.

cc @aduth

Test live: https://calypso.live/?branch=fix/target-blank-vuln

aduth · 2016-08-25T22:12:00Z

client/blocks/reader-author-link/index.jsx

@@ -29,7 +29,7 @@ const ReaderAuthorLink = ( { author, post, siteUrl, children } ) => {
 	}

 	return (
-		<ExternalLink className="reader-author-link" href={ linkUrl } target="_blank" onClick={ recordAuthorClick }>
+		<ExternalLink className="reader-author-link" href={ linkUrl } target="_blank" rel="noopener noreferrer" onClick={ recordAuthorClick }>


The implementation of <ExternalLink /> is such that rel will be overridden and remove the noopener noreferrer:

https://github.com/Automattic/wp-calypso/blob/dcf8306/client/components/external-link/index.jsx#L41

To that end, if the purpose of external link is for linking externally, should we just add these values to the component itself?

I had the same thought, but I'm not sure our new lint rule (Automattic/eslint-config-wpcalypso#2) is smart enough to figure that out :) Might mean peppering the code with eslint exception comments?

As I suspected, the eslint rule is a bit of a blunt instrument. If we specify target="_blank" on an ExternalLink, the rule complains even though we are setting rel in the underlying component.

I've worked around the handful of cases with

eslint-disable react/jsx-no-target-blank

for the moment. Another way around this would be to introduce a new prop on ExternalLink (openInNewWindow or similar) that applies target inside the component.

bluefuton · 2016-08-26T05:19:15Z

Todo:

apply rel in EmptyContent (components/empty-content) when target is specified
apply rel in PurchaseDetail when target is specified
specify rel in ExternalLink when target is specified
clean up EmptyContent and ExternalLink with a now-redundant rel
run linter and check warnings/exceptions

bluefuton · 2016-08-30T20:06:59Z

Rebased and ready for 👀

aduth · 2016-08-30T20:34:11Z

client/components/button/index.jsx

-		borderless: PropTypes.bool
+		borderless: PropTypes.bool,
+		target: PropTypes.string,
+		rel: PropTypes.string


Might be argued that these be included in the else block in render to push into omitProps for the button case, since they're not valid attributes for a button. This assumes someone renders as a button with rel and target but not href, which is unlikely, be "possible".

Good call. Added in b88562b.

bluefuton · 2016-09-01T11:41:58Z

Does this look good to go @aduth? Keeping on top of the conflicts is getting tricky as it touches so many files.

I'll remove the eslint-disable-line statements later if your change to the rule gets merged (jsx-eslint/eslint-plugin-react#793).

aduth · 2016-09-01T12:54:52Z

client/components/external-link/index.jsx

@@ -41,6 +42,10 @@ export default React.createClass( {
 			rel: 'external'
 		} );

+		if ( props.target ) {
+			props.rel = props.rel.concat( ' noopener noreferrer' );


TIL String#concat. Then again, this could have just been props.rel += ' noopener noreferrer';

No need to change anything.

aduth · 2016-09-01T12:59:19Z

LGTM 👍

You may want to rebase once more locally and run npm run lint just to make sure that no need target props have crept in since the last rebase.

bluefuton · 2016-09-01T13:46:57Z

Thanks @aduth! I've just given it a final rebase and lint. 🚢

bluefuton added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. Security labels Aug 25, 2016

bluefuton self-assigned this Aug 25, 2016

aduth mentioned this pull request Aug 25, 2016

Stats: add rel=noopener to stats list actions. #7682

Merged

aduth reviewed Aug 25, 2016
View reviewed changes

aduth added [Status] Needs Author Reply and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Aug 25, 2016

bluefuton force-pushed the fix/target-blank-vuln branch 2 times, most recently from bad0bf4 to 3a740b3 Compare August 30, 2016 11:11

bluefuton added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Author Reply labels Aug 30, 2016

aduth mentioned this pull request Aug 30, 2016

jsx-no-target-blank: Target only anchor elements jsx-eslint/eslint-plugin-react#793

Merged

bluefuton force-pushed the fix/target-blank-vuln branch from 519772f to 08aedfd Compare August 30, 2016 20:06

aduth reviewed Aug 30, 2016
View reviewed changes

bluefuton added [Status] Needs Author Reply and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Aug 31, 2016

bluefuton force-pushed the fix/target-blank-vuln branch from 8848da9 to 1f9fca3 Compare August 31, 2016 10:20

bluefuton added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Author Reply labels Aug 31, 2016

tyxla mentioned this pull request Aug 31, 2016

Me: ES6ify the Profile Links components #7773

Merged

bluefuton force-pushed the fix/target-blank-vuln branch from b51c6e2 to 05522ce Compare September 1, 2016 11:34

aduth reviewed Sep 1, 2016
View reviewed changes

aduth added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Sep 1, 2016

bluefuton added 13 commits September 1, 2016 14:42

Add rel="noopener noreferrer" wherever target="_blank" is used

1dbaeb2

Fix Polldaddy test for post normalizer

723efdf

Clean up mistakely renamed attributes

fec2764

Pass rel attribute to purchase button

efd6fda

Apply rel="noopener noreferrer" in EmptyContent if there's a target set

e995945

ExternalLink: add noopener noreferrer to rel if target is specified

37d2345

Remove redundant rel attributes

2a0299a

Fix linting errors

6b04348

Remove react/jsx-no-target-blank override

5ebfef5

Restore @timmyc refactoring of stats-list/action-link

158a719

Linebreak fixup

9a0c6cc

Button component: omit target and rel for <button>

d8f5162

Add eslint-disable for ExternalLink example

c7c5163

bluefuton force-pushed the fix/target-blank-vuln branch from 05522ce to c7c5163 Compare September 1, 2016 13:42

bluefuton merged commit 6394a9c into master Sep 1, 2016

bluefuton deleted the fix/target-blank-vuln branch September 1, 2016 13:47

matticbot removed the [Status] Ready to Merge label Sep 1, 2016

bluefuton mentioned this pull request Sep 2, 2016

Fix ignore-click rule for links with more than one value for rel="" #7835

Merged

tyxla mentioned this pull request Oct 14, 2016

Jetpack Plugins: Open links in plugin content in a new window #8744

Merged

aduth mentioned this pull request Oct 24, 2016

Site Settings: Show Site Icon setting even when feature disabled #8924

Merged

tyxla mentioned this pull request Dec 14, 2016

Add InfoLink component #10036

Closed

vindl mentioned this pull request Mar 10, 2017

Plugins: Add Action Links to Plugin Detail View #11718

Merged

roundhill mentioned this pull request Mar 21, 2017

Posts: Comment count doesn't update when adding a comment #11791

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add rel="noopener noreferrer" wherever target="_blank" is used #7680

Add rel="noopener noreferrer" wherever target="_blank" is used #7680

bluefuton commented Aug 25, 2016 •

edited by aduth

Loading

aduth Aug 25, 2016

bluefuton Aug 26, 2016

bluefuton Aug 30, 2016

bluefuton commented Aug 26, 2016 •

edited

Loading

bluefuton commented Aug 30, 2016

aduth Aug 30, 2016

bluefuton Aug 31, 2016

bluefuton commented Sep 1, 2016

aduth Sep 1, 2016

aduth commented Sep 1, 2016

bluefuton commented Sep 1, 2016

Add rel="noopener noreferrer" wherever target="_blank" is used #7680

Add rel="noopener noreferrer" wherever target="_blank" is used #7680

Conversation

bluefuton commented Aug 25, 2016 • edited by aduth Loading

aduth Aug 25, 2016

Choose a reason for hiding this comment

bluefuton Aug 26, 2016

Choose a reason for hiding this comment

bluefuton Aug 30, 2016

Choose a reason for hiding this comment

bluefuton commented Aug 26, 2016 • edited Loading

bluefuton commented Aug 30, 2016

aduth Aug 30, 2016

Choose a reason for hiding this comment

bluefuton Aug 31, 2016

Choose a reason for hiding this comment

bluefuton commented Sep 1, 2016

aduth Sep 1, 2016

Choose a reason for hiding this comment

aduth commented Sep 1, 2016

bluefuton commented Sep 1, 2016

bluefuton commented Aug 25, 2016 •

edited by aduth

Loading

bluefuton commented Aug 26, 2016 •

edited

Loading