From 3da658e52fea361c78d0be6cd455214ba53f955b Mon Sep 17 00:00:00 2001 From: Zhuk Sergey Date: Tue, 23 Jan 2018 17:52:37 +0300 Subject: [PATCH 1/3] Fix request headers when following redirect --- src/Io/Transaction.php | 2 +- tests/Io/TransactionTest.php | 27 +++++++++++++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/src/Io/Transaction.php b/src/Io/Transaction.php index 4ec3b1e..6fceb97 100644 --- a/src/Io/Transaction.php +++ b/src/Io/Transaction.php @@ -133,7 +133,7 @@ private function onResponseRedirect(ResponseInterface $response, RequestInterfac // naïve approach.. $method = ($request->getMethod() === 'HEAD') ? 'HEAD' : 'GET'; - $request = $this->messageFactory->request($method, $location); + $request = $this->messageFactory->request($method, $location, $request->getHeaders()); $this->progress('redirect', array($request)); diff --git a/tests/Io/TransactionTest.php b/tests/Io/TransactionTest.php index 0566255..1b3e79c 100644 --- a/tests/Io/TransactionTest.php +++ b/tests/Io/TransactionTest.php @@ -2,6 +2,7 @@ use Clue\React\Buzz\Io\Transaction; use Clue\React\Buzz\Message\ResponseException; +use Psr\Http\Message\RequestInterface; use RingCentral\Psr7\Response; use Clue\React\Buzz\Message\MessageFactory; use React\Promise; @@ -104,4 +105,30 @@ public function testReceivingStreamingBodyWillResolveWithStreamingResponseIfStre $this->assertEquals(200, $response->getStatusCode()); $this->assertEquals('', (string)$response->getBody()); } + + public function testFollowingRedirectWithSpecifiedHeaders() + { + $messageFactory = new MessageFactory(); + + $requestWithUserAgent = $messageFactory->request('GET', 'http://example.com', ['User-Agent' => 'Chrome']); + $sender = $this->getMockBuilder('Clue\React\Buzz\Io\Sender')->disableOriginalConstructor()->getMock(); + + // mock sender to resolve promise with the given $redirectResponse in + // response to the given $requestWithUserAgent + $redirectResponse = $messageFactory->response(1.0, 301, null); + $sender->expects($this->at(0))->method('send')->willReturn(Promise\resolve($redirectResponse)); + + // mock sender to resolve promise with the given $okResponse in + // response to the given $requestWithUserAgent + $okResponse = $messageFactory->response(1.0, 200, 'OK'); + $sender->expects($this->at(1))->method('send') + ->with($this->callback(function(RequestInterface $request){ + $this->assertEquals(['Chrome'], $request->getHeader('User-Agent')); + return true; + })) + ->willReturn(Promise\resolve($okResponse)); + + $transaction = new Transaction($requestWithUserAgent, $sender, array(), $messageFactory); + $transaction->send(); + } } From dcb963a9f904f4acca3538c41b48fc8122eabaed Mon Sep 17 00:00:00 2001 From: Zhuk Sergey Date: Tue, 23 Jan 2018 22:02:31 +0300 Subject: [PATCH 2/3] Fix custom request headers forwarding when redirect --- src/Io/Transaction.php | 29 +++++++-- tests/Io/TransactionTest.php | 120 +++++++++++++++++++++++++++++++---- 2 files changed, 133 insertions(+), 16 deletions(-) diff --git a/src/Io/Transaction.php b/src/Io/Transaction.php index 6fceb97..3e9c96f 100644 --- a/src/Io/Transaction.php +++ b/src/Io/Transaction.php @@ -8,6 +8,7 @@ use Clue\React\Buzz\Message\MessageFactory; use Psr\Http\Message\RequestInterface; use Psr\Http\Message\ResponseInterface; +use Psr\Http\Message\UriInterface; use React\Promise; use React\Promise\Stream; use React\Stream\ReadableStreamInterface; @@ -131,10 +132,7 @@ private function onResponseRedirect(ResponseInterface $response, RequestInterfac // resolve location relative to last request URI $location = $this->messageFactory->uriRelative($request->getUri(), $response->getHeaderLine('Location')); - // naïve approach.. - $method = ($request->getMethod() === 'HEAD') ? 'HEAD' : 'GET'; - $request = $this->messageFactory->request($method, $location, $request->getHeaders()); - + $request = $this->makeRedirectRequest($request, $location); $this->progress('redirect', array($request)); if ($this->numRequests >= $this->maxRedirects) { @@ -144,6 +142,29 @@ private function onResponseRedirect(ResponseInterface $response, RequestInterfac return $this->next($request); } + /** + * @param RequestInterface $request + * @param UriInterface $location + * @return \Clue\React\Buzz\Message\RequestInterface + */ + private function makeRedirectRequest(RequestInterface $request, UriInterface $location) + { + $originalHost = $request->getUri()->getHost(); + $request = $request + ->withoutHeader('Host') + ->withoutHeader('Content-Type') + ->withoutHeader('Content-Length'); + + if($location->getHost() !== $originalHost) { + $request = $request->withoutHeader('Authentication'); + } + + // naïve approach.. + $method = ($request->getMethod() === 'HEAD') ? 'HEAD' : 'GET'; + + return $this->messageFactory->request($method, $location, $request->getHeaders()); + } + private function progress($name, array $args = array()) { return; diff --git a/tests/Io/TransactionTest.php b/tests/Io/TransactionTest.php index 1b3e79c..764680e 100644 --- a/tests/Io/TransactionTest.php +++ b/tests/Io/TransactionTest.php @@ -2,6 +2,7 @@ use Clue\React\Buzz\Io\Transaction; use Clue\React\Buzz\Message\ResponseException; +use PHPUnit\Framework\MockObject\MockObject; use Psr\Http\Message\RequestInterface; use RingCentral\Psr7\Response; use Clue\React\Buzz\Message\MessageFactory; @@ -18,7 +19,7 @@ public function testReceivingErrorResponseWillRejectWithResponseException() $response = new Response(404); // mock sender to resolve promise with the given $response in response to the given $request - $sender = $this->getMockBuilder('Clue\React\Buzz\Io\Sender')->disableOriginalConstructor()->getMock(); + $sender = $this->makeSenderMock(); $sender->expects($this->once())->method('send')->with($this->equalTo($request))->willReturn(Promise\resolve($response)); $transaction = new Transaction($request, $sender, array(), new MessageFactory()); @@ -48,7 +49,7 @@ public function testReceivingStreamingBodyWillResolveWithBufferedResponseByDefau $response = $messageFactory->response(1.0, 200, 'OK', array(), $stream); // mock sender to resolve promise with the given $response in response to the given $request - $sender = $this->getMockBuilder('Clue\React\Buzz\Io\Sender')->disableOriginalConstructor()->getMock(); + $sender = $this->makeSenderMock(); $sender->expects($this->once())->method('send')->with($this->equalTo($request))->willReturn(Promise\resolve($response)); $transaction = new Transaction($request, $sender, array(), $messageFactory); @@ -76,7 +77,7 @@ public function testCancelBufferingResponseWillCloseStreamAndReject() $response = $messageFactory->response(1.0, 200, 'OK', array(), $stream); // mock sender to resolve promise with the given $response in response to the given $request - $sender = $this->getMockBuilder('Clue\React\Buzz\Io\Sender')->disableOriginalConstructor()->getMock(); + $sender = $this->makeSenderMock(); $sender->expects($this->once())->method('send')->with($this->equalTo($request))->willReturn(Promise\resolve($response)); $transaction = new Transaction($request, $sender, array(), $messageFactory); @@ -94,7 +95,7 @@ public function testReceivingStreamingBodyWillResolveWithStreamingResponseIfStre $response = $messageFactory->response(1.0, 200, 'OK', array(), $this->getMockBuilder('React\Stream\ReadableStreamInterface')->getMock()); // mock sender to resolve promise with the given $response in response to the given $request - $sender = $this->getMockBuilder('Clue\React\Buzz\Io\Sender')->disableOriginalConstructor()->getMock(); + $sender = $this->makeSenderMock(); $sender->expects($this->once())->method('send')->with($this->equalTo($request))->willReturn(Promise\resolve($response)); $transaction = new Transaction($request, $sender, array('streaming' => true), $messageFactory); @@ -110,25 +111,120 @@ public function testFollowingRedirectWithSpecifiedHeaders() { $messageFactory = new MessageFactory(); - $requestWithUserAgent = $messageFactory->request('GET', 'http://example.com', ['User-Agent' => 'Chrome']); - $sender = $this->getMockBuilder('Clue\React\Buzz\Io\Sender')->disableOriginalConstructor()->getMock(); + $customHeaders = ['User-Agent' => 'Chrome']; + $requestWithUserAgent = $messageFactory->request('GET', 'http://example.com', $customHeaders); + $sender = $this->makeSenderMock(); // mock sender to resolve promise with the given $redirectResponse in // response to the given $requestWithUserAgent - $redirectResponse = $messageFactory->response(1.0, 301, null); + $redirectResponse = $messageFactory->response(1.0, 301, null, ['Location' => 'http://redirect.com']); $sender->expects($this->at(0))->method('send')->willReturn(Promise\resolve($redirectResponse)); // mock sender to resolve promise with the given $okResponse in // response to the given $requestWithUserAgent $okResponse = $messageFactory->response(1.0, 200, 'OK'); - $sender->expects($this->at(1))->method('send') - ->with($this->callback(function(RequestInterface $request){ + $sender->expects($this->at(1)) + ->method('send') + ->with($this->callback(function (RequestInterface $request) { $this->assertEquals(['Chrome'], $request->getHeader('User-Agent')); return true; - })) - ->willReturn(Promise\resolve($okResponse)); + }))->willReturn(Promise\resolve($okResponse)); - $transaction = new Transaction($requestWithUserAgent, $sender, array(), $messageFactory); + $transaction = new Transaction($requestWithUserAgent, $sender, [], $messageFactory); $transaction->send(); } + + public function testRemovingAuthorizationHeaderWhenChangingHostnamesDuringRedirect() + { + $messageFactory = new MessageFactory(); + + $customHeaders = ['Authentication' => 'secret']; + $requestWithAuthentication = $messageFactory->request('GET', 'http://example.com', $customHeaders); + $sender = $this->makeSenderMock(); + + // mock sender to resolve promise with the given $redirectResponse in + // response to the given $requestWithAuthentication + $redirectResponse = $messageFactory->response(1.0, 301, null, ['Location' => 'http://redirect.com']); + $sender->expects($this->at(0))->method('send')->willReturn(Promise\resolve($redirectResponse)); + + // mock sender to resolve promise with the given $okResponse in + // response to the given $requestWithAuthentication + $okResponse = $messageFactory->response(1.0, 200, 'OK'); + $sender->expects($this->at(1)) + ->method('send') + ->with($this->callback(function (RequestInterface $request) { + $this->assertFalse($request->hasHeader('Authentication')); + return true; + }))->willReturn(Promise\resolve($okResponse)); + + $transaction = new Transaction($requestWithAuthentication, $sender, [], $messageFactory); + $transaction->send(); + } + + public function testAuthorizationHeaderIsForwardedWhenRedirectingToSameDomain() + { + $messageFactory = new MessageFactory(); + + $customHeaders = ['Authentication' => 'secret']; + $requestWithAuthentication = $messageFactory->request('GET', 'http://example.com', $customHeaders); + $sender = $this->makeSenderMock(); + + // mock sender to resolve promise with the given $redirectResponse in + // response to the given $requestWithAuthentication + $redirectResponse = $messageFactory->response(1.0, 301, null, ['Location' => 'http://example.com/new']); + $sender->expects($this->at(0))->method('send')->willReturn(Promise\resolve($redirectResponse)); + + // mock sender to resolve promise with the given $okResponse in + // response to the given $requestWithAuthentication + $okResponse = $messageFactory->response(1.0, 200, 'OK'); + $sender->expects($this->at(1)) + ->method('send') + ->with($this->callback(function (RequestInterface $request) { + $this->assertEquals(['secret'], $request->getHeader('Authentication')); + return true; + }))->willReturn(Promise\resolve($okResponse)); + + $transaction = new Transaction($requestWithAuthentication, $sender, [], $messageFactory); + $transaction->send(); + } + + public function testSomeRequestHeadersShouldBeRemovedWhenRedirecting() + { + $messageFactory = new MessageFactory(); + + $customHeaders = [ + 'Content-Type' => 'text/html; charset=utf-8', + 'Content-Length' => '111', + ]; + + $requestWithCustomHeaders = $messageFactory->request('GET', 'http://example.com', $customHeaders); + $sender = $this->makeSenderMock(); + + // mock sender to resolve promise with the given $redirectResponse in + // response to the given $requestWithCustomHeaders + $redirectResponse = $messageFactory->response(1.0, 301, null, ['Location' => 'http://example.com/new']); + $sender->expects($this->at(0))->method('send')->willReturn(Promise\resolve($redirectResponse)); + + // mock sender to resolve promise with the given $okResponse in + // response to the given $requestWithCustomHeaders + $okResponse = $messageFactory->response(1.0, 200, 'OK'); + $sender->expects($this->at(1)) + ->method('send') + ->with($this->callback(function (RequestInterface $request) { + $this->assertFalse($request->hasHeader('Content-Type')); + $this->assertFalse($request->hasHeader('Content-Length')); + return true; + }))->willReturn(Promise\resolve($okResponse)); + + $transaction = new Transaction($requestWithCustomHeaders, $sender, [], $messageFactory); + $transaction->send(); + } + + /** + * @return MockObject + */ + private function makeSenderMock() + { + return $this->getMockBuilder('Clue\React\Buzz\Io\Sender')->disableOriginalConstructor()->getMock(); + } } From 1f3466f7fe71cf13770d5ab85f8e0c98a32ccdcd Mon Sep 17 00:00:00 2001 From: Zhuk Sergey Date: Wed, 24 Jan 2018 08:50:52 +0300 Subject: [PATCH 3/3] Add comments to redirect request building --- src/Io/Transaction.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Io/Transaction.php b/src/Io/Transaction.php index 3e9c96f..90d4835 100644 --- a/src/Io/Transaction.php +++ b/src/Io/Transaction.php @@ -155,7 +155,8 @@ private function makeRedirectRequest(RequestInterface $request, UriInterface $lo ->withoutHeader('Content-Type') ->withoutHeader('Content-Length'); - if($location->getHost() !== $originalHost) { + // Remove authorization if changing hostnames (but not if just changing ports or protocols). + if ($location->getHost() !== $originalHost) { $request = $request->withoutHeader('Authentication'); }