From 6bc30542a029610785ef5717a882e592df87b82b Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 12 Jan 2020 10:58:00 +0100 Subject: [PATCH 1/5] Updated cross domain middleware so that it always returns success response on OPTIONS requests --- .../src/Middleware/CrossDomainMiddleware.php | 9 ++-- .../Middleware/CrossDomainMiddlewareTest.php | 44 +++++++++++++++++-- 2 files changed, 45 insertions(+), 8 deletions(-) diff --git a/module/Rest/src/Middleware/CrossDomainMiddleware.php b/module/Rest/src/Middleware/CrossDomainMiddleware.php index aacda9fc..09a7cc03 100644 --- a/module/Rest/src/Middleware/CrossDomainMiddleware.php +++ b/module/Rest/src/Middleware/CrossDomainMiddleware.php @@ -10,8 +10,10 @@ use Psr\Http\Message\ServerRequestInterface; use Psr\Http\Server\MiddlewareInterface; use Psr\Http\Server\RequestHandlerInterface; use Shlinkio\Shlink\Rest\Authentication; +use Zend\Diactoros\Response\EmptyResponse;; use Zend\Expressive\Router\RouteResult; +use function array_merge; use function implode; class CrossDomainMiddleware implements MiddlewareInterface, RequestMethodInterface @@ -54,10 +56,7 @@ class CrossDomainMiddleware implements MiddlewareInterface, RequestMethodInterfa 'Access-Control-Allow-Headers' => $request->getHeaderLine('Access-Control-Request-Headers'), ]; - foreach ($corsHeaders as $key => $value) { - $response = $response->withHeader($key, $value); - } - - return $response; + // Options requests should always be empty and have a 204 status code + return EmptyResponse::withHeaders(array_merge($response->getHeaders(), $corsHeaders)); } } diff --git a/module/Rest/test/Middleware/CrossDomainMiddlewareTest.php b/module/Rest/test/Middleware/CrossDomainMiddlewareTest.php index 1716c19e..317b8603 100644 --- a/module/Rest/test/Middleware/CrossDomainMiddlewareTest.php +++ b/module/Rest/test/Middleware/CrossDomainMiddlewareTest.php @@ -34,14 +34,14 @@ class CrossDomainMiddlewareTest extends TestCase /** @test */ public function nonCrossDomainRequestsAreNotAffected(): void { - $originalResponse = new Response(); + $originalResponse = (new Response())->withStatus(404); $this->handler->handle(Argument::any())->willReturn($originalResponse)->shouldBeCalledOnce(); $response = $this->middleware->process(new ServerRequest(), $this->handler->reveal()); - $this->assertSame($originalResponse, $response); - $headers = $response->getHeaders(); + $this->assertSame($originalResponse, $response); + $this->assertEquals(404, $response->getStatusCode()); $this->assertArrayNotHasKey('Access-Control-Allow-Origin', $headers); $this->assertArrayNotHasKey('Access-Control-Expose-Headers', $headers); $this->assertArrayNotHasKey('Access-Control-Allow-Methods', $headers); @@ -96,6 +96,7 @@ class CrossDomainMiddlewareTest extends TestCase $this->assertArrayHasKey('Access-Control-Allow-Methods', $headers); $this->assertEquals('1000', $response->getHeaderLine('Access-Control-Max-Age')); $this->assertEquals('foo, bar, baz', $response->getHeaderLine('Access-Control-Allow-Headers')); + $this->assertEquals(204, $response->getStatusCode()); } /** @@ -115,6 +116,7 @@ class CrossDomainMiddlewareTest extends TestCase $response = $this->middleware->process($request, $this->handler->reveal()); $this->assertEquals($response->getHeaderLine('Access-Control-Allow-Methods'), $expectedAllowedMethods); + $this->assertEquals(204, $response->getStatusCode()); } public function provideRouteResults(): iterable @@ -129,4 +131,40 @@ class CrossDomainMiddlewareTest extends TestCase 'DELETE,PATCH,PUT', ]; } + + /** + * @test + * @dataProvider provideMethods + */ + public function expectedStatusCodeIsReturnDependingOnRequestMethod( + string $method, + int $status, + int $expectedStatus + ): void { + $originalResponse = (new Response())->withStatus($status); + $request = (new ServerRequest())->withMethod($method) + ->withHeader('Origin', 'local'); + $this->handler->handle(Argument::any())->willReturn($originalResponse)->shouldBeCalledOnce(); + $response = $this->middleware->process($request, $this->handler->reveal()); + $this->assertEquals($expectedStatus, $response->getStatusCode()); + } + + public function provideMethods(): iterable + { + yield 'POST 200' => ['POST', 200, 200]; + yield 'POST 400' => ['POST', 400, 400]; + yield 'POST 500' => ['POST', 500, 500]; + yield 'GET 200' => ['GET', 200, 200]; + yield 'GET 400' => ['GET', 400, 400]; + yield 'GET 500' => ['GET', 500, 500]; + yield 'PATCH 200' => ['PATCH', 200, 200]; + yield 'PATCH 400' => ['PATCH', 400, 400]; + yield 'PATCH 500' => ['PATCH', 500, 500]; + yield 'DELETE 200' => ['DELETE', 200, 200]; + yield 'DELETE 400' => ['DELETE', 400, 400]; + yield 'DELETE 500' => ['DELETE', 500, 500]; + yield 'OPTIONS 200' => ['OPTIONS', 200, 204]; + yield 'OPTIONS 400' => ['OPTIONS', 400, 204]; + yield 'OPTIONS 500' => ['OPTIONS', 500, 204]; + } } From 127f7e80f538ddf904b74a3b1da9dc926187d47f Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 12 Jan 2020 10:58:04 +0100 Subject: [PATCH 2/5] Added v1.21.2 to the changelog --- CHANGELOG.md | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index d0f69aa0..81df3083 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,30 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com), and this project adheres to [Semantic Versioning](https://semver.org). +## 1.21.2 - 2020-01-12 + +#### Added + +* *Nothing* + +#### Changed + +* *Nothing* + +#### Deprecated + +* *Nothing* + +#### Removed + +* *Nothing* + +#### Fixed + +* [#614](https://github.com/shlinkio/shlink/issues/614) Fixed `OPTIONS` requests including the `Origin` header not always returning an empty body with status 2xx. +* [#615](https://github.com/shlinkio/shlink/issues/615) Fixed query args with no value being lost from the long URL when users are redirected. + + ## 1.21.1 - 2020-01-02 #### Added From cc8474aefb25cf748ef0622fdf57c79b51b4958e Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 12 Jan 2020 10:26:59 +0100 Subject: [PATCH 3/5] Replaced standard http_build_query by guzzle's build_query, which keeps params with no value --- module/Core/src/Action/AbstractTrackingAction.php | 13 ++----------- module/Core/test/Action/RedirectActionTest.php | 1 + 2 files changed, 3 insertions(+), 11 deletions(-) diff --git a/module/Core/src/Action/AbstractTrackingAction.php b/module/Core/src/Action/AbstractTrackingAction.php index ff8d91f2..9ebe6e9b 100644 --- a/module/Core/src/Action/AbstractTrackingAction.php +++ b/module/Core/src/Action/AbstractTrackingAction.php @@ -20,8 +20,8 @@ use Zend\Diactoros\Uri; use function array_key_exists; use function array_merge; +use function GuzzleHttp\Psr7\build_query; use function GuzzleHttp\Psr7\parse_query; -use function http_build_query; abstract class AbstractTrackingAction implements MiddlewareInterface { @@ -46,15 +46,6 @@ abstract class AbstractTrackingAction implements MiddlewareInterface $this->logger = $logger ?: new NullLogger(); } - /** - * Process an incoming server request and return a response, optionally delegating - * to the next middleware component to create the response. - * - * @param ServerRequestInterface $request - * @param RequestHandlerInterface $handler - * - * @return ResponseInterface - */ public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface { $shortCode = $request->getAttribute('shortCode', ''); @@ -86,7 +77,7 @@ abstract class AbstractTrackingAction implements MiddlewareInterface } $mergedQuery = array_merge($hardcodedQuery, $currentQuery); - return (string) $uri->withQuery(http_build_query($mergedQuery)); + return (string) $uri->withQuery(build_query($mergedQuery)); } abstract protected function createSuccessResp(string $longUrl): ResponseInterface; diff --git a/module/Core/test/Action/RedirectActionTest.php b/module/Core/test/Action/RedirectActionTest.php index 55342cb5..41fd6156 100644 --- a/module/Core/test/Action/RedirectActionTest.php +++ b/module/Core/test/Action/RedirectActionTest.php @@ -67,6 +67,7 @@ class RedirectActionTest extends TestCase { yield ['http://domain.com/foo/bar?some=thing', []]; yield ['http://domain.com/foo/bar?some=thing', ['foobar' => 'notrack']]; + yield ['http://domain.com/foo/bar?some=thing&else', ['else' => null]]; yield ['http://domain.com/foo/bar?some=thing&foo=bar', ['foo' => 'bar']]; yield ['http://domain.com/foo/bar?some=overwritten&foo=bar', ['foo' => 'bar', 'some' => 'overwritten']]; yield ['http://domain.com/foo/bar?some=overwritten', ['foobar' => 'notrack', 'some' => 'overwritten']]; From 0d7f9c05944cf646d6d51a646e7e69d1ca9f5bff Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 12 Jan 2020 11:06:16 +0100 Subject: [PATCH 4/5] Removed extra semicolon added by mistake --- module/Rest/src/Middleware/CrossDomainMiddleware.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/module/Rest/src/Middleware/CrossDomainMiddleware.php b/module/Rest/src/Middleware/CrossDomainMiddleware.php index 09a7cc03..e88fa343 100644 --- a/module/Rest/src/Middleware/CrossDomainMiddleware.php +++ b/module/Rest/src/Middleware/CrossDomainMiddleware.php @@ -10,7 +10,7 @@ use Psr\Http\Message\ServerRequestInterface; use Psr\Http\Server\MiddlewareInterface; use Psr\Http\Server\RequestHandlerInterface; use Shlinkio\Shlink\Rest\Authentication; -use Zend\Diactoros\Response\EmptyResponse;; +use Zend\Diactoros\Response\EmptyResponse; use Zend\Expressive\Router\RouteResult; use function array_merge; From 81affb8c15db59119421d1504c840933523b2b6c Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Sun, 12 Jan 2020 11:06:36 +0100 Subject: [PATCH 5/5] Removed builds in PHP 7.4 envs --- .travis.yml | 5 ----- 1 file changed, 5 deletions(-) diff --git a/.travis.yml b/.travis.yml index a4b5d1de..27bd48ad 100644 --- a/.travis.yml +++ b/.travis.yml @@ -7,11 +7,6 @@ branches: php: - '7.2' - '7.3' - - '7.4' - -matrix: - allow_failures: - - php: '7.4' services: - mysql