Opened 2 years ago

Last modified 3 months ago

#16889 reopened defect

Having a location header does not mean that there should be a redirection.

Reported by: hakre Owned by:
Priority: normal Milestone: 3.6
Component: HTTP Version: 3.1
Severity: normal Keywords: has-patch
Cc: Viper007Bond, mike@…

Description

Having a location header does not mean that there should be a redirection.

Automatic redirection depends foremost on the response code, not not having a location header.

See the descrption of #16888 for related RFC links.

Related: #11305

Attachments (3)

16889.patch (768 bytes) - added by hakre 2 years ago.
First digit counts
16889.2.patch (800 bytes) - added by hakre 2 years ago.
curl_getinfo might not return string - user comment in http://www.php.net/manual/en/function.curl-getinfo.php
16889.diff (2.4 KB) - added by dd32 3 months ago.

Download all attachments as: .zip

Change History (17)

hakre2 years ago

First digit counts

Out of scope of this ticket but related in the code: It fails if the response contains more than one location header line. Just running over it.

Related: #16890 - a patch for that problem is available there.

Last edited 2 years ago by hakre (previous) (diff)

hakre2 years ago

curl_getinfo might not return string - user comment in http://www.php.net/manual/en/function.curl-getinfo.php

And there was an additional error in the if clause which prevented it from working.

comment:3   dd322 years ago

  • Component changed from General to HTTP

comment:4 follow-up: ↓ 6   dd322 years ago

  • Keywords close added

Unfortunately, I don't think it's going to be possible to adhere to this specification.

cURL(CURLOPT_FOLLOWLOCATION), HTTP Extension, and Streams (therefor PHP Internally) all follow redirects on 200 (ie. non-3xx) responses, this seems to be done (From what I can understand) for compatibility reasons with older web servers (unsure of which ones specifically)

It's probably best to standardise on following location regardless of the response code as we're currently doing.

comment:5   dd322 years ago

  • Keywords close removed
  • Milestone Awaiting Review deleted
  • Resolution set to invalid
  • Status changed from new to closed

based on my previous comment, and further research into if it's possible, I'm closing as invalid. It's simply not possible to adhere to it whilst relying on any current-generation HTTP API offered by PHP.

comment:6 in reply to: ↑ 4   hakre2 years ago

Replying to dd32:

Unfortunately, I don't think it's going to be possible to adhere to this specification.

cURL(CURLOPT_FOLLOWLOCATION), HTTP Extension, and Streams (therefor PHP Internally) all follow redirects on 200 (ie. non-3xx) responses, this seems to be done (From what I can understand) for compatibility reasons with older web servers (unsure of which ones specifically)

Most certainly by 6.2 Response Header Fields and 14.30 Location.

It's probably best to standardise on following location regardless of the response code as we're currently doing.

I now think so as well now. Thanks for your support so far.

  • Keywords has-patch removed
  • Milestone set to Awaiting Review
  • Resolution invalid deleted
  • Status changed from closed to reopened

I'm re-opening this to look into it again, hopefully things have changed enough, or at least changed that we can partially fix this.

In 3.4 the way Curl follows redirects has changed, it's moved from internal in curl, to being handled in PHP by us to make it more straight forward.

In addition, it's been pointed out that following Location headers causes problems for 201 (Created) requests, since WP_HTTP blindly follows the location, the result from the Created command is lost.

An example of a request which this fouls up is this the following Google Contacts API request (note, this output is taken directly from the curl command line binary)

> POST /m8/feeds/contacts/default/full/?access_token=......
> HTTP/1.1
> User-Agent: curl/7.21.4 (universal-apple-darwin11.0) libcurl/7.21.4 OpenSSL/0.9.8r zlib/1.2.5
> Host: www.google.com
> Accept: */*
> Content-Type: application/atom+xml
> Content-Length: 465
> 
< HTTP/1.1 201 Created
< Content-Type: application/atom+xml; charset=UTF-8
< Expires: Sat, 05 May 2012 01:36:29 GMT
< Date: Sat, 05 May 2012 01:36:29 GMT
< Cache-Control: private, max-age=0, must-revalidate, no-transform
< Vary: Accept, X-GData-Authorization, GData-Version
< GData-Version: 1.0
< Location: https://www.google.com/m8/feeds/contacts/user%40gmail.com/full/123/345
< Content-Location: https://www.google.com/m8/feeds/contacts/user%40gmail.com/full/123/345
< X-Content-Type-Options: nosniff
< X-Frame-Options: SAMEORIGIN
< X-XSS-Protection: 1; mode=block
< Server: GSE
< Transfer-Encoding: chunked

<?xml version='1.0' encoding='UTF-8'?>
...

If WP_HTTP makes the request it'll follow the Location header and the client can't retrieve the XML payload.

One can pass 'redirection' => 0 to the request and it "just works" (at least in the case of the curl transport) so hopefully that's a sign we can properly fix this now

  • Cc viper007bond added
  • Cc Viper007Bond added; viper007bond removed
  • Cc mike@… added

In 1231/tests:

Rename the WP_HTTP testcase 'test_location_header_on_200' to 'test_location_header_on_201' and modify test / test script accordingly, PHP doesn't allow us to issue a Location with a 200 response, and the ticket it refers to is specifically about 201 responses.
See #16889

dd323 months ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 3.6

16889.diff is a patch which worksforme, and passes the unit tests (once I corrected the unit tests from attempting a 200 response (which PHP declines to send) to a 201) - Thats the first time in a long time, that I've seen 100% of the HTTP unit tests pass ;)

Worth having a filter in there?

Note: See TracTickets for help on using tickets.