merge 1.3.1

master
Thomas Lynch 2 years ago
parent ded0b1e69c
commit 4b66908beb
  1. 3
      .gitignore
  2. 13
      .travis.yml
  3. 12
      README.md
  4. 8
      example/README.md
  5. 3
      lib/index.js
  6. 5
      lib/processMultipart.js
  7. 17
      lib/processNested.js
  8. 63
      lib/utilities.js
  9. 3576
      package-lock.json
  10. 17
      package.json
  11. 85
      test/utilities.spec.js

3
.gitignore vendored

@ -13,8 +13,9 @@ node_modules
*.gz
coverage
.nyc_output
test/uploads
example/uploads/*
!example/uploads/placeholder.txt
yarn.lock
yarn.lock

@ -1,13 +0,0 @@
language: node_js
node_js:
- "10"
- "11"
- "12"
env:
- COVERALLS_REPO_TOKEN=vNV8IQ0jJAuWGikebCeIHJryRulP6aEHa
script:
- npm run lint
- npm test
- npm run coveralls
after_success: 'npm run coveralls'

@ -2,10 +2,18 @@
Simple express middleware for uploading files.
[![npm](https://img.shields.io/npm/v/express-fileupload.svg)](https://www.npmjs.org/package/express-fileupload)
[![Build Status](https://travis-ci.com/richardgirges/express-fileupload.svg?branch=master)](https://travis-ci.com/richardgirges/express-fileupload)
[![downloads per month](http://img.shields.io/npm/dm/express-fileupload.svg)](https://www.npmjs.org/package/express-fileupload)
[![CircleCI](https://circleci.com/gh/richardgirges/express-fileupload/tree/master.svg?style=svg)](https://circleci.com/gh/richardgirges/express-fileupload/tree/master)
[![Coverage Status](https://img.shields.io/coveralls/richardgirges/express-fileupload.svg)](https://coveralls.io/r/richardgirges/express-fileupload)
# Help us Improve express-fileupload
This package is still very much supported and maintained. But the more help the better. If you're interested any of the following:
* Ticket and PR triage
* Feature scoping and implementation
* Maintenance (upgrading packages, fixing security vulnerabilities, etc)
...please contact richardgirges '-at-' gmail.com
# Install
```bash
# With NPM
@ -119,7 +127,7 @@ debug | <ul><li><code>false</code>&nbsp;**(default)**</li><li><code>true</code><
uploadTimeout | <ul><li><code>60000</code>&nbsp;**(default)**</li><li><code>Integer</code></ul> | This defines how long to wait for data before aborting. Set to 0 if you want to turn off timeout checks.
# Help Wanted
Looking for additional maintainers. Please contact `richardgirges [ at ] gmail.com` if you're interested. Pull Requests are welcomed!
Looking for additional maintainers. Please contact `richardgirges [ at ] gmail.com` if you're interested. Pull Requests are welcome!
# Thanks & Credit
[Brian White](https://github.com/mscdex) for his stellar work on the [Busboy Package](https://github.com/mscdex/busboy) and the [connect-busboy Package](https://github.com/mscdex/connect-busboy)

@ -11,15 +11,19 @@ const app = express();
app.use(fileUpload());
app.post('/upload', function(req, res) {
let sampleFile;
let uploadPath;
if (!req.files || Object.keys(req.files).length === 0) {
return res.status(400).send('No files were uploaded.');
}
// The name of the input field (i.e. "sampleFile") is used to retrieve the uploaded file
let sampleFile = req.files.sampleFile;
sampleFile = req.files.sampleFile;
uploadPath = __dirname + '/somewhere/on/your/server/' + sampleFile.name;
// Use the mv() method to place the file somewhere on your server
sampleFile.mv('/somewhere/on/your/server/filename.jpg', function(err) {
sampleFile.mv(uploadPath, function(err) {
if (err)
return res.status(500).send(err);

@ -4,6 +4,7 @@ const path = require('path');
const processMultipart = require('./processMultipart');
const isEligibleRequest = require('./isEligibleRequest');
const { buildOptions, debugLog } = require('./utilities');
const busboy = require('busboy'); // eslint-disable-line no-unused-vars
const DEFAULT_OPTIONS = {
debug: false,
@ -26,7 +27,7 @@ const DEFAULT_OPTIONS = {
/**
* Expose the file upload middleware
* @param {Object} options - Middleware options.
* @param {DEFAULT_OPTIONS & busboy.BusboyConfig} options - Middleware options.
* @returns {Function} - express-fileupload middleware.
*/
module.exports = (options) => {

@ -51,6 +51,11 @@ module.exports = (options, req, res, next) => {
res.end(reason || 'Bad Request');
};
// Express proxies sometimes attach multipart data to a buffer
if (req.body instanceof Buffer) {
req.body = Object.create(null);
}
// Build multipart req.body fields
busboy.on('field', (field, val) => req.body = buildFields(req.body, field, val));

@ -1,9 +1,9 @@
const INVALID_KEYS = ['__proto__', 'constructor'];
const { isSafeFromPollution } = require("./utilities");
module.exports = function(data){
if (!data || data.length < 1) return {};
let d = {},
if (!data || data.length < 1) return Object.create(null);
let d = Object.create(null),
keys = Object.keys(data);
for (let i = 0; i < keys.length; i++) {
@ -13,24 +13,23 @@ module.exports = function(data){
keyParts = key
.replace(new RegExp(/\[/g), '.')
.replace(new RegExp(/\]/g), '')
.split('.');
.split('.');
for (let index = 0; index < keyParts.length; index++){
let k = keyParts[index];
// Ensure we don't allow prototype pollution
if (INVALID_KEYS.includes(k)) {
if (!isSafeFromPollution(current, k)) {
continue;
}
if (index >= keyParts.length - 1){
current[k] = value;
} else {
if (!current[k]) current[k] = !isNaN(keyParts[index + 1]) ? [] : {};
if (!current[k]) current[k] = !isNaN(keyParts[index + 1]) ? [] : Object.create(null);
current = current[k];
}
}
}
return d;
};
};

@ -2,7 +2,6 @@
const fs = require('fs');
const path = require('path');
const mkdirp = require('mkdirp');
const Readable = require('stream').Readable;
// Parameters for safe file name parsing.
@ -55,7 +54,15 @@ const errorFunc = (resolve, reject) => isFunc(reject) ? reject : resolve;
* @returns {Function}
*/
const promiseCallback = (resolve, reject) => {
return err => err ? errorFunc(resolve, reject)(err) : resolve();
let hasFired = false;
return (err) => {
if (hasFired) {
return;
}
hasFired = true;
return err ? errorFunc(resolve, reject)(err) : resolve();
};
};
/**
@ -71,6 +78,26 @@ const buildOptions = function(){
return result;
};
// The default prototypes for both objects and arrays.
// Used by isSafeFromPollution
const OBJECT_PROTOTYPE_KEYS = Object.getOwnPropertyNames(Object.prototype);
const ARRAY_PROTOTYPE_KEYS = Object.getOwnPropertyNames(Array.prototype);
/**
* Determines whether a key insertion into an object could result in a prototype pollution
* @param {Object} base - The object whose insertion we are checking
* @param {string} key - The key that will be inserted
*/
const isSafeFromPollution = (base, key) => {
// We perform an instanceof check instead of Array.isArray as the former is more
// permissive for cases in which the object as an Array prototype but was not constructed
// via an Array constructor or literal.
const TOUCHES_ARRAY_PROTOTYPE = (base instanceof Array) && ARRAY_PROTOTYPE_KEYS.includes(key);
const TOUCHES_OBJECT_PROTOTYPE = OBJECT_PROTOTYPE_KEYS.includes(key);
return !TOUCHES_ARRAY_PROTOTYPE && !TOUCHES_OBJECT_PROTOTYPE;
};
/**
* Builds request fields (using to build req.body and req.files)
* @param {Object} instance - request object.
@ -81,7 +108,11 @@ const buildOptions = function(){
const buildFields = (instance, field, value) => {
// Do nothing if value is not set.
if (value === null || value === undefined) return instance;
instance = instance || {};
instance = instance || Object.create(null);
if (!isSafeFromPollution(instance, field)) {
return instance;
}
// Non-array fields
if (!instance[field]) {
instance[field] = value;
@ -110,18 +141,17 @@ const checkAndMakeDir = (fileUploadOptions, filePath) => {
if (!filePath) return false;
const parentPath = path.dirname(filePath);
// Create folder if it does not exist.
if (!fs.existsSync(parentPath)) {
mkdirp.sync(parentPath);
}
if (!fs.existsSync(parentPath)) fs.mkdirSync(parentPath, { recursive: true });
// Check folder again and return the result.
return fs.existsSync(parentPath);
};
/**
* Delete file.
* @param {String} file - Path to the file to delete.
* Deletes a file.
* @param {string} file - Path to the file to delete.
* @param {Function} callback
*/
const deleteFile = (file, callback) => fs.unlink(file, err => err ? callback(err) : callback());
const deleteFile = (file, callback) => fs.unlink(file, callback);
/**
* Copy file via streams
@ -151,15 +181,15 @@ const copyFile = (src, dst, callback) => {
};
/**
* moveFile - moves the file from src to dst.
* moveFile: moves the file from src to dst.
* Firstly trying to rename the file if no luck copying it to dst and then deleteing src.
* @param {String} src - Path to the source file
* @param {String} dst - Path to the destination file.
* @param {string} src - Path to the source file
* @param {string} dst - Path to the destination file.
* @param {Function} callback - A callback function.
*/
const moveFile = (src, dst, callback) => fs.rename(src, dst, err => (!err
? callback()
: copyFile(src, dst, err => err ? callback(err) : deleteFile(src, callback))
const moveFile = (src, dst, callback) => fs.rename(src, dst, err => (err
? copyFile(src, dst, err => err ? callback(err) : deleteFile(src, callback))
: callback()
));
/**
@ -275,5 +305,6 @@ module.exports = {
saveBufferToFile,
parseFileName,
getTempFilename,
uriDecodeFileName
uriDecodeFileName,
isSafeFromPollution,
};

3576
package-lock.json generated

File diff suppressed because it is too large Load Diff

@ -1,19 +1,20 @@
{
"name": "express-fileupload",
"version": "1.1.10",
"version": "1.3.1",
"author": "Richard Girges <richardgirges@gmail.com>",
"description": "Simple express file upload middleware that wraps around Busboy",
"main": "./lib/index",
"scripts": {
"test": "istanbul cover node_modules/mocha/bin/_mocha -- -R spec",
"test": "nyc --reporter=html --reporter=text mocha -- -R spec",
"lint": "eslint ./",
"coveralls": "cat ./coverage/lcov.info | coveralls"
"lint:fix": "eslint --fix ./",
"coveralls": "nyc report --reporter=text-lcov | coveralls"
},
"dependencies": {
"busboy": "^0.3.1"
},
"engines": {
"node": ">=8.0.0"
"node": ">=12.0.0"
},
"keywords": [
"express",
@ -29,12 +30,12 @@
"repository": "richardgirges/express-fileupload",
"devDependencies": {
"body-parser": "^1.19.0",
"coveralls": "^3.0.14",
"eslint": "^7.20.0",
"coveralls": "^3.1.1",
"eslint": "^7.31.0",
"express": "^4.17.1",
"istanbul": "^0.4.5",
"md5": "^2.3.0",
"mocha": "^8.3.0",
"mocha": "^9.2.0",
"nyc": "^15.1.0",
"rimraf": "^3.0.2",
"supertest": "^4.0.2"
}

@ -19,7 +19,8 @@ const {
copyFile,
saveBufferToFile,
parseFileName,
uriDecodeFileName
uriDecodeFileName,
isSafeFromPollution
} = require('../lib/utilities');
const mockFile = 'basketball.png';
@ -135,7 +136,7 @@ describe('Test of the utilities functions', function() {
const result = parseFileName({}, name);
assert.equal(result.length, 255);
});
it(
'Strips away all non-alphanumeric characters (excluding hyphens/underscores) when enabled.',
() => {
@ -202,12 +203,12 @@ describe('Test of the utilities functions', function() {
it('buildOptions adds value to the result from the several source argumets', () => {
let result = buildOptions(source, sourceAddon);
assert.deepStrictEqual(result, expectedAddon);
});
});
});
//buildFields tests
describe('Test buildOptions function', () => {
it('buildFields does nothing if null value has been passed', () => {
let fields = null;
fields = buildFields(fields, 'test', null);
@ -293,7 +294,7 @@ describe('Test of the utilities functions', function() {
it('Failed if nonexistent file passed', function(done){
let filePath = path.join(uploadDir, getTempFilename());
deleteFile(filePath, function(err){
if (err) {
return done();
@ -330,11 +331,11 @@ describe('Test of the utilities functions', function() {
});
});
});
});
});
});
});
describe('Test copyFile function', function(){
beforeEach(function() {
server.clearUploadsDir();
@ -343,7 +344,7 @@ describe('Test of the utilities functions', function() {
it('Copy a file and check a hash', function(done) {
let srcPath = path.join(fileDir, mockFile);
let dstPath = path.join(uploadDir, mockFile);
copyFile(srcPath, dstPath, function(err){
if (err) {
return done(err);
@ -363,7 +364,7 @@ describe('Test of the utilities functions', function() {
it('Failed if wrong source file path passed', function(done){
let srcPath = path.join(fileDir, 'unknown');
let dstPath = path.join(uploadDir, mockFile);
copyFile(srcPath, dstPath, function(err){
if (err) {
return done();
@ -374,7 +375,7 @@ describe('Test of the utilities functions', function() {
it('Failed if wrong destination file path passed', function(done){
let srcPath = path.join(fileDir, 'unknown');
let dstPath = path.join('unknown', 'unknown');
copyFile(srcPath, dstPath, function(err){
if (err) {
return done();
@ -406,4 +407,68 @@ describe('Test of the utilities functions', function() {
});
});
});
describe('Test for no prototype pollution in buildFields', function() {
const prototypeFields = [
{ name: '__proto__', data: {} },
{ name: 'constructor', data: {} },
{ name: 'toString', data: {} }
];
const nonPrototypeFields = [
{ name: 'a', data: {} },
{ name: 'b', data: {} }
];
let fieldObject = undefined;
[...prototypeFields, ...nonPrototypeFields].forEach((field) => {
fieldObject = buildFields(fieldObject, field.name, field.data);
});
it(`Has ${nonPrototypeFields.length} keys`, () => {
assert.equal(Object.keys(fieldObject).length, nonPrototypeFields.length);
});
it(`Has null as its prototype`, () => {
assert.equal(Object.getPrototypeOf(fieldObject), null);
});
prototypeFields.forEach((field) => {
it(`${field.name} property is not an array`, () => {
// Note, Array.isArray is an insufficient test due to it returning false
// for Objects with an array prototype.
assert.equal(fieldObject[field.name] instanceof Array, false);
});
});
});
describe('Test for correct detection of prototype pollution', function() {
const validInsertions = [
{ base: {}, key: 'a' },
{ base: { a: 1 }, key: 'a' },
{ base: { __proto__: { a: 1 } }, key: 'a' },
{ base: [1], key: 0 },
{ base: { __proto__: [1] }, key: 0 }
];
const invalidInsertions = [
{ base: {}, key: '__proto__' },
{ base: {}, key: 'constructor' },
{ base: [1], key: '__proto__' },
{ base: [1], key: 'length' },
{ base: { __proto__: [1] }, key: 'length' }
];
validInsertions.forEach((insertion) => {
it(`Key ${insertion.key} should be valid for ${JSON.stringify(insertion.base)}`, () => {
assert.equal(isSafeFromPollution(insertion.base, insertion.key), true);
});
});
invalidInsertions.forEach((insertion) => {
it(`Key ${insertion.key} should not be valid for ${JSON.stringify(insertion.base)}`, () => {
assert.equal(isSafeFromPollution(insertion.base, insertion.key), false);
});
});
});
});

Loading…
Cancel
Save