CURLrequest fix debug verbose #2263

jbotka · 2019-09-23T22:08:55Z

Description

Issue fix: CURLRequest - supplied argument is not a valid File-Handle resource #Windows x64 #2202 cURL request returns 404 #2250
Added additional check inside check if $config['debug'] is true to check if is set exactly to 'true'. If yes, it will assign 'php://temp' stream for CURLOPT_STDERR instead of 'php://output' that is not allowed to be used there.
Added check inside sendRequest method to check if is $this->config['debug'] set exactly to 'true'. If yes it will know that it was used temp stream and it will rewind stream to the begging and after that, it will send a formatted text to output from temp stream. If it will not pass the check for 'true' it will throw an exception like in normal mode.
Fixed header issue when CI automatically set the header for base_url. This shouldn' happen because it will produce 404 error when accessing URL outside of the local server. So I removed 'Host' header from headers.
Fixed also test case since https://example.com is valid URL it will never throw error, to test debugging we need to produce error so I have chosen 'google.com/CodeIgniter4' to produce 404 error. (or we can change it to point to invalid addresses on CI installation 'localhost/curlTest'.

Checklist:

jbotka · 2019-09-23T22:52:10Z

@michalsn Now it should be properly set. Sorry I didn' understand that verbose produce something else than standard errors.

michalsn · 2019-09-24T07:25:02Z

For the starter - you're right about debug mode when the error occurs. We always throw an exception. I guess when debug = true we should just display curl error... but the more I think about it, the more unsure I am about it 😅 It could be helpful for debugging in production environment though.

'php://output' that is not allowed to be used there

Can you elaborate on this? Because setting it that way works just fine to me.

Fixed header issue when CI automatically set the header for base_url

I couldn't reproduce it but maybe I don't understand the problem. Can you give us any sample code to recreate this bug?

Fixed also test case since https://example.com is valid URL...

All curl tests are mocked, so we don't really make a request.

michalsn · 2019-09-24T07:25:30Z

system/HTTP/CURLRequest.php

@@ -489,6 +489,9 @@ protected function applyRequestHeaders(array $curl_options = []): array
 	{
 		$headers = $this->getHeaders();

+		// Unset 'Host' header, CURL will determinate it automatically from URL.
+		unset($headers['Host']);


I don't think we can do this. There are many different scenarios where Host header may be necessary, i.e when you call internal IP on the server.

I don't think we can do this. There are many different scenarios where Host header may be necessary, i.e when you call internal IP on the server.

True, we can put here check if is set custom header for Host, if not it will be unset, if yes it will use set from options. Because now in every request you have to define Host, it will never determinate it automatically what is redundant to write it to each request.

I don't understand the issue you have with a Host header at all. The only place where we set something automatically are tests. And you can set default base_url when grabbing the service.

$client = Services::curlrequest([ 'base_uri' => '...', ]);

When you are grabbing service without options it will set Host from $_SERVER like a host in example case it is 'example.com' or for local development server, it will be 'localhost or so... Case is that if you are trying to access any server you need to specify host each time. It is not necessary since CURL php library will set Host based on URL. Another thing is that it is not mentioned in the guide that you need set host to make a valid request for another server. Simply it is redundant and it confuses people why there is 404 error when you set full valid URL but CI will use localhost all the time.

For example, this will produce error.

$client = \Config\Services::curlrequest(); $response = $client->get('https://google.com');

This is how looks default header from above example:

Host: localhost Connection: keep-alive Cache-Control: max-age=0 Upgrade-Insecure-Requests: 1 User-Agent: Mozilla/5.0 (Windows NT 6.3; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/76.0.3809.132 Safari/537.36 Sec-Fetch-Mode: navigate Sec-Fetch-User: ?1 Accept: text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,image/apng,*/*;q=0.8,application/signed-exchange;v=b3 Sec-Fetch-Site: none Accept-Encoding: gzip, deflate, br Accept-Language: sk-SK,sk;q=0.9,cs;q=0.8,en-US;q=0.7,en;q=0.6 Cookie: SEARCH_MORE_RESULTS=Show+more+results

To make work above example you need to do this:

$client = \Config\Services::curlrequest(['headers' => ['Host' => 'google.com']]); $response = $client->get('https://google.com');

Does it make sense to do it this way all the time when using CURL when we can skip host part completely if we do not need specific server?

@lonnieezell should it work this way?

Another is directly from the guide:

$client = \Config\Services::curlrequest(); $response = $client->request('GET', 'https://api.github.com/user', [ 'auth' => ['user', 'pass'] ]);

It will produce 404 error as well, but curl request has all necessary data to make it work but we specified in default headers Host and other headers from $_SERVER[] global variable.

foreach ($_SERVER as $key => $val) { if (sscanf($key, 'HTTP_%s', $header) === 1) { // take SOME_HEADER and turn it into Some-Header $header = str_replace('_', ' ', strtolower($header)); $header = str_replace(' ', '-', ucwords($header)); $this->setHeader($header, $_SERVER[$key]); // Add us to the header map so we can find them case-insensitively $this->headerMap[strtolower($header)] = $header; } }

@xbotkaj - thanks for the details, now I get it.

I think the solution would be a change in Message class for populateHeaders() method. We should skip HTTP_HOST in foreach loop:

if ($key === 'HTTP_HOST') { continue; }

@michalsn we cannot do that since Message is deeper class than CURLrequest class that is extend of Request and that is extend of Message class is used by other method and calls that parse response headers.

For fix just in CURL we could use something like this in applyRequestHeaders() :

if (empty($this->headers)) { $headers = $this->getHeaders(); unset($headers['Host']); } else { $headers = $this->getHeaders(); }

or make new option 'default_headers' that can be boolean and it will handle it. Now users use it and they do not know that CI automatically grab default headers and use it. It can make problems in some cases.

michalsn · 2019-09-24T07:26:41Z

system/HTTP/CURLRequest.php

+
+			if ($config['debug'] === true)
+			{
+				$curl_options[CURLOPT_STDERR] = fopen('php://temp', 'w+');


fopen('php://output', 'w+') is working just fine for outputting things. We should stay with that because otherwise it just complicates things later. So I would just go with the initial one-liner.

For me using php://output will produce error: CRITICAL --> curl_setopt_array(): supplied argument is not a valid File-Handle resource

Well... it can't be happening. fopen('php://output', 'w+') is a perfectly valid resource. Please double-check if you're using the latest framework version from develop branch when you're testing it.

We definitely want php://output as that directs tot he buffered output, whereas php://temp would require harvesting the contents from the temp directory.

The issue may be that php://output is explicitly write-only, and w+ specifies opening a resource for "reading and writing" which may cause errors on some systems and not others (just a guess). Try using fopen('php://output', 'w')

Actually reviewing the docs it states "this will enable additional debugging to echo to STDOUT during the script execution" so we may want to use fopen('php://stdout', 'w') which bypasses buffering altogether. Hardcore debugging :)

@MGatner I tried:

fopen('php://output', 'w') -> produce CRITICAL - 2019-09-24 12:07:11 --> curl_setopt_array(): cannot represent a stream of type Output as a STDIO FILE*

fopen('php://stdout', 'w') ->produce nothing (empty output on screen but maybe something in CLI)

STDOUT ->produce CRITICAL - 2019-09-24 12:10:04 --> Use of undefined constant STDOUT-assumed 'STDOUT' (this will throw an Error in a future version of PHP)

Only working solution how to print to screen is from php://temp or any file that we can make but this is temporary and we do not have to delete it afterwards. I am not sure about output in CLI.

Actually, I tested fopen('php://stderr', 'w+') - it works fine, plus it play nice with php spark serve.

@MGatner in the past, Windows used to have an old versions of libraries - which suffered from the bugs. I'm sure it have changed. In this case the difference may be the type of the server (apache, nginx) - and in what mode does php work? Idk... it's a blind shot.

@michalsn yep but it will work only in CLI and not like the output to screen during the regular request (if it is wanted the behavior to debug only in CLI).

@xbotkaj A quick internet search reveals that this definitely is an issue that crops up with Windows PHP cURL. One question (from guzzle/guzzle#1413 (comment)), are you testing against the same server? If so, can you test against some external site and see if you still get the error with fopen('php://output', 'w')? I believe that is our desired stream since php://stdout doesn't seem to write to the web-accessible buffer.

@MGatner It is confirmed. It is a windows problem. I uploaded it to my linux server and it works without problem. :)

I am suggesting to put to the guide INFO block about the issue on windows platform with quick-fix with php://temp or to do not use this feature.

I will put header issue in own commit so it will not be merged here.

I am closing this PR since it should be addressed in issues.

michalsn · 2019-09-24T07:27:50Z

system/HTTP/CURLRequest.php

@@ -812,7 +821,19 @@ protected function sendRequest(array $curl_options = []): string

 		if ($output === false)
 		{
-			throw HTTPException::forCurlError(curl_errno($ch), curl_error($ch));
+			// Check if debug is set to 'true'. If yes send verbose to output. If not throw exceptions.
+			if ($this->config['debug'] === true)


Suggested change

if ($this->config['debug'] === true)

if ($this->config['debug'] === true && $this->config['debug'] !== true)

I would change it to this condition and only then would throw an exception. Only when debug is turned on we want to see the "pure" error from curl.

I think there's an extra condition in here, or may an extra =? IMO it is fine to specify in the docs that $config['debug'] can be a string file path OR boolean true and then let this throw otherwise.

@michalsn Nope, it works fine here. Print to the screen is triggered only when is set to 'true'. condition in this form if($this->config['debug]) would be triggered even when there would be string. Now it is triggered only and only with 'true'.

@MGatner if there would be something else, it will throw a critical error from CURL or fopen() method since we are passing this value to fopen().

michalsn · 2019-09-24T07:28:06Z

system/HTTP/CURLRequest.php

+			if ($this->config['debug'] === true)
+			{
+				// Rewind written stream to the begging
+				rewind($curl_options[CURLOPT_STDERR]);


Since we use fopen('php://output', 'w+'), all this code is no longer needed.

CURLrequest fix debug verbose

ff73c0a

jbotka added 2 commits September 24, 2019 01:31

CURLrequest fix 'host' header

389970d

CURLrequest fix tested domains (request must return error)

0a54fcc

michalsn reviewed Sep 24, 2019

View reviewed changes

Fixed header (populate header without 'host')

e3c5144

jbotka closed this Sep 25, 2019

TheNorthMemory mentioned this pull request Jan 5, 2022

在使用示例程序微信支付平台证书下载的代码的时候出现错误 wechatpay-apiv3/wechatpay-php#80

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CURLrequest fix debug verbose #2263

CURLrequest fix debug verbose #2263

jbotka commented Sep 23, 2019 •

edited

Loading

jbotka commented Sep 23, 2019

michalsn commented Sep 24, 2019

michalsn Sep 24, 2019

jbotka Sep 24, 2019

michalsn Sep 24, 2019

jbotka Sep 24, 2019 •

edited

Loading

michalsn Sep 24, 2019

jbotka Sep 25, 2019 •

edited

Loading

michalsn Sep 24, 2019

jbotka Sep 24, 2019 •

edited

Loading

michalsn Sep 24, 2019

MGatner Sep 24, 2019

MGatner Sep 24, 2019

jbotka Sep 24, 2019 •

edited

Loading

michalsn Sep 24, 2019

jbotka Sep 25, 2019

MGatner Sep 25, 2019

jbotka Sep 25, 2019 •

edited

Loading

michalsn Sep 24, 2019

MGatner Sep 24, 2019

jbotka Sep 24, 2019 •

edited

Loading

michalsn Sep 24, 2019

	if ($this->config['debug'] === true)
	if ($this->config['debug'] === true && $this->config['debug'] !== true)

CURLrequest fix debug verbose #2263

CURLrequest fix debug verbose #2263

Conversation

jbotka commented Sep 23, 2019 • edited Loading

jbotka commented Sep 23, 2019

michalsn commented Sep 24, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbotka Sep 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbotka Sep 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbotka Sep 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbotka Sep 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbotka Sep 25, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbotka Sep 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbotka commented Sep 23, 2019 •

edited

Loading

jbotka Sep 24, 2019 •

edited

Loading

jbotka Sep 25, 2019 •

edited

Loading

jbotka Sep 24, 2019 •

edited

Loading

jbotka Sep 24, 2019 •

edited

Loading

jbotka Sep 25, 2019 •

edited

Loading

jbotka Sep 24, 2019 •

edited

Loading