$type should be escaping in 'user-new.php'

[monzuralam]

File path: src/wp-admin/user-new.php
Here esc_attr() escaping function is missing ( Line: 446 ).

Thanks.

Trac ticket: [57133]

Thanks to your PR you can escape the $label variable also.

[hztyfoon]

Welcome back @monzuralam 🤗
Your PR looks good. I've also added a comment in the PR. can you check please?

[SergeyBiryukov]

Hi there, welcome back to WordPress Trac! Thanks for the ticket and the PR.

I don't see why these values should be escaped:

• $type is a hardcoded value​ of either email or text, not a user-supplied value, so escaping is redundant.
• $label is a translatable string. Core translations are considered safe because we have a review process for them, see #42639 and the discussion in #30724. (Also related: #32233, #44637.)

In WordPress core and older bundled themes, strings are generally only escaped in attributes or in <option> tags. However, this was recently reconsidered for newer bundled themes, see comment:5:ticket:54127.

Some other related tickets: #47384, #47385, #49535, #49536, #49537, #56110.

[hztyfoon]

Thanks @SergeyBiryukov ,
+1, That totally makes sense to me.

[rudlinkon]

Replying to SergeyBiryukov:

Hi there, welcome back to WordPress Trac! Thanks for the ticket and the PR.

I don't see why these values should be escaped:

• $type is a hardcoded value​ of either email or text, not a user-supplied value, so escaping is redundant.
• $label is a translatable string. Core translations are considered safe because we have a review process for them, see #42639 and the discussion in #30724. (Also related: #32233, #44637.)

In WordPress core and older bundled themes, strings are generally only escaped in attributes or in <option> tags. However, this was recently reconsidered for newer bundled themes, see comment:5:ticket:54127.

Some other related tickets: #47384, #47385, #49535, #49536, #49537, #56110.

Thanks, @SergeyBiryukov for clarifying but why here [51820] did the string being escaped?

[peterwilsoncc]

My inclination is to add the escaping as a defensive coding measure.

While the variables are set to values that do not require escaping currently, this may change in the future so it would be good to protect core contributors from their future selves.

My personal approach is to escape anything that is a variable:

<?php
// Needs escaping
$attr = __( 'Attribute value' );
?>
<div data-attr="<?php echo esc_attr( $attr ); ?>"></div>
<?php

// Does not need escaping
?>
<div data-attr="<?php echo __( 'Attribute value' ); ?>"></div>

[rudlinkon]

Replying to peterwilsoncc:

My inclination is to add the escaping as a defensive coding measure.

While the variables are set to values that do not require escaping currently, this may change in the future so it would be good to protect core contributors from their future selves.

My personal approach is to escape anything that is a variable:

<?php
// Needs escaping
$attr = __( 'Attribute value' );
?>
<div data-attr="<?php echo esc_attr( $attr ); ?>"></div>
<?php

// Does not need escaping
?>
<div data-attr="<?php echo __( 'Attribute value' ); ?>"></div>

Thank you @peterwilsoncc for your valuable comment. Actually, I'm following the rules which you mentioned.

[SergeyBiryukov]

Replying to rudlinkon:

but why here [51820] did the string being escaped?

As noted above, this was recently reconsidered for newer bundled themes, see comment:5:ticket:54127. This commit was done on that ticket.

Replying to peterwilsoncc:

My inclination is to add the escaping as a defensive coding measure.

While the variables are set to values that do not require escaping currently, this may change in the future so it would be good to protect core contributors from their future selves.

My personal approach is to escape anything that is a variable

Thanks! This does seem like a sensible approach and resolves the second thoughts I had after my comment.

[rudlinkon]

Replying to SergeyBiryukov:

As noted above, this was recently reconsidered for newer bundled themes, see comment:5:ticket:54127. This commit was done on that ticket.

Thank you got it.

Thanks! This does seem like a sensible approach and resolves the second thoughts I had after my comment.

So it may be accepted.

[SergeyBiryukov]

In 54857:

Users: Add missing escaping on the Add New User screen.

While the $type and $label variables are set to values that do not currently require escaping, this may change in the future, so it is preferable to add the escaping as a defensive coding measure.

Follow-up to [16294], [29030].

Props monzuralam, rudlinkon, hztyfoon, peterwilsoncc.
Fixes #57133.

Thanks for the PR! Merged in r54857.

Hi there! Thanks for contributing to WordPress!

Yes, I think so that it should be escaped.
Here is a similar things I found at https://core.trac.wordpress.org/ticket/57133#comment:7

Trac ticket: https://core.trac.wordpress.org/ticket/58303

This ticket was closed as wontfix.