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

parseFormData crashes node express server when exceeding max size #28

Open
lasseklovstad opened this issue Nov 2, 2024 · 5 comments
Open

Comments

@lasseklovstad
Copy link

When uploading a file that is too large the parseFormData throws an MultipartParseError that is uncatchable. When it is thrown in a Remix action function, Remix is unable to catch it so the server crashes :(

const formData = await parseFormData(
    request,
    createTempUploadHandler(),
    { maxFileSize: 20 * 1024 * 1024 },
  );
MultipartParseError: File size exceeds maximum allowed size of 20971520 bytes
    at MultipartParser.#writeBody (file:///C:/projects/pils/node_modules/@mjackson/multipart-parser/dist/lib/multipart.js:240:25)
    at MultipartParser.#write (file:///C:/projects/pils/node_modules/@mjackson/multipart-parser/dist/lib/multipart.js:173:40)
    at MultipartParser.parse (file:///C:/projects/pils/node_modules/@mjackson/multipart-parser/dist/lib/multipart.js:123:28)
    at processTicksAndRejections (node:internal/process/task_queues:105:5)
@Bessonov
Copy link

I'm not sure if I encountered the same error. I use parseMultipartRequest to retrieve files from Node's request object. When I set maxFileSize, I can catch the error and respond with a 422 status. However, after this, the browser's next request to another endpoint hangs, although opening a new tab works as expected. There is also a proxy in-between, so not sure it comes from the library.

If I let maxFileSize to Infinity and stop processing while reading chunks from file.body, everything works as expected. It seems like something might not be terminated, closed, or ended properly. However, just by looking at the source code of multipart.ts, I don't see anything obviously wrong 🤷

@mattoni
Copy link

mattoni commented Dec 21, 2024

I'm also encountering this issue.

image

image

@mattoni
Copy link

mattoni commented Dec 21, 2024

I am only able to prevent my server from crashing by adding

process.on("unhandledRejection", (reason, promise) => {
	logger.error("Unhandled rejection at:", promise, "reason:", reason);
});

but that isn't ideal since the error is no longer being handled within the action, so we can't gracefully return a message to the user.

@Apsysikal
Copy link

I'm having the same issue currently. Couldn't get it to catch the error, no matter where i add the catch clause.
Did anyone have any luck in fixing it?

@lukejagodzinski
Copy link

lukejagodzinski commented Jan 12, 2025

I've tried to investigate this issue. There is a test for the parseMultipartRequest function that is being used inside parseFormData. However, it's testing a case where developer didn't provide uploadHandler, meaning that the default one is being used which doesn't stream but reads the whole body into memory at once:

async function defaultFileUploadHandler(file) {
  let buffer = await file.arrayBuffer();
  return new File([buffer], file.name, { type: file.type, lastModified: file.lastModified });
}

And in deed, when I didn't provide uploadHandler the function doesn't kill the server. I've tried to pinpoint the exact place where the problem happens and I think it's the incorrect handling of the error in the parseMultipart function.

When error happens it doesn't get to the line where it should actually throw error:

  if (parseError) {
    throw parseError;
  }

It's stuck in the while loop? I couldn't determine that. I have a hunch that error should be thrown earlier. But I would have to better understand the code.

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

No branches or pull requests

5 participants