-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Rework host/hostname/authority implementation. #1561
Conversation
This is still a draft/prototype. |
a618e48
to
6bc2a70
Compare
d710df5
to
efede61
Compare
This is quite a complex change, so I welcome a little bit of scrutiny. That being said, the vast majority of the specs are passing without changes, so that's a good sign. |
…ame`. u = URI("http://[::1]/bar") p u.hostname #=> "::1" p u.host #=> "[::1]"
With IPv6, any time we have a string which represents an authority, as defined by RFC7540, the address must be contained within square brackets, e.g.: "[2020::1985]:443". Representations from the `host` header and `authority` pseudo-header must conform to this format. Some headers, notably `x-forwarded-for` and `x-forwarded-host` do not format the authority correctly. So we introduce a private method `wrap_ipv6` which uses a heuristic to detect these situations and fix the formatting. Additionally, we introduce some new assertions in `Rack::Lint` to ensure SREVER_NAME and HTTP_HOST match the formatting requirements.
efede61
to
aa68f07
Compare
def hostname | ||
host, address, port = split_authority(self.authority) | ||
|
||
return address | ||
end |
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.
This looks like inconsistency: the method hostname
returns address
… why? I think we should name it either hostname
or address
ecerywhere.
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.
It matches the implementation of URI
. I'm not saying it's right. I just decided to follow the existing design/method names. But you are right. Maybe it should be address
or something else...
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 mean, if you even just rename local variable to hostname
— it'd be better. I don't see address
as URI method or in its sources.
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.
Understood. It is actually confusing to me too in a way.
u = URI("http://[::1]/bar")
p u.hostname #=> "::1"
p u.host #=> "[::1]"
What does make sense to me is when you say "host_with_port". That means, the "escaped" host and :port
appended. You can't have hostname_with_port
. It's simply not possible because it becomes ambiguous to parse.
So, there's that logic... But yeah, I also see it's confusing.
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, we could rename it hostname
everywhere. I see what you are saying, and I don't think it would be a problem. Except that technically, I don't even know if 192.168.1.1
is a hostname? Or 1::
is a hostname? To me, something like www.google.com
is a hostname. Not sure what is the right word/definition 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 know it's about local variable name but the entire thing is kind of confusing, so we should try to get it right for our users or we just perpetuate the chaos.
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.
So we have several options:
1/ Rename local variable address
-> hostname
and return address
to return hostname
.
2/ Rename method def hostname
to def address
.
3/ Remove def hostname
.
4/ Add def uri
which returns an instance of URI which represents what the URI was used to make the request.
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.
1 is like compliance with URI from stdlib.
2 is like try to improve naming, but, as I see, still with logic violations.
3 is radical, and, I guess, we still need for a helper method. If not — the best code is no code.
4 seems interesting, and "URI" sounds common enough for IPs and domains, especially with URI
instance inside.
So, I can't say before trying, but I'd try to make in this order: 3, 4, 1, 2.
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'll wait for feedback from @jeremyevans before I make any further changes.
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.
Just for the record, there are several new methods, and that includes def hostname
. It's not released in Rack 2.1.
Thanks @jeremyevans I will update the specs according to your feedback. |
63119ed
to
40fb1f4
Compare
40fb1f4
to
eec2635
Compare
Attempt to rework the host/hostname/authority methods to better follow the RFCs regarding host/authority, along with URI specifications for host/hostname. Fixes #1560.