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

[madhusudandev]

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

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

[nazmulhudadev]

#58252 was marked as a duplicate.

[audrasjb]

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.

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

[dilipbheda]

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

[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.

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 ) )

[dilipbheda]

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

[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 :)

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.

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

[SergeyBiryukov]

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.

[audrasjb]

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 🙂

[audrasjb]

@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!

[SergeyBiryukov]

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 :)

[nazmulhudadev]

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.

[costdev]

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

[azaozz]

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.

[SergeyBiryukov]

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 :)

[sabernhardt]

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

[sabernhardt]

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.)

[hellofromTonya]

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.

[sabernhardt]

Adding a note about formatting to the add_node docblock

[sabernhardt]

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

[gaambo]

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.

[azaozz]

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.