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

fetch: removal of redundant condition #1821

Merged
merged 1 commit into from
Dec 20, 2022

Conversation

debadree25
Copy link
Member

Removed a redundant condition as per the issue

Fixes: #1813

Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

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

How is it redundant?

@@ -171,11 +171,6 @@ function extractBody (object, keepalive = false) {
if (object.type) {
type = object.type
}
} else if (object instanceof Uint8Array) {
Copy link
Member

Choose a reason for hiding this comment

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

@KhafraDev what is byte sequence here, actually? I think the condition might actually be intended as something else.

Copy link
Member

Choose a reason for hiding this comment

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

A byte sequence is a webidl term that doesn't really have a javascript representation -- at least, not a single one. The actual definition is "a sequence of bytes", which, for us, could mean 2 things: an array of bytes, or a Uint8Array.

For example, both could technically be a "byte sequence":

const byteSequence = [0x41, 0x42, 0x43] // ABC

const byteSequence2 = new Uint8Array([0x41, 0x42, 0x43])

An array isn't a valid body, and is converted to a DOMString (becoming 65,66,67) and a Uint8Array is handled in the ArrayBuffer.isView check as mentioned in the issue.


According to codecov, this condition is never reached (as expected).
image

Copy link
Contributor

Choose a reason for hiding this comment

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

I was also confused what a byte sequence meant in the beginning when i started working on Blob.js polyfill and also fetch-blob. My earlier version of blobs was solo based on having a ArrayBuffer to back up all the data that it represent.

Later learned that byte sequence could meant anything that isn't a javascript representation.
And fyi file/blobs are not only made up by things stored in memory.
A blob part can be represented by a file on the user hard drive too.
it wasn't until a saw the 3rd slide in https://docs.google.com/presentation/d/1MOm-8kacXAon1L2tF6VthesNjXgx0fp5AP17L7XDPSM/htmlpresent that i truly started to understand what Blobs really are good for.
So when you slice a blob then all you are really doing is changing the read offset and the size property but keeps a references to the old blob so you know where you should start reading from and when you should stop reading.

So doing things like new Blob([ fileA, fileB ]) dose not mean that it will allocate any memory for both file a and b.
you just tell it to read stuff from fileA then start reading the content from fileB in one single stream.

the blobParts in blob constructor is also a byte sequence

chrome have a very nice design system of how their blobs are made up and how they work under the hood
https://chromium.googlesource.com/chromium/src/+/HEAD/storage/browser/blob/README.md

I have later gotten much more appreciation of blobs after that.

I have for example built a zip parser that only read the central directory at the end of a blob (zip), then figure out where the content of a file start/ends and then just simply slice the zip and hand it back. it's such a beautiful design that makes it possible to never ever have to read anything into memory. it's sooo browser/RAM friendly and also way much faster than any other zip reader.

Copy link
Member

Choose a reason for hiding this comment

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

A byte sequence is a sequence of bytes

The byte type is a signed integer type that has values in the range [−128, 127].

... and to convert a value to a byte:

Let x be ? ConvertToInt(V, 8, "signed").

Correcting myself earlier: a Uint8Array cannot be a valid byte sequence. The only typed array that can be a valid byte sequence is an Int8Array.

The blobParts parameter is not a byte sequence. It allows Blobs, ArrayBufferViews (typed arrays and DataView), ArrayBuffer, and strings.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, yea. you are right. blobparts isn't exactly a byte sequence, but they are turned into a byte sequences in the constructor.

guess that is what it means by: Set value’s underlying byte sequence to serialized.[[ByteSequence]].

https://w3c.github.io/FileAPI/#dfn-BlobPropertyBagMembers

1 Let bytes be an empty sequence of bytes.
2, 3 If element is a Blob, append the bytes it represents to bytes.

lite off topic but a fun discussion non the less.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, everything is converted to a byte sequence:

const uint8 = new Uint8Array([0xFF])
const blob = new Blob([uint8])
const text = await blob.text()

text.charCodeAt(0) // \uFFFD

@ronag ronag merged commit fcc1698 into nodejs:main Dec 20, 2022
anonrig pushed a commit to anonrig/undici that referenced this pull request Apr 4, 2023
kodiakhq bot pushed a commit to X-oss-byte/Canary-nextjs that referenced this pull request 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 ([@​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 [nodejs/undici#1910
-   fix: content-disposition header parsing by [@&#8203;climba03003](https://togithub.com/climba03003) in [nodejs/undici#1911
-   fix: remove test by [@&#8203;KhafraDev](https://togithub.com/KhafraDev) in [nodejs/undici#1916
-   feat: add Headers.prototype.getSetCookie by [@&#8203;KhafraDev](https://togithub.com/KhafraDev) in [nodejs/undici#1915
-   fix(headers): clone getSetCookie list & add getSetCookie type by [@&#8203;KhafraDev](https://togithub.com/KhafraDev) in [nodejs/undici#1917
-   doc(mock): update out-of-date reply documentation by [@&#8203;p9f](https://togithub.com/p9f) in [nodejs/undici#1913
-   fix(types): add missing keepAlive params by [@&#8203;SkeLLLa](https://togithub.com/SkeLLLa) in [nodejs/undici#1918
-   Make the fetch() abort test pass locally, on Linux and Mac, Node 18/19. by [@&#8203;mcollina](https://togithub.com/mcollina) in [nodejs/undici#1927

#### New Contributors

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

#### New Contributors

-   [@&#8203;pan93412](https://togithub.com/pan93412) made their first contribution in [nodejs/undici#1896
-   [@&#8203;rafaelcr](https://togithub.com/rafaelcr) made their first contribution in [nodejs/undici#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 [nodejs/undici#1877

#### New Contributors

-   [@&#8203;Sebmaster](https://togithub.com/Sebmaster) made their first contribution in [nodejs/undici#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 [nodejs/undici#1854
-   fix: wpts on node v18.13.0+ by [@&#8203;KhafraDev](https://togithub.com/KhafraDev) in [nodejs/undici#1859
-   perf: allow keep alive for HEAD requests by [@&#8203;ronag](https://togithub.com/ronag) in [nodejs/undici#1858
-   fix: flaky abort test by [@&#8203;KhafraDev](https://togithub.com/KhafraDev) in [nodejs/undici#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 [nodejs/undici#1801
-   feat: implement websockets by [@&#8203;KhafraDev](https://togithub.com/KhafraDev) in [nodejs/undici#1795
-   feat(websocket): handle ping/pong frames & fix fragmented frames by [@&#8203;KhafraDev](https://togithub.com/KhafraDev) in [nodejs/undici#1809
-   docs: add basic fetch & company docs by [@&#8203;KhafraDev](https://togithub.com/KhafraDev) in [nodejs/undici#1810
-   make formdata body immutable and encode it only once by [@&#8203;jimmywarting](https://togithub.com/jimmywarting) in [nodejs/undici#1814
-   test: add regression test for [#&#8203;1814](https://togithub.com/nodejs/undici/issues/1814) by [@&#8203;KhafraDev](https://togithub.com/KhafraDev) in [nodejs/undici#1815
-   feat(websocket): only consume necessary bytes by [@&#8203;KhafraDev](https://togithub.com/KhafraDev) in [nodejs/undici#1812
-   websocket: use Buffer.allocUnsafe by [@&#8203;KhafraDev](https://togithub.com/KhafraDev) in [nodejs/undici#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 [nodejs/undici#1819
-   fix(websocket): deprecation warning & 64-bit unsigned int body length by [@&#8203;KhafraDev](https://togithub.com/KhafraDev) in [nodejs/undici#1818
-   Use nodejs.stream.destroyed symbol by [@&#8203;ronag](https://togithub.com/ronag) in [nodejs/undici#1816
-   fetch: removal of redundant condition by [@&#8203;debadree25](https://togithub.com/debadree25) in [nodejs/undici#1821
-   fix(request): request headers array by [@&#8203;jd-carroll](https://togithub.com/jd-carroll) in [nodejs/undici#1807
-   fix(websocket): validate payload length received by [@&#8203;KhafraDev](https://togithub.com/KhafraDev) in [nodejs/undici#1822
-   fix(websocket): run parser in loop, instead of recursively by [@&#8203;KhafraDev](https://togithub.com/KhafraDev) in [nodejs/undici#1828
-   fix(fetch): weaker refs by [@&#8203;ronag](https://togithub.com/ronag) in [nodejs/undici#1824
-   websocket: add tests for opening handshake by [@&#8203;KhafraDev](https://togithub.com/KhafraDev) in [nodejs/undici#1831
-   websocket: add tests for constructor, close, and send by [@&#8203;KhafraDev](https://togithub.com/KhafraDev) in [nodejs/undici#1832
-   websocket: more test coverage by [@&#8203;KhafraDev](https://togithub.com/KhafraDev) in [nodejs/undici#1833
-   fix(WPTs): flaky abort test by [@&#8203;KhafraDev](https://togithub.com/KhafraDev) in [nodejs/undici#1835
-   wpt: add test by [@&#8203;KhafraDev](https://togithub.com/KhafraDev) in [nodejs/undici#1836
-   fix: don't send keep-alive if we want reset by [@&#8203;ronag](https://togithub.com/ronag) in [nodejs/undici#1846
-   fetch: update body consume to match spec by [@&#8203;KhafraDev](https://togithub.com/KhafraDev) in [nodejs/undici#1847
-   feat: allow connection header in request by [@&#8203;metcoder95](https://togithub.com/metcoder95) in [nodejs/undici#1829
-   feat: add cookie parsing ability by [@&#8203;KhafraDev](https://togithub.com/KhafraDev) in [nodejs/undici#1848
-   fix(cookie): add docs & expose in node v16 by [@&#8203;KhafraDev](https://togithub.com/KhafraDev) in [nodejs/undici#1849
-   fix(cookies): work with global Headers by [@&#8203;KhafraDev](https://togithub.com/KhafraDev) in [nodejs/undici#1850
-   docs(Dispatcher): adjust documentation for reset flag by [@&#8203;metcoder95](https://togithub.com/metcoder95) in [nodejs/undici#1852
-   Fix broken interceptor test by [@&#8203;mcollina](https://togithub.com/mcollina) in [nodejs/undici#1853

#### New Contributors

-   [@&#8203;sosoba](https://togithub.com/sosoba) made their first contribution in [nodejs/undici#1801
-   [@&#8203;debadree25](https://togithub.com/debadree25) made their first contribution in [nodejs/undici#1821
-   [@&#8203;jd-carroll](https://togithub.com/jd-carroll) made their first contribution in [nodejs/undici#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.

some unnecessary code?
4 participants