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.

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

Handle http dev_server setting properly in the proxy #1420

Merged
merged 1 commit into from
Apr 16, 2018
Merged

Handle http dev_server setting properly in the proxy #1420

merged 1 commit into from
Apr 16, 2018

Conversation

chrisjohnson
Copy link
Contributor

@chrisjohnson chrisjohnson commented Apr 11, 2018

If dev_server is not set for https, the proxy should always forward requests to http, irrespective of the source request's scheme

The logic for which headers to change comes from looking at the Rack::Proxy source,
which determines if it should make an ssl request by using
http://www.rubydoc.info/gems/rack/Rack/Request/Helpers#scheme-instance_method

Background

In our dev environment, we use an nginx proxy which terminates SSL and talks to the rails app over http. Our webpacker.yml contains the following config:

  dev_server:
    https: false

Which does effectively start webpack-dev-server in http mode, but the rails proxy baked into webpacker does not take this setting into account when forwarding requests onto webpack-dev-server. Instead, it blindly passes in env without mutating it, which Rack::Proxy then uses to determine that it should communicate to webpack-dev-server over https, matching the source request.

If there's some circumstance I'm ignoring/missing here, please let me know, I welcome feedback.

@chrisjohnson
Copy link
Contributor Author

chrisjohnson commented Apr 12, 2018

I verified that nginx-proxy isn't the cause here, by changing things up to run rails server over ssl (following this guide https://www.devmynd.com/blog/rails-local-development-https-using-self-signed-ssl-certificate/) and talking directly to the rails server. I left https: false in my webpacker.yml, and requested my rails application over https. I confirmed that the same error occurred on master, and confirmed that my fork fixes that error. Here's the error output:

#<OpenSSL::SSL::SSLError: SSL_connect SYSCALL returned=5 errno=0 state=SSLv2/v3 read server hello A>
/opt/rh/rh-ruby22/root/usr/share/ruby/net/http.rb:923:in `connect'
/opt/rh/rh-ruby22/root/usr/share/ruby/net/http.rb:923:in `block in connect'
/opt/rh/rh-ruby22/root/usr/share/ruby/timeout.rb:74:in `timeout'
/opt/rh/rh-ruby22/root/usr/share/ruby/net/http.rb:923:in `connect'
/opt/rh/rh-ruby22/root/usr/share/ruby/net/http.rb:863:in `do_start'
/opt/rh/rh-ruby22/root/usr/share/ruby/net/http.rb:858:in `start'
/gems/gems/rack-proxy-0.6.2/lib/rack/http_streaming_response.rb:71:in `session'
/gems/gems/rack-proxy-0.6.2/lib/rack/http_streaming_response.rb:60:in `response'
/gems/gems/rack-proxy-0.6.2/lib/rack/http_streaming_response.rb:29:in `headers'
/gems/gems/rack-proxy-0.6.2/lib/rack/proxy.rb:120:in `perform_request'
/gems/gems/webpacker-3.0.2/lib/webpacker/dev_server_proxy.rb:15:in `perform_request'
/gems/gems/rack-proxy-0.6.2/lib/rack/proxy.rb:57:in `call'
/gems/gems/railties-5.1.4/lib/rails/engine.rb:522:in `call'
/gems/gems/puma-3.10.0/lib/puma/configuration.rb:225:in `call'
/gems/gems/puma-3.10.0/lib/puma/server.rb:605:in `handle_request'
/gems/gems/puma-3.10.0/lib/puma/server.rb:437:in `process_client'
/gems/gems/puma-3.10.0/lib/puma/server.rb:301:in `block in run'
/gems/gems/puma-3.10.0/lib/puma/thread_pool.rb:120:in `call'
/gems/gems/puma-3.10.0/lib/puma/thread_pool.rb:120:in `block in spawn_thread'
2018-04-12 02:00:05 +0000: Rack app error handling request { GET /packs/enrollment-443840a5ee9625a12947.js }

(Note, this same behavior exists in 3.0, 3.4, and 4.0-pre versions of webpacker)

@gauravtiwari
Copy link
Member

Thanks, @chrisjohnson I will take a look at this later today.

@Systho Systho mentioned this pull request Apr 12, 2018
@gauravtiwari
Copy link
Member

@chrisjohnson I have merged #1425 Does that solve this issue for you?

The reason to keep it strict was this
screen shot 2018-04-16 at 09 20 04

The HMR requests to dev server will fail unless both uses same scheme.

@chrisjohnson
Copy link
Contributor Author

chrisjohnson commented Apr 16, 2018

@gauravtiwari : I see, our setups differ -- my webpack-dev-server is also terminated by the https proxy, which is why I don't get those ERR_CONNECTION_CLOSED errors -- my proxy talks to webpack-dev-server over http.

The other PR does not fix the issue for me though. If you see this link: http://www.rubydoc.info/gems/rack/Rack/Request/Helpers#scheme-instance_method

You can see how Rack::Proxy determines if the request should be made to https or http. So the 3 headers that it looks at all need to be overridden. Just overwriting one header will not suffice, given that Rack::Proxy looks at several places to determine if the request should be made over https

As far as I can tell, both my PR and #1425 are compatible with one another, and seem like they will, once combined, solve the issue for all proxy configurations. I don't believe the error you are getting in your console is related to the code we are changing here. I've rebased mine on top of latest master and tested both with and without my change. Without my change, the issue is not solved, and with my change, the issue is solved.

Look forward to your feedback!

If dev_server is not set for https, the proxy should always forward requests to http, irrespective of the source request's scheme

The logic for which headers to change comes from looking at the Rack::Proxy source,
which determines if it should make an ssh request by using
http://www.rubydoc.info/gems/rack/Rack/Request/Helpers#scheme-instance_method
@gauravtiwari gauravtiwari merged commit 182fab0 into rails:master Apr 16, 2018
@gauravtiwari
Copy link
Member

Thanks, @chrisjohnson PR merged but the above still is going to be an issue if your app uses HMR or webpacker in inline mode, although totally unrelated to this fix so no worries :)

Anyway, if using HMR then dev server must use HTTPS mode if it's an HTTPS request since the requests above are directly made to dev server from browser client instead of going through a proxy.

@chrisjohnson
Copy link
Contributor Author

We use HMR with HTTPS requests by also proxying the webpack-dev-server and configuring webpacker's public host to point to the proxy, which terminates SSL and then talks to webpack-dev-server over http.

Thanks so much for merging this!

@gauravtiwari
Copy link
Member

Of course, if dev server requests are proxied too then everything should work just fine 👍

Your welcome and thanks for sharing.

@chrisjohnson
Copy link
Contributor Author

@gauravtiwari Any chance you could make a git tag including this change so I can update our Gemfile to consume it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants