remix-run / remix

Build Better Websites. Create modern, resilient user experiences with web fundamentals.
https://remix.run
MIT License
29.22k stars 2.46k forks source link

Server crash on multipart file upload containing apostrophe or semicolon #4443

Open elledienne opened 1 year ago

elledienne commented 1 year ago

What version of Remix are you using?

1.7.2

Steps to Reproduce

  1. Create a file called something ' something.png or something ; something.png (file extension doesn't really matter
  2. Upload the file to a Remix app - I've used the code in the official doc to create a simple uploadHandler leveraging unstable_parseMultipartFormData

Expected Behavior

The file is successfully uploaded

Actual Behavior

The server crashes with the following error

Error: malformed content-disposition header: mismatched quotations in `form-data; name="file-upload"; filename="something ' something.png"`
    at parseContentDisposition (/Users/[...]/remix/node_modules/@web3-storage/multipart-parser/cjs/src/index.js:28:13)
    at parsePartHeaders (/Users/[...]/remix/node_modules/@web3-storage/multipart-parser/cjs/src/index.js:50:38)
    at Object.streamMultipart (/Users/[...]/remix/node_modules/@web3-storage/multipart-parser/cjs/src/index.js:169:10)
    at runMicrotasks (<anonymous>)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at parseMultipartFormData (/Users/[...]/remix/node_modules/@remix-run/server-runtime/dist/formData.js:48:18)
    at action27 (/Users/[...]/remix/app/routes/api/content/index.ts:65:16)
    at /Users/[...]/remix/node_modules/@sentry/remix/cjs/utils/instrumentServer.js:140:13
    at Object.callRouteAction (/Users/[...]/remix/node_modules/@remix-run/server-runtime/dist/data.js:40:14)
    at handleDataRequest (/Users/[...]/remix/node_modules/@remix-run/server-runtime/dist/server.js:94:18)
    at requestHandler (/Users/[...]/remix/node_modules/@remix-run/server-runtime/dist/server.js:34:18)
    at /Users/[...]/remix/node_modules/@sentry/remix/cjs/utils/instrumentServer.js:345:18
    at /Users/[...]/remix/node_modules/@remix-run/express/dist/server.js:39:22

As you can see, the error comes from a dependency of Remix, @web3-storage but the repo seems quite dead, and existing issues are not being addressed.

dmarkow commented 1 year ago

Ran into the same issue (I actually created that issue over there). @web3-storage's header handling is pretty basic. I haven't had time to open a proper PR on this repo to use a different lib, but for now this diff with patch-package is working around it.

patches/@web3-storage+multipart-parser+1.0.0.patch:

diff --git a/node_modules/@web3-storage/multipart-parser/cjs/src/index.js b/node_modules/@web3-storage/multipart-parser/cjs/src/index.js
index 8be5ee9..311b48a 100644
--- a/node_modules/@web3-storage/multipart-parser/cjs/src/index.js
+++ b/node_modules/@web3-storage/multipart-parser/cjs/src/index.js
@@ -9,17 +9,24 @@ const mergeArrays2 = Function.prototype.apply.bind(utils.mergeArrays, undefined)
 const dash = utils.stringToArray('--');
 const CRLF = utils.stringToArray('\r\n');
 function parseContentDisposition(header) {
-  const parts = header.split(';').map(part => part.trim());
+  const parts = header.split(/;(?=(?:[^"]*"[^"]*")*[^"]*$)/).map(part => part.trim());
   if (parts.shift() !== 'form-data') {
     throw new Error('malformed content-disposition header: missing "form-data" in `' + JSON.stringify(parts) + '`');
   }
   const out = {};
   for (const part of parts) {
-    const kv = part.split('=', 2);
-    if (kv.length !== 2) {
-      throw new Error('malformed content-disposition header: key-value pair not found - ' + part + ' in `' + header + '`');
+    const equals = part.indexOf('=');
+    if (equals < 0) {
+      throw new Error('malformed key-value string: missing value in `' + part + '`');
     }
-    const [name, value] = kv;
+
+    const name = part.slice(0, equals);
+    const value = part.slice(equals + 1);
+    //const kv = part.split('=', 2);
+    //if (kv.length !== 2) {
+      // throw new Error('malformed content-disposition header: key-value pair not found - ' + part + ' in `' + header + '`');
+    // }
+    // const [name, value] = kv;
     if (value[0] === '"' && value[value.length - 1] === '"') {
       out[name] = value.slice(1, -1).replace(/\\"/g, '"');
     } else if (value[0] !== '"' && value[value.length - 1] !== '"') {
kof commented 1 year ago

We caught the same error https://github.com/webstudio-is/webstudio-designer/pull/483

brophdawg11 commented 1 year ago

I confirmed this is an issue when the filename uses characters that are used as delimiters in the Content-Disposition header, such as ;, =, and "

// Parses fine
Content-Disposition: form-data; name="file"; filename="something.txt"

// Fails to parse
Content-Disposition: form-data; name="file"; filename="something;else.txt"
Content-Disposition: form-data; name="file"; filename="something=else.txt"
Content-Disposition: form-data; name="file"; filename="something"else.txt"

Going to mark this as external for now during this v2 bug sweep - but I agree it looks like that package may be abandoned so we may need to fork or inline that code prior to stabilizing unstable_parseMultipartFormData

apaofficial commented 9 months ago

This issue is still present in version 2.2

etimberg commented 4 months ago

The @web3-storage/multipart-parser package is archived now so definitely abandoned.

zmillman commented 3 months ago

Just ran into this on remix-run@2.9.1 with @web3-storage@1.0.0