From 5f35f870651aa9421da6775f3e292aaf13b4b448 Mon Sep 17 00:00:00 2001 From: michalsn Date: Fri, 25 Oct 2024 12:29:19 +0200 Subject: [PATCH 1/5] fix: prioritize headers set by the Response class --- system/HTTP/ResponseTrait.php | 6 ++- tests/system/HTTP/ResponseSendTest.php | 50 +++++++++++++++++++++ user_guide_src/source/changelogs/v4.6.0.rst | 14 ++++++ 3 files changed, 68 insertions(+), 2 deletions(-) diff --git a/system/HTTP/ResponseTrait.php b/system/HTTP/ResponseTrait.php index 726e85bae5a4..bb785b6e644d 100644 --- a/system/HTTP/ResponseTrait.php +++ b/system/HTTP/ResponseTrait.php @@ -403,16 +403,18 @@ public function sendHeaders() if ($value instanceof Header) { header( $name . ': ' . $value->getValueLine(), - false, + true, $this->getStatusCode() ); } else { + $replace = true; foreach ($value as $header) { header( $name . ': ' . $header->getValueLine(), - false, + $replace, $this->getStatusCode() ); + $replace = false; } } } diff --git a/tests/system/HTTP/ResponseSendTest.php b/tests/system/HTTP/ResponseSendTest.php index c7ad15195d97..b11bc1f8cbcf 100644 --- a/tests/system/HTTP/ResponseSendTest.php +++ b/tests/system/HTTP/ResponseSendTest.php @@ -179,4 +179,54 @@ public function testDoNotSendUnSecureCookie(): void // send it $response->send(); } + + /** + * Make sure that the headers set by the header() function + * are overridden by the headers defined in the Response class. + */ + #[PreserveGlobalState(false)] + #[RunInSeparateProcess] + #[WithoutErrorHandler] + public function testHeaderOverride(): void + { + $response = new Response(new App()); + $response->pretend(false); + + $body = 'Hello'; + $response->setBody($body); + + // single header + $response->setHeader('Vary', 'Accept-Encoding'); + $this->assertSame('Accept-Encoding', $response->header('Vary')->getValue()); + + // multiple headers + $response->setHeader('Access-Control-Expose-Headers', 'X-Custom-Header'); + $response->addHeader('Access-Control-Expose-Headers', 'Content-Length'); + $header = $response->header('Access-Control-Expose-Headers'); + $this->assertIsArray($header); + $this->assertSame('X-Custom-Header', $header[0]->getValue()); + $this->assertSame('Content-Length', $header[1]->getValue()); + + // send it + ob_start(); + header('Vary: User-Agent'); + header('Access-Control-Expose-Headers: Content-Encoding'); + header('Allow: GET, POST'); + $response->send(); + if (ob_get_level() > 0) { + ob_end_clean(); + } + + // single header + $this->assertHeaderEmitted('Vary: Accept-Encoding'); + $this->assertHeaderNotEmitted('Vary: User-Agent'); + + // multiple headers + $this->assertHeaderEmitted('Access-Control-Expose-Headers: X-Custom-Header'); + $this->assertHeaderEmitted('Access-Control-Expose-Headers: Content-Length'); + $this->assertHeaderNotEmitted('Access-Control-Expose-Headers: Content-Encoding'); + + // not overridden by the response class + $this->assertHeaderEmitted('Allow: GET, POST'); + } } diff --git a/user_guide_src/source/changelogs/v4.6.0.rst b/user_guide_src/source/changelogs/v4.6.0.rst index 14edaa14ce29..ddc1e811b51a 100644 --- a/user_guide_src/source/changelogs/v4.6.0.rst +++ b/user_guide_src/source/changelogs/v4.6.0.rst @@ -99,6 +99,16 @@ See :ref:`Upgrading Guide ` for details. .. _v460-interface-changes: +Headers +------- + +The headers set by the ``Response`` class replace those that can be set by the PHP +``header()`` function. + +In previous versions, headers set by the ``Response`` class were added to existing +ones - giving no options to change them. That could lead to unexpected behavior when +the same headers were set with mutually exclusive directives. + Interface Changes ================= @@ -298,6 +308,10 @@ Deprecations Bugs Fixed ********** +- **Response:** + - Headers set using the ``Response`` class are now prioritized and replace headers + that can be set manually using the PHP ``header()`` function. + See the repo's `CHANGELOG.md `_ for a complete list of bugs fixed. From bdc1a0b1991014f44b6e4edbe75ea2a9d5a39a3a Mon Sep 17 00:00:00 2001 From: michalsn Date: Fri, 25 Oct 2024 12:32:41 +0200 Subject: [PATCH 2/5] cs fix --- system/HTTP/ResponseTrait.php | 1 + 1 file changed, 1 insertion(+) diff --git a/system/HTTP/ResponseTrait.php b/system/HTTP/ResponseTrait.php index bb785b6e644d..ede7d89770ed 100644 --- a/system/HTTP/ResponseTrait.php +++ b/system/HTTP/ResponseTrait.php @@ -408,6 +408,7 @@ public function sendHeaders() ); } else { $replace = true; + foreach ($value as $header) { header( $name . ': ' . $header->getValueLine(), From dd7c025ef5c2e688496da640194e58610ccf4af9 Mon Sep 17 00:00:00 2001 From: michalsn Date: Fri, 25 Oct 2024 12:51:44 +0200 Subject: [PATCH 3/5] add a note to the user guide --- user_guide_src/source/outgoing/response.rst | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/user_guide_src/source/outgoing/response.rst b/user_guide_src/source/outgoing/response.rst index c23b44b700fa..034eca9289b6 100644 --- a/user_guide_src/source/outgoing/response.rst +++ b/user_guide_src/source/outgoing/response.rst @@ -57,6 +57,10 @@ which can be either a string or an array of values that will be combined correct Using these functions instead of using the native PHP functions allows you to ensure that no headers are sent prematurely, causing errors, and makes testing possible. +.. important:: Since v4.6.0, if you set a header using PHP's native ``header()`` + function and then use the ``Response`` class to set the same header, the + previous one will be overwritten. + .. note:: This method just sets headers to the response instance. So, if you create and return another response instance (e.g., if you call :php:func:`redirect()`), the headers set here will not be sent automatically. From 63f755acad3e2cdd69cc8bb30e072abedbf26c0e Mon Sep 17 00:00:00 2001 From: michalsn Date: Wed, 25 Dec 2024 08:00:26 +0100 Subject: [PATCH 4/5] docs: add an example to the header changes --- user_guide_src/source/changelogs/v4.6.0.rst | 32 +++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/user_guide_src/source/changelogs/v4.6.0.rst b/user_guide_src/source/changelogs/v4.6.0.rst index ddc1e811b51a..de80820e9ee4 100644 --- a/user_guide_src/source/changelogs/v4.6.0.rst +++ b/user_guide_src/source/changelogs/v4.6.0.rst @@ -109,6 +109,38 @@ In previous versions, headers set by the ``Response`` class were added to existi ones - giving no options to change them. That could lead to unexpected behavior when the same headers were set with mutually exclusive directives. +For example, session will automatically set headers with the ``header()`` function: + +.. code-block:: none + + Expires: Thu, 19 Nov 1981 08:52:00 GMT + Cache-Control: no-store, no-cache, must-revalidate + Pragma: no-cache + +So if we set **Expires** header one more time we will end up with a duplicated header: + +.. code-block:: php + + $response->removeHeader('Expires'); // has no effect + return $response->setHeader('Expires', 'Sun, 17 Nov 2024 14:17:37 GMT'); + +Response headers: + +.. code-block:: none + + Expires: Thu, 19 Nov 1981 08:52:00 GMT + // ... + Expires: Sun, 17 Nov 2024 14:17:37 GMT + +Now, we don't know which one will be picked by the browser or which header is the correct one. +With changes in this version our previous header will be override: + +.. code-block:: none + + Cache-Control: no-store, no-cache, must-revalidate + Pragma: no-cache + Expires: Sun, 17 Nov 2024 14:17:37 GMT + Interface Changes ================= From 7355199b01d0da3f1fdfb2f8c0d47de820d9f651 Mon Sep 17 00:00:00 2001 From: Michal Sniatala Date: Fri, 27 Dec 2024 08:27:41 +0100 Subject: [PATCH 5/5] Update user_guide_src/source/changelogs/v4.6.0.rst Co-authored-by: John Paul E. Balandan, CPA --- user_guide_src/source/changelogs/v4.6.0.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/user_guide_src/source/changelogs/v4.6.0.rst b/user_guide_src/source/changelogs/v4.6.0.rst index de80820e9ee4..2c75473b5220 100644 --- a/user_guide_src/source/changelogs/v4.6.0.rst +++ b/user_guide_src/source/changelogs/v4.6.0.rst @@ -133,7 +133,7 @@ Response headers: Expires: Sun, 17 Nov 2024 14:17:37 GMT Now, we don't know which one will be picked by the browser or which header is the correct one. -With changes in this version our previous header will be override: +With changes in this version our previous header will be overridden: .. code-block:: none