websocket
websocket copied to clipboard
jakartaee
Fix #188 Clarify the interaction of upgrade with Servlet specification
Includes how the HTTP upgrade to WebSocket interacts with:
- Filters;
- Listeners; and
- RequestDispatchers when running on a Servlet container
The test is heavily influenced by how Tomcat currently does things. Happy to adjust where necessary to make it more generally applicable.
Way too specific wording.
WebSocket Upgrade should come after servlet security behaviors (constraints), session behaviors, and be first in the FilterChain (always) IMO. Too many libraries and things that make horrible assumptions about HTTP requests and never consider the UPGRADE request rules (eg: wrapping).
Controlling where in the FilterChain the upgrade occurs is out of scope for this spec IMO. (containers can support different models, but it shouldn't be codified in this spec)
IMO, In Servlet terms, any use of HttpServletRequestWrapper or HttpServletResponseWrapper should automatically fail an HTTP Upgrade (any kind, websocket or otherwise).
Way too specific wording.
Do you have suggestions for changes? My aim was to address all the points @gregw raised in the original issue. I was trying to balance being specific enough to address the questions and remove the ambiguity without being more specific than I needed to be to do that.
WebSocket Upgrade should come after servlet security behaviors (constraints), session behaviors, and be first in the FilterChain (always) IMO.
I agree in principle but sometimes some of those behaviours are implemented by Filters so WebSocket upgrade needs to be after them in the FilterChain.
Too many libraries and things that make horrible assumptions about HTTP requests and never consider the UPGRADE request rules (eg: wrapping).
That is an issue for those libraries to address. I don't think we should restrict this spec on the basis that some libraries are poorly designed.
Controlling where in the FilterChain the upgrade occurs is out of scope for this spec IMO. (containers can support different models, but it shouldn't be codified in this spec).
That was my intention with this spec. That the WebSocket upgrade could be anywhere in the FilterChain but this specification doesn't state where. Happy for alternative wording suggestions to make that clearer.
IMO, In Servlet terms, any use of HttpServletRequestWrapper or HttpServletResponseWrapper should automatically fail an HTTP Upgrade (any kind, websocket or otherwise).
I think that is unnecessarily limiting. I'd have no objection to a warning to ensure that Filters wrapping the request/response need to consider the HTTP upgrade to WebSocket case.
Way too specific wording.
Do you have suggestions for changes? My aim was to address all the points @gregw raised in the original issue. I was trying to balance being specific enough to address the questions and remove the ambiguity without being more specific than I needed to be to do that.
This PR seems to introduce WebSocket upgrade as a Filter, up until now we didn't dictate how / when an upgrade occurs on a Servlet container in the spec.
WebSocket Upgrade should come after servlet security behaviors (constraints), session behaviors, and be first in the FilterChain (always) IMO.
I agree in principle but sometimes some of those behaviours are implemented by Filters so WebSocket upgrade needs to be after them in the FilterChain.
This PR introduces WebSocket upgrade as a Filter but doesn't define the scope. It also puts a burden on other Servlet implementations to introduce this functionality (which prior to this PR was not a requirement of the WebSocket spec).
The WebSocket upgrade as Filter use case should either be fully defined by the spec, or not be defined by the spec (like it was before). The wording in this PR is still too ambiguous. I'm basing this on the years of answering Jakarta WebSocket questions on stackoverflow. This WebSocket upgrade as a Filter question has come up repeatedly.
Some general groupings of the questions asked.
- How to configure order of the WebSocket upgrade filter provided by Server implementation?
- What kinds of actions before the WebSocket upgrade Filter are acceptable? (wrapping, header modifications, stream wrapping, stream access, stream use, etc)
- How can a custom WebSocket upgrade filter be written?
- How can my custom WebSocket upgrade filter be used instead of the Server implementation?
- How can I disable the automatic WebSocket upgrade in the Server implementation if I want to do it myself?
Too many libraries and things that make horrible assumptions about HTTP requests and never consider the UPGRADE request rules (eg: wrapping).
That is an issue for those libraries to address. I don't think we should restrict this spec on the basis that some libraries are poorly designed.
We should call out these concerns in the spec then. If the WebSocket upgrade is defined as a Filter, then all Filters before WebSocket as a Filter interacting with the potential WebSocket upgrade must not interfere with the WebSocket upgrade. This includes not messing with the request headers, or response headers, or wrapping the request input stream/reader, or the response output stream/writer.
Controlling where in the FilterChain the upgrade occurs is out of scope for this spec IMO. (containers can support different models, but it shouldn't be codified in this spec).
That was my intention with this spec. That the WebSocket upgrade could be anywhere in the FilterChain but this specification doesn't state where. Happy for alternative wording suggestions to make that clearer.
Again, I'm seeing a need to define "WebSocket upgrade as a Filter" and what is expected (eg: no messing with websocket request/response headers, no wrapping of streams/readers/writer)
Perhaps even dictating what a websocket server implementation of WebSocket upgrade as a Filter should do if ...
- it sees a wrapped stream/reader/writer
- it sees an active stream/reader/writer (meaning those servlet APIs were called)
Should it:
- Fail the entire request with an Exception?
- Quietly skip the WebSocket upgrade filter?
We cannot reliably even respond to the upgrade request in this scenarios.
IMO, In Servlet terms, any use of HttpServletRequestWrapper or HttpServletResponseWrapper should automatically fail an HTTP Upgrade (any kind, websocket or otherwise).
I think that is unnecessarily limiting. I'd have no objection to a warning to ensure that Filters wrapping the request/response need to consider the HTTP upgrade to WebSocket case.
Yup, but in a WebSocket upgrade as a Filter section again.
@joakime I've tried to improve the wording based on your feedback. Is it any better? What still needs to be addressed?