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

Response\Headers: add input validation + more defensive coding #605

Merged
merged 2 commits into from
Nov 8, 2021

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Nov 7, 2021

Class Response\Headers: add input validation

This commit adds input validation to the getValues() and flatten() methods in the Response\Headers class.

  • The getValues() method is only set up to handle integer/string array keys, so should not accept any other type of array key.
  • The flatten() method is set up to only handle string or array values to flatten, so should not accept any other type of value.
    Throwing the exception constitutes a BC-break as previously non-string, non-array values would be returned as-is, while now an exception will be thrown. All the same, in those cases, the return type would previously not comply with the documented behaviour of the method, so this could be considered a bug fix.

As for the other methods:

  • The public ArrayAccess/ArrayIterator methods should not need additional input validation as they should not be called directly, but only indirectly and when called that way, will receive the correct input type.

Includes adding perfunctory tests for the added input validation and for the flatten() method in general.

Response\Headers: add more defensive coding

The Response\Headers class extends the CaseInsensitiveDictionary. In the second commit of PR #558, extra defensive coding was added to the offsetGet() and offsetSet() methods to prevent passing non-string keys to the PHP native strtolower() method.

As per the commit message of that commit:

The use of strtolower() on non-string keys, is 1) not necessary and 2) had side-effects when non-string keys were used as those were then cast to string...

While this class is intended to be used with requests headers, the class in itself is not limited to this use-case, so should function as per the specifications, independently of the use-case.

This commit applies the same fixes to the Response\Headers::offsetGet() and Response\Headers::offsetSet() methods.

Includes adding unit tests covering the changes.

This commit adds input validation to the `getValues()` and `flatten()` methods in the `Response\Headers` class.
* The `getValues()` method is only set up to handle integer/string array keys, so should not accept any other type of array key.
* The `flatten()` method is set up to only handle string or array values to flatten, so should not accept any other type of value.
    Throwing the exception constitutes a BC-break as previously non-string, non-array values would be returned as-is, while now an exception will be thrown. All the same, in those cases, the return type would previously not comply with the documented behaviour of the method, so this could be considered a bug fix.

As for the other methods:
* The `public` ArrayAccess/ArrayIterator methods should not need additional input validation as they should not be called directly, but only indirectly and when called that way, will receive the correct input type.

Includes adding perfunctory tests for the added input validation and for the `flatten()` method in general.
The `Response\Headers` class extends the `CaseInsensitiveDictionary`. In the second commit of PR 558, extra defensive coding was added to the `offsetGet()` and `offsetSet()` methods to prevent passing non-string keys to the PHP native `strtolower()` method.

As per the commit message of that commit:
> The use of `strtolower()` on non-string keys, is 1) not necessary and 2) had side-effects when non-string keys were used as those were then cast to string...
>
> While this class is intended to be used with requests headers, the class in itself is not limited to this use-case, so should function as per the specifications, independently of the use-case.

This commit applies the same fixes to the `Response\Headers::offsetGet()` and `Response\Headers::offsetSet()` methods.

Includes adding unit tests covering the changes.
@schlessera schlessera merged commit 1129cdd into develop Nov 8, 2021
@schlessera schlessera deleted the feature/response-headers-add-input-validation branch November 8, 2021 09:13
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