TakahikoKawasaki / nv-websocket-client

High-quality WebSocket client implementation in Java.
Apache License 2.0
2.03k stars 292 forks source link

Update HandshakeBuilder.java #207

Closed tasict closed 4 years ago

tasict commented 4 years ago

The "HOST" value in standard http header can not contain port.

MinnDevelopment commented 4 years ago

The RFC disagrees: https://tools.ietf.org/html/rfc2616#section-14.23

Host = "Host" ":" host [ ":" port ]

tasict commented 4 years ago

Yes, but most browsers like Chrome, Firebox and Safari did not contain the port column. Addition port value may cause some issue in websocket work with some version haproxy

MinnDevelopment commented 4 years ago

I think the problem here is that you specify a default port which shouldn't be done:

The request MUST contain a |Host| header field whose value contains /host/ plus optionally ":" followed by /port/ (when not using the default port).

Source: https://tools.ietf.org/html/rfc6455#section-4

And to this

but most browsers like Chrome, Firebox and Safari did not contain the port column

It doesn't make sense to make this library non-compliant with the standard because others aren't compliant. If anything the other implementation should be made standard compliant instead.

MinnDevelopment commented 4 years ago

It looks like firefox does use the port in the Host header when you use a non-default port:

image

So the correct change would be to remove the port if it's default.