Refactor get_ip_address to avoid 500 errors#4614
Merged
mauromsl merged 4 commits intoFeb 14, 2025
Merged
Conversation
Collaborator
Author
|
Original code by @LorenzoGrama |
mauromsl
requested changes
Feb 11, 2025
mauromsl
left a comment
mauromsl
left a comment
Member
There was a problem hiding this comment.
Thank you @yakky (and @LorenzoGrama) , good catch!
I just made a comment regarding LoginAttempt.ip_address being now nullable, which I think we do not need. thoughts?
|
|
||
| class LoginAttempt(models.Model): | ||
| ip_address = models.GenericIPAddressField() | ||
| ip_address = models.GenericIPAddressField(blank=True, null=True) |
Member
There was a problem hiding this comment.
Is it even worth registering a login attempt if we do not have an IP address?
As this model is only used to count the number of failed attempts from a given IP, we might use the python logger as INFO for correlation but ignore the write to db
There was a problem hiding this comment.
Yes, I agree. Should I add something like the following?
def add_failed_login_attempt(request):
user_agent, ip_address = get_ua_and_ip(request)
if ip_address:
models.LoginAttempt.objects.create(user_agent=user_agent, ip_address=ip_address)Or we risk IntegrityError in the unlikely case that ip is None
ajrbyers
approved these changes
Feb 12, 2025
mauromsl
approved these changes
Feb 14, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This refactoring aims to prevent 500 errors when the
HTTP_X_FORWARDED_FORandREMOTE_ADDRheaders are manipulated by potentially malicious users and do not contain a valid IP address.For example, the following request could cause such an issue:
wget "http://<domain>/article/pubid/<pubid>/" -O /dev/null --header="X-Forwarded-For: some-random-text,1.2.3.4"Models changes
The
LoginAttempt.ip_addressfield needs to be updated to allowNonevalues. This is necessary because if theget_ip_addressmethod returnsNone, we need to be able to saveNonewhen there are failed login attempts with invalid or manipulated headers.The same applies to the
Contact.client_ipfield, which is set injournal.views.contactusingnew_contact.client_ip = shared.get_ip_address(request). Without this change, an error will occur if the IP address is None.Although it's highly unlikely that ipware will ever return
None, actually Itsget_client_ipmethod can returnNonevalues.Code changes
We also need to handle TypeError in the
get_iso_country_codemethod, asgeoip2.database.Reader().countrywill fail if the IP address isNone. It seems appropriate to modify this method to returnNone, as its output is used byiso_to_country_object, which already returnsNoneif the corresponding Country object is not found among the registered Countries.