Make WordPress Core

Opened 12 years ago

Closed 4 years ago

#20009 closed defect (bug) (fixed)

Escape later when getting post and body classes

Reported by: mfields's profile mfields Owned by: whyisjake's profile whyisjake
Milestone: 5.5 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 (4)

20009.diff (789 bytes) - added by mfields 12 years ago.
20009.2.diff (820 bytes) - added by mfields 12 years ago.
Escape even later.
20009.3.diff (842 bytes) - added by whyisjake 4 years ago.
20009.4.diff (3.3 KB) - added by whyisjake 4 years ago.

Download all attachments as: .zip

Change History (25)

@mfields
12 years ago

#1 @ocean90
12 years ago

Makes sense.

@mfields
12 years ago

Escape even later.

#2 @mfields
12 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
12 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
12 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
12 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
11 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
11 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
11 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
11 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
11 years ago

  • Keywords close added; dev-feedback removed

Still thinking close here.

#11 @nacin
10 years ago

  • Milestone changed from 3.7 to Awaiting Review

#12 @SergeyBiryukov
10 years ago

#29259 was marked as a duplicate.

#13 in reply to: ↑ 9 @obenland
9 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
7 years 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
7 years ago

  • Milestone set to Awaiting Review

I'm not against 20009.2.diff..

#16 in reply to: ↑ 14 @jdgrimes
7 years 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
7 years ago

  • Keywords needs-testing added

#18 @whyisjake
4 years ago

  • Milestone changed from Awaiting Review to 5.5
  • Owner set to whyisjake
  • Status changed from reopened to accepted

@whyisjake
4 years ago

#19 @whyisjake
4 years ago

Patch refreshed, would love some testing.

/cc @garyj

@whyisjake
4 years ago

#21 @whyisjake
4 years ago

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

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.

Note: See TracTickets for help on using tickets.