Skip to content

fix(fetch): parse multipart body up to the full boundary delimiter#5561

Open
arshsmith1 wants to merge 2 commits into
nodejs:mainfrom
arshsmith1:multipart-body-delimiter
Open

fix(fetch): parse multipart body up to the full boundary delimiter#5561
arshsmith1 wants to merge 2 commits into
nodejs:mainfrom
arshsmith1:multipart-body-delimiter

Conversation

@arshsmith1

Copy link
Copy Markdown

This relates to...

Multipart parsing in response.formData() / request.formData().

Rationale

The body loop in multipartFormDataParser looked for a bare boundary token (boundary.subarray(2)) rather than the \r\n--boundary delimiter line, then blindly subtracted 4 from the match index. A part whose body legitimately contains the boundary token gets truncated at the wrong offset, and in most cases the whole body is rejected with expected CRLF even though it is well framed. This block carried a // TODO: the steps here are completely wrong note. The fix matches the full delimiter so boundary-like bytes inside a body are no longer mistaken for a delimiter, which also removes an ambiguity where such bytes could be read as an extra part.

Changes

Search for \r\n--boundary when terminating a part body and slice up to that index, instead of finding the bare boundary token and offsetting by 4.

Features

N/A

Bug Fixes

  • multipart body loop now terminates a part at the full boundary delimiter, so bodies containing the boundary token parse correctly instead of being truncated or rejected.

Breaking Changes and Deprecations

N/A

Status

Signed-off-by: arshiya tabasum <arshi@bugqore.com>
Comment thread lib/web/fetch/formdata-parser.js Outdated
Comment on lines +79 to +82
// The delimiter that terminates a part's body is the CRLF that precedes the
// next boundary delimiter line, i.e. "\r\n--boundary". Matching the full
// delimiter (not a bare boundary token) is required so boundary-like bytes
// inside a body are not mistaken for a delimiter.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
// The delimiter that terminates a part's body is the CRLF that precedes the
// next boundary delimiter line, i.e. "\r\n--boundary". Matching the full
// delimiter (not a bare boundary token) is required so boundary-like bytes
// inside a body are not mistaken for a delimiter.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Dropped the comment.

Comment thread test/fetch/client-fetch.js Outdated
t.assert.strictEqual(form.get('fiŝo'), 'value1')
})

test('multipart formdata body containing the boundary token', async (t) => {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

please move this to test/busboy

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Moved it to test/busboy/formdata-boundary-token.js.

Drop the explanatory comment on crlfDashBoundary and relocate the
regression test out of client-fetch.js into test/busboy.

Signed-off-by: arshiya tabasum <arshi@bugqore.com>
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.

2 participants