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

Prevent unsafe links to cross-origin destinations #1846

Merged
merged 1 commit into from Mar 10, 2020

Conversation

@cattywampus
Copy link
Contributor

cattywampus commented Mar 2, 2020

Description

When using target="_blank" to open a link to another page, it's possible for the JavaScript on this newly opened tab to access the the window of the initiating link, allowing content in the dom to be modified to something that could deceive or harm the user. The fix for this is to include the attribute rel="noopener" in the anchor tag alongside target="_blank". This ensures that any new browsing context created by the hyperlink does not have the window.opener browsing context. More details about this issue can be found here:

https://mathiasbynens.github.io/rel-noopener/

According to the link above, support for the noopener value was added to these browser versions:

  • Chrome 49
  • Firefox 55
  • Desktop Safari 10.1
  • iOS Safari 10.3

It's possible this could be implemented in Microsoft Edge, but I was not able to confirm that. In order to support this fix on older versions of the browsers above you have to set the rel attribute to noreferrer. This disables the opener context but also disables the HTTP Referrer header as well which might have an effect on referrer tracking through legitimate links.

This was discovered through a customer security scan of an on-premise Chef Supermarket installation. The recommended solution is to support both noopener and noreferrer, however I think the decision to use the second value is subjective and should be decided by the product manager. So long as customer browsers are relatively up to date (possible big leap?) only supporting noopener for now should be sufficient.

Signed-off-by: Keith Walters kwalters@taphere.com

Related Issue

Issue #1845

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Chore (non-breaking change that does not add functionality or fix an issue)

Checklist:

  • I have read the CONTRIBUTING document.
  • I have run the pre-merge tests locally and they pass.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • All commits have been signed-off for the Developer Certificate of Origin.
@cattywampus

This comment has been minimized.

Copy link
Contributor Author

cattywampus commented Mar 2, 2020

Looks like the travis build failed because bundle exec bundle-audit check --update discovered a vulnerability with the current version of nokogiri and should be upgraded from 1.10.7 to 1.10.9.

Should that upgrade be handled as part of this pull request or opened as a new pr?

@cattywampus

This comment has been minimized.

Copy link
Contributor Author

cattywampus commented Mar 2, 2020

Looks like the travis build errors would be addressed by Issues #1840 and #1841 #1847 and #1848.

@robbkidd robbkidd self-assigned this Mar 2, 2020
When using `target="_blank"` to open a link to another page, it's
possible for the JavaScript on this newly opened tab to access the the
`window` of the initiating link, allowing content in the dom to be
modified to something that could deceive or harm the user. The fix for
this is to include the attribute `rel="noopener"` in the anchor tag
alongside `target="_blank"`. This ensures that any new browsing context
created by the hyperlink does not have the `window.opener` browsing
context. More details about this issue can be found here:

https://mathiasbynens.github.io/rel-noopener/

According to the link above, support for the `noopener` value was added
to these broweser versions:

  * Chrome 49
  * Firefox 55
  * Desktop Safari 10.1
  * iOS Safari 10.3

It's possible this could be implemented in Microsoft Edge, but I was not
able to confirm that. In order to support this fix on older versions of
the browsers above you have to set the rel attribute to `noreferrer`.
This disables the opener context but also disables the HTTP Referrer
header as well which might have an effect on referrer tracking through
legitimate links.

This was discovered through a customer security scan of an on-premise
Chef Supermarket installation. The recommended solution is to support
both `noopener` and `noreferrer`, however I think the decision to use
the second value is subjective and should be decided by the product
manager. So long as customer browsers are relatively up to date
(possible big leap?) only supporting `noopener` for now should be
sufficient.

Signed-off-by: Keith Walters <kwalters@taphere.com>
@cattywampus cattywampus force-pushed the cattywampus:rel_noopener branch from 31a56b5 to 6e4c459 Mar 2, 2020
@robbkidd

This comment has been minimized.

Copy link
Member

robbkidd commented Mar 2, 2020

@cattywampus Hey! Thanks for the PR. The master branch has been updated with newer rake and nokogiri to address the CVE tests. ... And I see you've rebased while I was typing this!

Copy link
Member

robbkidd left a comment

I'm jiggy with noopener over noreferrer to make usage by the browsers that support it more better. It's conceivable that there would be a circling back to add some browser version detection logic that could determine which value to use.

@robbkidd robbkidd merged commit f1c9e2f into chef:master Mar 10, 2020
3 checks passed
3 checks passed
DCO This commit has a DCO Signed-off-by
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
expeditor/config-validation Validated your Expeditor config file
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.