Add request exclusion to Host Authorization #38829
Conversation
|
It looks like the keyword argument changes in Ruby 2.8 are causing test failures. I haven't checked (installing Ruby master now), but I'm assuming this means the |
cbisnett
force-pushed
the
host_authorization_exclude
branch
from
d2c6309 to
beeaeac
Compare
|
Nevermind, I found the issue. It should be fixed now. |
|
@eileencodes can you see any reason this change would cause ActiveRecord PostgreSQL tests to fail? I’m assuming it’s an intermittent test failure but I don’t know how to kick off a re-test. |
There was a problem hiding this comment.
I fixed that test in ddaa24a. Don't worry too much about CI yet, let's review the patch first.
🔔 @gsamokovarov in case you want to 👀, since you implemented this feature in #33145.
| @@ -34,6 +34,7 @@ def initialize(*) | |||
| @filter_redirect = [] | |||
| @helpers_paths = [] | |||
| @hosts = Array(([".localhost", IPAddr.new("0.0.0.0/0"), IPAddr.new("::/0")] if Rails.env.development?)) | |||
| @host_authorization = { response_app: action_dispatch.hosts_response_app } | |||
There was a problem hiding this comment.
This is referencing action_dispatch.hosts_response_app too early, which will prevent applications from being able to set it. I think we should continue to pass it to the middleware, but emit a deprecation warning if it's set.
There was a problem hiding this comment.
I tested this and it appeared to work. I thought that action_dispatch would have been created prior to this. Is there a better way to pass the existing setting?
There was a problem hiding this comment.
I added a deprecation warning and a fallback to using action_dispatch.hosts_response_app if response_app is not supplied in the configuration. Is there anything else that needs to be done for deprecating a configuration setting? Let me know what you think of this solution.
actionpack/lib/action_dispatch/middleware/host_authorization.rb
Outdated
Show resolved
Hide resolved
cbisnett
force-pushed
the
host_authorization_exclude
branch
3 times, most recently
from
e5192b6 to
9250bd5
Compare
|
I've updated the code and added a deprecation warning, squashed and rebased on master, and tests are passing. |
|
@eugeneius Anything else I need to do or change for this? |
actionpack/lib/action_dispatch/middleware/host_authorization.rb
Outdated
Show resolved
Hide resolved
cbisnett
force-pushed
the
host_authorization_exclude
branch
from
bf5fbd8 to
a48bae3
Compare
|
@eugeneius Any interest in getting this merged? |
|
I'd like to see how #38888 is addressed first, since your particular use case for this feature could potentially be resolved by default. You're right that there are other possible reasons to exclude requests from the check, but I'm reluctant to merge a feature based on a hypothetical example. |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
In the same way that requests may need to be excluded from forced SSL, requests may also need to be excluded from the Host Authorization checks. By providing this additional flexibility more applications will be able to enable Host Authorization while excluding requests that may not conform. For example, AWS Classic Load Balancers don't provide a Host header and cannot be configured to send one. This means that Host Authorization must be disabled to use the health check provided by the load balancer. This change will allow an application to exclude the health check requests from the Host Authorization requirements. I've modified the `ActionDispatch::HostAuthorization` middleware to accept arguments in a similar way to `ActionDispatch::SSL`. The hosts configuration setting still exists separately as does the hosts_response_app but I've tried to group the Host Authorization settings like the ssl_options. It may make sense to deprecate the global hosts_response_app if it's only used as part of the Host Authorization failure response. I've also updated the existing tests as the method signature changed and added new tests to verify the exclusion functionality.
eugeneius
force-pushed
the
host_authorization_exclude
branch
from
a48bae3 to
1f76740
Compare
|
Sorry I let this go stale - let's go for it. |
In the same way that requests may need to be excluded from forced SSL,
requests may also need to be excluded from the Host Authorization
checks. By providing this additional flexibility more applications
will be able to enable Host Authorization while excluding requests
that may not conform. For example, AWS Classic Load Balancers don't
provide a Host header and cannot be configured to send one. This means
that Host Authorization must be disabled to use the health check
provided by the load balancer. This change will allow an application
to exclude the health check requests from the Host Authorization
requirements.
I've modified the
ActionDispatch::HostAuthorizationmiddleware toaccept arguments in a similar way to
ActionDispatch::SSL. The hostsconfiguration setting still exists separately as does the
hosts_response_app but I've tried to group the Host Authorization
settings like the ssl_options. It may make sense to deprecate the
global hosts_response_app if it's only used as part of the Host
Authorization failure response. I've also updated the existing tests
as the method signature changed and added new tests to verify the
exclusion functionality.
Summary
Other Information