cURL fails to follow redirects sometimes

[evansolomon]

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.

[dd32]

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.

[dd32]

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

[dd32]

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]

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.

[dd32]

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)

[nacin]

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

[dd32]

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]

How does 20434.diff​ look?

[dd32]

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.

[ryan]

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

[dd32]

Unit Tests

[dd32]

WP_HTTP::make_absolute_url()

[dd32]

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.

[ryan]

In [20767]:

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

[ryan]

​[UT713]