fix: handle (& part) files and fields correctly

as mentioned here #531 (comment) Signed-off-by: Charlike Mike Reagent <opensource@tunnckocore.com>
node-formidable · Jan 28, 2020 · 621e72a · 621e72a
1 parent 15f174f
commit 621e72a
Show file tree

Hide file tree

Showing 5 changed files with 109 additions and 85 deletions.
diff --git a/.eslintignore b/.eslintignore
@@ -12,7 +12,7 @@ test-legacy
 *.cache
 
 # TODO: lint
-example
+# example
 benchmark
 
 # Build environment

diff --git a/example/json.js b/example/json.js
@@ -1,19 +1,19 @@
+'use strict';
+
 const http = require('http');
 const util = require('util');
-const common = require('../test/common');
-
-const { formidable } = common;
-const { port } = common;
-let server;
+const Formidable = require('../src/index');
 
-server = http.createServer((req, res) => {
+// random OS choosen port
+const PORT = 13532;
+const server = http.createServer((req, res) => {
   if (req.method !== 'POST') {
     res.writeHead(200, { 'content-type': 'text/plain' });
-    res.end(`Please POST a JSON payload to http://localhost:${port}/`);
+    res.end(`Please POST a JSON payload to http://localhost:${PORT}/`);
     return;
   }
 
-  const form = new formidable.IncomingForm();
+  const form = new Formidable();
   const fields = {};
 
   form
@@ -33,41 +33,43 @@ server = http.createServer((req, res) => {
     });
   form.parse(req);
 });
-server.listen(port);
 
-console.log(`listening on http://localhost:${port}/`);
+server.listen(PORT, () => {
+  const choosenPort = server.address().port;
+  console.log(`Listening on http://localhost:${choosenPort}/`);
 
-const message = '{"numbers":[1,2,3,4,5],"nested":{"key":"value"}}';
-const request = http.request(
-  {
-    host: 'localhost',
-    path: '/',
-    port,
-    method: 'POST',
-    headers: {
-      'content-type': 'application/json',
-      'content-length': message.length,
+  const message = '{"numbers":[1,2,3,4,5],"nested":{"key":"value"}}';
+  const request = http.request(
+    {
+      host: 'localhost',
+      path: '/',
+      port: choosenPort,
+      method: 'POST',
+      headers: {
+        'content-type': 'application/json',
+        'content-length': message.length,
+      },
     },
-  },
-  (response) => {
-    const data = '';
-    console.log('\nServer responded with:');
-    console.log('Status:', response.statusCode);
-    response.pipe(process.stdout);
-    response.on('end', () => {
-      console.log('\n');
-      process.exit();
-    });
-    // response.on('data', function(chunk) {
-    //   data += chunk.toString('utf8');
-    // });
-    // response.on('end', function() {
-    //   console.log('Response Data:');
-    //   console.log(data);
-    //   process.exit();
-    // });
-  },
-);
+    (response) => {
+      console.log('\nServer responded with:');
+      console.log('Status:', response.statusCode);
+      response.pipe(process.stdout);
+      response.on('end', () => {
+        console.log('\n');
+        process.exit();
+      });
+      // const data = '';
+      // response.on('data', function(chunk) {
+      //   data += chunk.toString('utf8');
+      // });
+      // response.on('end', function() {
+      //   console.log('Response Data:');
+      //   console.log(data);
+      //   process.exit();
+      // });
+    },
+  );
 
-request.write(message);
-request.end();
+  request.write(message);
+  request.end();
+});
diff --git a/example/multipartParser.js b/example/multipartParser.js
@@ -1,11 +1,13 @@
-const { MultipartParser } = require('../lib/multipart_parser.js');
+'use strict';
+
+const { MultipartParser } = require('../src/multipart_parser.js');
 
 // hand crafted multipart
 const boundary = '--abcxyz';
 const next = '\r\n';
 const formData = 'Content-Disposition: form-data; ';
-const buffer = Buffer.from(
-  `${boundary}${next}${formData}name="text"${next}${next}text ...${next}${next}${boundary}${next}${formData}name="z"${next}${next}text inside z${next}${next}${boundary}${next}${formData}name="file1"; filename="a.txt"${next}Content-Type: text/plain${next}${next}Content of a.txt.${next}${next}${boundary}${next}${formData}name="file2"; filename="a.html"${next}Content-Type: text/html${next}${next}<!DOCTYPE html><title>Content of a.html.</title>${next}${next}${boundary}--`,
+const bufferToWrite = Buffer.from(
+  `${boundary}${next}${formData}name="text"${next}${next}some text ...${next}${next}${boundary}${next}${formData}name="z"${next}${next}text inside z${next}${next}${boundary}${next}${formData}name="file1"; filename="a.txt"${next}Content-Type: text/plain${next}${next}Content of a.txt.${next}${next}${boundary}${next}${formData}name="file2"; filename="a.html"${next}Content-Type: text/html${next}${next}<!DOCTYPE html><title>Content of a.html.</title>${next}${next}${boundary}--`,
 );
 
 const multipartParser = new MultipartParser();
@@ -22,5 +24,6 @@ multipartParser.on('error', (error) => {
 
 multipartParser.initWithBoundary(boundary.substring(2)); // todo make better error message when it is forgotten
 // const shouldWait = !multipartParser.write(buffer);
+multipartParser.write(bufferToWrite);
 multipartParser.end();
 // multipartParser.destroy();
diff --git a/example/multiple.js b/example/multiple.js
@@ -1,37 +1,36 @@
+'use strict';
+
+const os = require('os');
 const http = require('http');
 const util = require('util');
-const os = require('os');
-const common = require('../test/common');
+const Formidable = require('../src/index');
 
-const { formidable } = common;
-const { port } = common;
-let server;
-
-server = http.createServer((req, res) => {
+const PORT = 13532;
+const server = http.createServer((req, res) => {
   if (req.url === '/') {
     res.writeHead(200, { 'content-type': 'text/html' });
     res.end(
       `<form action="/upload" enctype="multipart/form-data" method="post">
-        <label>simple<input type="text" name="simple"></label><br>
+        <label>simple<input type="text" name="text_single" autofocus /></label><br />
 
-        <label>array text 0<input type="text" name="atext[]"></label><br>
-        <label>array text 1<input type="text" name="atext[]"></label><br>
+        <label>array text 0<input type="text" name="text_multiple[]" /></label><br />
+        <label>array text 1<input type="text" name="text_multiple[]" /></label><br />
 
-        <label>file simple<input type="file" name="filesimple"></label><br>
+        <label>file simple<input type="file" name="file_single" /></label><br />
 
-        <label>file attribute multiple<input type="file" name="multiplefile" multiple></label><br>
+        <label>file attribute multiple<input type="file" name="file_multiple" multiple /></label><br />
 
-        <label>file html array0<input type="file" name="filearray[]"></label><br>
-        <label>file html array1<input type="file" name="filearray[]"></label><br>
+        <label>file html array0<input type="file" name="filearray[]" /></label><br />
+        <label>file html array1<input type="file" name="filearray[]" /></label><br />
 
-        <label>file html array and mulitple0<input type="file" name="mfilearray[]" multiple></label><br>
-        <label>file html array and mulitple1<input type="file" name="mfilearray[]" multiple></label><br>
-        <br>
+        <label>file html array and mulitple0<input type="file" name="filearray_with_multiple[]" multiple /></label><br />
+        <label>file html array and mulitple1<input type="file" name="filearray_with_multiple[]" multiple /></label><br />
+        <br />
         <button>Upload</button>
       </form>`,
     );
   } else if (req.url === '/upload') {
-    const form = new formidable.IncomingForm({ multiples: true });
+    const form = new Formidable({ multiples: true });
 
     form.uploadDir = os.tmpdir();
 
@@ -46,6 +45,7 @@ server = http.createServer((req, res) => {
     res.end('404');
   }
 });
-server.listen(port);
 
-console.log(`listening on http://localhost:${port}/`);
+server.listen(PORT, () => {
+  console.log(`listening on http://localhost:${PORT}/`);
+});
diff --git a/src/incoming_form.js b/src/incoming_form.js
@@ -95,28 +95,37 @@ class IncomingForm extends EventEmitter {
         } else {
           fields[name] = value;
         }
-      })
-        .on('file', (name, file) => {
-          // TODO: too much nesting
-          if (this.multiples) {
-            if (hasOwnProp(files, name)) {
-              if (!Array.isArray(files[name])) {
-                files[name] = [files[name]];
-              }
-              files[name].push(file);
-            } else {
-              files[name] = file;
+        // if (name === 'simple') {
+        //   console.log('fields name!!', name);
+        //   console.log('fields value!!', value);
+        // }
+      });
+      this.on('file', (name, file) => {
+        // TODO: too much nesting
+        if (this.multiples) {
+          if (hasOwnProp(files, name)) {
+            if (!Array.isArray(files[name])) {
+              files[name] = [files[name]];
             }
+            files[name].push(file);
           } else {
             files[name] = file;
           }
-        })
-        .on('error', (err) => {
-          cb(err, fields, files);
-        })
-        .on('end', () => {
-          cb(null, fields, files);
-        });
+        } else {
+          files[name] = file;
+        }
+        // console.log('files!!', files);
+        // if (name === 'simple') {
+        //   console.log('files name!!', name);
+        //   console.log('files value!!', file);
+        // }
+      });
+      this.on('error', (err) => {
+        cb(err, fields, files);
+      });
+      this.on('end', () => {
+        cb(null, fields, files);
+      });
     }
 
     // Parse headers and setup the parser, ready to start listening for data.
@@ -191,8 +200,18 @@ class IncomingForm extends EventEmitter {
   }
 
   handlePart(part) {
+    if (part.filename && typeof part.filename !== 'string') {
+      this._error(new Error(`the part.filename should be string when exists`));
+      return;
+    }
+
     // This MUST check exactly for undefined. You can not change it to !part.filename.
-    if (part.filename === undefined) {
+
+    // @tunnckocore: no it can be any falsey value, it most probably depends on what's returned
+    // from somewhere else. Where recently I changed the return statements
+    // and such thing because code style
+    // @tunnckocore: or even better, if there is no mime, then it's for sure a field
+    if (!part.mime) {
       let value = '';
       const decoder = new StringDecoder(this.encoding);