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

Class Ipv6: add input validation #601

Merged
merged 1 commit into from
Nov 8, 2021

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Nov 7, 2021

The documented accepted parameter type for all methods in the Ipv6 class 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 all public methods in the class, allowing only for strings and stringable objects.
This commit adds input validation to the uncompress() method, allowing only for strings and stringable objects.
As the other public methods call uncompress() as the first point of action in their flow, this is sufficient to ensure input validation in all cases.

As this class was until now only indirectly tested via the IriTest class, a new Ipv6Test class is being introduced containing - for now - only perfunctory tests for the input validation.
This test class should be expanded to cover the actual logic in the methods at a later date.

Open questions:

  • It is up for discussion whether stringable objects should even be allowed as input for these methods. Opinions welcome.
  • As both the compress() as well as the check_ipv6() method call the uncompress() method at the very start of the method logic, it could be argued that adding input validation to those methods is not needed as adding input validation to uncompress() is sufficient.
    I've chosen to add the (duplicate) input validation anyway as it will make the error message more descriptive by pointing to the actual method which initially received the invalid input.
    Again: opinions welcome.

@jrfnl jrfnl added this to the 2.0.0 milestone Nov 7, 2021
@jrfnl jrfnl requested a review from schlessera November 7, 2021 01:53
@jrfnl jrfnl mentioned this pull request Nov 7, 2021
28 tasks
The documented accepted parameter type for all methods in the `Ipv6` class 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 `uncompress()` method, allowing only for strings and _stringable_ objects.
As the other `public` methods call `uncompress()` as the first point of action in their flow, this is sufficient to ensure input validation in all cases.

As this class was until now only indirectly tested via the `IriTest` class, a new `Ipv6Test` class is being introduced containing - for now - only perfunctory tests for the input validation.
This test class should be expanded to cover the actual logic in the methods at a later date.

**Open questions:**
* It is up for discussion whether _stringable objects_ should even be allowed as input for these methods. Opinions welcome.
@jrfnl jrfnl force-pushed the feature/ipv6-add-input-validation branch from 53511fc to e3b00af Compare November 8, 2021 09:45
@jrfnl
Copy link
Member Author

jrfnl commented Nov 8, 2021

This PR was discussed between @schlessera and me in a call and we've decided that having input validation done once is enough as long as it is safeguarded that the validation is executed.

To that end:

  • I've removed the input validation from the compress() and check_ipv6() methods.
  • Added a note to both these methods about the input validation being done in the uncompress() method.

I've left the tests for compress() and check_ipv6() expecting an exception on invalid input in place. Instead of directly testing the input validation, those now safeguard that the input validation is actually done for those methods.

@schlessera schlessera merged commit e13b535 into develop Nov 8, 2021
@schlessera schlessera deleted the feature/ipv6-add-input-validation branch November 8, 2021 10:30
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