Merge pull request #2489 from acelaya-forks/feature/cors-credentials-fix

Make sure Access-Control-Allow-Credentials is always set if configured

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', '*');

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(
+            $this->responseFromOptions($options)->getHeaderLine('Access-Control-Allow-Origin'),
-                ServerRequestFactory::fromGlobals()->withHeader('Origin', 'https://example.com'),
+        );
-                new Response(),
+    }
-            )->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(),
         );
     }
 }

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

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

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([true, RequestMethodInterface::METHOD_OPTIONS])]
-    #[TestWith([false])]
+    #[TestWith([false, RequestMethodInterface::METHOD_OPTIONS])]
-    public function credentialsAreAllowedIfConfiguredSo(bool $allowCredentials): void
+    #[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);