Make WordPress Core

#58251 closed defect (bug) (invalid)

Escaping issue found while echoing attribute's dynamic value in html attribute.

Reported by: madhusudandev's profile madhusudandev Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Toolbar Keywords: has-patch close
Focuses: coding-standards Cc:

Description

In wp-includes/class-wp-admin-bar.php file, I've found that the there is an escaping issue while echoing attribute's dynamic value in html attribute (like class). The issue is found at line 458 of that file. I think it should be escaped.

I've seen the attribute's dynamic value were escaped in the other lines of that file. Link mentioned below:

https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/class-wp-admin-bar.php#L487
https://github.com/WordPress/wordpress-develop/blob/trunk/src/wp-includes/class-wp-admin-bar.php#L514

Attachments (1)

58251-docs.diff (639 bytes) - added by sabernhardt 11 months ago.
Adding a note about formatting to the add_node docblock

Download all attachments as: .zip

Change History (27)

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


13 months ago
#1

  • Keywords has-patch added

Fixed escaping issue in wp-includes/class-wp-admin-bar.php
Trac ticket: https://core.trac.wordpress.org/ticket/58251

#2 @nazmulhudadev
13 months ago

#58252 was marked as a duplicate.

#3 @audrasjb
13 months ago

  • Component changed from Administration to Toolbar
  • Version 6.2 deleted

Hello, welcome to Trac and thanks for the ticket,

Line 458 is pretty different than the two other ones.

  • L487: in _render_container(), the variable $node is passed as parameter of the function and comes from something than can be filtered by plugins or various snippets.
  • L514: same goes for the related $node variable.
  • L458: $class doesn't comes from the function and is purely static, non-filterable.

#5 @dilipbheda
13 months ago

@audrasjb I've applied your suggestion, please review my changes and let me know if need any changes.

#6 follow-up: @sabernhardt
13 months ago

In the _render() function, the $class variable on line 458 stands for either nojq nojs or nojq nojs mobile. Neither string would require escaping.

If the ID values are trimmed where they are used in an id attribute, these functions should only trim the variable:

esc_attr( 'wp-admin-bar-' . trim( (string) $node->id ) )

#7 @dilipbheda
13 months ago

@sabernhardt Thank you for the review, I've applied your suggestion. Please check the attached PR https://github.com/WordPress/wordpress-develop/pull/4427

#8 in reply to: ↑ 6 ; follow-up: @audrasjb
13 months ago

Replying to sabernhardt:

In the _render() function, the $class variable on line 458 stands for either nojq nojs or nojq nojs mobile. Neither string would require escaping.

That's what I meant in comment:3. I don't see any value in escaping something that cannot be altered by third parties :)

@SergeyBiryukov commented on PR #4427:


13 months ago
#9

Thanks for the PR! Unless I'm missing something, I don't think casting $node->id to a string is needed here, it is documented as a string in all of the WP_Admin_Bar class methods and should not ever be anything else, so casting it to a string in this particular place would be confusing.

@dilipbheda commented on PR #4427:


13 months ago
#10

@SergeyBiryukov Thank you for the review, I've removed casting and now $node->id is only trimmed

#11 in reply to: ↑ 8 ; follow-up: @SergeyBiryukov
13 months ago

Replying to audrasjb:

Replying to sabernhardt:

In the _render() function, the $class variable on line 458 stands for either nojq nojs or nojq nojs mobile. Neither string would require escaping.

That's what I meant in comment:3. I don't see any value in escaping something that cannot be altered by third parties :)

That used to be my thinking too, but @peterwilsoncc raised a good point in comment:7:ticket:57133 that even though the values do not require escaping currently, this may change in the future so it would be a good idea to escape anything that is a variable. I've been following that approach lately.

#12 @audrasjb
13 months ago

Wow, I missed that info Sergey. It sounds a bit like a rabbit hole to me (what about text strings and so on?) but ok, why not 🙂

#13 follow-up: @audrasjb
13 months ago

@SergeyBiryukov I think it would be nice to discuss and clarify this during a devchat and to make it a clear guideline on the Core Handbook, then!

#14 in reply to: ↑ 13 @SergeyBiryukov
13 months ago

Replying to audrasjb:

I think it would be nice to discuss and clarify this during a devchat and to make it a clear guideline on the Core Handbook, then!

Indeed, that would be great :)

#15 @nazmulhudadev
13 months ago

Hi @audrasjb and @SergeyBiryukov,

Thanks to both of you for helpful comments.

I also think same as @SergeyBiryukov, that it is a good idea to escape anything that is a variable. I follow that approach and that is why I've added the suggestion to escape the variable $class.

I appreciate the insights and suggestions, and I'm open to further feedback to improve my code.

#16 @costdev
13 months ago

I agree that discussing this during devchat would be great! 🙂

#17 in reply to: ↑ 11 ; follow-up: @azaozz
13 months ago

  • Keywords close added

Replying to SergeyBiryukov:

That used to be my thinking too, but @peterwilsoncc raised a good point in comment:7:ticket:57133 that even though the values do not require escaping currently, this may change in the future so it would be a good idea to escape anything that is a variable.

Frankly I tend to disagree. Why would you want to run a function that will consume some resources (no matter how small) and do absolutely nothing? Wouldn't it be better to add a comment or two to where the variable is hard-coded?

Also, I kind of understand the hesitation to trust future developers but changing a variable that is set to a static string should always trigger a review of where and how that variable is used, right? :)

So it is a -1 from me for this type of pointless use of functions. A comment would work just as well if not better.

#18 in reply to: ↑ 17 @SergeyBiryukov
13 months ago

Replying to azaozz:

changing a variable that is set to a static string should always trigger a review of where and how that variable is used, right? :)

Fair enough :)

#19 @sabernhardt
13 months ago

#28117 suggests changes to the $class variable output, though it might never have custom classes.

#20 @sabernhardt
13 months ago

  • Keywords close removed
  • Milestone changed from Awaiting Review to 6.3

Even if no function calls are added, this will still need an update to the documentation.

For the id attributes, trim() may not be very helpful on its own. Plugins are probably more likely to add a space in the middle of the string than at either end. I would consider having spaces doing it wrong, but the add_node() documentation does not (yet) explain any character restrictions for the ID string.

I found two rather popular plugins that could benefit from sanitize_title( trim( $node->id ) ). Both use the add_menu() alias of add_node(), one using a translatable string (which they already replaced on GitHub) and the other starting with the hash character. Checking dozens of other plugin search results, I did not find any additional plugins following either the translatable string pattern or the hash character pattern for a toolbar link.

(Update: both those plugins mentioned have fixed their IDs.)

Last edited 11 months ago by sabernhardt (previous) (diff)

#21 @oglekler
11 months ago

  • Keywords changes-requested added

This ticket was mentioned in Slack in #core by sabernhardt. View the logs.


11 months ago

#23 follow-up: @hellofromTonya
11 months ago

  • Keywords close added

I agree with @azaozz. Escaping is not needed in this instance.

Why not escape?
There's no security risk within its value at the time it's echoed out.

Why no security risk?
The $class variable:

  1. is set to a static string, meaning there's no data being passed it from other sources such as other code, database, or user input.
  2. is contained within a function, i.e. has function scope, making it not accessible outside of the _render() method.

I can appreciate why @madhusudandev requested the escaping change (hello and thank you). But in this case, there's no security gain by adding the esc_. Instead, the change would increase processing time and could cause confusion for contributors who see that it is a static string value.

What about an inline comment to explain why it's not escaped?
Would this help with understanding the code? If yes, then I'd suggest opening a separate Trac ticket to globally apply the inline commit to all static value instances. Rather than adding 1 comment for this 1 instance, instead add the comment to all instances.

I'm marking this ticket as a close candidate.

Last edited 11 months ago by hellofromTonya (previous) (diff)

@sabernhardt
11 months ago

Adding a note about formatting to the add_node docblock

#24 @sabernhardt
11 months ago

  • Keywords changes-requested removed

I made a patch for a docblock change, which probably can be improved.

#25 @gaambo
10 months ago

Just wanted to add, that we've gotten feedback form the plugin review team for one of our plugins, that every variable (even with hardcoded contents) should be escaped. Here's the quote:

At this time, we ask you escape all $-variables, options, and any sort of generated data when it is being echoed. That means you should not be escaping when you build a variable, but when you output it at the end. We call this 'escaping late.'
Besides protecting yourself from a possible XSS vulnerability, escaping late makes sure that you're keeping the future you safe. While today your code may be only outputted hardcoded content, that may not be true in the future. By taking the time to properly escape when you echo, you prevent a mistake in the future from becoming a critical security issue.

I think the same rules should apply to core as well as plugins.

#26 in reply to: ↑ 23 @azaozz
10 months ago

  • Milestone 6.3 deleted
  • Resolution set to invalid
  • Status changed from new to closed

Replying to hellofromTonya:

Escaping is not needed in this instance.
...
I'm marking this ticket as a close candidate.

Sounds good.

@gaambo

every variable (even with hardcoded contents) should be escaped

I don't see explicit mention that hard-coded strings should be escaped. But you're right, the quoted text seems to suggests that.

Unfortunately I don't see the reason why hard-coded strings need to be escaped or "pre-processed" in any way if they meet the standards for the intended use. In this case the $class is hard-coded to nojq nojs and mobile may be appended. The syntax of the $class strings meets the specific requirements for its intended use and there is no chance for it to be changed to anything else or to stop meeting these requirements.

Note: See TracTickets for help on using tickets.