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

feat(websocket): only consume necessary bytes #1812

Merged
merged 4 commits into from
Dec 19, 2022

Conversation

KhafraDev
Copy link
Member

@KhafraDev KhafraDev commented Dec 18, 2022

Refs: #1811

Before, for every parsing stage, we would concatenate every buffer, even if we only needed to read the first few bytes (for most cases). Now, we can consume n bytes from the buffers, removing them & handling the total length. This simplifies the logic quite a bit and should improve performance (although there aren't any benchmarks to confirm this).

ws implements something similar:
https://github.com/websockets/ws/blob/fb1dfd217861757d60c1d02c6e66b4da3630cc93/lib/receiver.js#L94-L131

@codecov-commenter
Copy link

codecov-commenter commented Dec 18, 2022

Codecov Report

Base: 87.27% // Head: 87.27% // Decreases project coverage by -0.00% ⚠️

Coverage data is based on head (1cc2f6a) compared to base (860c261).
Patch coverage: 75.67% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1812      +/-   ##
==========================================
- Coverage   87.27%   87.27%   -0.01%     
==========================================
  Files          66       66              
  Lines        5792     5799       +7     
==========================================
+ Hits         5055     5061       +6     
- Misses        737      738       +1     
Impacted Files Coverage Δ
lib/websocket/receiver.js 76.35% <73.52%> (+0.29%) ⬆️
lib/websocket/constants.js 100.00% <100.00%> (ø)
lib/websocket/websocket.js 42.16% <100.00%> (ø)
lib/fetch/body.js 96.53% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@lpinca
Copy link
Member

lpinca commented Dec 18, 2022

Can you add the copyright notice? ByteParser is a slightly modified copy of ws Receiver.

FWIW Receiver is exposed by ws for third party usage.

const { Receiver } = require('ws');

@KhafraDev
Copy link
Member Author

KhafraDev commented Dec 18, 2022

Sure, although none of the code is copied directly from ws. Everything was written by hand, which is probably the reason it took so long to get right (and the reason there are still bugs). Since both use Writable node streams, there's going to be overlap with how some things are handled, but if I did copy code anywhere, I would have left a copyright notice already.

Obviously ws was a big inspiration and a good library to base another implementation of because it's much older and much more used.

@lpinca
Copy link
Member

lpinca commented Dec 18, 2022

There are so many similarities here and in a0094a4 that I think it is deserved even if ws Receiver was used only as a reference. You could add something along the lines "This code was adapted from XXX released under the MIT license". It is the only requirement of the MIT license.

Extending stream.Writable is a good choice for a future permessage-deflate implementation where decompression is asynchronous and might be slower than incoming data. At the moment, without permessage-deflate, extending stream.Writable is only unnecessary overhead as everything is synchronous.

@KhafraDev
Copy link
Member Author

KhafraDev commented Dec 18, 2022

I've added a notice, let me know if there's anything else!

@lpinca
Copy link
Member

lpinca commented Dec 18, 2022

That's ok, thanks. If you think no notice should be added and want to remove it, feel free to do that, I won't get in the way anymore.

@KhafraDev
Copy link
Member Author

No, you deserve recognition - without your help the last week, there's no way I'd have as good of an understanding of websockets, and undici's implementation, without ws, would be incomplete and full of bugs. A notice is the least I can do, all things considered.

lib/websocket/receiver.js Outdated Show resolved Hide resolved
@ronag ronag merged commit 5165d67 into nodejs:main Dec 19, 2022
@KhafraDev KhafraDev deleted the websocket-consume branch December 19, 2022 14:53
anonrig pushed a commit to anonrig/undici that referenced this pull request Apr 4, 2023
kodiakhq bot referenced this pull request in X-oss-byte/Canary-nextjs Sep 18, 2023
[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [undici](https://undici.nodejs.org) ([source](https://togithub.com/nodejs/undici)) | [`5.14.0` -> `5.19.1`](https://renovatebot.com/diffs/npm/undici/5.14.0/5.19.1) | [![age](https://developer.mend.io/api/mc/badges/age/npm/undici/5.19.1?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://developer.mend.io/api/mc/badges/adoption/npm/undici/5.19.1?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://developer.mend.io/api/mc/badges/compatibility/npm/undici/5.14.0/5.19.1?slim=true)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://developer.mend.io/api/mc/badges/confidence/npm/undici/5.14.0/5.19.1?slim=true)](https://docs.renovatebot.com/merge-confidence/) |

### GitHub Vulnerability Alerts

#### [CVE-2023-23936](https://togithub.com/nodejs/undici/security/advisories/GHSA-5r9g-qh6m-jxff)

### Impact

undici library does not protect `host` HTTP header from CRLF injection vulnerabilities.

### Patches

This issue was patched in Undici v5.19.1.

### Workarounds

Sanitize the `headers.host` string before passing to undici.

### References

Reported at https://hackerone.com/reports/1820955.

### Credits

Thank you to Zhipeng Zhang ([@&#8203;timon8](https://hackerone.com/timon8)) for reporting this vulnerability.

---

### Release Notes

<details>
<summary>nodejs/undici (undici)</summary>

### [`v5.19.1`](https://togithub.com/nodejs/undici/releases/tag/v5.19.1)

[Compare Source](https://togithub.com/nodejs/undici/compare/v5.19.0...v5.19.1)

#### ⚠️ Security Release ⚠️

-   [Regular Expression Denial of Service in Headers](https://togithub.com/nodejs/undici/security/advisories/GHSA-r6ch-mqf9-qc9w) with CVE-2023-24807
-   [CRLF Injection in Nodejs ‘undici’ via host](https://togithub.com/nodejs/undici/security/advisories/GHSA-5r9g-qh6m-jxff) with CVE-2023-23936

This release is part of the Node.js security release train: https://nodejs.org/en/blog/vulnerability/february-2023-security-releases/

### [`v5.19.0`](https://togithub.com/nodejs/undici/releases/tag/v5.19.0)

[Compare Source](https://togithub.com/nodejs/undici/compare/v5.18.0...v5.19.0)

#### What's Changed

-   fix(fetch): raise AbortSignal max event listeners by [@&#8203;KhafraDev](https://togithub.com/KhafraDev) in [https://github.com/nodejs/undici/pull/1910](https://togithub.com/nodejs/undici/pull/1910)
-   fix: content-disposition header parsing by [@&#8203;climba03003](https://togithub.com/climba03003) in [https://github.com/nodejs/undici/pull/1911](https://togithub.com/nodejs/undici/pull/1911)
-   fix: remove test by [@&#8203;KhafraDev](https://togithub.com/KhafraDev) in [https://github.com/nodejs/undici/pull/1916](https://togithub.com/nodejs/undici/pull/1916)
-   feat: add Headers.prototype.getSetCookie by [@&#8203;KhafraDev](https://togithub.com/KhafraDev) in [https://github.com/nodejs/undici/pull/1915](https://togithub.com/nodejs/undici/pull/1915)
-   fix(headers): clone getSetCookie list & add getSetCookie type by [@&#8203;KhafraDev](https://togithub.com/KhafraDev) in [https://github.com/nodejs/undici/pull/1917](https://togithub.com/nodejs/undici/pull/1917)
-   doc(mock): update out-of-date reply documentation by [@&#8203;p9f](https://togithub.com/p9f) in [https://github.com/nodejs/undici/pull/1913](https://togithub.com/nodejs/undici/pull/1913)
-   fix(types): add missing keepAlive params by [@&#8203;SkeLLLa](https://togithub.com/SkeLLLa) in [https://github.com/nodejs/undici/pull/1918](https://togithub.com/nodejs/undici/pull/1918)
-   Make the fetch() abort test pass locally, on Linux and Mac, Node 18/19. by [@&#8203;mcollina](https://togithub.com/mcollina) in [https://github.com/nodejs/undici/pull/1927](https://togithub.com/nodejs/undici/pull/1927)

#### New Contributors

-   [@&#8203;climba03003](https://togithub.com/climba03003) made their first contribution in [https://github.com/nodejs/undici/pull/1911](https://togithub.com/nodejs/undici/pull/1911)
-   [@&#8203;p9f](https://togithub.com/p9f) made their first contribution in [https://github.com/nodejs/undici/pull/1913](https://togithub.com/nodejs/undici/pull/1913)

**Full Changelog**: nodejs/undici@v5.18.0...v5.19.0

### [`v5.18.0`](https://togithub.com/nodejs/undici/releases/tag/v5.18.0)

[Compare Source](https://togithub.com/nodejs/undici/compare/v5.17.1...v5.18.0)

##### What's Changed

-   Add ability to set TCP keepalive by [@&#8203;xconverge](https://togithub.com/xconverge) in [https://github.com/nodejs/undici/pull/1904](https://togithub.com/nodejs/undici/pull/1904)
-   use faster timers by [@&#8203;ronag](https://togithub.com/ronag) in [https://github.com/nodejs/undici/pull/1908](https://togithub.com/nodejs/undici/pull/1908)
-   fix: ensure header value is a string by [@&#8203;ronag](https://togithub.com/ronag) in [https://github.com/nodejs/undici/pull/1899](https://togithub.com/nodejs/undici/pull/1899)

**Full Changelog**: nodejs/undici@v5.17.1...v5.18.0

### [`v5.17.1`](https://togithub.com/nodejs/undici/releases/tag/v5.17.1)

[Compare Source](https://togithub.com/nodejs/undici/compare/v5.17.0...v5.17.1)

#### What's Changed

-   fix: bad buffer slice (nodejs/undici@d2be675)

**Full Changelog**: nodejs/undici@v5.17.0...v5.17.1

### [`v5.17.0`](https://togithub.com/nodejs/undici/releases/tag/v5.17.0)

[Compare Source](https://togithub.com/nodejs/undici/compare/v5.16.0...v5.17.0)

#### What's Changed

-   fix(wpts): Blob is a global getter in >=v19.x.x by [@&#8203;KhafraDev](https://togithub.com/KhafraDev) in [https://github.com/nodejs/undici/pull/1880](https://togithub.com/nodejs/undici/pull/1880)
-   doc: fix anchor links dispatcher.stream by [@&#8203;RafaelGSS](https://togithub.com/RafaelGSS) in [https://github.com/nodejs/undici/pull/1881](https://togithub.com/nodejs/undici/pull/1881)
-   wpt: make runner more resilient by [@&#8203;KhafraDev](https://togithub.com/KhafraDev) in [https://github.com/nodejs/undici/pull/1884](https://togithub.com/nodejs/undici/pull/1884)
-   Make test pass in v19.x by [@&#8203;mcollina](https://togithub.com/mcollina) in [https://github.com/nodejs/undici/pull/1879](https://togithub.com/nodejs/undici/pull/1879)
-   Correct the type of DispatchOptions\["headers"] by [@&#8203;pan93412](https://togithub.com/pan93412) in [https://github.com/nodejs/undici/pull/1896](https://togithub.com/nodejs/undici/pull/1896)
-   perf(content-type parser): faster string collector by [@&#8203;KhafraDev](https://togithub.com/KhafraDev) in [https://github.com/nodejs/undici/pull/1894](https://togithub.com/nodejs/undici/pull/1894)
-   feat: expose content-type parser by [@&#8203;KhafraDev](https://togithub.com/KhafraDev) in [https://github.com/nodejs/undici/pull/1895](https://togithub.com/nodejs/undici/pull/1895)
-   fix(types): Update DispatchOptions type for missing "blocking" by [@&#8203;xconverge](https://togithub.com/xconverge) in [https://github.com/nodejs/undici/pull/1889](https://togithub.com/nodejs/undici/pull/1889)
-   fix(types): update error type definitions by [@&#8203;rafaelcr](https://togithub.com/rafaelcr) in [https://github.com/nodejs/undici/pull/1888](https://togithub.com/nodejs/undici/pull/1888)
-   fix: ensure connection header is a string by [@&#8203;ronag](https://togithub.com/ronag) in [https://github.com/nodejs/undici/pull/1900](https://togithub.com/nodejs/undici/pull/1900)
-   fix: throw if invalid content-type header by [@&#8203;ronag](https://togithub.com/ronag) in [https://github.com/nodejs/undici/pull/1901](https://togithub.com/nodejs/undici/pull/1901)
-   fix(fetch): use semicolon for Cookie header delimiter by [@&#8203;KhafraDev](https://togithub.com/KhafraDev) in [https://github.com/nodejs/undici/pull/1906](https://togithub.com/nodejs/undici/pull/1906)
-   Use FastBuffer by [@&#8203;ronag](https://togithub.com/ronag) in [https://github.com/nodejs/undici/pull/1907](https://togithub.com/nodejs/undici/pull/1907)

#### New Contributors

-   [@&#8203;pan93412](https://togithub.com/pan93412) made their first contribution in [https://github.com/nodejs/undici/pull/1896](https://togithub.com/nodejs/undici/pull/1896)
-   [@&#8203;rafaelcr](https://togithub.com/rafaelcr) made their first contribution in [https://github.com/nodejs/undici/pull/1888](https://togithub.com/nodejs/undici/pull/1888)

**Full Changelog**: nodejs/undici@v5.16.0...v5.17.0

### [`v5.16.0`](https://togithub.com/nodejs/undici/releases/tag/v5.16.0)

[Compare Source](https://togithub.com/nodejs/undici/compare/v5.15.2...v5.16.0)

#### What's Changed

-   Add feature to specify custom headers for proxies by [@&#8203;Sebmaster](https://togithub.com/Sebmaster) in [https://github.com/nodejs/undici/pull/1877](https://togithub.com/nodejs/undici/pull/1877)

#### New Contributors

-   [@&#8203;Sebmaster](https://togithub.com/Sebmaster) made their first contribution in [https://github.com/nodejs/undici/pull/1877](https://togithub.com/nodejs/undici/pull/1877)

**Full Changelog**: nodejs/undici@v5.15.2...v5.16.0

### [`v5.15.2`](https://togithub.com/nodejs/undici/compare/9d5f23177408dc16d3d4cbb8cebf463081c54e16...9457c9719029945ef9ff36b71d58557443730942)

[Compare Source](https://togithub.com/nodejs/undici/compare/v5.15.1...v5.15.2)

### [`v5.15.1`](https://togithub.com/nodejs/undici/releases/tag/v5.15.1)

[Compare Source](https://togithub.com/nodejs/undici/compare/v5.15.0...v5.15.1)

#### What's Changed

-   fix(websocket): simplify typedarray copying by [@&#8203;KhafraDev](https://togithub.com/KhafraDev) in [https://github.com/nodejs/undici/pull/1854](https://togithub.com/nodejs/undici/pull/1854)
-   fix: wpts on node v18.13.0+ by [@&#8203;KhafraDev](https://togithub.com/KhafraDev) in [https://github.com/nodejs/undici/pull/1859](https://togithub.com/nodejs/undici/pull/1859)
-   perf: allow keep alive for HEAD requests by [@&#8203;ronag](https://togithub.com/ronag) in [https://github.com/nodejs/undici/pull/1858](https://togithub.com/nodejs/undici/pull/1858)
-   fix: flaky abort test by [@&#8203;KhafraDev](https://togithub.com/KhafraDev) in [https://github.com/nodejs/undici/pull/1863](https://togithub.com/nodejs/undici/pull/1863)

**Full Changelog**: nodejs/undici@v5.15.0...v5.15.1

### [`v5.15.0`](https://togithub.com/nodejs/undici/releases/tag/v5.15.0)

[Compare Source](https://togithub.com/nodejs/undici/compare/v5.14.0...v5.15.0)

#### What's Changed

-   \[types] update ProxyAgent Options (timeout) by [@&#8203;sosoba](https://togithub.com/sosoba) in [https://github.com/nodejs/undici/pull/1801](https://togithub.com/nodejs/undici/pull/1801)
-   feat: implement websockets by [@&#8203;KhafraDev](https://togithub.com/KhafraDev) in [https://github.com/nodejs/undici/pull/1795](https://togithub.com/nodejs/undici/pull/1795)
-   feat(websocket): handle ping/pong frames & fix fragmented frames by [@&#8203;KhafraDev](https://togithub.com/KhafraDev) in [https://github.com/nodejs/undici/pull/1809](https://togithub.com/nodejs/undici/pull/1809)
-   docs: add basic fetch & company docs by [@&#8203;KhafraDev](https://togithub.com/KhafraDev) in [https://github.com/nodejs/undici/pull/1810](https://togithub.com/nodejs/undici/pull/1810)
-   make formdata body immutable and encode it only once by [@&#8203;jimmywarting](https://togithub.com/jimmywarting) in [https://github.com/nodejs/undici/pull/1814](https://togithub.com/nodejs/undici/pull/1814)
-   test: add regression test for [#&#8203;1814](https://togithub.com/nodejs/undici/issues/1814) by [@&#8203;KhafraDev](https://togithub.com/KhafraDev) in [https://github.com/nodejs/undici/pull/1815](https://togithub.com/nodejs/undici/pull/1815)
-   feat(websocket): only consume necessary bytes by [@&#8203;KhafraDev](https://togithub.com/KhafraDev) in [https://github.com/nodejs/undici/pull/1812](https://togithub.com/nodejs/undici/pull/1812)
-   websocket: use Buffer.allocUnsafe by [@&#8203;KhafraDev](https://togithub.com/KhafraDev) in [https://github.com/nodejs/undici/pull/1817](https://togithub.com/nodejs/undici/pull/1817)
-   build(deps-dev): bump [@&#8203;sinonjs/fake-timers](https://togithub.com/sinonjs/fake-timers) from 9.1.2 to 10.0.2 by [@&#8203;dependabot](https://togithub.com/dependabot) in [https://github.com/nodejs/undici/pull/1819](https://togithub.com/nodejs/undici/pull/1819)
-   fix(websocket): deprecation warning & 64-bit unsigned int body length by [@&#8203;KhafraDev](https://togithub.com/KhafraDev) in [https://github.com/nodejs/undici/pull/1818](https://togithub.com/nodejs/undici/pull/1818)
-   Use nodejs.stream.destroyed symbol by [@&#8203;ronag](https://togithub.com/ronag) in [https://github.com/nodejs/undici/pull/1816](https://togithub.com/nodejs/undici/pull/1816)
-   fetch: removal of redundant condition by [@&#8203;debadree25](https://togithub.com/debadree25) in [https://github.com/nodejs/undici/pull/1821](https://togithub.com/nodejs/undici/pull/1821)
-   fix(request): request headers array by [@&#8203;jd-carroll](https://togithub.com/jd-carroll) in [https://github.com/nodejs/undici/pull/1807](https://togithub.com/nodejs/undici/pull/1807)
-   fix(websocket): validate payload length received by [@&#8203;KhafraDev](https://togithub.com/KhafraDev) in [https://github.com/nodejs/undici/pull/1822](https://togithub.com/nodejs/undici/pull/1822)
-   fix(websocket): run parser in loop, instead of recursively by [@&#8203;KhafraDev](https://togithub.com/KhafraDev) in [https://github.com/nodejs/undici/pull/1828](https://togithub.com/nodejs/undici/pull/1828)
-   fix(fetch): weaker refs by [@&#8203;ronag](https://togithub.com/ronag) in [https://github.com/nodejs/undici/pull/1824](https://togithub.com/nodejs/undici/pull/1824)
-   websocket: add tests for opening handshake by [@&#8203;KhafraDev](https://togithub.com/KhafraDev) in [https://github.com/nodejs/undici/pull/1831](https://togithub.com/nodejs/undici/pull/1831)
-   websocket: add tests for constructor, close, and send by [@&#8203;KhafraDev](https://togithub.com/KhafraDev) in [https://github.com/nodejs/undici/pull/1832](https://togithub.com/nodejs/undici/pull/1832)
-   websocket: more test coverage by [@&#8203;KhafraDev](https://togithub.com/KhafraDev) in [https://github.com/nodejs/undici/pull/1833](https://togithub.com/nodejs/undici/pull/1833)
-   fix(WPTs): flaky abort test by [@&#8203;KhafraDev](https://togithub.com/KhafraDev) in [https://github.com/nodejs/undici/pull/1835](https://togithub.com/nodejs/undici/pull/1835)
-   wpt: add test by [@&#8203;KhafraDev](https://togithub.com/KhafraDev) in [https://github.com/nodejs/undici/pull/1836](https://togithub.com/nodejs/undici/pull/1836)
-   fix: don't send keep-alive if we want reset by [@&#8203;ronag](https://togithub.com/ronag) in [https://github.com/nodejs/undici/pull/1846](https://togithub.com/nodejs/undici/pull/1846)
-   fetch: update body consume to match spec by [@&#8203;KhafraDev](https://togithub.com/KhafraDev) in [https://github.com/nodejs/undici/pull/1847](https://togithub.com/nodejs/undici/pull/1847)
-   feat: allow connection header in request by [@&#8203;metcoder95](https://togithub.com/metcoder95) in [https://github.com/nodejs/undici/pull/1829](https://togithub.com/nodejs/undici/pull/1829)
-   feat: add cookie parsing ability by [@&#8203;KhafraDev](https://togithub.com/KhafraDev) in [https://github.com/nodejs/undici/pull/1848](https://togithub.com/nodejs/undici/pull/1848)
-   fix(cookie): add docs & expose in node v16 by [@&#8203;KhafraDev](https://togithub.com/KhafraDev) in [https://github.com/nodejs/undici/pull/1849](https://togithub.com/nodejs/undici/pull/1849)
-   fix(cookies): work with global Headers by [@&#8203;KhafraDev](https://togithub.com/KhafraDev) in [https://github.com/nodejs/undici/pull/1850](https://togithub.com/nodejs/undici/pull/1850)
-   docs(Dispatcher): adjust documentation for reset flag by [@&#8203;metcoder95](https://togithub.com/metcoder95) in [https://github.com/nodejs/undici/pull/1852](https://togithub.com/nodejs/undici/pull/1852)
-   Fix broken interceptor test by [@&#8203;mcollina](https://togithub.com/mcollina) in [https://github.com/nodejs/undici/pull/1853](https://togithub.com/nodejs/undici/pull/1853)

#### New Contributors

-   [@&#8203;sosoba](https://togithub.com/sosoba) made their first contribution in [https://github.com/nodejs/undici/pull/1801](https://togithub.com/nodejs/undici/pull/1801)
-   [@&#8203;debadree25](https://togithub.com/debadree25) made their first contribution in [https://github.com/nodejs/undici/pull/1821](https://togithub.com/nodejs/undici/pull/1821)
-   [@&#8203;jd-carroll](https://togithub.com/jd-carroll) made their first contribution in [https://github.com/nodejs/undici/pull/1807](https://togithub.com/nodejs/undici/pull/1807)

**Full Changelog**: nodejs/undici@v5.14.0...v5.15.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - "" (UTC), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/sammyfilly/Canary-nextjs).
crysmags pushed a commit to crysmags/undici that referenced this pull request Feb 27, 2024
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.

4 participants