Support RFC 8441 WebSocket upgrade with HTTP/2 CONNECT - #34362

@jazdw thanks for testing and sending this. Could you check the DCO instructions on how to sign off your commit and update the PR branch? This is required for us to process this.

About checking for HTTP/2, ServletRequest has a getProtocolVersion() method that we could use, but would have to expose it first through ServerHttpRequest. I don't think that's truly necessary.

About checking for HTTP/2, ServletRequest has a getProtocolVersion() method that we could use, but would have to expose it first through ServerHttpRequest. I don't think that's truly necessary.

I dug into this a little more, Jetty verifies the :protocol pseudo header internally, see org.eclipse.jetty.websocket.core.server.internal.RFC8441Negotiation#validateHeaders.

https://github.com/jetty/jetty.project/blob/a171ad3a4d46bde2dfcc7cc5803f12f6919088fb/jetty-core/jetty-websocket/jetty-websocket-core-server/src/main/java/org/eclipse/jetty/websocket/core/server/internal/RFC8441Negotiation.java#L31-L37

It also verifies the method is CONNECT and the HTTP version is HTTP/2.0 in org.eclipse.jetty.websocket.core.server.internal.RFC8441Handshaker#isWebSocketUpgradeRequest

https://github.com/jetty/jetty.project/blob/a171ad3a4d46bde2dfcc7cc5803f12f6919088fb/jetty-core/jetty-websocket/jetty-websocket-core-server/src/main/java/org/eclipse/jetty/websocket/core/server/internal/RFC8441Handshaker.java#L35-L47

I am wondering if we should add a supportsRfc8441() method to the RequestUpgradeStrategy? That way we wouldn't attempt the upgrade if we know the implementation doesn't support CONNECT.

I don't mind a dedicated supports check for RFC 8441 as long as it is possible through the Servlet API. Jetty is at a lower level and can perform more checks I suspect.

@rstoyanchev I've pushed another commit, I don't know that it is necessary though. Happy for you to drop the second commit if you are happy with just the first. I've tested it on Jetty 12 EE10 with both GET setConnectProtocolEnabled(false) and CONNECT setConnectProtocolEnabled(true)

I dropped the second commit. Out handshake checks are there to provide a consistent behavior, but Servlet containers will perform their own checks when upgrading. So we don't need this to be configurable with a flag.

Thanks for the feedback and the changes!

I dropped the second commit. Out handshake checks are there to provide a consistent behavior, but Servlet containers will perform their own checks when upgrading. So we don't need this to be configurable with a flag.

Thanks for the feedback and the changes!

No problem, thanks Rossen