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

Respect X-Forwarded-Proto header #87

Closed
wants to merge 4 commits into from
Closed

Respect X-Forwarded-Proto header #87

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ad0c43e
Respect X-Forwarded-Proto headers
jellybob Nov 12, 2019
70f79c3
Try moving forwarded proto support after header unwrap
jellybob Nov 12, 2019
83ac77c
Don't touch the headers at all
jellybob Nov 12, 2019
9f1501b
Loosen requirements on async-http to allow git sourcing
jellybob Nov 13, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion falcon.gemspec
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ Gem::Specification.new do |spec|

spec.add_dependency "async", "~> 1.13"
spec.add_dependency "async-io", "~> 1.22"
spec.add_dependency "async-http", "~> 0.48.0"
spec.add_dependency "async-http"
spec.add_dependency "async-container", "~> 0.14.0"

spec.add_dependency "rack", ">= 1.0"
Expand Down
3 changes: 2 additions & 1 deletion lib/falcon/adapters/rack.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ def unwrap_request(request, env)
# https://tools.ietf.org/html/rfc7239#section-5.4
# https://github.com/rack/rack/issues/1310
env[HTTP_X_FORWARDED_PROTO] ||= request.scheme
env[RACK_URL_SCHEME] = env[HTTP_X_FORWARDED_PROTO]
Copy link
Member

Choose a reason for hiding this comment

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

Did we set RACK_URL_SCHEME anywhere else or did I just completely miss it?

Also, is this checked by rack linter?

Copy link
Author

Choose a reason for hiding this comment

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

RACK_URL_SCHEME defaults to request.scheme, however that's not accurate in cases where we're listening on HTTP but have SSL terminated upstream.

Copy link
Member

Choose a reason for hiding this comment

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

If the user sent the X-Forwarded-Proto: https could that break the application that's running on http? Just wondering if there are security implications to this.

Copy link
Contributor

Choose a reason for hiding this comment

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

For context, here's how puma sets RACK_URL_SCHEME:

env[RACK_URL_SCHEME] = default_server_port(env) == PORT_443 ? HTTPS : HTTP

Here's default_server_port:

def default_server_port(env)
  if ['on', HTTPS].include?(env[HTTPS_KEY]) || env[HTTP_X_FORWARDED_PROTO].to_s[0...5] == HTTPS || env[HTTP_X_FORWARDED_SCHEME] == HTTPS || env[HTTP_X_FORWARDED_SSL] == "on"
    PORT_443
  else
    PORT_80
  end
end

In theory if the application was exposed directly, you could bypass middleware to force ssl by sending X-Forwarded-Proto: https. Whether or not that has security implications I don't know, since it could only affect the current user's session.

I'm not certain on this, but I would think it's up to nginx, haproxy, or whatever is used in front of the application to set or reject these headers. Here's how Heroku handles it:

Copy link

@sfnelson sfnelson Aug 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@ioquatix the purpose of the scheme check is to protect the user from sending information over unencrypted connections. If the user provides the X-Forwarded-Proto: https it will only impact them. I don't think this is a security concern.


if remote_address = request.remote_address
env[REMOTE_ADDR] = remote_address.ip_address if remote_address.ip?
Expand Down Expand Up @@ -157,7 +158,7 @@ def call(request)
}

self.unwrap_request(request, env)

if request.push?
env[RACK_EARLY_HINTS] = EarlyHints.new(request)
end
Expand Down
22 changes: 22 additions & 0 deletions spec/falcon/adapters/rack_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,28 @@
end
end

context "rack.url_scheme" do
include_context Falcon::Server
let(:protocol) {Async::HTTP::Protocol::HTTP1}

let(:app) do
lambda do |env|
[200, {}, ["Scheme: #{env['rack.url_scheme'].inspect}"]]
end
end

it 'defaults to http' do
response = client.get('/')

expect(response.read).to be == 'Scheme: "http"'
end

it 'responses X-Forwarded-Proto headers' do
response = client.get('/', [["X-Forwarded-Proto", "https"]])

expect(response.read).to be == 'Scheme: "https"'
end
end
context "early hints" do
it_behaves_like Falcon::Adapters::EarlyHints
end
Expand Down