GET - Too many redirects

[Workshopshed]

The class-http.php incorrectly reports too many redirects when checking a redirected request using CURL.

In function "request" the check on line 1113 only looks for HEAD requests but this would also be valid for GET request with a redirect. The code for handling redirection is later in the file.
Either the message on line 1118 should be changed or the sequenced changed to process redirects correctly.

// If no response, and It's not a HEAD request with valid headers returned
		if ( 0 == strlen($theResponse) && ('HEAD' != $args['method'] || empty($this->headers)) ) {

[Workshopshed]

dif with one possible fix for this issue

[kawauso]

Is

if ( $r['redirection']-- > 0 )

equivalent to

if ( $r['redirection'] > 1 )

and if so, can we use the latter for readability's sake?

Also there's a typo in the WP_Error error string.

[dd32]

Alright, so the problem here is, that when running under Safe Mode you're finding redirection's aren't followed? What specific request is causing it?

That was previously covered in #11305.

The code block you're patching, should only be hit when curl_exec() returns a failure, which in general terms, it shouldn't get that far, as it should've returned the headers in $theResponse, so it wouldn't get to that block..

Can you give some more details of the problem that you're seeing? The full request being used (I'm assuming it's in custom code?) as well as the PHP details for cURL in your install?

[dd32]

That too many redirects was added in [9091]. If cURL's internal Redirection-follow has been used (In that case, $r['redirections'] will be at the innitial requested value) the final redirection returns the headers from the request, not an error condition, so the response check passses, skipping the code in that spot.

[Workshopshed]

kawauso, thanks for reviewing the DIFF. The double minus is used intentionally to decrement a counter and hence stop the procedure recursing infinately. The count starts at 5 and decreases to zero. The same error message occurrs multiple times in the file so to locate which instance was causing the problem I added a number to the end. It was not my intention to include that in the diff so well spotted.

dd32: Additional information as requested.

Yes, the code is in a custom plugin.
It was working ok till around November 2011 when something changed and some of my feeds stopped being read.
I tracked that down to being those with a redirected feed.

Settings:

Safe Mode: Off

Curl: On, Version 7.15.5

Test Code is very simple, as all I am doing is calling fetch_feed, if you check what is returned you will see the error

$rss = fetch_feed('http://blog.makezine.com/feed/');

The values that end up in the particular function are

$theResponse - string length = 0
$args['method'] - GET
Curl exec error - no
CURLINFO_HTTP_CODE - is either 301 or 302
$r['redirection'] - 5

[dd32]

(Note for simplification: safe_mode references below apply to safe_mode OR it's little brother, open_basedir, which has the same effect upon cURL for no good reason - I believe you'll have open_basedir in use)

Thanks for the extra info, I can duplicate it with safe_mode enabled, as in that cause, cURL doesn't follow redirections at all, causing $theResponse to be empty.

The line you've tracked it to above is correct, There's a logic error there, or it's not taking into account safe_mode in the mix somehow.

Some quick testing between safe mode enabled, disabled, and GET/HEAD requests hasn't lead me to stumble over the right logical option to use here though. The proposed patch allows for significantly more redirections than anticipated on non-safe_mode environments, It shouldn't be entering that if block at all under ideal situations. meaning either the conditions need to be altered, or the block further down the function related to #11305 needs to be brought up.

While I'm looking at the method in question.. $args is used directly a few times where $r (the parsed args) should be used.

[Workshopshed]

Yes, open_basedir is set. Was wondering if this check could be changed to simply check for a curl error and to leave the later check to handle the redirection.

[dd32]

Was wondering if this check could be changed to simply check for a curl error and to leave the later check to handle the redirection.

That unfortunately wouldn't catch non-safe_mode curl reaching the max redirects and returning the appropriate error.

It has been suggested elsewhere in another ticket that perhaps we can simplify it all by not using curl's auto-redirection, and instead, just do it ourselves. This will work around this difference in implementation between safe_mode and non-safe_mode. (I don't see the ticket right now)

[Workshopshed]

Explicitly set an "autoredirect" flag

[Workshopshed]

Dion, based on your comments, I came up with the attached version 2. I can't see any documentation on what happens when the curl is using CURLOPT_FOLLOWLOCATION and exceeds it's maximum redirects so I've assumed that the existing check is valid. To simplify the code I added a variable $autoredirect and then use that to control the latest checks so we should have the correct behaviour for safe and nonsafe modes.

I see from the other threads that you have some kind of unit test to put this through, and a url for testing redirects so if you could let me now how that goes.

cheers,

Andy

[nacin]

[20207]

[dd32]

There's a patch on #17490 which deals with moving redirects from curl to WP, which I'm testing right now.. reducing the number of different flows through each transport makes it easier to test and more reliable (since it's less likely to be a bug from a certain configuration)

[dd32]

In [20208]:

WP_HTTP: Curl: Handle Redirections in PHP rather than internally in Curl, Simplifies code flow between safe_mode On and Off, and works around certain bugs. Props simonwheatley for initial patch. Fixes #20219, Fixes #17490

[SergeyBiryukov]

Seems that [20208] has some side effects.

While doing a fresh install for #13839, I've noticed that secret keys in wp-config.php were empty.

$secret_keys = wp_remote_get( 'https://api.wordpress.org/secret-key/1.1/salt/' ) returned:

Array
(
    [headers] => Array()
    [body] => 
    [response] => Array(
            [code] => 0
            [message] => 
    )
    [cookies] => Array()
    [filename] => 
)

Turned out that before [20208] wp_remote_get() returned a cURL error:

SSL certificate problem, verify that the CA cert is OK. Details:
error:14090086:SSL routines:SSL3_GET_SERVER_CERTIFICATE:certificate verify failed

Perhaps SSL on my local WAMP is misconfigured, but I guess that can happen on some hosts too.

Since [20208], the error is ignored and an empty response is returned instead, so
setup-config.php doesn't fall back to wp_generate_password().

$theHeaders value in this case is:

Array(
    [response] => Array(
            [code] => 0
            [message] => 
    )
    [headers] => Array()
    [cookies] => Array()
)

Seems that empty( $theHeaders ) would always be false.

Shouldn't it be empty( $theHeaders->headers )? 20219.patch​ is an attempt to fix the issue.

[dd32]

In [20370]:

WP_HTTP: Curl: Correct a typo in [20208] causing failed curl requests not to return a WP_Error under certain situations. Props SergeyBiryukov. Fixes #20219

[dd32]

Yeah, that's caused by your local Curl setup not having the root CA's installed (or something). I could verify by attempting to retrieve a self-signed cert (​https://dd32.id.au).

Thanks for catching it Sergey, it was a typo from switching from the raw curl headers in $this->headers and the processed data.

[dd32]

In [20399]:

WP_HTTP: Curl: $theHeaders is an array, not an object, introduced in [20370]. Props kurtpayne. Fixes #20389 See #20219