fix #2, return full host:port info from getHost#383
fix #2, return full host:port info from getHost#383elithrar merged 1 commit intogorilla:masterfrom santsai:getHostFix
Conversation
elithrar
left a comment
There was a problem hiding this comment.
See comment.
I would also expect an update of the .Host() method to indicate that matches will include/require the port in the non-80 case.
|
|
||
| // getHost tries its best to return the request host. | ||
| // According to section 14.23 of RFC 2616 the Host header | ||
| // can include the port number if the default value of 80 is not used. |
There was a problem hiding this comment.
Can you point me to the relevant section of the RFC that covers this requirement for the Host header? It is mentioned in the context of a URL, but seems more ambiguous in the context of a Host header. It is not immediately clear in 7230 either: https://tools.ietf.org/html/rfc7230#section-5.4
3.2.2 http URL
The "http" scheme is used to locate network resources via the HTTP
protocol. This section defines the scheme-specific syntax and
semantics for http URLs.
http_URL = "http:" "//" host [ ":" port ] [ abs_path [ "?" query ]]
If the port is empty or not given, port 80 is assumed.
I'm cautious about making changes here as it may change the matching behaviour (read: breaking change) of existing users.
There was a problem hiding this comment.
The section covering Host header is here https://tools.ietf.org/html/rfc2616#section-14.23
I understood your concern about breaking changes, however it's strange for getHost() to have different behaviour for host from abs url (requires "host:port" in non-80 port case for .Host() to match) and host from header (requires "host" and no port for non-80 port). In the later case, if one build a url from such route, the resulting url would also be wrong since the port number would be missing.
the abs url behaviour is covered by existing test case:
Lines 80 to 88 in e0b5aba
the host from header behaviour is marked as bug in a test case:
Line 116 in e0b5aba
|
It's a bit of a scary change, but it makes sense in theory and I guess we can merge it if all the tests (plus additional ones) pass. |
|
I'm OK to merge this. It will require:
summary: getHost() now returns full host & port information. |
Previously, getHost only returned the host. As it now returns the port as well, any .Host matches on a route will need to be updated to also support matching on the port for cases where the port is non default, eg: 80 for http or 443 for https.
|
Tracking down why my apps suddenly stopped doing host matching properly, and stumbled onto this. Seems like a big backward-breaking change for a minor release. At the very least, when the Route.Host() contains just a host, and not a port, then hostname:* should be assumed, I'd think. |
|
I'm open to a PR for that.
…On Tue, Feb 12, 2019 at 10:29 AM M@ ***@***.***> wrote:
Tracking down why my apps suddenly stopped doing host matching properly,
and stumbled onto this. Seems like a big backward-breaking change for a
minor release. At the very least, when the .Host() contains just a host,
and not a port, then hostname:* should be assumed, I'd think.
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#383 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AABIcBssUbxHGPjewAPXr-5-Vk1l1Eclks5vMwgagaJpZM4UQJkz>
.
|
|
PR #447 does the job efficiently.
|
|
This broke API compatibility with 1.6.x we have a regression in MinIO because of this. |
gorilla/mux#383 broke the compatibility with the existing code. This PR handles that scenario.
gorilla/mux#383 broke the compatibility with the existing code. This PR handles that scenario.
I ran into #2, i think its safe to simply
return r.Hostbecauser.URL.IsAbs()case is already doing that and is covered by existing tests cases.