-
Notifications
You must be signed in to change notification settings - Fork 471
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
#1229: Replace rel=“author"
with rel="noopener noreferrer"
in footer
#1243
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for fixing this @nielslange!
woocommerce.com
is a domain owned by Automattic, so I think we can trust it. But at the same time, I don't see any drawback on settingrel="noopener noreferrer"
, so that part LGTM.- 💯 for the change in the string
Built with...
. - About the
rel="author"
removal: while it looks like Google stopped using it for their search ranks, it doesn't seem to be completely deprecated so I initially thought that we should keep it. But at the same time, I don't think Storefront can be considered theauthor
of all sites done with WooCommerce/Storefront. So it looks good to me to remove it. 👍
tl;dr I think this PR can be merged as-is. @haszari & @senadir thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree - this looks like a good change, thanks @shirokoweb @nielslange for raising & addressing this.
I looked up noreferrer
/ noopener
(see also MDN) to better understand the security implications. Looks like we could catch things like this by using Lighthouse or other validator tools when testing.
Based on the Google docs, I think we should include either noreferrer
/ noopener
- not both. I don't think this is a major blocker but it's worth tidying up now IMO. noopener
seems like the right fit for our case; I don't see an issue with including referrer header from a site running storefront => woocommerce.com.
IMO, if you want to keep only one attribute instead of both, it should be noreferrer. rel="noreferrer" attribute has the same effect as "noopener", but also prevents the Referer header from being sent to the new page. Although this header has many innocent uses it can have undesirable consequences for user security and privacy. See Referer header: privacy and security concerns for more information and mitigations. I'm not sure, logged in customers would be happy to potentially have some data sent to anybody, especially under GDPR. |
Fair point @shirokoweb, and that's the most conservative (safest) option. I'm happy for us to go with |
@nielslange can you review the suggested changes and commit to your branch? Then we can get this merged and wrapped up for 2.5.4. Thank you :) cc @Aljullu |
Co-Authored-By: Rua Haszard <haszari@cartoonbeats.com>
Co-Authored-By: Rua Haszard <haszari@cartoonbeats.com>
@haszari I've committed the requested changes so that this PR can be merged. |
Fixes #1229
Replaces
rel=“author"
withrel="noopener noreferrer"
instorefront/inc/storefront-template-functions.php
Line 140 in 5ab42c7