Escape later when getting post and body classes

[mfields]

Both get_body_class() and get_post_class() provide filters that allow plugins and themes to add custom values to the list. These filters are applied after the values in the $classes array have been filtered through esc_attr(). I think that it would be best to move the escaping after the filter has fired.

esc_attr() was first added to get_body_class() and get_post_class() in [11838]

[ocean90]

Makes sense.

[mfields]

Escape even later.

[mfields]

Looking at this again, it may be better to escape directly in body_class() and post_class() as shown in 20009.2.diff

[scribu]

Moving the escaping into body_class() isn't so good, because theme authors might use get_body_class() directly, without bothering to escape it's output. So 20009.diff​ looks good to me.

[mfields]

@scribu - Thanks for the input here. Both solutions make sense to me for different reasons. I agree that 20009.diff is the better of the two.

[azaozz]

What exactly are we escaping here? Values added by plugins? Don't think escaping is really needed on class names added from trusted source, keep in mind that the HTML class attribute allows the whole UTF-8 charset to be used with very little restrictions.

[obenland]

Replying to azaozz:

What exactly are we escaping here? Values added by plugins? Don't think escaping is really needed on class names added from trusted source, keep in mind that the HTML class attribute allows the whole UTF-8 charset to be used with very little restrictions.

Pre-patch, escaping happened before plugins filtered the output. So essentially class names added from a trusted source (core) are being escaped, while the ones added through the filter are not.

mfields' patch swaps the order of function calls so that all class names are being escaped.

[dd32]

I can tell that there's going to be a plugin out there that's doing something funky here.. For example:

add_filter( 'body_class', function( $classes ) {
  $classes[] = '" anotherattribute="123"';
  return $classes;
} );

I agree that we should be escaping it though, and as long as esc_attr( esc_attr() ) doesn't cause any major issues, I think both patches should be applied.

[obenland]

Once the entire output of get_body_class() is filtered we don't really need to filter it again in body_class(), do we? It uses get_body_class() directly, without providing an opportunity to alter the escaped classes.

[nacin]

Replying to dd32:

I can tell that there's going to be a plugin out there that's doing something funky here..

There is a "fun" workaround for this that MarkJaquith came up with a while ago to actually echo an attribute from the body class filter. But I also pretty much guarantee this will break someone's code and probably not in a pretty way. I get the idea of escaping wherever possible, but we can't safeguard against every possible misuse of a filter (in this case the misuse I am referring to is returning an unsanitized class name), so we should make sure we are absolutely sure we want to do this.

and as long as esc_attr( esc_attr() ) doesn't cause any major issues

Indeed it does not — it doesn't double-escape.

[nacin]

Still thinking close here.

[SergeyBiryukov]

#29259 was marked as a duplicate.

[obenland]

Replying to nacin:

But I also pretty much guarantee this will break someone's code and probably not in a pretty way.

I agree. Let's not do that.

[jrf]

I'd like to ask for reconsideration of this issue, most particularly patch 20009.2.diff. While the other patch might break existing plugin filters, patch 20009.2.diff will not and will provide a better safeguard for users.

[dd32]

I'm not against 20009.2.diff​..

[jdgrimes]

Replying to jrf:

I'd like to ask for reconsideration of this issue, most particularly patch 20009.2.diff. While the other patch might break existing plugin filters, patch 20009.2.diff will not and will provide a better safeguard for users.

That patch could still break stuff as pointed out above. However, anything that would break is already quite fragile anyway, since it is relying on placing the attribute as the very last element in the array. If it isn't the last element, the whole thing falls apart. This can be achieved by using a very late filter priority, but there really is no guarantee. It is meant to break. ;-)

[whyisjake]

Patch refreshed, would love some testing.

/cc @garyj

[whyisjake]

Related: ​https://github.com/WordPress/WordPress-Coding-Standards/pull/758

[whyisjake]

In 48060:

Themes: Add additional later escaping to post_class() and body_class() functions.

Additionally, this adds a few tests to test output.

Fixes #20009.

Props mfields, scribu, azaozz, obenland, dd32, nacin, jrf, jdgrimes, garyj, whyisjake.