Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

support for trusted proxies and IPv6 for Request#ip

Signed-off-by: Konstantin Haase <konstantin.mailinglists@googlemail.com>
  • Loading branch information...
commit 250cde152b186e43dde264cf16808abfd459d394 1 parent b25cf63
@martoche martoche authored rkh committed
Showing with 66 additions and 16 deletions.
  1. +17 −4 lib/rack/request.rb
  2. +49 −12 test/spec_request.rb
View
21 lib/rack/request.rb
@@ -306,11 +306,24 @@ def accept_encoding
end
def ip
- if addr = @env['HTTP_X_FORWARDED_FOR']
- (addr.split(',').grep(/\d\./).first || @env['REMOTE_ADDR']).to_s.strip
- else
- @env['REMOTE_ADDR']
+ # Copied from https://github.com/rails/rails/blob/master/actionpack/lib/
+ # action_dispatch/middleware/remote_ip.rb
+ trusted_proxies = /(^127\.0\.0\.1$|^(10|172\.(1[6-9]|2[0-9]|30|31)|192\.168)\.)/i
+
+ remote_addrs = @env['REMOTE_ADDR'] ? @env['REMOTE_ADDR'].split(/[,\s]+/) : []
+ remote_addrs.reject! { |addr| addr =~ trusted_proxies }
+
+ 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 forwarded_ips doesn't include the client_ip, it might be an
+ # ip spoofing attempt, so we ignore HTTP_CLIENT_IP
+ return client_ip if forwarded_ips.include?(client_ip)
end
+
+ return forwarded_ips.reject { |ip| ip =~ trusted_proxies }.last || @env["REMOTE_ADDR"]
end
protected
View
61 test/spec_request.rb
@@ -770,26 +770,63 @@
}
mock = Rack::MockRequest.new(Rack::Lint.new(app))
- res = mock.get '/', 'REMOTE_ADDR' => '123.123.123.123'
- res.body.should.equal '123.123.123.123'
- res = mock.get '/',
- 'REMOTE_ADDR' => '123.123.123.123',
- 'HTTP_X_FORWARDED_FOR' => '234.234.234.234'
+ res = mock.get '/', 'REMOTE_ADDR' => '1.2.3.4'
+ res.body.should.equal '1.2.3.4'
+
+ res = mock.get '/', 'REMOTE_ADDR' => 'fe80::202:b3ff:fe1e:8329'
+ res.body.should.equal 'fe80::202:b3ff:fe1e:8329'
- res.body.should.equal '234.234.234.234'
+ res = mock.get '/', 'REMOTE_ADDR' => '1.2.3.4,3.4.5.6'
+ res.body.should.equal '1.2.3.4'
+
+ res = mock.get '/',
+ 'REMOTE_ADDR' => '1.2.3.4',
+ 'HTTP_X_FORWARDED_FOR' => '3.4.5.6'
+ res.body.should.equal '1.2.3.4'
res = mock.get '/',
- 'REMOTE_ADDR' => '123.123.123.123',
- 'HTTP_X_FORWARDED_FOR' => '234.234.234.234,212.212.212.212'
+ 'REMOTE_ADDR' => '127.0.0.1',
+ 'HTTP_X_FORWARDED_FOR' => '3.4.5.6'
+ res.body.should.equal '3.4.5.6'
+
+ res = mock.get '/', 'HTTP_X_FORWARDED_FOR' => 'unknown,3.4.5.6'
+ res.body.should.equal '3.4.5.6'
+
+ res = mock.get '/', 'HTTP_X_FORWARDED_FOR' => '192.168.0.1,3.4.5.6'
+ res.body.should.equal '3.4.5.6'
+
+ res = mock.get '/', 'HTTP_X_FORWARDED_FOR' => '10.0.0.1,3.4.5.6'
+ res.body.should.equal '3.4.5.6'
- res.body.should.equal '234.234.234.234'
+ res = mock.get '/', 'HTTP_X_FORWARDED_FOR' => '10.0.0.1, 10.0.0.1, 3.4.5.6'
+ res.body.should.equal '3.4.5.6'
+
+ res = mock.get '/', 'HTTP_X_FORWARDED_FOR' => '127.0.0.1, 3.4.5.6'
+ res.body.should.equal '3.4.5.6'
+
+ res = mock.get '/', 'HTTP_X_FORWARDED_FOR' => 'unknown,192.168.0.1'
+ res.body.should.equal 'unknown'
+
+ res = mock.get '/', 'HTTP_X_FORWARDED_FOR' => '9.9.9.9, 3.4.5.6, 10.0.0.1, 172.31.4.4'
+ res.body.should.equal '3.4.5.6'
res = mock.get '/',
- 'REMOTE_ADDR' => '123.123.123.123',
- 'HTTP_X_FORWARDED_FOR' => 'unknown,234.234.234.234,212.212.212.212'
+ 'HTTP_X_FORWARDED_FOR' => '1.1.1.1, 127.0.0.1',
+ 'HTTP_CLIENT_IP' => '1.1.1.1'
+ res.body.should.equal '1.1.1.1'
+
+ # Spoofing attempt
+ res = mock.get '/',
+ 'HTTP_X_FORWARDED_FOR' => '1.1.1.1',
+ 'HTTP_CLIENT_IP' => '2.2.2.2'
+ res.body.should.equal '1.1.1.1'
+
+ res = mock.get '/', 'HTTP_X_FORWARDED_FOR' => '8.8.8.8, 9.9.9.9'
+ res.body.should.equal '9.9.9.9'
- res.body.should.equal '234.234.234.234'
+ res = mock.get '/', 'HTTP_X_FORWARDED_FOR' => '8.8.8.8, fe80::202:b3ff:fe1e:8329'
+ res.body.should.equal 'fe80::202:b3ff:fe1e:8329'
end
class MyRequest < Rack::Request

7 comments on commit 250cde1

@jnunemaker

Any particular reason for the change from HTTP_X_FORWARDED_FOR being checked first to now REMOTE_ADDR being checked first? Just got burned by this as our load balancer reports X-Forwarded-For correctly, but Remote-Addr is the server ip, not the original client ip.

@rkh
Owner

What load balancer are you using?

@jnunemaker

Haproxy. Pretty sure that forwarded for is the best option there.

@rkh
Owner

Agreed. I just wanted to know, because if we change it, a discussion might come up again.

@jnunemaker
@martoche

When I submitted the patch, the idea was to match actionpack's behaviour which checked REMOTE_ADDR first. But it looks like actionpack has changed and HTTP_X_FORWARDED_FOR is now checked first.

@fredv

Is this going to be changed again, or not - or is there another active discussion on this? Has using the HTTP_X_FORWARDED_FOR header for forwarding the client ip at the proxy major disadvantages? Letting the proxy overwrite REMOTE_ADDR seems to me a lot messier.

For the last years we've always been using HTTP_X_FORWARDED_FOR in a nginx + passenger/unicorn setup with hardware lbs as well as haproxy.

Please sign in to comment.
Something went wrong with that request. Please try again.