mirror of
https://github.com/chillerlan/php-httpinterface.git
synced 2026-04-26 04:55:49 +03:00
[GH-ISSUE #2] CurlClient fail to exec HTTPS with empty HTTPOptions #2
Labels
No labels
pull-request
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
starred/php-httpinterface#2
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Originally created by @eclipxe13 on GitHub (Mar 22, 2019).
Original GitHub issue: https://github.com/chillerlan/php-httpinterface/issues/2
Hello, thanks for your great work!
Was trying to use
CurlClientwithout any (default)HTTPOptionsbut it fail with a curl error:wich is weird because making a plain curl connection didn't fail:
So I take a look into your package and I see that in
CurlHandle::init()you are setting$this->options->curl_options[\CURLOPT_CAINFO]with aNULLvalue.If you modify the test and add this line will fail in the same way.
I think that the problem is in
CurlHandle::initCurlOptions()that returns an array withCURLOPT_CAINFO => $this->options->ca_infoeven when$this->options->ca_info === NULL. I tested includingCURLOPT_CAINFOonly when$this->options->ca_infois not falsy and it work just fine.By the way, the same would happens with
CURLOPT_CAPATHif you provide a way to work with.Looks like
CURLOPT_CAINFOorCURLOPT_CAPATHcannot be removed using null or empty strings, and it will fail. Also, if both are set then both must succeed.Could you consider to fix this? Do you want me to make a PR? I'm not familiar with your test suite. The only this I was thinking is to simply remove
'ca_info' => __DIR__.'/../cacert.pem'from the initiation parameters onCurlClientTest.php.@eclipxe13 commented on GitHub (Mar 22, 2019):
I just did #3 that also covers
StreamClientwith the same issue.@codemasher commented on GitHub (Mar 22, 2019):
Hey, thank you for the heads-up! I think #3 is a bit too invasive. The concept is to decouple the "settings" logic as far as possible from the application logic. So the whole process of determining the CA info went into
HTTPOptionsover here (see also #1):github.com/chillerlan/php-httpinterface@a7805699cd/src/HTTPOptionsTrait.php (L83-L189)The cURL options are applied here before firing the request:
github.com/chillerlan/php-httpinterface@a7805699cd/src/Psr18/CurlHandle.php (L249-L250)So if there's a problem finding the correct CA on your system, it might be that i'm probably missing a default path or something. The tests currently don't include much testing for default paths. So if you're looking to fix that, the best would be in
HTTPOptionsTraitin first place. Also, i should point out, that theStreamClientis in "experimental" status at best.@eclipxe13 commented on GitHub (Mar 22, 2019):
As you mention, on
php-httpinterface/src/Psr18/CurlHandle.php:250is where anullvalue forCURLOPT_CAINFOis set.My first thought was to check if
CURLOPT_CAINFOexists (not withisset, but usingarray_key_exists), and if it is falsy then remove it from the options to apply.Then I think that would be better to check the place where this is set and found that this
nullvalue comes fromCurlHandle::initCurlOptionsnot includeCURLOPT_CAINFO.Anyhow, please, play around with the test I post in the issue.
It is not correct to set CURLOPT_CAINFO or CURLOPT_CAPATH to
null,falseor""empty strings.@codemasher commented on GitHub (Mar 22, 2019):
Ok, i investigated a bit further and found an error on my end over here:
github.com/chillerlan/php-httpinterface@a7805699cd/src/Psr18/CurlHandle.php (L249-L250)The PHP documentation says:
So it would be simply:
To circumvent any future bugs, preserve the numerical array keys (
array_merge()doesn't) and dump the+operator i just changed it to a quick loop.github.com/chillerlan/php-httpinterface@5afc32711d/src/Psr18/CurlHandle.php (L249-L252)As for the fix in
StreamClient, i've added this line to make sure a CA is set in the options:github.com/chillerlan/php-httpinterface@c28ada36a5/src/HTTPOptionsTrait.php (L174)Anything else should throw an error now. Please let me know if this helps.
@eclipxe13 commented on GitHub (Mar 23, 2019):
Thank you.