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

Added rel="noopener noreferrer" to all target=_blank #15949

Closed
wants to merge 1 commit into from

Conversation

nyurik
Copy link
Contributor

@nyurik nyurik commented Jan 10, 2018

I'm not sure the rel attribute is needed for these cases, so to review, creating this PR.
This was mentioned to me at #15014 (comment)

@nyurik nyurik requested a review from timroes January 10, 2018 05:42
@nyurik nyurik mentioned this pull request Jan 10, 2018
15 tasks
@timroes
Copy link
Contributor

timroes commented Jan 10, 2018

You will need to update the snapshots for the tests for this to pass, by running node scripts/jest -u.

Also I wonder, whether we could solve this in KuiLink(Button) directly maybe? @cjcenizal do you think we could add rel="noopener noreferrer" to all links, that have target="_blank" automatically?

@cjcenizal
Copy link
Contributor

cjcenizal commented Jan 10, 2018

@timroes Yes I agree that we should do this at the component level, since it's a security concern. @nyurik Would you mind making this change to the relevant components so that all links with target="_bank" also get rel="noopener noreferrer"? And could you add the link I shared as a comment so people who read the code understand the rationale?

@nyurik
Copy link
Contributor Author

nyurik commented Jan 10, 2018

@cjcenizal I'm not very familiar with that code - could you give me a link to where this would be implemented?

@cjcenizal
Copy link
Contributor

@nyurik Sure, it's here: https://github.com/elastic/kibana/blob/master/ui_framework/src/components/button/button.js#L123

We'll need to add the target propType and add a test to verify that the rel attribute is added when its value is _blank. Do you want to pair program on this?

@cjcenizal
Copy link
Contributor

@nyurik I'm about to start work on this. Unless you've already started? I don't want to step on your toes. 😄

@nyurik
Copy link
Contributor Author

nyurik commented Jan 26, 2018

@cjcenizal sorry, got sidetracked with some other projects :( I need to get a bunch of things ready for EON.

@cjcenizal
Copy link
Contributor

No worries! I'll bang this out. Mind if I tag you as a reviewer?

@nyurik
Copy link
Contributor Author

nyurik commented Jan 27, 2018

@cjcenizal I saw your patch, thx! Add me any time :)

@nyurik
Copy link
Contributor Author

nyurik commented Jan 27, 2018

Should I abandon this patch?

@cjcenizal
Copy link
Contributor

@nyurik Yes please close this. I think that because the KUI Framework has been deprecated, we don't need to worry about changing it as I suggested in #15949 (comment). I did a search and it looks like we're not using this component in conjunction with target="_blank".

@nyurik nyurik closed this Feb 1, 2018
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.

3 participants