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:

.github/workflows/tests.yml

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

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/Plugin/RedirectPlugin.php

@@ -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;

tests/Plugin/RedirectPluginTest.php

@@ -26,10 +26,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
     {