Make WordPress Core

Opened 13 years ago

Closed 13 years ago

Last modified 13 years ago

#20434 closed defect (bug) (fixed)

cURL fails to follow redirects sometimes

Reported by: evansolomon's profile evansolomon Owned by: ryan's profile ryan
Milestone: 3.4 Priority: high
Severity: critical Version: 3.4
Component: HTTP API Keywords: needs-unit-tests has-patch
Focuses: Cc:

Description

r20208 changed the way WP_HTTP (specifically cURL) handles redirects, and as a result broke the way it handles some redirects (both 301 and 302) that were previously working. It doesn't fail on all 301/302 redirects.

At first I thought it failed only for requests with more than 1 redirect, but in testing with the Quantcast top 100 lists (assuming these sites should have no problems loading usually) I found an exception to that rule in http://facebook.com, which works and has 2 redirects. Then I thought it only failed for requests with more than 1 301 redirect, but I found an exception to that in http://dailymotion.com, which fails and only has 1 301 redirect (and a 302).

Here are a couple examples that fail using wp_remote_get(): http://skype.com, http://microsoft.com, http://dailymotion.com,

Here are a couple from the same list that work (and go through redirects): http://google.com, http://facebook.com, http://yahoo.com

As of r20208 wp_remote_get() returns empty arrays and strings for headers, body, response, etc. As of r20370 the same failed requests return a WP_Error object with an error code of 'http_request_failed' and an error message of ' malformed' (including the leading space). As of r20460 the same WP_Error objects are still returned.

Attachments (5)

20434[1].diff (3.0 KB) - added by dd32 13 years ago.
20434.2.diff (3.1 KB) - added by evansolomon 13 years ago.
20434.diff (1.9 KB) - added by nacin 13 years ago.
20434-ut.diff (3.8 KB) - added by dd32 13 years ago.
Unit Tests
20434.3.diff (3.0 KB) - added by dd32 13 years ago.
WP_HTTP::make_absolute_url()

Download all attachments as: .zip

Change History (22)

#1 @dd32
13 years ago

  • Milestone changed from Awaiting Review to 3.4

The problem is with relative locations in the Location header (which, IIRC, is not allowed in the spec, but used in the wild by many).

http://skype.com/
=> http://www.skype.com/
=> http://www.skype.com/intl/en/
=> /intl/en/home

That last one causes the failure.

#2 @dd32
13 years ago

the Fsockopen class doesn't handle relative url's without a hostname either, Streams seems to be unaffected.

@dd32
13 years ago

#3 @dd32
13 years ago

attachment 20434[1].diff added

That's a quick attempt from me that covers the skype.com redirections, and should handle the most common relative responses.. not 100% it's correct though in it's logic that browsers have perfected in handling the non-spec responses

Only testing is skype.com and the following: ( Result should be a bunch of arrays of the same values.. )

var_dump( array( 'http://example.com/test/url', WP_HTTP::make_absolute_url( '/test/url', 'http://example.com/here/' ) ) );
var_dump( array( 'http://example.com/bro', WP_HTTP::make_absolute_url( 'bro', 'http://example.com/here' ) ) );

var_dump( array( 'http://example.com/here/sub', WP_HTTP::make_absolute_url( 'sub', 'http://example.com/here/' ) ) );
var_dump( array( 'http://example.com/test/url', WP_HTTP::make_absolute_url( 'test/url', 'http://example.com' ) ) );

var_dump( array( '/test/url', WP_HTTP::make_absolute_url( '/test/url', 'http://' ) ) );

var_dump( array( 'http://example.com/test', WP_HTTP::make_absolute_url( '../test', 'http://example.com/product' ) ) );
var_dump( array( 'http://example.com/test', WP_HTTP::make_absolute_url( '../test', 'http://example.com/product/' ) ) );
var_dump( array( 'http://example.com/product/test', WP_HTTP::make_absolute_url( '../test', 'http://example.com/product/one/' ) ) );
var_dump( array( 'http://example.com/test', WP_HTTP::make_absolute_url( '../test', 'http://example.com/product/two' ) ) );

@evansolomon
13 years ago

#4 @evansolomon
13 years ago

Confirmed 20434[1].diff (Trac doesn't seem to like this file name format) works. So far I haven't found any that it misses; it's corrected all the previously-failed requests.

I uploaded a new patch (20434.2.diff). It makes the same changes but patches from the root instead of /wp-includes. It took me a few minutes to figure out why the patch wouldn't apply at first, so I figured I'd save someone else the confusion in case they want to test it.

#5 @nacin
13 years ago

  • Keywords needs-unit-tests has-patch added; needs-patch removed

#6 @nacin
13 years ago

  • Severity changed from normal to critical
  • Version set to 3.4

#7 @dd32
13 years ago

I'm not happy with the combining logic in that patch, primarily for the ../ case, which is the only edgecase that would really cause an issue

I've just taken a look around, and this code (once create_function is removed) looks like it might fit the bill better: http://www.liranuna.com/php-path-resolution-class-relative-paths-made-easy/ (license: WTFPL) or http://bsd-noobz.com/blog/php-script-for-converting-relative-to-absolute-url (license: Not stated)

#8 @nacin
13 years ago

Is there a reason to support non-root-relative redirects? Seems edge.

#9 @dd32
13 years ago

Is there a reason to support non-root-relative redirects? Seems edge.

I've seen it used plenty of times by not-too-great devs, and can just see it being raised as a issue in 3.6 :) That being said, I'm definitely not against skipping non-root-relative links for 3.4 given timing.

@nacin
13 years ago

#10 @nacin
13 years ago

  • Keywords commit added

How does 20434.diff look?

#11 @ryan
13 years ago

  • Priority changed from normal to high

#12 @dd32
13 years ago

How does 20434.diff look?

Won't work for Location: redirect.php in subdirectories, To be clear, I was purely thinking of skipping ../index.php style url's.

The order I would expect to see the popularity of redirect types would be:

1. http://absolute.url/
2. /root/relative/path.ext
3. file-in-same-dir.ext
4. ../file-up-a-directory.ext

Until recently I'm pretty sure Core used version 3 in a few spots in the admin, and it's reasonably common elsewhere I've seen. Number 4 is edge, used, but edge.

#13 @ryan
13 years ago

Need traction soon on this, otherwise we should consider reverting r20208.

#14 @ryan
13 years ago

  • Keywords commit removed

@dd32
13 years ago

Unit Tests

@dd32
13 years ago

WP_HTTP::make_absolute_url()

#15 @dd32
13 years ago

attachment 20434-ut.diff added Unit Tests

attachment 20434.3.diff added WP_HTTP::make_absolute_url()

Eyes + Testing + kicking & screaming please.

Doesn't bother with the #fragment since that's not supposed to be sent to/from the server.

#16 @ryan
13 years ago

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

In [20767]:

Handle relative urls when processing redirects. Introduce WP_Http::make_absolute_url(). Props dd32. fixes #20434

Note: See TracTickets for help on using tickets.