WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 9 months ago

#17588 closed defect (bug) (fixed)

fsockopen performs a POST request to a redirected location

Reported by: dd32 Owned by: dd32
Milestone: 3.7 Priority: normal
Severity: normal Version: 3.2
Component: HTTP API Keywords: has-patch commit
Focuses: Cc:

Description

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.

Attachments (4)

post-to-get-fsockopen.diff (739 bytes) - added by wonderboymusic 20 months ago.
17588.unit-tests.diff (1.8 KB) - added by kovshenin 13 months ago.
17588.diff (738 bytes) - added by kovshenin 13 months ago.
17588.2.diff (5.6 KB) - added by dd32 9 months ago.

Download all attachments as: .zip

Change History (19)

comment:1 nacin3 years ago

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.

comment:2 dd323 years ago

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)

comment:3 simsmaster3 years ago

  • Cc info@… added

comment:4 wonderboymusic20 months ago

  • Keywords has-patch added; needs-patch removed

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.

comment:5 wonderboymusic16 months ago

  • Milestone changed from Future Release to 3.6

dd32 should probably chime in here

comment:6 dd3216 months ago

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.

comment:7 rmccue16 months ago

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

comment:8 rmccue15 months ago

  • Keywords needs-unit-tests added

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

kovshenin13 months ago

comment:9 kovshenin13 months ago

  • Keywords needs-unit-tests removed

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.

comment:10 wonderboymusic10 months ago

  • Keywords commit added

comment:11 dd3210 months ago

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?

comment:12 markjaquith10 months ago

  • Milestone changed from 3.6 to Future Release

Let's fix it for both together.

dd329 months ago

comment:13 dd329 months ago

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)

comment:14 wonderboymusic9 months ago

  • Milestone changed from Future Release to 3.7

comment:15 dd329 months ago

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

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

Note: See TracTickets for help on using tickets.