WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 9 months ago

#16889 closed defect (bug) (fixed)

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

Reported by: hakre Owned by: dd32
Milestone: 3.7 Priority: normal
Severity: normal Version: 3.1
Component: HTTP API Keywords: has-patch needs-codex
Focuses: Cc:

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 (4)

16889.patch (768 bytes) - added by hakre 3 years ago.
First digit counts
16889.2.patch (800 bytes) - added by hakre 3 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 14 months ago.
16889.2.diff (1.0 KB) - added by DrewAPicture 9 months ago.
phpdoc fixes for WP_Http::handle_redirects()

Download all attachments as: .zip

Change History (23)

hakre3 years ago

First digit counts

comment:1 hakre3 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.

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

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

hakre3 years ago

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

comment:2 hakre3 years ago

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

comment:3 dd323 years ago

  • Component changed from General to HTTP

comment:4 follow-up: dd323 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 dd323 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 hakre3 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 hakre3 years ago

  • Keywords has-patch removed

comment:8 dd322 years 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 Viper007Bond2 years ago

  • Cc viper007bond added

comment:10 Viper007Bond2 years ago

  • Cc Viper007Bond added; viper007bond removed

comment:11 mbijon2 years ago

  • Cc mike@… added

comment:12 dd3214 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

dd3214 months ago

comment:13 dd3214 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 Viper007Bond14 months ago

Worth having a filter in there?

comment:15 markjaquith10 months ago

  • Milestone changed from 3.6 to Future Release

comment:16 dd329 months ago

  • Milestone changed from Future Release to 3.7

comment:17 dd329 months ago

  • Resolution set to fixed
  • Status changed from reopened 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

Last edited 9 months ago by dd32 (previous) (diff)

comment:18 DrewAPicture9 months ago

  • Keywords needs-codex added
  • Resolution fixed deleted
  • Status changed from closed to reopened

16889.2.diff adds a trailing zero to the @since tag for the new WP_Http::handle_redirects() method, as well as types for the @param tags.

Last edited 9 months ago by DrewAPicture (previous) (diff)

comment:19 dd329 months ago

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

In 24890:

WP_HTTP: PHPDoc updates for WP_Http::handle_redirects(). Props DrewAPicture. Fixes #16889

DrewAPicture9 months ago

phpdoc fixes for WP_Http::handle_redirects()

Note: See TracTickets for help on using tickets.