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.

Sign up for GitHub

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

Jump to bottom

AbstractHandshakeHandler should support RFC 8441 WebSocket upgrade via CONNECT with HTTP/2 #34044

Closed
jazdw opened this issue Dec 6, 2024 · 11 comments
Closed

AbstractHandshakeHandler should support RFC 8441 WebSocket upgrade via CONNECT with HTTP/2 #34044

jazdw opened this issue Dec 6, 2024 · 11 comments
Assignees
@rstoyanchev
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) status: superseded An issue that has been superseded by another type: enhancement A general enhancement

Comments

@jazdw
Copy link
Contributor

jazdw commented Dec 6, 2024 •
edited
Loading

If the web server advertises that it supports RFC 8441 via the HTTP/2 SETTINGS_ENABLE_CONNECT_PROTOCOL parameter then some web browsers (Chrome, Firefox) will follow RFC 8441 to open a WebSocket over a HTTP/2 stream using the CONNECT HTTP method.

The org.springframework.web.socket.server.support.AbstractHandshakeHandler#doHandshake method however only supports the GET method and will respond with 405 Method Not Allowed. and log an error message: Handshake failed due to unexpected HTTP method: CONNECT

The work around is to disable RFC 8441 in your web server, e.g. with Jetty you can set org.eclipse.jetty.http2.server.AbstractHTTP2ServerConnectionFactory#setConnectProtocolEnabled to false (the default is that it is enabled in Jetty 12 EE10).

This problem was observed in Spring 6.2.0.

spring-framework/spring-websocket/src/main/java/org/springframework/web/socket/server/support/AbstractHandshakeHandler.java

Lines 172 to 179 in f8fd6da

if (HttpMethod.GET != request.getMethod()) {
response.setStatusCode(HttpStatus.METHOD_NOT_ALLOWED);
response.getHeaders().setAllow(Collections.singleton(HttpMethod.GET));
if (logger.isErrorEnabled()) {
logger.error("Handshake failed due to unexpected HTTP method: " + request.getMethod());
}
return false;
}

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Dec 6, 2024
@bclozel bclozel added the in: web Issues in web modules (web, webmvc, webflux, websocket) label Dec 6, 2024
@v-perfilev v-perfilev mentioned this issue Jan 4, 2025
Closed
@bclozel
Copy link
Member

bclozel commented Jan 27, 2025

@jazdw As far as I understand the Servlet spec does not support CONNECT requests. Can you explain what kind of support you're asking here?

@bclozel bclozel added the status: waiting-for-feedback We need additional information before we can continue label Jan 27, 2025
@jazdw
Copy link
Contributor Author

jazdw commented Jan 27, 2025

@jazdw As far as I understand the Servlet spec does not support CONNECT requests. Can you explain what kind of support you're asking here?

I'm asking for Spring to support RFC 8441 WebSocket upgrades. I noticed that out of the box on Jetty 12 EE 10 the WebSocket upgrades didn't work so I opened an issue.

Here's a link to the relevant section of the Servlet spec - https://github.com/markt-asf/servlet-api/blob/master/spec/src/main/asciidoc/servlet-spec-body.adoc#213-additional-methods

The CONNECT method applies to proxies whereas the Jakarta Servlet API is targeted at endpoints so, by default, Containers must reject such requests with an SC_NOT_IMPLEMENTED (501) response and not pass the request to a Filter or Servlet. Containers may provide container specific functionality to handle CONNECT requests. If that functionality includes passing CONNECT requests to a Filter or Servlet, then the container should define the expected behaviour of the Servlet API for such requests in that container.

From reading that, I don't think the statement "the Servlet spec does not support CONNECT requests" is true. It looks like it is up to the container implementation.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jan 27, 2025
@bclozel
Copy link
Member

bclozel commented Jan 27, 2025

Can we take a step back?
You have mentioned that disabling this option in Jetty is required otherwise it fails. How can we reproduce this setup? Are you using Spring Boot?

Indeed, Servlet containers can support other HTTP methods. Spring is using Servlet containers but isn't one itself. Our standard websocket upgrade is using a standard Servlet which won't support connect requests, hence my comment.

Maybe something can be done at Servlet container configuration level in Spring Boot?

@bclozel bclozel added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Jan 27, 2025
@jazdw
Copy link
Contributor Author

jazdw commented Jan 28, 2025 •
edited
Loading

Can we take a step back? You have mentioned that disabling this option in Jetty is required otherwise it fails. How can we reproduce this setup? Are you using Spring Boot?

I am not using Spring Boot, this is a Jetty webserver + Spring MVC. I could create a project to reproduce this issue but I don't have time right at the moment.

Indeed, Servlet containers can support other HTTP methods. Spring is using Servlet containers but isn't one itself.

Agreed. I am using the Jetty webserver and Jetty servlet container, both of which do seem to support CONNECT.

Our standard websocket upgrade is using a standard Servlet which won't support connect requests, hence my comment.

It doesn't? Which servlet are we talking about here? The dispatcher servlet? Maybe I am misunderstanding something that will become more clear if I try and create a reproduction project. The application I am working on is pretty massive and confusing the follow. To be clear though, I observed the CONNECT request making its way through to AbstractHandshakeHandler L172 from the above snippet.

Maybe something can be done at Servlet container configuration level in Spring Boot?

Not Spring boot, but yeah you can definitely address this if you are using Jetty by setting AbstractHTTP2ServerConnectionFactory.setConnectProtocolEnabled(false) and this workaround is fine with me. I mainly just thought that Spring would probably want to start supporting RFC 8441 CONNECT upgrades for HTTP/2 since browsers support it.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Jan 28, 2025
@bclozel
Copy link
Member

bclozel commented Jan 28, 2025

@jazdw thanks a lot for your feedback. We would like to support this case and we'll look into reproducing this.
Anything you can point us to (including the client side) is welcome.

@rstoyanchev
Copy link
Contributor

It doesn't? Which servlet are we talking about here?

The default implementation of HttpServlet that all servlets, including DispatcherServlet, extends from doesn't handle CONNECT. It you could break at AbstractHandshakeHandler and show a stacktrace that would be helpful.

@jazdw
Copy link
Contributor Author

jazdw commented Jan 29, 2025

As requested, the line numbers are from Spring 6.2.2 so a little different to above. It looks like the FrameworkServlet/DispatcherServlet do support other methods.

java.lang.Exception: Stack trace
	at java.base/java.lang.Thread.dumpStack(Thread.java:1389)
	at org.springframework.web.socket.server.support.AbstractHandshakeHandler.doHandshake(AbstractHandshakeHandler.java:214)
	at org.springframework.web.socket.server.support.WebSocketHttpRequestHandler.handleRequest(WebSocketHttpRequestHandler.java:177)
	at org.springframework.web.servlet.mvc.HttpRequestHandlerAdapter.handle(HttpRequestHandlerAdapter.java:52)
	at org.springframework.web.servlet.DispatcherServlet.doDispatch(DispatcherServlet.java:1088)
	at org.springframework.web.servlet.DispatcherServlet.doService(DispatcherServlet.java:978)
	at org.springframework.web.servlet.FrameworkServlet.processRequest(FrameworkServlet.java:1014)
	at org.springframework.web.servlet.FrameworkServlet.service(FrameworkServlet.java:888)
	at jakarta.servlet.http.HttpServlet.service(HttpServlet.java:614)
	at org.eclipse.jetty.ee10.servlet.ServletHolder.handle(ServletHolder.java:736)
	at org.eclipse.jetty.ee10.servlet.ServletHandler$ChainEnd.doFilter(ServletHandler.java:1619)
	at com.radixiot.grpcweb.GrpcWebFilter.doFilter(GrpcWebFilter.java:66)
	at org.eclipse.jetty.ee10.servlet.FilterHolder.doFilter(FilterHolder.java:205)
	at org.eclipse.jetty.ee10.servlet.ServletHandler$Chain.doFilter(ServletHandler.java:1591)
	at org.springframework.web.filter.CharacterEncodingFilter.doFilterInternal(CharacterEncodingFilter.java:201)
	at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:116)
	at org.eclipse.jetty.ee10.servlet.FilterHolder.doFilter(FilterHolder.java:205)
	at org.eclipse.jetty.ee10.servlet.ServletHandler$Chain.doFilter(ServletHandler.java:1591)
	at com.infiniteautomation.mango.webapp.filters.MangoCacheControlHeaderFilter.doFilter(MangoCacheControlHeaderFilter.java:88)
	at org.eclipse.jetty.ee10.servlet.FilterHolder.doFilter(FilterHolder.java:205)
	at org.eclipse.jetty.ee10.servlet.ServletHandler$Chain.doFilter(ServletHandler.java:1591)
	at org.springframework.security.web.FilterChainProxy.lambda$doFilterInternal$3(FilterChainProxy.java:231)
	at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:365)
	at org.springframework.security.web.authentication.switchuser.SwitchUserFilter.doFilter(SwitchUserFilter.java:216)
	at org.springframework.security.web.authentication.switchuser.SwitchUserFilter.doFilter(SwitchUserFilter.java:176)
	at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:374)
	at org.springframework.security.web.access.intercept.AuthorizationFilter.doFilter(AuthorizationFilter.java:101)
	at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:374)
	at com.serotonin.m2m2.web.mvc.spring.security.RateLimitingFilter.doFilterInternal(RateLimitingFilter.java:99)
	at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:116)
	at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:374)
	at org.springframework.security.web.access.ExceptionTranslationFilter.doFilter(ExceptionTranslationFilter.java:126)
	at org.springframework.security.web.access.ExceptionTranslationFilter.doFilter(ExceptionTranslationFilter.java:120)
	at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:374)
	at org.springframework.security.web.session.SessionManagementFilter.doFilter(SessionManagementFilter.java:131)
	at org.springframework.security.web.session.SessionManagementFilter.doFilter(SessionManagementFilter.java:85)
	at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:374)
	at org.springframework.security.web.authentication.AnonymousAuthenticationFilter.doFilter(AnonymousAuthenticationFilter.java:100)
	at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:374)
	at org.springframework.security.web.servletapi.SecurityContextHolderAwareRequestFilter.doFilter(SecurityContextHolderAwareRequestFilter.java:179)
	at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:374)
	at org.springframework.security.web.session.ConcurrentSessionFilter.doFilter(ConcurrentSessionFilter.java:151)
	at org.springframework.security.web.session.ConcurrentSessionFilter.doFilter(ConcurrentSessionFilter.java:129)
	at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:374)
	at org.springframework.security.web.authentication.AbstractAuthenticationProcessingFilter.doFilter(AbstractAuthenticationProcessingFilter.java:227)
	at org.springframework.security.web.authentication.AbstractAuthenticationProcessingFilter.doFilter(AbstractAuthenticationProcessingFilter.java:221)
	at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:374)
	at org.springframework.security.web.authentication.AbstractAuthenticationProcessingFilter.doFilter(AbstractAuthenticationProcessingFilter.java:227)
	at org.springframework.security.web.authentication.AbstractAuthenticationProcessingFilter.doFilter(AbstractAuthenticationProcessingFilter.java:221)
	at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:374)
	at org.springframework.security.web.authentication.logout.LogoutFilter.doFilter(LogoutFilter.java:107)
	at org.springframework.security.web.authentication.logout.LogoutFilter.doFilter(LogoutFilter.java:93)
	at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:374)
	at org.springframework.security.web.csrf.CsrfFilter.doFilterInternal(CsrfFilter.java:117)
	at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:116)
	at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:374)
	at org.springframework.security.web.header.HeaderWriterFilter.doHeadersAfter(HeaderWriterFilter.java:90)
	at org.springframework.security.web.header.HeaderWriterFilter.doFilterInternal(HeaderWriterFilter.java:75)
	at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:116)
	at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:374)
	at org.springframework.security.web.context.SecurityContextHolderFilter.doFilter(SecurityContextHolderFilter.java:82)
	at org.springframework.security.web.context.SecurityContextHolderFilter.doFilter(SecurityContextHolderFilter.java:69)
	at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:374)
	at org.springframework.security.web.context.request.async.WebAsyncManagerIntegrationFilter.doFilterInternal(WebAsyncManagerIntegrationFilter.java:62)
	at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:116)
	at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:374)
	at org.springframework.security.web.access.channel.ChannelProcessingFilter.doFilter(ChannelProcessingFilter.java:133)
	at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:374)
	at org.springframework.security.web.session.DisableEncodeUrlFilter.doFilterInternal(DisableEncodeUrlFilter.java:42)
	at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:116)
	at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:374)
	at org.springframework.security.web.FilterChainProxy.doFilterInternal(FilterChainProxy.java:233)
	at org.springframework.security.web.FilterChainProxy.doFilter(FilterChainProxy.java:191)
	at org.springframework.web.filter.DelegatingFilterProxy.invokeDelegate(DelegatingFilterProxy.java:362)
	at org.springframework.web.filter.DelegatingFilterProxy.doFilter(DelegatingFilterProxy.java:278)
	at org.eclipse.jetty.ee10.servlet.FilterHolder.doFilter(FilterHolder.java:205)
	at org.eclipse.jetty.ee10.servlet.ServletHandler$Chain.doFilter(ServletHandler.java:1591)
	at com.infiniteautomation.mango.cloudConnect.proxy.CloudConnectProxyFilter.doFilter(CloudConnectProxyFilter.java:62)
	at org.eclipse.jetty.ee10.servlet.FilterHolder.doFilter(FilterHolder.java:205)
	at org.eclipse.jetty.ee10.servlet.ServletHandler$Chain.doFilter(ServletHandler.java:1591)
	at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:101)
	at org.eclipse.jetty.ee10.servlet.FilterHolder.doFilter(FilterHolder.java:205)
	at org.eclipse.jetty.ee10.servlet.ServletHandler$Chain.doFilter(ServletHandler.java:1591)
	at org.eclipse.jetty.ee10.servlet.ServletHandler$MappedServlet.handle(ServletHandler.java:1552)
	at org.eclipse.jetty.ee10.servlet.ServletChannel.dispatch(ServletChannel.java:819)
	at org.eclipse.jetty.ee10.servlet.ServletChannel.handle(ServletChannel.java:436)
	at org.eclipse.jetty.ee10.servlet.ServletHandler.handle(ServletHandler.java:469)
	at org.eclipse.jetty.security.SecurityHandler.handle(SecurityHandler.java:575)
	at org.eclipse.jetty.ee10.servlet.SessionHandler.handle(SessionHandler.java:717)
	at org.eclipse.jetty.server.handler.ContextHandler.handle(ContextHandler.java:1060)
	at org.eclipse.jetty.server.handler.ContextHandlerCollection.handle(ContextHandlerCollection.java:151)
	at org.eclipse.jetty.server.handler.gzip.GzipHandler.handle(GzipHandler.java:563)
	at org.eclipse.jetty.server.Server.handle(Server.java:182)
	at org.eclipse.jetty.server.internal.HttpChannelState$HandlerInvoker.run(HttpChannelState.java:662)
	at org.eclipse.jetty.util.thread.Invocable$ReadyTask.run(Invocable.java:175)
	at org.eclipse.jetty.http2.server.internal.HttpStreamOverHTTP2$1.run(HttpStreamOverHTTP2.java:135)
	at org.eclipse.jetty.util.thread.strategy.AdaptiveExecutionStrategy.runTask(AdaptiveExecutionStrategy.java:480)
	at org.eclipse.jetty.util.thread.strategy.AdaptiveExecutionStrategy.consumeTask(AdaptiveExecutionStrategy.java:443)
	at org.eclipse.jetty.util.thread.strategy.AdaptiveExecutionStrategy.tryProduce(AdaptiveExecutionStrategy.java:293)
	at org.eclipse.jetty.util.thread.strategy.AdaptiveExecutionStrategy.produce(AdaptiveExecutionStrategy.java:195)
	at org.eclipse.jetty.http2.HTTP2Connection.produce(HTTP2Connection.java:209)
	at org.eclipse.jetty.http2.HTTP2Connection.onFillable(HTTP2Connection.java:156)
	at org.eclipse.jetty.http2.HTTP2Connection$FillableCallback.succeeded(HTTP2Connection.java:442)
	at org.eclipse.jetty.io.FillInterest.fillable(FillInterest.java:99)
	at org.eclipse.jetty.io.ssl.SslConnection$SslEndPoint.onFillable(SslConnection.java:575)
	at org.eclipse.jetty.io.ssl.SslConnection.onFillable(SslConnection.java:390)
	at org.eclipse.jetty.io.ssl.SslConnection$2.succeeded(SslConnection.java:150)
	at org.eclipse.jetty.io.FillInterest.fillable(FillInterest.java:99)
	at org.eclipse.jetty.io.SelectableChannelEndPoint$1.run(SelectableChannelEndPoint.java:53)
	at org.eclipse.jetty.util.thread.strategy.AdaptiveExecutionStrategy.runTask(AdaptiveExecutionStrategy.java:480)
	at org.eclipse.jetty.util.thread.strategy.AdaptiveExecutionStrategy.consumeTask(AdaptiveExecutionStrategy.java:443)
	at org.eclipse.jetty.util.thread.strategy.AdaptiveExecutionStrategy.tryProduce(AdaptiveExecutionStrategy.java:293)
	at org.eclipse.jetty.util.thread.strategy.AdaptiveExecutionStrategy.run(AdaptiveExecutionStrategy.java:201)
	at org.eclipse.jetty.util.thread.ReservedThreadExecutor$ReservedThread.run(ReservedThreadExecutor.java:311)
	at org.eclipse.jetty.util.thread.QueuedThreadPool.runJob(QueuedThreadPool.java:979)
	at org.eclipse.jetty.util.thread.QueuedThreadPool$Runner.doRunJob(QueuedThreadPool.java:1209)
	at org.eclipse.jetty.util.thread.QueuedThreadPool$Runner.run(QueuedThreadPool.java:1164)
	at java.base/java.lang.Thread.run(Thread.java:840)

@rstoyanchev
Copy link
Contributor

Thanks for that. Indeed the DispatcherServlet allows handling of custom HTTP methods, and in this case lets the HTTP CONNECT through, so as long as Jetty is configured to allow this, it will get through the HandshakeHandler.

We can make a targeted change in AbstractHandshakeHandler to recognize HTTP CONNECT as a valid way to upgrade along with all the other WebSocket upgrade checks it makes.

@rstoyanchev rstoyanchev self-assigned this Feb 3, 2025
@rstoyanchev rstoyanchev added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on status: feedback-provided Feedback has been provided labels Feb 3, 2025
@rstoyanchev rstoyanchev changed the title AbstractHandshakeHandler does not support HTTP/2 CONNECT (RFC 8441) AbstractHandshakeHandler should support RFC 8441 WebSocket upgrade via CONNECT with HTTP/2 Feb 3, 2025
@rstoyanchev rstoyanchev added this to the 6.2.3 milestone Feb 3, 2025
@jazdw
Copy link
Contributor Author

jazdw commented Feb 3, 2025 •
edited
Loading

@rstoyanchev I just tested this out on 6.2.3-20250203.154249-25 and unfortunately there's an issue. You are checking for equality of the httpMethod with CONNECT_METHOD but they will never be the same instance, we need to use equals() here. GET works because org.springframework.http.HttpMethod#valueOf always returns the same instance for GET.

f477c16#diff-113f56c6d87cc3c535d4cddb10c55d4ce8f1e358a21072be082ad3cbecdeac46R218

@jazdw
Copy link
Contributor Author

jazdw commented Feb 3, 2025

Further changes are actually required, but I do have it working. I will open a PR.

@jazdw jazdw mentioned this issue Feb 3, 2025
Closed
@rstoyanchev rstoyanchev reopened this Feb 4, 2025
@rstoyanchev
Copy link
Contributor

Superseded by #34362.

@rstoyanchev rstoyanchev closed this as not planned Won't fix, can't repro, duplicate, stale Feb 4, 2025
@rstoyanchev rstoyanchev removed this from the 6.2.3 milestone Feb 4, 2025
@rstoyanchev rstoyanchev added the status: superseded An issue that has been superseded by another label Feb 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rstoyanchev rstoyanchev

Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) status: superseded An issue that has been superseded by another type: enhancement A general enhancement
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@bclozel @rstoyanchev @jazdw @spring-projects-issues