feat: safe default filename (#689)

* feat: allow to use filename option without keepExtension * feat: can use uploadDir option and filename together * refactor: rename * feat: by default prevent directory traversal attacks filename takes ext part, if filename returns extension it will not be duplicated anymore * refactor: make octet stream less divergent feat: store newName * refactor: pass through, avoid renaming variable names * refactor: prefer Object.assign * feat: pass newName * feat: give createFileWriteStream more, including newName * docs: update * docs: update examples * fix: fix missing variables * feat: remove duplicate fix tests * lint: lint * refactor: rename mime into mimetype * refactor: explicit this.options.keepExtensions !== true * tests: reverse expectation order * tests: rename to xname to avoid confusion * refactor: inline old _uploadPath * refactor: rename newName into newFileName * refactor: rename filename into originalFilename * refactor: direcly filepath * fix: test * refactor: split hash into hashAlgorithm and hash * feat: this.lastModifiedDate = null remains * refactor: finalpath: filepath * fix: change order * refactor: better be explicit * feat: display more in toString * docs: update changelog * chore: update version * fix: revert, renamed too much * fix: _flush is more appropriate
node-formidable · Mar 18, 2021 · 748f1db · 748f1db
1 parent 997e852
commit 748f1db
Show file tree

Hide file tree

Showing 27 changed files with 268 additions and 240 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,6 +2,12 @@
 
 ### Unreleased (`canary` & `dev` dist-tags)
 
+ * rename option hash into hashAlgorithm
+ * rename file.path into file.filepath
+ * rename file.type into file.mimetype
+ * split file.name into file.newFilename and file.originalFilename
+ * prevent directory traversal attacks by default
+ * options.fileWriteStreamHandler receives the entire file instance (including file.newFilename)
  * Add fileWriteStreamHandler option
  * Add allowEmptyFiles and minFileSize options
  * Test only on Node.js >= v10. Support only Node LTS and latest ([#515](https://github.com/node-formidable/node-formidable/pull/515))

diff --git a/README.md b/README.md
@@ -335,7 +335,7 @@ See it's defaults in [src/Formidable.js DEFAULT_OPTIONS](./src/Formidable.js)
 - `options.maxFieldsSize` **{number}** - default `20 * 1024 * 1024` (20mb);
   limit the amount of memory all fields together (except files) can allocate in
   bytes.
-- `options.hash` **{string | false}** - default `false`; include checksums calculated
+- `options.hashAlgorithm` **{string | false}** - default `false`; include checksums calculated
   for incoming files, set this to some hash algorithm, see
   [crypto.createHash](https://nodejs.org/api/crypto.html#crypto_crypto_createhash_algorithm_options)
   for available algorithms
@@ -354,6 +354,10 @@ See it's defaults in [src/Formidable.js DEFAULT_OPTIONS](./src/Formidable.js)
   files for inputs which submit multiple files using the HTML5 `multiple`
   attribute. Also, the `fields` argument will contain arrays of values for
   fields that have names ending with '[]'.
+- `options.filename` **{function}** - default `undefined` Use it to control
+  newFilename. Must return a string. Will be joined with options.uploadDir.
+
+#### `options.filename`  **{function}** function (name, ext, part, form) -> string
 
 _**Note:** If this size of combined fields, or size of some file is exceeded, an
 `'error'` event is fired._
@@ -528,7 +532,7 @@ you can remove it from the `options.enabledPlugins`, like so
 const { Formidable } = require('formidable');
 
 const form = new Formidable({
-  hash: 'sha1',
+  hashAlgorithm: 'sha1',
   enabledPlugins: ['octetstream', 'querystring', 'json'],
 });
 ```
@@ -565,9 +569,9 @@ const form = formidable();
 
 form.onPart = function (part) {
   // let formidable handle only non-file parts
-  if (part.filename === '' || !part.mime) {
+  if (part.originalFilename === '' || !part.mimetype) {
     // used internally, please do not override!
-    form.handlePart(part);
+    form._handlePart(part);
   }
 };
 ```
@@ -583,7 +587,7 @@ export interface File {
 
   // The path this file is being written to. You can modify this in the `'fileBegin'` event in
   // case you are unhappy with the way formidable generates a temporary path for your files.
-  file.path: string;
+  file.filepath: string;
 
   // The name this file had according to the uploading client.
   file.name: string | null;
@@ -595,8 +599,9 @@ export interface File {
   // Mostly here for compatibility with the [W3C File API Draft](http://dev.w3.org/2006/webapi/FileAPI/).
   file.lastModifiedDate: Date | null;
 
-  // If `options.hash` calculation was set, you can read the hex digest out of this var.
-  file.hash: string | 'sha1' | 'md5' | 'sha256' | null;
+  file.hashAlgorithm: false | |'sha1' | 'md5' | 'sha256'
+  // If `options.hashAlgorithm` calculation was set, you can read the hex digest out of this var.
+  file.hash: string | object | null;
 }
 ```
 
@@ -634,10 +639,11 @@ file system.
 ```js
 form.on('fileBegin', (formName, file) => {
     // accessible here 
-    // formName the name in the form (<input name="thisname" type="file">)
-    // file.name http filename or null if there was a parsing error
-    // file.path default pathnme as per options.uploadDir and options.filename
-    // file.path = CUSTOM_PATH // to change the final path
+    // formName the name in the form (<input name="thisname" type="file">) or http filename for octetstream
+    // file.originalFilename http filename or null if there was a parsing error
+    // file.newFilename generated hexoid or what options.filename returned
+    // file.filepath default pathnme as per options.uploadDir and options.filename
+    // file.filepath = CUSTOM_PATH // to change the final path
 });
 ```
 
@@ -649,7 +655,7 @@ Emitted whenever a field / file pair has been received. `file` is an instance of
 ```js
 form.on('file', (formname, file) => {
     // same as fileBegin, except
-    // it is too late to change file.path
+    // it is too late to change file.filepath
     // file.hash is available if options.hash was used
 });
 ```

diff --git a/examples/log-file-content-to-console.js b/examples/log-file-content-to-console.js
@@ -8,7 +8,7 @@ const server = http.createServer((req, res) => {
   if (req.url === '/api/upload' && req.method.toLowerCase() === 'post') {
     // parse a file upload
     const form = formidable({
-      fileWriteStreamHandler: () => {
+      fileWriteStreamHandler: (/* file */) => {
         const writable = Writable();
         // eslint-disable-next-line no-underscore-dangle
         writable._write = (chunk, enc, next) => {

diff --git a/examples/store-files-on-s3.js b/examples/store-files-on-s3.js
@@ -15,12 +15,12 @@ const s3Client = new AWS.S3({
   },
 });
 
-const uploadStream = (filename) => {
+const uploadStream = (file) => {
   const pass = PassThrough();
   s3Client.upload(
     {
       Bucket: 'demo-bucket',
-      Key: filename,
+      Key: file.newName,
       Body: pass,
     },
     (err, data) => {

diff --git a/examples/with-http.js b/examples/with-http.js
@@ -6,15 +6,22 @@ const formidable = require('../src/index');
 const server = http.createServer((req, res) => {
   if (req.url === '/api/upload' && req.method.toLowerCase() === 'post') {
     // parse a file upload
-    const form = formidable({ multiples: true });
-
-    form.on('fileBegin', (formName, file) => {
-      if (file.name === null) {
-        // todo change lint rules because that is what we recommend
-        // eslint-disable-next-line
-        file.name = 'invalid-characters';
-      }
+    const form = formidable({
+      multiples: true,
+      uploadDir: `uploads`,
+      keepExtensions: true,
+      filename(/*name, ext, part, form*/) {
+        /* name basename of the http originalFilename
+          ext with the dot ".txt" only if keepExtension is true
+         */
+        // slugify to avoid invalid filenames
+        // substr to define a maximum length
+        // return `${slugify(name).${slugify(ext, separator: '')}`.substr(0, 100);
+        return 'yo.txt'; // or completly different name
+        // return 'z/yo.txt'; // subdirectory
+      },
     });
+
     form.parse(req, (err, fields, files) => {
       if (err) {
         console.error(err);

diff --git a/package.json b/package.json
@@ -1,6 +1,6 @@
 {
   "name": "formidable",
-  "version": "2.0.0-canary.20210216",
+  "version": "2.0.0-canary.20210302",
   "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/Formidable.js b/src/Formidable.js
@@ -21,11 +21,12 @@ const DEFAULT_OPTIONS = {
   allowEmptyFiles: true,
   keepExtensions: false,
   encoding: 'utf-8',
-  hash: false,
+  hashAlgorithm: false,
   uploadDir: os.tmpdir(),
   multiples: false,
   enabledPlugins: ['octetstream', 'querystring', 'multipart', 'json'],
   fileWriteStreamHandler: null,
+  defaultInvalidName: 'invalid-name',
 };
 
 const PersistentFile = require('./PersistentFile');
@@ -46,7 +47,9 @@ class IncomingForm extends EventEmitter {
 
     this.options = { ...DEFAULT_OPTIONS, ...options };
 
-    const dir = this.options.uploadDir || this.options.uploaddir || os.tmpdir();
+    const dir = path.resolve(
+      this.options.uploadDir || this.options.uploaddir || os.tmpdir(),
+    );
 
     this.uploaddir = dir;
     this.uploadDir = dir;
@@ -266,27 +269,27 @@ class IncomingForm extends EventEmitter {
   }
 
   _handlePart(part) {
-    if (part.filename && typeof part.filename !== 'string') {
+    if (part.originalFilename && typeof part.originalFilename !== 'string') {
       this._error(
         new FormidableError(
-          `the part.filename should be string when it exists`,
+          `the part.originalFilename should be string when it exists`,
           errors.filenameNotString,
         ),
       );
       return;
     }
 
-    // This MUST check exactly for undefined. You can not change it to !part.filename.
+    // This MUST check exactly for undefined. You can not change it to !part.originalFilename.
 
     // todo: uncomment when switch tests to Jest
     // console.log(part);
 
     // ? NOTE(@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
-    // ? NOTE(@tunnckocore): or even better, if there is no mime, then it's for sure a field
-    // ? NOTE(@tunnckocore): filename is an empty string when a field?
-    if (!part.mime) {
+    // ? NOTE(@tunnckocore): or even better, if there is no mimetype, then it's for sure a field
+    // ? NOTE(@tunnckocore): originalFilename is an empty string when a field?
+    if (!part.mimetype) {
       let value = '';
       const decoder = new StringDecoder(
         part.transferEncoding || this.options.encoding,
@@ -315,10 +318,13 @@ class IncomingForm extends EventEmitter {
 
     this._flushing += 1;
 
+    const newFilename = this._getNewName(part);
+    const filepath = this._joinDirectoryName(newFilename);
     const file = this._newFile({
-      path: this._rename(part),
-      filename: part.filename,
-      mime: part.mime,
+      newFilename,
+      filepath,
+      originalFilename: part.originalFilename,
+      mimetype: part.mimetype,
     });
     file.on('error', (err) => {
       this._error(err);
@@ -475,19 +481,22 @@ class IncomingForm extends EventEmitter {
     return new MultipartParser(this.options);
   }
 
-  _newFile({ path: filePath, filename: name, mime: type }) {
+  _newFile({ filepath, originalFilename, mimetype, newFilename }) {
     return this.options.fileWriteStreamHandler
       ? new VolatileFile({
-          name,
-          type,
+          newFilename,
+          filepath,
+          originalFilename,
+          mimetype,
           createFileWriteStream: this.options.fileWriteStreamHandler,
-          hash: this.options.hash,
+          hashAlgorithm: this.options.hashAlgorithm,
         })
       : new PersistentFile({
-          path: filePath,
-          name,
-          type,
-          hash: this.options.hash,
+          newFilename,
+          filepath,
+          originalFilename,
+          mimetype,
+          hashAlgorithm: this.options.hashAlgorithm,
         });
   }
 
@@ -499,13 +508,13 @@ class IncomingForm extends EventEmitter {
     if (!m) return null;
 
     const match = m[2] || m[3] || '';
-    let filename = match.substr(match.lastIndexOf('\\') + 1);
-    filename = filename.replace(/%22/g, '"');
-    filename = filename.replace(/&#([\d]{4});/g, (_, code) =>
+    let originalFilename = match.substr(match.lastIndexOf('\\') + 1);
+    originalFilename = originalFilename.replace(/%22/g, '"');
+    originalFilename = originalFilename.replace(/&#([\d]{4});/g, (_, code) =>
       String.fromCharCode(code),
     );
 
-    return filename;
+    return originalFilename;
   }
 
   _getExtension(str) {
@@ -525,28 +534,45 @@ class IncomingForm extends EventEmitter {
     return basename.slice(firstDot, lastDot) + extname;
   }
 
-  _uploadPath(part, fp) {
-    const name = fp || `${this.uploadDir}${path.sep}${toHexoId()}`;
 
-    if (part && this.options.keepExtensions) {
-      const filename = typeof part === 'string' ? part : part.filename;
-      return `${name}${this._getExtension(filename)}`;
+
+  _joinDirectoryName(name) {
+    const newPath = path.join(this.uploadDir, name);
+
+    // prevent directory traversal attacks
+    if (!newPath.startsWith(this.uploadDir)) {
+      return path.join(this.uploadDir, this.options.defaultInvalidName);
     }
 
-    return name;
+    return newPath;
   }
 
   _setUpRename() {
     const hasRename = typeof this.options.filename === 'function';
-
-    if (this.options.keepExtensions === true && hasRename) {
-      this._rename = (part) => {
-        const resultFilepath = this.options.filename.call(this, part, this);
-
-        return this._uploadPath(part, resultFilepath);
+    if (hasRename) {
+      this._getNewName = (part) => {
+        let ext = '';
+        let name = this.options.defaultInvalidName;
+        if (part.originalFilename) {
+          // can be null
+          ({ ext, name } = path.parse(part.originalFilename));
+          if (this.options.keepExtensions !== true) {
+            ext = '';
+          }
+        }
+        return this.options.filename.call(this, name, ext, part, this);
       };
     } else {
-      this._rename = (part) => this._uploadPath(part);
+      this._getNewName = (part) => {
+        const name = toHexoId();
+
+        if (part && this.options.keepExtensions) {
+          const originalFilename = typeof part === 'string' ? part : part.originalFilename;
+          return `${name}${this._getExtension(originalFilename)}`;
+        }
+
+        return name;
+      }
     }
   }
 

diff --git a/src/FormidableError.js b/src/FormidableError.js
@@ -1,6 +1,5 @@
 /* eslint-disable no-plusplus */
 
-
 const missingPlugin = 1000;
 const pluginFunction = 1001;
 const aborted = 1002;