-
Notifications
You must be signed in to change notification settings - Fork 498
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
Requests: add input validation #621
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Follow up on 492 The new `Requests::get_transport_class()` and `Requests::has_capabilities()` can safely be given a type declaration as they are new in Requests 2.0.0 and as the `Requests::get_transport()` method is a protected method, we may as well add it there too.
This adds input validation to the `Requests::request()` method and all direct "fall-through" methods calling this method. * The `$url` parameter is validated to allow for a string or Stringable (Iri) object. * The `$type` parameter is validated to allow solely for a string as it expects one of the `Requests` class constants. * The `$options` parameter is validated to allow only an array as the parameter is used with `array_merge()`. The `$headers` and `$data` parameters remain unvalidated as they are not actually used within the method itself, but only passed through and should be validated in the `Transport::request()` method instead. Includes adding perfunctory tests for the new exceptions + adjusting one existing test to ensure that passing the `$url` as a stringable object is safeguarded.
This commit adds input validation for all parameters of the `Requests::request_multiple()` method: * For `$requests`, arrays and iterable objects with array access should be accepted based on how the parameter is used within this class. * For the `$options` parameter, only plain arrays can be accepted due to the use of `array_merge()` in the method. Includes tests for these new exceptions. Includes adding perfunctory tests for the new exceptions.
Based on the documentation for the `Requests::request()` method, the `$options['verify']` allows for a string path or a boolean value, so let's allow the same for the `Requests:;set_certificate_path()` method (and include Stringable as well). Includes perfunctory tests for the exception + for actually setting the path with valid input. Includes: * Removing the `DEFAULT_CERT_PATH` constant (not a BC-break as the constant was only introduced in 2.0.0-dev) and adding the default value to the `$certificate_path`. * Simplifying the `get_certificate_path()` method. * And using the `Requests::$certificate_path` property directly in the `get_default_options()` method as `get_certificate_path()` now no longer contains logic.
While the documented parameter type for the `$dictionary` parameter was `array`, in reality, the only real requirement is that the parameter is iterable, so adding input validation to match the real requirement. Includes adding perfunctory tests for the new exception + for the method itself which was otherwise untested.
The documented accepted parameter type for both these methods is `string`, but no input validation was done on the parameter, which could lead to various PHP errors, most notably a "passing null to non-nullable" deprecation notice on PHP 8.1. This commit adds input validation to the `Requests::decompress()` and the `Requests::compatible_gzinflate()` methods, allowing only for strings. Note: Stringable objects will not be considered as valid input for these methods. Includes adding perfunctory tests for the new exceptions.
schlessera
approved these changes
Nov 15, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Requests::get_transport[_class]/has_capabilities(): add type declaration
Follow up on #492
The new
Requests::get_transport_class()
andRequests::has_capabilities()
can safely be given a type declaration as they are new in Requests 2.0.0 and as theRequests::get_transport()
method is a protected method, we may as well add it there too.Requests::request(): add input validation
This adds input validation to the
Requests::request()
method and all direct "fall-through" methods calling this method.$url
parameter is validated to allow for a string or Stringable (Iri) object.$type
parameter is validated to allow solely for a string as it expects one of theRequests
class constants.$options
parameter is validated to allow only an array as the parameter is used witharray_merge()
.The
$headers
and$data
parameters remain unvalidated as they are not actually used within the method itself, but only passed through and should be validated in theTransport::request()
method instead.Includes adding perfunctory tests for the new exceptions + adjusting one existing test to ensure that passing the
$url
as a stringable object is safeguarded.Requests::request_multiple(): add input validation
This commit adds input validation for all parameters of the
Requests::request_multiple()
method:$requests
, arrays and iterable objects with array access should be accepted based on how the parameter is used within this class.$options
parameter, only plain arrays can be accepted due to the use ofarray_merge()
in the method.Includes tests for these new exceptions.
Includes adding perfunctory tests for the new exceptions.
Request::set_certificate_path(): add input validation
Based on the documentation for the
Requests::request()
method, the$options['verify']
allows for a string path or a boolean value, so let's allow the same for theRequests:;set_certificate_path()
method (and include Stringable as well).Includes perfunctory tests for the exception + for actually setting the path with valid input.
Includes:
DEFAULT_CERT_PATH
constant (not a BC-break as the constant was only introduced in 2.0.0-dev) and adding the default value to the$certificate_path
.get_certificate_path()
method.Requests::$certificate_path
property directly in theget_default_options()
method asget_certificate_path()
now no longer contains logic.Requests::flatten(): add input validation
While the documented parameter type for the
$dictionary
parameter wasarray
, in reality, the only real requirement is that the parameter is iterable, so adding input validation to match the real requirement.Includes adding perfunctory tests for the new exception + for the method itself which was otherwise untested.
Requests::decompress()/compatible_gzinflate(): add input validation
The documented accepted parameter type for both these methods is
string
, but no input validation was done on the parameter, which could lead to various PHP errors, most notably a "passing null to non-nullable" deprecation notice on PHP 8.1.This commit adds input validation to the
Requests::decompress()
and theRequests::compatible_gzinflate()
methods, allowing only for strings.Note: Stringable objects will not be considered as valid input for these methods.
Includes adding perfunctory tests for the new exceptions.
Regarding the other
public
methods:add_transport()
method expects a class name as a string, where the class implements theTransport
interface. Checking this, however, isn't really an option without also (auto-)loading the class.As this is a user-selected overwrite, let's just defer to the error PHP will throw when the class will be instantiated and cannot be found.
get()
,head()
etc methods don't need input validation as they will fall through to theRequests::request()
method for which (partial) input validation has been added.get_default_options()
method only uses the parameter in one condition and does a hard check against the default value, so doesn't need further input validation.set_defaults()
method isprotected
. While it could definitely use improvement, that's outside the scope of this PR.parse_multiple()
method ispublic
, but only intended for internal use.