Fix edge cases with unknown IP address#111
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.
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.
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(",")) |
| pass | ||
|
|
||
| return ipaddr | ||
| return raw_possibles[0] |
There was a problem hiding this comment.
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_FORmight 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_FORheader 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_FORheader contains 'unknown' by parsing and using the closest valid IP address.HTTP_X_FORWARDED_FORcontains 'unknown'.