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

Add unsafe_remote_ip property #2975

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

betatim
Copy link

@betatim betatim commented Jan 11, 2021

This property represents the IP closest to the user. This means it is
potentially unsafe because clients can fake it but might be more useful for geolocation purposes.

Closes #904


Hi 👋!

This is my first attempt at contributing to tornado (long time user via JupyterHub and BinderHub). I picked #904 because it looked interesting and not too hard. However it is a very old issue so maybe thinking has moved on since then about how useful it is?

I thought I'd open this PR already to get some early feedback. I think if things look about right a better/more doc string is needed and maybe some more edge case test. Also need to look at availability of the ipaddress module in terms of which versions tornado supports.

I think it is ok to always provide this, not only when xheaders=True, as it is always "unsafe" to look at this value/take it at face value.

What do you think of the name of the new property? Is the HTTPServerRequest class the right place for this or should it be in the connection class like the remote_ip handling?

(should this be a draft PR until it is ready for reviewing?)

This property represents the IP closest to the user. This means it is
potentially unsafe.
@bdarnell
Copy link
Member

However it is a very old issue so maybe thinking has moved on since then about how useful it is?

I don't think anyone has changed their minds about how useful this is, but it's always been something that's just a little bit useful, so no one has gotten around to implementing it. I'm happy to have a contribution for it.

Also need to look at availability of the ipaddress module in terms of which versions tornado supports.

Using the ipaddress module is fine (it was added in 3.3; our minimum is now 3.6).

I think it is ok to always provide this, not only when xheaders=True, as it is always "unsafe" to look at this value/take it at face value.

I agree. But it is strange that this uses different rules than xheaders, i.e. that it only looks at X-Forwarded-For and not X-Real-IP, and it only returns "global" IPs. Maybe that's OK (I don't think I'd want to change either of those things), but it should be documented.

What do you think of the name of the new property?

"Unsafe" is OK, but it sounds a little too strong and scary. Maybe "untrusted" or "unverified"?

Is the HTTPServerRequest class the right place for this or should it be in the connection class like the remote_ip handling?

I'm not sure. It feels like they both belong in the same place, but on the other hand I prefer to put things on HTTPServerRequest instead of in the HTTPServer as much as possible if they don't need state or direct access to the connection.

(should this be a draft PR until it is ready for reviewing?)

Nah, I never really use draft PRs.

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

Successfully merging this pull request may close these issues.

Add method to get client's claimed IP
2 participants