Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HTTP client: Preserve request method and body for 307 Temporary Redirect and 308 Permanent Redirect #442

Merged
merged 1 commit into from
Oct 29, 2022
Merged

HTTP client: Preserve request method and body for 307 Temporary Redirect and 308 Permanent Redirect #442

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -342,9 +342,10 @@ $browser->get($url, $headers)->then(function (Psr\Http\Message\ResponseInterface
Any redirected requests will follow the semantics of the original request and
will include the same request headers as the original request except for those
listed below.
If the original request contained a request body, this request body will never
be passed to the redirected request. Accordingly, each redirected request will
remove any `Content-Length` and `Content-Type` request headers.
If the original request is a temporary (307) or a permanent (308) redirect, request
body and headers will be passed to the redirected request. Otherwise, the request
body will never be passed to the redirected request. Accordingly, each redirected
request will remove any `Content-Length` and `Content-Type` request headers.

If the original request used HTTP authentication with an `Authorization` request
header, this request header will only be passed as part of the redirected
Expand Down
33 changes: 22 additions & 11 deletions src/Io/Transaction.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
use Psr\Http\Message\RequestInterface;
use Psr\Http\Message\ResponseInterface;
use Psr\Http\Message\UriInterface;
use React\Http\Message\Response;
use RingCentral\Psr7\Request;
use RingCentral\Psr7\Uri;
use React\EventLoop\LoopInterface;
Expand Down Expand Up @@ -234,6 +235,8 @@ public function onResponse(ResponseInterface $response, RequestInterface $reques
/**
* @param ResponseInterface $response
* @param RequestInterface $request
* @param Deferred $deferred
* @param ClientRequestState $state
* @return PromiseInterface
* @throws \RuntimeException
*/
Expand All @@ -242,7 +245,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) {
Expand All @@ -255,25 +258,33 @@ private function onResponseRedirect(ResponseInterface $response, RequestInterfac
/**
* @param RequestInterface $request
* @param UriInterface $location
* @param int $statusCode
* @return RequestInterface
* @throws \RuntimeException
*/
dinooo13 marked this conversation as resolved.
Show resolved Hide resolved
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');

// Remove authorization if changing hostnames (but not if just changing ports or protocols).
$originalHost = $request->getUri()->getHost();
if ($location->getHost() !== $originalHost) {
$request = $request->withoutHeader('Authorization');
}

// naïve approach..
$method = ($request->getMethod() === 'HEAD') ? 'HEAD' : 'GET';
$request = $request->withoutHeader('Host')->withUri($location);

if ($statusCode === Response::STATUS_TEMPORARY_REDIRECT || $statusCode === Response::STATUS_PERMANENT_REDIRECT) {
if ($request->getBody() instanceof ReadableStreamInterface) {
throw new \RuntimeException('Unable to redirect request with streaming body');
}
} else {
$request = $request
->withMethod($request->getMethod() === 'HEAD' ? 'HEAD' : 'GET')
->withoutHeader('Content-Type')
->withoutHeader('Content-Length')
->withBody(new EmptyBodyStream());
}

return new Request($method, $location, $request->getHeaders());
return $request;
}

private function progress($name, array $args = array())
Expand Down
118 changes: 117 additions & 1 deletion tests/Io/TransactionTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -663,7 +663,7 @@ public function testSomeRequestHeadersShouldBeRemovedWhenRedirecting()
array($this->callback(function (RequestInterface $request) use ($that) {
$that->assertFalse($request->hasHeader('Content-Type'));
$that->assertFalse($request->hasHeader('Content-Length'));
return true;;
return true;
}))
)->willReturnOnConsecutiveCalls(
Promise\resolve($redirectResponse),
Expand All @@ -674,6 +674,122 @@ 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());
dinooo13 marked this conversation as resolved.
Show resolved Hide resolved
$that->assertFalse($request->hasHeader('Content-Type'));
$that->assertFalse($request->hasHeader('Content-Length'));
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());
dinooo13 marked this conversation as resolved.
Show resolved Hide resolved
$that->assertEquals(
array(
'Content-Type' => array('text/html; charset=utf-8'),
'Content-Length' => array('111'),
'Host' => array('example.com')
),
$request->getHeaders()
);
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->once())->method('send')->withConsecutive(
array($this->anything())
)->willReturnOnConsecutiveCalls(
Promise\resolve($redirectResponse)
);

$transaction = new Transaction($sender, $loop);
$promise = $transaction->send($request);

$exception = null;
$promise->then(null, function ($reason) use (&$exception) {
$exception = $reason;
});

assert($exception instanceof \RuntimeException);
$this->assertEquals('Unable to redirect request with streaming body', $exception->getMessage());
}

public function testCancelTransactionWillCancelRequest()
{
$loop = $this->getMockBuilder('React\EventLoop\LoopInterface')->getMock();
Expand Down