Skip to content

Commit

Permalink
Merge pull request #948 from acelaya-forks/feature/cors-improvements
Browse files Browse the repository at this point in the history
Feature/cors improvements
  • Loading branch information
acelaya committed Dec 31, 2020
2 parents 202a732 + 9e7f2ae commit 09029df
Show file tree
Hide file tree
Showing 7 changed files with 107 additions and 9 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
2 changes: 1 addition & 1 deletion composer.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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",
Expand Down
11 changes: 11 additions & 0 deletions config/autoload/cors.global.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<?php

declare(strict_types=1);

return [

'cors' => [
'max_age' => 3600,
],

];
3 changes: 2 additions & 1 deletion module/Rest/config/dependencies.config.php
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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',
Expand Down
14 changes: 11 additions & 3 deletions module/Rest/src/Middleware/CrossDomainMiddleware.php
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -25,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;
}
Expand All @@ -36,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() : [
Expand All @@ -48,8 +56,8 @@ private function addOptionsHeaders(ServerRequestInterface $request, ResponseInte
];
$corsHeaders = [
'Access-Control-Allow-Methods' => implode(',', $matchedMethods),
'Access-Control-Max-Age' => '1000',
'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
Expand Down
80 changes: 80 additions & 0 deletions module/Rest/test-api/Middleware/CorsTest.php
Original file line number Diff line number Diff line change
@@ -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
}
}
5 changes: 1 addition & 4 deletions module/Rest/test/Middleware/CrossDomainMiddlewareTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

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

0 comments on commit 09029df

Please sign in to comment.