Merge pull request #222 from php-http/fix/remove-request-body-on-redirect

remove body on redirection if needed

Author: dbu

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

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

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(https://github.com/%3C/div%3E%3C/code%3E%3C/div%3E%3C/td%3E%3C/tr%3E%3Ctr%20class=%22diff-line-row%22%3E%3Ctd%20data-grid-cell-id=%22diff-f78dc7233024c74dcfd8c90a25c9e8792c8086eb3dc16ec3c5465aea0537e304-153-155-0%22%20data-selected=%22false%22%20role=%22gridcell%22%20style=%22background-color:var(--bgColor-default);text-align:center" tabindex="-1" valign="top" class="focusable-grid-cell diff-line-number position-relative diff-line-number-neutral left-side">153
         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');

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,13 +107,19 @@ 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
      *
      * Configuration options:
      *   - 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,17 +128,22 @@ 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 [];
             }
 
             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);
                 }
             }

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'));
     }
 }