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.

6 files changed, 91 insertions, 23 deletions

diff --git a/raven/contrib/django/client.py b/raven/contrib/django/client.py
index 6f81a62..7ac20a8 100644
--- a/raven/contrib/django/client.py
+++ b/raven/contrib/django/client.py
@@ -30,7 +30,7 @@ from raven.contrib.django.utils import get_data_from_template, get_host
 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 @@ class DjangoClient(Client):
     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 @@ class DjangoClient(Client):
                 else:
                     authenticated = user.is_authenticated
                 if not authenticated:
-                    return None
+                    return user_info
 
             user_info = {}
 
@@ -164,22 +171,18 @@ class DjangoClient(Client):
                 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()
diff --git a/raven/contrib/flask.py b/raven/contrib/flask.py
index 39b4b69..4853ffa 100644
--- a/raven/contrib/flask.py
+++ b/raven/contrib/flask.py
@@ -143,11 +143,18 @@ class Sentry(object):
         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
@@ -155,17 +162,15 @@ class Sentry(object):
             # 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']:
diff --git a/raven/utils/wsgi.py b/raven/utils/wsgi.py
index f68bd5e..71f4679 100644
--- a/raven/utils/wsgi.py
+++ b/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')
diff --git a/tests/contrib/django/tests.py b/tests/contrib/django/tests.py
index 14b68a1..e76f1f5 100644
--- a/tests/contrib/django/tests.py
+++ b/tests/contrib/django/tests.py
@@ -250,6 +250,7 @@ class DjangoClientTest(TestCase):
     @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 DjangoClientTest(TestCase):
             email='admin@example.com',
             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 DjangoClientTest(TestCase):
     @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 @@ class DjangoClientTest(TestCase):
             email='admin@example.com',
             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,
diff --git a/tests/contrib/flask/tests.py b/tests/contrib/flask/tests.py
index ab7d803..e90a70d 100644
--- a/tests/contrib/flask/tests.py
+++ b/tests/contrib/flask/tests.py
@@ -59,7 +59,7 @@ def create_app(ignore_exceptions=None, debug=False, **config):
     def capture_exception():
         try:
             raise ValueError('Boom')
-        except:
+        except Exception:
             current_app.extensions['sentry'].captureException()
         return 'Hello'
 
@@ -318,4 +318,4 @@ class FlaskLoginTest(BaseTest):
         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()))
diff --git a/tests/utils/wsgi/tests.py b/tests/utils/wsgi/tests.py
index 5b2ffdc..79022ae 100644
--- a/tests/utils/wsgi/tests.py
+++ b/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 @@ class GetHostTest(TestCase):
             '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')