fix: reject cross-realm FormData request bodies#5521
Conversation
KhafraDev
left a comment
There was a problem hiding this comment.
This shouldn't be under web/fetch, if this is something we want to support at all.
|
Moved it out of web/fetch in 5faa679: On supporting it at all: the h1/h2 clients already accept any FormData-like through |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5521 +/- ##
==========================================
- Coverage 93.44% 93.44% -0.01%
==========================================
Files 110 111 +1
Lines 37329 37367 +38
==========================================
+ Hits 34883 34918 +35
- Misses 2446 2449 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Is this something we want to support? We don't support this pattern anywhere else. Wouldn't it make more sense to just throw an error? |
Actually yes, it would be. |
5faa679 to
1bb4878
Compare
request() accepts FormData bodies through the duck-typed util.isFormDataLike() check, but only undici's own FormData class can be encoded: the multipart extraction is gated on the webidl brand check, so a FormData from another realm (Node.js' builtin globalThis.FormData, another undici copy, a polyfill) fell through and was dispatched as a body that never produced any bytes, hanging the request until an external timeout with zero bytes on the wire. Per review, reject such bodies instead of encoding them: the Request constructor now throws InvalidArgumentError when a FormData-like body is not an instance of undici's FormData, in the same place other invalid body types are rejected, so both the HTTP/1 and HTTP/2 clients reject cleanly at dispatch time. Note this is a behavior change relative to v6, which encoded cross-realm FormData successfully; v7.0.0 through 7.19.x failed these bodies with an unrelated TypeError and 7.20.0+ hung. Fixes: nodejs#5520 Signed-off-by: Pedro Cunha <pedroagracio@gmail.com>
1bb4878 to
5a0d56a
Compare
|
Reworked to reject, per the discussion: the |
Drop the lazy-loading and rationale comments (the error message and issue link carry the context), the tautological precondition assert, and the tests already covered elsewhere: own-FormData encoding is tested by test/client-request.js and test/http2-body.js, and the string-only case is identical to the file case under rejection. Signed-off-by: Pedro Cunha <pedroagracio@gmail.com>
This relates to...
Fixes #5520. Related context: #5500 / #5502 (legacy dispatcher bridge), #5016 and #4285 (fetch-side cross-realm FormData behavior, unchanged here).
Rationale
request()accepts FormData bodies through the duck-typedutil.isFormDataLike()check, but only undici's ownFormDatacan be encoded: the multipart extraction is gated on thewebidl.is.FormDatabrand check. A FormData from another realm (Node.js' builtinglobalThis.FormData, another undici copy, a polyfill) passed the gate, fell through the extraction, and was dispatched as a body that never produced any bytes: the request hung silently until an external timeout, with zero bytes written to the socket (full version matrix and reproductions in #5520).Per review direction (encode vs reject, maintainers chose reject): such bodies now fail fast with
InvalidArgumentError. The check lives in theRequestconstructor, where other invalid body types are already rejected, so both the HTTP/1 and HTTP/2 clients reject cleanly at dispatch time and the fetch code is untouched. The FormData brand check is loaded lazily so the fetch machinery is not pulled in upfront.Note for changelog purposes: this is a behavior change relative to v6, which encoded cross-realm FormData successfully (v7.0.0 through 7.19.x failed these bodies with an unrelated TypeError; 7.20.0+ hung).
Changes
lib/core/request.js: FormData-like bodies that are not instances of undici'sFormDatanow throwInvalidArgumentErrorinstead of falling through to iterable handling.test/issue-5520.js: cross-realm FormData rejection over HTTP/1 and HTTP/2 (own-FormData encoding is already covered bytest/client-request.jsandtest/http2-body.js).Features
N/A
Bug Fixes
request()with a FormData from another realm now rejects immediately with a clear error instead of hanging forever.Breaking Changes and Deprecations
N/A (the affected input previously hung or produced a broken body; it never worked on v7/v8)
Status
test/issue-5520.jsplus unit, fetch, client-request, http2-body and h2 core suites; lint clean)