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

Workaround for ipv4 address validation #5135

Closed
wants to merge 17 commits into from
Closed

Workaround for ipv4 address validation #5135

wants to merge 17 commits into from

Conversation

HJones82493
Copy link

This fixes the is_ipv4_address function. Initially intended to be a workaround for Issue #5131 it is now a removal of an external dependency. Since the check is a simple one, there is no need to rely on external sources.

@@ -616,7 +616,8 @@ def address_in_network(ip, net):

:rtype: bool
"""
ipaddr = struct.unpack('=L', socket.inet_aton(ip))[0]
if not is_ipv4_address(ip):

Choose a reason for hiding this comment

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

The code below uses ipaddr local variable which has been removed.

Choose a reason for hiding this comment

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

I assume that we don't care about 3-1 octet IP addresses that the socket.inet_aton can parse e.g.:

In [11]: socket.inet_aton('0x7f000001')
Out[11]: b'\x7f\x00\x00\x01'

In [12]: socket.inet_aton('127.0.0')
Out[12]: b'\x7f\x00\x00\x00'

In [13]: socket.inet_aton('127.0')
Out[13]: b'\x7f\x00\x00\x00'

If so, we can just use the builtin ipaddress module to validate if IP is in proper format:

In [2]: ipaddress.IPv4Address('192.168.0.1')
Out[2]: IPv4Address('192.168.0.1')

In [3]: ipaddress.IPv4Address('192')
AddressValueError: Expected 4 octets in '192'

and then use the ipaddr = struct.unpack('=L', socket.inet_aton(ip))[0] line to parse it.

Note that the same problem stands for net argument - i.e. it also uses sockeet.inet_aton so we need to validate it's format first.

Copy link
Author

Choose a reason for hiding this comment

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

I didn't plan on handling 1-3 octet IPs unless we also consider those valid? I didn't think we would consider those as valid IPs.

We already have a check for valid ip that works (is_valid_ipv4) that I changed to remove the socket dependency in it. I think the best solution would be to leave the socket dependencies in the functions that use them and just put a check first, that way we don't allow it to pass IPs like '1.1.1.1 wtf' into the broken Python functions.

@disconnect3d
Copy link

Just for a reference, the socket.inet_aton issue is being discussed on Python's bug tracker: https://bugs.python.org/issue37495

@@ -645,11 +645,12 @@ def is_ipv4_address(string_ip):
return False
for octet in ip_split:
try:
int(octet)
tmp = int(octet)

Choose a reason for hiding this comment

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

We already have a check for valid ip that works (is_valid_ipv4)
Sure, but it's better to use standard methods that work properly instead of reinventing the wheel.

The current version of is_ipv4_address still doesn't handle all the edge cases - for example, it returns True for an ip of '08.08.08.08'.

The ipaddress module has functions to check given IPs and networks. This could probably be used for both IP and CIDR checking.

Copy link
Author

Choose a reason for hiding this comment

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

The ipaddress module doesn't exist for Python 2.7

return False
if int(octet[1]) > 0:
return False
tmp = int(octet)

Choose a reason for hiding this comment

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

Here seems to me that it need to rewrite

Choose a reason for hiding this comment

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

between 646 and 657

@nateprewitt nateprewitt changed the base branch from master to main January 3, 2022 15:29
@nateprewitt
Copy link
Member

Now that we've dropped support for Python 2.7-3.6, I think we're more likely to rely on ipaddress than try to maintain this ourselves. I'm going to close this out in favor of that approach.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 12, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants