Trust X-Forwarded-For from the right to the left

README.md

@@ -1,4 +1,5 @@
 # CloudflareRails [![Gem Version](https://badge.fury.io/rb/cloudflare-rails.svg)](https://badge.fury.io/rb/cloudflare-rails)
+
 This gem correctly configures Rails for [CloudFlare](https://www.cloudflare.com) so that `request.remote_ip` / `request.ip` both work correctly. It also exposes a `#cloudflare?` method on `Rack::Request`.
 
 ## Rails Compatibility
@@ -38,39 +39,48 @@ Using Cloudflare means it's hard to identify the IP address of incoming requests
 `cloudflare-rails` mitigates this attack by checking that the originating ip address of any incoming connection is from one of Cloudflare's ip address ranges. If so, the incoming `X-Forwarded-For` header is trusted and used as the ip address provided to `rack` and `rails` (via `request.ip` and `request.remote_ip`). If the incoming connection does not originate from a Cloudflare server then the `X-Forwarded-For` header is ignored and the actual remote ip address is used.
 
 ## How it works
+
 This code fetches and caches CloudFlare's current [IPv4](https://www.cloudflare.com/ips-v4) and [IPv6](https://www.cloudflare.com/ips-v6) lists. It then patches `Rack::Request::Helpers` and `ActionDispatch::RemoteIP` to treat these addresses as trusted proxies. The `X-Forwarded-For` header will then be trusted only from those ip addresses.
 
 ### Why not use `config.action_dispatch.trusted_proxies` or `Rack::Request.ip_filter?`
+
 By default Rails includes the [ActionDispatch::RemoteIp](https://api.rubyonrails.org/classes/ActionDispatch/RemoteIp.html) middleware. This middleware uses a default list of [trusted proxies](https://github.com/rails/rails/blob/6b93fff8af32ef5e91f4ec3cfffb081d0553faf0/actionpack/lib/action_dispatch/middleware/remote_ip.rb#L36C5-L42). Any values from `config.action_dispatch.trusted_proxies` are appended to this list. If you were to set `config.action_dispatch.trusted_proxies` to the current list of Cloudflare IP addresses `request.remote_ip` would work correctly.
 
 Unfortunately this does not fix `request.ip`. This method comes from the [Rack::Request](https://github.com/rack/rack/blob/main/lib/rack/request.rb) middleware. It has a separate implementation of [trusted proxies](https://github.com/rack/rack/blob/main/lib/rack/request.rb#L48-L56) and [ip filtering](https://github.com/rack/rack/blob/main/lib/rack/request.rb#L58C1-L59C1). The only way to use a different implementation is to set `Rack::Request.ip_filter` which expects a callable value. Providing a new one will override the old one so you'd lose the default values (all of which should be there). Those values aren't exported anywhere so your callable would now have to maintain _that_ list on top of the Cloudflare IPs.
 
 These issues are why this gem patches both `Rack::Request::Helpers` and `ActionDispatch::RemoteIP` rather than using the built-in configuration methods.
 
 ## Prerequisites
+
 You must have a [`cache_store`](https://guides.rubyonrails.org/caching_with_rails.html#configuration) configured in your `rails` application.
 
 ## Usage
+
 You can configure the HTTP `timeout` and `expires_in` cache parameters inside of your `rails` config:
+
 ```ruby
 config.cloudflare.expires_in = 12.hours # default value
 config.cloudflare.timeout = 5.seconds # default value
 ```
 
 ## Blocking non-Cloudflare traffic
-You can use the `#cloudfront?` method from this gem to block all non-Cloudflare traffic to your application. Here's an example of doing this with [`Rack::Attack`](https://github.com/rack/rack-attack):
+
+You can use the `#cloudflare?` method from this gem to block all non-Cloudflare traffic to your application. Here's an example of doing this with [`Rack::Attack`](https://github.com/rack/rack-attack):
+
 ```ruby
   Rack::Attack.blocklist('CloudFlare WAF bypass') do |req|
     !req.cloudflare?
   end
 ```
-Note that the request may optionally pass through additional trusted proxies, so it will return true for any of these scenarios:
 
- * `REMOTE_ADDR: CloudFlare`
- * `REMOTE_ADDR: trusted_proxy`, `X_HTTP_FORWARDED_FOR: CloudFlare`
- * `REMOTE_ADDR: trusted_proxy`, `X_HTTP_FORWARDED_FOR: trusted_proxy2,CloudFlare,...`
+Note that the request may optionally pass through additional trusted proxies, so it will return `true` for any of these scenarios:
+
+-   `REMOTE_ADDR: CloudFlare`
+-   `REMOTE_ADDR: trusted_proxy`, `X_HTTP_FORWARDED_FOR: CloudFlare`
+-   `REMOTE_ADDR: trusted_proxy`, `X_HTTP_FORWARDED_FOR: CloudFlare,trusted_proxy2`
+-   `REMOTE_ADDR: trusted_proxy`, `X_HTTP_FORWARDED_FOR: untrusted,CloudFlare`
 
-but it will return false if CloudFlare comes after the trusted prefix of `X-Forwarded-For`.
+but it will return `false` if CloudFlare comes to the left of an untrusted IP in `X-Forwarded-For`.
 
 ## Alternatives
 

lib/cloudflare_rails/check_trusted_proxies.rb

@@ -20,7 +20,7 @@ def cloudflare?
       forwarded_for = self.forwarded_for || []
 
       # Select only the trusted prefix of REMOTE_ADDR + X_HTTP_FORWARDED_FOR
-      trusted_proxies = (remote_addresses + forwarded_for).take_while do |ip|
+      trusted_proxies = (remote_addresses + forwarded_for.reverse).take_while do |ip|
         trusted_proxy?(ip)
       end
 

spec/cloudflare/rails_spec.rb

@@ -122,9 +122,14 @@
                                      'HTTP_X_FORWARDED_FOR' => '10.2.2.2,197.234.240.1')).to be_cloudflare
           end
 
-          it 'returns true if the request originated from CloudFlare via one trusted proxy and one untrusted upstream IP' do
+          it 'returns true if the right-most addresses in the forwarding chain are trusted proxies and include CloudFlare' do
             expect(Rack::Request.new('REMOTE_ADDR' => '10.1.1.1',
-                                     'HTTP_X_FORWARDED_FOR' => '197.234.240.1,1.2.3.4')).to be_cloudflare
+                                     'HTTP_X_FORWARDED_FOR' => '1.2.3.4,10.2.2.2,197.234.240.1')).to be_cloudflare
+          end
+
+          it 'returns false if the request went through an untrusted IP address after Cloudflare' do
+            expect(Rack::Request.new('REMOTE_ADDR' => '10.1.1.1',
+                                     'HTTP_X_FORWARDED_FOR' => '197.234.240.1,1.2.3.4')).not_to be_cloudflare
           end
 
           it 'returns false if the request did not originate from CloudFlare' do
@@ -139,11 +144,6 @@
           it 'returns false if the request has a trusted REMOTE_ADDR but did not originate from CloudFlare' do
             expect(Rack::Request.new('REMOTE_ADDR' => '10.1.1.1', 'HTTP_X_FORWARDED_FOR' => '1.2.3.4')).not_to be_cloudflare
           end
-
-          it 'returns false if the request has a trusted REMOTE_ADDR and an untrusted proxy before CloudFlare' do
-            expect(Rack::Request.new('REMOTE_ADDR' => '10.1.1.1',
-                                     'HTTP_X_FORWARDED_FOR' => '1.2.3.4,197.234.240.1')).not_to be_cloudflare
-          end
         end
       end