Make WordPress Core

#56701 closed defect (bug) (wontfix)

Sanitize HTML Classes added to single row columns in WP_List_Table

Reported by: bananastalktome's profile bananastalktome Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: General Keywords: has-patch needs-testing 2nd-opinion close
Focuses: administration Cc:

Description

Currently, class names added to each rows columns in WP_List_Table in single_row_columns are not sanitized, and as such can break HTML output. For example, adding a filter to include a new column on the Sites page of a Network install:

<?php
add_filter('manage_sites-network_columns', function($columns) {
  $columns["'><script>alert('Hello!')</script>"] = 'Hello?';
  return $columns;
});

does, in fact, output a script tag which is evaluated for each row being shown.

I don't think this is just an issue for the Network Sites page, I think any pages including list table classes extending WP_List_Table are impacted.

Attached (will be) a patch that uses sanitize_html_class on the $column_name.

Attachments (2)

56701.diff (696 bytes) - added by bananastalktome 17 months ago.
56701-1.diff (1.4 KB) - added by bananastalktome 17 months ago.

Download all attachments as: .zip

Change History (12)

#1 @bananastalktome
17 months ago

Including a refresh since I noticed the column headings are also affected.

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


16 months ago

#3 @desrosj
16 months ago

  • Focuses administration added
  • Milestone changed from Awaiting Review to 6.1.1
  • Version 6.1 deleted

Thanks @bananastalktome.

Version is used to note the first version of WordPress affected by an issue. That's going to be quite a bit earlier than 6.1 for this, so I've reset that value.

Moving to 6.1.1 for further investigation.

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


16 months ago

#5 @JeffPaul
16 months ago

  • Keywords needs-testing added

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


16 months ago

#7 follow-up: @Clorith
16 months ago

Thank you for the patch @bananastalktome, I had a look at it, and the thought behind it is good, I do have some suggestions though.

Normally, what we want to do is sanitize anything that is saved, and escape when outputting it, so in this case, we should instead of relying on sanitize functions, use an escaping function either in the echo portion of the code, or as close as we can get. Since classes are attributes, using esc_attr() will probably be the approach you want, so instead of introducing a new variable, on line 1338 the class attribute that will be echoed is generated, one could then replace the existing implementation:

$class = "class='" . implode( ' ', $class ) . "'";

With one that escapes, such as:

$class = "class='" . esc_attr( implode( ' ', $class ) ) . "'";

#8 in reply to: ↑ 7 @azaozz
16 months ago

  • Keywords 2nd-opinion added

Replying to Clorith:

Normally, what we want to do is sanitize anything that is saved, and escape when outputting it

Right. This applies for strings that are typed by a user. So the question here is: can a user add HTML classnames there? That doesn't seem possible in core. Seems only plugins can, and the classname(s) are most likely not saved in the DB (so they can be sanitized on saving), but hard-coded in the plugin.

Agree with @costdev's comment on Slack that this is similar to #56655. In both cases the strings can only come from trusted source (plugins and themes) and are likely hard-coded. No point to sanitize them (if there is malicious code/intent, it can do a lot more harm in many other places).

in this case, we should instead of relying on sanitize functions, use an escaping function either in the echo portion of the code...

Frankly I'm not even sure that escaping is needed here. There is no point to escape hard-coded classnames, right? The only difference here seems to be to "catch" plugins that misuse the filter(s) to break out of the current tag and add arbitrarily HTML (if there are any). In that case a _doing_it_wrong() would probably be better?

Last edited 16 months ago by azaozz (previous) (diff)

#9 @peterwilsoncc
16 months ago

  • Keywords close added

I'm inclined to close this without a fix for similar reasons to #56655.

If a plugin wishes to allow a user to add arbitory classes using the filter, the plugin is responsible for filtering.

As Ozz mentions, once something takes PHP to exploit (for want of a better word) it's not really a concern as there are many other developer APIs available they can do far nastier things with.

#10 @JeffPaul
16 months ago

  • Milestone 6.1.1 deleted
  • Resolution set to wontfix
  • Status changed from new to closed

I concur, closing as wontfix.

Note: See TracTickets for help on using tickets.