Skip to content

remove body on redirection if needed #222

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.

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

Merged
merged 1 commit into from
Sep 29, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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: 7 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -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
Expand Down
12 changes: 12 additions & 0 deletions phpstan.neon.dist
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
9 changes: 8 additions & 1 deletion spec/Plugin/RedirectPluginSpec.php
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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');

Expand Down Expand Up @@ -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');

Expand Down Expand Up @@ -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');
Expand Down
68 changes: 65 additions & 3 deletions src/Plugin/RedirectPlugin.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;

/**
Expand Down Expand Up @@ -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 = [])
{
Expand All @@ -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'];
Expand All @@ -137,6 +154,8 @@ public function __construct(array $config = [])
$this->redirectCodes[301]['switch'] = false;
$this->redirectCodes[302]['switch'] = false;
}

$this->streamFactory = $options['stream_factory'];
}

/**
Expand Down Expand Up @@ -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);
}

Expand All @@ -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);
}
}
Expand Down
53 changes: 52 additions & 1 deletion tests/Plugin/RedirectPluginTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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 [
Expand Down Expand Up @@ -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'));
}
}