Fix #188 Clarify the interaction of upgrade with Servlet specification #415
Conversation
|
Way too specific wording. WebSocket Upgrade should come after servlet security behaviors (constraints), session behaviors, and be first in the FilterChain (always) IMO. 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). |
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.
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.
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.
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.
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. |
|
Ping @joakime |
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.
This PR introduces WebSocket upgrade as a Filter but doesn't define the scope. 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). Some general groupings of the questions asked.
We should call out these concerns in the spec then.
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 ...
Should it:
We cannot reliably even respond to the upgrade request in this scenarios.
Yup, but in a WebSocket upgrade as a Filter section again. |
…cification Includes how the HTTP upgrade to WebSocket interacts with: - Filters; - Listeners; and - RequestDispatchers when running on a Servlet container
markt-asf
force-pushed
the
fix-issue-188
branch
from
16eba9c to
7367610
Compare
|
@joakime I've tried to improve the wording based on your feedback. Is it any better? What still needs to be addressed? |
|
@joakime ping |
| The WebSocket upgrade *Filter* is positioned in the *FilterChain* after any | ||
| **Filter**s defined in the deployment descriptor (*web.xml*). Applications | ||
| requiring that the WebSocket upgrade *Filter* is positioned earlier in the | ||
| *FilterChain* may define it explicitly in the deployment descriptor. |
There was a problem hiding this comment.
The server defined and implemented Filter should be after Server filters and before deployment descriptor filters, not after.
- Server Filters (there are many examples of this: CORS, Forwarding, Security features that don't fit into Servlet Securtity, Auditing, GZIP, etc)
- WebSocket Filter
- Deployment Descriptor (web.xml and annotations from webapp)
| If the response passed to the *doFilter()* method of the WebSocket upgrade | ||
| *Filter* has been committed, then the WebSocket upgrade *Filter* will take no | ||
| further action and pass the request and response to the next *Filter* in the | ||
| *FilterChain*. |
| *Filter* instances earlier in the *FilterChain* may manipulate and/or wrap the | ||
| request and/or response and these behaviours may influence the functionality of | ||
| the WebSocket upgrade *Filter*. Developers should be aware that some *Filter* | ||
| implementations may not have considered HTTP upgrade and configuring such | ||
| **Filter**s before the WebSocket upgrade *Filter* in the *FilterChain* may break | ||
| the HTTP upgrade process. |
There was a problem hiding this comment.
Should the websocket server implementation supplied WebSocket as a Filter unwrap and requests or responses before it upgrades? (I say yes)
| If the response passed to the *doFilter()* method of the WebSocket upgrade | ||
| *Filter* has not been committed and the WebSocket upgrade *Filter* determines | ||
| that the criteria have been met to allow the upgrade to WebSocket to proceed, | ||
| then the response will be reset before the WebSocket upgrade *Filter* writes the | ||
| appropriate response headers and status code. |
There was a problem hiding this comment.
Seems like a duplicate of the paragraph immediately above this.
| Applications that require a greater degree of control over the HTTP upgrade | ||
| process should use *WsServerContainer.upgradeHttpToWebSocket* method to manually | ||
| trigger the HTTP upgrade process to WebSocket. Such applications will probably | ||
| wish to avoid the use of WebSocket annotations so that the container does not | ||
| automatically deploy WebSocket endpoints as described in | ||
| <<application-deployment-on-web-containers>>. |
There was a problem hiding this comment.
Can we get a ServletContext attribute that disables/configures specific jakarta.websocket features?
- WebSocket Upgrade as a Filter from implementation (options: enable/disable, default is enabled)
- WebSocket Upgrade as a Filter location (options: before/after deployment descriptor from webapp, default is before)
I could see some attributes to disable automatic endpoint additions too.
This would be useful for some scenarios.
There's also the scanning of the webapp to support jakarta.websocket.server.ServerApplicationConfig features.
- Scan and deploy annotated endpoints.
- Scan and report to ServletApplicationConfig entries.
Includes how the HTTP upgrade to WebSocket interacts with:
The test is heavily influenced by how Tomcat currently does things. Happy to adjust where necessary to make it more generally applicable.