From c430f347200ec3a505cb48233f577edef5a1de5c Mon Sep 17 00:00:00 2001 From: Michal Sniatala Date: Sat, 8 Feb 2025 19:43:28 +0100 Subject: [PATCH] fix: set headers for CORS (#9437) * fix: set everything in the before filter for CORS * fix append vary header * cors: apply response headers in the after filter only if they are not already set * cs fix --- system/Filters/Cors.php | 38 +++++++++++------ system/HTTP/Cors.php | 20 +++++++++ tests/system/Filters/CorsTest.php | 45 +++++++++++++++++++++ tests/system/HTTP/CorsTest.php | 37 +++++++++++++++++ user_guide_src/source/changelogs/v4.6.1.rst | 1 + 5 files changed, 128 insertions(+), 13 deletions(-) diff --git a/system/Filters/Cors.php b/system/Filters/Cors.php index 9a9bbb2081..c070d72925 100644 --- a/system/Filters/Cors.php +++ b/system/Filters/Cors.php @@ -58,22 +58,32 @@ class Cors implements FilterInterface $this->createCorsService($arguments); - if (! $this->cors->isPreflightRequest($request)) { - return null; - } - /** @var ResponseInterface $response */ $response = service('response'); - $response = $this->cors->handlePreflightRequest($request, $response); + if ($this->cors->isPreflightRequest($request)) { + $response = $this->cors->handlePreflightRequest($request, $response); - // Always adds `Vary: Access-Control-Request-Method` header for cacheability. - // If there is an intermediate cache server such as a CDN, if a plain - // OPTIONS request is sent, it may be cached. But valid preflight requests - // have this header, so it will be cached separately. - $response->appendHeader('Vary', 'Access-Control-Request-Method'); + // Always adds `Vary: Access-Control-Request-Method` header for cacheability. + // If there is an intermediate cache server such as a CDN, if a plain + // OPTIONS request is sent, it may be cached. But valid preflight requests + // have this header, so it will be cached separately. + $response->appendHeader('Vary', 'Access-Control-Request-Method'); - return $response; + return $response; + } + + if ($request->is('OPTIONS')) { + // Always adds `Vary: Access-Control-Request-Method` header for cacheability. + // If there is an intermediate cache server such as a CDN, if a plain + // OPTIONS request is sent, it may be cached. But valid preflight requests + // have this header, so it will be cached separately. + $response->appendHeader('Vary', 'Access-Control-Request-Method'); + } + + $this->cors->addResponseHeaders($request, $response); + + return null; } /** @@ -87,8 +97,6 @@ class Cors implements FilterInterface /** * @param list|null $arguments - * - * @return ResponseInterface|null */ public function after(RequestInterface $request, ResponseInterface $response, $arguments = null) { @@ -98,6 +106,10 @@ class Cors implements FilterInterface $this->createCorsService($arguments); + if ($this->cors->hasResponseHeaders($request, $response)) { + return null; + } + // Always adds `Vary: Access-Control-Request-Method` header for cacheability. // If there is an intermediate cache server such as a CDN, if a plain // OPTIONS request is sent, it may be cached. But valid preflight requests diff --git a/system/HTTP/Cors.php b/system/HTTP/Cors.php index 2f151a7383..270b344abb 100644 --- a/system/HTTP/Cors.php +++ b/system/HTTP/Cors.php @@ -227,4 +227,24 @@ class Cors ); } } + + /** + * Check if response headers were set + */ + public function hasResponseHeaders(RequestInterface $request, ResponseInterface $response): bool + { + if (! $response->hasHeader('Access-Control-Allow-Origin')) { + return false; + } + + if ($this->config['supportsCredentials'] + && ! $response->hasHeader('Access-Control-Allow-Credentials')) { + return false; + } + + return ! ($this->config['exposedHeaders'] !== [] && (! $response->hasHeader('Access-Control-Expose-Headers') || ! str_contains( + $response->getHeaderLine('Access-Control-Expose-Headers'), + implode(', ', $this->config['exposedHeaders']), + ))); + } } diff --git a/tests/system/Filters/CorsTest.php b/tests/system/Filters/CorsTest.php index 1cbf471821..38389f25c2 100644 --- a/tests/system/Filters/CorsTest.php +++ b/tests/system/Filters/CorsTest.php @@ -16,6 +16,7 @@ namespace CodeIgniter\Filters; use CodeIgniter\Exceptions\ConfigException; use CodeIgniter\HTTP\CLIRequest; use CodeIgniter\HTTP\IncomingRequest; +use CodeIgniter\HTTP\RedirectResponse; use CodeIgniter\HTTP\RequestInterface; use CodeIgniter\HTTP\ResponseInterface; use CodeIgniter\HTTP\SiteURI; @@ -154,6 +155,25 @@ final class CorsTest extends CIUnitTestCase return $response; } + private function handleRedirect(RequestInterface $request): ResponseInterface + { + $response = $this->cors->before($request); + if ($response instanceof ResponseInterface) { + $this->response = $response; + + return $response; + } + + $response = service('redirectresponse'); + + $response = $this->cors->after($request, $response); + $response ??= service('redirectresponse'); + + $this->response = $response; + + return $response; + } + public function testItDoesModifyOnARequestWithSameOrigin(): void { $this->cors = $this->createCors(['allowedOrigins' => ['*']]); @@ -461,4 +481,29 @@ final class CorsTest extends CIUnitTestCase // Always adds `Vary: Access-Control-Request-Method` header. $this->assertHeader('Vary', 'Access-Control-Request-Method'); } + + public function testItReturnsAllowOriginHeaderOnValidActualRequestWithRedirect(): void + { + $this->cors = $this->createCors(); + $request = $this->createValidActualRequest(); + + $response = $this->handleRedirect($request); + + $this->assertInstanceOf(RedirectResponse::class, $response); + $this->assertTrue($response->hasHeader('Access-Control-Allow-Origin')); + $this->assertHeader('Access-Control-Allow-Origin', 'http://localhost'); + } + + public function testItReturnsAllowOriginHeaderOnAllowAllOriginRequestWithRedirect(): void + { + $this->cors = $this->createCors(['allowedOrigins' => ['*']]); + $request = $this->createRequest(); + $request->setHeader('Origin', 'http://localhost'); + + $response = $this->handleRedirect($request); + + $this->assertInstanceOf(RedirectResponse::class, $response); + $this->assertTrue($response->hasHeader('Access-Control-Allow-Origin')); + $this->assertHeader('Access-Control-Allow-Origin', '*'); + } } diff --git a/tests/system/HTTP/CorsTest.php b/tests/system/HTTP/CorsTest.php index 1002c557d3..5373959746 100644 --- a/tests/system/HTTP/CorsTest.php +++ b/tests/system/HTTP/CorsTest.php @@ -521,4 +521,41 @@ final class CorsTest extends CIUnitTestCase $response->hasHeader('Access-Control-Allow-Methods'), ); } + + public function testHasResponseHeadersFalse(): void + { + $config = $this->getDefaultConfig(); + $config['allowedOrigins'] = ['http://localhost:8080']; + $config['allowedMethods'] = ['GET', 'POST', 'PUT']; + $cors = $this->createCors($config); + + $request = $this->createRequest() + ->withMethod('GET') + ->setHeader('Origin', 'http://localhost:8080'); + + $response = service('redirectresponse', null, false); + + $this->assertFalse( + $cors->hasResponseHeaders($request, $response), + ); + } + + public function testHasResponseHeadersTrue(): void + { + $config = $this->getDefaultConfig(); + $config['allowedOrigins'] = ['http://localhost:8080']; + $config['allowedMethods'] = ['GET', 'POST']; + $cors = $this->createCors($config); + + $request = $this->createRequest() + ->withMethod('GET') + ->setHeader('Origin', 'http://localhost:8080'); + + $response = service('redirectresponse', null, false); + $response = $cors->addResponseHeaders($request, $response); + + $this->assertTrue( + $cors->hasResponseHeaders($request, $response), + ); + } } diff --git a/user_guide_src/source/changelogs/v4.6.1.rst b/user_guide_src/source/changelogs/v4.6.1.rst index c230ee6d32..9a3ccfe764 100644 --- a/user_guide_src/source/changelogs/v4.6.1.rst +++ b/user_guide_src/source/changelogs/v4.6.1.rst @@ -31,6 +31,7 @@ Bugs Fixed ********** - **CURLRequest:** Fixed an issue where multiple header sections appeared in the CURL response body during multiple redirects from the target server. +- **Cors:** Fixed a bug in the Cors filter that caused the appropriate headers to not be added when another filter returned a response object in the ``before`` filter. See the repo's `CHANGELOG.md `_