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

fix: parse multipart/form-data requests correctly #39

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

c0per
Copy link

@c0per c0per commented Dec 30, 2023

In current version, multipart form request will throw a parsing error.

This is because createMiddleware() treat req.body as the raw body and pass it to new Request().
But req.body is generated by express.json(), which only parse requests with content-type application/json and leave req.body undefined for other requests (like multipart form).

In the PR, req.body is prepared by express.raw({ type: '*/*' }). It will get a Buffer as the raw body for all the requests and pass it directly to new Request(). json, form, even other binary content types will be untouched by express and handled by user with Request API.

Use Express.js instead of HttpServer from open-draft since open-draft
will automatically convert json request body to Object.

With Express.js and express.raw(), everything works fine.
@c0per
Copy link
Author

c0per commented Dec 30, 2023

This will unfortunately be a breaking change. The must-need express.json() will be replaced by a express.raw({ type: '*/*' }).

I didn't think of a way to avoid a middleware. Express.js doesn't provide any way to access the raw request body but using express.raw()

@LeBenLeBen
Copy link

Just spent 2 hours trying to debug why calling req.formData() like in the docs was returning Unexpected end of multipart data 😅 Thank you for contributing.

Let me know if I can be of any help to get this merged and released.

src/server.ts Outdated Show resolved Hide resolved
c0per and others added 2 commits March 27, 2024 09:38
Co-authored-by: Benoît Burgener <benoit.burgener@gmail.com>
@c0per
Copy link
Author

c0per commented Mar 27, 2024

I think a larger default size limit is a good idea.
I also changed the createServer() function to accept bodyParser options (optionally).

So for createServer() users, it's not a breaking change.
for createMiddleware(), it is breaking. They need to add app.use(express.raw()) manually.

@LeBenLeBen Please review these changes, wish we can merge this one. Thanks :)


const app = express()

app.use(express.raw({ type: '*/*' }))
Copy link
Member

Choose a reason for hiding this comment

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

What do you think if we bake in express.raw() into createMiddleware() itself?

Applying it to the entire server is too much anyway since people can attach handlers-based routes to an existing Express server.

In other words, having the raw body parser is a prerequisite of the middleware. So it must be defined there, next to the middleware.

@kettanaito kettanaito changed the title Fix multipart/form-data request fix: parse multipart/form-data requests correctly Mar 27, 2024
@ernestostifano
Copy link

Hello guys, I got this error today: TypeError: Failed to parse body as FormData..

Is there anything I can do to help?

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.

None yet

4 participants