fix(request): request headers array #1807

jd-carroll · 2022-12-15T17:02:46Z

Fix for #1806

KhafraDev

Can you add tests using request? Fetch already technically supported arrays of headers due to webidl.

jd-carroll · 2022-12-15T18:34:14Z

Happy to, to be honest wasn't really sure those were the correct tests in the first place (but wanted something).

Could you point me to a good place to add them?

KhafraDev · 2022-12-15T18:36:55Z

https://github.com/nodejs/undici/blob/main/test/headers-as-array.js Would be fine

jd-carroll · 2022-12-15T18:48:00Z

@KhafraDev thanks, added

jd-carroll · 2022-12-15T18:59:24Z

Seeing the test failures, not exactly sure what is going on. I was getting all kinds of failure locally (from unrelated code).

Do I need to look into anything?

KhafraDev · 2022-12-16T05:00:04Z

The failures are likely caused by the change. Locally no errors should occur either.

jd-carroll · 2022-12-17T06:18:05Z

Tests should be all set, though I was having a problem committing locally (error code 13?)

Turns out a number of the tests (incorrectly?) use numbers for header values and not strings. While this is against the "types", my guess is that many people are already doing this, so best not to break them.

lib/core/request.js

codecov-commenter · 2022-12-17T15:20:39Z

Codecov Report

Base: 82.95% // Head: 87.30% // Increases project coverage by +4.34% 🎉

Coverage data is based on head (c0170bb) compared to base (7e12397).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1807      +/-   ##
==========================================
+ Coverage   82.95%   87.30%   +4.34%     
==========================================
  Files          66       66              
  Lines        5755     5796      +41     
==========================================
+ Hits         4774     5060     +286     
+ Misses        981      736     -245

Impacted Files	Coverage Δ
lib/core/request.js	`100.00% <100.00%> (ø)`
lib/fetch/body.js	`96.53% <0.00%> (ø)`
lib/fetch/util.js	`86.52% <0.00%> (+0.86%)`	⬆️
lib/fetch/webidl.js	`93.93% <0.00%> (+1.01%)`	⬆️
lib/fetch/index.js	`84.65% <0.00%> (+2.52%)`	⬆️
lib/websocket/events.js	`50.84% <0.00%> (+28.81%)`	⬆️
lib/websocket/websocket.js	`42.16% <0.00%> (+30.81%)`	⬆️
lib/websocket/util.js	`54.05% <0.00%> (+43.24%)`	⬆️
lib/websocket/connection.js	`73.11% <0.00%> (+55.22%)`	⬆️
lib/websocket/frame.js	`80.00% <0.00%> (+70.00%)`	⬆️
... and 1 more

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.

lib/core/request.js

jd-carroll · 2022-12-20T18:48:38Z

Anything else either of you would like changed?

KhafraDev · 2022-12-20T18:55:23Z

test/headers-as-array.js

+  const headers = { a: '1', b: '2', c: '3', d: ['4', '5'] }
+
+  const server = createServer((req, res) => {
+    t.match(req.headers, { a: '1', b: '2', c: '3', d: '4, 5' })


One thing to note: this behaves differently from fetch. Fetch's webidl checks will stringify the array, giving us 4,5 (no space), but node's http parser joins them with , , giving us 4, 5 (with space).

Is this something you think needs to be fixed / changed?

No, just wanted to point it out since it might have been missed in the original review. 👍

* fix(request): request headers arry * Add more tests specific to request * Allow all primitive types * Object and not array * Check values in array

[![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 [@KhafraDev](https://togithub.com/KhafraDev) in [nodejs/undici#1910 - fix: content-disposition header parsing by [@climba03003](https://togithub.com/climba03003) in [nodejs/undici#1911 - fix: remove test by [@KhafraDev](https://togithub.com/KhafraDev) in [nodejs/undici#1916 - feat: add Headers.prototype.getSetCookie by [@KhafraDev](https://togithub.com/KhafraDev) in [nodejs/undici#1915 - fix(headers): clone getSetCookie list & add getSetCookie type by [@KhafraDev](https://togithub.com/KhafraDev) in [nodejs/undici#1917 - doc(mock): update out-of-date reply documentation by [@p9f](https://togithub.com/p9f) in [nodejs/undici#1913 - fix(types): add missing keepAlive params by [@SkeLLLa](https://togithub.com/SkeLLLa) in [nodejs/undici#1918 - Make the fetch() abort test pass locally, on Linux and Mac, Node 18/19. by [@mcollina](https://togithub.com/mcollina) in [nodejs/undici#1927 #### New Contributors - [@climba03003](https://togithub.com/climba03003) made their first contribution in [nodejs/undici#1911 - [@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 [@xconverge](https://togithub.com/xconverge) in [nodejs/undici#1904 - use faster timers by [@ronag](https://togithub.com/ronag) in [nodejs/undici#1908 - fix: ensure header value is a string by [@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 [@KhafraDev](https://togithub.com/KhafraDev) in [nodejs/undici#1880 - doc: fix anchor links dispatcher.stream by [@RafaelGSS](https://togithub.com/RafaelGSS) in [nodejs/undici#1881 - wpt: make runner more resilient by [@KhafraDev](https://togithub.com/KhafraDev) in [nodejs/undici#1884 - Make test pass in v19.x by [@mcollina](https://togithub.com/mcollina) in [nodejs/undici#1879 - Correct the type of DispatchOptions\["headers"] by [@pan93412](https://togithub.com/pan93412) in [nodejs/undici#1896 - perf(content-type parser): faster string collector by [@KhafraDev](https://togithub.com/KhafraDev) in [nodejs/undici#1894 - feat: expose content-type parser by [@KhafraDev](https://togithub.com/KhafraDev) in [nodejs/undici#1895 - fix(types): Update DispatchOptions type for missing "blocking" by [@xconverge](https://togithub.com/xconverge) in [nodejs/undici#1889 - fix(types): update error type definitions by [@rafaelcr](https://togithub.com/rafaelcr) in [nodejs/undici#1888 - fix: ensure connection header is a string by [@ronag](https://togithub.com/ronag) in [nodejs/undici#1900 - fix: throw if invalid content-type header by [@ronag](https://togithub.com/ronag) in [nodejs/undici#1901 - fix(fetch): use semicolon for Cookie header delimiter by [@KhafraDev](https://togithub.com/KhafraDev) in [nodejs/undici#1906 - Use FastBuffer by [@ronag](https://togithub.com/ronag) in [nodejs/undici#1907 #### New Contributors - [@pan93412](https://togithub.com/pan93412) made their first contribution in [nodejs/undici#1896 - [@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 [@Sebmaster](https://togithub.com/Sebmaster) in [nodejs/undici#1877 #### New Contributors - [@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 [@KhafraDev](https://togithub.com/KhafraDev) in [nodejs/undici#1854 - fix: wpts on node v18.13.0+ by [@KhafraDev](https://togithub.com/KhafraDev) in [nodejs/undici#1859 - perf: allow keep alive for HEAD requests by [@ronag](https://togithub.com/ronag) in [nodejs/undici#1858 - fix: flaky abort test by [@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 [@sosoba](https://togithub.com/sosoba) in [nodejs/undici#1801 - feat: implement websockets by [@KhafraDev](https://togithub.com/KhafraDev) in [nodejs/undici#1795 - feat(websocket): handle ping/pong frames & fix fragmented frames by [@KhafraDev](https://togithub.com/KhafraDev) in [nodejs/undici#1809 - docs: add basic fetch & company docs by [@KhafraDev](https://togithub.com/KhafraDev) in [nodejs/undici#1810 - make formdata body immutable and encode it only once by [@jimmywarting](https://togithub.com/jimmywarting) in [nodejs/undici#1814 - test: add regression test for [#1814](https://togithub.com/nodejs/undici/issues/1814) by [@KhafraDev](https://togithub.com/KhafraDev) in [nodejs/undici#1815 - feat(websocket): only consume necessary bytes by [@KhafraDev](https://togithub.com/KhafraDev) in [nodejs/undici#1812 - websocket: use Buffer.allocUnsafe by [@KhafraDev](https://togithub.com/KhafraDev) in [nodejs/undici#1817 - build(deps-dev): bump [@sinonjs/fake-timers](https://togithub.com/sinonjs/fake-timers) from 9.1.2 to 10.0.2 by [@dependabot](https://togithub.com/dependabot) in [nodejs/undici#1819 - fix(websocket): deprecation warning & 64-bit unsigned int body length by [@KhafraDev](https://togithub.com/KhafraDev) in [nodejs/undici#1818 - Use nodejs.stream.destroyed symbol by [@ronag](https://togithub.com/ronag) in [nodejs/undici#1816 - fetch: removal of redundant condition by [@debadree25](https://togithub.com/debadree25) in [nodejs/undici#1821 - fix(request): request headers array by [@jd-carroll](https://togithub.com/jd-carroll) in [nodejs/undici#1807 - fix(websocket): validate payload length received by [@KhafraDev](https://togithub.com/KhafraDev) in [nodejs/undici#1822 - fix(websocket): run parser in loop, instead of recursively by [@KhafraDev](https://togithub.com/KhafraDev) in [nodejs/undici#1828 - fix(fetch): weaker refs by [@ronag](https://togithub.com/ronag) in [nodejs/undici#1824 - websocket: add tests for opening handshake by [@KhafraDev](https://togithub.com/KhafraDev) in [nodejs/undici#1831 - websocket: add tests for constructor, close, and send by [@KhafraDev](https://togithub.com/KhafraDev) in [nodejs/undici#1832 - websocket: more test coverage by [@KhafraDev](https://togithub.com/KhafraDev) in [nodejs/undici#1833 - fix(WPTs): flaky abort test by [@KhafraDev](https://togithub.com/KhafraDev) in [nodejs/undici#1835 - wpt: add test by [@KhafraDev](https://togithub.com/KhafraDev) in [nodejs/undici#1836 - fix: don't send keep-alive if we want reset by [@ronag](https://togithub.com/ronag) in [nodejs/undici#1846 - fetch: update body consume to match spec by [@KhafraDev](https://togithub.com/KhafraDev) in [nodejs/undici#1847 - feat: allow connection header in request by [@metcoder95](https://togithub.com/metcoder95) in [nodejs/undici#1829 - feat: add cookie parsing ability by [@KhafraDev](https://togithub.com/KhafraDev) in [nodejs/undici#1848 - fix(cookie): add docs & expose in node v16 by [@KhafraDev](https://togithub.com/KhafraDev) in [nodejs/undici#1849 - fix(cookies): work with global Headers by [@KhafraDev](https://togithub.com/KhafraDev) in [nodejs/undici#1850 - docs(Dispatcher): adjust documentation for reset flag by [@metcoder95](https://togithub.com/metcoder95) in [nodejs/undici#1852 - Fix broken interceptor test by [@mcollina](https://togithub.com/mcollina) in [nodejs/undici#1853 #### New Contributors - [@sosoba](https://togithub.com/sosoba) made their first contribution in [nodejs/undici#1801 - [@debadree25](https://togithub.com/debadree25) made their first contribution in [nodejs/undici#1821 - [@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).

* fix(request): request headers arry * Add more tests specific to request * Allow all primitive types * Object and not array * Check values in array

fix(request): request headers arry

57ade72

ronag approved these changes Dec 15, 2022

View reviewed changes

KhafraDev requested changes Dec 15, 2022

View reviewed changes

Add more tests specific to request

5c06a84

jd-carroll changed the title ~~fix(request): request headers arry~~ fix(request): request headers array Dec 15, 2022

jd-carroll added 2 commits December 16, 2022 23:47

Merge branch 'main' of https://github.com/nodejs/undici

6c4e6a2

Allow all primitive types

7e84a48

ronag requested changes Dec 17, 2022

View reviewed changes

lib/core/request.js Outdated Show resolved Hide resolved

Object and not array

c0170bb

ronag approved these changes Dec 19, 2022

View reviewed changes

ronag reviewed Dec 19, 2022

View reviewed changes

lib/core/request.js Show resolved Hide resolved

Check values in array

84ff9bd

KhafraDev reviewed Dec 20, 2022

View reviewed changes

ronag approved these changes Dec 20, 2022

View reviewed changes

KhafraDev merged commit c70b9f0 into nodejs:main Dec 20, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(request): request headers array #1807

fix(request): request headers array #1807

jd-carroll commented Dec 15, 2022

KhafraDev left a comment

jd-carroll commented Dec 15, 2022

KhafraDev commented Dec 15, 2022

jd-carroll commented Dec 15, 2022

jd-carroll commented Dec 15, 2022

KhafraDev commented Dec 16, 2022

jd-carroll commented Dec 17, 2022

codecov-commenter commented Dec 17, 2022 •

edited

jd-carroll commented Dec 20, 2022

KhafraDev Dec 20, 2022

jd-carroll Dec 20, 2022

KhafraDev Dec 20, 2022

fix(request): request headers array #1807

fix(request): request headers array #1807

Conversation

jd-carroll commented Dec 15, 2022

KhafraDev left a comment

Choose a reason for hiding this comment

jd-carroll commented Dec 15, 2022

KhafraDev commented Dec 15, 2022

jd-carroll commented Dec 15, 2022

jd-carroll commented Dec 15, 2022

KhafraDev commented Dec 16, 2022

jd-carroll commented Dec 17, 2022

codecov-commenter commented Dec 17, 2022 • edited

Codecov Report

jd-carroll commented Dec 20, 2022

KhafraDev Dec 20, 2022

Choose a reason for hiding this comment

jd-carroll Dec 20, 2022

Choose a reason for hiding this comment

KhafraDev Dec 20, 2022

Choose a reason for hiding this comment

codecov-commenter commented Dec 17, 2022 •

edited