WordPress.org

Make WordPress Core

Opened 6 years ago

Last modified 10 months ago

#20009 reopened defect (bug)

Escape later when getting post and body classes

Reported by: mfields Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: Themes Keywords: has-patch needs-testing
Focuses: Cc:

Description

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]

Attachments (2)

20009.diff (789 bytes) - added by mfields 6 years ago.
20009.2.diff (820 bytes) - added by mfields 6 years ago.
Escape even later.

Download all attachments as: .zip

Change History (19)

@mfields
6 years ago

#1 @ocean90
6 years ago

Makes sense.

@mfields
6 years ago

Escape even later.

#2 @mfields
6 years ago

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

#3 @scribu
6 years ago

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.

#4 @mfields
6 years ago

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

#5 follow-up: @azaozz
6 years ago

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.

#6 in reply to: ↑ 5 @obenland
4 years ago

  • Milestone changed from Awaiting Review to 3.7

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.

#7 follow-up: @dd32
4 years ago

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.

#8 @obenland
4 years ago

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.

#9 in reply to: ↑ 7 ; follow-up: @nacin
4 years ago

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.

#10 @nacin
4 years ago

  • Keywords close added; dev-feedback removed

Still thinking close here.

#11 @nacin
4 years ago

  • Milestone changed from 3.7 to Awaiting Review

#12 @SergeyBiryukov
3 years ago

#29259 was marked as a duplicate.

#13 in reply to: ↑ 9 @obenland
3 years ago

  • Keywords close removed
  • Milestone Awaiting Review deleted
  • Resolution set to wontfix
  • Status changed from new to closed

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.

#14 follow-up: @jrf
11 months ago

  • Resolution wontfix deleted
  • Status changed from closed to reopened

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.

#15 @dd32
11 months ago

  • Milestone set to Awaiting Review

I'm not against 20009.2.diff..

#16 in reply to: ↑ 14 @jdgrimes
11 months ago

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

#17 @swissspidy
10 months ago

  • Keywords needs-testing added
Note: See TracTickets for help on using tickets.