Merge pull request #218 from ostrolucky/fix-redirect-plugin-path-hand…

…ling RedirectPlugin: Default to empty path when Location doesn't specify any
php-http · Sep 21, 2022 · 76730e3 · 76730e3
2 parents a2dd39f + c06f154
commit 76730e3
Show file tree

Hide file tree

Showing 4 changed files with 65 additions and 16 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,5 +1,11 @@
 # Change Log
 
+## 2.5.1 - 2022-09-??
+
+### Fixed
+
+- Fixes false positive circular detection in RedirectPlugin in cases when target location does not contain path
+
 ## 2.5.0 - 2021-11-26
 
 ### Added

diff --git a/spec/Plugin/RedirectPluginSpec.php b/spec/Plugin/RedirectPluginSpec.php
@@ -162,6 +162,9 @@ public function it_replace_full_url(
 
         $request->getUri()->willReturn($uri);
         $uri->withScheme('https')->willReturn($uriRedirect);
+        $uri->withPath('/redirect')->willReturn($uri);
+        $uri->withQuery('query')->willReturn($uri);
+        $uri->withFragment('fragment')->willReturn($uri);
         $uriRedirect->withHost('server.com')->willReturn($uriRedirect);
         $uriRedirect->withPort('8000')->willReturn($uriRedirect);
         $uriRedirect->withPath('/redirect')->willReturn($uriRedirect);
@@ -520,6 +523,9 @@ public function it_redirects_http_to_https(
         $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);

diff --git a/src/Plugin/RedirectPlugin.php b/src/Plugin/RedirectPlugin.php
@@ -231,6 +231,11 @@ private function createUri(ResponseInterface $redirectResponse, RequestInterface
         }
 
         $uri = $originalRequest->getUri();
+        $uri = $uri
+            ->withPath(array_key_exists('path', $parsedLocation) ? $parsedLocation['path'] : '')
+            ->withQuery(array_key_exists('query', $parsedLocation) ? $parsedLocation['query'] : '')
+            ->withFragment(array_key_exists('fragment', $parsedLocation) ? $parsedLocation['fragment'] : '')
+        ;
 
         if (array_key_exists('scheme', $parsedLocation)) {
             $uri = $uri->withScheme($parsedLocation['scheme']);
@@ -244,22 +249,6 @@ private function createUri(ResponseInterface $redirectResponse, RequestInterface
             $uri = $uri->withPort($parsedLocation['port']);
         }
 
-        if (array_key_exists('path', $parsedLocation)) {
-            $uri = $uri->withPath($parsedLocation['path']);
-        }
-
-        if (array_key_exists('query', $parsedLocation)) {
-            $uri = $uri->withQuery($parsedLocation['query']);
-        } else {
-            $uri = $uri->withQuery('');
-        }
-
-        if (array_key_exists('fragment', $parsedLocation)) {
-            $uri = $uri->withFragment($parsedLocation['fragment']);
-        } else {
-            $uri = $uri->withFragment('');
-        }
-
         return $uri;
     }
 }
diff --git a/tests/Plugin/RedirectPluginTest.php b/tests/Plugin/RedirectPluginTest.php
@@ -0,0 +1,48 @@
+<?php
+declare(strict_types=1);
+
+namespace Plugin;
+
+use Http\Client\Common\Exception\CircularRedirectionException;
+use Http\Client\Common\Plugin\RedirectPlugin;
+use Http\Promise\FulfilledPromise;
+use Nyholm\Psr7\Request;
+use Nyholm\Psr7\Response;
+use PHPUnit\Framework\TestCase;
+use Psr\Http\Message\RequestInterface;
+use Psr\Http\Message\ResponseInterface;
+
+class RedirectPluginTest extends TestCase
+{
+    public function testCircularDetection(): void
+    {
+        $this->expectException(CircularRedirectionException::class);
+        (new RedirectPlugin())->handleRequest(
+            new Request('GET', 'https://example.com/path?query=value'),
+            function () {
+                return new FulfilledPromise(new Response(302, ['Location' => 'https://example.com/path?query=value']));
+            },
+            function () {}
+        )->wait();
+    }
+
+    /**
+     * @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"]
+     */
+    public function testTargetUriMappingFromLocationHeader(string $originalUri, string $locationUri, string $targetUri): void
+    {
+        $response = (new RedirectPlugin())->handleRequest(
+            new Request('GET', $originalUri),
+            function () use ($locationUri) {
+                return new FulfilledPromise(new Response(302, ['Location' => $locationUri]));
+            },
+            function (RequestInterface $request) {
+                return new FulfilledPromise(new Response(200, ['uri' => $request->getUri()->__toString()]));
+            }
+        )->wait();
+        $this->assertInstanceOf(ResponseInterface::class, $response);
+        $this->assertEquals($targetUri, $response->getHeaderLine('uri'));
+    }
+}