The host header should also include the port #237
Conversation
| @@ -253,6 +253,11 @@ extension HTTPClient { | |||
| return self.scheme == "https" || self.scheme == "https+unix" | |||
| } | |||
|
|
|||
| /// Specified port. | |||
| public var specifiedPort: Int? { | |||
There was a problem hiding this comment.
Do we need to expose this in public API?
There was a problem hiding this comment.
ah we don't need that I guess
There was a problem hiding this comment.
While this is pretty good, in some cases we should omit the port, even if the user asked for it. In particular, we should omit the port when the explicitly specified port matches the implied port (e.g. 443 for https:// and 80 for http://). This parse is explicitly specified in the WHATWG URL specification, and while the Foundation URL type doesn't obey it, we should endeavour to.
|
@Lukasa as an alternative to what is there I could remove the specifiedPort and just output a port when it isn’t 80 or 443. |
|
I think that's the right choice. |
adam-fowler
force-pushed
the
host-includes-port
branch
from
ef296e2 to
1d442b0
Compare
|
Tests aren't working just now, seems like there were some changes on the main branch that broke a few tests. |
adam-fowler
force-pushed
the
host-includes-port
branch
from
1d442b0 to
d61b31a
Compare
| @@ -772,7 +772,9 @@ extension TaskHandler: ChannelDuplexHandler { | |||
| var headers = request.headers | |||
|
|
|||
| if !request.headers.contains(name: "Host") { | |||
| headers.add(name: "Host", value: request.host) | |||
| let port = request.port | |||
| let host = (port != 80 && port != 443) ? "\(request.host):\(port)" : request.host | |||
There was a problem hiding this comment.
shouldn't we only exclude 80 if we're speaking (plain text) HTTP and only exclude 443 when we're speaking HTTPS?
Say I wanted to do http://localhost:443 (note the missing s in http), then I should get localhost:443 or am I missing something? CC @Lukasa ?
There was a problem hiding this comment.
From RFC7230 "If the port is equal to the default port for a scheme, the normal form is to omit the port subcomponent"
I guess 443 is not the normal port, for http so it should not be submitted. Do we want to support the other way. ie https://locahost:80/?
There was a problem hiding this comment.
I think so. I think the only instances where we can omit the port numbers are
http://...:80https://...:443
There was a problem hiding this comment.
swift format is wanting me to output it as
if port != 80 || request.scheme != "http", port != 443 || request.scheme != "https" {
instead of
if (port != 80 || request.scheme != "http") && (port != 443 || request.scheme != "https") {
Is that what you are expecting?
There was a problem hiding this comment.
I really don't like swift-format messing with those things but I guess the argument is consistency ...
There was a problem hiding this comment.
@adam-fowler I think we're discussing on the wrong line here :)
There was a problem hiding this comment.
I meant applying De Morgan to the condition so switching from
(port != 80 || request.scheme != "http") && (port != 443 || request.scheme != "https")
to
! ((port == 80 && request.scheme == "http") || (port == 443 && request.scheme == "https"))
but it's personal preference
adam-fowler
force-pushed
the
host-includes-port
branch
from
a117674 to
5b4ec0b
Compare
See https://tools.ietf.org/html/rfc7230#section-5.4