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

Rack handler should use provided default host #1700

Merged

Conversation

schneems
Copy link
Contributor

@schneems schneems commented Jan 4, 2019

This issue is somewhat tricky. When Rails is booted via rails server there are two types of configuration options passed, ones specified directly by a user like rails s -p 3001 will always "win".

For any other config that is not explicitly passed in, puma will consider it a "default". For example when you run rails s (without -p) then the default port will be 3000.

There is one other way to configure puma though, and that is via a config file:

# config/puma.rb
port 3002

This is the order of precedence for configuration

  1. Anything the user explicitly passes to rails s
  2. Config specified in config/puma.rb file
  3. Default values passed in via rails s
  4. Defaults values stored in puma

This fallback mechanism works well except in the case of calling port in a config/puma.rb file. To understand look at the old method definition:

def port(port, host=nil)
  host ||= Configuration::DefaultTCPHost
  bind "tcp://#{host}:#{port}"
end

When the port method gets called, even if the user did not specify a host the Configuration::DefaultTCPHost will be used, which is a problem for local development because it defaults to 0.0.0.0. SO about 0.0.0.0 versus localhost.

In this case, while a user did directly specify a port, they did not specify a host, so you would expect the rails s defaults passed in to take affect.

To make Puma respect that the host coming from rails s has more precedence than it's own default host, we must introduce the ability to set and retrieve a default_host value.

This is then used in the rack handler so when rails s passes in :Host => "localhost" then it is used instead of reverting to 0.0.0.0.

The issue with #1699 is the test was wrong, it would have failed if a config file was present with a port invocation.

This issue is somewhat tricky. When Rails is booted via `rails server` there are two types of configuration options passed, ones specified directly by a user like `rails s -p 3001` will always "win".

For any other config that is not explicitly passed in, puma will consider it a "default". For example when you run `rails s` (without -p) then the default port will be 3000.

There is one other way to configure puma though, and that is via a config file:

```
# config/puma.rb
port 3002
```

This is the order of precedence for configuration

1) Anything the user explicitly passes to `rails s`
2) Config specified in `config/puma.rb` file
3) Default values passed in via `rails s`
4) Defaults values stored in puma

This fallback mechanism works well except in the case of calling `port` in a `config/puma.rb` file. To understand look at the [old method definition](https://github.com/puma/puma/blob/2668597ec1dd9546d83db9f2ec5ad092add483e6/lib/puma/dsl.rb#L140-L145):

```
def port(port, host=nil)
  host ||= Configuration::DefaultTCPHost
  bind "tcp://#{host}:#{port}"
end
```

When the `port` method gets called, even if the user did not specify a `host` the `Configuration::DefaultTCPHost` will be used, which is a problem for local development because it defaults to `0.0.0.0`. [SO about 0.0.0.0 versus localhost](https://stackoverflow.com/questions/20778771/what-is-the-difference-between-0-0-0-0-127-0-0-1-and-localhost).

In this case, while a user did directly specify a port, they did not specify a host, so you would expect the `rails s` defaults passed in to take affect.

To make Puma respect that the host coming from `rails s` has more precedence than it's own default host, we must introduce the ability to set and retrieve a default_host value.

This is then used in the rack handler so when `rails s` passes in `:Host => "localhost"` then it is used instead of reverting to `0.0.0.0`.

The issue with puma#1699 is the test was wrong, it would have failed if a config file was present with a `port` invocation.
@schneems schneems force-pushed the schneems/fix-puma-rack-handler-config branch from a6d92a1 to c24c0c8 Compare January 4, 2019 22:10
@schneems schneems merged commit 36964ec into puma:master Jan 5, 2019
y-yagi added a commit to rails/rails that referenced this pull request Mar 21, 2019
Since puma/puma#1700, the default host is
correctly used. So `localhost` is used instead of `0.0.0.0`.

As a result, the log output on restart is changed, and the restart test
fails on Puma 3.12.1.
https://travis-ci.org/rails/rails/jobs/509239592#L2303-L2305

Specify binding explicitly to avoid being affected by Puma changes.
y-yagi added a commit to y-yagi/rails that referenced this pull request Mar 22, 2019
Since puma/puma#1700, the default host is
correctly used. So `localhost` is used instead of `0.0.0.0`.

As a result, the log output on restart is changed, and the restart test
fails on Puma 3.12.1.
https://travis-ci.org/rails/rails/jobs/509239592#L2303-L2305

Specify binding explicitly to avoid being affected by Puma changes.
y-yagi added a commit to rails/rails that referenced this pull request Mar 22, 2019
Since puma/puma#1700, the default host is
correctly used. So `localhost` is used instead of `0.0.0.0`.

As a result, the log output on restart is changed, and the restart test
fails on Puma 3.12.1.
https://travis-ci.org/rails/rails/jobs/509239592#L2303-L2305

Specify binding explicitly to avoid being affected by Puma changes.
@Fjan
Copy link

Fjan commented Mar 25, 2019

I agree this is a good change (safe bij default) but for anyone looking to restore the old behaviour because you need rails accessible to the outside world most of the time, you can do that by setting a new default for the host in puma.rb like this:

port        ENV.fetch("PORT") { 3000 }, ENV.fetch("HOST") { '0.0.0.0' }

@nicovak
Copy link

nicovak commented Apr 19, 2019

@rafaelfranca @schneems I think this kind of behavior's change should have been notified to the community. I searched for hours why I wasn't able anymore to access my application through local network while developing ... The problem could have been network, proxy or anything else.
Anyway thank you for the change, security first.

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.

None yet

4 participants