Late escape site blocks #37880
Late escape site blocks #37880
Conversation
getdave
added
[Type] Code Quality
| @@ -40,7 +40,7 @@ function render_block_core_site_logo( $attributes ) { | |||
| // Add the link target after the rel="home". | |||
| // Add an aria-label for informing that the page opens in a new tab. | |||
| $aria_label = 'aria-label="' . esc_attr__( '(Home link, opens in a new tab)' ) . '"'; | |||
| $custom_logo = str_replace( 'rel="home"', 'rel="home" target="' . $attributes['linkTarget'] . '"' . $aria_label, $custom_logo ); | |||
| $custom_logo = str_replace( 'rel="home"', 'rel="home" target="' . esc_attr( $attributes['linkTarget'] ) . '"' . $aria_label, $custom_logo ); | |||
There was a problem hiding this comment.
This is under a '_blank' === $attributes['linkTarget'] check, so we already know what the value is, and it's hardcoded without any user-input involved. Escaping this string is not necessary, it can just be _blank
There was a problem hiding this comment.
IMHO, if data is coming from a variable I believe we should escape at point of output. Who's to say what the variable might contain in future. It's hardcoded for now but that's no guarantee it will stay that way.
This is minor overhead for the sake of long term peace of mind and resilience against XSS.
There was a problem hiding this comment.
I agree... But why not just make it target="_blank"? It can't be anything else. That way we make it easier to read, and at the same time it's a performance micro-optimization 🤔
There was a problem hiding this comment.
Let me look again. If we can hard code inline when we build the markup then let's do that,
There was a problem hiding this comment.
Ok I've looked again. Yes this block of code is behind the following conditional:
if ( $attributes['isLink'] && '_blank' === $attributes['linkTarget'] ) {
Whilst I recognise this is how it is now, that could easily change in the future.
What if there's a refactor and the $custom_logo markup ends up outside this conditional? What if the developer forgets to escape the linkTarget as part of the refactor?
I appreciate this is a "what if" scenario but given the low overhead (how many site logo blocks are going to be on one page?) I believe it is better to be safe than sorry when it comes to XSS. I believe it's a good trade off.
There was a problem hiding this comment.
What's the next step here? Is the addition of this esc_attr() a blocker? If it absolutely is I will revert the change.
|
I do not know how to do what you are saying in regards to package/block. I
noted a few months back with git hun I was no developer when I started to
notice major changes in my stuff and asked for help: who can o bring this
to? I’ll go in person. So this person stops getting all my personal
information and I can stop this it is so overwhelming
…On Fri, Jan 14, 2565 BE at 7:58 AM Dave Smith ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In packages/block-library/src/site-logo/index.php
<#37880 (comment)>:
> @@ -40,7 +40,7 @@ function render_block_core_site_logo( $attributes ) {
// Add the link target after the rel="home".
// Add an aria-label for informing that the page opens in a new tab.
$aria_label = 'aria-label="' . esc_attr__( '(Home link, opens in a new tab)' ) . '"';
- $custom_logo = str_replace( 'rel="home"', 'rel="home" target="' . $attributes['linkTarget'] . '"' . $aria_label, $custom_logo );
+ $custom_logo = str_replace( 'rel="home"', 'rel="home" target="' . esc_attr( $attributes['linkTarget'] ) . '"' . $aria_label, $custom_logo );
What's the next step here? Is the addition of this esc_attr() a blocker?
If it absolutely is I will revert the change.
—
Reply to this email directly, view it on GitHub
<#37880 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AU6ET4SMGNRZCUG7D5MZDRLUWBI4LANCNFSM5LWQOWTA>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
|
@Rockstar907 As you mention, this is a Github issue and not specifically related to this repository. I recommend taking a look at their support article on notifications to learn how to avoid getting notified. |
Description
This is not a security problem. This PR simply moves escaping of all PHP output to be as "late" as possible. This means we avoid escaping variables until they are output in the HTML markup.
This is a WP Core best practice.
How has this been tested?
Screenshots
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).