From 63e4037dc7f91526cf05d156864d8ca073060174 Mon Sep 17 00:00:00 2001 From: michalsn Date: Mon, 27 Jan 2025 08:35:47 +0100 Subject: [PATCH 1/4] fix: set everything in the before filter for CORS --- system/Filters/Cors.php | 42 +++++++------------- user_guide_src/source/changelogs/v4.6.1.rst | 3 ++ user_guide_src/source/libraries/cors/002.php | 1 - 3 files changed, 17 insertions(+), 29 deletions(-) diff --git a/system/Filters/Cors.php b/system/Filters/Cors.php index 9a9bbb208152..cb0e09c138ea 100644 --- a/system/Filters/Cors.php +++ b/system/Filters/Cors.php @@ -58,22 +58,24 @@ public function before(RequestInterface $request, $arguments = null) $this->createCorsService($arguments); - if (! $this->cors->isPreflightRequest($request)) { - return null; - } - /** @var ResponseInterface $response */ $response = service('response'); - $response = $this->cors->handlePreflightRequest($request, $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'); + } + + if ($this->cors->isPreflightRequest($request)) { + return $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'); + $this->cors->addResponseHeaders($request, $response); - return $response; + return null; } /** @@ -87,25 +89,9 @@ private function createCorsService(?array $arguments): void /** * @param list|null $arguments - * - * @return ResponseInterface|null */ public function after(RequestInterface $request, ResponseInterface $response, $arguments = null) { - if (! $request instanceof IncomingRequest) { - return null; - } - - $this->createCorsService($arguments); - - // 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. - if ($request->is('OPTIONS')) { - $response->appendHeader('Vary', 'Access-Control-Request-Method'); - } - - return $this->cors->addResponseHeaders($request, $response); + return null; } } diff --git a/user_guide_src/source/changelogs/v4.6.1.rst b/user_guide_src/source/changelogs/v4.6.1.rst index c230ee6d32b9..5f502cc02207 100644 --- a/user_guide_src/source/changelogs/v4.6.1.rst +++ b/user_guide_src/source/changelogs/v4.6.1.rst @@ -22,6 +22,8 @@ Message Changes Changes ******* +- **Cors:** From now on only the ``before`` filter is used. You can remove all the ``after`` filter occurrences from your configuration for CORS. + ************ Deprecations ************ @@ -31,6 +33,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. From now on all CORS headers are added in the ``before`` filter and the ``after`` filter is no longer used. See the repo's `CHANGELOG.md `_ diff --git a/user_guide_src/source/libraries/cors/002.php b/user_guide_src/source/libraries/cors/002.php index 1229eaace3a4..cee6ab9e8566 100644 --- a/user_guide_src/source/libraries/cors/002.php +++ b/user_guide_src/source/libraries/cors/002.php @@ -13,7 +13,6 @@ class Filters extends BaseFilters // ... 'cors' => [ 'before' => ['api/*'], - 'after' => ['api/*'], ], ]; } From 729969632a6f80d01b5eb4e0593e4461ea1ddfba Mon Sep 17 00:00:00 2001 From: michalsn Date: Mon, 27 Jan 2025 08:52:13 +0100 Subject: [PATCH 2/4] fix append vary header --- system/Filters/Cors.php | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/system/Filters/Cors.php b/system/Filters/Cors.php index cb0e09c138ea..3ed2d7d1c66e 100644 --- a/system/Filters/Cors.php +++ b/system/Filters/Cors.php @@ -61,16 +61,24 @@ public function before(RequestInterface $request, $arguments = null) /** @var ResponseInterface $response */ $response = service('response'); - if ($request->is('OPTIONS')) { + 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'); + + return $response; } - if ($this->cors->isPreflightRequest($request)) { - return $this->cors->handlePreflightRequest($request, $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); From ff66dd83e1c20b9694f133f80fe33a5cc68039d7 Mon Sep 17 00:00:00 2001 From: michalsn Date: Fri, 31 Jan 2025 08:27:18 +0100 Subject: [PATCH 3/4] cors: apply response headers in the after filter only if they are not already set --- system/Filters/Cors.php | 20 ++++++++- system/HTTP/Cors.php | 24 +++++++++++ tests/system/Filters/CorsTest.php | 45 ++++++++++++++++++++ tests/system/HTTP/CorsTest.php | 37 ++++++++++++++++ user_guide_src/source/changelogs/v4.6.1.rst | 4 +- user_guide_src/source/libraries/cors/002.php | 1 + 6 files changed, 127 insertions(+), 4 deletions(-) diff --git a/system/Filters/Cors.php b/system/Filters/Cors.php index 3ed2d7d1c66e..c070d72925e5 100644 --- a/system/Filters/Cors.php +++ b/system/Filters/Cors.php @@ -100,6 +100,24 @@ private function createCorsService(?array $arguments): void */ public function after(RequestInterface $request, ResponseInterface $response, $arguments = null) { - return null; + if (! $request instanceof IncomingRequest) { + return null; + } + + $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 + // have this header, so it will be cached separately. + if ($request->is('OPTIONS')) { + $response->appendHeader('Vary', 'Access-Control-Request-Method'); + } + + return $this->cors->addResponseHeaders($request, $response); } } diff --git a/system/HTTP/Cors.php b/system/HTTP/Cors.php index 2f151a738344..3c019ffcb637 100644 --- a/system/HTTP/Cors.php +++ b/system/HTTP/Cors.php @@ -227,4 +227,28 @@ private function setExposeHeaders(ResponseInterface $response): void ); } } + + /** + * 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; + } + + if ($this->config['exposedHeaders'] !== [] && (! $response->hasHeader('Access-Control-Expose-Headers') || ! str_contains( + $response->getHeaderLine('Access-Control-Expose-Headers'), + implode(', ', $this->config['exposedHeaders']), + ))) { + return false; + } + + return true; + } } diff --git a/tests/system/Filters/CorsTest.php b/tests/system/Filters/CorsTest.php index 5e25af0572f2..f8f0fb3383a2 100644 --- a/tests/system/Filters/CorsTest.php +++ b/tests/system/Filters/CorsTest.php @@ -16,6 +16,7 @@ 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 @@ private function handle(RequestInterface $request): ResponseInterface 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 @@ public function testItAddsVaryAccessControlRequestMethodHeaderEvenIfItIsNormalOp // 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 1002c557d313..5373959746ac 100644 --- a/tests/system/HTTP/CorsTest.php +++ b/tests/system/HTTP/CorsTest.php @@ -521,4 +521,41 @@ public function testAddResponseHeadersMultipleAllowedOriginsNotAllowed(): void $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 5f502cc02207..9a3ccfe76490 100644 --- a/user_guide_src/source/changelogs/v4.6.1.rst +++ b/user_guide_src/source/changelogs/v4.6.1.rst @@ -22,8 +22,6 @@ Message Changes Changes ******* -- **Cors:** From now on only the ``before`` filter is used. You can remove all the ``after`` filter occurrences from your configuration for CORS. - ************ Deprecations ************ @@ -33,7 +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. From now on all CORS headers are added in the ``before`` filter and the ``after`` filter is no longer used. +- **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 `_ diff --git a/user_guide_src/source/libraries/cors/002.php b/user_guide_src/source/libraries/cors/002.php index cee6ab9e8566..1229eaace3a4 100644 --- a/user_guide_src/source/libraries/cors/002.php +++ b/user_guide_src/source/libraries/cors/002.php @@ -13,6 +13,7 @@ class Filters extends BaseFilters // ... 'cors' => [ 'before' => ['api/*'], + 'after' => ['api/*'], ], ]; } From 1b7b1ae97ce31e541567b80b7eb7fd224721c5fa Mon Sep 17 00:00:00 2001 From: michalsn Date: Fri, 31 Jan 2025 08:31:50 +0100 Subject: [PATCH 4/4] cs fix --- system/HTTP/Cors.php | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/system/HTTP/Cors.php b/system/HTTP/Cors.php index 3c019ffcb637..270b344abb8b 100644 --- a/system/HTTP/Cors.php +++ b/system/HTTP/Cors.php @@ -242,13 +242,9 @@ public function hasResponseHeaders(RequestInterface $request, ResponseInterface return false; } - if ($this->config['exposedHeaders'] !== [] && (! $response->hasHeader('Access-Control-Expose-Headers') || ! str_contains( + return ! ($this->config['exposedHeaders'] !== [] && (! $response->hasHeader('Access-Control-Expose-Headers') || ! str_contains( $response->getHeaderLine('Access-Control-Expose-Headers'), implode(', ', $this->config['exposedHeaders']), - ))) { - return false; - } - - return true; + ))); } }