UpgradeHttpServletRequest.setAttribute & UpgradeHttpServletRequest.removeAttribute can throw NullPointerException

[EricBlanquer]

if the "request" object is null
UpgradeHttpServletRequest.setAttribute:
https://github.com/eclipse/jetty.project/blob/2c812f631e3a5c2fd69147f34f5c65cd36dd2137/jetty-websocket/websocket-core-server/src/main/java/org/eclipse/jetty/websocket/core/server/internal/UpgradeHttpServletRequest.java#L433
and
UpgradeHttpServletRequest.removeAttribute:
https://github.com/eclipse/jetty.project/blob/2c812f631e3a5c2fd69147f34f5c65cd36dd2137/jetty-websocket/websocket-core-server/src/main/java/org/eclipse/jetty/websocket/core/server/internal/UpgradeHttpServletRequest.java#L441

can throw NullPointerException

[joakime]

Can you show us a full stacktrace of your NullPointerException?

[lachlan-roberts]

I think we are just missing the else branch here as request could be null.
See:

https://github.com/eclipse/jetty.project/blob/2c812f631e3a5c2fd69147f34f5c65cd36dd2137/jetty-websocket/websocket-core-server/src/main/java/org/eclipse/jetty/websocket/core/server/internal/UpgradeHttpServletRequest.java#L428-L434

[lachlan-roberts]

Opened PR #7982

[EricBlanquer]

Can you show us a full stacktrace of your NullPointerException?

@joakime I don't think that will help you, but as @lachlan-roberts says, the "else" is missing

2022/05/12-09:01:41.439 [ClusterRequest][E][moduleName=HttpProxy, moduleType=HttpProxy]exception while getting cluster name from token=c7bce380-8a88-4ec9-93b8-a8f0340362f2 java.lang.NullPointerException: Cannot invoke "javax.servlet.http.HttpServletRequest.setAttribute(String, Object)" because "this.request" is null
 	at org.eclipse.jetty.websocket.core.server.internal.UpgradeHttpServletRequest.setAttribute(UpgradeHttpServletRequest.java:433)
 	at javax.servlet.ServletRequestWrapper.setAttribute(ServletRequestWrapper.java:239)
 	at com.centile.cluster.utils.ClusterRequest.setCommunityID(ClusterRequest.java:1143)
 	  **** Exception caught in the method below ****
...
 	at com.centile.cluster.websocket.ClusterWebSocketForClient.onWebSocketConnect(ClusterWebSocketForClient.java:28)
 	at org.eclipse.jetty.websocket.common.JettyWebSocketFrameHandler.onOpen(JettyWebSocketFrameHandler.java:177)
 	at org.eclipse.jetty.websocket.core.internal.WebSocketCoreSession.lambda$onOpen$6(WebSocketCoreSession.java:410)
 	at org.eclipse.jetty.server.handler.ContextHandler.handle(ContextHandler.java:1463)
 	at org.eclipse.jetty.server.handler.ContextHandler.handle(ContextHandler.java:1482)
 	at org.eclipse.jetty.websocket.core.server.internal.AbstractHandshaker$1.handle(AbstractHandshaker.java:212)
 	at org.eclipse.jetty.websocket.core.internal.WebSocketCoreSession.onOpen(WebSocketCoreSession.java:410)
 	at org.eclipse.jetty.websocket.core.internal.WebSocketConnection.onOpen(WebSocketConnection.java:542)
 	at org.eclipse.jetty.io.AbstractEndPoint.upgrade(AbstractEndPoint.java:451)
 	at org.eclipse.jetty.server.HttpConnection.upgrade(HttpConnection.java:419)
 	at org.eclipse.jetty.server.HttpConnection.onCompleted(HttpConnection.java:450)
 	at org.eclipse.jetty.server.HttpChannel.onCompleted(HttpChannel.java:968)
 	at org.eclipse.jetty.server.HttpChannel.handle(HttpChannel.java:485)
 	at org.eclipse.jetty.server.HttpConnection.onFillable(HttpConnection.java:282)
 	at org.eclipse.jetty.io.AbstractConnection$ReadCallback.succeeded(AbstractConnection.java:319)
 	at org.eclipse.jetty.io.FillInterest.fillable(FillInterest.java:100)
 	at org.eclipse.jetty.io.SelectableChannelEndPoint$1.run(SelectableChannelEndPoint.java:53)
 	at org.eclipse.jetty.util.thread.strategy.AdaptiveExecutionStrategy.runTask(AdaptiveExecutionStrategy.java:412)
 	at org.eclipse.jetty.util.thread.strategy.AdaptiveExecutionStrategy.consumeTask(AdaptiveExecutionStrategy.java:381)
 	at org.eclipse.jetty.util.thread.strategy.AdaptiveExecutionStrategy.tryProduce(AdaptiveExecutionStrategy.java:268)
 	at org.eclipse.jetty.util.thread.strategy.AdaptiveExecutionStrategy.lambda$new$0(AdaptiveExecutionStrategy.java:138)
 	at org.eclipse.jetty.util.thread.ReservedThreadExecutor$ReservedThread.run(ReservedThreadExecutor.java:407)
 	at org.eclipse.jetty.util.thread.QueuedThreadPool.runJob(QueuedThreadPool.java:894)
 	at org.eclipse.jetty.util.thread.QueuedThreadPool$Runner.run(QueuedThreadPool.java:1038)
 	at java.base/java.lang.Thread.run(Thread.java:833)
 	```

[joakime]

I think we are just missing the else branch here as request could be null. See:

It may seem obvious, but how did his code even reach this point where request is null?
Having a null request is only possible if using those classes directly, the null checks we have there are to fufill our unit testing requirements where we access those objects directly, not a real server.

The fact that there's a ServletRequestWrapper in the stacktrace is a red flag indicating bad usage of the APIs.
It could mean that their implementation of a ServletRequestWrapper is not taking into account WebSocket upgrade requests, which should never wrap the HttpServletRequest or HttpServletResponse objects.

The stacktrace also shows ...

at javax.servlet.ServletRequestWrapper.setAttribute(ServletRequestWrapper.java:239)
at com.centile.cluster.utils.ClusterRequest.setCommunityID(ClusterRequest.java:1143)
at com.centile.cluster.websocket.ClusterWebSocketForClient.onWebSocketConnect(ClusterWebSocketForClient.java:28)

Also, when you enter onWebSocketConnect there is are no longer any valid HttpServletRequest or HttpServletResponse objects (you left HTTP and are now WebSocket, which no longer has any HTTP facilities), you shouldn't be using those objects in any way from this point forward.

This also hints at code that holds onto those objects improperly for both WebSocket and Servlet specs.

The proposed fix in PR #7982 is just going to quietly break their usage of WebSocket resulting in a broken experience with no details on why.

I feel the fix should go the other way entirely, and throw an exception if the HttpServletRequest (or HttpServletResponse) are wrapped, or not original Jetty request objects, or null.

[lachlan-roberts]

The original intention in this code was to copy the request attributes across to this UpgradeHttpServletRequest after the upgrade so that the attributes would still be accessible after the upgrade when the request has been recycled.

But we have discouraged the reliance on this by having the HttpServletRequest only available from the JettyServerUpgradeRequest during the websocket negotiation. The UpgradeRequest available from the session does not let you set attributes or get the HttpServletRequest.

@EricBlanquer How are you getting the HttpServletRequest inside onWebSocketConnect, did you saving a reference to it from your JettyWebSocketCreator?

[EricBlanquer]

I need to route client HTTP request and WebSocket based on informations that can be found into headers, cookie or path of the request
for that I've use the wapper to use the same code on HTTP/WS request and that's OK

now, I need to provide some metrics on each requests: time, user name found, etc...
for that, when a user is found, I have added this setAttribute to be able to retreive it when the HTTP request is completed
but like I've also use this same code for the WS => NPE

I've added a check to use this setAttribute only if the request is not a UpgradeHttpServletRequest request, that's OK for me

but the UpgradeHttpServletRequest should not throw this NPE

[joakime]

@EricBlanquer you need to go further.

This is important.
Once you enter WebSocket, aka onConnect, you must not use any HTTP component from the Servlet API.
This includes the HttpServletRequest, HttpServletResponse, or HttpSession.
Doesn't matter if it's in its natural form, raw form, wrapped form, etc, you cannot use those classes anymore, HTTP is over, those classes are closed/recycled and can even be used for other HTTP requests.

The only safe time to access the HttpServletRequest, HttpServletResponse, and HttpSession is during the negotiation of the upgrade (which is before onConnect).
This means either using the jakarta.websocket Configurator, or the Jetty WebSocket API WebSocketCreator.
You access what you need and initialize your websocket endpoint at that point. (for jakarta.websocket use the UserProperties to move content from negotiation to onConnect, for Jetty WebSocket API just instantiate and configure your endpoint in traditional java ways)

[lachlan-roberts]

Merged PR #7982 which prevents NPE when changing attributes after WS upgrade.

These changes will be available in the next release (10.0.10/11.0.10).