From a48935c90a01cf2fa4e7715c76a16ec3f39082e8 Mon Sep 17 00:00:00 2001 From: David Buchmann Date: Thu, 29 Sep 2022 09:48:13 +0200 Subject: [PATCH] remove body on redirection --- CHANGELOG.md | 7 +++ phpstan.neon.dist | 12 +++++ spec/Plugin/RedirectPluginSpec.php | 9 +++- src/Plugin/RedirectPlugin.php | 68 +++++++++++++++++++++++++++-- tests/Plugin/RedirectPluginTest.php | 53 +++++++++++++++++++++- 5 files changed, 144 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 020d740..e795fce 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,12 @@ # Change Log +## 2.6.0 - 2022-09-29 + +- [RedirectPlugin] Redirection of non GET/HEAD requests with a body now removes the body on follow-up requests, if the + HTTP method changes. To do this, the plugin needs to find a PSR-7 stream implementation. If none is found, you can + explicitly pass a PSR-17 StreamFactoryInterface in the `stream_factory` option. + To keep sending the body in all cases, set the `stream_factory` option to null explicitly. + ## 2.5.1 - 2022-09-29 ### Fixed diff --git a/phpstan.neon.dist b/phpstan.neon.dist index 328ca33..236087f 100644 --- a/phpstan.neon.dist +++ b/phpstan.neon.dist @@ -36,6 +36,18 @@ parameters: count: 1 path: src/Plugin/RedirectPlugin.php + # phpstan is confused by the optional dependencies. we check for existence first + - + message: "#^Method Http\\\\Client\\\\Common\\\\Plugin\\\\RedirectPlugin::guessStreamFactory\\(\\) should return Psr\\\\Http\\\\Message\\\\StreamFactoryInterface\\|null but returns Nyholm\\\\Psr7\\\\Factory\\\\Psr17Factory\\.$#" + count: 1 + path: src/Plugin/RedirectPlugin.php + + # phpstan is confused by the optional dependencies. we check for existence first + - + message: "#^Call to static method streamFor\\(\\) on an unknown class GuzzleHttp\\\\Psr7\\\\Utils\\.$#" + count: 1 + path: src/Plugin/RedirectPlugin.php + - message: "#^Method Http\\\\Client\\\\Common\\\\Plugin\\\\RetryPlugin\\:\\:retry\\(\\) should return Psr\\\\Http\\\\Message\\\\ResponseInterface but returns mixed\\.$#" count: 1 diff --git a/spec/Plugin/RedirectPluginSpec.php b/spec/Plugin/RedirectPluginSpec.php index a8df45c..9bcfb7f 100644 --- a/spec/Plugin/RedirectPluginSpec.php +++ b/spec/Plugin/RedirectPluginSpec.php @@ -37,6 +37,7 @@ public function it_redirects_on_302( ResponseInterface $finalResponse, Promise $promise ) { + $this->beConstructedWith(['stream_factory' => null]); $responseRedirect->getStatusCode()->willReturn(302); $responseRedirect->hasHeader('Location')->willReturn(true); $responseRedirect->getHeaderLine('Location')->willReturn('/redirect'); @@ -81,6 +82,7 @@ public function it_use_storage_on_301( ResponseInterface $finalResponse, ResponseInterface $redirectResponse ) { + $this->beConstructedWith(['stream_factory' => null]); $request->getUri()->willReturn($uri); $uri->__toString()->willReturn('/original'); $uri->withPath('/redirect')->willReturn($uriRedirect); @@ -153,6 +155,7 @@ public function it_replace_full_url( ResponseInterface $finalResponse, Promise $promise ) { + $this->beConstructedWith(['stream_factory' => null]); $request->getUri()->willReturn($uri); $uri->__toString()->willReturn('/original'); @@ -275,6 +278,7 @@ public function it_switch_method_for_302( ResponseInterface $finalResponse, Promise $promise ) { + $this->beConstructedWith(['stream_factory' => null]); $request->getUri()->willReturn($uri); $uri->__toString()->willReturn('/original'); @@ -367,7 +371,10 @@ public function it_clears_headers( ResponseInterface $finalResponse, Promise $promise ) { - $this->beConstructedWith(['preserve_header' => ['Accept']]); + $this->beConstructedWith([ + 'preserve_header' => ['Accept'], + 'stream_factory' => null, + ]); $request->getUri()->willReturn($uri); $uri->__toString()->willReturn('/original'); diff --git a/src/Plugin/RedirectPlugin.php b/src/Plugin/RedirectPlugin.php index 7b96b1b..ee5c232 100644 --- a/src/Plugin/RedirectPlugin.php +++ b/src/Plugin/RedirectPlugin.php @@ -4,14 +4,20 @@ namespace Http\Client\Common\Plugin; +use GuzzleHttp\Psr7\Utils; use Http\Client\Common\Exception\CircularRedirectionException; use Http\Client\Common\Exception\MultipleRedirectionException; use Http\Client\Common\Plugin; use Http\Client\Exception\HttpException; +use Http\Discovery\Psr17FactoryDiscovery; use Http\Promise\Promise; +use Nyholm\Psr7\Factory\Psr17Factory; use Psr\Http\Message\RequestInterface; use Psr\Http\Message\ResponseInterface; +use Psr\Http\Message\StreamFactoryInterface; +use Psr\Http\Message\StreamInterface; use Psr\Http\Message\UriInterface; +use Symfony\Component\OptionsResolver\Options; use Symfony\Component\OptionsResolver\OptionsResolver; /** @@ -101,6 +107,11 @@ final class RedirectPlugin implements Plugin */ private $circularDetection = []; + /** + * @var StreamFactoryInterface|null + */ + private $streamFactory; + /** * @param array{'preserve_header'?: bool|string[], 'use_default_for_multiple'?: bool, 'strict'?: bool} $config * @@ -108,6 +119,7 @@ final class RedirectPlugin implements Plugin * - 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 + * - stream_factory: If set, must be a PSR-17 StreamFactoryInterface - if not set, we try to discover one */ public function __construct(array $config = []) { @@ -116,10 +128,12 @@ public function __construct(array $config = []) 'preserve_header' => true, 'use_default_for_multiple' => true, 'strict' => false, + 'stream_factory' => null, ]); $resolver->setAllowedTypes('preserve_header', ['bool', 'array']); $resolver->setAllowedTypes('use_default_for_multiple', 'bool'); $resolver->setAllowedTypes('strict', 'bool'); + $resolver->setAllowedTypes('stream_factory', [StreamFactoryInterface::class, 'null']); $resolver->setNormalizer('preserve_header', function (OptionsResolver $resolver, $value) { if (is_bool($value) && false === $value) { return []; @@ -127,6 +141,9 @@ public function __construct(array $config = []) return $value; }); + $resolver->setDefault('stream_factory', function (Options $options): ?StreamFactoryInterface { + return $this->guessStreamFactory(); + }); $options = $resolver->resolve($config); $this->preserveHeader = $options['preserve_header']; @@ -137,6 +154,8 @@ public function __construct(array $config = []) $this->redirectCodes[301]['switch'] = false; $this->redirectCodes[302]['switch'] = false; } + + $this->streamFactory = $options['stream_factory']; } /** @@ -170,7 +189,7 @@ public function handleRequest(RequestInterface $request, callable $next, callabl $this->circularDetection[$chainIdentifier][] = (string) $request->getUri(); - if (in_array((string) $redirectRequest->getUri(), $this->circularDetection[$chainIdentifier])) { + if (in_array((string) $redirectRequest->getUri(), $this->circularDetection[$chainIdentifier], true)) { throw new CircularRedirectionException('Circular redirection detected', $request, $response); } @@ -186,19 +205,62 @@ public function handleRequest(RequestInterface $request, callable $next, callabl }); } + /** + * The default only needs to be determined if no value is provided. + */ + public function guessStreamFactory(): ?StreamFactoryInterface + { + if (class_exists(Psr17FactoryDiscovery::class)) { + try { + return Psr17FactoryDiscovery::findStreamFactory(); + } catch (\Throwable $t) { + // ignore and try other options + } + } + if (class_exists(Psr17Factory::class)) { + return new Psr17Factory(); + } + if (class_exists(Utils::class)) { + return new class() implements StreamFactoryInterface { + public function createStream(string $content = ''): StreamInterface + { + return Utils::streamFor($content); + } + + public function createStreamFromFile(string $filename, string $mode = 'r'): StreamInterface + { + throw new \RuntimeException('Internal error: this method should not be needed'); + } + + public function createStreamFromResource($resource): StreamInterface + { + throw new \RuntimeException('Internal error: this method should not be needed'); + } + }; + } + + return null; + } + private function buildRedirectRequest(RequestInterface $originalRequest, UriInterface $targetUri, int $statusCode): RequestInterface { $originalRequest = $originalRequest->withUri($targetUri); - if (false !== $this->redirectCodes[$statusCode]['switch'] && !in_array($originalRequest->getMethod(), $this->redirectCodes[$statusCode]['switch']['unless'])) { + if (false !== $this->redirectCodes[$statusCode]['switch'] && !in_array($originalRequest->getMethod(), $this->redirectCodes[$statusCode]['switch']['unless'], true)) { $originalRequest = $originalRequest->withMethod($this->redirectCodes[$statusCode]['switch']['to']); + if ('GET' === $this->redirectCodes[$statusCode]['switch']['to'] && $this->streamFactory) { + // if we found a stream factory, remove the request body. otherwise leave the body there. + $originalRequest = $originalRequest->withoutHeader('content-type'); + $originalRequest = $originalRequest->withoutHeader('content-length'); + $originalRequest = $originalRequest->withBody($this->streamFactory->createStream()); + } } if (is_array($this->preserveHeader)) { $headers = array_keys($originalRequest->getHeaders()); foreach ($headers as $name) { - if (!in_array($name, $this->preserveHeader)) { + if (!in_array($name, $this->preserveHeader, true)) { $originalRequest = $originalRequest->withoutHeader($name); } } diff --git a/tests/Plugin/RedirectPluginTest.php b/tests/Plugin/RedirectPluginTest.php index 5e26703..9211bee 100644 --- a/tests/Plugin/RedirectPluginTest.php +++ b/tests/Plugin/RedirectPluginTest.php @@ -7,6 +7,7 @@ use Http\Client\Common\Exception\CircularRedirectionException; use Http\Client\Common\Plugin\RedirectPlugin; use Http\Promise\FulfilledPromise; +use Nyholm\Psr7\Factory\Psr17Factory; use Nyholm\Psr7\Request; use Nyholm\Psr7\Response; use PHPUnit\Framework\TestCase; @@ -27,6 +28,56 @@ function () {} )->wait(); } + public function testPostGetDropRequestBody(): void + { + $response = (new RedirectPlugin())->handleRequest( + new Request('POST', 'https://example.com/path', ['Content-Type' => 'text/plain', 'Content-Length' => '10'], (new Psr17Factory())->createStream('hello test')), + function (RequestInterface $request) { + $this->assertSame(10, $request->getBody()->getSize()); + $this->assertTrue($request->hasHeader('Content-Type')); + $this->assertTrue($request->hasHeader('Content-Length')); + + return new FulfilledPromise(new Response(302, ['Location' => 'https://example.com/other'])); + }, + function (RequestInterface $request) { + $this->assertSame('GET', $request->getMethod()); + $this->assertSame(0, $request->getBody()->getSize()); + $this->assertFalse($request->hasHeader('Content-Type')); + $this->assertFalse($request->hasHeader('Content-Length')); + + return new FulfilledPromise(new Response(200, ['uri' => $request->getUri()->__toString()])); + } + )->wait(); + + $this->assertSame('https://example.com/other', $response->getHeaderLine('uri')); + } + + public function testPostGetNoFactory(): void + { + // We explicitly set the stream factory to null. Same happens if no factory can be found. + // In this case, the redirect will leave the body alone. + $response = (new RedirectPlugin(['stream_factory' => null]))->handleRequest( + new Request('POST', 'https://example.com/path', ['Content-Type' => 'text/plain', 'Content-Length' => '10'], (new Psr17Factory())->createStream('hello test')), + function (RequestInterface $request) { + $this->assertSame(10, $request->getBody()->getSize()); + $this->assertTrue($request->hasHeader('Content-Type')); + $this->assertTrue($request->hasHeader('Content-Length')); + + return new FulfilledPromise(new Response(302, ['Location' => 'https://example.com/other'])); + }, + function (RequestInterface $request) { + $this->assertSame('GET', $request->getMethod()); + $this->assertSame(10, $request->getBody()->getSize()); + $this->assertTrue($request->hasHeader('Content-Type')); + $this->assertTrue($request->hasHeader('Content-Length')); + + return new FulfilledPromise(new Response(200, ['uri' => $request->getUri()->__toString()])); + } + )->wait(); + + $this->assertSame('https://example.com/other', $response->getHeaderLine('uri')); + } + public function provideRedirections(): array { return [ @@ -60,6 +111,6 @@ function (RequestInterface $request) { } )->wait(); $this->assertInstanceOf(ResponseInterface::class, $response); - $this->assertEquals($targetUri, $response->getHeaderLine('uri')); + $this->assertSame($targetUri, $response->getHeaderLine('uri')); } }