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

Invalid ipv4 address are converted to valid ones #58

Closed
zcstarr opened this issue Mar 14, 2018 · 4 comments
Closed

Invalid ipv4 address are converted to valid ones #58

zcstarr opened this issue Mar 14, 2018 · 4 comments

Comments

@zcstarr
Copy link
Contributor

zcstarr commented Mar 14, 2018

Summary: The ipv4 regex here is what node-ip uses to verify valid ip address format, this logic is being used to validate ip addresses , and right now an invalid address 555.555.555.55 will be converted to a valid address and return 43.43.43.55.

Proposal: I think ideally it would make sense that the regex within the ip-node package were fixed, however the project seems like it hasn't been touched in the past year. Alternatively I think it might make sense to just wrap the calls here and here with a regex check and throw an error just like the package would in the event of an ascii character in the ip address.

@vmx
Copy link
Member

vmx commented Mar 14, 2018

As it looks like an upstream fix won't be merged soon, wrapping it directly makes sense. If you add the additional check, how hard would it be to also do an upstream patch? I'm always in favour of fixing upstream bugs as well.

@zcstarr
Copy link
Contributor Author

zcstarr commented Mar 14, 2018

I think it would be really simple its really just changing the regex within ip-node, I'll file a PR in both places 🤞 if the upstream lands :)

@zcstarr
Copy link
Contributor Author

zcstarr commented Mar 19, 2018

Wrote the patch for ipv4 and realized ipv6 validations are also incorrect. Ipv6 validation is a little more complex, I'm not yet convinced that ip-node does the right thing, even if I patch the first issue that I've spotted with ipv6. It might be a decent idea to use a different package for translating the ipv6 string to a buffer. This seems fairly complete, downside here is that it's coffeescript and might not be a package that belongs within the ecosystem.

@daviddias
Copy link
Member

Fixed with #60

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

No branches or pull requests

3 participants