-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Add HostAuthorization
rack-protection middleware
#2053
Merged
Merged
Changes from 1 commit
Commits
Show all changes
34 commits
Select commit
Hold shift + click to select a range
fb2d76d
Add `permitted_hosts` setting
dentarg b5df6e1
Split bad/good cases in `HostAuthorization` specs
dentarg 492589f
Less brittle `HostAuthorization` specs
dentarg 841ee31
Simplify `HostAuthorization` tests for Sinatra
dentarg d2293d9
Document `HostAuthorization` in rack-protection README
dentarg c9c2625
Expose all the options for `HostAuthorization`
dentarg 6e3711e
Test with multiple hostnames in headers
dentarg 1701645
Improve test descriptions
dentarg 071358e
Port should not matter for `HostAuthorization`
dentarg 856d8e6
No need for `Rack::Protection::HostAuthorization.forwarded?` now
dentarg ffaf412
Support for `IPAddr` hosts
dentarg 44dd856
Always look at the `Host` header
dentarg 76e614a
Better grouping of `HostAuthorization` specs
dentarg e6264be
Test with more exotic header values
dentarg d179c0c
Support for subdomains
dentarg 4652ad2
Support `Forwarded` header in `#forwarded?`
dentarg a167d8b
Use the same `require` order as other protections
dentarg 3d5383d
Better `allow_if` example in tests
dentarg dd73d0e
`HostAuthorization` development settings
dentarg 8466a9e
Test with `nil` `HTTP_HOST`
dentarg 1081bf8
Fix typo in README
dentarg 85d2079
Mention `IPAddr` objects
dentarg 801e821
Add (optional) debug logging in `HostAuthorization`
dentarg 579a2e6
Fix incorrect test description
dentarg affc961
Use keyword argument for `mock_app` helper
dentarg 543bac2
Better test description
dentarg b1bf256
Fix typo in test description
dentarg 9f0d130
Reject invalid hostnames
dentarg 5947a6d
Document `host_authorization` defaults
dentarg b29d698
Lint `HostAuthorization` middleware
dentarg 5ba1250
Lint `HostAuthorization` specs
dentarg 3059727
Avoid `warning: character class has '-' without escape: ...`
dentarg 4dfd160
Silence `Style/SafeNavigationChainLength` cop
dentarg 7697337
No need to silence `SafeNavigationChainLength` cop
dentarg File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Test with more exotic header values
- Loading branch information
commit e6264be6236594cd1ecf6f898181ecf804ef98fe
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure where to put the suggestion, so it will be file-wide. I saw two tests for the corresponding Rails middleware that may be of interest here:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will just copy the tests here for easier discussion (no need to open the link every time you visit this convo)
https://github.com/rails/rails/blob/6230bd334ab47f9efc6d97c6627abe76c80d8058/actionpack/test/dispatch/host_authorization_test.rb#L108
https://github.com/rails/rails/blob/6230bd334ab47f9efc6d97c6627abe76c80d8058/actionpack/test/dispatch/host_authorization_test.rb#L449
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both of these are not blocked currently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder of they can be taken advantage of 🤔 They are not valid hostnames (Wikipedia, see "LDH rule")
$ host example.com#example.org Host example.com#example.org not found: 3(NXDOMAIN) $ host .example.org host: '.example.org' is not a legal name (empty label)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I handled them both in 9f0d130, no longer allowed.
I think there's one difference between Sinatra and Rails now: with
.example.com
in the list of permitted hosts, Rails will allowbar.example.com
but notfoo.bar.example.com
. Sinatra will allow both. This is intentional, I think it makes sense. WDYT @walro?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both ways seem acceptable to me. I guess for Rails one would need to add
.bar.example.com
in addition to.example.com
to allow forfoo.bar.example.com
?In the Sinatra case, to only allow
foo.bar.example.com
(and its siblings) but notbaz.example.com
, one could setup.bar.example.com
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noticed an interesting comment in rack/rack#1604