Make WordPress Core

Opened 8 years ago

Closed 4 years ago

#40401 closed defect (bug) (fixed)

Value of data-colname in wp-list-table is not escaped

Reported by: rellect's profile rellect Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 5.8 Priority: normal
Severity: normal Version: 4.3
Component: Administration Keywords: has-patch commit
Focuses: Cc:

Description

It looks like at some point the esc_attr() was removed in favor of wp_strip_all_tags
wp-admin/includes/class-wp-list-table.php

<?php
// Comments column uses HTML in the display name with screen reader text.
// Instead of using esc_attr(), we strip tags to get closer to a user-friendly string.
$data = 'data-colname="' . wp_strip_all_tags( $column_display_name ) . '"';

But wp_strip_all_tags does not escape the value, so wp_strip_all_tags should've been added as addition to esc_attr, and not as a replacement.

Attachments (2)

40401.patch (1.2 KB) - added by rellect 8 years ago.
40401.2.diff (1.3 KB) - added by audrasjb 4 years ago.
Patch refresh

Download all attachments as: .zip

Change History (14)

@rellect
8 years ago

#1 @rellect
8 years ago

  • Keywords has-patch added
  • Resolution set to invalid
  • Status changed from new to closed

#2 @rellect
8 years ago

  • Resolution invalid deleted
  • Status changed from closed to reopened

#3 @rellect
8 years ago

Sorry, accidently clicked on the 'resolve as invalid' checkbox

#4 @SergeyBiryukov
8 years ago

  • Version changed from 4.7.3 to 4.3

Related: [33016]

#5 @rellect
7 years ago

Still an issue with current wordpress 4.9.8

#6 @Hareesh Pillai
4 years ago

  • Keywords needs-refresh added; has-patch removed
  • Milestone changed from Awaiting Review to 5.8

Patch needs a refresh against trunk.

@audrasjb
4 years ago

Patch refresh

#7 @audrasjb
4 years ago

  • Keywords has-patch commit added; needs-refresh removed

Patch refreshed. I think it's good to go.

#8 follow-up: @Hareesh Pillai
4 years ago

Thanks for the refresh, @audrasjb.

I notice a few other instances of wp_strip_all_tags() used without escaping. In the class-wp-screen file, for instance.
Should we handle those cases as well?

#9 @SergeyBiryukov
4 years ago

  • Owner set to SergeyBiryukov
  • Status changed from reopened to reviewing

#10 in reply to: ↑ 8 @audrasjb
4 years ago

Replying to Hareesh Pillai:

Should we handle those cases as well?

Let's commit that one for now, we can still open another ticket to address the other potential references :)

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


4 years ago

#12 @whyisjake
4 years ago

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

In 51115:

Administration: Escape the values of data-colname.

Adds a esc_attr wrapper to strip_all_tags.

See [33016].

Fixes #40401.

Props rellect, SergeyBiryukov, hareesh-pillai, audrasjb.

Note: See TracTickets for help on using tickets.