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.
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 #2053Add
HostAuthorization
rack-protection middleware #2053Changes from 1 commit
fb2d76d
b5df6e1
492589f
841ee31
d2293d9
c9c2625
6e3711e
1701645
071358e
856d8e6
ffaf412
44dd856
76e614a
e6264be
d179c0c
4652ad2
a167d8b
3d5383d
dd73d0e
8466a9e
1081bf8
85d2079
801e821
579a2e6
affc961
543bac2
b1bf256
9f0d130
5947a6d
b29d698
5ba1250
3059727
4dfd160
7697337
File filter
Filter by extension
Conversations
Jump to
IPAddr
hostsThere are no files selected for viewing
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