728862 - is_valid() in SetRemoteAddrFromForwardedFor::process_request() is not IPv6-safe

Status: RESOLVED WORKSFORME

(addons.mozilla.org Graveyard :: Public Pages, defect)

Description

[Reed Loden [:reed]]

https://github.com/jsocol/commonware/blob/master/commonware/request/middleware.py

Not sure where bugs in middleware go, so filing this here...

is_valid() is defined as the following:

        def is_valid(ip):
            try:
                socket.inet_aton(ip)
                return True
            except socket.error:
                return False

However, inet_aton() is not IPv6-safe. From http://docs.python.org/library/socket.html#socket.inet_aton, "inet_aton() does not support IPv6, and inet_pton() should be used instead for IPv4/v6 dual stack support."

Instead of dealing with the socket layer, you could just use some IP validation library instead... See Perl's Data::Validation::IP module's is_ipv4() and is_ipv6() functions.

Comment 1

[James Socol [:jsocol, :james]]

On Github repos that have issues, please file Github issues. I filed https://github.com/jsocol/commonware/issues/10.

Perl library functions are most useless to us here ;)

Comment 2

[James Socol [:jsocol, :james]]

Fixed in v0.3.0 of commonware[1].

Note that users of the Graphite* middleware will need to start using django-statsd[2] if they want to upgrade commonware, as those classes have moved out of commonware and into that project.

Now it's up to someone to upgrade this in AMO.

[1] https://github.com/jsocol/commonware
[2] https://github.com/andymckay/django-statsd

Comment 3

[David Larlet [:davidbgk]]

Given that the current commonware version in production is 0.4.2, can we close that bug now?

Comment 4

[Mathieu Pillard [:mat]]

Yeah. Do check out bug 975888 though, looks similar.