Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: [CURLRequest] skip hostname checks if options 'verify' false #8258

Merged
merged 10 commits into from
Dec 2, 2023
Merged

fix: [CURLRequest] skip hostname checks if options 'verify' false #8258

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
7e7821b
add: skip hostname checks if CURLRequest options 'verify' is set to f…
NicolaeIotu Nov 26, 2023
3685232
add: SSL verify options when config.verify is string and boolean
NicolaeIotu Nov 27, 2023
e159bfd
fix: CURLOPT_SSL_VERIFYPEER values and tests related with CURLOPT_SSL…
NicolaeIotu Nov 28, 2023
e601954
recreate fix: CURLOPT_SSL_VERIFYPEER values and tests related with CU…
NicolaeIotu Nov 30, 2023
f8dd0b7
Updated Bugs and Breaking sections
NicolaeIotu Nov 30, 2023
25b6d40
Fix sphinx-action warning 'Title underline too short'
NicolaeIotu Nov 30, 2023
ab25092
Update user_guide_src/source/changelogs/v4.4.4.rst
NicolaeIotu Dec 1, 2023
6df07b0
Added the summary of the bug
NicolaeIotu Dec 1, 2023
bfb9ab4
Revert manual changes to CHANGELOG.md
NicolaeIotu Dec 1, 2023
5b4482c
Added upgrading instructions
NicolaeIotu Dec 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 10 additions & 6 deletions system/HTTP/CURLRequest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -548,17 +548,21 @@ protected function setCURLOptions(array $curlOptions = [], array $config = [])

// SSL Verification
if (isset($config['verify'])) {
if (is_string($config['verify'])) {
$file = realpath($config['ssl_key']) ?: $config['ssl_key'];
$configVerify = $config['verify'];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't introduce an additional variable, it makes the code less readable - use $config['verify'].

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to clarify. Array element $config['verify'] is accessed 8 times. Isn't this less efficient than using a local variable?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, why would it be?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the element of the array it's not changing value, you get a performance increase if assigning its value to a local variable and using that instead of accessing the array every time especially if the key is not an integer and the value of the element is required many times.

This is pretty much valid for all languages when data type is using hash tables and others alike.

In this case there is a performance increase if using the local variable, but it may be more important to make the code readable.

I forgot to ask. The breaking change is that $config['verify'] cannot take the value 'yes' anymore. This is the only breaking change?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the element of the array it's not changing value, you get a performance increase...

In this case, I'm pretty sure we would not see the difference. That's why I choose the readability.

I forgot to ask. The breaking change is that $config['verify'] cannot take the value 'yes' anymore. This is the only breaking change?

Was using the value yes was documented somewhere? If not this can be skipped.
The breaking change is not using the ssl_key array key anymore.


if (is_string($configVerify)) {
$file = realpath($configVerify) ?: $configVerify;

if (! is_file($file)) {
throw HTTPException::forInvalidSSLKey($config['ssl_key']);
throw HTTPException::forInvalidSSLKey($configVerify);
}

$curlOptions[CURLOPT_CAINFO] = $file;
$curlOptions[CURLOPT_SSL_VERIFYPEER] = 1;
} elseif (is_bool($config['verify'])) {
$curlOptions[CURLOPT_SSL_VERIFYPEER] = $config['verify'];
$curlOptions[CURLOPT_SSL_VERIFYPEER] = true;
$curlOptions[CURLOPT_SSL_VERIFYHOST] = 2;
} elseif (is_bool($configVerify)) {
$curlOptions[CURLOPT_SSL_VERIFYPEER] = $configVerify;
$curlOptions[CURLOPT_SSL_VERIFYHOST] = $configVerify ? 2 : 0;
}
}

Expand Down
11 changes: 6 additions & 5 deletions tests/system/HTTP/CURLRequestDoNotShareOptionsTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -535,8 +535,7 @@ public function testSSLVerification(): void
$file = __FILE__;

$this->request->request('get', 'http://example.com', [
'verify' => 'yes',
'ssl_key' => $file,
'verify' => $file,
]);

$options = $this->request->curl_options;
Expand All @@ -545,7 +544,10 @@ public function testSSLVerification(): void
$this->assertSame($file, $options[CURLOPT_CAINFO]);

$this->assertArrayHasKey(CURLOPT_SSL_VERIFYPEER, $options);
$this->assertSame(1, $options[CURLOPT_SSL_VERIFYPEER]);
$this->assertTrue($options[CURLOPT_SSL_VERIFYPEER]);

$this->assertArrayHasKey(CURLOPT_SSL_VERIFYHOST, $options);
$this->assertSame(2, $options[CURLOPT_SSL_VERIFYHOST]);
}

public function testSSLWithBadKey(): void
Expand All @@ -554,8 +556,7 @@ public function testSSLWithBadKey(): void
$this->expectException(HTTPException::class);

$this->request->request('get', 'http://example.com', [
'verify' => 'yes',
'ssl_key' => $file,
'verify' => $file,
]);
}

Expand Down
26 changes: 21 additions & 5 deletions tests/system/HTTP/CURLRequestTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -518,8 +518,7 @@ public function testSSLVerification(): void
$file = __FILE__;

$this->request->request('get', 'http://example.com', [
'verify' => 'yes',
'ssl_key' => $file,
'verify' => $file,
]);

$options = $this->request->curl_options;
Expand All @@ -528,7 +527,25 @@ public function testSSLVerification(): void
$this->assertSame($file, $options[CURLOPT_CAINFO]);

$this->assertArrayHasKey(CURLOPT_SSL_VERIFYPEER, $options);
$this->assertSame(1, $options[CURLOPT_SSL_VERIFYPEER]);
$this->assertTrue($options[CURLOPT_SSL_VERIFYPEER]);

$this->assertArrayHasKey(CURLOPT_SSL_VERIFYHOST, $options);
$this->assertSame(2, $options[CURLOPT_SSL_VERIFYHOST]);
}

public function testNoSSL(): void
{
$this->request->request('get', 'http://example.com', [
'verify' => false,
]);

$options = $this->request->curl_options;

$this->assertArrayHasKey(CURLOPT_SSL_VERIFYPEER, $options);
$this->assertFalse($options[CURLOPT_SSL_VERIFYPEER]);

$this->assertArrayHasKey(CURLOPT_SSL_VERIFYHOST, $options);
$this->assertSame(0, $options[CURLOPT_SSL_VERIFYHOST]);
}

public function testSSLWithBadKey(): void
Expand All @@ -537,8 +554,7 @@ public function testSSLWithBadKey(): void
$this->expectException(HTTPException::class);

$this->request->request('get', 'http://example.com', [
'verify' => 'yes',
'ssl_key' => $file,
'verify' => $file,
]);
}

Expand Down
Loading