From 7501e7324fe7c3191f8839422a5d085563c156de Mon Sep 17 00:00:00 2001 From: Matt Bostock Date: Sun, 21 Feb 2016 10:29:07 +0000 Subject: [PATCH 1/2] Fix typo `standartd` should read `standard`. --- test/spec_common_logger.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/spec_common_logger.rb b/test/spec_common_logger.rb index 3589576b8..a76219af5 100644 --- a/test/spec_common_logger.rb +++ b/test/spec_common_logger.rb @@ -36,7 +36,7 @@ log.string.must_match(/"GET \/ " 200 #{length} /) end - it "work with standartd library logger" do + it "work with standard library logger" do logdev = StringIO.new log = Logger.new(logdev) Rack::MockRequest.new(Rack::CommonLogger.new(app, log)).get("/") From 42b5af52265b9e84b20932e910e1ab02dcdcdd67 Mon Sep 17 00:00:00 2001 From: Matt Bostock Date: Sun, 21 Feb 2016 11:08:20 +0000 Subject: [PATCH 2/2] Support RFC 7239: HTTP Forwarded header [RFC 7239][] is an IETF proposed standard for a HTTP `Forwarded` request header that aims to replace non-standard `X-Forwarded-*` headers. This commit supports all relevant tokens that may be set in the Forwarded header: - proto - host - for I have prioritised the Forwarded header over the non-standard headers as I think it's preferable to use the standard header when specified. Multiple values for each token are allowed by the standard. When multiple values are supplied, I've followed these rules: - for the `proto` token, use the first value specified as it's most likely to be the protocol used by the original client - for the `host` token, use the last value specified following the convention already used by Rack for multiple `X-Forwarded-For` headers When both `X-Forwarded-For` and `Forwarded` headers are supplied and the latter includes `for` values, I've concatenated both sets of values with the `Forwarded` values coming last. I've been careful to support mixed case in the header and optional double quotes encapsulating the token value as specified in the RFC. [RFC 7239]: https://tools.ietf.org/html/rfc7239 --- lib/rack/common_logger.rb | 3 ++- lib/rack/request.rb | 20 +++++++++++++++-- lib/rack/utils.rb | 13 +++++++++++ test/spec_common_logger.rb | 14 ++++++++++++ test/spec_request.rb | 44 ++++++++++++++++++++++++++++++++++++++ test/spec_utils.rb | 37 ++++++++++++++++++++++++++++++++ 6 files changed, 128 insertions(+), 3 deletions(-) diff --git a/lib/rack/common_logger.rb b/lib/rack/common_logger.rb index 1ec8266d6..fde3668f9 100644 --- a/lib/rack/common_logger.rb +++ b/lib/rack/common_logger.rb @@ -41,9 +41,10 @@ def call(env) def log(env, status, header, began_at) now = Time.now length = extract_content_length(header) + forwarded_for = Utils::forwarded_values(env['HTTP_FORWARDED'])[:for].last msg = FORMAT % [ - env['HTTP_X_FORWARDED_FOR'] || env["REMOTE_ADDR"] || "-", + forwarded_for || env['HTTP_X_FORWARDED_FOR'] || env["REMOTE_ADDR"] || "-", env["REMOTE_USER"] || "-", now.strftime("%d/%b/%Y:%H:%M:%S %z"), env[REQUEST_METHOD], diff --git a/lib/rack/request.rb b/lib/rack/request.rb index a76f15cd3..4c416f664 100644 --- a/lib/rack/request.rb +++ b/lib/rack/request.rb @@ -134,6 +134,7 @@ module Helpers # to include the port in a generated URI. DEFAULT_PORTS = { 'http' => 80, 'https' => 443, 'coffee' => 80 } + HTTP_FORWARDED = 'HTTP_FORWARDED'.freeze HTTP_X_FORWARDED_SCHEME = 'HTTP_X_FORWARDED_SCHEME'.freeze HTTP_X_FORWARDED_PROTO = 'HTTP_X_FORWARDED_PROTO'.freeze HTTP_X_FORWARDED_HOST = 'HTTP_X_FORWARDED_HOST'.freeze @@ -203,6 +204,10 @@ def unlink?; request_method == UNLINK end def scheme if get_header(HTTPS) == 'on' 'https' + elsif get_header(HTTP_FORWARDED) + # Use first proto field set as it's most likely to be represent the + # original client: https://tools.ietf.org/html/rfc7239#section-4 + get_http_forwarder(:proto)[0] elsif get_header(HTTP_X_FORWARDED_SSL) == 'on' 'https' elsif get_header(HTTP_X_FORWARDED_SCHEME) @@ -240,7 +245,9 @@ def xhr? end def host_with_port - if forwarded = get_header(HTTP_X_FORWARDED_HOST) + if forwarded = get_http_forwarder(:host).last + forwarded + elsif forwarded = get_header(HTTP_X_FORWARDED_HOST) forwarded.split(/,\s?/).last else get_header(HTTP_HOST) || "#{get_header(SERVER_NAME) || get_header(SERVER_ADDR)}:#{get_header(SERVER_PORT)}" @@ -257,6 +264,8 @@ def port port.to_i elsif port = get_header(HTTP_X_FORWARDED_PORT) port.to_i + elsif get_http_forwarder(:proto) + DEFAULT_PORTS[scheme] elsif has_header?(HTTP_X_FORWARDED_HOST) DEFAULT_PORTS[scheme] elsif has_header?(HTTP_X_FORWARDED_PROTO) @@ -276,7 +285,9 @@ def ip return remote_addrs.first if remote_addrs.any? - forwarded_ips = split_ip_addresses(get_header('HTTP_X_FORWARDED_FOR')) + rfc7239_forwarded_ips = get_http_forwarder(:for) + x_forwarded_ips = split_ip_addresses(get_header('HTTP_X_FORWARDED_FOR')) + forwarded_ips = x_forwarded_ips.concat(rfc7239_forwarded_ips) return reject_trusted_ip_addresses(forwarded_ips).last || get_header("REMOTE_ADDR") end @@ -452,6 +463,11 @@ def parse_http_accept_header(header) end end + # Get an array of values set in the RFC 7239 `Forwarded` request header. + def get_http_forwarder(token) + Utils::forwarded_values(get_header(HTTP_FORWARDED))[token] + end + def query_parser Utils.default_query_parser end diff --git a/lib/rack/utils.rb b/lib/rack/utils.rb index d541608ae..45946ec70 100644 --- a/lib/rack/utils.rb +++ b/lib/rack/utils.rb @@ -140,6 +140,19 @@ def q_values(q_value_header) end module_function :q_values + def forwarded_values(forwarded_header) + values = Hash.new{ |k,v| k[v] = [] } + forwarded_header.to_s.split(/\s*,\s*/).each do |field| + field.split(/\s*;\s*/).each do |pair| + if /\A\s*(by|for|host|proto)\s*=\s*"?([^"]+)"?\s*\Z/i =~ pair + values[$1.downcase.to_sym] << $2 + end + end + end + values + end + module_function :forwarded_values + def best_q_match(q_value_header, available_mimes) values = q_values(q_value_header) diff --git a/test/spec_common_logger.rb b/test/spec_common_logger.rb index a76219af5..3fb38f917 100644 --- a/test/spec_common_logger.rb +++ b/test/spec_common_logger.rb @@ -58,6 +58,20 @@ res.errors.must_match(/"GET \/ " 200 - /) end + it "log - records host from X-Forwarded-For header" do + res = Rack::MockRequest.new(Rack::CommonLogger.new(app)).get("/", 'HTTP_X_FORWARDED_FOR' => '203.0.113.0') + + res.errors.wont_be :empty? + res.errors.must_match(/203\.0\.113\.0 - /) + end + + it "log - records host from RFC 7239 forwarded for header" do + res = Rack::MockRequest.new(Rack::CommonLogger.new(app)).get("/", 'HTTP_FORWARDED' => 'for=203.0.113.0') + + res.errors.wont_be :empty? + res.errors.must_match(/203\.0\.113\.0 - /) + end + def with_mock_time(t = 0) mc = class << Time; self; end mc.send :alias_method, :old_now, :now diff --git a/test/spec_request.rb b/test/spec_request.rb index 74f2fa871..232d72e00 100644 --- a/test/spec_request.rb +++ b/test/spec_request.rb @@ -120,6 +120,23 @@ class RackRequestTest < Minitest::Spec Rack::MockRequest.env_for("/", "SERVER_NAME" => "example.org", "SERVER_PORT" => "9292") req.host.must_equal "example.org" + req = make_request \ + Rack::MockRequest.env_for("/", "HTTP_HOST" => "localhost:81", "HTTP_FORWARDED" => "host=example.org:9292") + req.host.must_equal "example.org" + + # Test obfuscated identifier: https://tools.ietf.org/html/rfc7239#section-6.3 + req = make_request \ + Rack::MockRequest.env_for("/", "HTTP_HOST" => "localhost:81", "HTTP_FORWARDED" => "host=ObFuScaTeD") + req.host.must_equal "ObFuScaTeD" + + req = make_request \ + Rack::MockRequest.env_for("/", "HTTP_HOST" => "localhost:81", "HTTP_FORWARDED" => "host=example.com; host=example.org:9292") + req.host.must_equal "example.org" + + req = make_request \ + Rack::MockRequest.env_for("/", "HTTP_HOST" => "localhost:81", "HTTP_X_FORWARDED_HOST" => "example.org:9292", "HTTP_FORWARDED" => "host=example.com") + req.host.must_equal "example.com" + req = make_request \ Rack::MockRequest.env_for("/", "HTTP_HOST" => "localhost:81", "HTTP_X_FORWARDED_HOST" => "example.org:9292") req.host.must_equal "example.org" @@ -539,6 +556,18 @@ def initialize(*) request.scheme.must_equal "http" request.wont_be :ssl? + request = make_request(Rack::MockRequest.env_for("/", 'HTTP_FORWARDED' => 'proto=https')) + request.scheme.must_equal "https" + 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("/", 'HTTPS' => 'on')) request.scheme.must_equal "https" request.must_be :ssl? @@ -1186,6 +1215,21 @@ def ip_app it 'deals with proxies' do mock = Rack::MockRequest.new(Rack::Lint.new(ip_app)) + res = mock.get '/', + 'REMOTE_ADDR' => '1.2.3.4', + 'HTTP_FORWARDED' => 'for=3.4.5.6' + res.body.must_equal '1.2.3.4' + + res = mock.get '/', + 'HTTP_X_FORWARDED_FOR' => '3.4.5.6', + 'HTTP_FORWARDED' => 'for=5.6.7.8' + res.body.must_equal '5.6.7.8' + + res = mock.get '/', + 'HTTP_X_FORWARDED_FOR' => '3.4.5.6', + 'HTTP_FORWARDED' => 'for=5.6.7.8, for=7.8.9.0' + res.body.must_equal '7.8.9.0' + res = mock.get '/', 'REMOTE_ADDR' => '1.2.3.4', 'HTTP_X_FORWARDED_FOR' => '3.4.5.6' diff --git a/test/spec_utils.rb b/test/spec_utils.rb index 17a121153..5e84f941f 100644 --- a/test/spec_utils.rb +++ b/test/spec_utils.rb @@ -354,6 +354,43 @@ def initialize(*) ] end + it "parses RFC 7239 Forwarded header" do + Rack::Utils.forwarded_values('for=3.4.5.6').must_equal({ + :for => [ '3.4.5.6' ], + }) + + Rack::Utils.forwarded_values(';;;for=3.4.5.6,,').must_equal({ + :for => [ '3.4.5.6' ], + }) + + Rack::Utils.forwarded_values('for=3.4.5.6').must_equal({ + :for => [ '3.4.5.6' ], + }) + + Rack::Utils.forwarded_values('for = 3.4.5.6').must_equal({ + :for => [ '3.4.5.6' ], + }) + + Rack::Utils.forwarded_values('for="3.4.5.6"').must_equal({ + :for => [ '3.4.5.6' ], + }) + + Rack::Utils.forwarded_values('for=3.4.5.6;proto=https').must_equal({ + :for => [ '3.4.5.6' ], + :proto => [ 'https' ] + }) + + Rack::Utils.forwarded_values('for=3.4.5.6; proto=http, proto=https').must_equal({ + :for => [ '3.4.5.6' ], + :proto => [ 'http', 'https' ] + }) + + Rack::Utils.forwarded_values('for=3.4.5.6; proto=http, proto=https; for=1.2.3.4').must_equal({ + :for => [ '3.4.5.6', '1.2.3.4' ], + :proto => [ 'http', 'https' ] + }) + end + it "select best quality match" do Rack::Utils.best_q_match("text/html", %w[text/html]).must_equal "text/html"