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

Add input validation for toLong #95

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

eritikass
Copy link

@eritikass eritikass commented Oct 31, 2018

This pull request is adding input validation for toLong method as for example it is possible to only get long values from ipv4 addresses.

NB: ci tests are failing, it is because mocha deps are not compatible with older nodes anymore (and the project has no lock file)... happy to resolve in separate in a separate pull, but I see there is already few open pull that tackles that issue.

it is possible to have ip value of null, for example from express.
@glensc
Copy link

glensc commented Oct 31, 2018

you forgot to add link to your mentioned pull request

@glensc
Copy link

glensc commented Oct 31, 2018

what's the current behavior? as 0 is also valid ip (0.0.0.0), it's not correct to return wrong value because you can't parse it.

maybe more accurate is to raise an error instead?

@glensc
Copy link

glensc commented Oct 31, 2018

what is the scenario where express returns null?

@eritikass
Copy link
Author

@glensc - you have probably right here, this function should throw an error if the input is invalid, check code there is already at least one other function who does it also. So updated my changes, so now it throws error if not valid ipv4

@eritikass
Copy link
Author

eritikass commented Nov 1, 2018

and there can be one more issue - ipv4 aadresses embeded into ipv6 ... in our case few examples
::ffff:192.128.24.214 or ::ffff:10.132.71.0

for those .isV4Format is false and .isV6Format is true

http://www.tcpipguide.com/free/t_IPv6IPv4AddressEmbedding.htm

@glensc
Copy link

glensc commented Nov 1, 2018

::ffff:192.128.24.214 is ipv6 address (term is afaik ipv4 translated/mapped address 1). and the method should throw an error if you attempt to get UINT4 out of it.

hint: disable ipv6 listen in your express application (listen to ipv4_all: 0.0.0.0)

ip.toLong(null);
}, Error);
assert.throws(function () {
ip.toLong(undefined);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

false, empty string '', Function ? :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would end up testing here this .isV4Format more as in the end this will be used to validated input - so just added few possible values that can perhaps happens as bad input other than ipv6

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.

2 participants