fix: Improve keep extension (#857)

* tests: add failing test case

* docs: add comment

* tests: add comment

* tests: add a test case that proves that the regex was always bad

* fix: replace regex with reliable filtering

* feat: stop at first invalid char

* feat: allow numbers in file extensions

* feat: stop extension from being '.'

* refactor: code style

* docs: use slugify in the example

* chore: prepare release

examples/with-http.js

@@ -1,4 +1,5 @@
 import http from 'node:http';
+import slugify from '@sindresorhus/slugify';
 import formidable from '../src/index.js';
 
 
@@ -14,16 +15,16 @@ const server = http.createServer((req, res) => {
     const form = formidable({
       // uploadDir: `uploads`,
       keepExtensions: true,
-      // filename(/*name, ext, part, form*/) {
-      //   /* name basename of the http originalFilename
-      //     ext with the dot ".txt" only if keepExtensions 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
-      // },
+      filename(name, ext, part, form) {
+        /* name basename of the http originalFilename
+          ext with the dot ".txt" only if keepExtensions is true
+         */
+        // slugify to avoid invalid filenames
+        // substr to define a maximum 
+        return `${slugify(name)}.${slugify(ext, {separator: ''})}`.substr(0, 100);
+        // return 'yo.txt'; // or completly different name
+        // return 'z/yo.txt'; // subdirectory
+      },
       // filter: function ({name, originalFilename, mimetype}) {
       //   // keep only images
       //   return mimetype && mimetype.includes("image");

package.json

@@ -1,6 +1,6 @@
 {
   "name": "formidable",
-  "version": "3.2.3",
+  "version": "3.2.4",
   "license": "MIT",
   "description": "A node.js module for parsing form data, especially file uploads.",
   "homepage": "https://github.com/node-formidable/formidable",
@@ -37,6 +37,7 @@
   "devDependencies": {
     "@commitlint/cli": "8.3.5",
     "@commitlint/config-conventional": "8.3.4",
+    "@sindresorhus/slugify": "^2.1.0",
     "@tunnckocore/prettier-config": "1.3.8",
     "del-cli": "3.0.0",
     "eslint": "6.8.0",

src/Formidable.js

@@ -42,6 +42,16 @@ function hasOwnProp(obj, key) {
   return Object.prototype.hasOwnProperty.call(obj, key);
 }
 
+const invalidExtensionChar = (c) => {
+  const code = c.charCodeAt(0);
+  return !(
+    code === 46 || // .
+    (code >= 48 && code <= 57) ||
+    (code >= 65 && code <= 90) ||
+    (code >= 97 && code <= 122)
+  );
+};
+
 class IncomingForm extends EventEmitter {
   constructor(options = {}) {
     super();
@@ -497,6 +507,9 @@ class IncomingForm extends EventEmitter {
     return originalFilename;
   }
 
+  // able to get composed extension with multiple dots
+  // "a.b.c" -> ".b.c"
+  // as opposed to path.extname -> ".c"
   _getExtension(str) {
     if (!str) {
       return '';
@@ -505,13 +518,23 @@ class IncomingForm extends EventEmitter {
     const basename = path.basename(str);
     const firstDot = basename.indexOf('.');
     const lastDot = basename.lastIndexOf('.');
-    const extname = path.extname(basename).replace(/(\.[a-z0-9]+).*/i, '$1');
+    let rawExtname = path.extname(basename);
 
-    if (firstDot === lastDot) {
-      return extname;
+    if (firstDot !== lastDot) {
+      rawExtname =  basename.slice(firstDot);
     }
 
-    return basename.slice(firstDot, lastDot) + extname;
+    let filtered;
+    const firstInvalidIndex = Array.from(rawExtname).findIndex(invalidExtensionChar);
+    if (firstInvalidIndex === -1) {
+      filtered = rawExtname;
+    } else {
+      filtered = rawExtname.substring(0, firstInvalidIndex);
+    }
+    if (filtered === '.') {
+      return '';
+    }
+    return filtered;
   }
 
   _joinDirectoryName(name) {

src/PersistentFile.js

@@ -1,7 +1,7 @@
 /* eslint-disable no-underscore-dangle */
 
-import { WriteStream, unlink } from 'node:fs';
-import { createHash } from 'node:crypto';
+import fs from 'node:fs';
+import crypto from 'node:crypto';
 import { EventEmitter } from 'node:events';
 
 class PersistentFile extends EventEmitter {
@@ -15,14 +15,14 @@ class PersistentFile extends EventEmitter {
     this._writeStream = null;
 
     if (typeof this.hashAlgorithm === 'string') {
-      this.hash = createHash(this.hashAlgorithm);
+      this.hash = crypto.createHash(this.hashAlgorithm);
     } else {
       this.hash = null;
     }
   }
 
   open() {
-    this._writeStream = new WriteStream(this.filepath);
+    this._writeStream = fs.createWriteStream(this.filepath);
     this._writeStream.on('error', (err) => {
       this.emit('error', err);
     });
@@ -80,7 +80,7 @@ class PersistentFile extends EventEmitter {
     this._writeStream.destroy();
     const filepath = this.filepath; 
     setTimeout(function () {
-        unlink(filepath, () => {});
+        fs.unlink(filepath, () => {});
     }, 1)
   }
 }

test/unit/formidable.test.js

@@ -65,6 +65,7 @@ function makeHeader(originalFilename) {
 
     const getBasename = (part) => path.basename(form._getNewName(part));
 
+    // tests below assume baseline hexoid 25 chars + a few more for the extension
     let basename = getBasename('fine.jpg?foo=bar');
     expect(basename).toHaveLength(29);
     let ext = path.extname(basename);
@@ -94,6 +95,16 @@ function makeHeader(originalFilename) {
     expect(basename).toHaveLength(30);
     ext = path.extname(basename);
     expect(ext).toBe('.QxZs');
+
+    basename = getBasename('test.pdf.jqlnn<img src=a onerror=alert(1)>.png');
+    expect(basename).toHaveLength(35);
+    ext = path.extname(basename);
+    expect(ext).toBe('.jqlnn');
+
+    basename = getBasename('test.<a.png');
+    expect(basename).toHaveLength(25);
+    ext = path.extname(basename);
+    expect(ext).toBe('');
   });
 
   test(`${name}#_Array parameters support`, () => {