[wp-trac] [WordPress Trac] #46199: REST: Escape already decoded values when adding query args

[WordPress Trac]

#46199: REST: Escape already decoded values when adding query args
--------------------------+-------------------------------------
 Reporter:  dmsnell       |      Owner:  (none)
     Type:  defect (bug)  |     Status:  new
 Priority:  normal        |  Milestone:  Awaiting Review
Component:  REST API      |    Version:  trunk
 Severity:  normal        |   Keywords:  has-patch needs-testing
  Focuses:                |
--------------------------+-------------------------------------
 `add_query_args()` assumes that when given an array of args as the first
 parameter that they are already `urlencode`ed but in various places we are
 passing `$_GET` (indirectly) as that first parameter; unfortunately
 ([http://php.net/manual/en/reserved.variables.get.php per the PHP docs])
 "//The GET variables are passed through urldecode().//"

 On a site with over two hundred tags a search was made in the Gutenberg
 editor for tags matching `#` - the site used tags like `#cool-thing` and
 `#important` - as part of its autocompleting of tag names. This produced a
 URL with `&search=%23` in the query.

 Inside of `WP_REST_Terms_Controller::get_items()` we call `add_query_args(
 $request->get_query_params(), … )` which by that time passes on `$_GET`
 which contains `$_GET['search'] = '#'` because PHP decoded it.

 Although `add_query_args()` runs //existing// args from its URI through
 `urlencode_deep()` it doesn't modify the args passed into the function
 itself. This leaves us calling `build_query()` which calls
 `_http_build_query()` with the `$urlencode` parameter set to `false`, thus
 the //decoded// value gets directly `implode()`ed into the final query
 string and we're left with a raw `#` //before// any actual fragment part
 of the URL.

 This resulted in the response for the site's tag getting a next link whose
 query args are like this…
 `?per_page=100&orderby=count&order=desc&_fields=id%2Cname&search&page=2#&_envelope=1`
 …when they should be like this…
 `?per_page=100&orderby=count&order=desc&_fields=id%2Cname&search=%23&_envelope=1`

 We can note that it //looks// like we've done something funky moving
 around the value for the `search` arg but actually we truncated at
 `search=#` and then treated everything after it as if it were the
 fragment, finally adding it back in at the end of `add_query_args()`.

 In Gutenberg this resulted in a crash after typing `#` into the tag box
 because the API call came back with invalid data as the `Link` for the
 next page of tag results (and that link was automatically followed by
 Gutenberg's attempt to get all of the results in a paged set). Comically
 the valid data was there but because WordPress stuck `_envelope=1`
 //after// the `#` it was ignored by PHP as not being a `$_GET` arg.

 ----

 To fix this I propose wrapping `$request->get_query_args()` in
 `urldecode_deep()` because we should be able to reliably know that they
 are decoded and thus need re-encoding. When I scanned around I found that
 a few other REST controllers were affected by this same bug and so am
 including corrections there in this patch as well.

 We could approach this with other means and do more to detect if incoming
 args to `add_query_args()` were already encoded or decoded but I don't
 think the additional complexity is worth it.

 We could have always run the array args to `add_query_args()` through
 `urlencode_deep()` but I don't think we can safely assume that it's not
 being called with already-encoded args; I was able to find places where we
 //do already encode them// when we're passing variables that //aren't//
 `$_GET` so I think it's inconceivable to paint broadly here and encode
 everything.

 There may be more places affected by this.

 I searched the codex to see if I could find any ban on including `#` in
 tag names but couldn't find any. Further, I hypothesize that this will
 affect many different API calls which allow user-input search terms.

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/46199>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform