Skip to content

fix(commons): skip prototype-polluting keys in _.merge#3690

Merged
marshallswain merged 1 commit into
dovefrom
fix/commons-merge-proto-pollution-dove
Jun 4, 2026
Merged

fix(commons): skip prototype-polluting keys in _.merge#3690
marshallswain merged 1 commit into
dovefrom
fix/commons-merge-proto-pollution-dove

Conversation

@marshallswain

Copy link
Copy Markdown
Member

Summary

The recursive _.merge helper in @feathersjs/commons iterates Object.keys(source). When the source came from JSON.parse('{"__proto__":...}'), __proto__ is returned as an own-enumerable key, so the recursive call resolves target['__proto__'] to Object.prototype and writes onto it.

This adds the standard guard to skip __proto__, constructor, and prototype keys during iteration.

Changes

  • src/index.ts: skip the three prototype-mutating keys in the merge loop.
  • test/utils.test.ts: regression test confirming a JSON-parsed __proto__/constructor.prototype source no longer mutates Object.prototype, fresh objects, or the target.

Notes

Reported-by: Andrew Ridings (@ridingsa)

Object.keys() returns __proto__ as an own enumerable key for
JSON-parsed sources, causing the recursive merge to write onto
Object.prototype. Skip __proto__/constructor/prototype keys.

Reported-by: Andrew Ridings (@ridingsa)
@marshallswain marshallswain merged commit 28b3c03 into dove Jun 4, 2026
4 checks passed
@marshallswain marshallswain deleted the fix/commons-merge-proto-pollution-dove branch June 4, 2026 02:12
@tjenkinson

Copy link
Copy Markdown

Maybe a better alternative is https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/defineProperty as this will never touch the prototype? Then we wouldn't need to add the exclusions?

Unlike accessor properties, data properties are always set on the object itself, not on a prototype. However, if a non-writable data property is inherited, it is still prevented from being modified on the object.

https://claude.ai/share/609aae22-2720-40e6-8479-04e023e37776

Cc @ridingsa

WDYT?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants