Should we have a remote_ip helper method in our Actions?

[jwoertink]

In Rails, you have access to the request.remote_ip helper method which does this:

# File actionpack/lib/action_controller/request.rb, line 221
    def remote_ip
      remote_addr_list = @env['REMOTE_ADDR'] && @env['REMOTE_ADDR'].scan(/[^,\s]+/)
      unless remote_addr_list.blank?
        not_trusted_addrs = remote_addr_list.reject {|addr| addr =~ TRUSTED_PROXIES}
        return not_trusted_addrs.first unless not_trusted_addrs.empty?
      end
      remote_ips = @env['HTTP_X_FORWARDED_FOR'] && @env['HTTP_X_FORWARDED_FOR'].split(',')
      if @env.include? 'HTTP_CLIENT_IP'
        if ActionController::Base.ip_spoofing_check && remote_ips && !remote_ips.include?(@env['HTTP_CLIENT_IP'])
          # We don't know which came from the proxy, and which from the user
          raise ActionControllerError.new("IP spoofing attack?!\nHTTP_CLIENT_IP=\#{@env['HTTP_CLIENT_IP'].inspect}\nHTTP_X_FORWARDED_FOR=\#{@env['HTTP_X_FORWARDED_FOR'].inspect}\n")
        end
        return @env['HTTP_CLIENT_IP']
      end
      if remote_ips
        while remote_ips.size > 1 && TRUSTED_PROXIES =~ remote_ips.last.strip
          remote_ips.pop
        end
        return remote_ips.last.strip
      end
      @env['REMOTE_ADDR']
    end

with spec

I didn't see anything in crystal that does this, but I did fine remote_address which doesn't seem to help much.

In one of my apps, I started by doing this:

  private def remote_ip : String
    request.headers["X-Real-IP"]? ||
      request.headers["HTTP_X_FORWARDED_FOR"]? ||
      request.headers["X_FORWARDED_FOR"]? ||
      request.headers["REMOTE_ADDR"]? ||
      request.headers["HTTP_CLIENT_IP"]? ||
      request.headers["HTTP_X_FORWARDED"]? ||
      request.headers["HTTP_X_CLUSTER_CLIENT_IP"]? ||
      "127.0.0.1"
  end

but realized that X-Real-IP gives me the wrong IP, and in my case, what I need is the HTTP_X_FORWARDED_FOR split by comma, and the first returned. It looks like the rails version of this would actually have returned the correct one for me. Also note that this method catches IP Spoofing attacks (which my rails apps throw all the time).

I'd be curious to hear everyone else's thoughts on if this is the type of logic that would be a per app basis, or something that would be beneficial to have baked in.

[jwoertink]

also, 1000 issues! 🎉 🥳

[paulcsmith]

@jwoertink Woo hoo! And yes we should have this, but I believe it is in Crystal now: crystal-lang/crystal#7610

There was a long-standing issue and they fixed it. So I think we can get it via request.remote_address in actions. We just need to test that it works as expected and maybe add some guides

[wout]

We just started two new Lucky projects this week and at some point we will need this in both apps.

[jwoertink]

Ok, I'm deploying checking this request.remote_address to see what it actually returns. Looks like this value is nilable, so you'd need a fallback or something if you're tracking an IP, but I'll report back on what my logs show.

[jwoertink]

Alright, so this may be specific to me, but my app is sitting behind CloudFront. The request.remote_address returns the same value for every user, and that value changes each time we deploy. The X-Forwarded-For header seems to have the actual IP as well as the same IP passed from the remote_address...

My guess is maybe this remote_address is giving me the IP of the load balancer that passes the request back to the app. So it's possible that remote_addess would work in a normal basic setting. Maybe we default to that with some configuration or something?

[paulcsmith]

Yeah I think we should document that you either use remote_address or the X-Forwarded-For.

Or maybe we add Lucky.remote_ip that takes an HTTP::Request and uses either X-Forwarded-For or remote_address if the header isn't prsent. But I'd lean toward not adding the method since this is dependent on the setup of whoever is using it. Probably just need to be documented in the HTTP guides

Thoughts?