fix: flush or fail always (don't hang) (#945)

* fix: flush or fail always (don't hang) * feat: more descriptive errors * tests: make test work when other tests are run * tests: add test to confirm issue
node-formidable · Aug 25, 2023 · 1699ec6 · 1699ec6
1 parent 489a0bb
commit 1699ec6
Show file tree

Hide file tree

Showing 5 changed files with 62 additions and 17 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,5 +1,10 @@
 # Changelog
 
+### 3.5.1
+
+ * fix: ([#945](https://github.com/node-formidable/formidable/pull/945)) multipart parser fix: flush or fail always (don't hang)
+
+
 ### 3.5.0
 
  * feature: ([#944](https://github.com/node-formidable/formidable/pull/944)) Dual package: Can be imported as ES module and required as commonjs module

diff --git a/package.json b/package.json
@@ -1,6 +1,6 @@
 {
   "name": "formidable",
-  "version": "3.5.0",
+  "version": "3.5.1",
   "license": "MIT",
   "description": "A node.js module for parsing form data, especially file uploads.",
   "homepage": "https://github.com/node-formidable/formidable",

diff --git a/src/parsers/Multipart.js b/src/parsers/Multipart.js
@@ -59,6 +59,14 @@ class MultipartParser extends Transform {
     this.flags = 0;
   }
 
+  _endUnexpected() {
+    return new FormidableError(
+      `MultipartParser.end(): stream ended unexpectedly: ${this.explain()}`,
+      errors.malformedMultipart,
+      400,
+    );
+  }
+
   _flush(done) {
     if (
       (this.state === STATE.HEADER_FIELD_START && this.index === 0) ||
@@ -68,13 +76,9 @@ class MultipartParser extends Transform {
       this._handleCallback('end');
       done();
     } else if (this.state !== STATE.END) {
-      done(
-        new FormidableError(
-          `MultipartParser.end(): stream ended unexpectedly: ${this.explain()}`,
-          errors.malformedMultipart,
-          400,
-        ),
-      );
+      done(this._endUnexpected());
+    } else {
+      done();
     }
   }
 
@@ -136,7 +140,8 @@ class MultipartParser extends Transform {
       c = buffer[i];
       switch (state) {
         case STATE.PARSER_UNINITIALIZED:
-          return i;
+          done(this._endUnexpected());
+          return;
         case STATE.START:
           index = 0;
           state = STATE.START_BOUNDARY;
@@ -145,7 +150,8 @@ class MultipartParser extends Transform {
             if (c === HYPHEN) {
               flags |= FBOUNDARY.LAST_BOUNDARY;
             } else if (c !== CR) {
-              return i;
+              done(this._endUnexpected());
+              return;
             }
             index++;
             break;
@@ -159,7 +165,8 @@ class MultipartParser extends Transform {
               this._handleCallback('partBegin');
               state = STATE.HEADER_FIELD_START;
             } else {
-              return i;
+              done(this._endUnexpected());
+              return;
             }
             break;
           }
@@ -190,7 +197,8 @@ class MultipartParser extends Transform {
           if (c === COLON) {
             if (index === 1) {
               // empty header field
-              return i;
+              done(this._endUnexpected());
+              return;
             }
             dataCallback('headerField', true);
             state = STATE.HEADER_VALUE_START;
@@ -199,7 +207,8 @@ class MultipartParser extends Transform {
 
           cl = lower(c);
           if (cl < A || cl > Z) {
-            return i;
+            done(this._endUnexpected());
+            return;
           }
           break;
         case STATE.HEADER_VALUE_START:
@@ -218,13 +227,15 @@ class MultipartParser extends Transform {
           break;
         case STATE.HEADER_VALUE_ALMOST_DONE:
           if (c !== LF) {
-            return i;
+            done(this._endUnexpected());
+return;
           }
           state = STATE.HEADER_FIELD_START;
           break;
         case STATE.HEADERS_ALMOST_DONE:
           if (c !== LF) {
-            return i;
+            done(this._endUnexpected());
+            return;
           }
 
           this._handleCallback('headersEnd');
@@ -311,7 +322,8 @@ class MultipartParser extends Transform {
         case STATE.END:
           break;
         default:
-          return i;
+          done(this._endUnexpected());
+          return;
       }
     }
 

diff --git a/test-node/standalone/createDirsFromUploads.test.js b/test-node/standalone/createDirsFromUploads.test.js
@@ -52,7 +52,8 @@ d\r
       },
       body
     }).then(res => {
-      deepEqual(fs.readdirSync(uploads), ['x']);
+      //may also contain tests from other tests
+      deepEqual(fs.readdirSync(uploads).includes('x'), true);
       deepEqual(fs.readdirSync(`${uploads}/x`), ['y']);
       deepEqual(fs.readdirSync(`${uploads}/x/y`), ['z.txt']);
       strictEqual(res.status, 200);

diff --git a/test-node/standalone/multipart_parser.test.js b/test-node/standalone/multipart_parser.test.js
@@ -0,0 +1,27 @@
+import {Readable} from 'node:stream';
+import MultipartParser from '../../src/parsers/Multipart.js';
+import {malformedMultipart} from '../../src/FormidableError.js';
+
+import test from 'node:test';
+import assert, { deepEqual } from 'node:assert';
+
+
+
+test('MultipartParser does not hang', async (t) => {
+    const mime = `--_\r\n--_--\r\n`;
+    const parser = new MultipartParser();
+    parser.initWithBoundary('_');
+    try {
+        for await (const {name, buffer, start, end} of Readable.from(mime).pipe(parser)) {
+            console.log(name, buffer ? buffer.subarray(start, end).toString() : '');
+        }
+
+    } catch (e) {
+        deepEqual(e.code, malformedMultipart)
+        return;
+        // console.error('error');
+        // console.error(e);
+
+    }
+    assert(false, 'should catch error');
+});