Skip to content

websocket: fix parsing of control frames#3239

Closed
tai-kun wants to merge 2 commits intonodejs:mainfrom
tai-kun:bugfix/#2859-fix-parsing-of-control-frames
Closed

websocket: fix parsing of control frames#3239
tai-kun wants to merge 2 commits intonodejs:mainfrom
tai-kun:bugfix/#2859-fix-parsing-of-control-frames

Conversation

@tai-kun
Copy link
Contributor

@tai-kun tai-kun commented May 10, 2024 •
edited
Loading

This relates to #2859

Rationale

If a PING frame is received before binary/text frames is received, those data will be dropped.

Message data is decoded to UTF-8 text if the opcode is TEXT, or converted to Blob or ArrayBuffer if it is BINARY. However, the receiver (ByteParser) determines that the opcode of a frame that cannot be fragmented, such as a PING, is the opcode of a potentially fragmented message. And it will not be overturned until a message handler is called. Therefore, by restricting the type of opcode that is set in this.#info.originalOpcode, we ensure that messages are processed accordingly with the appropriate opcode.

Changes

  • Restrict the frame type of fragmented messages to be binary or text only.

Features

N/A

Bug Fixes

Breaking Changes and Deprecations

N/A

Status

@Uzlopak Uzlopak requested a review from KhafraDev May 10, 2024 11:26
@Uzlopak
Copy link
Contributor

Uzlopak commented May 10, 2024

Ok, existing tests are not breaking but can you add a unit test, please?

@tai-kun
Copy link
Contributor Author

tai-kun commented May 10, 2024

yes. I'll write tests as soon as possible, even though I won't be able to write any code for a while.

@KhafraDev KhafraDev mentioned this pull request May 10, 2024
Merged
@KhafraDev
Copy link
Member

KhafraDev commented May 10, 2024

I believe my PR should fix this also, but we'd need a test.

@tai-kun
Copy link
Contributor Author

tai-kun commented May 11, 2024

I have added a test here:
test/websocket/issue-2859.js

Added test is based on test/websocket/fragments.js. In this test, a TEXT frame is sent before sending a PING, so message data was never dropped. Added test reproduces the problem this time by sending a PING first.

Before fixing control frames parsing, the test issue-2859.js fails with an error similar to the following:

✖ Fragmented frame with a ping frame in the first of it (42.77125ms)
  AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
  
  null !== 'Hello'
  
      at <...trace> {
    generatedMessage: true,
    code: 'ERR_ASSERTION',
    actual: null,
    expected: 'Hello',
    operator: 'strictEqual'
  }

mcollina
mcollina approved these changes May 11, 2024
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

KhafraDev
KhafraDev requested changes May 11, 2024
View reviewed changes
Copy link
Member

@KhafraDev KhafraDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fixed in #3241 as well, which is a more reliable fix. Thank you for the test, I added it there as well..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@KhafraDev KhafraDev KhafraDev requested changes

@mcollina mcollina mcollina approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@tai-kun @Uzlopak @KhafraDev @mcollina