Merge pull request #948 from acelaya-forks/feature/cors-improvements

Feature/cors improvements
This commit is contained in:
Alejandro Celaya 2020-12-31 15:54:31 +01:00 committed by GitHub
commit 09029dff37
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
7 changed files with 107 additions and 9 deletions

View File

@ -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. * [#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. * [#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 ### Changed
* [#912](https://github.com/shlinkio/shlink/issues/912) Changed error templates to be plain html files, removing the dependency on `league/plates` package. * [#912](https://github.com/shlinkio/shlink/issues/912) Changed error templates to be plain html files, removing the dependency on `league/plates` package.

View File

@ -23,7 +23,6 @@
"doctrine/migrations": "^3.0.2", "doctrine/migrations": "^3.0.2",
"doctrine/orm": "^2.8", "doctrine/orm": "^2.8",
"endroid/qr-code": "^3.6", "endroid/qr-code": "^3.6",
"friendsofphp/proxy-manager-lts": "^1.0",
"geoip2/geoip2": "^2.9", "geoip2/geoip2": "^2.9",
"guzzlehttp/guzzle": "^7.0", "guzzlehttp/guzzle": "^7.0",
"laminas/laminas-config": "^3.3", "laminas/laminas-config": "^3.3",
@ -43,6 +42,7 @@
"mezzio/mezzio-swoole": "^2.6.4", "mezzio/mezzio-swoole": "^2.6.4",
"monolog/monolog": "^2.0", "monolog/monolog": "^2.0",
"nikolaposa/monolog-factory": "^3.1", "nikolaposa/monolog-factory": "^3.1",
"ocramius/proxy-manager": "^2.11",
"php-middleware/request-id": "^4.1", "php-middleware/request-id": "^4.1",
"predis/predis": "^1.1", "predis/predis": "^1.1",
"pugx/shortid-php": "^0.7", "pugx/shortid-php": "^0.7",

View File

@ -0,0 +1,11 @@
<?php
declare(strict_types=1);
return [
'cors' => [
'max_age' => 3600,
],
];

View File

@ -41,7 +41,7 @@ return [
ImplicitOptionsMiddleware::class => Middleware\EmptyResponseImplicitOptionsMiddlewareFactory::class, ImplicitOptionsMiddleware::class => Middleware\EmptyResponseImplicitOptionsMiddlewareFactory::class,
Middleware\BodyParserMiddleware::class => InvokableFactory::class, Middleware\BodyParserMiddleware::class => InvokableFactory::class,
Middleware\CrossDomainMiddleware::class => InvokableFactory::class, Middleware\CrossDomainMiddleware::class => ConfigAbstractFactory::class,
Middleware\ShortUrl\CreateShortUrlContentNegotiationMiddleware::class => InvokableFactory::class, Middleware\ShortUrl\CreateShortUrlContentNegotiationMiddleware::class => InvokableFactory::class,
Middleware\ShortUrl\DropDefaultDomainFromRequestMiddleware::class => ConfigAbstractFactory::class, Middleware\ShortUrl\DropDefaultDomainFromRequestMiddleware::class => ConfigAbstractFactory::class,
Middleware\ShortUrl\DefaultShortCodesLengthMiddleware::class => ConfigAbstractFactory::class, Middleware\ShortUrl\DefaultShortCodesLengthMiddleware::class => ConfigAbstractFactory::class,
@ -76,6 +76,7 @@ return [
Action\Tag\UpdateTagAction::class => [TagService::class], Action\Tag\UpdateTagAction::class => [TagService::class],
Action\Domain\ListDomainsAction::class => [DomainService::class, 'config.url_shortener.domain.hostname'], 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\DropDefaultDomainFromRequestMiddleware::class => ['config.url_shortener.domain.hostname'],
Middleware\ShortUrl\DefaultShortCodesLengthMiddleware::class => [ Middleware\ShortUrl\DefaultShortCodesLengthMiddleware::class => [
'config.url_shortener.default_short_codes_length', 'config.url_shortener.default_short_codes_length',

View File

@ -17,6 +17,13 @@ use function implode;
class CrossDomainMiddleware implements MiddlewareInterface, RequestMethodInterface class CrossDomainMiddleware implements MiddlewareInterface, RequestMethodInterface
{ {
private array $config;
public function __construct(array $config)
{
$this->config = $config;
}
public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface public function process(ServerRequestInterface $request, RequestHandlerInterface $handler): ResponseInterface
{ {
$response = $handler->handle($request); $response = $handler->handle($request);
@ -25,8 +32,7 @@ class CrossDomainMiddleware implements MiddlewareInterface, RequestMethodInterfa
} }
// Add Allow-Origin header // Add Allow-Origin header
$response = $response->withHeader('Access-Control-Allow-Origin', $request->getHeader('Origin')) $response = $response->withHeader('Access-Control-Allow-Origin', $request->getHeader('Origin'));
->withHeader('Access-Control-Expose-Headers', AuthenticationMiddleware::API_KEY_HEADER);
if ($request->getMethod() !== self::METHOD_OPTIONS) { if ($request->getMethod() !== self::METHOD_OPTIONS) {
return $response; return $response;
} }
@ -36,6 +42,8 @@ class CrossDomainMiddleware implements MiddlewareInterface, RequestMethodInterfa
private function addOptionsHeaders(ServerRequestInterface $request, ResponseInterface $response): ResponseInterface 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 */ /** @var RouteResult|null $matchedRoute */
$matchedRoute = $request->getAttribute(RouteResult::class); $matchedRoute = $request->getAttribute(RouteResult::class);
$matchedMethods = $matchedRoute !== null ? $matchedRoute->getAllowedMethods() : [ $matchedMethods = $matchedRoute !== null ? $matchedRoute->getAllowedMethods() : [
@ -48,8 +56,8 @@ class CrossDomainMiddleware implements MiddlewareInterface, RequestMethodInterfa
]; ];
$corsHeaders = [ $corsHeaders = [
'Access-Control-Allow-Methods' => implode(',', $matchedMethods), 'Access-Control-Allow-Methods' => implode(',', $matchedMethods),
'Access-Control-Max-Age' => '1000',
'Access-Control-Allow-Headers' => $request->getHeaderLine('Access-Control-Request-Headers'), '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 // Options requests should always be empty and have a 204 status code

View File

@ -0,0 +1,80 @@
<?php
declare(strict_types=1);
namespace ShlinkioApiTest\Shlink\Rest\Middleware;
use GuzzleHttp\RequestOptions;
use Shlinkio\Shlink\TestUtils\ApiTest\ApiTestCase;
class CorsTest extends ApiTestCase
{
/** @test */
public function responseDoesNotIncludeCorsHeadersWhenOriginIsNotSent(): void
{
$resp = $this->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-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::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-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
}
}

View File

@ -26,7 +26,7 @@ class CrossDomainMiddlewareTest extends TestCase
public function setUp(): void public function setUp(): void
{ {
$this->middleware = new CrossDomainMiddleware(); $this->middleware = new CrossDomainMiddleware(['max_age' => 1000]);
$this->handler = $this->prophesize(RequestHandlerInterface::class); $this->handler = $this->prophesize(RequestHandlerInterface::class);
} }
@ -42,7 +42,6 @@ class CrossDomainMiddlewareTest extends TestCase
self::assertSame($originalResponse, $response); self::assertSame($originalResponse, $response);
self::assertEquals(404, $response->getStatusCode()); self::assertEquals(404, $response->getStatusCode());
self::assertArrayNotHasKey('Access-Control-Allow-Origin', $headers); 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-Allow-Methods', $headers);
self::assertArrayNotHasKey('Access-Control-Max-Age', $headers); self::assertArrayNotHasKey('Access-Control-Max-Age', $headers);
self::assertArrayNotHasKey('Access-Control-Allow-Headers', $headers); self::assertArrayNotHasKey('Access-Control-Allow-Headers', $headers);
@ -63,7 +62,6 @@ class CrossDomainMiddlewareTest extends TestCase
$headers = $response->getHeaders(); $headers = $response->getHeaders();
self::assertEquals('local', $response->getHeaderLine('Access-Control-Allow-Origin')); 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-Allow-Methods', $headers);
self::assertArrayNotHasKey('Access-Control-Max-Age', $headers); self::assertArrayNotHasKey('Access-Control-Max-Age', $headers);
self::assertArrayNotHasKey('Access-Control-Allow-Headers', $headers); self::assertArrayNotHasKey('Access-Control-Allow-Headers', $headers);
@ -85,7 +83,6 @@ class CrossDomainMiddlewareTest extends TestCase
$headers = $response->getHeaders(); $headers = $response->getHeaders();
self::assertEquals('local', $response->getHeaderLine('Access-Control-Allow-Origin')); 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::assertArrayHasKey('Access-Control-Allow-Methods', $headers);
self::assertEquals('1000', $response->getHeaderLine('Access-Control-Max-Age')); self::assertEquals('1000', $response->getHeaderLine('Access-Control-Max-Age'));
self::assertEquals('foo, bar, baz', $response->getHeaderLine('Access-Control-Allow-Headers')); self::assertEquals('foo, bar, baz', $response->getHeaderLine('Access-Control-Allow-Headers'));