Websocket shouldUpgrade() fail causes empty reply from server

[challfry]

Describe the bug

When the shouldUpgrade() closure in a webSocket route returns nil or throws an error, Vapor returns a zero length response.

Expected: When an attempt to open a WebSocket fails the server should send a 400 or 500 level HTTP response.

To Reproduce

1. Add the following 3 lines to "routes.swift" in a template Vapor project created with vapor new hello -n.

	app.webSocket("successsocket", shouldUpgrade: { req in return [:] }, onUpgrade: { req, socket in })
	app.webSocket("failsocket", shouldUpgrade: { req in return nil }, onUpgrade: { req, socket in })
	app.webSocket("throwsocket", shouldUpgrade: { req in throw Abort(.unauthorized) }, onUpgrade: { req, socket in })

2. run curl, this opens a WebSocket connection:

curl --include \
     --header "Connection: Upgrade" \
     --header "Upgrade: websocket" \
     --header "Host: 127.0.0.1" \
     --header "Origin: http://127.0.0.1:8080" \
     --header "Sec-WebSocket-Key: SGVsbG8sIHdvcmxkIQ==" \
     --header "Sec-WebSocket-Version: 13" \
     http://127.0.0.1:8080/successsocket
HTTP/1.1 101 Switching Protocols
Upgrade: websocket
Sec-WebSocket-Accept: qGEgH3En71di5rrssAZTmtRTyFk=
Connection: upgrade
date: Sat, 02 Mar 2024 23:18:56 GMT
^C

3. run curl again, this time using the route that returns nil from shouldUpgrade():

curl --include \
     --header "Connection: Upgrade" \
     --header "Upgrade: websocket" \
     --header "Host: 127.0.0.1" \
     --header "Origin: http://127.0.0.1:8080" \
     --header "Sec-WebSocket-Key: SGVsbG8sIHdvcmxkIQ==" \
     --header "Sec-WebSocket-Version: 13" \
     http://127.0.0.1:8080/failsocket
curl: (52) Empty reply from server

4. And, with the route that throws a 401 Unauthorized error:

curl --include \
     --header "Connection: Upgrade" \
     --header "Upgrade: websocket" \
     --header "Host: 127.0.0.1" \
     --header "Origin: http://127.0.0.1:8080" \
     --header "Sec-WebSocket-Key: SGVsbG8sIHdvcmxkIQ==" \
     --header "Sec-WebSocket-Version: 13" \
     http://127.0.0.1:8080/throwsocket
curl: (52) Empty reply from server

Expected behavior

I believe HTTP servers are always supposed to return at least a status line in responses. A zero-length response leaves clients no way to communicate why the WebSocket request failed.

Environment

• Vapor Framework version: "4.92.4"
• Vapor Toolbox version:
• OS version: 14.3.1 (23D60)

Additional context

These curl commands use http:// instead of ws:// to make it easy to show the issue. Note that the success case opens the socket and gives the proper 101 response.

[gwynne]

Without the -f flag, curl is not reporting the actual status code from the server in your example - try using the -v flag to see the full protocol exchange, including the actual response code for the empty replies.

[challfry]

Adding the -f and -v flags:

curl -f -v --include \
     --header "Connection: Upgrade" \
     --header "Upgrade: websocket" \
     --header "Host: 127.0.0.1" \
     --header "Origin: http://127.0.0.1:8080" \
     --header "Sec-WebSocket-Key: SGVsbG8sIHdvcmxkIQ==" \
     --header "Sec-WebSocket-Version: 13" \
     http://127.0.0.1:8080/throwsocket
*   Trying 127.0.0.1:8080...
* Connected to 127.0.0.1 (127.0.0.1) port 8080
> GET /throwsocket HTTP/1.1
> Host: 127.0.0.1
> User-Agent: curl/8.4.0
> Accept: */*
> Connection: Upgrade
> Upgrade: websocket
> Origin: http://127.0.0.1:8080
> Sec-WebSocket-Key: SGVsbG8sIHdvcmxkIQ==
> Sec-WebSocket-Version: 13
> 
* Empty reply from server
* Closing connection
curl: (52) Empty reply from server

[gwynne]

Ah ha, literally no response at all, not even an HTTP status line. My apologies for my partial misread of your original post! That is definitely incorrect behavior 😕.

[challfry]

More information that's likely related. When calling a ws route, the middleware futures (or async completions; the part that runs on the way out of the middleware stack) all fire before the "shouldUpgrade" closure is called. Oddly, as the middlewares unwind they all receive a Response object with a 101 status. This means that e.g. ErrorMiddleware never sees an error response when shouldUpgrade() returns an error, because ErrorMiddleware exits before shouldUpgrade gets to run.

Later, after shouldUpgrade throws an error or returns nil the future chain enters the .failure state, the Response object with the 101 status disappears, and eventually NIO closes the channel. I wish I could understand NIO better to figure out which part was where things go wrong, but alas.