Request headers sent incorrectly from `WP_Http` to `Requests`

[flixos90]

While having a closer look at Requests (and also the way it is used in WP), I noticed something that appears to be a bug.

The $headers variable that is passed from WP_Http::request() to Requests::request() is an array of $key => $value pairs where $value may be an array itself in case multiple values for that header have been passed to WP_Http::request(). However, the Requests library expects each $value of the $headers array to always be a string, as it uses sprintf() with it directly (the passed $headers array is sent through Requests::flatten()).

This can cause issues when specifying multiple headers of the same name.

[flixos90]

39687.diff​ is a patch with what I think needs to be done here to pass the $headers to Requests correctly.

[dd32]

We've never really supported passing multiple header values as an array when explicitly using array syntax, and it's definately never been supported when passing a multiple-header string (which is a edgecase we support).

For comparison, Here's the code in 4.4 which definately didnt' support it:
https://core.trac.wordpress.org/browser/branches/4.4/src/wp-includes/class-wp-http-curl.php?marks=195-199#L192
https://core.trac.wordpress.org/browser/branches/4.4/src/wp-includes/class-wp-http-streams.php?marks=191-196#L188

WP_Http::processHeaders() was originally intended to be used for return-value purposes, returning duplicate headers from the server to the caller in an array format instead of the standard comma-separated format. We could change that, but I fear others may be using the method in plugins/etc and expecting that format.

Looking at 39687.diff​ I think I'm willing to suggest wontfix here, or perhaps just an extra parameter to be added..

If you want to pass multiple header values, pass an array with a properly formatted value, don't send a string with multiple headers ie. pass [ 'Header' => 'Value1,Value2' ] rather than Header: Value1\nHeader:Value2

@rmccue thoughts?

[flixos90]

@dd32 Thanks for providing some background on this, now I see where this code comes from.
I like the idea that we could add a third parameter to WP_Http::processHeaders(), a flag called $is_request. Then based on this flag, if we're verifying request headers, the code from 39687.diff​ should be applied, otherwise the code as it is now should be applied.
We would be backward-compatible here, and at the same time allow passing headers like [ 'Header: Value1', 'Header: Value2' ].

In either way, I think we need to clarify the docs for the headers argument of wp_remote_request() accordingly.

I would tend to use needs-refresh. :)