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 remote_ip to request #955

Closed
elorest opened this issue Sep 18, 2018 · 6 comments
Closed

Add remote_ip to request #955

elorest opened this issue Sep 18, 2018 · 6 comments

Comments

@elorest
Copy link
Member

elorest commented Sep 18, 2018

Description

Many people have asked how to find the ip of the client as they so easily can in Rails.
Rails does it with this middleware. Would be super useful to Amber and I'm sure Lucky, and Kemal if this middleware was converted to crystal.

https://github.com/rails/rails/blob/5-2-1/actionpack/lib/action_dispatch/middleware/remote_ip.rb

@firoxer
Copy link

firoxer commented Oct 8, 2018

I tried to do something about this but it turned out that Crystal is making things hard. As far as I understand Crystal provides no remote IP in the Request object which would make it necessary to rely on headers such as X-Forwarded-For. Those, in turn, may not be reliable. There's some discussion about the matter in #534.

@elaine-jackson
Copy link
Contributor

@firoxer It looks like one of the following needs to happen.

  1. Someone contributes a change to the crystal standard library to add a remote IP to the Request object.

  2. We create code which extends the standard library at runtime.

  3. We write our own HTTP Server instead of relying on Crystal's built in one (which would add technical debt)

I found crystal-lang/crystal#5784 and https://github.com/crystal-lang/crystal/pull/7610/files both being fairly recent and relevant to the issue at hand.

@eliasjpr
Copy link
Contributor

Based on the changes to the crystal HTTP::Server::Request we should refactor the context class to use this instead.

Who would like to take the lead on this issue? @elorest @nsuchy @firoxer

@elorest
Copy link
Member Author

elorest commented Sep 12, 2019

I could head this. Does request contain remote_ip now?

@eliasjpr
Copy link
Contributor

Yes, I believe so.

@eliasjpr
Copy link
Contributor

eliasjpr commented Aug 5, 2020

This is fixed by standard library HTTP::Request.remote_address method see https://crystal-lang.org/api/0.34.0/HTTP/Request.html#remote_address:String?-instance-method

@eliasjpr eliasjpr closed this as completed Aug 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants