diff --git a/CHANGELOG.md b/CHANGELOG.md index e54e297e0..9c224af19 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,7 +27,7 @@ All notable changes to this project will be documented in this file. For info on - `Rack::Headers` added to support lower-case header keys. ([@jeremyevans](https://github.com/jeremyevans)) - `Rack::RewindableInput` supports size. ([@ahorek](https://github.com/ahorek)) - `Rack::RewindableInput::Middleware` added for making `rack.input` rewindable. ([@jeremyevans](https://github.com/jeremyevans)) -- Rack::Session::Pool now accepts `:allow_fallback` option to disable fallback to public id. ([#1431](https://github.com/rack/rack/issues/1431), [@jeremyevans](https://github.com/jeremyevans)) +- The RFC 7239 Forwarded header is now supported and considered by default when looking for information on forwarding, falling back to the the X-Forwarded-* headers. `Rack::Request.forwarded_priority` accessor has been added for configuring the priority of which header to check. ([#1423](https://github.com/rack/rack/issues/1423), [@jeremyevans](https://github.com/jeremyevans)) ### Changed @@ -47,6 +47,8 @@ All notable changes to this project will be documented in this file. For info on - Explicitly deprecate `Rack::File` which was an alias for `Rack::Files`. ([#1811](https://github.com/rack/rack/pull/1720), [@ioquatix](https://github.com/ioquatix)). - Moved `Rack::Session` into [separate gem](https://github.com/rack/rack-session). ([#1805](https://github.com/rack/rack/pull/1805), [@ioquatix](https://github.com/ioquatix)) - rackup -D option to daemonizes no longer changes the working directory to the root. ([#1813](https://github.com/rack/rack/pull/1813), [@jeremyevans](https://github.com/jeremyevans)) +- The X-Forwarded-Proto header is now considered before the X-Forwarded-Scheme header for determining the forwarded protocol. `Rack::Request.x_forwarded_proto_priority` accessor has been added for configuring the priority of which header to check. ([#1809](https://github.com/rack/rack/issues/1809), [@jeremyevans](https://github.com/jeremyevans)) +- `Rack::Request.forwarded_authority` (and methods that call it, such as `host`) now returns the last authority in the forwarded header, instead of the first, as earlier forwarded authorities can be forged by clients. This restores the Rack 2.1 behavior. ([#1829](https://github.com/rack/rack/issues/1809), [@jeremyevans](https://github.com/jeremyevans)) ### Fixed diff --git a/lib/rack/request.rb b/lib/rack/request.rb index abb41eebb..dddb71b81 100644 --- a/lib/rack/request.rb +++ b/lib/rack/request.rb @@ -16,8 +16,33 @@ module Rack class Request class << self attr_accessor :ip_filter + + # The priority when checking forwarded headers. The default + # is [:forwarded, :x_forwarded], which means, check the + # +Forwarded+ header first, followed by the appropriate + # X-Forwarded-* header. You can revert the priority by + # reversing the priority, or remove checking of either + # or both headers by removing elements from the array. + # + # This should be set as appropriate in your environment + # based on what reverse proxies are in use. If you are not + # using reverse proxies, you should probably use an empty + # array. + attr_accessor :forwarded_priority + + # The priority when checking either the X-Forwarded-Proto + # or X-Forwarded-Scheme header for the forwarded protocol. + # The default is [:proto, :scheme], to try the + # X-Forwarded-Proto header before the + # X-Forwarded-Scheme header. Rack 2 had behavior + # similar to [:scheme, :proto]. You can remove either or + # both of the entries in array to ignore that respective header. + attr_accessor :x_forwarded_proto_priority end + @forwarded_priority = [:forwarded, :x_forwarded] + @x_forwarded_proto_priority = [:proto, :scheme] + valid_ipv4_octet = /\.(25[0-5]|2[0-4][0-9]|[01]?[0-9]?[0-9])/ trusted_proxies = Regexp.union( @@ -346,37 +371,60 @@ def port end def forwarded_for - if forwarded_for = get_http_forwarded(:for) - forwarded_for.map! do |authority| - split_authority(authority)[1] + forwarded_priority.each do |type| + case type + when :forwarded + if forwarded_for = get_http_forwarded(:for) + return(forwarded_for.map! do |authority| + split_authority(authority)[1] + end) + end + when :x_forwarded + if value = get_header(HTTP_X_FORWARDED_FOR) + return(split_header(value).map do |authority| + split_authority(wrap_ipv6(authority))[1] + end) + end end end - if value = get_header(HTTP_X_FORWARDED_FOR) - x_forwarded_for = split_header(value).map do |authority| - split_authority(wrap_ipv6(authority))[1] - end - end - - forwarded_for || x_forwarded_for + nil end def forwarded_port - if forwarded = get_http_forwarded(:for) - forwarded.map do |authority| - split_authority(authority)[2] - end.compact! - elsif value = get_header(HTTP_X_FORWARDED_PORT) - split_header(value).map(&:to_i) + forwarded_priority.each do |type| + case type + when :forwarded + if forwarded = get_http_forwarded(:for) + return(forwarded.map do |authority| + split_authority(authority)[2] + end.compact) + end + when :x_forwarded + if value = get_header(HTTP_X_FORWARDED_PORT) + return split_header(value).map(&:to_i) + end + end end + + nil end def forwarded_authority - if forwarded = get_http_forwarded(:host) - forwarded.last - elsif value = get_header(HTTP_X_FORWARDED_HOST) - wrap_ipv6(split_header(value).last) + forwarded_priority.each do |type| + case type + when :forwarded + if forwarded = get_http_forwarded(:host) + return forwarded.last + end + when :x_forwarded + if value = get_header(HTTP_X_FORWARDED_HOST) + return wrap_ipv6(split_header(value).last) + end + end end + + nil end def ssl? @@ -611,8 +659,7 @@ def parse_http_accept_header(header) # Get an array of values set in the RFC 7239 `Forwarded` request header. def get_http_forwarded(token) - values = Utils.forwarded_values(get_header(HTTP_FORWARDED)) - values[token] if values + Utils.forwarded_values(get_header(HTTP_FORWARDED))&.[](token) end def query_parser @@ -660,11 +707,33 @@ def reject_trusted_ip_addresses(ip_addresses) ip_addresses.reject { |ip| trusted_proxy?(ip) } end + FORWARDED_SCHEME_HEADERS = { + proto: HTTP_X_FORWARDED_PROTO, + scheme: HTTP_X_FORWARDED_SCHEME + }.freeze + private_constant :FORWARDED_SCHEME_HEADERS def forwarded_scheme - forwarded_proto = get_http_forwarded(:proto) - (forwarded_proto && allowed_scheme(forwarded_proto.first)) || - allowed_scheme(get_header(HTTP_X_FORWARDED_SCHEME)) || - allowed_scheme(extract_proto_header(get_header(HTTP_X_FORWARDED_PROTO))) + forwarded_priority.each do |type| + case type + when :forwarded + if (forwarded_proto = get_http_forwarded(:proto)) && + (scheme = allowed_scheme(forwarded_proto.last)) + return scheme + end + when :x_forwarded + x_forwarded_proto_priority.each do |x_type| + if header = FORWARDED_SCHEME_HEADERS[x_type] + split_header(get_header(header)).reverse_each do |scheme| + if allowed_scheme(scheme) + return scheme + end + end + end + end + end + end + + nil end def allowed_scheme(header) @@ -680,6 +749,14 @@ def extract_proto_header(header) end end end + + def forwarded_priority + Request.forwarded_priority + end + + def x_forwarded_proto_priority + Request.x_forwarded_proto_priority + end end include Env diff --git a/test/spec_request.rb b/test/spec_request.rb index 266b52a63..4cf9fca24 100644 --- a/test/spec_request.rb +++ b/test/spec_request.rb @@ -262,6 +262,183 @@ class RackRequestTest < Minitest::Spec req.port.must_equal 443 end + it "have forwarded_* methods respect forwarded_priority" do + begin + default_priority = Rack::Request.forwarded_priority + default_proto_priority = Rack::Request.x_forwarded_proto_priority + + def self.req(headers) + req = make_request Rack::MockRequest.env_for("/", headers) + req.singleton_class.send(:public, :forwarded_scheme) + req + end + + req("HTTP_FORWARDED"=>"for=1.2.3.4", + "HTTP_X_FORWARDED_FOR" => "2.3.4.5"). + forwarded_for.must_equal ['1.2.3.4'] + + req("HTTP_FORWARDED"=>"for=1.2.3.4:1234", + "HTTP_X_FORWARDED_PORT" => "2345"). + forwarded_port.must_equal [1234] + + req("HTTP_FORWARDED"=>"for=1.2.3.4", + "HTTP_X_FORWARDED_PORT" => "2345"). + forwarded_port.must_equal [] + + req("HTTP_FORWARDED"=>"host=1.2.3.4, host=3.4.5.6", + "HTTP_X_FORWARDED_HOST" => "2.3.4.5,4.5.6.7"). + forwarded_authority.must_equal '3.4.5.6' + + req("HTTP_X_FORWARDED_PROTO" => "ws", + "HTTP_X_FORWARDED_SCHEME" => "http"). + forwarded_scheme.must_equal "ws" + + req("HTTP_X_FORWARDED_SCHEME" => "http"). + forwarded_scheme.must_equal "http" + + Rack::Request.forwarded_priority = [:x_forwarded, :forwarded] + + req("HTTP_FORWARDED"=>"for=1.2.3.4", + "HTTP_X_FORWARDED_FOR" => "2.3.4.5"). + forwarded_for.must_equal ['2.3.4.5'] + + req("HTTP_FORWARDED"=>"for=1.2.3.4", + "HTTP_X_FORWARDED_PORT" => "2345"). + forwarded_port.must_equal [2345] + + req("HTTP_FORWARDED"=>"host=1.2.3.4, host=3.4.5.6", + "HTTP_X_FORWARDED_HOST" => "2.3.4.5,4.5.6.7"). + forwarded_authority.must_equal '4.5.6.7' + + req("HTTP_FORWARDED"=>"proto=https", + "HTTP_X_FORWARDED_PROTO" => "ws", + "HTTP_X_FORWARDED_SCHEME" => "http"). + forwarded_scheme.must_equal "ws" + + req("HTTP_FORWARDED"=>"proto=https", + "HTTP_X_FORWARDED_SCHEME" => "http"). + forwarded_scheme.must_equal "http" + + req("HTTP_FORWARDED"=>"proto=https"). + forwarded_scheme.must_equal "https" + + Rack::Request.x_forwarded_proto_priority = [:scheme, :proto] + + req("HTTP_FORWARDED"=>"proto=https", + "HTTP_X_FORWARDED_PROTO" => "ws", + "HTTP_X_FORWARDED_SCHEME" => "http"). + forwarded_scheme.must_equal "http" + + req("HTTP_FORWARDED"=>"proto=https", + "HTTP_X_FORWARDED_PROTO" => "ws"). + forwarded_scheme.must_equal "ws" + + req("HTTP_FORWARDED"=>"proto=https"). + forwarded_scheme.must_equal "https" + + Rack::Request.forwarded_priority = [:x_forwarded] + + req("HTTP_FORWARDED"=>"proto=https", + "HTTP_X_FORWARDED_PROTO" => "ws", + "HTTP_X_FORWARDED_SCHEME" => "http"). + forwarded_scheme.must_equal "http" + + req("HTTP_FORWARDED"=>"proto=https", + "HTTP_X_FORWARDED_PROTO" => "ws"). + forwarded_scheme.must_equal "ws" + + req("HTTP_FORWARDED"=>"proto=https"). + forwarded_scheme.must_be_nil + + Rack::Request.x_forwarded_proto_priority = [:scheme] + + req("HTTP_FORWARDED"=>"proto=https", + "HTTP_X_FORWARDED_PROTO" => "ws", + "HTTP_X_FORWARDED_SCHEME" => "http"). + forwarded_scheme.must_equal "http" + + req("HTTP_FORWARDED"=>"proto=https", + "HTTP_X_FORWARDED_PROTO" => "ws"). + forwarded_scheme.must_be_nil + + req("HTTP_FORWARDED"=>"proto=https"). + forwarded_scheme.must_be_nil + + Rack::Request.x_forwarded_proto_priority = [:proto] + + req("HTTP_FORWARDED"=>"proto=https", + "HTTP_X_FORWARDED_PROTO" => "ws", + "HTTP_X_FORWARDED_SCHEME" => "http"). + forwarded_scheme.must_equal "ws" + + req("HTTP_FORWARDED"=>"proto=https", + "HTTP_X_FORWARDED_SCHEME" => "http"). + forwarded_scheme.must_be_nil + + req("HTTP_FORWARDED"=>"proto=https"). + forwarded_scheme.must_be_nil + + Rack::Request.x_forwarded_proto_priority = [] + + req("HTTP_FORWARDED"=>"proto=https", + "HTTP_X_FORWARDED_PROTO" => "ws", + "HTTP_X_FORWARDED_SCHEME" => "http"). + forwarded_scheme.must_be_nil + + req("HTTP_FORWARDED"=>"proto=https", + "HTTP_X_FORWARDED_SCHEME" => "http"). + forwarded_scheme.must_be_nil + + req("HTTP_FORWARDED"=>"proto=https"). + forwarded_scheme.must_be_nil + + Rack::Request.x_forwarded_proto_priority = default_proto_priority + Rack::Request.forwarded_priority = [:forwarded] + + req("HTTP_FORWARDED"=>"proto=https", + "HTTP_X_FORWARDED_PROTO" => "ws", + "HTTP_X_FORWARDED_SCHEME" => "http"). + forwarded_scheme.must_equal 'https' + + req("HTTP_X_FORWARDED_PROTO" => "ws", + "HTTP_X_FORWARDED_SCHEME" => "http"). + forwarded_scheme.must_be_nil + + req("HTTP_X_FORWARDED_PROTO" => "ws"). + forwarded_scheme.must_be_nil + + Rack::Request.forwarded_priority = [] + + req("HTTP_FORWARDED"=>"for=1.2.3.4", + "HTTP_X_FORWARDED_FOR" => "2.3.4.5"). + forwarded_for.must_be_nil + + req("HTTP_FORWARDED"=>"for=1.2.3.4", + "HTTP_X_FORWARDED_PORT" => "2345"). + forwarded_port.must_be_nil + + req("HTTP_FORWARDED"=>"host=1.2.3.4, host=3.4.5.6", + "HTTP_X_FORWARDED_HOST" => "2.3.4.5,4.5.6.7"). + forwarded_authority.must_be_nil + + req("HTTP_FORWARDED"=>"proto=https", + "HTTP_X_FORWARDED_PROTO" => "ws", + "HTTP_X_FORWARDED_SCHEME" => "http"). + forwarded_scheme.must_be_nil + + req("HTTP_FORWARDED"=>"proto=https", + "HTTP_X_FORWARDED_SCHEME" => "http"). + forwarded_scheme.must_be_nil + + req("HTTP_FORWARDED"=>"proto=https"). + forwarded_scheme.must_be_nil + + ensure + Rack::Request.forwarded_priority = default_priority + Rack::Request.x_forwarded_proto_priority = default_proto_priority + end + end + it "figure out the correct host with port" do req = make_request \ Rack::MockRequest.env_for("/", "HTTP_HOST" => "www2.example.org") @@ -661,13 +838,13 @@ def initialize(*) request.must_be :ssl? request = make_request(Rack::MockRequest.env_for("/", 'HTTP_FORWARDED' => 'proto=https, proto=http')) - request.scheme.must_equal "https" - request.must_be :ssl? - - request = make_request(Rack::MockRequest.env_for("/", 'HTTP_FORWARDED' => 'proto=http, proto=https')) request.scheme.must_equal "http" request.wont_be :ssl? + request = make_request(Rack::MockRequest.env_for("/", 'HTTP_FORWARDED' => 'proto=http, proto=https')) + request.scheme.must_equal "https" + request.must_be :ssl? + request = make_request(Rack::MockRequest.env_for("/", 'HTTPS' => 'on')) request.scheme.must_equal "https" request.must_be :ssl? @@ -705,8 +882,8 @@ def initialize(*) request.must_be :ssl? request = make_request(Rack::MockRequest.env_for("/", 'HTTP_X_FORWARDED_PROTO' => 'https, http, http')) - request.scheme.must_equal "https" - request.must_be :ssl? + request.scheme.must_equal "http" + request.wont_be :ssl? request = make_request(Rack::MockRequest.env_for("/", 'HTTP_X_FORWARDED_PROTO' => 'wss')) request.scheme.must_equal "wss"