[BUGFIX] Prototype pollution via unsafe object merge

[GAURAV-1313]

Merges without checking for proto keys.

[netlify]

✅ Deploy Preview for nofriction canceled.

Name	Link
🔨 Latest commit	b217c6c
🔍 Latest deploy log	https://app.netlify.com/projects/nofriction/deploys/6a3c32328ce552000969ed62

[GAURAV-1313]

AI Code Review

Summary

The PR adds a simple object merging function that performs a shallow copy of source properties into a target object. The method lacks input validation and does not guard against potential prototype pollution or mutation hazards, presenting a moderate risk if used unguarded in production.

Key Findings

Bugs

• [MEDIUM] mergeObjects function — missing input validation on target and source parameters to ensure they are objects — could cause runtime errors if non-objects are passed
• [MEDIUM] mergeObjects function — unguarded property assignment on target from source without checks for keys like 'proto' or 'constructor' — risks prototype pollution vulnerability

Security

• HIGH The mergeObjects function performs an unguarded shallow merge of source properties into the target object using a for-in loop without hasOwnProperty checks. This opens a prototype pollution vulnerability if attacker-controlled objects are passed as source, allowing arbitrary modification of Object.prototype properties.

Recommended Fixes

General

• Add validation at the start of mergeObjects to confirm both target and source are non-null objects and throw an error otherwise
• Exclude or explicitly disallow special keys like 'proto', 'constructor', and 'prototype' during iteration to mitigate prototype pollution risks
• Consider cloning objects deeply or use built-in safe utilities if deep merges are required to avoid mutation of nested objects and unintended sharing

Security Fixes

• The mergeObjects function performs an unguarded shallow merge of source properties into the target object using a for-in loop without hasOwnProperty checks. This opens a prototype pollution vulnerability if attacker-controlled objects are passed as source, allowing arbitrary modification of Object.prototype properties. — Add a hasOwnProperty check inside the loop: for (let key in source) { if (Object.prototype.hasOwnProperty.call(source, key)) { target[key] = source[key]; } } Alternatively, use safer libraries like lodash.merge or implement deep merging with explicit key whitelisting to prevent prototype pollution.

Suggested Tests

mergeObjects

const { mergeObjects } = require('./merge');
describe('mergeObjects', () => {
  let target;
  beforeEach(() => {
    target = { a: 1, b: 2 };
  });

  test('should merge properties from source into target (happy path)', () => {
    const source = { b: 3, c: 4 };
    const result = mergeObjects(target, source);
    expect(result).toEqual({ a: 1, b: 3, c: 4 });
    expect(result).toBe(target); // returns the original target object
  });

  test('should handle empty source object without modifying target', () => {
    const source = {};
    const originalTarget = { ...target };
    const result = mergeObjects(target, source);
    expect(result).toEqual(originalTarget);
    expect(result).toBe(target);
  });

  test('should handle source with a single key', () => {
    const source = { c: 5 };
    const result = mergeObjects(target, source);
    expect(result).toEqual({ a: 1, b: 2, c: 5 });
  });

  test('should overwrite existing target keys with source keys', () => {
    const source = { a: 100, b: 200 };
    const result = mergeObjects(target, source);
    expect(result).toEqual({ a: 100, b: 200 });
  });

  test('should handle source with keys having undefined or null values', () => {
    const source = { a: undefined, d: null };
    const result = mergeObjects(target, source);
    expect(result).toEqual({ a: undefined, b: 2, d: null });
  });

  test('should handle source with numeric and symbol keys', () => {
    const symbolKey = Symbol('sym');
    const source = { 42: 'numberKey', [symbolKey]: 'symbolValue' };
    const result = mergeObjects(target, source);
    expect(result[42]).toBe('numberKey');
    expect(result[symbolKey]).toBe('symbolValue');
  });

  test('should copy all enumerable own properties including inherited enumerable properties if any', () => {
    const parent = { inherited: 'yes' };
    const source = Object.create(parent);
    source.own = 'ownValue';
    // for..in includes inherited enumerable keys
    const result = mergeObjects({}, source);
    expect(result).toHaveProperty('own', 'ownValue');
    expect(result).toHaveProperty('inherited', 'yes');
  });

  test('should not mutate the source object', () => {
    const source = { c: 4 };
    const sourceCopy = { ...source };
    mergeObjects(target, source);
    expect(source).toEqual(sourceCopy);
  });

  test('should throw if target is null or undefined', () => {
    expect(() => mergeObjects(null, { a: 1 })).toThrow(TypeError);
    expect(() => mergeObjects(undefined, { a: 1 })).toThrow(TypeError);
  });

  test('should throw if source is null or undefined', () => {
    expect(() => mergeObjects({}, null)).toThrow(TypeError);
    expect(() => mergeObjects({}, undefined)).toThrow(TypeError);
  });

  test('should handle source with non-object values gracefully', () => {
    expect(() => mergeObjects({}, 123)).toThrow(TypeError);
    expect(() => mergeObjects({}, 'string')).toThrow(TypeError);
    expect(() => mergeObjects({}, true)).toThrow(TypeError);
  });
});

Auto-Fix Available

Add the apply-ai-fixes label to this pull request to have RepoSpace
automatically generate and commit code fixes for the issues listed above.

---

3 issues found. Reviewed by RepoSpace AI.