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.

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

PHP 8.1: improve input validation for Requests_Transport_(fsockopen|cURL) #499

Merged
merged 1 commit into from
Jul 30, 2021

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Jun 21, 2021

PHP 8.1 deprecates passing null to non-nullable parameters for PHP native functions.
Ref: https://wiki.php.net/rfc/deprecate_null_to_scalar_internal_arg

The Requests_Transport_fsockopen::request() method passes $request_body into the PHP native strlen() function without sufficient validation, which on PHP 8.1 results in a strlen(): Passing null to parameter #1 ($string) of type string is deprecated notification.

The existing RequestsTest_Transport_Base::testEmptyPOST() test method exposed this.

When investigating this issue, I realized that no significant input validation was being done on the $data parameter.
PR #368 in response to issue #248 added a test to verify that the "content-length" header was correctly set when null would be passed as $data and added a safe-guard specifically for when null would be passed. Also, as a matter of form, integers and floats were handled correctly for fsockopen, but anything else would lead to PHP errors in unexpected places in every supported PHP version, including integers and floats in combination with the cURL class.

To mitigate this, I propose to:

  • Add a Requests_Exception_InvalidArgument class which extends the PHP native InvalidArgumentException.
  • Add input validation for the $data parameter to both the cURL as well as the fsockopen Transport class which maintains and stabilizes the pre-existing behaviour for handling of null, synchronizes the behaviour for integers and floats to be the same across fsockopen and cURL, but will throw the new InvalidArgument exception for any other type of unexpected input for the $data parameter.

Includes additional tests covering this change.

@jrfnl jrfnl added this to the 2.0.0 milestone Jun 21, 2021
@jrfnl jrfnl force-pushed the php-8.1/fix-strlen-null-to-non-nullable branch from ec0cadc to 4e1b2e0 Compare June 21, 2021 16:04
@jrfnl jrfnl force-pushed the php-8.1/fix-strlen-null-to-non-nullable branch from 4e1b2e0 to eedad6d Compare July 24, 2021 20:44
library/Requests/Transport/cURL.php Outdated Show resolved Hide resolved
library/Requests/Transport/fsockopen.php Outdated Show resolved Hide resolved
…cURL)`

PHP 8.1 deprecates passing `null` to non-nullable parameters for PHP native functions.
Ref: https://wiki.php.net/rfc/deprecate_null_to_scalar_internal_arg

The `Requests_Transport_fsockopen::request()` method passes `$request_body` into the PHP native `strlen()` function without sufficient validation, which on PHP 8.1 results in a `strlen(): Passing null to parameter #1 ($string) of type string is deprecated` notification.

The existing `RequestsTest_Transport_Base::testEmptyPOST()` test method exposed this.

When investigating this issue, I realized that no significant input validation was being done on the `$data` parameter.
PR 368 in response to issue 248 added a test to verify that the "content-length" header was correctly set when `null` would be passed as `$data` and added a safe-guard specifically for when `null` would be passed. Also, as a matter of form, integers and floats were handled correctly for `fsockopen`, but anything else would lead to PHP errors in unexpected places in every supported PHP version, including integers and floats in combination with the `cURL` class.

To mitigate this, I propose to:
* Add a `Requests_Exception_InvalidArgument` class which extends the PHP native `InvalidArgumentException`.
* Add input validation for the `$data` parameter to both the `cURL` as well as the `fsockopen` Transport class which maintains and stabilizes the pre-existing behaviour for handling of `null`, synchronizes the behaviour for integers and floats to be the same across `fsockopen` and `cURL`, but will throw the new `InvalidArgument` exception for any other type of unexpected input for the `$data` parameter.

Includes additional tests covering this change.
@jrfnl jrfnl force-pushed the php-8.1/fix-strlen-null-to-non-nullable branch from eedad6d to 4c7e3e2 Compare July 30, 2021 10:22
@schlessera schlessera merged commit 025e908 into develop Jul 30, 2021
@schlessera schlessera deleted the php-8.1/fix-strlen-null-to-non-nullable branch July 30, 2021 10:25
@jrfnl jrfnl mentioned this pull request Aug 27, 2021
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants