fix(commons): prevent deepMerge from mutating source objects (#5202)

Author: svozza

package-lock.json

@@ -44,7 +44,8 @@
     "@aws/lambda-invoke-store": "0.2.4"
   },
   "devDependencies": {
-    "@aws-lambda-powertools/testing-utils": "file:../testing"
+    "@aws-lambda-powertools/testing-utils": "file:../testing",
+    "fast-check": "^4.7.0"
   },
   "main": "./lib/cjs/index.js",
   "types": "./lib/cjs/index.d.ts",

packages/commons/package.json

@@ -14,44 +14,100 @@ const isPlainObject = (value: unknown): value is Record<string, unknown> => {
 };
 
 /**
- * Merge source array items into target array by index.
+ * Clone a source item for safe assignment into a target array.
+ * Plain objects are cloned via mergeRecursive, arrays via mergeArrayItemsByIndex,
+ * and primitives (including undefined) are returned as-is.
+ * Returns `{ skip: true }` only when a circular array reference is detected.
  *
- * When both source and target items at the same index are plain objects,
- * they are merged recursively. Otherwise, the source item replaces the target.
+ * @internal
+ */
+const cloneItem = (
+  srcItem: unknown,
+  ancestors: object[]
+): { skip: true } | { skip: false; value: unknown } => {
+  if (isPlainObject(srcItem)) {
+    const cloned: Record<string, unknown> = {};
+    ancestors.push(srcItem);
+    mergeRecursive(cloned, srcItem, ancestors);
+    ancestors.pop();
+
+    return { skip: false, value: cloned };
+  }
+
+  if (Array.isArray(srcItem)) {
+    if (ancestors.includes(srcItem)) return { skip: true };
+    const cloned: unknown[] = [];
+    ancestors.push(srcItem);
+    mergeArrayItemsByIndex(cloned, srcItem, ancestors);
+    ancestors.pop();
+
+    return { skip: false, value: cloned };
+  }
+
+  return { skip: false, value: srcItem };
+};
+
+/**
+ * Merge a single source item into a target array at the given index.
  *
  * @internal
  */
-const mergeArrayItemsByIndex = (
+const mergeArrayItem = (
   targetArray: unknown[],
-  sourceArray: unknown[],
+  index: number,
+  srcItem: unknown,
   ancestors: object[]
 ): void => {
-  for (let i = 0; i < sourceArray.length; i++) {
-    const srcItem = sourceArray[i];
-    const tgtItem = targetArray[i];
+  const tgtItem = targetArray[index];
+  const isSrcPlainObject = isPlainObject(srcItem);
 
-    const isSrcPlainObject = isPlainObject(srcItem);
+  // Skip circular plain object references
+  if (isSrcPlainObject && ancestors.includes(srcItem)) return;
 
-    // Skip objects in the current ancestor chain to prevent circular references
-    if (isSrcPlainObject && ancestors.includes(srcItem)) {
-      continue;
-    }
+  // Merge two plain objects recursively
+  if (isSrcPlainObject && isPlainObject(tgtItem)) {
+    ancestors.push(srcItem);
+    mergeRecursive(tgtItem, srcItem, ancestors);
+    ancestors.pop();
+
+    return;
+  }
 
-    // Merge nested plain objects recursively
-    if (isSrcPlainObject && isPlainObject(tgtItem)) {
+  // Merge two arrays by index
+  if (Array.isArray(srcItem) && Array.isArray(tgtItem)) {
+    if (!ancestors.includes(srcItem)) {
       ancestors.push(srcItem);
-      mergeRecursive(tgtItem, srcItem, ancestors);
+      mergeArrayItemsByIndex(tgtItem, srcItem, ancestors);
       ancestors.pop();
-      continue;
     }
 
-    // Otherwise, replace the target item with source item
-    if (srcItem !== undefined || tgtItem === undefined) {
-      targetArray[i] = srcItem;
+    return;
+  }
+
+  // Replace with a cloned copy of the source item
+  if (srcItem !== undefined || tgtItem === undefined) {
+    const result = cloneItem(srcItem, ancestors);
+    if (!result.skip) {
+      targetArray[index] = result.value;
     }
   }
 };
 
+/**
+ * Merge source array items into target array by index.
+ *
+ * @internal
+ */
+const mergeArrayItemsByIndex = (
+  targetArray: unknown[],
+  sourceArray: unknown[],
+  ancestors: object[]
+): void => {
+  for (let i = 0; i < sourceArray.length; i++) {
+    mergeArrayItem(targetArray, i, sourceArray[i], ancestors);
+  }
+};
+
 /**
  * Handle merging when source value is an array.
  *
@@ -65,7 +121,9 @@ const handleArrayMerge = (
   ancestors: object[]
 ): void => {
   if (!Array.isArray(targetValue)) {
-    target[key] = [...sourceArray];
+    const freshArray: unknown[] = [];
+    mergeArrayItemsByIndex(freshArray, sourceArray, ancestors);
+    target[key] = freshArray;
     return;
   }
 

packages/commons/src/deepMerge.ts

@@ -0,0 +1,86 @@
+import fc from 'fast-check';
+import { describe, expect, it } from 'vitest';
+import { deepMerge } from '../../src/deepMerge.js';
+
+/**
+ * Arbitrary that generates JSON-like plain objects (no class instances,
+ * no circular refs) suitable for deepMerge property tests.
+ */
+const jsonObject = (): fc.Arbitrary<Record<string, unknown>> =>
+  fc.dictionary(
+    fc.string({ minLength: 1, maxLength: 5 }),
+    fc.letrec((tie) => ({
+      value: fc.oneof(
+        { depthSize: 'small' },
+        fc.string(),
+        fc.integer(),
+        fc.boolean(),
+        fc.constant(null),
+        fc.array(tie('value'), { maxLength: 3, depthSize: 'small' }),
+        fc.dictionary(fc.string({ minLength: 1, maxLength: 5 }), tie('value'), {
+          maxKeys: 3,
+        })
+      ),
+    })).value,
+    { maxKeys: 5 }
+  );
+
+describe('deepMerge property tests', () => {
+  it('never mutates source objects', () => {
+    fc.assert(
+      fc.property(
+        fc.array(jsonObject(), { minLength: 1, maxLength: 4 }),
+        (sources) => {
+          const snapshots = sources.map((s) => structuredClone(s));
+
+          deepMerge({}, ...sources);
+
+          for (let i = 0; i < sources.length; i++) {
+            expect(sources[i]).toEqual(snapshots[i]);
+          }
+        }
+      ),
+      { numRuns: 200 }
+    );
+  });
+
+  it('returns the target reference', () => {
+    fc.assert(
+      fc.property(jsonObject(), jsonObject(), (target, source) => {
+        const result = deepMerge(target, source);
+        expect(result).toBe(target);
+      }),
+      { numRuns: 200 }
+    );
+  });
+
+  it('result contains all top-level keys from all sources', () => {
+    fc.assert(
+      fc.property(
+        fc.array(jsonObject(), { minLength: 1, maxLength: 4 }),
+        (sources) => {
+          const result = deepMerge({}, ...sources);
+          const resultKeys = new Set(Object.keys(result));
+
+          for (const source of sources) {
+            for (const key of Object.keys(source)) {
+              expect(resultKeys.has(key)).toBe(true);
+            }
+          }
+        }
+      ),
+      { numRuns: 200 }
+    );
+  });
+
+  it('merging a source into empty object then merging same source again is idempotent', () => {
+    fc.assert(
+      fc.property(jsonObject(), (source) => {
+        const first = deepMerge({}, source);
+        const second = deepMerge(structuredClone(first), source);
+        expect(second).toEqual(first);
+      }),
+      { numRuns: 200 }
+    );
+  });
+});

packages/commons/tests/unit/deepMerge.property.test.ts

@@ -332,6 +332,26 @@ describe('Function: deepMerge', () => {
       expect(result).toHaveProperty('arr[1]', 2);
     });
 
+    it('skips circular arrays nested inside arrays', () => {
+      // Prepare - outer array contains itself as an element
+      const outer: unknown[] = [1, 2];
+      outer.push(outer); // outer[2] = outer (circular)
+      const target = {
+        arr: [
+          [10, 20],
+          [30, 40],
+          [50, 60],
+        ],
+      };
+      const source = { arr: outer };
+
+      // Act
+      const result = deepMerge(target, source);
+
+      // Assess - index 0 and 1 are primitives replacing arrays, index 2 is circular so skipped
+      expect(result).toEqual({ arr: [1, 2, [50, 60]] });
+    });
+
     it('skips array that references an ancestor array', () => {
       // Prepare
       const arr: unknown[] = [];
@@ -556,6 +576,75 @@ describe('Function: deepMerge', () => {
     });
   });
 
+  describe('Source immutability', () => {
+    it('does not mutate sources when merging arrays of objects across multiple sources', () => {
+      // Prepare
+      const source1 = { a: [{ a: 1 }] };
+      const source2 = { a: [{ b: 2 }] };
+
+      // Act
+      const result = deepMerge({}, source1, source2);
+
+      // Assess
+      expect(result).toEqual({ a: [{ a: 1, b: 2 }] });
+      expect(source1).toEqual({ a: [{ a: 1 }] });
+      expect(source2).toEqual({ a: [{ b: 2 }] });
+    });
+
+    it('does not mutate sources when merging nested arrays across multiple sources', () => {
+      // Prepare
+      const src1 = { a: [[1, 2, 3]] };
+      const src2 = { a: [[3, 4]] };
+
+      // Act
+      const result = deepMerge({}, src1, src2);
+
+      // Assess
+      expect(result).toEqual({ a: [[3, 4, 3]] });
+      expect(src1).toEqual({ a: [[1, 2, 3]] });
+      expect(src2).toEqual({ a: [[3, 4]] });
+    });
+
+    it('does not mutate a single source with array of objects merged into empty target', () => {
+      // Prepare
+      const source = { a: [{ x: 1 }, { y: 2 }] };
+
+      // Act
+      const result = deepMerge({}, source);
+      result.a[0].x = 999;
+
+      // Assess
+      expect(source).toEqual({ a: [{ x: 1 }, { y: 2 }] });
+    });
+
+    it('does not mutate sources with plain objects (non-array path)', () => {
+      // Prepare
+      const source1 = { nested: { a: 1 } };
+      const source2 = { nested: { b: 2 } };
+
+      // Act
+      deepMerge({}, source1, source2);
+
+      // Assess
+      expect(source1).toEqual({ nested: { a: 1 } });
+      expect(source2).toEqual({ nested: { b: 2 } });
+    });
+
+    it('does not mutate sources with mixed arrays and nested objects', () => {
+      // Prepare
+      const source1 = { data: [{ items: [{ id: 1 }] }] };
+      const source2 = { data: [{ items: [{ name: 'a' }] }] };
+
+      // Act
+      const result = deepMerge({}, source1, source2);
+
+      // Assess
+      expect(result).toEqual({ data: [{ items: [{ id: 1, name: 'a' }] }] });
+      expect(source1).toEqual({ data: [{ items: [{ id: 1 }] }] });
+      expect(source2).toEqual({ data: [{ items: [{ name: 'a' }] }] });
+    });
+  });
+
   describe('Edge cases', () => {
     it('handles Date objects (treats as value, not merged)', () => {
       // Prepare