-
Notifications
You must be signed in to change notification settings - Fork 59
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
Fix edge cases with unknown IP address #111
base: master
Are you sure you want to change the base?
Conversation
Reviewer's Guide by SourceryThis pull request addresses the issue where HTTP_X_FORWARDED_FOR might contain 'unknown' followed by a valid IP address. The _get_ip_address method in rest_framework_tracking/base_mixins.py was updated to parse and use the closest valid IP address. Additionally, a new test case was added to tests/test_mixins.py to ensure the new logic works as expected. File-Level Changes
Tips
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @yuekui - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 4 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
raw_possibles = request.META.get("HTTP_X_FORWARDED_FOR", "").split(",") | ||
raw_possibles += request.META.get("REMOTE_ADDR", "").split(",") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (performance): Combining lists directly can be inefficient.
Using the +=
operator to combine lists can be less efficient than using extend()
, especially for large lists. Consider using raw_possibles.extend(request.META.get("REMOTE_ADDR", "").split(","))
for better performance.
raw_possibles = request.META.get("HTTP_X_FORWARDED_FOR", "").split(",") | |
raw_possibles += request.META.get("REMOTE_ADDR", "").split(",") | |
raw_possibles = request.META.get("HTTP_X_FORWARDED_FOR", "").split(",") | |
raw_possibles.extend(request.META.get("REMOTE_ADDR", "").split(",")) |
for addr in possibles: | ||
try: | ||
return str(ipaddress.ip_address(addr)) | ||
except ValueError: | ||
pass | ||
|
||
return ipaddr | ||
return raw_possibles[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue (bug_risk): Returning the first element of raw_possibles may not be reliable.
If raw_possibles
is empty, this line will raise an IndexError
. Consider adding a check to ensure raw_possibles
is not empty before attempting to access its first element.
For some reason,
HTTP_X_FORWARDED_FOR
might be set as"unknown, 128.1.1.9"
, we should be able to parse and use the closest valid value.Summary by Sourcery
This pull request addresses an edge case in IP address parsing where the
HTTP_X_FORWARDED_FOR
header might contain 'unknown'. The code now correctly parses and uses the closest valid IP address. Additionally, a test has been added to ensure this functionality works as expected.HTTP_X_FORWARDED_FOR
header contains 'unknown' by parsing and using the closest valid IP address.HTTP_X_FORWARDED_FOR
contains 'unknown'.