From 4c7e3e2f040267be21d8184418b34f6d2df2bfff Mon Sep 17 00:00:00 2001 From: jrfnl Date: Sat, 19 Jun 2021 01:24:02 +0200 Subject: [PATCH] PHP 8.1: improve input validation for `Requests_Transport_(fsockopen|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. --- .../Requests/Exception/InvalidArgument.php | 8 +++ library/Requests/Transport/cURL.php | 19 +++++ library/Requests/Transport/fsockopen.php | 19 +++++ tests/Transport/BaseTestCase.php | 71 +++++++++++++++++++ 4 files changed, 117 insertions(+) create mode 100644 library/Requests/Exception/InvalidArgument.php diff --git a/library/Requests/Exception/InvalidArgument.php b/library/Requests/Exception/InvalidArgument.php new file mode 100644 index 000000000..6553742d3 --- /dev/null +++ b/library/Requests/Exception/InvalidArgument.php @@ -0,0 +1,8 @@ +hooks = $options['hooks']; $this->setup_handle($url, $headers, $data, $options); diff --git a/library/Requests/Transport/fsockopen.php b/library/Requests/Transport/fsockopen.php index 34b65d4a6..1c624233d 100644 --- a/library/Requests/Transport/fsockopen.php +++ b/library/Requests/Transport/fsockopen.php @@ -56,6 +56,25 @@ class Requests_Transport_fsockopen implements Requests_Transport { * @return string Raw HTTP result */ public function request($url, $headers = array(), $data = array(), $options = array()) { + if (!is_array($data) && !is_string($data)) { + if ($data === null) { + $data = ''; + } elseif (is_int($data) || is_float($data)) { + $data = (string) $data; + } else { + throw new Requests_Exception_InvalidArgument( + sprintf( + '%s: Argument #%d (%s) must be of type %s, %s given', + __METHOD__, + 3, + '$data', + 'array|string', + gettype($data) + ) + ); + } + } + $options['hooks']->dispatch('fsockopen.before_request'); $url_parts = parse_url($url); diff --git a/tests/Transport/BaseTestCase.php b/tests/Transport/BaseTestCase.php index 81ecd2679..e845c24ba 100644 --- a/tests/Transport/BaseTestCase.php +++ b/tests/Transport/BaseTestCase.php @@ -8,6 +8,7 @@ use Requests_Exception; use Requests_Hooks; use Requests_Response; +use stdClass; abstract class BaseTestCase extends TestCase { public function set_up() { @@ -176,6 +177,76 @@ public function testEmptyPOST() { } } + /** + * Test that when a $data parameter of an incorrect type gets passed, it gets handled correctly. + * + * Only string/array are officially supported as accepted data types, but null, integers and floats + * will also be handled (cast to string). + * + * This test safeguards handling of data types in response to changes in PHP 8.1. + * + * @dataProvider dataIncorrectDataTypeAccepted + * + * @param mixed $data Input to be used as the $data parameter. + * @param string $expected Expected value for the Json decoded request body data key. + * + * @return void + */ + public function testIncorrectDataTypeAcceptedPOST($data, $expected) { + $request = Requests::post(httpbin('/post'), array(), $data, $this->getOptions()); + $this->assertIsObject($request, 'POST request did not return an object'); + $this->assertObjectHasAttribute('status_code', $request, 'POST request object does not have a "status_code" property'); + $this->assertSame(200, $request->status_code, 'POST request status code is not 200'); + + $this->assertObjectHasAttribute('body', $request, 'POST request object does not have a "body" property'); + $result = json_decode($request->body, true); + + $this->assertIsArray($result, 'Json decoded POST request body is not an array'); + $this->assertArrayHasKey('data', $result, 'Json decoded POST request body does not contain array key "data"'); + $this->assertSame($expected, $result['data'], 'Json decoded POST request body "data" key did not match expected contents'); + } + + /** + * Data provider. + * + * @return array + */ + public function dataIncorrectDataTypeAccepted() { + return array( + 'null' => array(null, ''), + 'integer' => array(12345, '12345'), + 'float' => array(12.345, '12.345'), + ); + } + + /** + * Test that when a $data parameter of an unhandled incorrect type gets passed, an exception gets thrown. + * + * @dataProvider dataIncorrectDataTypeException + * + * @param mixed $data Input to be used as the $data parameter. + * + * @return void + */ + public function testIncorrectDataTypeExceptionPOST($data) { + $this->expectException('Requests_Exception_InvalidArgument'); + $this->expectExceptionMessage('Argument #3 ($data) must be of type array|string'); + + $request = Requests::post(httpbin('/post'), array(), $data, $this->getOptions()); + } + + /** + * Data provider. + * + * @return array + */ + public function dataIncorrectDataTypeException() { + return array( + 'boolean' => array(true), + 'object' => array(new stdClass()), + ); + } + public function testFormPost() { $data = 'test=true&test2=test'; $request = Requests::post(httpbin('/post'), array(), $data, $this->getOptions());