From c56800a4e94e5b3af54a2b1ab57bf6a8c629c45b Mon Sep 17 00:00:00 2001 From: ztk Date: Wed, 23 Nov 2022 21:33:17 +0800 Subject: [PATCH 1/8] make `modifiedPaths` safer. --- lib/helpers/common.js | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/lib/helpers/common.js b/lib/helpers/common.js index c1433ce4748..13b2301533e 100644 --- a/lib/helpers/common.js +++ b/lib/helpers/common.js @@ -67,7 +67,19 @@ function flatten(update, path, options, schema) { * ignore */ -function modifiedPaths(update, path, result) { +function modifiedPaths(update, path, result, recursion=null) { + if(recursion == null){ + recursion = { + count: 0, + raw: {update, path} + }; + } + + recursion.count++; + if(recursion.count >= 1024){ + throw new Error(`Mongoose: bad update value, ${recursion.raw.update}, ${recursion.raw.path}`); + } + const keys = Object.keys(update || {}); const numKeys = keys.length; result = result || {}; @@ -83,7 +95,7 @@ function modifiedPaths(update, path, result) { val = val.toObject({ transform: false, virtuals: false }); } if (shouldFlatten(val)) { - modifiedPaths(val, path + key, result); + modifiedPaths(val, path + key, result, recursion); } } From 7974f004fae549ac11bbc247ac371fe6be4aa137 Mon Sep 17 00:00:00 2001 From: zzztttkkk Date: Thu, 24 Nov 2022 12:01:57 +0800 Subject: [PATCH 2/8] =?UTF-8?q?[=F0=9F=A7=AAtest]=20(test/helpers/common.t?= =?UTF-8?q?est.js):=20-?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- lib/helpers/common.js | 33 ++++++++++++++---------- test/helpers/common.test.js | 51 +++++++++++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 14 deletions(-) create mode 100644 test/helpers/common.test.js diff --git a/lib/helpers/common.js b/lib/helpers/common.js index 13b2301533e..08d9a1046e2 100644 --- a/lib/helpers/common.js +++ b/lib/helpers/common.js @@ -7,6 +7,8 @@ const Binary = require('../driver').get().Binary; const isBsonType = require('./isBsonType'); const isMongooseObject = require('./isMongooseObject'); +const MongooseError = require('../error'); +const util = require('util'); exports.flatten = flatten; exports.modifiedPaths = modifiedPaths; @@ -67,18 +69,21 @@ function flatten(update, path, options, schema) { * ignore */ -function modifiedPaths(update, path, result, recursion=null) { - if(recursion == null){ +function modifiedPaths(update, path, result, recursion = null) { + if (recursion == null) { recursion = { - count: 0, - raw: {update, path} + raw: { update, path }, + trace: new WeakSet() }; } - recursion.count++; - if(recursion.count >= 1024){ - throw new Error(`Mongoose: bad update value, ${recursion.raw.update}, ${recursion.raw.path}`); + if (recursion.trace.has(update)) { + throw new MongooseError(`a circular reference in the update value, updateValue: +${util.inspect(recursion.raw.update, { showHidden: false, depth: 1 })} +updatePath: ${recursion.raw.path} +-------`); } + recursion.trace.add(update); const keys = Object.keys(update || {}); const numKeys = keys.length; @@ -108,11 +113,11 @@ function modifiedPaths(update, path, result, recursion=null) { function shouldFlatten(val) { return val && - typeof val === 'object' && - !(val instanceof Date) && - !isBsonType(val, 'ObjectID') && - (!Array.isArray(val) || val.length !== 0) && - !(val instanceof Buffer) && - !isBsonType(val, 'Decimal128') && - !(val instanceof Binary); + typeof val === 'object' && + !(val instanceof Date) && + !isBsonType(val, 'ObjectID') && + (!Array.isArray(val) || val.length !== 0) && + !(val instanceof Buffer) && + !isBsonType(val, 'Decimal128') && + !(val instanceof Binary); } diff --git a/test/helpers/common.test.js b/test/helpers/common.test.js new file mode 100644 index 00000000000..fdbd1fa5bbd --- /dev/null +++ b/test/helpers/common.test.js @@ -0,0 +1,51 @@ +'use strict'; + +const start = require('../common'); + +const modifiedPaths = require('../../lib/helpers/common').modifiedPaths; +const mongoose = start.mongoose; +const { Schema } = mongoose; + + +describe('modifiedPaths, bad update value which has circular reference field', () => { + + it('values with obvious error', function() { + const objA = {}; + objA.a = objA; + + try { + modifiedPaths(objA, 'path', null); + } catch (e) { + console.log(e); + } + }); + + it('original error i made', async function() { + await mongoose.connect(start.uri); + + const test1Schema = new Schema({ + v: Number, + n: String + }); + + const Test1Model = mongoose.model('Test1', test1Schema); + + const test2Schema = new Schema({ + v: Number + }); + + const Test2Model = mongoose.model('Test2', test2Schema); + + for (let i = 0; i < 5; i++) { + const doc = new Test2Model({ v: i }); + await doc.save(); + } + + try { + // miss an `await` before `Test2Model.countDocuments()` + await Test1Model.updateOne({ n: 'x' }, { v: Test2Model.countDocuments() }, { upsert: true }); + } catch (e) { + console.log(e); + } + }); +}); From 88d584db40c87971753eee243a5cda1d2a6468cc Mon Sep 17 00:00:00 2001 From: zzztttkkk Date: Mon, 28 Nov 2022 10:32:41 +0800 Subject: [PATCH 3/8] =?UTF-8?q?[=F0=9F=90=9Bfix]=20(lib/help/common.js):?= =?UTF-8?q?=20update=20value=20can=20be=20null?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- lib/helpers/common.js | 5 +++-- test/helpers/common.test.js | 4 ++++ 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/lib/helpers/common.js b/lib/helpers/common.js index 08d9a1046e2..c12db0aacda 100644 --- a/lib/helpers/common.js +++ b/lib/helpers/common.js @@ -70,6 +70,8 @@ function flatten(update, path, options, schema) { */ function modifiedPaths(update, path, result, recursion = null) { + if (update == null) return result; + if (recursion == null) { recursion = { raw: { update, path }, @@ -80,8 +82,7 @@ function modifiedPaths(update, path, result, recursion = null) { if (recursion.trace.has(update)) { throw new MongooseError(`a circular reference in the update value, updateValue: ${util.inspect(recursion.raw.update, { showHidden: false, depth: 1 })} -updatePath: ${recursion.raw.path} --------`); +updatePath: '${recursion.raw.path}'`); } recursion.trace.add(update); diff --git a/test/helpers/common.test.js b/test/helpers/common.test.js index fdbd1fa5bbd..bd53b5ac213 100644 --- a/test/helpers/common.test.js +++ b/test/helpers/common.test.js @@ -9,6 +9,10 @@ const { Schema } = mongoose; describe('modifiedPaths, bad update value which has circular reference field', () => { + it('update value can be null', function() { + modifiedPaths(null, 'path', null); + }); + it('values with obvious error', function() { const objA = {}; objA.a = objA; From 62ad08d98aae94b72aa4d88c00424c07d5e48296 Mon Sep 17 00:00:00 2001 From: zzztttkkk Date: Mon, 28 Nov 2022 10:41:26 +0800 Subject: [PATCH 4/8] =?UTF-8?q?[=F0=9F=90=9Bfix]=20(lib/helpers/common.js)?= =?UTF-8?q?:=20-?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- lib/helpers/common.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/helpers/common.js b/lib/helpers/common.js index c12db0aacda..398dc4c3229 100644 --- a/lib/helpers/common.js +++ b/lib/helpers/common.js @@ -70,7 +70,7 @@ function flatten(update, path, options, schema) { */ function modifiedPaths(update, path, result, recursion = null) { - if (update == null) return result; + if (update == null) return result || {}; if (recursion == null) { recursion = { From 23d44e4e8eada462d23c73cc103a7e23fda85688 Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Mon, 28 Nov 2022 13:18:59 -0500 Subject: [PATCH 5/8] fix: quick fix to avoid checking non-objects for modified paths --- lib/helpers/common.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/helpers/common.js b/lib/helpers/common.js index 398dc4c3229..fb3dbdd098a 100644 --- a/lib/helpers/common.js +++ b/lib/helpers/common.js @@ -70,7 +70,9 @@ function flatten(update, path, options, schema) { */ function modifiedPaths(update, path, result, recursion = null) { - if (update == null) return result || {}; + if (update == null || typeof update !== 'object') { + return; + } if (recursion == null) { recursion = { From f899973c619c4777bd821dbaf1b305901d8e15dd Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Mon, 28 Nov 2022 13:27:45 -0500 Subject: [PATCH 6/8] test: make tests for #12719 more durable instead of relying on console log --- test/helpers/common.test.js | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/test/helpers/common.test.js b/test/helpers/common.test.js index bd53b5ac213..0e2cc147b44 100644 --- a/test/helpers/common.test.js +++ b/test/helpers/common.test.js @@ -17,11 +17,7 @@ describe('modifiedPaths, bad update value which has circular reference field', ( const objA = {}; objA.a = objA; - try { - modifiedPaths(objA, 'path', null); - } catch (e) { - console.log(e); - } + assert.throws(() => modifiedPaths(objA, 'path', null), /circular reference/); }); it('original error i made', async function() { @@ -44,12 +40,7 @@ describe('modifiedPaths, bad update value which has circular reference field', ( const doc = new Test2Model({ v: i }); await doc.save(); } - - try { - // miss an `await` before `Test2Model.countDocuments()` - await Test1Model.updateOne({ n: 'x' }, { v: Test2Model.countDocuments() }, { upsert: true }); - } catch (e) { - console.log(e); - } + + assert.rejects(() => Test1Model.updateOne({ n: 'x' }, { v: Test2Model.countDocuments() }, { upsert: true }), /circular reference/); }); }); From b393f87847644ae763068489a5e37155cbbeb213 Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Mon, 28 Nov 2022 13:32:35 -0500 Subject: [PATCH 7/8] style: fix lint --- test/helpers/common.test.js | 1 + 1 file changed, 1 insertion(+) diff --git a/test/helpers/common.test.js b/test/helpers/common.test.js index 0e2cc147b44..1e98764a347 100644 --- a/test/helpers/common.test.js +++ b/test/helpers/common.test.js @@ -1,5 +1,6 @@ 'use strict'; +const assert = require('assert'); const start = require('../common'); const modifiedPaths = require('../../lib/helpers/common').modifiedPaths; From f60b59a9be101400eb09ee5b1bd182cdeb77f336 Mon Sep 17 00:00:00 2001 From: Valeri Karpov Date: Mon, 28 Nov 2022 13:35:56 -0500 Subject: [PATCH 8/8] test: remove unnecessary test --- test/helpers/common.test.js | 27 +-------------------------- 1 file changed, 1 insertion(+), 26 deletions(-) diff --git a/test/helpers/common.test.js b/test/helpers/common.test.js index 1e98764a347..4d10afb8419 100644 --- a/test/helpers/common.test.js +++ b/test/helpers/common.test.js @@ -9,39 +9,14 @@ const { Schema } = mongoose; describe('modifiedPaths, bad update value which has circular reference field', () => { - it('update value can be null', function() { modifiedPaths(null, 'path', null); }); - it('values with obvious error', function() { + it('values with obvious error on circular reference', function() { const objA = {}; objA.a = objA; assert.throws(() => modifiedPaths(objA, 'path', null), /circular reference/); }); - - it('original error i made', async function() { - await mongoose.connect(start.uri); - - const test1Schema = new Schema({ - v: Number, - n: String - }); - - const Test1Model = mongoose.model('Test1', test1Schema); - - const test2Schema = new Schema({ - v: Number - }); - - const Test2Model = mongoose.model('Test2', test2Schema); - - for (let i = 0; i < 5; i++) { - const doc = new Test2Model({ v: i }); - await doc.save(); - } - - assert.rejects(() => Test1Model.updateOne({ n: 'x' }, { v: Test2Model.countDocuments() }, { upsert: true }), /circular reference/); - }); });