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

Correct IP validation logic order-of-operations #92

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

YSaxon
Copy link

@YSaxon YSaxon commented Nov 21, 2023

This pull request fixes the logic used for validating IP addresses.

Old code:

(existy(value) && regexes.ipv4.test(value)) || regexes.ipv6.test(value)

In pseudocode

If value is not null and is IPv4, return true.
If value is IPv6, return true.
Otherwise, return false.

The existing code evaluates regexes.ipv6.test(value) regardless of whether existy(value) returns true or false. This happens due to the parenthesis placement, which only associates existy(value) with regexes.ipv4.test(value).

New code:

existy(value) && (regexes.ipv4.test(value) || regexes.ipv6.test(value))

In psuedocode:

If value is null, return false.
If value is IPv4, return true.
If value is IPv6, return true.
Otherwise, return false.

The updated logic ensures that both regexes.ipv4.test(value) and regexes.ipv6.test(value) are evaluated only if existy(value) is true. This is achieved by encompassing the OR condition within parentheses, applying the existy(value) check to both IPv4 and IPv6 conditions.

Presumably the ipv6 regex doesn't throw an error when handling a null value, or you would have had bug reports about this before. So you could also consider just removing the null check. But if you want to keep the null check in there for efficiency, to quickly return false without applying the ipv4 regex if there is nothing to check, then this pull request will double that efficiency gain by also avoiding applying the ipv6 regex check to null values, without any additional null checking.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant