Merge pull request #221 from php-http/fix-redirect-host

redirection to different domain must not keep previous port

Author: dbu

.github/workflows/static.yml

@@ -2,6 +2,8 @@ name: static
 
 on:
   push:
+      branches:
+          - master
   pull_request:
 
 jobs:
@@ -19,3 +21,16 @@ jobs:
           REQUIRE_DEV: false
         with:
           args: analyze --no-progress
+
+  php-cs-fixer:
+      name: PHP-CS-Fixer
+      runs-on: ubuntu-latest
+
+      steps:
+          - name: Checkout code
+            uses: actions/checkout@v2
+
+          - name: PHP-CS-Fixer
+            uses: docker://oskarstark/php-cs-fixer-ga
+            with:
+                args: --dry-run --diff

.github/workflows/tests.yml

@@ -2,6 +2,8 @@ name: tests
 
 on:
   push:
+      branches:
+          - master
   pull_request:
 
 jobs:

.gitignore

@@ -1,5 +1,5 @@
-/.php_cs
-/.php_cs.cache
+/.php-cs-fixer.php
+/.php-cs-fixer.cache
 /behat.yml
 /build/
 /composer.lock

.php-cs-fixer.dist.php

@@ -0,0 +1,18 @@
+<?php
+
+$finder = PhpCsFixer\Finder::create()
+    ->in(__DIR__.'/src')
+    ->in(__DIR__.'/tests')
+    ->name('*.php')
+;
+
+$config = new PhpCsFixer\Config();
+
+return $config
+    ->setRiskyAllowed(true)
+    ->setRules([
+        '@Symfony' => true,
+        'single_line_throw' => false,
+    ])
+    ->setFinder($finder)
+;

CHANGELOG.md

@@ -4,7 +4,8 @@
 
 ### Fixed
 
-- Fixes false positive circular detection in RedirectPlugin in cases when target location does not contain path
+- [RedirectPlugin] Fixed handling of redirection to different domain with default port
+- [RedirectPlugin] Fixed false positive circular detection in RedirectPlugin in cases when target location does not contain path
 
 ## 2.5.0 - 2021-11-26
 

spec/Plugin/RedirectPluginSpec.php

@@ -506,54 +506,4 @@ public function it_throws_circular_redirection_exception_on_alternating_redirect
         $promise->shouldReturnAnInstanceOf(HttpRejectedPromise::class);
         $promise->shouldThrow(CircularRedirectionException::class)->duringWait();
     }
-
-    public function it_redirects_http_to_https(
-        UriInterface $uri,
-        UriInterface $uriRedirect,
-        RequestInterface $request,
-        ResponseInterface $responseRedirect,
-        RequestInterface $modifiedRequest,
-        ResponseInterface $finalResponse,
-        Promise $promise
-    ) {
-        $responseRedirect->getStatusCode()->willReturn(302);
-        $responseRedirect->hasHeader('Location')->willReturn(true);
-        $responseRedirect->getHeaderLine('Location')->willReturn('https://my-site.com/original');
-
-        $request->getUri()->willReturn($uri);
-        $request->withUri($uriRedirect)->willReturn($modifiedRequest);
-        $uri->__toString()->willReturn('http://my-site.com/original');
-        $uri->withPath('/original')->willReturn($uri);
-        $uri->withFragment('')->willReturn($uri);
-        $uri->withQuery('')->willReturn($uri);
-
-        $uri->withScheme('https')->willReturn($uriRedirect);
-        $uriRedirect->withHost('my-site.com')->willReturn($uriRedirect);
-        $uriRedirect->withPath('/original')->willReturn($uriRedirect);
-        $uriRedirect->withFragment('')->willReturn($uriRedirect);
-        $uriRedirect->withQuery('')->willReturn($uriRedirect);
-        $uriRedirect->__toString()->willReturn('https://my-site.com/original');
-
-        $modifiedRequest->getUri()->willReturn($uriRedirect);
-        $modifiedRequest->getMethod()->willReturn('GET');
-
-        $next = function (RequestInterface $receivedRequest) use ($request, $responseRedirect) {
-            if (Argument::is($request->getWrappedObject())->scoreArgument($receivedRequest)) {
-                return new HttpFulfilledPromise($responseRedirect->getWrappedObject());
-            }
-        };
-
-        $first = function (RequestInterface $receivedRequest) use ($modifiedRequest, $promise) {
-            if (Argument::is($modifiedRequest->getWrappedObject())->scoreArgument($receivedRequest)) {
-                return $promise->getWrappedObject();
-            }
-        };
-
-        $promise->getState()->willReturn(Promise::FULFILLED);
-        $promise->wait()->shouldBeCalled()->willReturn($finalResponse);
-
-        $finalPromise = $this->handleRequest($request, $next, $first);
-        $finalPromise->shouldReturnAnInstanceOf(HttpFulfilledPromise::class);
-        $finalPromise->wait()->shouldReturn($finalResponse);
-    }
 }

src/Deferred.php

@@ -146,7 +146,10 @@ public function wait($unwrap = true)
             return $this->value;
         }
 
-        /** @var ClientExceptionInterface */
+        if (null === $this->failure) {
+            throw new \RuntimeException('Internal Error: Promise is not fulfilled but has no exception stored');
+        }
+
         throw $this->failure;
     }
 }

src/HttpClientPool/HttpClientPool.php

@@ -46,9 +46,9 @@ public function addHttpClient($client): void
     /**
      * Return an http client given a specific strategy.
      *
-     * @throws HttpClientNotFoundException When no http client has been found into the pool
-     *
      * @return HttpClientPoolItem Return a http client that can do both sync or async
+     *
+     * @throws HttpClientNotFoundException When no http client has been found into the pool
      */
     abstract protected function chooseHttpClient(): HttpClientPoolItem;
 

src/Plugin/AddHostPlugin.php

@@ -31,7 +31,7 @@ final class AddHostPlugin implements Plugin
      * @param array{'replace'?: bool} $config
      *
      * Configuration options:
-     *   - replace: True will replace all hosts, false will only add host when none is specified.
+     *   - replace: True will replace all hosts, false will only add host when none is specified
      */
     public function __construct(UriInterface $host, array $config = [])
     {

src/Plugin/AddPathPlugin.php

@@ -70,7 +70,7 @@ public function handleRequest(RequestInterface $request, callable $next, callabl
         if (substr($path, 0, strlen($prepend)) !== $prepend) {
             $request = $request->withUri($request->getUri()
                  ->withPath($prepend.$path)
-             );
+            );
         }
 
         return $next($request);

src/Plugin/ContentTypePlugin.php

@@ -39,7 +39,7 @@ final class ContentTypePlugin implements Plugin
      *
      * Configuration options:
      *   - skip_detection: true skip detection if stream size is bigger than $size_limit
-     *   - size_limit: size stream limit for which the detection as to be skipped.
+     *   - size_limit: size stream limit for which the detection as to be skipped
      */
     public function __construct(array $config = [])
     {

src/Plugin/DecoderPlugin.php

@@ -34,7 +34,7 @@ final class DecoderPlugin implements Plugin
      * @param array{'use_content_encoding'?: bool} $config
      *
      * Configuration options:
-     *   - use_content_encoding: Whether this plugin should look at the Content-Encoding header first or only at the Transfer-Encoding (defaults to true).
+     *   - use_content_encoding: Whether this plugin should look at the Content-Encoding header first or only at the Transfer-Encoding (defaults to true)
      */
     public function __construct(array $config = [])
     {

src/Plugin/ErrorPlugin.php

@@ -40,7 +40,7 @@ final class ErrorPlugin implements Plugin
      * @param array{'only_server_exception'?: bool} $config
      *
      * Configuration options:
-     *   - only_server_exception: Whether this plugin should only throw 5XX Exceptions (default to false).
+     *   - only_server_exception: Whether this plugin should only throw 5XX Exceptions (default to false)
      */
     public function __construct(array $config = [])
     {
@@ -72,10 +72,10 @@ public function handleRequest(RequestInterface $request, callable $next, callabl
      * @param RequestInterface  $request  Request of the call
      * @param ResponseInterface $response Response of the call
      *
+     * @return ResponseInterface If status code is not in 4xx or 5xx return response
+     *
      * @throws ClientErrorException If response status code is a 4xx
      * @throws ServerErrorException If response status code is a 5xx
-     *
-     * @return ResponseInterface If status code is not in 4xx or 5xx return response
      */
     private function transformResponseToException(RequestInterface $request, ResponseInterface $response): ResponseInterface
     {

src/Plugin/RedirectPlugin.php

@@ -107,7 +107,7 @@ final class RedirectPlugin implements Plugin
      * Configuration options:
      *   - preserve_header: True keeps all headers, false remove all of them, an array is interpreted as a list of header names to keep
      *   - use_default_for_multiple: Whether the location header must be directly used for a multiple redirection status code (300)
-     *   - strict: When true, redirect codes 300, 301, 302 will not modify request method and body.
+     *   - strict: When true, redirect codes 300, 301, 302 will not modify request method and body
      */
     public function __construct(array $config = [])
     {
@@ -226,13 +226,39 @@ private function createUri(ResponseInterface $redirectResponse, RequestInterface
         $location = $redirectResponse->getHeaderLine('Location');
         $parsedLocation = parse_url($location);
 
-        if (false === $parsedLocation) {
-            throw new HttpException(sprintf('Location %s could not be parsed', $location), $originalRequest, $redirectResponse);
+        if (false === $parsedLocation || '' === $location) {
+            throw new HttpException(sprintf('Location "%s" could not be parsed', $location), $originalRequest, $redirectResponse);
         }
 
         $uri = $originalRequest->getUri();
+
+        // Redirections can either use an absolute uri or a relative reference https://www.rfc-editor.org/rfc/rfc3986#section-4.2
+        // If relative, we need to check if we have an absolute path or not
+
+        $path = array_key_exists('path', $parsedLocation) ? $parsedLocation['path'] : '';
+        if (!array_key_exists('host', $parsedLocation) && '/' !== $location[0]) {
+            // the target is a relative-path reference, we need to merge it with the base path
+            $originalPath = $uri->getPath();
+            if ('' === $path) {
+                $path = $originalPath;
+            } elseif (($pos = strrpos($originalPath, '/')) !== false) {
+                $path = substr($originalPath, 0, $pos + 1).$path;
+            } else {
+                $path = '/'.$path;
+            }
+            /* replace '/./' or '/foo/../' with '/' */
+            $re = ['#(/\./)#', '#/(?!\.\.)[^/]+/\.\./#'];
+            for ($n = 1; $n > 0; $path = preg_replace($re, '/', $path, -1, $n)) {
+                if (null === $path) {
+                    throw new HttpException(sprintf('Failed to resolve Location %s', $location), $originalRequest, $redirectResponse);
+                }
+            }
+        }
+        if (null === $path) {
+            throw new HttpException(sprintf('Failed to resolve Location %s', $location), $originalRequest, $redirectResponse);
+        }
         $uri = $uri
-            ->withPath(array_key_exists('path', $parsedLocation) ? $parsedLocation['path'] : '')
+            ->withPath($path)
             ->withQuery(array_key_exists('query', $parsedLocation) ? $parsedLocation['query'] : '')
             ->withFragment(array_key_exists('fragment', $parsedLocation) ? $parsedLocation['fragment'] : '')
         ;
@@ -247,6 +273,8 @@ private function createUri(ResponseInterface $redirectResponse, RequestInterface
 
         if (array_key_exists('port', $parsedLocation)) {
             $uri = $uri->withPort($parsedLocation['port']);
+        } elseif (array_key_exists('host', $parsedLocation)) {
+            $uri = $uri->withPort(null);
         }
 
         return $uri;

src/PluginChain.php

@@ -5,6 +5,7 @@
 namespace Http\Client\Common;
 
 use function array_reverse;
+
 use Http\Client\Common\Exception\LoopException;
 use Http\Promise\Promise;
 use Psr\Http\Message\RequestInterface;

src/PluginClient.php

@@ -124,7 +124,6 @@ private function configure(array $options = []): array
      */
     private function createPluginChain(array $plugins, callable $clientCallable): callable
     {
-        /** @var callable(RequestInterface): Promise */
         return new PluginChain($plugins, $clientCallable, $this->options);
     }
 }

src/PluginClientFactory.php

@@ -39,11 +39,11 @@ public static function setFactory(callable $factory): void
     /**
      * @param ClientInterface|HttpAsyncClient $client
      * @param Plugin[]                        $plugins
-     * @param array{'client_name'?: string}    $options
+     * @param array{'client_name'?: string}   $options
      *
      * Configuration options:
      *   - client_name: to give client a name which may be used when displaying client information
-     *     like in the HTTPlugBundle profiler.
+     *     like in the HTTPlugBundle profiler
      *
      * @see PluginClient constructor for PluginClient specific $options.
      */

tests/Plugin/RedirectPluginTest.php

@@ -1,4 +1,5 @@
 <?php
+
 declare(strict_types=1);
 
 namespace Plugin;
@@ -26,10 +27,26 @@ function () {}
         )->wait();
     }
 
+    public function provideRedirections(): array
+    {
+        return [
+            'no path on target' => ['https://example.com/path?query=value', 'https://example.com?query=value', 'https://example.com?query=value'],
+            'root path on target' => ['https://example.com/path?query=value', 'https://example.com/?query=value', 'https://example.com/?query=value'],
+            'redirect to query' => ['https://example.com', 'https://example.com?query=value', 'https://example.com?query=value'],
+            'redirect to different domain without port' => ['https://example.com:8000', 'https://foo.com?query=value', 'https://foo.com?query=value'],
+            'network-path redirect, preserve scheme' => ['https://example.com:8000', '//foo.com/path?query=value', 'https://foo.com/path?query=value'],
+            'absolute-path redirect, preserve host' => ['https://example.com:8000', '/path?query=value', 'https://example.com:8000/path?query=value'],
+            'relative-path redirect, append' => ['https://example.com:8000/path/', 'sub/path?query=value', 'https://example.com:8000/path/sub/path?query=value'],
+            'relative-path on non-folder' => ['https://example.com:8000/path/foo', 'sub/path?query=value', 'https://example.com:8000/path/sub/path?query=value'],
+            'relative-path moving up' => ['https://example.com:8000/path/', '../other?query=value', 'https://example.com:8000/other?query=value'],
+            'relative-path with ./' => ['https://example.com:8000/path/', './other?query=value', 'https://example.com:8000/path/other?query=value'],
+            'relative-path with //' => ['https://example.com:8000/path/', 'other//sub?query=value', 'https://example.com:8000/path/other//sub?query=value'],
+            'relative-path redirect with only query' => ['https://example.com:8000/path', '?query=value', 'https://example.com:8000/path?query=value'],
+       ];
+    }
+
     /**
-     * @testWith ["https://example.com/path?query=value", "https://example.com?query=value", "https://example.com?query=value"]
-     *           ["https://example.com/path?query=value", "https://example.com/?query=value", "https://example.com/?query=value"]
-     *           ["https://example.com", "https://example.com?query=value", "https://example.com?query=value"]
+     * @dataProvider provideRedirections
      */
     public function testTargetUriMappingFromLocationHeader(string $originalUri, string $locationUri, string $targetUri): void
     {

tests/PluginChainTest.php

@@ -9,15 +9,13 @@
 use Http\Client\Common\PluginChain;
 use Http\Promise\Promise;
 use PHPUnit\Framework\TestCase;
-use Prophecy\Argument;
 use Psr\Http\Message\RequestInterface;
 
 class PluginChainTest extends TestCase
 {
     private function createPlugin(callable $func): Plugin
     {
-        return new class ($func) implements Plugin
-        {
+        return new class($func) implements Plugin {
             public $func;
 
             public function __construct(callable $func)