Always supply a user.ip_address value

This is explicitly choosing to also parse the X-Forwarded-For header to
yank out this value. Otherwise, the Sentry server relies only on the
REMOTE_ADDR value which will always be wrong when when someone is behind
a reverse proxy.

This logic already exists in some other clients, and this has been
brought up a number of times with users via tickets and support.

Worth noting that it's potentially possible for this value to now be
forged from a user, but the ramification of doing so is so low, it's not
worth putting this behavior behind a feature flag IMO. The worst someone
could do is make some data inside Sentry inaccurate and possibly
confusing. No worse than the current state of the world where the data
is completely inaccurate.

Author: mattrobenolt

raven/contrib/django/client.py

@@ -30,7 +30,7 @@
 from raven.contrib.django.middleware import SentryMiddleware
 from raven.utils.compat import string_types, binary_type, iterlists
 from raven.contrib.django.resolver import RouteResolver
-from raven.utils.wsgi import get_headers, get_environ
+from raven.utils.wsgi import get_headers, get_environ, get_client_ip
 from raven.utils import once
 from raven import breadcrumbs
 
@@ -142,7 +142,14 @@ def __init__(self, *args, **kwargs):
     def install_sql_hook(self):
         install_sql_hook()
 
-    def get_user_info(self, user):
+    def get_user_info(self, request):
+        user_info = {
+            'ip_address': get_client_ip(request.META),
+        }
+        user = getattr(request, 'user', None)
+        if user is None:
+            return user_info
+
         try:
             if hasattr(user, 'is_authenticated'):
                 # is_authenticated was a method in Django < 1.10
@@ -151,7 +158,7 @@ def get_user_info(self, user):
                 else:
                     authenticated = user.is_authenticated
                 if not authenticated:
-                    return None
+                    return user_info
 
             user_info = {}
 
@@ -164,22 +171,18 @@ def get_user_info(self, user):
                 user_info['username'] = user.get_username()
             elif hasattr(user, 'username'):
                 user_info['username'] = user.username
-
-            return user_info
         except Exception:
             # We expect that user objects can be somewhat broken at times
             # and try to just handle as much as possible and ignore errors
             # as good as possible here.
-            return None
+            pass
+
+        return user_info
 
     def get_data_from_request(self, request):
         result = {}
 
-        user = getattr(request, 'user', None)
-        if user is not None:
-            user_info = self.get_user_info(user)
-            if user_info:
-                result['user'] = user_info
+        result['user'] = self.get_user_info(request)
 
         try:
             uri = request.build_absolute_uri()

raven/contrib/flask.py

@@ -143,29 +143,34 @@ def get_user_info(self, request):
         Requires Flask-Login (https://pypi.python.org/pypi/Flask-Login/)
         to be installed and setup.
         """
+        try:
+            user_info = {
+                'ip_address': request.access_route[0],
+            }
+        except IndexError:
+            user_info = {}
+
         if not has_flask_login:
-            return
+            return user_info
 
         if not hasattr(current_app, 'login_manager'):
-            return
+            return user_info
 
         try:
             is_authenticated = current_user.is_authenticated
         except AttributeError:
             # HACK: catch the attribute error thrown by flask-login is not attached
             # >   current_user = LocalProxy(lambda: _request_ctx_stack.top.user)
             # E   AttributeError: 'RequestContext' object has no attribute 'user'
-            return {}
+            return user_info
 
         if callable(is_authenticated):
             is_authenticated = is_authenticated()
 
         if not is_authenticated:
-            return {}
+            return user_info
 
-        user_info = {
-            'id': current_user.get_id(),
-        }
+        user_info['id'] = current_user.get_id()
 
         if 'SENTRY_USER_ATTRS' in current_app.config:
             for attr in current_app.config['SENTRY_USER_ATTRS']:

raven/utils/wsgi.py

@@ -92,3 +92,17 @@ def get_current_url(environ, root_only=False, strip_querystring=False,
             if qs:
                 cat('?' + qs)
     return ''.join(tmp)
+
+
+def get_client_ip(environ):
+    """
+    Naively yank the first IP address in an X-Forwarded-For header
+    and assume this is correct.
+
+    Note: Don't use this in security sensitive situations since this
+    value may be forged from a client.
+    """
+    try:
+        return environ['HTTP_X_FORWARDED_FOR'].split(',')[0].strip()
+    except (KeyError, IndexError):
+        return environ.get('REMOTE_ADDR')

tests/contrib/django/tests.py

@@ -250,6 +250,7 @@ def test_user_info(self):
     @pytest.mark.skipif(not DJANGO_15, reason='< Django 1.5')
     def test_get_user_info_abstract_user(self):
         from django.db import models
+        from django.http import HttpRequest
         from django.contrib.auth.models import AbstractBaseUser
 
         class MyUser(AbstractBaseUser):
@@ -263,8 +264,25 @@ class MyUser(AbstractBaseUser):
             email='[email protected]',
             id=1,
         )
-        user_info = self.raven.get_user_info(user)
+
+        request = HttpRequest()
+        request.META['REMOTE_ADDR'] = '127.0.0.1'
+        request.user = user
+        user_info = self.raven.get_user_info(request)
+        assert user_info == {
+            'ip_address': '127.0.0.1',
+            'username': user.username,
+            'id': user.id,
+            'email': user.email,
+        }
+
+        request = HttpRequest()
+        request.META['REMOTE_ADDR'] = '127.0.0.1'
+        request.META['HTTP_X_FORWARDED_FOR'] = '1.1.1.1, 2.2.2.2'
+        request.user = user
+        user_info = self.raven.get_user_info(request)
         assert user_info == {
+            'ip_address': '1.1.1.1',
             'username': user.username,
             'id': user.id,
             'email': user.email,
@@ -273,6 +291,7 @@ class MyUser(AbstractBaseUser):
     @pytest.mark.skipif(not DJANGO_110, reason='< Django 1.10')
     def test_get_user_info_is_authenticated_property(self):
         from django.db import models
+        from django.http import HttpRequest
         from django.contrib.auth.models import AbstractBaseUser
 
         class MyUser(AbstractBaseUser):
@@ -289,8 +308,25 @@ def is_authenticated(self):
             email='[email protected]',
             id=1,
         )
-        user_info = self.raven.get_user_info(user)
+
+        request = HttpRequest()
+        request.META['REMOTE_ADDR'] = '127.0.0.1'
+        request.user = user
+        user_info = self.raven.get_user_info(request)
+        assert user_info == {
+            'ip_address': '127.0.0.1',
+            'username': user.username,
+            'id': user.id,
+            'email': user.email,
+        }
+
+        request = HttpRequest()
+        request.META['REMOTE_ADDR'] = '127.0.0.1'
+        request.META['HTTP_X_FORWARDED_FOR'] = '1.1.1.1, 2.2.2.2'
+        request.user = user
+        user_info = self.raven.get_user_info(request)
         assert user_info == {
+            'ip_address': '1.1.1.1',
             'username': user.username,
             'id': user.id,
             'email': user.email,

tests/contrib/flask/tests.py

@@ -59,7 +59,7 @@ def log_a_generic_error():
     def capture_exception():
         try:
             raise ValueError('Boom')
-        except:
+        except Exception:
             current_app.extensions['sentry'].captureException()
         return 'Hello'
 
@@ -318,4 +318,4 @@ def test_user(self):
         assert event['message'] == 'ValueError: hello world'
         assert 'request' in event
         assert 'user' in event
-        self.assertDictEqual(event['user'], User().to_dict())
+        self.assertDictEqual(event['user'], dict({'ip_address': '127.0.0.1'}, **User().to_dict()))

tests/utils/wsgi/tests.py

@@ -1,5 +1,5 @@
 from raven.utils.testutils import TestCase
-from raven.utils.wsgi import get_headers, get_host, get_environ
+from raven.utils.wsgi import get_headers, get_host, get_environ, get_client_ip
 
 
 class GetHeadersTest(TestCase):
@@ -84,3 +84,13 @@ def test_http_nonstandard_port(self):
             'SERVER_PORT': '81',
         })
         self.assertEquals(result, 'example.com:81')
+
+
+class GetClientIpTest(TestCase):
+    def test_has_remote_addr(self):
+        result = get_client_ip({'REMOTE_ADDR': '127.0.0.1'})
+        self.assertEquals(result, '127.0.0.1')
+
+    def test_xff(self):
+        result = get_client_ip({'HTTP_X_FORWARDED_FOR': '1.1.1.1, 127.0.0.1'})
+        self.assertEquals(result, '1.1.1.1')