Make WordPress Core

Opened 17 months ago

Closed 17 months ago

Last modified 11 months ago

#57133 closed defect (bug) (fixed)

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

Reported by: monzuralam's profile monzuralam Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.2 Priority: normal
Severity: normal Version: 6.2
Component: Users Keywords: has-patch 2nd-opinion
Focuses: administration, coding-standards Cc:

Description

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

Thanks.

Change History (13)

This ticket was mentioned in PR #3641 on WordPress/wordpress-develop by @monzuralam.


17 months ago
#1

  • Keywords has-patch added; needs-patch removed

Trac ticket: [57133]

@rudlinkon commented on PR #3641:


17 months ago
#2

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

#3 @hztyfoon
17 months ago

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

#4 follow-up: @SergeyBiryukov
17 months ago

  • Keywords close 2nd-opinion added

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.

#5 @hztyfoon
17 months ago

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

#6 in reply to: ↑ 4 ; follow-up: @rudlinkon
17 months ago

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?

#7 follow-up: @peterwilsoncc
17 months ago

  • Keywords close removed

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>

#8 in reply to: ↑ 7 @rudlinkon
17 months ago

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.

#9 in reply to: ↑ 6 ; follow-up: @SergeyBiryukov
17 months ago

  • Milestone changed from Awaiting Review to 6.2
  • Owner set to SergeyBiryukov
  • Status changed from new to accepted

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.

#10 in reply to: ↑ 9 @rudlinkon
17 months ago

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.

#11 @SergeyBiryukov
17 months ago

  • Resolution set to fixed
  • Status changed from accepted to closed

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.

@SergeyBiryukov commented on PR #3641:


17 months ago
#12

Thanks for the PR! Merged in r54857.

This ticket was mentioned in PR #4451 on WordPress/wordpress-develop by @nazmulhudadev.


11 months ago
#13

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

Note: See TracTickets for help on using tickets.