Skip to content

Update GetRemoteAddr documentation to clarify that it may return nil. #1149

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

Merged
merged 1 commit into from
Apr 22, 2024

Conversation

pkulchenko
Copy link
Collaborator

No description provided.

Copy link
Collaborator

@mrdomino mrdomino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

It is probably worth mentioning that the nil case can only happen if the client IP is trusted and contains an erroneous X-Forwarded-For.

As a matter of fact, the documentation for this method could probably be improved overall. IsTrustedIp seems to encompass more cases than just IsPrivateIp or IsLoopbackIp, so there's probably a simple sentence or two that describes what GetRemoteAddr currently does including the nil case.

@pkulchenko
Copy link
Collaborator Author

pkulchenko commented Apr 22, 2024

Right, but it doesn't even need to be erroneous (in the true sense). I think the last case where we ran into this was an IPv6 address supplied as part of the X-Forwarded-For header.

I wasn't sure how detailed we wanted the documentation to be, but thought it was worth mentioning that nil may be returned. Do you want me to add the IPv6 case?

@mrdomino
Copy link
Collaborator

This is a part of the system that I'm not too familiar with, so I'll defer to you on this one.

Perhaps something like "returns the client IP, unless the client IP is trusted and includes an X-Forwarded-For. In the latter case, if we fail to understand the forwarded IP (e.g. because it is misformatted, or an IPv6 address), this method returns nil."

@pkulchenko
Copy link
Collaborator Author

Updated the description; there are more cases covered in IsTrustedIp description, so I just referred to that.

@mrdomino
Copy link
Collaborator

Great, looks good.

@mrdomino mrdomino merged commit 01267ea into jart:master Apr 22, 2024
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

Successfully merging this pull request may close these issues.

2 participants