fix: use `addHeader()` #8240

kenjis · 2023-11-21T08:49:26Z

Description
Follow-up #8194
Fixes #5041

Checklist:

Securely signed commits
Component(s) with PHPDoc blocks, only if necessary or adds value
Unit testing, with >80% coverage
User guide updated
Conforms to style guide

------ -------------------------------------------------- Line system/HTTP/CURLRequest.php ------ -------------------------------------------------- 485 Call to an undefined method CodeIgniter\HTTP\ResponseInterface::addHeader(). ------ --------------------------------------------------

------ ------------------------------------------------------------------- Line system/Debug/Toolbar.php ------ ------------------------------------------------------------------- 150 Binary operation "+" between non-falsy-string and 1 results in an error. 175 Binary operation "+" between non-falsy-string and 1 results in an error. ------ -------------------------------------------------------------------

NicolaeIotu · 2023-11-22T08:25:43Z

I'm setting up the environment and revert when done testing

NicolaeIotu

Hi,

Not recommended for production as it is.
I tested these changes and found the following:

Cannot find method addHeader. Is this method added recently? Action: replaced with method appendHeader and continued testing
File system/HTTP/CURLRequest.php @ change "if ($this->response instanceof Response) {": this check may be feasible but not here. If $this->response is not a Response why $this->response->setHeader(... ? and what is the connection with the existence of a certain header? Action: replaced with "if ($this->response->hasHeader($title)) {" and continued testing
This observation may or may not be correct or acceptable for the framework. I'm personally redirecting while appending set-cookie headers obtained from a remote CURLRequest. When this happens my proposal is able to detect all the headers (including the ones appended manually) and 'sendHeaders' correctly in file system/HTTP/ResponseTrait.php. This is a sensitive syntax or you won't be able to get all the headers:

foreach (array_keys($this->headers()) as $name)
{
        $headerValue = $this->getHeader($name)->getValue();
        // !!! custom adjustment to framework
        if(is_array($headerValue)) {
                foreach ($headerValue as $h_value) {
                        header($name . ': ' . $h_value, false, $this->getStatusCode());
                }
        } else {
                header($name . ': ' . $this->getHeaderLine($name), false, $this->getStatusCode());
        }
}

Bottom line is that the change in file system/HTTP/ResponseTrait.php may be correct, but cannot detect all the headers including the ones set manually.

The following is probably a subject for another PR.
When CURLRequest 'verify' is set to false the only action taken is "$curlOptions[CURLOPT_SSL_VERIFYPEER] = $config['verify']".
This may appear to be correct, but actually it relies on a few defaults and assumptions which are mostly correct except for some use cases.
If the request is done to services on private networks where there is no intention or possibility of making host checks when doing SSL requests, then these requests will fail.
For my purposes I'm just adding the following:

if ($config['verify'] === FALSE) {
    $curlOptions[CURLOPT_SSL_VERIFYHOST] = 0;
}

Note that requests to services on private networks are something common nowadays. It may be useful to add an option for CURLOPT_SSL_VERIFYHOST. Some will want to make the checks and some will want to skip this check.
For example the tool curl has the option -k which skips any SSL checks. This is very useful on private networks a.o. cases.

That's all I could find for now. I really hope it helps.

kenjis · 2023-11-22T21:49:26Z

Cannot find method addHeader. Is this method added recently? Action: replaced with method appendHeader and continued testing

Can you tell us what you did to test this?
You must checkout this PR branch or download the branch.
https://github.com/kenjis/CodeIgniter4/tree/fix-use-addHeader

kenjis · 2023-11-22T21:53:32Z

The CURLOPT_SSL_VERIFYHOST thing has nothing to do with this PR, so please don't put it here.
It will be forgotten once this PR is completed.
If you think it is a bug, please submit a PR or create an Issue.

NicolaeIotu · 2023-11-23T16:14:07Z

Okay. Found the branch. I'm gonna revert when done testing because I have to merge again with my implementation. Sorry for the delay.
As far as I can see in order to make a PR you need a fork of the project and a branch which will include some commits. Is that correct technically?

NicolaeIotu

Done testing.

File ResponseTrait.php: I cannot make it work without the suggested changes.

For file CURLRequest.php I think we can agree to replace if ($this->response instanceof Response) { with if ($this->response->hasHeader($title)) {.

Others changes are only suggestions in order to avoid any errors.

NicolaeIotu · 2023-11-23T20:17:25Z

system/Cache/ResponseCache.php

+                foreach ($value as $header) {
+                    $headers[$name][] = $header->getValueLine();
+                }


Suggested change

foreach ($value as $header) {

$headers[$name][] = $header->getValueLine();

}

if(is_array($value)) {

foreach ($value as $header) {

$headers[$name][] = $header->getValueLine();

}

}

NicolaeIotu · 2023-11-23T20:20:38Z

system/Debug/Toolbar.php

+                foreach ($value as $i => $header) {
+                    $index = $i + 1;
+                    $data['vars']['headers'][esc($name)] ??= '';
+                    $data['vars']['headers'][esc($name)] .= ' (' . $index . ') '
+                        . esc($header->getValueLine());
+                }


Suggested change

foreach ($value as $i => $header) {

$index = $i + 1;

$data['vars']['headers'][esc($name)] ??= '';

$data['vars']['headers'][esc($name)] .= ' (' . $index . ') '

. esc($header->getValueLine());

}

if(is_array($value)) {

foreach ($value as $i => $header) {

$index = $i + 1;

$data['vars']['headers'][esc($name)] ??= '';

$data['vars']['headers'][esc($name)] .= ' (' . $index . ') '

. esc($header->getValueLine());

}

}

NicolaeIotu · 2023-11-23T22:03:09Z

system/HTTP/ResponseTrait.php

+        foreach ($this->headers() as $name => $value) {
+            if ($value instanceof Header) {
+                header(
+                    $name . ': ' . $value->getValueLine(),
+                    false,
+                    $this->getStatusCode()
+                );
+            } else {
+                foreach ($value as $header) {
+                    header(
+                        $name . ': ' . $header->getValueLine(),
+                        false,
+                        $this->getStatusCode()
+                    );
+                }
+            }


Suggested change

foreach ($this->headers() as $name => $value) {

if ($value instanceof Header) {

header(

$name . ': ' . $value->getValueLine(),

false,

$this->getStatusCode()

);

} else {

foreach ($value as $header) {

header(

$name . ': ' . $header->getValueLine(),

false,

$this->getStatusCode()

);

}

}

foreach (array_keys($this->headers()) as $name)

{

$headerValue = $this->getHeader($name)->getValue();

if(is_array($headerValue)) {

foreach ($headerValue as $h_value) {

header($name . ': ' . $h_value, false, $this->getStatusCode());

}

} else {

header($name . ': ' . $this->getHeaderLine($name), false, $this->getStatusCode());

}

}

I don't know why my code does not work for you.
It seems your code does the same thing.

What was wrong? Any error message?

NicolaeIotu · 2023-11-23T22:05:52Z

system/HTTP/CURLRequest.php


-                $this->response->setHeader($title, $value);
+                if ($this->response instanceof Response) {


Suggested change

if ($this->response instanceof Response) {

if ($this->response->hasHeader($title)) {

We cannot remove if ($this->response instanceof Response) because the type of $this->response is ResponseInterface, and it does not have the addHeader() method.

NicolaeIotu · 2023-11-23T22:08:52Z

system/HTTP/RedirectResponse.php

+                foreach ($value as $header) {
+                    $this->addHeader($name, $header->getValue());
+                }


Suggested change

foreach ($value as $header) {

$this->addHeader($name, $header->getValue());

}

if(is_array($value)) {

foreach ($value as $header) {

$this->addHeader($name, $header->getValue());

}

}

kenjis · 2023-11-24T02:55:38Z

As far as I can see in order to make a PR you need a fork of the project and a branch which will include some commits. Is that correct technically?

Yes, you can send one PR with one branch.

kenjis added bug Verified issues on the current code behavior or pull requests that will fix them 4.5 labels Nov 21, 2023

kenjis marked this pull request as draft November 21, 2023 08:58

kenjis added 6 commits November 22, 2023 09:06

test: add test for multiple headers

9e6be65

fix: CURLRequest losts headers with same name

4986bc2

fix: trim header name and value

7852e3f

feat: add check if headers() returns array

a769e53

kenjis force-pushed the fix-use-addHeader branch from 7d2d4b2 to 568538e Compare November 22, 2023 00:20

chore: add skip_violations

8b0e1fe

kenjis marked this pull request as ready for review November 22, 2023 01:20

kenjis mentioned this pull request Nov 22, 2023

Bug: CURLRequest keeps the last value provided by headers with the same name #5041

Closed

NicolaeIotu suggested changes Nov 22, 2023

View reviewed changes

NicolaeIotu reviewed Nov 23, 2023

View reviewed changes

michalsn approved these changes Nov 28, 2023

View reviewed changes

kenjis merged commit b4f6af0 into codeigniter4:4.5 Nov 28, 2023
45 checks passed

kenjis deleted the fix-use-addHeader branch November 28, 2023 21:09

kenjis mentioned this pull request Dec 6, 2023

Bug: CURLRequest resolving invalid headers #8299

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: use `addHeader()` #8240

fix: use `addHeader()` #8240

kenjis commented Nov 21, 2023 •

edited

Loading

NicolaeIotu commented Nov 22, 2023

NicolaeIotu left a comment •

edited by kenjis

Loading

kenjis commented Nov 22, 2023

kenjis commented Nov 22, 2023

NicolaeIotu commented Nov 23, 2023

NicolaeIotu left a comment

NicolaeIotu Nov 23, 2023

NicolaeIotu Nov 23, 2023

NicolaeIotu Nov 23, 2023

kenjis Nov 24, 2023

NicolaeIotu Nov 23, 2023

kenjis Nov 24, 2023

NicolaeIotu Nov 23, 2023

kenjis commented Nov 24, 2023


		$this->response->setHeader($title, $value);
		if ($this->response instanceof Response) {

	if ($this->response instanceof Response) {
	if ($this->response->hasHeader($title)) {

fix: use addHeader() #8240

fix: use addHeader() #8240

Conversation

kenjis commented Nov 21, 2023 • edited Loading

NicolaeIotu commented Nov 22, 2023

NicolaeIotu left a comment • edited by kenjis Loading

Choose a reason for hiding this comment

kenjis commented Nov 22, 2023

kenjis commented Nov 22, 2023

NicolaeIotu commented Nov 23, 2023

NicolaeIotu left a comment

Choose a reason for hiding this comment

NicolaeIotu Nov 23, 2023

Choose a reason for hiding this comment

NicolaeIotu Nov 23, 2023

Choose a reason for hiding this comment

NicolaeIotu Nov 23, 2023

Choose a reason for hiding this comment

kenjis Nov 24, 2023

Choose a reason for hiding this comment

NicolaeIotu Nov 23, 2023

Choose a reason for hiding this comment

kenjis Nov 24, 2023

Choose a reason for hiding this comment

NicolaeIotu Nov 23, 2023

Choose a reason for hiding this comment

kenjis commented Nov 24, 2023

fix: use `addHeader()` #8240

fix: use `addHeader()` #8240

kenjis commented Nov 21, 2023 •

edited

Loading

NicolaeIotu left a comment •

edited by kenjis

Loading