[wp-trac] [WordPress Trac] #32937: $wp_query->parse_orderby() allows incorrect keys to go through(edge case)

[WordPress Trac]

#32937: $wp_query->parse_orderby() allows incorrect keys to go through(edge case)
--------------------------+------------------
 Reporter:  nikolov.tmw   |       Owner:
     Type:  defect (bug)  |      Status:  new
 Priority:  normal        |   Milestone:  4.4
Component:  Query         |     Version:
 Severity:  normal        |  Resolution:
 Keywords:                |     Focuses:
--------------------------+------------------
Changes (by boonebgorges):

 * milestone:  Awaiting Review => 4.4


Comment:

 Thanks for the report, nikolov.tmw. It is kind of a doozy.

 A strict `in_array()` check seems OK only if we can be confident that the
 `orderby` keys would always be strings. So I was a bit disconcerted to see
 that your sample array had `int(1)` at the end of it.

 After digging into it a bit, I see that there's a somewhat sloppy `!
 $clause_key` check in `get_sql_for_clauses()` that's causing this to
 happen. In a nutshell: The clauses of a meta query each get a unique name
 (or `$clause_key`) that can be used as the value of `'orderby'` in
 `WP_Query`, etc. The `if ( ! $clause_key )` check is meant to check for an
 empty string, but it ends up catching the clause key `0` and converting it
 to `mt1`. When the ability to reference a meta query clause by key was
 introduced (#31045), I meant to eliminate the possibility of clause keys
 being integers, and the accidental conversion of `0` just described fooled
 me into thinking I'd succeeded :) But once you get to `1`, the integer key
 seeps through.

 In [attachment:32937.diff] I'm suggesting a two-pronged fix. First, use
 strict mode for the `in_array()` check in `parse_orderby()`, as you
 suggest. Second, don't allow integer clause keys in meta queries. In a
 sense, the latter fix means that the `$allowed_keys` array will always be
 strings, which makes the strict `in_array()` check strictly unnecessary
 (no pun intended). But it seems reasonable to add it anyway.

 Can you take a look and see if the fix makes sense to you?

--
Ticket URL: <https://core.trac.wordpress.org/ticket/32937#comment:1>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform