Make WordPress Core

Opened 7 years ago

Last modified 7 years ago

#39687 new defect (bug)

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

Reported by: flixos90's profile flixos90 Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version:
Component: HTTP API Keywords: has-patch 2nd-opinion
Focuses: Cc:

Description

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.

Attachments (1)

39687.diff (536 bytes) - added by flixos90 7 years ago.

Download all attachments as: .zip

Change History (4)

@flixos90
7 years ago

#1 @flixos90
7 years ago

  • Keywords has-patch added

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

#2 @dd32
7 years ago

  • Keywords 2nd-opinion added

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?

#3 @flixos90
7 years ago

@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. :)

Note: See TracTickets for help on using tickets.