WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#20219 closed defect (bug) (fixed)

GET - Too many redirects

Reported by: Workshopshed Owned by: dd32
Milestone: 3.4 Priority: normal
Severity: normal Version:
Component: HTTP API Keywords: has-patch
Focuses: Cc:

Description

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)) ) {

Attachments (3)

class-http.php.diff (716 bytes) - added by Workshopshed 3 years ago.
dif with one possible fix for this issue
class-http.php.2.diff (2.7 KB) - added by Workshopshed 3 years ago.
Explicitly set an "autoredirect" flag
20219.patch (987 bytes) - added by SergeyBiryukov 3 years ago.

Download all attachments as: .zip

Change History (20)

@Workshopshed3 years ago

dif with one possible fix for this issue

comment:1 @scribu3 years ago

  • Component changed from General to HTTP
  • Keywords has-patch added

comment:2 @kawauso3 years ago

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.

comment:3 @dd323 years ago

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?

comment:4 @dd323 years ago

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.

comment:5 @Workshopshed3 years ago

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

comment:6 @dd323 years ago

(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.

comment:7 @Workshopshed3 years ago

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.

comment:8 @dd323 years ago

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)

@Workshopshed3 years ago

Explicitly set an "autoredirect" flag

comment:9 @Workshopshed3 years ago

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

comment:11 @dd323 years ago

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)

comment:12 @dd323 years ago

  • Milestone changed from Awaiting Review to 3.4

comment:13 @dd323 years ago

  • Owner set to dd32
  • Resolution set to fixed
  • Status changed from new to closed

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

@SergeyBiryukov3 years ago

comment:14 @SergeyBiryukov3 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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.

comment:15 @dd323 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

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

comment:16 @dd323 years ago

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.

comment:17 @dd323 years ago

In [20399]:

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

Note: See TracTickets for help on using tickets.