Sanitize HTML Classes added to single row columns in WP_List_Table

[bananastalktome]

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.

[bananastalktome]

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

[desrosj]

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.

[Clorith]

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

[azaozz]

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?

[peterwilsoncc]

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.

[JeffPaul]

I concur, closing as wontfix.