From 199624794c376f1516f717c27a0765aaa624dcb8 Mon Sep 17 00:00:00 2001 From: Baptiste Langlade Date: Fri, 1 Mar 2024 14:21:55 +0100 Subject: [PATCH 1/6] make sure the initial request has an authority --- tests/FollowRedirectionsTest.php | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tests/FollowRedirectionsTest.php b/tests/FollowRedirectionsTest.php index 2d430a4..78eb1b9 100644 --- a/tests/FollowRedirectionsTest.php +++ b/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\{ @@ -279,7 +282,7 @@ public function testRedirect() { $this ->forAll( - FUrl::any(), + FUrl::any()->filter(static fn($url) => !$url->authority()->equals(Authority::none())), FUrl::any(), Set\Elements::of(Method::get, Method::head), // unsafe methods are not redirected Set\Elements::of( From 70a109640b3131ad63940084a5f3fb5028f88f59 Mon Sep 17 00:00:00 2001 From: Baptiste Langlade Date: Fri, 1 Mar 2024 14:23:21 +0100 Subject: [PATCH 2/6] CS --- src/Curl.php | 16 ++++------------ src/Curl/Ready.php | 1 - src/Curl/Scheduled.php | 1 - src/ExponentialBackoff.php | 6 +----- src/Logger.php | 1 - src/Transport.php | 5 +---- 6 files changed, 6 insertions(+), 24 deletions(-) diff --git a/src/Curl.php b/src/Curl.php index b1320b0..b7f9ff4 100644 --- a/src/Curl.php +++ b/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 diff --git a/src/Curl/Ready.php b/src/Curl/Ready.php index 8125f6a..920e0bb 100644 --- a/src/Curl/Ready.php +++ b/src/Curl/Ready.php @@ -27,7 +27,6 @@ use Innmind\Filesystem\File\Content; use Innmind\IO\IO; use Innmind\Stream\{ - Readable, Writable, Bidirectional, }; diff --git a/src/Curl/Scheduled.php b/src/Curl/Scheduled.php index c8736d4..2fa6374 100644 --- a/src/Curl/Scheduled.php +++ b/src/Curl/Scheduled.php @@ -27,7 +27,6 @@ use Innmind\Immutable\{ Either, Str, - Sequence, }; /** diff --git a/src/ExponentialBackoff.php b/src/ExponentialBackoff.php index 0c5fdfa..edb81d0 100644 --- a/src/ExponentialBackoff.php +++ b/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, diff --git a/src/Logger.php b/src/Logger.php index f7ce2b9..66d7d38 100644 --- a/src/Logger.php +++ b/src/Logger.php @@ -8,7 +8,6 @@ Response, Headers, Header, - Header\Value, }; use Innmind\Immutable\{ Either, diff --git a/src/Transport.php b/src/Transport.php index 43734e9..227e16d 100644 --- a/src/Transport.php +++ b/src/Transport.php @@ -3,10 +3,7 @@ namespace Innmind\HttpTransport; -use Innmind\Http\{ - Request, - Response, -}; +use Innmind\Http\Request; use Innmind\Immutable\Either; /** From af5d7e35f0f160d780044ad032c365f2dd941790 Mon Sep 17 00:00:00 2001 From: Baptiste Langlade Date: Fri, 1 Mar 2024 14:43:12 +0100 Subject: [PATCH 3/6] make sure there is always an authority when following redirections --- src/FollowRedirections.php | 21 +++++++++++++++++++-- tests/FollowRedirectionsTest.php | 25 +++++++++++++++++++++---- 2 files changed, 40 insertions(+), 6 deletions(-) diff --git a/src/FollowRedirections.php b/src/FollowRedirections.php index 32a4aef..ae63ccf 100644 --- a/src/FollowRedirections.php +++ b/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()), + ); + } } diff --git a/tests/FollowRedirectionsTest.php b/tests/FollowRedirectionsTest.php index 78eb1b9..7c4b4f0 100644 --- a/tests/FollowRedirectionsTest.php +++ b/tests/FollowRedirectionsTest.php @@ -152,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; @@ -251,7 +252,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()); } @@ -322,7 +331,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()); } From 3935e6839c0f1defe5fcd923a74080c9bd48cc0c Mon Sep 17 00:00:00 2001 From: Baptiste Langlade Date: Fri, 1 Mar 2024 14:52:46 +0100 Subject: [PATCH 4/6] make sure the initial request url has an authority --- tests/FollowRedirectionsTest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/FollowRedirectionsTest.php b/tests/FollowRedirectionsTest.php index 7c4b4f0..4354a57 100644 --- a/tests/FollowRedirectionsTest.php +++ b/tests/FollowRedirectionsTest.php @@ -218,7 +218,7 @@ public function testRedirectSeeOther() { $this ->forAll( - FUrl::any(), + FUrl::any()->filter(static fn($url) => !$url->authority()->equals(Authority::none())), FUrl::any(), Set\Elements::of(...Method::cases()), Set\Elements::of( From 36dcc42b182e7510e2f79e81baf08e99b6828efb Mon Sep 17 00:00:00 2001 From: Baptiste Langlade Date: Fri, 1 Mar 2024 14:56:55 +0100 Subject: [PATCH 5/6] make sure the initial request path is absolute --- tests/FollowRedirectionsTest.php | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/tests/FollowRedirectionsTest.php b/tests/FollowRedirectionsTest.php index 4354a57..d34901d 100644 --- a/tests/FollowRedirectionsTest.php +++ b/tests/FollowRedirectionsTest.php @@ -218,7 +218,9 @@ public function testRedirectSeeOther() { $this ->forAll( - FUrl::any()->filter(static fn($url) => !$url->authority()->equals(Authority::none())), + 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( @@ -291,7 +293,9 @@ public function testRedirect() { $this ->forAll( - FUrl::any()->filter(static fn($url) => !$url->authority()->equals(Authority::none())), + 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( From 0d50715b18a9ee44eee1b0f7eb4ef59b42e9c56c Mon Sep 17 00:00:00 2001 From: Baptiste Langlade Date: Fri, 1 Mar 2024 15:03:46 +0100 Subject: [PATCH 6/6] update changelog --- CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 323458c..6ea50fb 100644 --- a/CHANGELOG.md +++ b/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