Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Using django-ipware get_client_ip instead of get_real_ip #45

Closed
shadinaif opened this issue Sep 15, 2018 · 5 comments
Closed

Using django-ipware get_client_ip instead of get_real_ip #45

shadinaif opened this issue Sep 15, 2018 · 5 comments

Comments

@shadinaif
Copy link

shadinaif commented Sep 15, 2018

Please help!
Already asked in stackoverflow: https://stackoverflow.com/questions/52162711/using-django-ipware-get-client-ip-instead-of-get-real-ip

In django-ipware version 2.1 ; old get_real_ip function is deprecated. When I use the new get_client_ip ; my test units are not showing the same results. Means that the two functions do not behave the same.

The following is an original test from django-ipware test unit (not mine)

def test_http_x_forwarded_for_multiple(self):
    request = HttpRequest()
    request.META = {
        'HTTP_X_FORWARDED_FOR': '192.168.255.182, 10.0.0.0, 127.0.0.1, 198.84.193.157, 177.139.233.139',
        'HTTP_X_REAL_IP': '177.139.233.132',
        'REMOTE_ADDR': '177.139.233.133',
    }
    ip = get_real_ip(request)
    self.assertEqual(ip, "198.84.193.157")

The above works fine of course, but I want to ensure that using the new get_client_ip will give the same results (for a system upgrade purposes). But the test is actually failing the assertion:

def test_http_x_forwarded_for_multiple(self):
    request = HttpRequest()
    request.META = {
        'HTTP_X_FORWARDED_FOR': '192.168.255.182, 10.0.0.0, 127.0.0.1, 198.84.193.157, 177.139.233.139',
        'HTTP_X_REAL_IP': '177.139.233.132',
        'REMOTE_ADDR': '177.139.233.133',
    }
    ip, is_routable = get_client_ip(request)
    self.assertEqual(ip, "198.84.193.157")

resulting:

AssertionError: '177.139.233.132' != '198.84.193.157'

After digging into the code, I found that the new get_client_ip is not iterating inside the meta like get_real_ip . It checks out the left-most ip (or right-most depending on the settings) and skips to the next meta if a Public IP is not found

My question(s) now are:
How can I call get_client_ip in a way that returns the same ip returned by get_real_ip ? What is the logic behind changing the behavior of the function ? Should I trust the new get_client_ip and forget about get_real_ip, or keep using the deprecated get_real_ip and forget about the new get_client_ip ?????

@un33k
Copy link
Owner

un33k commented Sep 15, 2018

@shadinaif Version 1 was simple and just wanted to get your the first routable IP address it found so users could identify clients uniquely.

Version 2 on the other hand, allows the advanced users to pass in the trusted proxies, the number of proxies as well as a custom right-most flag, as per their network configuration.

The default behavior is to find the left-most ip address for HTTP_X_FORWARDED_FOR OR X_FORWARDED_FOR and if that IP is not routable, it will go down the IPWARE_META_PRECEDENCE_ORDER to find the best routable IP address.

As for HTTP_X_FORWARDED_FOR OR X_FORWARDED_FOR, if the left most IP address is not routable, it cannot be client's read IP address, so it skips over to find the next routable IP.

SemVer was update to 2.0 indicating a backward incompatibility.

Feel free to switch to version two.

Closing the issue, but feel free to reopen if you feel that you have found a bug in version 2.

@un33k un33k closed this as completed Sep 15, 2018
@shadinaif
Copy link
Author

Thank you @un33k 👍

@kleptog
Copy link

kleptog commented Sep 17, 2020

The above explanation is a bit incomplete. If I read the code right, it stops on the first public IP, so if you are coming from a local private network, then get_client_ip will return based on the lowest priority header. Not necessarily a problem, but not an obvious change from the previous version.

Just had to fix our test cases :)

@un33k
Copy link
Owner

un33k commented Sep 17, 2020

@kleptog Moving from 1.x to 2.x was a breaking change and the behavior changed as well. Therefore your test cases that were for 1.x needed to also changed. Hope this helps,

@un33k
Copy link
Owner

un33k commented Mar 8, 2023

@kleptog django-ipware is on its way to being deprecated.

With that said, I have a python package that is going to replace it.

Have a look at python-ipware and if works for you switch to it.

Feedback is welcomed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants