diff --git a/.github/workflows/static.yml b/.github/workflows/static.yml index c8e6274..fb1e01a 100644 --- a/.github/workflows/static.yml +++ b/.github/workflows/static.yml @@ -2,6 +2,8 @@ name: static on: push: + branches: + - master pull_request: jobs: @@ -19,3 +21,16 @@ jobs: REQUIRE_DEV: false with: args: analyze --no-progress + + php-cs-fixer: + name: PHP-CS-Fixer + runs-on: ubuntu-latest + + steps: + - name: Checkout code + uses: actions/checkout@v2 + + - name: PHP-CS-Fixer + uses: docker://oskarstark/php-cs-fixer-ga + with: + args: --dry-run --diff diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 678f13f..df61234 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -2,6 +2,8 @@ name: tests on: push: + branches: + - master pull_request: jobs: diff --git a/.gitignore b/.gitignore index 1cb59e9..5874147 100644 --- a/.gitignore +++ b/.gitignore @@ -1,5 +1,5 @@ -/.php_cs -/.php_cs.cache +/.php-cs-fixer.php +/.php-cs-fixer.cache /behat.yml /build/ /composer.lock diff --git a/.php-cs-fixer.dist.php b/.php-cs-fixer.dist.php new file mode 100644 index 0000000..92bc748 --- /dev/null +++ b/.php-cs-fixer.dist.php @@ -0,0 +1,18 @@ +in(__DIR__.'/src') + ->in(__DIR__.'/tests') + ->name('*.php') +; + +$config = new PhpCsFixer\Config(); + +return $config + ->setRiskyAllowed(true) + ->setRules([ + '@Symfony' => true, + 'single_line_throw' => false, + ]) + ->setFinder($finder) +; diff --git a/CHANGELOG.md b/CHANGELOG.md index 596ad94..9ff3bd8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,8 @@ ### Fixed -- Fixes false positive circular detection in RedirectPlugin in cases when target location does not contain path +- [RedirectPlugin] Fixed handling of redirection to different domain with default port +- [RedirectPlugin] Fixed false positive circular detection in RedirectPlugin in cases when target location does not contain path ## 2.5.0 - 2021-11-26 diff --git a/spec/Plugin/RedirectPluginSpec.php b/spec/Plugin/RedirectPluginSpec.php index 2682499..a8df45c 100644 --- a/spec/Plugin/RedirectPluginSpec.php +++ b/spec/Plugin/RedirectPluginSpec.php @@ -506,54 +506,4 @@ public function it_throws_circular_redirection_exception_on_alternating_redirect $promise->shouldReturnAnInstanceOf(HttpRejectedPromise::class); $promise->shouldThrow(CircularRedirectionException::class)->duringWait(); } - - public function it_redirects_http_to_https( - UriInterface $uri, - UriInterface $uriRedirect, - RequestInterface $request, - ResponseInterface $responseRedirect, - RequestInterface $modifiedRequest, - ResponseInterface $finalResponse, - Promise $promise - ) { - $responseRedirect->getStatusCode()->willReturn(302); - $responseRedirect->hasHeader('Location')->willReturn(true); - $responseRedirect->getHeaderLine('Location')->willReturn('https://my-site.com/original'); - - $request->getUri()->willReturn($uri); - $request->withUri($uriRedirect)->willReturn($modifiedRequest); - $uri->__toString()->willReturn('http://my-site.com/original'); - $uri->withPath('/original')->willReturn($uri); - $uri->withFragment('')->willReturn($uri); - $uri->withQuery('')->willReturn($uri); - - $uri->withScheme('https')->willReturn($uriRedirect); - $uriRedirect->withHost('my-site.com')->willReturn($uriRedirect); - $uriRedirect->withPath('/original')->willReturn($uriRedirect); - $uriRedirect->withFragment('')->willReturn($uriRedirect); - $uriRedirect->withQuery('')->willReturn($uriRedirect); - $uriRedirect->__toString()->willReturn('https://my-site.com/original'); - - $modifiedRequest->getUri()->willReturn($uriRedirect); - $modifiedRequest->getMethod()->willReturn('GET'); - - $next = function (RequestInterface $receivedRequest) use ($request, $responseRedirect) { - if (Argument::is($request->getWrappedObject())->scoreArgument($receivedRequest)) { - return new HttpFulfilledPromise($responseRedirect->getWrappedObject()); - } - }; - - $first = function (RequestInterface $receivedRequest) use ($modifiedRequest, $promise) { - if (Argument::is($modifiedRequest->getWrappedObject())->scoreArgument($receivedRequest)) { - return $promise->getWrappedObject(); - } - }; - - $promise->getState()->willReturn(Promise::FULFILLED); - $promise->wait()->shouldBeCalled()->willReturn($finalResponse); - - $finalPromise = $this->handleRequest($request, $next, $first); - $finalPromise->shouldReturnAnInstanceOf(HttpFulfilledPromise::class); - $finalPromise->wait()->shouldReturn($finalResponse); - } } diff --git a/src/Deferred.php b/src/Deferred.php index ef2b309..646f375 100644 --- a/src/Deferred.php +++ b/src/Deferred.php @@ -146,7 +146,10 @@ public function wait($unwrap = true) return $this->value; } - /** @var ClientExceptionInterface */ + if (null === $this->failure) { + throw new \RuntimeException('Internal Error: Promise is not fulfilled but has no exception stored'); + } + throw $this->failure; } } diff --git a/src/HttpClientPool/HttpClientPool.php b/src/HttpClientPool/HttpClientPool.php index d6d1777..a30bdb0 100644 --- a/src/HttpClientPool/HttpClientPool.php +++ b/src/HttpClientPool/HttpClientPool.php @@ -46,9 +46,9 @@ public function addHttpClient($client): void /** * Return an http client given a specific strategy. * - * @throws HttpClientNotFoundException When no http client has been found into the pool - * * @return HttpClientPoolItem Return a http client that can do both sync or async + * + * @throws HttpClientNotFoundException When no http client has been found into the pool */ abstract protected function chooseHttpClient(): HttpClientPoolItem; diff --git a/src/Plugin/AddHostPlugin.php b/src/Plugin/AddHostPlugin.php index c7fb05a..2a866ee 100644 --- a/src/Plugin/AddHostPlugin.php +++ b/src/Plugin/AddHostPlugin.php @@ -31,7 +31,7 @@ final class AddHostPlugin implements Plugin * @param array{'replace'?: bool} $config * * Configuration options: - * - replace: True will replace all hosts, false will only add host when none is specified. + * - replace: True will replace all hosts, false will only add host when none is specified */ public function __construct(UriInterface $host, array $config = []) { diff --git a/src/Plugin/AddPathPlugin.php b/src/Plugin/AddPathPlugin.php index 9d43104..87fcdce 100644 --- a/src/Plugin/AddPathPlugin.php +++ b/src/Plugin/AddPathPlugin.php @@ -70,7 +70,7 @@ public function handleRequest(RequestInterface $request, callable $next, callabl if (substr($path, 0, strlen($prepend)) !== $prepend) { $request = $request->withUri($request->getUri() ->withPath($prepend.$path) - ); + ); } return $next($request); diff --git a/src/Plugin/ContentTypePlugin.php b/src/Plugin/ContentTypePlugin.php index 9a87f99..da3758e 100644 --- a/src/Plugin/ContentTypePlugin.php +++ b/src/Plugin/ContentTypePlugin.php @@ -39,7 +39,7 @@ final class ContentTypePlugin implements Plugin * * Configuration options: * - skip_detection: true skip detection if stream size is bigger than $size_limit - * - size_limit: size stream limit for which the detection as to be skipped. + * - size_limit: size stream limit for which the detection as to be skipped */ public function __construct(array $config = []) { diff --git a/src/Plugin/DecoderPlugin.php b/src/Plugin/DecoderPlugin.php index b685967..41c1a58 100644 --- a/src/Plugin/DecoderPlugin.php +++ b/src/Plugin/DecoderPlugin.php @@ -34,7 +34,7 @@ final class DecoderPlugin implements Plugin * @param array{'use_content_encoding'?: bool} $config * * Configuration options: - * - use_content_encoding: Whether this plugin should look at the Content-Encoding header first or only at the Transfer-Encoding (defaults to true). + * - use_content_encoding: Whether this plugin should look at the Content-Encoding header first or only at the Transfer-Encoding (defaults to true) */ public function __construct(array $config = []) { diff --git a/src/Plugin/ErrorPlugin.php b/src/Plugin/ErrorPlugin.php index 06c795f..678977f 100644 --- a/src/Plugin/ErrorPlugin.php +++ b/src/Plugin/ErrorPlugin.php @@ -40,7 +40,7 @@ final class ErrorPlugin implements Plugin * @param array{'only_server_exception'?: bool} $config * * Configuration options: - * - only_server_exception: Whether this plugin should only throw 5XX Exceptions (default to false). + * - only_server_exception: Whether this plugin should only throw 5XX Exceptions (default to false) */ public function __construct(array $config = []) { @@ -72,10 +72,10 @@ public function handleRequest(RequestInterface $request, callable $next, callabl * @param RequestInterface $request Request of the call * @param ResponseInterface $response Response of the call * + * @return ResponseInterface If status code is not in 4xx or 5xx return response + * * @throws ClientErrorException If response status code is a 4xx * @throws ServerErrorException If response status code is a 5xx - * - * @return ResponseInterface If status code is not in 4xx or 5xx return response */ private function transformResponseToException(RequestInterface $request, ResponseInterface $response): ResponseInterface { diff --git a/src/Plugin/RedirectPlugin.php b/src/Plugin/RedirectPlugin.php index d5e52f8..7b96b1b 100644 --- a/src/Plugin/RedirectPlugin.php +++ b/src/Plugin/RedirectPlugin.php @@ -107,7 +107,7 @@ final class RedirectPlugin implements Plugin * 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. + * - strict: When true, redirect codes 300, 301, 302 will not modify request method and body */ public function __construct(array $config = []) { @@ -226,13 +226,39 @@ private function createUri(ResponseInterface $redirectResponse, RequestInterface $location = $redirectResponse->getHeaderLine('Location'); $parsedLocation = parse_url($location); - if (false === $parsedLocation) { - throw new HttpException(sprintf('Location %s could not be parsed', $location), $originalRequest, $redirectResponse); + if (false === $parsedLocation || '' === $location) { + throw new HttpException(sprintf('Location "%s" could not be parsed', $location), $originalRequest, $redirectResponse); } $uri = $originalRequest->getUri(); + + // Redirections can either use an absolute uri or a relative reference https://www.rfc-editor.org/rfc/rfc3986#section-4.2 + // If relative, we need to check if we have an absolute path or not + + $path = array_key_exists('path', $parsedLocation) ? $parsedLocation['path'] : ''; + if (!array_key_exists('host', $parsedLocation) && '/' !== $location[0]) { + // the target is a relative-path reference, we need to merge it with the base path + $originalPath = $uri->getPath(); + if ('' === $path) { + $path = $originalPath; + } elseif (($pos = strrpos($originalPath, '/')) !== false) { + $path = substr($originalPath, 0, $pos + 1).$path; + } else { + $path = '/'.$path; + } + /* replace '/./' or '/foo/../' with '/' */ + $re = ['#(/\./)#', '#/(?!\.\.)[^/]+/\.\./#']; + for ($n = 1; $n > 0; $path = preg_replace($re, '/', $path, -1, $n)) { + if (null === $path) { + throw new HttpException(sprintf('Failed to resolve Location %s', $location), $originalRequest, $redirectResponse); + } + } + } + if (null === $path) { + throw new HttpException(sprintf('Failed to resolve Location %s', $location), $originalRequest, $redirectResponse); + } $uri = $uri - ->withPath(array_key_exists('path', $parsedLocation) ? $parsedLocation['path'] : '') + ->withPath($path) ->withQuery(array_key_exists('query', $parsedLocation) ? $parsedLocation['query'] : '') ->withFragment(array_key_exists('fragment', $parsedLocation) ? $parsedLocation['fragment'] : '') ; @@ -247,6 +273,8 @@ private function createUri(ResponseInterface $redirectResponse, RequestInterface if (array_key_exists('port', $parsedLocation)) { $uri = $uri->withPort($parsedLocation['port']); + } elseif (array_key_exists('host', $parsedLocation)) { + $uri = $uri->withPort(null); } return $uri; diff --git a/src/PluginChain.php b/src/PluginChain.php index fcb1914..2dd630c 100644 --- a/src/PluginChain.php +++ b/src/PluginChain.php @@ -5,6 +5,7 @@ namespace Http\Client\Common; use function array_reverse; + use Http\Client\Common\Exception\LoopException; use Http\Promise\Promise; use Psr\Http\Message\RequestInterface; diff --git a/src/PluginClient.php b/src/PluginClient.php index d67145d..db81243 100644 --- a/src/PluginClient.php +++ b/src/PluginClient.php @@ -124,7 +124,6 @@ private function configure(array $options = []): array */ private function createPluginChain(array $plugins, callable $clientCallable): callable { - /** @var callable(RequestInterface): Promise */ return new PluginChain($plugins, $clientCallable, $this->options); } } diff --git a/src/PluginClientFactory.php b/src/PluginClientFactory.php index f7e93e3..1d2b2be 100644 --- a/src/PluginClientFactory.php +++ b/src/PluginClientFactory.php @@ -39,11 +39,11 @@ public static function setFactory(callable $factory): void /** * @param ClientInterface|HttpAsyncClient $client * @param Plugin[] $plugins - * @param array{'client_name'?: string} $options + * @param array{'client_name'?: string} $options * * Configuration options: * - client_name: to give client a name which may be used when displaying client information - * like in the HTTPlugBundle profiler. + * like in the HTTPlugBundle profiler * * @see PluginClient constructor for PluginClient specific $options. */ diff --git a/tests/Plugin/RedirectPluginTest.php b/tests/Plugin/RedirectPluginTest.php index 18cd230..5e26703 100644 --- a/tests/Plugin/RedirectPluginTest.php +++ b/tests/Plugin/RedirectPluginTest.php @@ -1,4 +1,5 @@ wait(); } + public function provideRedirections(): array + { + return [ + 'no path on target' => ['https://example.com/path?query=value', 'https://example.com?query=value', 'https://example.com?query=value'], + 'root path on target' => ['https://example.com/path?query=value', 'https://example.com/?query=value', 'https://example.com/?query=value'], + 'redirect to query' => ['https://example.com', 'https://example.com?query=value', 'https://example.com?query=value'], + 'redirect to different domain without port' => ['https://example.com:8000', 'https://foo.com?query=value', 'https://foo.com?query=value'], + 'network-path redirect, preserve scheme' => ['https://example.com:8000', '//foo.com/path?query=value', 'https://foo.com/path?query=value'], + 'absolute-path redirect, preserve host' => ['https://example.com:8000', '/path?query=value', 'https://example.com:8000/path?query=value'], + 'relative-path redirect, append' => ['https://example.com:8000/path/', 'sub/path?query=value', 'https://example.com:8000/path/sub/path?query=value'], + 'relative-path on non-folder' => ['https://example.com:8000/path/foo', 'sub/path?query=value', 'https://example.com:8000/path/sub/path?query=value'], + 'relative-path moving up' => ['https://example.com:8000/path/', '../other?query=value', 'https://example.com:8000/other?query=value'], + 'relative-path with ./' => ['https://example.com:8000/path/', './other?query=value', 'https://example.com:8000/path/other?query=value'], + 'relative-path with //' => ['https://example.com:8000/path/', 'other//sub?query=value', 'https://example.com:8000/path/other//sub?query=value'], + 'relative-path redirect with only query' => ['https://example.com:8000/path', '?query=value', 'https://example.com:8000/path?query=value'], + ]; + } + /** - * @testWith ["https://example.com/path?query=value", "https://example.com?query=value", "https://example.com?query=value"] - * ["https://example.com/path?query=value", "https://example.com/?query=value", "https://example.com/?query=value"] - * ["https://example.com", "https://example.com?query=value", "https://example.com?query=value"] + * @dataProvider provideRedirections */ public function testTargetUriMappingFromLocationHeader(string $originalUri, string $locationUri, string $targetUri): void { diff --git a/tests/PluginChainTest.php b/tests/PluginChainTest.php index 95fdfab..1c8d29a 100644 --- a/tests/PluginChainTest.php +++ b/tests/PluginChainTest.php @@ -9,15 +9,13 @@ use Http\Client\Common\PluginChain; use Http\Promise\Promise; use PHPUnit\Framework\TestCase; -use Prophecy\Argument; use Psr\Http\Message\RequestInterface; class PluginChainTest extends TestCase { private function createPlugin(callable $func): Plugin { - return new class ($func) implements Plugin - { + return new class($func) implements Plugin { public $func; public function __construct(callable $func)