Malformed scheme logical expression check in WebSocket ClientUpgradeRequest

[steffzahn]

Jetty version(s) 11.0.5

Java version/vendor (use: java -version) 15.0.1

OS type/version Windows 10

Description
in the class ClientUpgradeRequest there is a malformed check in the constructor:
if (!HttpScheme.WS.is(scheme) || !HttpScheme.WSS.is(scheme))

That should obviously be
if (!HttpScheme.WS.is(scheme) && !HttpScheme.WSS.is(scheme))

How to reproduce?
Perform a websocket upgrade request

[joakime]

Referenced lines ...

https://github.com/eclipse/jetty.project/blob/jetty-11.0.5/jetty-websocket/websocket-jetty-client/src/main/java/org/eclipse/jetty/websocket/client/ClientUpgradeRequest.java#L57-L58

https://github.com/eclipse/jetty.project/blob/50df21626238a1755802a672481c3b79997e82b2/jetty-websocket/websocket-jetty-client/src/main/java/org/eclipse/jetty/websocket/client/ClientUpgradeRequest.java#L57-L58

Note: it's the same logic in 10.0.5

This is surprising, we have many tests using this code that are working just fine.

In Jetty 9.4.42 we have different logic here.

https://github.com/eclipse/jetty.project/blob/5cd5e6d2375eeab146813b0de9f19eda6ab6e6cb/jetty-websocket/websocket-common/src/main/java/org/eclipse/jetty/websocket/common/UpgradeRequestAdapter.java#L351-L365

Also, where's the identification of the secure flag on 10+?

[lachlan-roberts]

This looks like it was bad refactor from PR #3740.

Looks like the URI of the upgrade request is never used. All of the WebSocketClient.connect() methods always take a URI parameter so we never use the URI from the ClientUpgradeRequest. And it turns out we never test the upgrade request with the constructor which takes a URI.

@steffzahn this is something that should be fixed for the next release, but as a workaround just create the ClientUpgradeRequest without passing it the URI parameter.

[steffzahn]

Well, at least I cannot find any code ( test code or other code ) in the jetty project, using the constructor ClientUpgradeRequest(URI uri). Apparently a test case is missing.

[steffzahn]

...

@steffzahn this is something that should be fixed for the next release, but as a workaround just create the ClientUpgradeRequest without passing it the URI parameter.

Well that does not work directly, since the fields are final and private. But it was possible to create a hack using reflection.

[lachlan-roberts]

@steffzahn you should not use reflection to set the fields. They do not need to be set, you can use the no-arg constructor instead as these fields on ClientUpgradeRequest are never used by the implementation.

The URI must still be passed to the call to WebSocketClient.connect() which does the correct check for the scheme.

[steffzahn]

The URI must still be passed to the call to WebSocketClient.connect() which does the correct check for the scheme.

Thank you, that hint works for me.