diff options
author | Matt Robenolt <matt@ydekproductions.com> | 2017-02-27 18:58:48 -0800 |
---|---|---|
committer | Ashley Camba <ashwoods@gmail.com> | 2017-09-26 21:16:59 +0200 |
commit | e3aad24b7a443e59b824a40bb17cd49593e3b3d3 (patch) | |
tree | 143d3ca181dde13432a9d5376b93dedebcd8df44 | |
parent | 2c30f0f1add43c5d8903bb3744b58cedbc41654a (diff) | |
download | raven-e3aad24b7a443e59b824a40bb17cd49593e3b3d3.tar.gz |
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.
-rw-r--r-- | raven/contrib/django/client.py | 25 | ||||
-rw-r--r-- | raven/contrib/flask.py | 19 | ||||
-rw-r--r-- | raven/utils/wsgi.py | 14 | ||||
-rw-r--r-- | tests/contrib/django/tests.py | 40 | ||||
-rw-r--r-- | tests/contrib/flask/tests.py | 4 | ||||
-rw-r--r-- | tests/utils/wsgi/tests.py | 12 |
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') |