Skip to content

Commit

Permalink
PHP 8.1: improve input validation for `Requests_Transport_(fsockopen|…
Browse files Browse the repository at this point in the history
…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.
  • Loading branch information
jrfnl committed Jul 30, 2021
1 parent 6c5f62e commit 4c7e3e2
Show file tree
Hide file tree
Showing 4 changed files with 117 additions and 0 deletions.
8 changes: 8 additions & 0 deletions library/Requests/Exception/InvalidArgument.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<?php

/**
* Exception for an invalid argument passed.
*
* @package Requests
*/
class Requests_Exception_InvalidArgument extends InvalidArgumentException {}
19 changes: 19 additions & 0 deletions library/Requests/Transport/cURL.php
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,25 @@ public function __destruct() {
* @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)
)
);
}
}

$this->hooks = $options['hooks'];

$this->setup_handle($url, $headers, $data, $options);
Expand Down
19 changes: 19 additions & 0 deletions library/Requests/Transport/fsockopen.php
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
71 changes: 71 additions & 0 deletions tests/Transport/BaseTestCase.php
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
use Requests_Exception;
use Requests_Hooks;
use Requests_Response;
use stdClass;

abstract class BaseTestCase extends TestCase {
public function set_up() {
Expand Down Expand 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());
Expand Down

0 comments on commit 4c7e3e2

Please sign in to comment.