WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 4 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 4 months ago.

Download all attachments as: .zip

Change History (17)

hakre2 years ago

First digit counts

comment:1 hakre2 years ago

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.

Version 0, edited 2 years ago by hakre (next)

hakre2 years ago

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

comment:2 hakre2 years ago

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: 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.

comment:7 hakre2 years ago

  • Keywords has-patch removed

comment:8 dd3214 months ago

  • 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

comment:9 Viper007Bond14 months ago

  • Cc viper007bond added

comment:10 Viper007Bond14 months ago

  • Cc Viper007Bond added; viper007bond removed

comment:11 mbijon13 months ago

  • Cc mike@… added

comment:12 dd324 months ago

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

dd324 months ago

comment:13 dd324 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 ;)

comment:14 Viper007Bond4 months ago

Worth having a filter in there?

Note: See TracTickets for help on using tickets.