diff --git a/module/Core/src/Config/Options/CorsOptions.php b/module/Core/src/Config/Options/CorsOptions.php index f4a23139..841bf9fc 100644 --- a/module/Core/src/Config/Options/CorsOptions.php +++ b/module/Core/src/Config/Options/CorsOptions.php @@ -37,7 +37,19 @@ final readonly class CorsOptions ); } - public function responseWithAllowOrigin(RequestInterface $request, ResponseInterface $response): ResponseInterface + /** + * Creates a new response which contains the CORS headers that apply to provided request + */ + public function responseWithCorsHeaders(RequestInterface $request, ResponseInterface $response): ResponseInterface + { + $response = $this->responseWithAllowOrigin($request, $response); + return $this->allowCredentials ? $response->withHeader('Access-Control-Allow-Credentials', 'true') : $response; + } + + /** + * If applicable, a new response with the appropriate Access-Control-Allow-Origin header is returned + */ + private function responseWithAllowOrigin(RequestInterface $request, ResponseInterface $response): ResponseInterface { if ($this->allowOrigins === '*') { return $response->withHeader('Access-Control-Allow-Origin', '*'); diff --git a/module/Core/test/Config/Options/CorsOptionsTest.php b/module/Core/test/Config/Options/CorsOptionsTest.php index b02799b0..f7420269 100644 --- a/module/Core/test/Config/Options/CorsOptionsTest.php +++ b/module/Core/test/Config/Options/CorsOptionsTest.php @@ -9,6 +9,7 @@ use Laminas\Diactoros\ServerRequestFactory; use PHPUnit\Framework\Attributes\Test; use PHPUnit\Framework\Attributes\TestWith; use PHPUnit\Framework\TestCase; +use Psr\Http\Message\ResponseInterface; use Shlinkio\Shlink\Core\Config\Options\CorsOptions; class CorsOptionsTest extends TestCase @@ -28,10 +29,30 @@ class CorsOptionsTest extends TestCase self::assertEquals($expectedAllowOrigins, $options->allowOrigins); self::assertEquals( $expectedAllowOriginsHeader, - $options->responseWithAllowOrigin( - ServerRequestFactory::fromGlobals()->withHeader('Origin', 'https://example.com'), - new Response(), - )->getHeaderLine('Access-Control-Allow-Origin'), + $this->responseFromOptions($options)->getHeaderLine('Access-Control-Allow-Origin'), + ); + } + + #[Test] + #[TestWith([true])] + #[TestWith([false])] + public function expectedAccessControlAllowCredentialsIsSet(bool $allowCredentials): void + { + $options = new CorsOptions(allowCredentials: $allowCredentials); + $resp = $this->responseFromOptions($options); + + if ($allowCredentials) { + self::assertEquals('true', $resp->getHeaderLine('Access-Control-Allow-Credentials')); + } else { + self::assertFalse($resp->hasHeader('Access-Control-Allow-Credentials')); + } + } + + private function responseFromOptions(CorsOptions $options): ResponseInterface + { + return $options->responseWithCorsHeaders( + ServerRequestFactory::fromGlobals()->withHeader('Origin', 'https://example.com'), + new Response(), ); } } diff --git a/module/Rest/src/Middleware/CrossDomainMiddleware.php b/module/Rest/src/Middleware/CrossDomainMiddleware.php index 37360e2e..e54c07a7 100644 --- a/module/Rest/src/Middleware/CrossDomainMiddleware.php +++ b/module/Rest/src/Middleware/CrossDomainMiddleware.php @@ -28,7 +28,7 @@ readonly class CrossDomainMiddleware implements MiddlewareInterface, RequestMeth } // Add Allow-Origin header - $response = $this->options->responseWithAllowOrigin($request, $response); + $response = $this->options->responseWithCorsHeaders($request, $response); if ($request->getMethod() !== self::METHOD_OPTIONS) { return $response; } @@ -38,18 +38,13 @@ readonly class CrossDomainMiddleware implements MiddlewareInterface, RequestMeth private function addOptionsHeaders(ServerRequestInterface $request, ResponseInterface $response): ResponseInterface { - $corsHeaders = [ + // Options requests should always be empty and have a 204 status code + return EmptyResponse::withHeaders([ + ...$response->getHeaders(), 'Access-Control-Allow-Methods' => $this->resolveCorsAllowedMethods($response), 'Access-Control-Allow-Headers' => $request->getHeaderLine('Access-Control-Request-Headers'), 'Access-Control-Max-Age' => $this->options->maxAge, - ]; - - if ($this->options->allowCredentials) { - $corsHeaders['Access-Control-Allow-Credentials'] = 'true'; - } - - // Options requests should always be empty and have a 204 status code - return EmptyResponse::withHeaders([...$response->getHeaders(), ...$corsHeaders]); + ]); } private function resolveCorsAllowedMethods(ResponseInterface $response): string diff --git a/module/Rest/test-api/Middleware/CorsTest.php b/module/Rest/test-api/Middleware/CorsTest.php index 8198a21c..4ed3f2cb 100644 --- a/module/Rest/test-api/Middleware/CorsTest.php +++ b/module/Rest/test-api/Middleware/CorsTest.php @@ -21,6 +21,7 @@ class CorsTest extends ApiTestCase self::assertFalse($resp->hasHeader('Access-Control-Allow-Methods')); self::assertFalse($resp->hasHeader('Access-Control-Max-Age')); self::assertFalse($resp->hasHeader('Access-Control-Allow-Headers')); + self::assertFalse($resp->hasHeader('Access-Control-Allow-Credentials')); } #[Test, DataProvider('provideOrigins')] @@ -38,6 +39,7 @@ class CorsTest extends ApiTestCase self::assertFalse($resp->hasHeader('Access-Control-Allow-Methods')); self::assertFalse($resp->hasHeader('Access-Control-Max-Age')); self::assertFalse($resp->hasHeader('Access-Control-Allow-Headers')); + self::assertFalse($resp->hasHeader('Access-Control-Allow-Credentials')); } public static function provideOrigins(): iterable diff --git a/module/Rest/test/Middleware/CrossDomainMiddlewareTest.php b/module/Rest/test/Middleware/CrossDomainMiddlewareTest.php index 5893ca8c..0b3abe3f 100644 --- a/module/Rest/test/Middleware/CrossDomainMiddlewareTest.php +++ b/module/Rest/test/Middleware/CrossDomainMiddlewareTest.php @@ -4,6 +4,7 @@ declare(strict_types=1); namespace ShlinkioTest\Shlink\Rest\Middleware; +use Fig\Http\Message\RequestMethodInterface; use Laminas\Diactoros\Response; use Laminas\Diactoros\ServerRequest; use PHPUnit\Framework\Attributes\DataProvider; @@ -142,13 +143,17 @@ class CrossDomainMiddlewareTest extends TestCase } #[Test] - #[TestWith([true])] - #[TestWith([false])] - public function credentialsAreAllowedIfConfiguredSo(bool $allowCredentials): void + #[TestWith([true, RequestMethodInterface::METHOD_OPTIONS])] + #[TestWith([false, RequestMethodInterface::METHOD_OPTIONS])] + #[TestWith([true, RequestMethodInterface::METHOD_GET])] + #[TestWith([false, RequestMethodInterface::METHOD_GET])] + #[TestWith([true, RequestMethodInterface::METHOD_POST])] + #[TestWith([false, RequestMethodInterface::METHOD_POST])] + public function credentialsAreAllowedIfConfiguredSo(bool $allowCredentials, string $method): void { $originalResponse = new Response(); $request = (new ServerRequest()) - ->withMethod('OPTIONS') + ->withMethod($method) ->withHeader('Origin', 'local'); $this->handler->method('handle')->willReturn($originalResponse);