-
Notifications
You must be signed in to change notification settings - Fork 545
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
fix(fetch): implement fully read body algorithm #1597
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job!
@KhafraDev Feel free to copy over parts of #1596 that are useful and make that pull request redundant. |
Codecov Report
@@ Coverage Diff @@
## main #1597 +/- ##
==========================================
- Coverage 94.90% 94.69% -0.22%
==========================================
Files 50 50
Lines 4732 4746 +14
==========================================
+ Hits 4491 4494 +3
- Misses 241 252 +11
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Bumps undici from 5.8.0 to 5.9.1. Release notes Sourced from undici's releases. v5.9.1 What's Changed fix: don't timeout while waiting for client to send request (#1604) Fix array headers by @mateonunez in nodejs/undici#1598 fix(fetch): implement fully read body algorithm by @KhafraDev in nodejs/undici#1597 fix: add support for integrity option to Fetch by @jelmervdl in nodejs/undici#1596 fix(File): respect typed array byteOffset and byteLength by @mrbbot in nodejs/undici#1601 New Contributors @mateonunez made their first contribution in nodejs/undici#1598 @jelmervdl made their first contribution in nodejs/undici#1596 @mrbbot made their first contribution in nodejs/undici#1601 Full Changelog: nodejs/undici@v5.8.2...v5.9.1 v5.8.2⚠️ Security Release⚠️ CRLF Injection in Nodejs ‘undici’ via Content-Type GHSA-f772-66g8-q5h3 CVE-2022-35948 undici.request vulnerable to SSRF using absolute URL on pathname GHSA-8qr4-xgw6-wmr3 CVE-2022-35949 What's Changed docs: mock different endpoints in a single file by @ritvik130 in nodejs/undici#1589 feat(webidl): better error message for ByteString converter by @KhafraDev in nodejs/undici#1591 New Contributors @ritvik130 made their first contribution in nodejs/undici#1589 Full Changelog: nodejs/undici@v5.8.1...v5.8.2 v5.8.1 What's Changed Do not decode the body while we are following a redirect by @mcollina in nodejs/undici#1554 docs: Fix spelling/grammar in "Mocking Request" by @meyfa in nodejs/undici#1555 fix(MockInterceptor): callback options.headers w/ fetch by @KhafraDev in nodejs/undici#1559 fix(fetch): ByteString checks & conversion in Headers by @KhafraDev in nodejs/undici#1560 test: update client certificates with ones that expires in 100 years by @jodevsa in nodejs/undici#1566 fix: x-www-form-urlencoded parser keep the BOM by @cola119 in nodejs/undici#1563 fix: prioritise error events over timeouts by @jodevsa in nodejs/undici#1551 fix: add isErrorLike by @KhafraDev in nodejs/undici#1570 fix(types): add missing pool stats by @SkeLLLa in nodejs/undici#1573 fix: fetch a long base64 url will crash and nothing happens (close: #1574) by @ahaoboy in nodejs/undici#1575 fix: follow signal.reason in Request by @LiviaMedeiros in nodejs/undici#1580 docs: Fix DiagnosticsChannel sidebar link by @trentm in nodejs/undici#1582 fix: make mock headers case-insensitive by @cola119 in nodejs/undici#1585 New Contributors @meyfa made their first contribution in nodejs/undici#1555 @cola119 made their first contribution in nodejs/undici#1563 @SkeLLLa made their first contribution in nodejs/undici#1573 ... (truncated) Commits 5890e16 5.9.1 ecae314 fix: don't timeout while waiting for client to send request (#1604) fa9fd90 fix(File): respect typed array byteOffset and byteLength (#1601) ae6f554 fix: add support for integrity option to Fetch (#1596) deed628 fix(fetch): implement fully read body algorithm (#1597) 0d1419c Fix array headers (#1598) 52d1ce5 Bumped v5.8.2 66165d6 Merge pull request from GHSA-f772-66g8-q5h3 124f7eb Merge pull request from GHSA-8qr4-xgw6-wmr3 aef314c feat(webidl): better error message for ByteString converter (#1591) Additional commits viewable in compare view Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting @dependabot rebase. Dependabot commands and options You can trigger Dependabot actions by commenting on this PR: @dependabot rebase will rebase this PR @dependabot recreate will recreate this PR, overwriting any edits that have been made to it @dependabot merge will merge this PR after your CI passes on it @dependabot squash and merge will squash and merge this PR after your CI passes on it @dependabot cancel merge will cancel a previously requested merge and block automerging @dependabot reopen will reopen this PR if it is closed @dependabot close will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually @dependabot ignore this major version will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) @dependabot ignore this minor version will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) @dependabot ignore this dependency will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) You can disable automated security fix PRs for this repo from the Security Alerts page. Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps undici from 5.8.1 to 5.9.1. Release notes Sourced from undici's releases. v5.9.1 What's Changed fix: don't timeout while waiting for client to send request (#1604) Fix array headers by @mateonunez in nodejs/undici#1598 fix(fetch): implement fully read body algorithm by @KhafraDev in nodejs/undici#1597 fix: add support for integrity option to Fetch by @jelmervdl in nodejs/undici#1596 fix(File): respect typed array byteOffset and byteLength by @mrbbot in nodejs/undici#1601 New Contributors @mateonunez made their first contribution in nodejs/undici#1598 @jelmervdl made their first contribution in nodejs/undici#1596 @mrbbot made their first contribution in nodejs/undici#1601 Full Changelog: nodejs/undici@v5.8.2...v5.9.1 v5.8.2⚠️ Security Release⚠️ CRLF Injection in Nodejs ‘undici’ via Content-Type GHSA-f772-66g8-q5h3 CVE-2022-35948 undici.request vulnerable to SSRF using absolute URL on pathname GHSA-8qr4-xgw6-wmr3 CVE-2022-35949 What's Changed docs: mock different endpoints in a single file by @ritvik130 in nodejs/undici#1589 feat(webidl): better error message for ByteString converter by @KhafraDev in nodejs/undici#1591 New Contributors @ritvik130 made their first contribution in nodejs/undici#1589 Full Changelog: nodejs/undici@v5.8.1...v5.8.2 Commits 5890e16 5.9.1 ecae314 fix: don't timeout while waiting for client to send request (#1604) fa9fd90 fix(File): respect typed array byteOffset and byteLength (#1601) ae6f554 fix: add support for integrity option to Fetch (#1596) deed628 fix(fetch): implement fully read body algorithm (#1597) 0d1419c Fix array headers (#1598) 52d1ce5 Bumped v5.8.2 66165d6 Merge pull request from GHSA-f772-66g8-q5h3 124f7eb Merge pull request from GHSA-8qr4-xgw6-wmr3 aef314c feat(webidl): better error message for ByteString converter (#1591) Additional commits viewable in compare view Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting @dependabot rebase. Dependabot commands and options You can trigger Dependabot actions by commenting on this PR: @dependabot rebase will rebase this PR @dependabot recreate will recreate this PR, overwriting any edits that have been made to it @dependabot merge will merge this PR after your CI passes on it @dependabot squash and merge will squash and merge this PR after your CI passes on it @dependabot cancel merge will cancel a previously requested merge and block automerging @dependabot reopen will reopen this PR if it is closed @dependabot close will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually @dependabot ignore this major version will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) @dependabot ignore this minor version will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) @dependabot ignore this dependency will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) @dependabot use these labels will set the current labels as the default for future PRs for this repo and language @dependabot use these reviewers will set the current reviewers as the default for future PRs for this repo and language @dependabot use these assignees will set the current assignees as the default for future PRs for this repo and language @dependabot use this milestone will set the current milestone as the default for future PRs for this repo and language You can disable automated security fix PRs for this repo from the Security Alerts page.
This fixes the incorrect error that is thrown in #1594; it will now throw an integrity-mismatch check instead because it hasn't been implemented yet.
This is a weird case where neither uses of
fully read body
had code coverage and I still can't add a test to this area of the code because an error is still being thrown. I did include a test that worked on my end by removing the integrity checks, so once it is implemented the test can be enabled & should run without an issue. I would implement this myself, but there is already a WIP PR at #1596.In either case, the
response
object referenced in these locations is not a WHATWG Response, but an internal representation of a response. It doesn't include the body mixin, which means calling.arrayBuffer()
on it was throwing an error (edit: also calling.arrayBuffer()
on a Readable web stream throws an error). This fixes that by implementing the algorithm in a more... efficient way than the spec while also still remaining compliant w/ the spec.