Opened 2 years ago

Closed 4 months ago

Last modified 4 months ago

#8241 defect closed fixed (fixed)

WSGI throws exception with IPv6 client due to string conversion on None result from twisted.web.http.getClientIP

Reported by: Trent Lloyd Owned by: mark williams
Priority: normal Milestone:
Component: web Keywords:
Cc: Branch:
Author:

Description

Connecting to a WSGI instance with an IPv6 client throws an exception in the request processing.

File "/usr/lib/python3/dist-packages/twisted/web/wsgi.py", line 293, in init

'REMOTE_ADDR': _wsgiString(request.getClientIP()),

File "/usr/lib/python3/dist-packages/twisted/web/wsgi.py", line 85, in _wsgiString

return string.decode("iso-8859-1")

builtins.AttributeError: 'NoneType' object has no attribute 'decode'

The problem is that twisted.web.http.getClientIP() specifically checks that the address object is an instance of IPv4Address before returning, otherwise it returns None which then blows up in the string type conversion by _wsgiString()

This has previously been documented in #7704, however that bug was closed as wontfix and split into bugs about designing a better interface then the current interface. While I agree that is a good goal, it would be create to fix the basic support now. In either case the current call within the WSGI module would be deprecated so making a change here seems fine in that regard.

Attaching patch suggestions for review input in the next update.

Change History (8)

comment:1 Changed 2 years ago by Trent Lloyd

I am not sure on the "best" way to make this change, so seeking review input.

The first change, is we could simply modify twisted.web.http.getClientIP() to check for either an IPv4Address or IPv6Address. The code will work in either case:

diff --git a/twisted/web/http.py b/twisted/web/http.py
index 66ba3da..07f6672 100644
--- a/twisted/web/http.py
+++ b/twisted/web/http.py
@@ -1244,7 +1244,7 @@ class Request:
         @returns: the client IP address
         @rtype: C{str}
         """
-        if isinstance(self.client, address.IPv4Address):
+        if isinstance(self.client, address.IPv4Address) or isinstance(self.client, address.IPv6Address):
             return self.client.host
         else:
             return None

The other change we could make is to simply bypass this function, and lookup the attribute directly in the wsgi module. This would require further work to work with any other socket type, but they wouldn't work now anyway as above.

diff --git a/twisted/web/wsgi.py b/twisted/web/wsgi.py
index 3f30658..b72d211 100644
--- a/twisted/web/wsgi.py
+++ b/twisted/web/wsgi.py
@@ -290,7 +290,7 @@ class _WSGIResponse:
         # *both* Python 2 and Python 3, so says PEP-3333.
         self.environ = {
             'REQUEST_METHOD': _wsgiString(request.method),
-            'REMOTE_ADDR': _wsgiString(request.getClientIP()),
+            'REMOTE_ADDR': _wsgiString(request.client.host),
             'SCRIPT_NAME': _wsgiString(scriptName),
             'PATH_INFO': _wsgiString(pathInfo),
             'QUERY_STRING': _wsgiString(queryString),

The rest of the code seems to work fine with an IPv6 request and works in my application with this change, my main concern is breaking other use cases of the code but I suspect that they would already be broken since it would always return None and thus throw an error. Would appreciate your input.

comment:2 Changed 2 years ago by Trent Lloyd

Summary: WSGI throws exception with IPv6 client due to twisted.web.getClientIPWSGI throws exception with IPv6 client due to string conversion on None result from twisted.web.http.getClientIP

comment:3 Changed 20 months ago by Glyph

See also #7704

comment:4 Changed 4 months ago by mark williams

Keywords: review added

comment:5 Changed 4 months ago by Adi Roiban

Keywords: review removed
Owner: set to mark williams

comment:6 Changed 4 months ago by mark williams

Keywords: review added

I actually fixed #7704 as well, and described in the newsfragment. I've added a test for this issue and fixed the newsfragment.

comment:7 Changed 4 months ago by Craig Rodrigues <rodrigc@…>

Resolution: fixed
Status: newclosed

In f2c70b0:

Merge pull request #975 from twisted/8241-ipv6-client-ip

Author: markrwilliams
Reviewer: adiroiban
Fixes: ticket:8241

Allow WSGI clients with IPv6 addresses

comment:8 Changed 4 months ago by Craig Rodrigues

Keywords: review removed
Note: See TracTickets for help on using tickets.