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

UpgradeHttpServletRequest.setAttribute & UpgradeHttpServletRequest.removeAttribute can throw NullPointerException #7977

Closed
EricBlanquer opened this issue May 11, 2022 · 9 comments · Fixed by #7982

Comments

@joakime
Copy link
Contributor

joakime commented May 11, 2022

Can you show us a full stacktrace of your NullPointerException?

@lachlan-roberts
Copy link
Contributor

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

Opened PR #7982

@lachlan-roberts lachlan-roberts self-assigned this May 12, 2022
@EricBlanquer
Copy link
Author

EricBlanquer commented May 12, 2022

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

joakime commented May 12, 2022

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

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

EricBlanquer commented May 13, 2022

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

joakime commented May 13, 2022

@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)

Jetty 10.0.10/11.0.10 - 🧊 FROZEN 🥶 automation moved this from To do to Done May 16, 2022
lachlan-roberts added a commit that referenced this issue May 16, 2022
Issue #7977 - prevent possible NPE from UpgradeHttpServletRequest
@lachlan-roberts
Copy link
Contributor

lachlan-roberts commented May 16, 2022

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

3 participants