Skip to content
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

Malformed scheme logical expression check in WebSocket ClientUpgradeRequest #6407

Closed
steffzahn opened this issue Jun 14, 2021 · 6 comments · Fixed by #6411
Closed

Malformed scheme logical expression check in WebSocket ClientUpgradeRequest #6407

steffzahn opened this issue Jun 14, 2021 · 6 comments · Fixed by #6411
Assignees
Labels
Bug For general bugs on Jetty side

Comments

@steffzahn
Copy link

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

@steffzahn steffzahn added the Bug For general bugs on Jetty side label Jun 14, 2021
@joakime joakime added this to To do in Jetty 10.0.6/11.0.6 FROZEN via automation Jun 14, 2021
@joakime joakime changed the title [Jetty 11.0.5] malformed logical expression in ClientUpgradeRequest Malformed scheme logical expression check in WebSocket ClientUpgradeRequest Jun 14, 2021
@lachlan-roberts
Copy link
Contributor

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
Copy link
Author

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
Copy link
Author

...

@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
Copy link
Contributor

@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
Copy link
Author

steffzahn commented Jun 15, 2021

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.

lachlan-roberts added a commit that referenced this issue Jun 15, 2021
…deprecate it

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@gregw gregw moved this from To do to Review in progress in Jetty 10.0.6/11.0.6 FROZEN Jun 21, 2021
Jetty 10.0.6/11.0.6 FROZEN automation moved this from Review in progress to Done Jun 23, 2021
lachlan-roberts added a commit that referenced this issue Jun 23, 2021
…RequestUri

Issue #6407 - Fix URI validation for WebSocket ClientUpgradeRequest
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For general bugs on Jetty side
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

3 participants