Make WordPress Core

Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#18875 closed defect (bug) (fixed)

CSS classes for WP List Table are not properly sanitized

Reported by: sbressler Owned by: scottbre
Milestone: 3.3 Priority: normal
Severity: normal Version: 3.1
Component: Administration Keywords: has-patch
Focuses: Cc:


The output of a WP List Table uses the singular and plural names of the table (provided as arguments to the constructor) as the CSS classes output for the table and tbody. Those names aren't sanitized at all. The biggest concern I have is that spaces aren't turned into dashes, though the lack of sanitization is bad practice in general.

Attachments (1)

18875.1.diff (537 bytes) - added by sbressler 9 years ago.
Patch that sanitizes the title of the constructor args

Download all attachments as: .zip

Change History (9)

9 years ago

Patch that sanitizes the title of the constructor args

#1 @sbressler
9 years ago

Attached (18875.1.diff) is a patch that fixes this issue. I decided to address the issue in the constructor rather than before the classes were output so that we can address any other uses of $args that might be added later to the file.

While $args['singular'] might be empty, there is no harm to running it through sanitize_title as well. However, that line could easily be wrapped in if ( $args['singular'] ) if performance is a concern.

Last edited 9 years ago by sbressler (previous) (diff)

#2 @nacin
9 years ago

Can we switch this out for sanitize_html_class()?

What could this break? What are the current inputs for singular/plural?

#3 @nacin
9 years ago

  • Milestone changed from Awaiting Review to 3.3

#4 @sbressler
9 years ago

sanitize_html_class doesn't change spaces into dashes, which sanitize_title does. While we can leave that to be the responsibility of users of WP_List_Table, I think using dashes for replacements makes more sense.

I need to check all the users of WP_List_Table to see if this would affect any of them. My guess is that it won't, but I'll verify later today.

#5 @nacin
9 years ago

I'm not entirely familiar with how these are used, but it doesn't look like they're just used strictly as classes. Perhaps sanitize_key() would be the best choice, and it wouldn't break anything in core (best I can tell - these are tough to search for).

#6 @sbressler
9 years ago

Finally got around to investigating this. $_args['plural'] is used as a CSS class (in the base class as well as the plugins list class) and as a nonce. $_args['singular'] is used only as a CSS class (in the base class). In all cases, I think converting spaces to dashes make the most sense. As such, I think sanitize_title makes the most sense.

However, if performance is a significant concern with using sanitize_title(), using sanitize_html_class() or sanitize_key() would be fine.

#7 @nacin
9 years ago

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

In [19020]:

Sanitize plural and singular args for list tables. props sbressler, fixes #18875.

#8 @nacin
9 years ago

If nonces or CSS classes break, there's no true damage. A plugin would A) need to be instantiating a list table with some weird non-key values, and B) trying to target said weirdness with CSS, for sanitize_key() to break something. I'd rather go with this than a sledgehammer.

Note: See TracTickets for help on using tickets.