$wp_query->parse_orderby() allows incorrect keys to go through(edge case)

[nikolov.tmw]

This is similar to #24674, in the sense that it's related to the quirks of non-strict in_array() checks.

Here's what I think is needed to reproduce the problem:

• Use a $table_prefix that starts with a number(my case was 1..._)
• Do a query that uses multiple meta queries

This results in the $allowed_keys(in query.php, the parse_orderby() function) looking like this:

array(24) {
  ...
  [18]=>
  string(6) "_price"
  [19]=>
  string(10) "meta_value"
  [20]=>
  string(14) "meta_value_num"
  [21]=>
  string(16) "1..._postmeta"
  [22]=>
  string(3) "mt1"
  [23]=>
  int(1)
}

I do have to mention that this was triggered by (in my opinion) incorrect usage by WooCommerce - they are setting the orderby clause to "meta_value_num {$wpdb->posts}.ID", which would go through(because it starts with 1 and item 23 in the array is int(1)) and then gets escaped to post_1..._postsid, which in turn would break the query, since there is no such column.

However, despite the incorrect orderby parameter, I think that we should use strict checking for this in_array() call in order to prevent keys that are not really valid to go through.

In case you were wondering where the int(1) in the end comes from, it's one of the keys of the $meta_clauses array - they get merged into the $allowed_keys array.

[boonebgorges]

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 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?

[boonebgorges]

In 34090:

Use stricter sanitization for meta query clause keys.

By forcing all clause keys to be strings, we make it possible to use strict
comparison when validating values of 'orderby' as passed to WP_Query. This
eliminates situations where the presence of numeric clause keys could result
in an improperly validated 'orderby' value.

Props nikolov.tmw.
Fixes #32937.