From 14b4e93b94e270be1a3906d106a772aa9f69afbc Mon Sep 17 00:00:00 2001 From: John Lees-Miller Date: Sat, 22 Jun 2019 15:22:23 +0100 Subject: [PATCH 1/6] Error on non-number limit rather than ignoring --- lib/types/multipart.js | 23 +++++++---------------- lib/types/urlencoded.js | 15 +++++---------- lib/utils.js | 15 +++++++++++++++ test/test-utils-get-limit.js | 16 ++++++++++++++++ 4 files changed, 43 insertions(+), 26 deletions(-) create mode 100644 test/test-utils-get-limit.js diff --git a/lib/types/multipart.js b/lib/types/multipart.js index b6d8e8b..6390518 100644 --- a/lib/types/multipart.js +++ b/lib/types/multipart.js @@ -12,7 +12,8 @@ var Dicer = require('dicer'); var parseParams = require('../utils').parseParams, decodeText = require('../utils').decodeText, - basename = require('../utils').basename; + basename = require('../utils').basename, + getLimit = require('../utils').getLimit; var RE_BOUNDARY = /^boundary$/i, RE_FIELD = /^form-data$/i, @@ -57,21 +58,11 @@ function Multipart(boy, cfg) { if (typeof boundary !== 'string') throw new Error('Multipart: Boundary not found'); - var fieldSizeLimit = (limits && typeof limits.fieldSize === 'number' - ? limits.fieldSize - : 1 * 1024 * 1024), - fileSizeLimit = (limits && typeof limits.fileSize === 'number' - ? limits.fileSize - : Infinity), - filesLimit = (limits && typeof limits.files === 'number' - ? limits.files - : Infinity), - fieldsLimit = (limits && typeof limits.fields === 'number' - ? limits.fields - : Infinity), - partsLimit = (limits && typeof limits.parts === 'number' - ? limits.parts - : Infinity); + var fieldSizeLimit = getLimit(limits, 'fieldSize', 1 * 1024 * 1024), + fileSizeLimit = getLimit(limits, 'fileSize', Infinity), + filesLimit = getLimit(limits, 'files', Infinity), + fieldsLimit = getLimit(limits, 'fields', Infinity), + partsLimit = getLimit(limits, 'parts', Infinity); var nfiles = 0, nfields = 0, diff --git a/lib/types/urlencoded.js b/lib/types/urlencoded.js index 361c804..0f61330 100644 --- a/lib/types/urlencoded.js +++ b/lib/types/urlencoded.js @@ -1,5 +1,6 @@ var Decoder = require('../utils').Decoder, - decodeText = require('../utils').decodeText; + decodeText = require('../utils').decodeText, + getLimit = require('../utils').getLimit; var RE_CHARSET = /^charset$/i; @@ -12,15 +13,9 @@ function UrlEncoded(boy, cfg) { parsedConType = cfg.parsedConType; this.boy = boy; - this.fieldSizeLimit = (limits && typeof limits.fieldSize === 'number' - ? limits.fieldSize - : 1 * 1024 * 1024); - this.fieldNameSizeLimit = (limits && typeof limits.fieldNameSize === 'number' - ? limits.fieldNameSize - : 100); - this.fieldsLimit = (limits && typeof limits.fields === 'number' - ? limits.fields - : Infinity); + this.fieldSizeLimit = getLimit(limits, 'fieldSize', 1 * 1024 * 1024); + this.fieldNameSizeLimit = getLimit(limits, 'fieldNameSize', 100); + this.fieldsLimit = getLimit(limits, 'fields', Infinity); var charset; for (var i = 0, len = parsedConType.length; i < len; ++i) { diff --git a/lib/utils.js b/lib/utils.js index 57d745f..8e899aa 100644 --- a/lib/utils.js +++ b/lib/utils.js @@ -170,3 +170,18 @@ function basename(path) { return (path === '..' || path === '.' ? '' : path); } exports.basename = basename; + + +function getLimit(limits, name, defaultLimit) { + if (!limits) return defaultLimit; + + var limit = limits[name]; + if (limit === undefined) return defaultLimit; + + if (typeof limit !== 'number') { + throw new Error('busboy: limit ' + name + ' is not a number'); + } + + return limit; +} +exports.getLimit = getLimit; diff --git a/test/test-utils-get-limit.js b/test/test-utils-get-limit.js new file mode 100644 index 0000000..49b3035 --- /dev/null +++ b/test/test-utils-get-limit.js @@ -0,0 +1,16 @@ +var getLimit = require('../lib/utils').getLimit; + +var assert = require('assert').strict; + +assert.deepStrictEqual(getLimit(undefined, 'fieldSize', 1), 1); +assert.deepStrictEqual(getLimit(undefined, 'fileSize', Infinity), Infinity); + +assert.deepStrictEqual(getLimit({}, 'fieldSize', 1), 1); +assert.deepStrictEqual(getLimit({}, 'fileSize', Infinity), Infinity); + +assert.deepStrictEqual(getLimit({ fieldSize: 0 }, 'fieldSize', 1), 0); +assert.deepStrictEqual(getLimit({ fileSize: 2 }, 'fileSize', 1), 2); + +assert.throws(function() { + getLimit({ fieldSize: '1' }, 'fieldSize', 1); +}, { message: 'busboy: limit fieldSize is not a number' }); From 1642554981f1bc974b6c5c4e90b52a9db4c59d2a Mon Sep 17 00:00:00 2001 From: John Lees-Miller Date: Sat, 22 Jun 2019 15:54:02 +0100 Subject: [PATCH 2/6] Use deepEqual for older node versions --- test/test-utils-get-limit.js | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/test/test-utils-get-limit.js b/test/test-utils-get-limit.js index 49b3035..092774a 100644 --- a/test/test-utils-get-limit.js +++ b/test/test-utils-get-limit.js @@ -1,15 +1,15 @@ var getLimit = require('../lib/utils').getLimit; -var assert = require('assert').strict; +var assert = require('assert'); -assert.deepStrictEqual(getLimit(undefined, 'fieldSize', 1), 1); -assert.deepStrictEqual(getLimit(undefined, 'fileSize', Infinity), Infinity); +assert.deepEqual(getLimit(undefined, 'fieldSize', 1), 1); +assert.deepEqual(getLimit(undefined, 'fileSize', Infinity), Infinity); -assert.deepStrictEqual(getLimit({}, 'fieldSize', 1), 1); -assert.deepStrictEqual(getLimit({}, 'fileSize', Infinity), Infinity); +assert.deepEqual(getLimit({}, 'fieldSize', 1), 1); +assert.deepEqual(getLimit({}, 'fileSize', Infinity), Infinity); -assert.deepStrictEqual(getLimit({ fieldSize: 0 }, 'fieldSize', 1), 0); -assert.deepStrictEqual(getLimit({ fileSize: 2 }, 'fileSize', 1), 2); +assert.deepEqual(getLimit({ fieldSize: 0 }, 'fieldSize', 1), 0); +assert.deepEqual(getLimit({ fileSize: 2 }, 'fileSize', 1), 2); assert.throws(function() { getLimit({ fieldSize: '1' }, 'fieldSize', 1); From a91cfe028373dcdf50a1875e9e25b8140622a23c Mon Sep 17 00:00:00 2001 From: John Lees-Miller Date: Sat, 22 Jun 2019 15:59:18 +0100 Subject: [PATCH 3/6] Use RegExp form of assert.throws for older node --- test/test-utils-get-limit.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/test-utils-get-limit.js b/test/test-utils-get-limit.js index 092774a..87d9486 100644 --- a/test/test-utils-get-limit.js +++ b/test/test-utils-get-limit.js @@ -13,4 +13,4 @@ assert.deepEqual(getLimit({ fileSize: 2 }, 'fileSize', 1), 2); assert.throws(function() { getLimit({ fieldSize: '1' }, 'fieldSize', 1); -}, { message: 'busboy: limit fieldSize is not a number' }); +}, /^Error: busboy: limit fieldSize is not a number$/); From c38b2488b65abf3d24189eb5ef692ccc32072002 Mon Sep 17 00:00:00 2001 From: John Lees-Miller Date: Sat, 22 Jun 2019 19:12:00 +0100 Subject: [PATCH 4/6] Apply suggestions from code review Co-Authored-By: mscdex --- lib/utils.js | 12 ++++++++---- test/test-utils-get-limit.js | 2 +- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/lib/utils.js b/lib/utils.js index 8e899aa..c2a0f9d 100644 --- a/lib/utils.js +++ b/lib/utils.js @@ -173,13 +173,17 @@ exports.basename = basename; function getLimit(limits, name, defaultLimit) { - if (!limits) return defaultLimit; + if (!limits) + return defaultLimit; var limit = limits[name]; - if (limit === undefined) return defaultLimit; + // Intentional double equals + if (limit == undefined) + return defaultLimit; - if (typeof limit !== 'number') { - throw new Error('busboy: limit ' + name + ' is not a number'); + // Ensure limit is a number and is not NaN + if (typeof limit !== 'number' || limit !== limit) { + throw new Error('Limit ' + name + ' is not a valid number'); } return limit; diff --git a/test/test-utils-get-limit.js b/test/test-utils-get-limit.js index 87d9486..fe62557 100644 --- a/test/test-utils-get-limit.js +++ b/test/test-utils-get-limit.js @@ -13,4 +13,4 @@ assert.deepEqual(getLimit({ fileSize: 2 }, 'fileSize', 1), 2); assert.throws(function() { getLimit({ fieldSize: '1' }, 'fieldSize', 1); -}, /^Error: busboy: limit fieldSize is not a number$/); +}, /^Error: Limit fieldSize is not a valid number$/); From 0c0f9ceb90e1bdea4e89d1192f6e50de493d1ac4 Mon Sep 17 00:00:00 2001 From: John Lees-Miller Date: Sat, 22 Jun 2019 19:15:25 +0100 Subject: [PATCH 5/6] Add tests for null and NaN limits --- test/test-utils-get-limit.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/test-utils-get-limit.js b/test/test-utils-get-limit.js index fe62557..d4a19c9 100644 --- a/test/test-utils-get-limit.js +++ b/test/test-utils-get-limit.js @@ -7,6 +7,8 @@ assert.deepEqual(getLimit(undefined, 'fileSize', Infinity), Infinity); assert.deepEqual(getLimit({}, 'fieldSize', 1), 1); assert.deepEqual(getLimit({}, 'fileSize', Infinity), Infinity); +assert.deepEqual(getLimit({ fieldSize: null }, 'fieldSize', 1), 1); +assert.deepEqual(getLimit({ fileSize: null }, 'fileSize', Infinity), Infinity); assert.deepEqual(getLimit({ fieldSize: 0 }, 'fieldSize', 1), 0); assert.deepEqual(getLimit({ fileSize: 2 }, 'fileSize', 1), 2); @@ -14,3 +16,7 @@ assert.deepEqual(getLimit({ fileSize: 2 }, 'fileSize', 1), 2); assert.throws(function() { getLimit({ fieldSize: '1' }, 'fieldSize', 1); }, /^Error: Limit fieldSize is not a valid number$/); + +assert.throws(function() { + getLimit({ fieldSize: NaN }, 'fieldSize', 1); +}, /^Error: Limit fieldSize is not a valid number$/); From a2d4b94e304340ba59f85fc09e42ba7e22161ba9 Mon Sep 17 00:00:00 2001 From: John Lees-Miller Date: Sun, 23 Jun 2019 07:52:46 +0100 Subject: [PATCH 6/6] Use strictEqual in tests --- test/test-utils-get-limit.js | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/test/test-utils-get-limit.js b/test/test-utils-get-limit.js index d4a19c9..2d1c7fe 100644 --- a/test/test-utils-get-limit.js +++ b/test/test-utils-get-limit.js @@ -2,16 +2,16 @@ var getLimit = require('../lib/utils').getLimit; var assert = require('assert'); -assert.deepEqual(getLimit(undefined, 'fieldSize', 1), 1); -assert.deepEqual(getLimit(undefined, 'fileSize', Infinity), Infinity); +assert.strictEqual(getLimit(undefined, 'fieldSize', 1), 1); +assert.strictEqual(getLimit(undefined, 'fileSize', Infinity), Infinity); -assert.deepEqual(getLimit({}, 'fieldSize', 1), 1); -assert.deepEqual(getLimit({}, 'fileSize', Infinity), Infinity); -assert.deepEqual(getLimit({ fieldSize: null }, 'fieldSize', 1), 1); -assert.deepEqual(getLimit({ fileSize: null }, 'fileSize', Infinity), Infinity); +assert.strictEqual(getLimit({}, 'fieldSize', 1), 1); +assert.strictEqual(getLimit({}, 'fileSize', Infinity), Infinity); +assert.strictEqual(getLimit({ fieldSize: null }, 'fieldSize', 1), 1); +assert.strictEqual(getLimit({ fileSize: null }, 'fileSize', Infinity), Infinity); -assert.deepEqual(getLimit({ fieldSize: 0 }, 'fieldSize', 1), 0); -assert.deepEqual(getLimit({ fileSize: 2 }, 'fileSize', 1), 2); +assert.strictEqual(getLimit({ fieldSize: 0 }, 'fieldSize', 1), 0); +assert.strictEqual(getLimit({ fileSize: 2 }, 'fileSize', 1), 2); assert.throws(function() { getLimit({ fieldSize: '1' }, 'fieldSize', 1);