fsockopen performs a POST request to a redirected location

[dd32]

Continuing off #17490.

Some background first: When you make a POST request, If you recieve a 3xx w/ Location in response, The correct thing to do next, is to perform a GET request to the new url, which is not what most people expect (Expecting the class to follow the Location with a new POST).

fsockopen doesn't follow this spec, cURL and Streams do, Upon receiving the redirection, fsockopen performs the exact same request against the new location, whereas, it should disregard the post fields (and the Content-length header) and perform a GET instead.

[nacin]

Good reads: ​http://www.alanflavell.org.uk/www/post-redirect.html.

According to that at least, a 301 should result in a POST to the new URL, while it appears a 302/3/7 should result in a GET. That said, it looks like browsers make a mess of what *should* occur.

The article appears quite definitive (if old), but of course, grain of salt for things you read on the interwebs.

[dd32]

I'm not surprised it's a mess with browser support.. hopefully that's just old data though and modern browsers handle it better.

It's worthwhile mentioning here, We don't follow the spec either with redirects as it is; We currently manually redirect on any location header being present, whereas, we should generally only redirect on it under certain response codes..

In the interest of potentially working with various API interfaces, I think it's best to support this the best we can.. Even if this only means we make fsockopen perform to the same rules as curl/streams use internally (even if they turn out to be technically wrong in some cases)

[wonderboymusic]

I almost fell asleep reading that article, but what I gathered was that 302/303 should redirect to GET (but we'll ignore verification with user for 302) and 307 should continue as a POST (but we'll ignore verification with user). Attached a patch.

[wonderboymusic]

dd32 should probably chime in here

[dd32]

I almost fell asleep reading that article

Welcome to the world of Specifications.

Seeing what "modern" browsers do with this spec would be interesting, as the article mentioned above is of 2000~2005 vintage. That being said, if someone is using WP_HTTP for interaction with a RESTful API, then we'd want to honor the spec correctly (ignoring the verification stuff)

but what I gathered was that 302/303 should redirect to GET (but we'll ignore verification with user for 302) and 307 should continue as a POST (but we'll ignore verification with user)

Thats what I gathered.

[rmccue]

Relevant from ​RFC2616 (HTTP/1.1):

If the 302 status code is received in response to a request other
than GET or HEAD, the user agent MUST NOT automatically redirect the
request unless it can be confirmed by the user, since this might
change the conditions under which the request was issued.

Note: RFC 1945 and RFC 2068 specify that the client is not allowed
to change the method on the redirected request. However, most
existing user agent implementations treat 302 as if it were a 303
response, performing a GET on the Location field-value regardless
of the original request method. The status codes 303 and 307 have
been added for servers that wish to make unambiguously clear which
kind of reaction is expected of the client.

I just tested with both Firefox and Chrome, and both change POST to GET automatically (so technically, this makes them non-2616 compliant). A 307 gives a dialog in Firefox, but not in Chrome.

What WP_HTTP should do is redirect this automatically for pragmatic reasons and ignore the spec in this case. A 307 should ignore the verification requirement however, as wonderboymusic noted, and there's precedent for doing this with Chrome.

As such, the patch looks good. +1

[rmccue]

As per short IRC chat with dd32, marking needs-unit-tests. Also recommending commit after those are done.

[kovshenin]

Added some unit tests in 17588.unit-tests.diff​. Tests curl, fsockopen and streams for the request method when POSTing to the redirection script. Makes sure it returns GET for 302 and 303, and POST for 301 and 307. It works well for fsockopen with the applied patch, but fails for curl and streams when doing a 302 redirect :(

17588.diff​ is wonderboymusic's patch refreshed against trunk.

[dd32]

It works well for fsockopen with the applied patch, but fails for curl and streams when doing a 302 redirect :(

With #23682 + [23603] we should now be able to fix it for curl+streams too, anyone out there that feels like taking a shot at implementing that?

[markjaquith]

Let's fix it for both together.

[dd32]

Attachment 17588.2.diff​ added

Covers:

• #17588 - POST's to redirected locations, including kovshenin's patch (Sorry, Didn't realise you had unit tests here)
• #16889 - Having a Location header doesn't mean it's a redirect

Unit tests pass with r16889-tests (​http://core.trac.wordpress.org/changeset/1316/tests)

[dd32]

In 24843:

WP_HTTP: Abstract out the Redirection handling code into it's own method and fix a bunch of redirection edgecases at the same time.
Fixes #17588
Fixes 16889
Props wonderboymusic and kovshenin for initial patches