websocket: fix ByteParser.run and parseCloseBody

[Uzlopak]

ParseCloseBody can return null. In that case this.#info.closeInfo would be null and the following line would result in a crash.

undici/lib/web/websocket/receiver.js

Line 111 in 5d54543

if (this.#info.closeInfo.code) {

Also reduces the amount of Buffers created.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 87.50000% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 94.19%. Comparing base (48af032) to head (3def0c3).
Report is 76 commits behind head on main.

Files	Patch %	Lines
lib/web/websocket/receiver.js	87.50%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3080      +/-   ##
==========================================
+ Coverage   94.00%   94.19%   +0.18%     
==========================================
  Files          89       90       +1     
  Lines       24314    24436     +122     
==========================================
+ Hits        22856    23017     +161     
+ Misses       1458     1419      -39     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[KhafraDev]

tests are rightfully failing, the PR assumes that having no closing code is invalid when it's optional

[mcollina]

lgtm

[Uzlopak]

@KhafraDev
PTAL

[Uzlopak]

I think there is a bug in the original logic.
parseCloseBody is returning in two places null. This will actually result in code breaking.

[Uzlopak]

@KhafraDev

What do you think now about this?

[KhafraDev]

There are some major issues with this PR

[KhafraDev]

A close frame doesn't need to contain a reason or code, data (the body) can be empty here. You could assert the data length isn't 1 here, but we do check for its length elsewhere so it'd just be for safety.

[Uzlopak]

The thing is:
https://app.codecov.io/gh/nodejs/undici/pull/3080/blob/lib/web/websocket/receiver.js

This line is covered by test. So it is possible that the size of the data is 0.

[KhafraDev]

I know, I said that. The buffer here is not the entire frame (which has to have a minimum length of 2), but the body of a frame. If there is no code and no reason, it is still a valid close frame, and will be empty here. Returning null means that the frame body is invalid, but that's not correct.

[KhafraDev]

On the other hand, the body can be empty or 2+ bytes, but it cannot be 1 byte, which is why I mentioned earlier that for safety we could assert that the body isn't a single byte. We already check elsewhere, but as I mentioned earlier, would be for safety.

[Uzlopak]

Well but it is happening:

[KhafraDev]

I don't understand what that has to do with what I said?

[Uzlopak]

This means, that your claim, that we already handled all cases with data.length < 2 is invalid. Either data.length === 0 or data.length === 1 or both are not covered by the calling function.

[KhafraDev]

There is a misunderstanding here. Returning null means that the close frame's body is invalid. If we can't parse the reason, due to invalid utf-8, we need to return null here.

[Uzlopak]

This seems to be a false conclusion. We got a valid code, so we wont need to set the code 1005 or 1006. The message is somewhat corrupt. So. We need to prefer the valid code and discard the message.

[KhafraDev]

I'm going to go ahead and close this. The main change is creating a singular less Buffer in a very cold case. As lpinca said in my original PR implementing websocket (paraphrasing): "the performance of the opening handshake doesn't matter because it rarely happens". It's the same for closing the connection.

[Uzlopak]

I disagree. At first it was about a performance improvement. Then I found a bug when I implemented this PR, which could lead to a crash.

[KhafraDev]

There isn't a bug being fixed without a test to cover it. The test that you added passes in main. The logic is mostly flawed as I pointed out.

[Uzlopak]

well, lets see if i can write a rogue websocket server.

[Uzlopak]

@KhafraDev
added unit test ;)

[KhafraDev]

This diff makes that test pass. There is quite a lot of extraneous stuff here.

diff --git a/lib/web/websocket/receiver.js b/lib/web/websocket/receiver.js
index 4b35ceb5..b1ef4720 100644
--- a/lib/web/websocket/receiver.js
+++ b/lib/web/websocket/receiver.js
@@ -316,7 +316,7 @@ class ByteParser extends Writable {
     try {
       reason = utf8Decode(reason)
     } catch {
-      return null
+      reason = undefined
     }

     return { code, reason }

[KhafraDev]

I'm not sure if ignoring the reason is correct though. Maybe failing the connection (basically, force closing it) is a better idea. In that case, nothing would need to be changed in parseCloseBody.

edit: Yeah, I don't think the test case makes sense. We're receiving an invalid frame, why are we handling it gracefully?

[Uzlopak]

same test against main:

[KhafraDev]

I understand, but the way this PR is fixing this is wrong.

[KhafraDev]

1007

      1007 indicates that an endpoint is terminating the connection
      because it has received data within a message that was not
      consistent with the type of the message (e.g., non-UTF-8 [[RFC3629]]
      data within a text message).