RACK_URL_SCHEME calculation caring about more headers #1491

olleolleolle · 2017-12-21T15:00:44Z

This PR is about #1179, as a way of pushing the discussion onwards.

add new constants to const.rb and use them
RACK_URL_SCHEME is now read from a series of HTTP headers (just like Rack does it)

nateberkopec · 2018-03-19T20:47:01Z

This definitely looks like the right approach, just needs the things you already pointed out to get it over the line.

evanphx · 2019-02-20T00:59:06Z

Can this be merged? Is anything waiting on it?

olleolleolle · 2019-02-20T11:34:47Z

@evanphx I apologize, this had been sitting - I'll turn these TODO items into a checkbox list and then do them.

When visible and less obtuse, review becomes much easier.

olleolleolle · 2019-07-09T17:43:37Z

In order to get this code mergeable, I offer it as-is to review. After re-reading this change, I think it's ready.

I had one question to my earlier self, who was borrowing that code.

Q: Why am I splitting `X-Forwarded-Proto` on first comma?

Would anyone add more content to that header? Are there any examples?

A: Example from the code author

The answer is: "yes there are examples!"

rack/rack@85ca454

Documentation about the `X-Forwarded-Proto` header, in general

None of these mention any of the practical examples of "multiple proxies after each other", but they can be nice sources of learning about the header, in general.

Mozilla MDN explains the X-Forwarded-Proto

Cloudflare also appends an X-Forwarded-Proto header, which can either be HTTP or HTTPS depending on the protocol the user used to visit the site, like this:
"X-Forwarded-Proto: https"

Cloudflare support docs on the header

Example: Cloudflare explains X-Forwarded-Proto

The X-Forwarded-Proto request header helps you identify the protocol (HTTP or HTTPS) that a client used to connect to your load balancer. Your server access logs contain only the protocol used between the server and the load balancer; they contain no information about the protocol used between the client and the load balancer. To determine the protocol used between the client and the load balancer, use the X-Forwarded-Proto request header. Elastic Load Balancing stores the protocol used between the client and the load balancer in the X-Forwarded-Proto request header and passes the header along to your server.

Your application or website can use the protocol stored in the X-Forwarded-Proto request header to render a response that redirects to the appropriate URL.

The X-Forwarded-Proto request header takes the following form:
X-Forwarded-Proto: originatingProtocol
The following example contains an X-Forwarded-Proto request header for a request that originated from the client as an HTTPS request:
X-Forwarded-Proto: https

AWS ELB documentation on the header

Example: AWS explains X-Forwarded-Proto

Addendum: TIL: the Forwarded header

There's a standardized header Forwarded (2014) and it has structure. We're not managing that in Puma yet.

lib/puma/server.rb

nateberkopec

Just a few stylistic changes. Also, needs tests.

olleolleolle · 2019-07-15T12:25:40Z

@nateberkopec I tried to interpret your comments as commits.

olleolleolle · 2019-07-15T20:56:10Z

Note: To run one test file, saving yourself time, use `TEST`:

bundle exec rake test TEST=test/test_puma_server.rb

olleolleolle · 2019-07-15T21:03:44Z

@nateberkopec Thanks for asking again, for tests. I sat down and had a go.

nateberkopec · 2019-07-15T21:14:00Z

lib/puma/server.rb

@@ -588,8 +588,11 @@ def normalize_env(env, client)
    end

    def default_server_port(env)
-      return PORT_443 if env[HTTPS_KEY] == 'on' || env[HTTPS_KEY] == 'https'
-      env['HTTP_X_FORWARDED_PROTO'] == 'https' ? PORT_443 : PORT_80
+      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"


OK - now that we've refactored this out a bit, line 591 and the if/if/else in like 634 look really similar. Is there any possibility for re-use here?

nateberkopec · 2019-07-15T21:16:38Z

test/test_puma_server.rb

+    req['HOST'] = "example.com"
+    req['X_FORWARDED_SSL'] = "on"
+
+    res = Net::HTTP.start @host, @server.connected_port do |http|


Hmmm. This doesn't match our current style of just writing onto a TCP socket. Thoughts on that?

Eh, I only repeated test bodies which were there, tests that checked similar things, to have something to start with.

When I refactor this, I'll try to narrow down the tests to be more about the methods I changed.

nateberkopec · 2019-07-15T21:17:16Z

test/test_puma_server.rb

+  end
+
+  def test_respect_x_forwarded_scheme
+    @server.app = proc do |env|


Once my other comment is resolved - the setup in all of these is so similar, we should extract to a helper method.

I'd be glad to!

nateberkopec · 2019-07-15T21:18:06Z

test/test_puma_server.rb

+      http.request(req)
+    end
+
+    assert_equal "80", res.body


I wonder - why not just test the actual methods modified rather than test res.body? This test is testing a lot more than it has to.

- See rack/rack@85ca454

…WARDED_SSL

olleolleolle · 2019-07-17T21:27:51Z

@nateberkopec I took a closer look, and made follow-up commits.

nateberkopec · 2019-07-17T22:48:34Z

🚢 🚢 🚢

nateberkopec · 2019-07-17T22:48:52Z

Thanks for all your hard work here, @olleolleolle !

nateberkopec mentioned this pull request Mar 19, 2018

set RACK_URL_SCHEME to https when X-FORWARDED-PROTO is https #1179

Closed

nateberkopec added the bug label Mar 19, 2018

olleolleolle force-pushed the feature/issue-1179 branch from 769d7f9 to 05acc62 Compare March 14, 2019 21:00

olleolleolle force-pushed the feature/issue-1179 branch from 05acc62 to 98693e9 Compare July 9, 2019 16:53

nateberkopec reviewed Jul 15, 2019

View reviewed changes

lib/puma/server.rb Outdated Show resolved Hide resolved

nateberkopec reviewed Jul 15, 2019

View reviewed changes

lib/puma/server.rb Outdated Show resolved Hide resolved

nateberkopec requested changes Jul 15, 2019

View reviewed changes

nateberkopec self-assigned this Jul 15, 2019

olleolleolle force-pushed the feature/issue-1179 branch from 08b9d9c to a094ba3 Compare July 15, 2019 13:17

nateberkopec reviewed Jul 15, 2019

View reviewed changes

nateberkopec self-requested a review July 15, 2019 21:15

nateberkopec requested changes Jul 15, 2019

View reviewed changes

olleolleolle added 7 commits July 17, 2019 22:53

RACK_URL_SCHEME read the same as in Rack

80f2c8e

Code comment on HTTP_X_FORWARDED_PROTO

836e1da

- See rack/rack@85ca454

Make default_server_port logic more readable

f916ac6

Test: X-Forwarded-Proto and friends

0f23e65

Calculate default_port respecting HTTP_X_FORWARDED_SCHEME, HTTP_X_FOR…

7b5789e

…WARDED_SSL

Calculate URL scheme using #default_server_port

aa066db

Narrow down tests for default_server_port(env)

329f00e

olleolleolle force-pushed the feature/issue-1179 branch from 3dc47ed to 329f00e Compare July 17, 2019 20:54

nateberkopec merged commit 30fbd84 into puma:master Jul 17, 2019

olleolleolle deleted the feature/issue-1179 branch July 18, 2019 04:32

Jesus mentioned this pull request Jul 18, 2019

Adds instructions on how to use m #1853

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RACK_URL_SCHEME calculation caring about more headers #1491

RACK_URL_SCHEME calculation caring about more headers #1491

olleolleolle commented Dec 21, 2017 •

edited

Loading

nateberkopec commented Mar 19, 2018

evanphx commented Feb 20, 2019

olleolleolle commented Feb 20, 2019

olleolleolle commented Jul 9, 2019 •

edited

Loading

nateberkopec left a comment

olleolleolle commented Jul 15, 2019

olleolleolle commented Jul 15, 2019

olleolleolle commented Jul 15, 2019

nateberkopec Jul 15, 2019

nateberkopec Jul 15, 2019

olleolleolle Jul 16, 2019

nateberkopec Jul 15, 2019

olleolleolle Jul 16, 2019

nateberkopec Jul 15, 2019

olleolleolle commented Jul 17, 2019

nateberkopec commented Jul 17, 2019

nateberkopec commented Jul 17, 2019

RACK_URL_SCHEME calculation caring about more headers #1491

RACK_URL_SCHEME calculation caring about more headers #1491

Conversation

olleolleolle commented Dec 21, 2017 • edited Loading

nateberkopec commented Mar 19, 2018

evanphx commented Feb 20, 2019

olleolleolle commented Feb 20, 2019

olleolleolle commented Jul 9, 2019 • edited Loading

Q: Why am I splitting X-Forwarded-Proto on first comma?

A: Example from the code author

Documentation about the X-Forwarded-Proto header, in general

nateberkopec left a comment

Choose a reason for hiding this comment

olleolleolle commented Jul 15, 2019

olleolleolle commented Jul 15, 2019

Note: To run one test file, saving yourself time, use TEST:

olleolleolle commented Jul 15, 2019

nateberkopec Jul 15, 2019

Choose a reason for hiding this comment

nateberkopec Jul 15, 2019

Choose a reason for hiding this comment

olleolleolle Jul 16, 2019

Choose a reason for hiding this comment

nateberkopec Jul 15, 2019

Choose a reason for hiding this comment

olleolleolle Jul 16, 2019

Choose a reason for hiding this comment

nateberkopec Jul 15, 2019

Choose a reason for hiding this comment

olleolleolle commented Jul 17, 2019

nateberkopec commented Jul 17, 2019

nateberkopec commented Jul 17, 2019

olleolleolle commented Dec 21, 2017 •

edited

Loading

olleolleolle commented Jul 9, 2019 •

edited

Loading

Q: Why am I splitting `X-Forwarded-Proto` on first comma?

Documentation about the `X-Forwarded-Proto` header, in general

Note: To run one test file, saving yourself time, use `TEST`: