Preserve method on redirect

reactphp · Aug 12, 2022 · c4fdb04 · c4fdb04
1 parent b5a66a4
commit c4fdb04
Show file tree

Hide file tree

Showing 2 changed files with 124 additions and 9 deletions.
diff --git a/src/Io/Transaction.php b/src/Io/Transaction.php
@@ -12,6 +12,7 @@
 use React\Promise\Deferred;
 use React\Promise\PromiseInterface;
 use React\Stream\ReadableStreamInterface;
+use RuntimeException;
 
 /**
  * @internal
@@ -242,7 +243,7 @@ private function onResponseRedirect(ResponseInterface $response, RequestInterfac
         // resolve location relative to last request URI
         $location = Uri::resolve($request->getUri(), $response->getHeaderLine('Location'));
 
-        $request = $this->makeRedirectRequest($request, $location);
+        $request = $this->makeRedirectRequest($request, $location, $response->getStatusCode());
         $this->progress('redirect', array($request));
 
         if ($state->numRequests >= $this->maxRedirects) {
@@ -255,25 +256,38 @@ private function onResponseRedirect(ResponseInterface $response, RequestInterfac
     /**
      * @param RequestInterface $request
      * @param UriInterface $location
+     * @param int $statusCode
      * @return RequestInterface
      */
-    private function makeRedirectRequest(RequestInterface $request, UriInterface $location)
+    private function makeRedirectRequest(RequestInterface $request, UriInterface $location, $statusCode)
     {
         $originalHost = $request->getUri()->getHost();
-        $request = $request
-            ->withoutHeader('Host')
-            ->withoutHeader('Content-Type')
-            ->withoutHeader('Content-Length');
+        $request = $request->withoutHeader('Host');
+
+        if ($statusCode !== 307 || $statusCode !== 308) {
+            $request = $request
+                ->withoutHeader('Content-Type')
+                ->withoutHeader('Content-Length');
+        }
 
         // Remove authorization if changing hostnames (but not if just changing ports or protocols).
         if ($location->getHost() !== $originalHost) {
             $request = $request->withoutHeader('Authorization');
         }
 
-        // naïve approach..
-        $method = ($request->getMethod() === 'HEAD') ? 'HEAD' : 'GET';
+        $body = null;
+        if ($statusCode === 307 || $statusCode === 308) {
+            $method = $request->getMethod();
+            if ($request->getBody() instanceof ReadableStreamInterface) {
+                throw new RuntimeException('Unable to redirect request with streaming body');
+            }
+            $body = $request->getBody();
+        } else {
+            // naïve approach..
+            $method = ($request->getMethod() === 'HEAD') ? 'HEAD' : 'GET';
+        }
 
-        return new Request($method, $location, $request->getHeaders());
+        return new Request($method, $location, $request->getHeaders(), $body);
     }
 
     private function progress($name, array $args = array())

diff --git a/tests/Io/TransactionTest.php b/tests/Io/TransactionTest.php
@@ -671,6 +671,107 @@ public function testSomeRequestHeadersShouldBeRemovedWhenRedirecting()
         $transaction->send($requestWithCustomHeaders);
     }
 
+    public function testRequestMethodShouldBeChangedWhenRedirectingWithSeeOther()
+    {
+        $loop = $this->getMockBuilder('React\EventLoop\LoopInterface')->getMock();
+
+        $customHeaders = array(
+            'Content-Type' => 'text/html; charset=utf-8',
+            'Content-Length' => '111',
+        );
+
+        $request = new Request('POST', 'http://example.com', $customHeaders);
+        $sender = $this->makeSenderMock();
+
+        // mock sender to resolve promise with the given $redirectResponse in
+        // response to the given $request
+        $redirectResponse = new Response(303, array('Location' => 'http://example.com/new'));
+
+        // mock sender to resolve promise with the given $okResponse in
+        // response to the given $request
+        $okResponse = new Response(200);
+        $that = $this;
+        $sender->expects($this->exactly(2))->method('send')->withConsecutive(
+            array($this->anything()),
+            array($this->callback(function (RequestInterface $request) use ($that) {
+                $that->assertEquals('GET', $request->getMethod());
+                return true;
+            }))
+        )->willReturnOnConsecutiveCalls(
+            Promise\resolve($redirectResponse),
+            Promise\resolve($okResponse)
+        );
+
+        $transaction = new Transaction($sender, $loop);
+        $transaction->send($request);
+    }
+
+    public function testRequestMethodAndBodyShouldNotBeChangedWhenRedirectingWith307Or308()
+    {
+        $loop = $this->getMockBuilder('React\EventLoop\LoopInterface')->getMock();
+
+        $customHeaders = array(
+            'Content-Type' => 'text/html; charset=utf-8',
+            'Content-Length' => '111',
+        );
+
+        $request = new Request('POST', 'http://example.com', $customHeaders, '{"key":"value"}');
+        $sender = $this->makeSenderMock();
+
+        // mock sender to resolve promise with the given $redirectResponse in
+        // response to the given $request
+        $redirectResponse = new Response(307, array('Location' => 'http://example.com/new'));
+
+        // mock sender to resolve promise with the given $okResponse in
+        // response to the given $request
+        $okResponse = new Response(200);
+        $that = $this;
+        $sender->expects($this->exactly(2))->method('send')->withConsecutive(
+            array($this->anything()),
+            array($this->callback(function (RequestInterface $request) use ($that) {
+                $that->assertEquals('POST', $request->getMethod());
+                $that->assertEquals('{"key":"value"}', (string)$request->getBody());
+                return true;
+            }))
+        )->willReturnOnConsecutiveCalls(
+            Promise\resolve($redirectResponse),
+            Promise\resolve($okResponse)
+        );
+
+        $transaction = new Transaction($sender, $loop);
+        $transaction->send($request);
+    }
+
+    public function testRedirectingStreamingBodyWith307Or308ShouldThrowCantRedirectStreamException()
+    {
+        $loop = $this->getMockBuilder('React\EventLoop\LoopInterface')->getMock();
+
+        $customHeaders = array(
+            'Content-Type' => 'text/html; charset=utf-8',
+            'Content-Length' => '111',
+        );
+
+        $stream = new ThroughStream();
+        $request = new Request('POST', 'http://example.com', $customHeaders, new ReadableBodyStream($stream));
+        $sender = $this->makeSenderMock();
+
+        // mock sender to resolve promise with the given $redirectResponse in
+        // response to the given $request
+        $redirectResponse = new Response(307, array('Location' => 'http://example.com/new'));
+
+        $sender->expects($this->exactly(1))->method('send')->withConsecutive(
+            array($this->anything())
+        )->willReturnOnConsecutiveCalls(
+            Promise\resolve($redirectResponse)
+        );
+
+        $transaction = new Transaction($sender, $loop);
+        $promise = $transaction->send($request);
+
+        $this->setExpectedException('RuntimeException', 'Unable to redirect request with streaming body');
+        Block\await($promise, $loop);
+    }
+
     public function testCancelTransactionWillCancelRequest()
     {
         $loop = $this->getMockBuilder('React\EventLoop\LoopInterface')->getMock();