diff --git a/lib/mergeDeep.js b/lib/mergeDeep.js index ab278e5c2..0dd87a259 100644 --- a/lib/mergeDeep.js +++ b/lib/mergeDeep.js @@ -82,8 +82,23 @@ class MergeDeep { // If the target is empty, then all the source is added to additions if (t === undefined || t === null || (this.isEmpty(t) && !this.isEmpty(s))) { - additions = Object.assign(additions, s) - return ({ additions, modifications, hasChanges: true }) + if (firstInvocation && Array.isArray(s)) { + // Match the shape of the non-empty-target path, which unwinds the + // `__array` wrapper before returning: a top-level array diff must come + // back as an array, not as the index-keyed object Object.assign builds. + additions = s.slice() + } else { + // Assign into the container passed by recursive callers so they observe + // the additions through their own reference. Copy keys explicitly to + // skip prototype pollution vectors, like the comparison loop does. + for (const key of Object.keys(s)) { + if (key === '__proto__' || key === 'constructor') { + continue + } + additions[key] = s[key] + } + } + return ({ additions, modifications, deletions, hasChanges: true }) } // Compare the entries in the objects or elements of the array @@ -92,8 +107,8 @@ class MergeDeep { // So any property in the target that is not in the source is not treated as a deletion for (const key in source) { // Skip prototype pollution vectors - if (key === "__proto__" || key === "constructor") { - continue; + if (key === '__proto__' || key === 'constructor') { + continue } // Logic specific for Github // API response includes urls for resources, or other ignorable fields; we can ignore them diff --git a/test/unit/lib/mergeDeep.test.js b/test/unit/lib/mergeDeep.test.js index dd8bd60ba..cb68e39c2 100644 --- a/test/unit/lib/mergeDeep.test.js +++ b/test/unit/lib/mergeDeep.test.js @@ -809,6 +809,7 @@ entries: ] }, modifications: {}, + deletions: {}, hasChanges: true } const ignorableFields = [] @@ -1882,4 +1883,37 @@ branches: expect(same.additions).toEqual({}) expect(same.modifications).toEqual({}) }) + + it('CompareDeep returns top-level array additions in the same shape for empty and non-empty targets', () => { + const mergeDeep = new MergeDeep(log, jest.fn(), []) + const ruleset = { name: 'first-ruleset', enforcement: 'active' } + + // Target already has an entry: additions come back as an array + const withExisting = mergeDeep.compareDeep( + [{ name: 'other', enforcement: 'active' }], + [{ name: 'other', enforcement: 'active' }, ruleset] + ) + expect(withExisting.additions).toEqual([ruleset]) + + // Empty target (e.g. a repo receiving its first ruleset) must produce the + // same shape, not an index-keyed object like { 0: { ... } } + const firstEntry = mergeDeep.compareDeep([], [ruleset]) + expect(firstEntry.additions).toEqual([ruleset]) + expect(firstEntry.hasChanges).toEqual(true) + }) + + it('CompareDeep always includes deletions in its result', () => { + const mergeDeep = new MergeDeep(log, jest.fn(), []) + const emptyTarget = mergeDeep.compareDeep([], [{ name: 'a' }]) + expect(emptyTarget).toHaveProperty('deletions') + }) + + it('CompareDeep skips prototype pollution vectors when the target is empty', () => { + const mergeDeep = new MergeDeep(log, jest.fn(), []) + // JSON.parse creates `__proto__` as an own key, unlike object literals + const source = JSON.parse('{"name": "a", "__proto__": { "polluted": true }}') + const result = mergeDeep.compareDeep({}, source) + expect(result.additions).toEqual({ name: 'a' }) + expect(result.additions.polluted).toBeUndefined() + }) })