REMOTE_ADDR may contain invalid data when using Nginx as reverse proxy

[iarp]

/paypal/standard/models.py the method named def initialize(self, request): has the following line:

self.ipaddress = request.META.get('REMOTE_ADDR', '')

If you ever reverse proxy a connection to django the REMOTE_ADDR does not contain the end-users actual IP, it'll contain nothing or the proxy servers ip or in my case it actually contained literally "b''" (thats B and 2 single quotes in a string).

There are also cases when someone is using a service such as cloudflare, I changed the self.ipaddress line to the following code so that it'll find an IP somewhere.

for val in ['HTTP_CF_CONNECTING_IP', 'HTTP_X_FORWARDED_FOR', 'HTTP_X_REAL_IP', 'REMOTE_ADDR']:
    self.ipaddress = request.META.get(val, '').split(',')[0]  # proxys may forwarded multiple ips, first one is clients
    if self.ipaddress:
        break

[iarp]

It may also be worth while using something like the following to verify the value given matches a valid IP format before breaking

try:
    socket.inet_aton(self.ipaddress)
    break
except socket.error:
    pass

[iarp]

In case you're curious how I came about this, when paypal went to send notification about a payment and we had just switched over to a nginx reverse proxy system, this is the resulting error:

[30/Jan/2017 14:32:40] ERROR [django.request:124] Internal Server Error: /payments-processor/
Traceback (most recent call last):
  File "/home/dev4371/venv-django/lib/python3.5/site-packages/django/db/backends/utils.py", line 64, in execute
    return self.cursor.execute(sql, params)
psycopg2.DataError: invalid input syntax for type inet: "b''"
LINE 1: ... '', '', '', NULL, '', '', '', 'USD', '0.00', '', 'b'''''::i...
                                                             ^

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/home/dev4371/venv-django/lib/python3.5/site-packages/django/core/handlers/exception.py", line 39, in inner
    response = get_response(request)
  File "/home/dev4371/venv-django/lib/python3.5/site-packages/django/core/handlers/base.py", line 187, in _get_response
    response = self.process_exception_by_middleware(e, request)
  File "/home/dev4371/venv-django/lib/python3.5/site-packages/django/core/handlers/base.py", line 185, in _get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
  File "/home/dev4371/venv-django/lib/python3.5/site-packages/django/views/decorators/http.py", line 40, in inner
    return func(request, *args, **kwargs)
  File "/home/dev4371/venv-django/lib/python3.5/site-packages/django/views/decorators/csrf.py", line 58, in wrapped_view
    return view_func(*args, **kwargs)
  File "/home/dev4371/venv-django/lib/python3.5/site-packages/paypal/standard/ipn/views.py", line 96, in ipn
    ipn_obj.verify()
  File "/home/dev4371/venv-django/lib/python3.5/site-packages/paypal/standard/models.py", line 364, in verify
    self.save()
  File "/home/dev4371/venv-django/lib/python3.5/site-packages/django/db/models/base.py", line 796, in save
    force_update=force_update, update_fields=update_fields)
  File "/home/dev4371/venv-django/lib/python3.5/site-packages/django/db/models/base.py", line 824, in save_base
    updated = self._save_table(raw, cls, force_insert, force_update, using, update_fields)
  File "/home/dev4371/venv-django/lib/python3.5/site-packages/django/db/models/base.py", line 908, in _save_table
    result = self._do_insert(cls._base_manager, using, fields, update_pk, raw)
  File "/home/dev4371/venv-django/lib/python3.5/site-packages/django/db/models/base.py", line 947, in _do_insert
    using=using, raw=raw)
  File "/home/dev4371/venv-django/lib/python3.5/site-packages/django/db/models/manager.py", line 85, in manager_method
    return getattr(self.get_queryset(), name)(*args, **kwargs)
  File "/home/dev4371/venv-django/lib/python3.5/site-packages/django/db/models/query.py", line 1045, in _insert
    return query.get_compiler(using=using).execute_sql(return_id)
  File "/home/dev4371/venv-django/lib/python3.5/site-packages/django/db/models/sql/compiler.py", line 1054, in execute_sql
    cursor.execute(sql, params)
  File "/home/dev4371/venv-django/lib/python3.5/site-packages/django/db/backends/utils.py", line 64, in execute
    return self.cursor.execute(sql, params)
  File "/home/dev4371/venv-django/lib/python3.5/site-packages/django/db/utils.py", line 94, in __exit__
    six.reraise(dj_exc_type, dj_exc_value, traceback)
  File "/home/dev4371/venv-django/lib/python3.5/site-packages/django/utils/six.py", line 685, in reraise
    raise value.with_traceback(tb)
  File "/home/dev4371/venv-django/lib/python3.5/site-packages/django/db/backends/utils.py", line 64, in execute
    return self.cursor.execute(sql, params)
django.db.utils.DataError: invalid input syntax for type inet: "b''"
LINE 1: ... '', '', '', NULL, '', '', '', 'USD', '0.00', '', 'b'''''::i...

[spookylukey]

The Django devs found that there is no reliable code you can have to calculate the remote IP address, and for that reason actually removed a middleware that attempted to automatically handle this for you:

https://www.djangoproject.com/weblog/2009/jul/28/security/#secondary-issue

Ideally you would implement your own middleware that fixes REMOTE_ADDR, perhaps using or https://pypi.python.org/pypi/django-xff/ or https://github.com/un33k/django-ipware (while reading the docs carefully). Making sure REMOTE_ADDR is correct is out of scope for django-paypal.

The only options for us here are: 1) leave as it is, potentially with a note in the docs. 2) remove any use of REMOTE_ADDR

[iarp]

I think saving the information into the database should outrank potentially trying to save a bad ip address that causes errors to be thrown and no data to be saved. Or at least a field type change from GenericIPAddressField to CharField so that it doesn't matter what the value contains.

[spookylukey]

Downgrading a GenericIPAddressField to a CharField would be a significant backwards incompatibility. I personally don't use those fields, but they were presumably added because they were useful to some people. Changing them to a CharField would change them into a junk field whose contents could not be relied upon to contain anything useful, shifting the burden of validation elsewhere, and further away from the source problem. So I think the current behaviour should stay - it is at least telling you that you have a setup where REMOTE_ADDR can be filled with garbage, which is not a good situation to be in, especially if part of your project wants to know that information.

If you simply need to fix this issue and get the data to be saved, you can always set a middleware that puts something like "127.0.0.1" into request.META['REMOTE_ADDR'].

[iarp]

I think at the very least the documentation needs to include a note about this. In my opinion if you're going to use a specialized field for anything you should at least validate said information when you can.

In the readme If you discover a bug, unless it is a critical data loss or security bug,, do you not think failure to save information is critical enough?

Regardless of server anyone can pass whatever they want within the headers information anyways, so it shouldn't just be explicitly trusted to be valid format.

In the end I've just forked it and applied my changes to my own repo.

[spookylukey]

I agree this deserves documentation, and I've now pushed some docs for this. The other proposed changes are just not appropriate:

1) Looking in other headers for the IP address, as per your patch, is dependent on setup, which means that it is unreliable and spoofable (as per previous links). That means it's not appropriate for inclusion in django-paypal, and it's also just the wrong place to do it - there should be a project wide solution to this problem, not one hard-coded into every app.

2) Downgrading the field to a CharField is an issue for anyone who wanted to rely on that field having an IP address (or nothing).

A third option is that when an invalid IP address is seen, it will be discarded and "" saved in the field instead. However, to address my concerns about silent failures (which are nasty to debug), an error should also be logged (using the normal logging framework).

Since there is a (now documented) workaround for this issue, I'm not going to have time to work on this myself. If you would like to provide a patch that implemented this third option, I would happily merge it. It would need to be applied to the IPN, PDT and the NVP model, presumably using a helper that takes a request and returns an IP address (possibly calling logger.error).

Thanks for highlighting the issue, by the way. I'm not trying to be awkward, but I'm only going to add solutions that don't make more problems for people.