Make WordPress Core

Opened 7 years ago

Closed 7 years ago

Last modified 5 years ago

#39315 closed defect (bug) (fixed)

WP_Tax_Query::transform_query() double escapes name term arguments

Reported by: bcworkz's profile bcworkz Owned by: boonebgorges's profile boonebgorges
Milestone: 4.8 Priority: normal
Severity: normal Version: 4.7
Component: Query Keywords: has-patch needs-testing
Focuses: Cc:

Description

As reported in Support Forums by @expert1

When making a new WP_Query that includes a "tax_query" argument involving a term name that has characters requiring escaping, such as apostrophes, WP_Tax_Query::transform_query() double escapes the term name so that the subsequent query always fails. The resulting SQL string includes WHERE 0=1 due to the double escaping.

The term is first escaped by wp_filter_kses() which is hooked into the "edit_term_name" filter of sanitize_term_field() which is called on line 626 of wp-includes/class-wp-tax-query.php. On return the term is escaped again with esc_sql() on the same line.

I'm not sure if it makes more sense to remove the esc_sql() call or to hook in wp_kses() instead of wp_filter_kses() for the "edit_term_name" filter. If the latter, we need to run the term through stripslashes() before passing it to the filter callbacks. I'm unsure how that may affect other filter callbacks. (sanitize_text_field() and _wp_specialchars() by default)

Attachments (1)

39315.diff (1.7 KB) - added by boonebgorges 7 years ago.

Download all attachments as: .zip

Change History (5)

#1 @boonebgorges
7 years ago

  • Keywords has-patch needs-testing added
  • Milestone changed from Awaiting Review to 4.8

@bcworkz Thanks for the ticket and for the diagnosis.

The principle in previous cases has been to apply the same sanitization at the time of *query* as what's applied at the time of *insertion*. So what we're trying to match here is https://core.trac.wordpress.org/browser/tags/4.7/src/wp-includes/taxonomy.php?marks=1992,1995#L1989 - sanitize_term_field() + wp_unslash(). The case is very similar to #35493.

In an ideal world, the sanitization-juggling would be less insane, but I think that 39315.diff is our best alternative in the actual world. What do you think?

@boonebgorges
7 years ago

#2 @bcworkz
7 years ago

@boonebgorges - Thanks for working up a solution!

It seems a bit round-a-bout, but I understand the desire for consistency. Since it solves the problem, it works for me :)

#3 @boonebgorges
7 years ago

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

In 39662:

Don't double-escape terms payload in WP_Tax_Query::transform_query().

terms values are passed through sanitize_term_field() with the 'db'
flag, which add slashes. Because terms are subsequently run through
esc_sql(), these slashes must be removed. See [36348], which added
a similar step to sanitization in get_terms().

Props bcworkz.
Fixes #39315.

#4 @boonebgorges
5 years ago

#37276 was marked as a duplicate.

Note: See TracTickets for help on using tickets.