Redirect Plugin - Circular redirection detected

[quetzyg]

Q	A
Bug?	yes
New Feature?	no
Version	1.2.1

Actual Behavior

An Http\Client\Common\Exception\CircularRedirectionException is thrown when executing the following code:

<?php

require 'vendor/autoload.php';

use Http\Adapter\Guzzle6\Client as HttpClient;
use Http\Client\Common\PluginClient;
use Http\Client\Common\Plugin\RedirectPlugin;
use Http\Message\MessageFactory\GuzzleMessageFactory as MessageFactory;

$message = new MessageFactory;

$pluginClient = new PluginClient(new HttpClient, [
    new RedirectPlugin,
]);

$request = $message->createRequest('GET', 'http://www.artstation.com/artwork.rss');
$response = $pluginClient->sendRequest($request);

var_dump($response->getBody()->getContents());

Expected Behavior

No exception should be thrown, since http://www.artstation.com/artwork.rss redirects to https://www.artstation.com/artwork.rss (http -> https)

Steps to Reproduce

Execute the code above.

Possible Solutions

The issue is on line 162, where the circular redirection detection is done.

This line of code:

if (in_array($redirectRequest->getRequestTarget(), $this->circularDetection[$chainIdentifier]))

is checking if the current request target already exists in the circular detection map.

Because two lines before (160), the same value is being added, it will always be true.

[dbu]

this is really strange, as we have an extensive test in phpspec on this: https://github.com/php-http/client-common/blob/master/spec/Plugin/RedirectPluginSpec.php

but looking at the actual code, this looks wrong to me as well. @sagikazarmark do you see something wrong with the spec? did we mess up with the mocking of what is going on and somehow hide the problem? we might want to use actual Response instances instead of mocks there, to avoid confusing ourselves with mocks.

[quetzyg]

Actually, I just realised that the code is adding the request target from the $request and checking if the $redirectRequest request target is the same. Still, in cases where the only difference is the protocol, we shouldn't get the an exception.

What if instead of using the request target for comparison (/artwork.rss in this case), the whole URL would be used? https://www.artstation.com/artwork.rss vs http://www.artstation.com/artwork.rss ?

[dbu]

ah, good. but yes, that is quite wrong. it could also be http://host.com/ => http://api.host.com/ or whatever. we totally have to take domain and scheme into account. can you do a pull request for that?

[quetzyg]

Line 160 would probably have to be:

$this->circularDetection[$chainIdentifier][] = (string) $request->getUri();

and line 162:

if (in_array((string) $redirectRequest->getUri(), $this->circularDetection[$chainIdentifier])) {

[quetzyg]

I'll do a PR, bear with me.

[dbu]

great! yep, sounds right. if you could also adjust the phpspec test to make sure this keeps working, that would be awesome. i'd start by adding a test for redirect that only changes the scheme, and see that fail, then change the code and see it works again :-)

[quetzyg]

OK, so this morning I started making the proposed changes on my local and started testing, but now I'm getting a \Http\Client\Common\Exception\LoopException with the message Too many restarts in plugin client.

@dbu any ideas?

[sagikazarmark]

It means the plugin chain has been restarted more than it is allowed to (10 by default).

Done here: https://github.com/php-http/client-common/blob/master/src/Plugin/RedirectPlugin.php#L174

Can you check whether the redirect request is correctly built?

[sagikazarmark]

@dbu Honestly I haven't written these specs, so I am not 100% sure about them, but I start to think that HTTP Client related code should be tested with the MockClient. Mocks are cool, but HTTP Messages are rather ValueObjects/DTOs and as such, they shouldn't really be mocked (when used extensively).

[quetzyg]

@sagikazarmark the redirect response should be OK, since the only thing that I'm changing is the value we're storing in $this->circularDetection[$chainIdentifier].

Here's what the object looks like, after being created:

object(GuzzleHttp\Psr7\Request)#1671 (7) {  
  ["method":"GuzzleHttp\Psr7\Request":private]=>
  string(3) "GET"
  ["requestTarget":"GuzzleHttp\Psr7\Request":private]=>
  NULL
  ["uri":"GuzzleHttp\Psr7\Request":private]=>
  object(GuzzleHttp\Psr7\Uri)#1645 (7) {
    ["scheme":"GuzzleHttp\Psr7\Uri":private]=>
    string(5) "https"
    ["userInfo":"GuzzleHttp\Psr7\Uri":private]=>
    string(0) ""
    ["host":"GuzzleHttp\Psr7\Uri":private]=>
    string(18) "www.artstation.com"
    ["port":"GuzzleHttp\Psr7\Uri":private]=>
    NULL
    ["path":"GuzzleHttp\Psr7\Uri":private]=>
    string(12) "/artwork.rss"
    ["query":"GuzzleHttp\Psr7\Uri":private]=>
    string(0) ""
    ["fragment":"GuzzleHttp\Psr7\Uri":private]=>
    string(0) ""
  }
  ["headers":"GuzzleHttp\Psr7\Request":private]=>
  array(1) {
    ["Host"]=>
    array(1) {
      [0]=>
      string(18) "www.artstation.com"
    }
  }
  ["headerNames":"GuzzleHttp\Psr7\Request":private]=>
  array(1) {
    ["host"]=>
    string(4) "Host"
  }
  ["protocol":"GuzzleHttp\Psr7\Request":private]=>
  string(3) "1.1"
  ["stream":"GuzzleHttp\Psr7\Request":private]=>
  object(GuzzleHttp\Psr7\Stream)#1655 (7) {
    ["stream":"GuzzleHttp\Psr7\Stream":private]=>
    resource(320) of type (stream)
    ["size":"GuzzleHttp\Psr7\Stream":private]=>
    int(0)
    ["seekable":"GuzzleHttp\Psr7\Stream":private]=>
    bool(true)
    ["readable":"GuzzleHttp\Psr7\Stream":private]=>
    bool(true)
    ["writable":"GuzzleHttp\Psr7\Stream":private]=>
    bool(true)
    ["uri":"GuzzleHttp\Psr7\Stream":private]=>
    string(10) "php://temp"
    ["customMetadata":"GuzzleHttp\Psr7\Stream":private]=>
    array(0) {
    }
  }
}

[sagikazarmark]

Maybe the problem is here:

https://github.com/php-http/client-common/blob/master/src/Plugin/RedirectPlugin.php#L142

Note the usage of $request->getRequestTarget() in other parts of the code.

[quetzyg]

@sagikazarmark, nicely spotted!

To make it work, line 137 needs to be changed to:

if (array_key_exists((string) $request->getUri(), $this->redirectStorage)) {

At least, that seems to do the trick for me.

[quetzyg]

In regards to the specs, I'm clueless on what changes need to be done.
When running vendor/bin/phpspec run spec/Plugin/RedirectPluginSpec.php I get:

✘ Fatal error happened while executing the following 
    it redirects on 302 
    Method Double\UriInterface\P4::__toString() must not throw an exception in /home/vagrant/GH/client-common/src/Plugin/RedirectPlugin.php on line 0 

[joelwurtz]

I start to think that HTTP Client related code should be tested with the MockClient. Mocks are cool, but HTTP Messages are rather ValueObjects/DTOs and as such, they shouldn't really be mocked (when used extensively).

👍 But we should do that in another PR

@quetzyg Can you submit a PR with your changes, even if tests are failing ? Would be easier to help with a diff :)

[quetzyg]

@joelwurtz, sure thing.