Permalink
Browse files

Move remote_ip to a middleware:

  * ActionController::Base.ip_spoofing_check deprecated => config.action_dispatch.ip_spoofing_check
  * ActionController::Base.trusted_proxies deprecated => config.action_dispatch.trusted_proxies
  • Loading branch information...
1 parent 9a9caf6 commit 93422af5d5bc0285bd72cfb2fd9b59f6d64ba141 Carlhuda committed with Carl Lerche Mar 4, 2010
@@ -42,13 +42,13 @@ module ActionDispatch
end
autoload_under 'middleware' do
- autoload :BlockUntrustedIps
autoload :Callbacks
autoload :Cascade
autoload :Cookies
autoload :Flash
autoload :Head
autoload :ParamsParser
+ autoload :RemoteIp
autoload :Rescue
autoload :ShowExceptions
autoload :Static
@@ -119,36 +119,7 @@ def xml_http_request?
# delimited list in the case of multiple chained proxies; the last
# address which is not trusted is the originating IP.
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 || addr =~ ActionController::Base.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 ActionController::ActionControllerError.new <<EOM
-IP spoofing attack?!
-HTTP_CLIENT_IP=#{@env['HTTP_CLIENT_IP'].inspect}
-HTTP_X_FORWARDED_FOR=#{@env['HTTP_X_FORWARDED_FOR'].inspect}
-EOM
- end
-
- return @env['HTTP_CLIENT_IP']
- end
-
- if remote_ips
- while remote_ips.size > 1 && (TRUSTED_PROXIES =~ remote_ips.last.strip || ActionController::Base.trusted_proxies =~ remote_ips.last.strip)
- remote_ips.pop
- end
-
- return remote_ips.last.strip
- end
-
- @env['REMOTE_ADDR']
+ (@env["action_dispatch.remote_ip"] || ip).to_s
end
# Returns the lowercase name of the HTTP server software.
@@ -1,25 +0,0 @@
-module ActionDispatch
- class BlockUntrustedIps
- class SpoofAttackError < StandardError ; end
-
- def initialize(app)
- @app = app
- end
-
- def call(env)
- if @env['HTTP_X_FORWARDED_FOR'] && @env['HTTP_CLIENT_IP']
- remote_ips = @env['HTTP_X_FORWARDED_FOR'].split(',')
-
- unless remote_ips.include?(@env['HTTP_CLIENT_IP'])
- http_client_ip = @env['HTTP_CLIENT_IP'].inspect
- http_forwarded_for = @env['HTTP_X_FORWARDED_FOR'].inspect
-
- raise SpoofAttackError, "IP spoofing attack?!\n " \
- "HTTP_CLIENT_IP=#{http_client_ip}\n HTTP_X_FORWARDED_FOR=http_forwarded_for"
- end
- end
-
- @app.call(env)
- end
- end
-end
@@ -0,0 +1,51 @@
+module ActionDispatch
+ class RemoteIp
+ class IpSpoofAttackError < StandardError ; end
+
+ class RemoteIpGetter
+ def initialize(env, check_ip_spoofing, trusted_proxies)
+ @env = env
+ @check_ip_spoofing = check_ip_spoofing
+ @trusted_proxies = trusted_proxies
+ end
+
+ def remote_addrs
+ @remote_addrs ||= begin
+ list = @env['REMOTE_ADDR'] ? @env['REMOTE_ADDR'].split(/[,\s]+/) : []
+ list.reject { |addr| addr =~ @trusted_proxies }
+ end
+ end
+
+ def to_s
+ return remote_addrs.first if remote_addrs.any?
+
+ forwarded_ips = @env['HTTP_X_FORWARDED_FOR'] ? @env['HTTP_X_FORWARDED_FOR'].strip.split(/[,\s]+/) : []
+
+ if client_ip = @env['HTTP_CLIENT_IP']
+ if @check_ip_spoofing && !forwarded_ips.include?(client_ip)
+ # We don't know which came from the proxy, and which from the user
+ raise IpSpoofAttackError, "IP spoofing attack?!" \
+ "HTTP_CLIENT_IP=#{@env['HTTP_CLIENT_IP'].inspect}" \
+ "HTTP_X_FORWARDED_FOR=#{@env['HTTP_X_FORWARDED_FOR'].inspect}"
+ end
+ return client_ip
+ end
+
+ return forwarded_ips.reject { |ip| ip =~ @trusted_proxies }.last || @env["REMOTE_ADDR"]
+ end
+ end
+
+ def initialize(app, check_ip_spoofing = true, trusted_proxies = nil)
+ @app = app
+ @check_ip_spoofing = check_ip_spoofing
+ regex = '(^127\.0\.0\.1$|^(10|172\.(1[6-9]|2[0-9]|30|31)|192\.168)\.)'
+ regex << "|(#{trusted_proxies})" if trusted_proxies
+ @trusted_proxies = Regexp.new(regex, "i")
+ end
+
+ def call(env)
+ env["action_dispatch.remote_ip"] = RemoteIpGetter.new(env, @check_ip_spoofing, @trusted_proxies)
+ @app.call(env)
+ end
+ end
+end
@@ -6,6 +6,7 @@ class Railtie < Rails::Railtie
railtie_name :action_dispatch
config.action_dispatch.x_sendfile_header = "X-Sendfile"
+ config.action_dispatch.ip_spoofing_check = true
# Prepare dispatcher callbacks and run 'prepare' callbacks
initializer "action_dispatch.prepare_dispatcher" do |app|
@@ -42,7 +42,7 @@ class RequestTest < ActiveSupport::TestCase
request = stub_request 'HTTP_X_FORWARDED_FOR' => '1.1.1.1',
'HTTP_CLIENT_IP' => '2.2.2.2'
- e = assert_raise(ActionController::ActionControllerError) {
+ e = assert_raise(ActionDispatch::RemoteIp::IpSpoofAttackError) {
request.remote_ip
}
assert_match /IP spoofing attack/, e.message
@@ -54,18 +54,17 @@ class RequestTest < ActiveSupport::TestCase
# example is WAP. Since the cellular network is not IP based, it's a
# leap of faith to assume that their proxies are ever going to set the
# HTTP_CLIENT_IP/HTTP_X_FORWARDED_FOR headers properly.
- ActionController::Base.ip_spoofing_check = false
request = stub_request 'HTTP_X_FORWARDED_FOR' => '1.1.1.1',
- 'HTTP_CLIENT_IP' => '2.2.2.2'
+ 'HTTP_CLIENT_IP' => '2.2.2.2',
+ :ip_spoofing_check => false
assert_equal '2.2.2.2', request.remote_ip
- ActionController::Base.ip_spoofing_check = true
request = stub_request 'HTTP_X_FORWARDED_FOR' => '8.8.8.8, 9.9.9.9'
assert_equal '9.9.9.9', request.remote_ip
end
test "remote ip with user specified trusted proxies" do
- ActionController::Base.trusted_proxies = /^67\.205\.106\.73$/i
+ @trusted_proxies = /^67\.205\.106\.73$/i
request = stub_request 'REMOTE_ADDR' => '67.205.106.73',
'HTTP_X_FORWARDED_FOR' => '3.4.5.6'
@@ -429,6 +428,9 @@ class RequestTest < ActiveSupport::TestCase
protected
def stub_request(env = {})
+ ip_spoofing_check = env.key?(:ip_spoofing_check) ? env.delete(:ip_spoofing_check) : true
+ ip_app = ActionDispatch::RemoteIp.new(Proc.new { }, ip_spoofing_check, @trusted_proxies)
+ ip_app.call(env)
ActionDispatch::Request.new(env)
end
@@ -84,11 +84,12 @@ def default_middleware
middleware.use('::Rack::Runtime')
middleware.use('::Rails::Rack::Logger')
middleware.use('::ActionDispatch::ShowExceptions', lambda { consider_all_requests_local })
+ middleware.use("::ActionDispatch::RemoteIp", lambda { action_dispatch.ip_spoofing_check }, lambda { action_dispatch.trusted_proxies })
middleware.use('::Rack::Sendfile', lambda { action_dispatch.x_sendfile_header })
middleware.use('::ActionDispatch::Callbacks', lambda { !cache_classes })
middleware.use('::ActionDispatch::Cookies')
- middleware.use(lambda { ActionController::Base.session_store }, lambda { ActionController::Base.session_options })
- middleware.use('::ActionDispatch::Flash', :if => lambda { ActionController::Base.session_store })
+ middleware.use(lambda { action_controller.session_store }, lambda { action_controller.session_options })
+ middleware.use('::ActionDispatch::Flash', :if => lambda { action_controller.session_store })
middleware.use(lambda { metal_loader.build_middleware(metals) }, :if => lambda { metal_loader.metals.any? })
middleware.use('ActionDispatch::ParamsParser')
middleware.use('::Rack::MethodOverride')
@@ -87,6 +87,7 @@ def logger
%w(info debug warn error fatal unknown).each do |level|
class_eval <<-METHOD, __FILE__, __LINE__ + 1
def #{level}(*args, &block)
+ return unless logger
logger.#{level}(*args, &block)
end
METHOD
@@ -0,0 +1,53 @@
+require 'isolation/abstract_unit'
+
+class MiddlewareStackDefaultsTest < Test::Unit::TestCase
+ include ActiveSupport::Testing::Isolation
+
+ def setup
+ boot_rails
+ require "rails"
+ require "action_controller/railtie"
+
+ Object.const_set(:MyApplication, Class.new(Rails::Application))
+ MyApplication.class_eval do
+ config.action_controller.session = { :key => "_myapp_session", :secret => "OMG A SEKRET" * 10 }
+ end
+ end
+
+ def remote_ip(env = {})
+ remote_ip = nil
+ env = Rack::MockRequest.env_for("/").merge(env).merge('action_dispatch.show_exceptions' => false)
+
+ endpoint = Proc.new do |e|
+ remote_ip = ActionDispatch::Request.new(e).remote_ip
+ [200, {}, ["Hello"]]
+ end
+
+ out = MyApplication.middleware.build(endpoint).call(env)
+ remote_ip
+ end
+
+ test "remote_ip works" do
+ assert_equal "1.1.1.1", remote_ip("REMOTE_ADDR" => "1.1.1.1")
+ end
+
+ test "checks IP spoofing by default" do
+ assert_raises(ActionDispatch::RemoteIp::IpSpoofAttackError) do
+ remote_ip("HTTP_X_FORWARDED_FOR" => "1.1.1.1", "HTTP_CLIENT_IP" => "1.1.1.2")
+ end
+ end
+
+ test "can disable IP spoofing check" do
+ MyApplication.config.action_dispatch.ip_spoofing_check = false
+
+ assert_nothing_raised(ActionDispatch::RemoteIp::IpSpoofAttackError) do
+ assert_equal "1.1.1.2", remote_ip("HTTP_X_FORWARDED_FOR" => "1.1.1.1", "HTTP_CLIENT_IP" => "1.1.1.2")
+ end
+ end
+
+ test "the user can set trusted proxies" do
+ MyApplication.config.action_dispatch.trusted_proxies = /^4\.2\.42\.42$/
+
+ assert_equal "1.1.1.1", remote_ip("REMOTE_ADDR" => "4.2.42.42,1.1.1.1")
+ end
+end

2 comments on commit 93422af

Contributor

deepak replied Jan 12, 2011

ActionDispatch::RemoteIp takes the ip in the 'REMOTE_ADDR' header, but this can be the address of the proxy server (if it is present).
should we not parse the HTTP_CLIENT_IP and HTTP_X_FORWARDED_FOR to get the remote_ip

on remote_ip.rb#L34, there is no need to check again for REMOTE_ADDR as we would have returned REMOTE_ADDR in the beginning of the function, it is was non-empty.

return forwarded_ips.reject { |ip| ip =~ @trusted_proxies }.last || @env["REMOTE_ADDR"]

Note that i do not have any data, as to how is it implemented on the wild by all the ISP and proxy servers.

Member

indirect replied Nov 11, 2011

Huh, I just ran into the exact same issue, and had the exact same thoughts. I think I'll fix this.

Please sign in to comment.