Merge pull request #5 from Innmind/fix-redirections

Fix redirections

CHANGELOG.md

@@ -1,5 +1,11 @@
 # Changelog
 
+## [Unreleased]
+
+### Fixed
+
+- When following redirections (via `FollowRedirections`) if the target url is only a path it crashed because no host was provided, now the target url is resolved based on the previous request
+
 ## 7.2.0 - 2023-12-14
 
 ### Added

src/Curl.php

@@ -3,13 +3,9 @@
 
 namespace Innmind\HttpTransport;
 
-use Innmind\HttpTransport\{
-    Curl\Scheduled,
-    Curl\Ready,
-    Curl\Concurrency,
-    Transport,
-    Failure,
-    Success,
+use Innmind\HttpTransport\Curl\{
+    Scheduled,
+    Concurrency
 };
 use Innmind\Http\{
     Request,
@@ -26,11 +22,7 @@
     Capabilities,
     Streams,
 };
-use Innmind\Immutable\{
-    Either,
-    Str,
-    Sequence,
-};
+use Innmind\Immutable\Either;
 
 /**
  * @psalm-import-type Errors from Transport

src/Curl/Ready.php

@@ -27,7 +27,6 @@
 use Innmind\Filesystem\File\Content;
 use Innmind\IO\IO;
 use Innmind\Stream\{
-    Readable,
     Writable,
     Bidirectional,
 };

src/Curl/Scheduled.php

@@ -27,7 +27,6 @@
 use Innmind\Immutable\{
     Either,
     Str,
-    Sequence,
 };
 
 /**

src/ExponentialBackoff.php

@@ -3,11 +3,7 @@
 
 namespace Innmind\HttpTransport;
 
-use Innmind\Http\{
-    Request,
-    Response,
-    Response\StatusCode,
-};
+use Innmind\Http\Request;
 use Innmind\TimeContinuum\{
     Period,
     Earth\Period\Millisecond,

src/FollowRedirections.php

@@ -9,6 +9,10 @@
     Response\StatusCode,
     Header\Location,
 };
+use Innmind\Url\{
+    Url,
+    Authority,
+};
 use Innmind\Immutable\{
     Either,
     Sequence,
@@ -93,7 +97,7 @@ private function redirect(Redirection $redirection): Either
             ->find(Location::class)
             ->map(static fn($header) => $header->url())
             ->map(static fn($url) => Request::of(
-                $url,
+                self::resolveUrl($redirection->request()->url(), $url),
                 $redirection->request()->method(),
                 $redirection->request()->protocolVersion(),
                 $redirection->request()->headers(),
@@ -122,7 +126,7 @@ private function seeOther(Redirection $redirection): Either
             ->find(Location::class)
             ->map(static fn($header) => $header->url())
             ->map(static fn($url) => Request::of(
-                $url,
+                self::resolveUrl($redirection->request()->url(), $url),
                 Method::get,
                 $redirection->request()->protocolVersion(),
                 $redirection->request()->headers(),
@@ -132,4 +136,17 @@ private function seeOther(Redirection $redirection): Either
                 static fn() => Either::left($redirection),
             );
     }
+
+    private static function resolveUrl(Url $request, Url $location): Url
+    {
+        if ($location->authority()->equals(Authority::none())) {
+            $location = $location
+                ->withScheme($request->scheme())
+                ->withAuthority($request->authority());
+        }
+
+        return $location->withPath(
+            $request->path()->resolve($location->path()),
+        );
+    }
 }

src/Logger.php

@@ -8,7 +8,6 @@
     Response,
     Headers,
     Header,
-    Header\Value,
 };
 use Innmind\Immutable\{
     Either,

src/Transport.php

@@ -3,10 +3,7 @@
 
 namespace Innmind\HttpTransport;
 
-use Innmind\Http\{
-    Request,
-    Response,
-};
+use Innmind\Http\Request;
 use Innmind\Immutable\Either;
 
 /**

tests/FollowRedirectionsTest.php

@@ -25,7 +25,10 @@
     Header\Location,
 };
 use Innmind\Filesystem\File\Content;
-use Innmind\Url\Url;
+use Innmind\Url\{
+    Url,
+    Authority,
+};
 use Innmind\Immutable\Either;
 use PHPUnit\Framework\TestCase;
 use Innmind\BlackBox\{
@@ -149,10 +152,11 @@ public function testRedirectMaximum5Times()
                 $inner
                     ->expects($matcher = $this->exactly(6))
                     ->method('__invoke')
-                    ->willReturnCallback(function($request) use ($matcher, $start, $newUrl, $expected) {
+                    ->willReturnCallback(function($request) use ($matcher, $start, $firstUrl, $expected) {
                         match ($matcher->numberOfInvocations()) {
                             1 => $this->assertSame($start, $request),
-                            default => $this->assertSame($newUrl, $request->url()),
+                            // assertions on the new url are done in self::testRedirect() and self::testRedirectSeeOther()
+                            default => $this->assertNotSame($firstUrl, $request->url()),
                         };
 
                         return $expected;
@@ -214,7 +218,9 @@ public function testRedirectSeeOther()
     {
         $this
             ->forAll(
-                FUrl::any(),
+                FUrl::any()
+                    ->filter(static fn($url) => !$url->authority()->equals(Authority::none()))
+                    ->filter(static fn($url) => $url->path()->absolute()),
                 FUrl::any(),
                 Set\Elements::of(...Method::cases()),
                 Set\Elements::of(
@@ -248,7 +254,15 @@ public function testRedirectSeeOther()
                             $this->assertSame($start, $request);
                         } else {
                             $this->assertSame(Method::get, $request->method());
-                            $this->assertSame($newUrl, $request->url());
+                            $this->assertFalse($request->url()->authority()->equals(Authority::none()));
+                            $this->assertTrue($request->url()->path()->absolute());
+                            // not a direct comparison as new url might be a relative path
+                            $this->assertStringEndsWith(
+                                $newUrl->path()->toString(),
+                                $request->url()->path()->toString(),
+                            );
+                            $this->assertSame($newUrl->query(), $request->url()->query());
+                            $this->assertSame($newUrl->fragment(), $request->url()->fragment());
                             $this->assertSame($start->headers(), $request->headers());
                             $this->assertSame('', $request->body()->toString());
                         }
@@ -279,7 +293,9 @@ public function testRedirect()
     {
         $this
             ->forAll(
-                FUrl::any(),
+                FUrl::any()
+                    ->filter(static fn($url) => !$url->authority()->equals(Authority::none()))
+                    ->filter(static fn($url) => $url->path()->absolute()),
                 FUrl::any(),
                 Set\Elements::of(Method::get, Method::head), // unsafe methods are not redirected
                 Set\Elements::of(
@@ -319,7 +335,15 @@ public function testRedirect()
                             $this->assertSame($start, $request);
                         } else {
                             $this->assertSame($start->method(), $request->method());
-                            $this->assertSame($newUrl, $request->url());
+                            $this->assertFalse($request->url()->authority()->equals(Authority::none()));
+                            $this->assertTrue($request->url()->path()->absolute());
+                            // not a direct comparison as new url might be a relative path
+                            $this->assertStringEndsWith(
+                                $newUrl->path()->toString(),
+                                $request->url()->path()->toString(),
+                            );
+                            $this->assertSame($newUrl->query(), $request->url()->query());
+                            $this->assertSame($newUrl->fragment(), $request->url()->fragment());
                             $this->assertSame($start->headers(), $request->headers());
                             $this->assertSame($start->body(), $request->body());
                         }