Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve keep extension #857

Merged
merged 11 commits into from
May 19, 2022
21 changes: 11 additions & 10 deletions examples/with-http.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import http from 'node:http';
import slugify from '@sindresorhus/slugify';
import formidable from '../src/index.js';


Expand All @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we better use filenamify instead.

filenamify(name, { replacement: '-' })

there will not be needed for ext, because we can just return path.extname, as per the other comments.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ooh, that's from the examples dir, lol

// 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");
Expand Down
3 changes: 2 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
@@ -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",
Expand Down Expand Up @@ -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",
Expand Down
31 changes: 27 additions & 4 deletions src/Formidable.js
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -497,6 +507,9 @@ class IncomingForm extends EventEmitter {
return originalFilename;
}

// able to get composed extension with multiple dots
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i remember pretty clearly there was some reason for that, some report or feature or issue.. but i don't remember the reason haha, so we can switch back to just getting the thing after the last dot with Node's builtin and all the drama goes to the trash.

// "a.b.c" -> ".b.c"
// as opposed to path.extname -> ".c"
_getExtension(str) {
if (!str) {
return '';
Expand All @@ -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) {
Expand Down
10 changes: 5 additions & 5 deletions src/PersistentFile.js
Original file line number Diff line number Diff line change
@@ -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 {
Expand All @@ -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);
});
Expand Down Expand Up @@ -80,7 +80,7 @@ class PersistentFile extends EventEmitter {
this._writeStream.destroy();
const filepath = this.filepath;
setTimeout(function () {
unlink(filepath, () => {});
fs.unlink(filepath, () => {});
}, 1)
}
}
Expand Down
11 changes: 11 additions & 0 deletions test/unit/formidable.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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`, () => {
Expand Down