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

WebSocket handling of Connection: upgrade,close. #6642

Closed
joakime opened this issue Aug 18, 2021 · 3 comments · Fixed by #6657
Closed

WebSocket handling of Connection: upgrade,close. #6642

joakime opened this issue Aug 18, 2021 · 3 comments · Fixed by #6657
Assignees
Labels
Bug For general bugs on Jetty side

Comments

@joakime
Copy link
Contributor

joakime commented Aug 18, 2021

Jetty version(s)
Jetty 10+

Description
In Jetty 9.4.x, there was a test for bad client Connection headers.

https://github.com/eclipse/jetty.project/blob/jetty-9.4.x/jetty-websocket/websocket-server/src/test/java/org/eclipse/jetty/websocket/server/FirefoxTest.java

This concept needs to be restored in a new testcase on the Jetty WebSocket rewrite.

Two test cases need to be tested and ensured that they work.

The "Firefox" case. of Connection: keep-alive, Upgrade

GET /socket/ HTTP/1.1
Host: example.com:8080
Upgrade: websocket
Connection: keep-alive, Upgrade
Sec-WebSocket-Key: 9Y7WzchJwKR72PHdKrf7Mg==
Sec-WebSocket-Protocol: chat
Sec-WebSocket-Version: 13

And the "iPhone" and "Proxy/Load Balancer" case of Connection: close, Upgrade

GET /socket/ HTTP/1.1
Host: example.com:8080
Upgrade: websocket
Connection: close, Upgrade
Sec-WebSocket-Key: 9Y7WzchJwKR72PHdKrf7Mg==
Sec-WebSocket-Protocol: chat
Sec-WebSocket-Version: 13
@lachlan-roberts
Copy link
Contributor

@joakime what are we supposed to do in the case of Connection: close, Upgrade?
Are we supposed to ignore the close part and keep the connection open to be used by websocket.

We don't even support this case in 9.4.

lachlan-roberts added a commit that referenced this issue Aug 24, 2021
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@joakime
Copy link
Contributor Author

joakime commented Aug 24, 2021

@joakime what are we supposed to do in the case of Connection: close, Upgrade?
Are we supposed to ignore the close part and keep the connection open to be used by websocket.

We don't even support this case in 9.4.

I'm not 100% sure what the behavior should be here.

On one hand, it wouldn't be too hard to argue that "close" is only for HTTP layer, once the connection is upgraded, the "close" behavior should no longer take effect.

On the other hand, the transition from HTTP to WebSocket is an HTTP Response with an upgraded connection. (the websocket handshake)

But I believe that's just a side effect of websocket on HTTP/1.1, is this HTTP response still a thing in HTTP/2?

@gregw what do you think?

@joakime
Copy link
Contributor Author

joakime commented Aug 24, 2021

You could argue that Connection: closed is also only for controlling HTTP persistence on HTTP/1.1, which an upgraded connection doesn't participate in anyway.

lachlan-roberts added a commit that referenced this issue Aug 24, 2021
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
lachlan-roberts added a commit that referenced this issue Aug 25, 2021
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
@gregw gregw changed the title WebSocket Server Tests needed for bad "Connection" headers. WebSocket handling of Connection: upgrade,close. Aug 25, 2021
lachlan-roberts added a commit that referenced this issue Aug 26, 2021
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
lachlan-roberts added a commit that referenced this issue Aug 26, 2021
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
lachlan-roberts added a commit that referenced this issue Aug 27, 2021
…nnection

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
lachlan-roberts added a commit that referenced this issue Aug 27, 2021
Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
lachlan-roberts added a commit that referenced this issue Aug 31, 2021
… response.

Signed-off-by: Lachlan Roberts <lachlan@webtide.com>
lachlan-roberts added a commit that referenced this issue Aug 31, 2021
…ectionHeaders

Issue #6642 - WebSocket handling of Connection: upgrade,close.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For general bugs on Jetty side
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants