From 84331135f784f731a58fd49bf5c0fed68ab35418 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Thu, 31 Dec 2020 13:28:06 +0100 Subject: [PATCH 1/3] Created API tests for CORS --- composer.json | 2 +- config/autoload/cors.global.php | 11 +++ module/Rest/config/dependencies.config.php | 3 +- .../src/Middleware/CrossDomainMiddleware.php | 9 +- module/Rest/test-api/Middleware/CorsTest.php | 83 +++++++++++++++++++ .../Middleware/CrossDomainMiddlewareTest.php | 2 +- 6 files changed, 106 insertions(+), 4 deletions(-) create mode 100644 config/autoload/cors.global.php create mode 100644 module/Rest/test-api/Middleware/CorsTest.php diff --git a/composer.json b/composer.json index b3de83717..e503edcb9 100644 --- a/composer.json +++ b/composer.json @@ -23,7 +23,6 @@ "doctrine/migrations": "^3.0.2", "doctrine/orm": "^2.8", "endroid/qr-code": "^3.6", - "friendsofphp/proxy-manager-lts": "^1.0", "geoip2/geoip2": "^2.9", "guzzlehttp/guzzle": "^7.0", "laminas/laminas-config": "^3.3", @@ -43,6 +42,7 @@ "mezzio/mezzio-swoole": "^2.6.4", "monolog/monolog": "^2.0", "nikolaposa/monolog-factory": "^3.1", + "ocramius/proxy-manager": "^2.11", "php-middleware/request-id": "^4.1", "predis/predis": "^1.1", "pugx/shortid-php": "^0.7", diff --git a/config/autoload/cors.global.php b/config/autoload/cors.global.php new file mode 100644 index 000000000..58ad94289 --- /dev/null +++ b/config/autoload/cors.global.php @@ -0,0 +1,11 @@ + [ + 'max_age' => 3600, + ], + +]; diff --git a/module/Rest/config/dependencies.config.php b/module/Rest/config/dependencies.config.php index bdd9d3a9a..8c1cdb8e2 100644 --- a/module/Rest/config/dependencies.config.php +++ b/module/Rest/config/dependencies.config.php @@ -41,7 +41,7 @@ ImplicitOptionsMiddleware::class => Middleware\EmptyResponseImplicitOptionsMiddlewareFactory::class, Middleware\BodyParserMiddleware::class => InvokableFactory::class, - Middleware\CrossDomainMiddleware::class => InvokableFactory::class, + Middleware\CrossDomainMiddleware::class => ConfigAbstractFactory::class, Middleware\ShortUrl\CreateShortUrlContentNegotiationMiddleware::class => InvokableFactory::class, Middleware\ShortUrl\DropDefaultDomainFromRequestMiddleware::class => ConfigAbstractFactory::class, Middleware\ShortUrl\DefaultShortCodesLengthMiddleware::class => ConfigAbstractFactory::class, @@ -76,6 +76,7 @@ Action\Tag\UpdateTagAction::class => [TagService::class], Action\Domain\ListDomainsAction::class => [DomainService::class, 'config.url_shortener.domain.hostname'], + Middleware\CrossDomainMiddleware::class => ['config.cors'], Middleware\ShortUrl\DropDefaultDomainFromRequestMiddleware::class => ['config.url_shortener.domain.hostname'], Middleware\ShortUrl\DefaultShortCodesLengthMiddleware::class => [ 'config.url_shortener.default_short_codes_length', diff --git a/module/Rest/src/Middleware/CrossDomainMiddleware.php b/module/Rest/src/Middleware/CrossDomainMiddleware.php index 171142a12..88d62904f 100644 --- a/module/Rest/src/Middleware/CrossDomainMiddleware.php +++ b/module/Rest/src/Middleware/CrossDomainMiddleware.php @@ -17,6 +17,13 @@ class CrossDomainMiddleware implements MiddlewareInterface, RequestMethodInterface { + private array $config; + + public function __construct(array $config) + { + $this->config = $config; + } + public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface { $response = $handler->handle($request); @@ -48,7 +55,7 @@ private function addOptionsHeaders(ServerRequestInterface $request, ResponseInte ]; $corsHeaders = [ 'Access-Control-Allow-Methods' => implode(',', $matchedMethods), - 'Access-Control-Max-Age' => '1000', + 'Access-Control-Max-Age' => $this->config['max_age'], 'Access-Control-Allow-Headers' => $request->getHeaderLine('Access-Control-Request-Headers'), ]; diff --git a/module/Rest/test-api/Middleware/CorsTest.php b/module/Rest/test-api/Middleware/CorsTest.php new file mode 100644 index 000000000..4e0603525 --- /dev/null +++ b/module/Rest/test-api/Middleware/CorsTest.php @@ -0,0 +1,83 @@ +callApiWithKey(self::METHOD_GET, '/short-urls'); + + self::assertEquals(200, $resp->getStatusCode()); + self::assertFalse($resp->hasHeader('Access-Control-Allow-Origin')); + self::assertFalse($resp->hasHeader('Access-Control-Expose-Headers')); + self::assertFalse($resp->hasHeader('Access-Control-Allow-Methods')); + self::assertFalse($resp->hasHeader('Access-Control-Max-Age')); + self::assertFalse($resp->hasHeader('Access-Control-Allow-Headers')); + } + + /** + * @test + * @dataProvider provideOrigins + */ + public function responseIncludesCorsHeadersIfOriginIsSent( + string $origin, + string $endpoint, + int $expectedStatusCode + ): void { + $resp = $this->callApiWithKey(self::METHOD_GET, $endpoint, [ + RequestOptions::HEADERS => ['Origin' => $origin], + ]); + + self::assertEquals($expectedStatusCode, $resp->getStatusCode()); + self::assertEquals($origin, $resp->getHeaderLine('Access-Control-Allow-Origin')); + self::assertEquals('X-Api-Key', $resp->getHeaderLine('Access-Control-Expose-Headers')); + self::assertFalse($resp->hasHeader('Access-Control-Allow-Methods')); + self::assertFalse($resp->hasHeader('Access-Control-Max-Age')); + self::assertFalse($resp->hasHeader('Access-Control-Allow-Headers')); + } + + public function provideOrigins(): iterable + { + yield 'foo.com' => ['foo.com', '/short-urls', 200]; + yield 'bar.io' => ['bar.io', '/foo/bar', 404]; + yield 'baz.dev' => ['baz.dev', '/short-urls', 200]; + } + + /** + * @test + * @dataProvider providePreflightEndpoints + */ + public function preflightRequestsIncludeExtraCorsHeaders(string $endpoint, string $expectedAllowedMethods): void + { + $allowedHeaders = 'Authorization'; + $resp = $this->callApiWithKey(self::METHOD_OPTIONS, $endpoint, [ + RequestOptions::HEADERS => [ + 'Origin' => 'foo.com', + 'Access-Control-Request-Headers' => $allowedHeaders, + ], + ]); + + self::assertEquals(204, $resp->getStatusCode()); + self::assertTrue($resp->hasHeader('Access-Control-Allow-Origin')); + self::assertTrue($resp->hasHeader('Access-Control-Expose-Headers')); + self::assertTrue($resp->hasHeader('Access-Control-Max-Age')); + self::assertEquals($expectedAllowedMethods, $resp->getHeaderLine('Access-Control-Allow-Methods')); + self::assertEquals($allowedHeaders, $resp->getHeaderLine('Access-Control-Allow-Headers')); + } + + public function providePreflightEndpoints(): iterable + { + yield 'invalid route' => ['/foo/bar', 'GET,POST,PUT,PATCH,DELETE,OPTIONS']; + yield 'short URLs routes' => ['/short-urls', 'GET,POST,PUT,PATCH,DELETE,OPTIONS']; +// yield 'short URLs routes' => ['/short-urls', 'GET,POST']; // TODO This should be the good one + yield 'tags routes' => ['/tags', 'GET,POST,PUT,PATCH,DELETE,OPTIONS']; +// yield 'tags routes' => ['/short-urls', 'GET,POST,PUT,DELETE']; // TODO This should be the good one + } +} diff --git a/module/Rest/test/Middleware/CrossDomainMiddlewareTest.php b/module/Rest/test/Middleware/CrossDomainMiddlewareTest.php index 03675fcee..72e95a364 100644 --- a/module/Rest/test/Middleware/CrossDomainMiddlewareTest.php +++ b/module/Rest/test/Middleware/CrossDomainMiddlewareTest.php @@ -26,7 +26,7 @@ class CrossDomainMiddlewareTest extends TestCase public function setUp(): void { - $this->middleware = new CrossDomainMiddleware(); + $this->middleware = new CrossDomainMiddleware(['max_age' => 1000]); $this->handler = $this->prophesize(RequestHandlerInterface::class); } From 850a5b412c4240becfdb35abf5d692a23eb31f31 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Thu, 31 Dec 2020 15:41:02 +0100 Subject: [PATCH 2/3] Removed Access-Control-Expose-Headers header from CrossDomainM;iddleware, as it's actually not correct --- module/Rest/src/Middleware/CrossDomainMiddleware.php | 7 ++++--- module/Rest/test-api/Middleware/CorsTest.php | 3 --- module/Rest/test/Middleware/CrossDomainMiddlewareTest.php | 3 --- 3 files changed, 4 insertions(+), 9 deletions(-) diff --git a/module/Rest/src/Middleware/CrossDomainMiddleware.php b/module/Rest/src/Middleware/CrossDomainMiddleware.php index 88d62904f..b438f7ec3 100644 --- a/module/Rest/src/Middleware/CrossDomainMiddleware.php +++ b/module/Rest/src/Middleware/CrossDomainMiddleware.php @@ -32,8 +32,7 @@ public function process(ServerRequestInterface $request, RequestHandlerInterface } // Add Allow-Origin header - $response = $response->withHeader('Access-Control-Allow-Origin', $request->getHeader('Origin')) - ->withHeader('Access-Control-Expose-Headers', AuthenticationMiddleware::API_KEY_HEADER); + $response = $response->withHeader('Access-Control-Allow-Origin', $request->getHeader('Origin')); if ($request->getMethod() !== self::METHOD_OPTIONS) { return $response; } @@ -43,6 +42,8 @@ public function process(ServerRequestInterface $request, RequestHandlerInterface private function addOptionsHeaders(ServerRequestInterface $request, ResponseInterface $response): ResponseInterface { + // TODO This won't work. The route has to be matched from the router as this middleware needs to be executed + // before trying to match the route /** @var RouteResult|null $matchedRoute */ $matchedRoute = $request->getAttribute(RouteResult::class); $matchedMethods = $matchedRoute !== null ? $matchedRoute->getAllowedMethods() : [ @@ -55,8 +56,8 @@ private function addOptionsHeaders(ServerRequestInterface $request, ResponseInte ]; $corsHeaders = [ 'Access-Control-Allow-Methods' => implode(',', $matchedMethods), - 'Access-Control-Max-Age' => $this->config['max_age'], 'Access-Control-Allow-Headers' => $request->getHeaderLine('Access-Control-Request-Headers'), + 'Access-Control-Max-Age' => $this->config['max_age'], ]; // Options requests should always be empty and have a 204 status code diff --git a/module/Rest/test-api/Middleware/CorsTest.php b/module/Rest/test-api/Middleware/CorsTest.php index 4e0603525..a1ca99015 100644 --- a/module/Rest/test-api/Middleware/CorsTest.php +++ b/module/Rest/test-api/Middleware/CorsTest.php @@ -16,7 +16,6 @@ public function responseDoesNotIncludeCorsHeadersWhenOriginIsNotSent(): void self::assertEquals(200, $resp->getStatusCode()); self::assertFalse($resp->hasHeader('Access-Control-Allow-Origin')); - self::assertFalse($resp->hasHeader('Access-Control-Expose-Headers')); self::assertFalse($resp->hasHeader('Access-Control-Allow-Methods')); self::assertFalse($resp->hasHeader('Access-Control-Max-Age')); self::assertFalse($resp->hasHeader('Access-Control-Allow-Headers')); @@ -37,7 +36,6 @@ public function responseIncludesCorsHeadersIfOriginIsSent( self::assertEquals($expectedStatusCode, $resp->getStatusCode()); self::assertEquals($origin, $resp->getHeaderLine('Access-Control-Allow-Origin')); - self::assertEquals('X-Api-Key', $resp->getHeaderLine('Access-Control-Expose-Headers')); self::assertFalse($resp->hasHeader('Access-Control-Allow-Methods')); self::assertFalse($resp->hasHeader('Access-Control-Max-Age')); self::assertFalse($resp->hasHeader('Access-Control-Allow-Headers')); @@ -66,7 +64,6 @@ public function preflightRequestsIncludeExtraCorsHeaders(string $endpoint, strin self::assertEquals(204, $resp->getStatusCode()); self::assertTrue($resp->hasHeader('Access-Control-Allow-Origin')); - self::assertTrue($resp->hasHeader('Access-Control-Expose-Headers')); self::assertTrue($resp->hasHeader('Access-Control-Max-Age')); self::assertEquals($expectedAllowedMethods, $resp->getHeaderLine('Access-Control-Allow-Methods')); self::assertEquals($allowedHeaders, $resp->getHeaderLine('Access-Control-Allow-Headers')); diff --git a/module/Rest/test/Middleware/CrossDomainMiddlewareTest.php b/module/Rest/test/Middleware/CrossDomainMiddlewareTest.php index 72e95a364..907fb678f 100644 --- a/module/Rest/test/Middleware/CrossDomainMiddlewareTest.php +++ b/module/Rest/test/Middleware/CrossDomainMiddlewareTest.php @@ -42,7 +42,6 @@ public function nonCrossDomainRequestsAreNotAffected(): void self::assertSame($originalResponse, $response); self::assertEquals(404, $response->getStatusCode()); self::assertArrayNotHasKey('Access-Control-Allow-Origin', $headers); - self::assertArrayNotHasKey('Access-Control-Expose-Headers', $headers); self::assertArrayNotHasKey('Access-Control-Allow-Methods', $headers); self::assertArrayNotHasKey('Access-Control-Max-Age', $headers); self::assertArrayNotHasKey('Access-Control-Allow-Headers', $headers); @@ -63,7 +62,6 @@ public function anyRequestIncludesTheAllowAccessHeader(): void $headers = $response->getHeaders(); self::assertEquals('local', $response->getHeaderLine('Access-Control-Allow-Origin')); - self::assertEquals('X-Api-Key', $response->getHeaderLine('Access-Control-Expose-Headers')); self::assertArrayNotHasKey('Access-Control-Allow-Methods', $headers); self::assertArrayNotHasKey('Access-Control-Max-Age', $headers); self::assertArrayNotHasKey('Access-Control-Allow-Headers', $headers); @@ -85,7 +83,6 @@ public function optionsRequestIncludesMoreHeaders(): void $headers = $response->getHeaders(); self::assertEquals('local', $response->getHeaderLine('Access-Control-Allow-Origin')); - self::assertEquals('X-Api-Key', $response->getHeaderLine('Access-Control-Expose-Headers')); self::assertArrayHasKey('Access-Control-Allow-Methods', $headers); self::assertEquals('1000', $response->getHeaderLine('Access-Control-Max-Age')); self::assertEquals('foo, bar, baz', $response->getHeaderLine('Access-Control-Allow-Headers')); From 9e7f2aea0d8101642c51671648634767cae348f7 Mon Sep 17 00:00:00 2001 From: Alejandro Celaya Date: Thu, 31 Dec 2020 15:42:00 +0100 Subject: [PATCH 3/3] Updated changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index b4554f60e..4ce3a5d63 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com), and this * [#896](https://github.com/shlinkio/shlink/issues/896) Added support for unicode characters in custom slugs. * [#930](https://github.com/shlinkio/shlink/issues/930) Added new `bin/set-option` script that allows changing individual configuration options on existing shlink instances. +* [#877](https://github.com/shlinkio/shlink/issues/877) Improved API tests on CORS, and "refined" middleware handling it. ### Changed * [#912](https://github.com/shlinkio/shlink/issues/912) Changed error templates to be plain html files, removing the dependency on `league/plates` package.