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: fix parsing of control frames #3239

Closed

Conversation

tai-kun
Copy link
Contributor

@tai-kun tai-kun commented May 10, 2024

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
Copy link
Member

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'
  }

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

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants