Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support RFC 7239: HTTP Forwarded header #1017

Closed
wants to merge 2 commits into from
Closed

Support RFC 7239: HTTP Forwarded header #1017

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7501e73
Fix typo
mattbostock Feb 21, 2016
42b5af5
Support RFC 7239: HTTP Forwarded header
mattbostock Feb 21, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion lib/rack/common_logger.rb
View file
Open in desktop
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This codebase uses . for method calls and :: for constant scope resolution.


msg = FORMAT % [
env['HTTP_X_FORWARDED_FOR'] || env["REMOTE_ADDR"] || "-",
forwarded_for || env['HTTP_X_FORWARDED_FOR'] || env["REMOTE_ADDR"] || "-",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems this is kinda-implementing Rack::Request#ip, maybe nicer to reuse that.

env["REMOTE_USER"] || "-",
now.strftime("%d/%b/%Y:%H:%M:%S %z"),
env[REQUEST_METHOD],
Expand Down
20 changes: 18 additions & 2 deletions lib/rack/request.rb
View file
Open in desktop
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious that we use the first forward for protocol, the last forward for host, and the first trusted forward for IP.

else
get_header(HTTP_HOST) || "#{get_header(SERVER_NAME) || get_header(SERVER_ADDR)}:#{get_header(SERVER_PORT)}"
Expand All @@ -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)
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down
13 changes: 13 additions & 0 deletions lib/rack/utils.rb
View file
Open in desktop
Expand Up @@ -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)

Expand Down
16 changes: 15 additions & 1 deletion test/spec_common_logger.rb
View file
Open in desktop
Expand Up @@ -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("/")
Expand All @@ -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
Expand Down
44 changes: 44 additions & 0 deletions test/spec_request.rb
View file
Open in desktop
Expand Up @@ -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"
Expand Down Expand Up @@ -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?
Expand Down Expand Up @@ -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',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test coverage demonstrating that trusted proxy detection behaves with Forwarded, too?

'HTTP_X_FORWARDED_FOR' => '3.4.5.6'
Expand Down
37 changes: 37 additions & 0 deletions test/spec_utils.rb
View file
Open in desktop
Expand Up @@ -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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we're covering https://tools.ietf.org/html/rfc7239#section-7.1 👍

Can't do much about multiple header testing since that falls on the Rack handler to coalesce.

it "select best quality match" do
Rack::Utils.best_q_match("text/html", %w[text/html]).must_equal "text/html"

Expand Down