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 Jun 21, 2021
1 parent c637258 commit 500d238
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) === false && is_string($data) === false) {
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) === false && is_string($data) === false) {
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/Base.php
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,77 @@ 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).
*
* Original test passing `null` was added in response to issue #248.
* The test was expanded to additional 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 content-length header.
*
* @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 500d238

Please sign in to comment.