Repository navigation
fix: patch prototype pollution vulnerability in deepMix and mix - #131
Conversation
Co-authored-by: hustcc <7856674+hustcc@users.noreply.github.com>
Co-authored-by: hustcc <7856674+hustcc@users.noreply.github.com>
|
@copilot mix 方法是不是也有类似的问题 |
Agent-Logs-Url: https://github.com/antvis/util/sessions/7731b37f-784f-4d46-bfc5-ea4bf127da73 Co-authored-by: hustcc <7856674+hustcc@users.noreply.github.com>
|
@copilot 升级版本号,我准备发版本。 |
Agent-Logs-Url: https://github.com/antvis/util/sessions/4c19cc22-f38e-4578-b3f6-9bc3d30b8ccf Co-authored-by: hustcc <7856674+hustcc@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR patches a prototype pollution vulnerability in the object-merge utilities (deepMix and mix) by skipping dangerous keys during merges, and adds regression tests to prevent reintroduction.
Changes:
- Add
__proto__/constructor/prototypekey skipping guards insrc/lodash/deep-mix.tsandsrc/lodash/mix.ts. - Add new Jest unit tests covering standard merge behavior and prototype pollution vectors for both functions.
- Update
.gitignoreto ignorepackage-lock.json.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
src/lodash/mix.ts |
Adds an early-continue guard for prototype-pollution keys during shallow merge. |
src/lodash/deep-mix.ts |
Adds a similar guard inside the hasOwn check during deep merge recursion. |
__tests__/unit/lodash/mix.spec.ts |
Introduces regression tests for mix merge behavior and pollution vectors. |
__tests__/unit/lodash/deep-mix.spec.ts |
Introduces regression tests for deepMix merge behavior and pollution vectors. |
.gitignore |
Ignores package-lock.json. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ) { | ||
| continue; | ||
| } | ||
| if (obj.hasOwnProperty(key) && obj[key] !== undefined) { |
There was a problem hiding this comment.
_mix uses obj.hasOwnProperty(key), which can throw for Object.create(null) inputs and can be overridden on attacker-controlled objects. Use a safe check like Object.prototype.hasOwnProperty.call(obj, key) (or reuse the hasOwn helper pattern from deep-mix.ts) to avoid DoS and ensure the guard is reliable.
| if (obj.hasOwnProperty(key) && obj[key] !== undefined) { | |
| if (Object.prototype.hasOwnProperty.call(obj, key) && obj[key] !== undefined) { |
| @@ -0,0 +1,25 @@ | |||
| import mix from '../../../src/lodash/mix'; | |||
There was a problem hiding this comment.
These tests import the implementation file directly. Most existing lodash unit tests import via ../../../src/lodash (public entrypoint), which better matches consumer usage and also ensures the index exports stay correct. Consider switching to import { mix } from '../../../src/lodash'.
| import mix from '../../../src/lodash/mix'; | |
| import { mix } from '../../../src/lodash'; |
| it('does not pollute Object.prototype via __proto__', () => { | ||
| const payload = JSON.parse('{"__proto__": {"polluted": true}}'); | ||
| mix({}, payload); | ||
| expect((Object.prototype as any).polluted).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('does not pollute via constructor key', () => { | ||
| const payload = JSON.parse('{"constructor": {"prototype": {"polluted": true}}}'); | ||
| mix({}, payload); | ||
| expect((Object.prototype as any).polluted).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('does not pollute via prototype key', () => { | ||
| mix({} as any, { prototype: { polluted: true } } as any); | ||
| expect((Object.prototype as any).polluted).toBeUndefined(); | ||
| }); |
There was a problem hiding this comment.
Prototype-pollution regression tests mutate global state if the protection ever regresses, which can cascade failures into unrelated tests. Add a beforeEach/afterEach cleanup (e.g., delete (Object.prototype as any).polluted) to keep the suite isolated and deterministic.
| @@ -0,0 +1,30 @@ | |||
| import deepMix from '../../../src/lodash/deep-mix'; | |||
There was a problem hiding this comment.
These tests import the implementation file directly. Most existing lodash unit tests import via ../../../src/lodash (public entrypoint), which better matches consumer usage and also ensures the index exports stay correct. Consider switching to import { deepMix } from '../../../src/lodash'.
| import deepMix from '../../../src/lodash/deep-mix'; | |
| import { deepMix } from '../../../src/lodash'; |
| it('does not pollute Object.prototype via __proto__', () => { | ||
| const payload = JSON.parse('{"__proto__": {"polluted": true}}'); | ||
| deepMix({}, payload); | ||
| expect((Object.prototype as any).polluted).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('does not pollute via constructor.prototype', () => { | ||
| const payload = JSON.parse('{"constructor": {"prototype": {"polluted": true}}}'); | ||
| deepMix({}, payload); | ||
| expect((Object.prototype as any).polluted).toBeUndefined(); | ||
| }); | ||
|
|
||
| it('does not pollute via prototype key', () => { | ||
| deepMix({}, { prototype: { polluted: true } }); | ||
| expect((Object.prototype as any).polluted).toBeUndefined(); | ||
| }); |
There was a problem hiding this comment.
Prototype-pollution regression tests can leave Object.prototype modified if the guard breaks, affecting later tests and making failures harder to interpret. Add cleanup in beforeEach/afterEach (e.g., delete (Object.prototype as any).polluted) to keep tests isolated.
nrps9909
left a comment
There was a problem hiding this comment.
AI assistance disclosure: Codex was used to trace the downstream Ant Design Charts report to this utility, reproduce the published exploit, inspect this exact diff, run the repository validation, and construct the additional adversarial probes below. I verified every reported result before submitting this review.
Reviewed exact head 965c783 against base 426c79b.
The dangerous-key guard in deepMix does block the currently published payload: after building this head, merging JSON.parse of the proto payload leaves Object.prototype unchanged. However, the security hardening added to mix still trusts the attacker-controlled source method at src/lodash/mix.ts:12:
obj.hasOwnProperty(key)
Two independent exact-head regressions fail before any safe property can be copied:
- mix({}, Object.assign(Object.create(null), { safe: true }))
TypeError: obj.hasOwnProperty is not a function - mix({}, JSON.parse of { hasOwnProperty: null, safe: true })
TypeError: obj.hasOwnProperty is not a function
Because mix is now explicitly included in this vulnerability fix, source ownership needs to be checked independently of the source object, for example with Object.prototype.hasOwnProperty.call or the same null-safe hasOwn helper already present in deep-mix.ts. Please add both null-prototype and shadowed-hasOwnProperty regressions so an untrusted merge source cannot turn the security path into a controllable denial of service.
There is also one changed-file gate failure: Prettier reports src/lodash/mix.ts as unformatted and collapses the multiline dangerous-key condition.
Validation summary:
- existing focused deepMix and mix tests: 8 passed
- independent security matrix: original pollution case passed; the two ownership cases failed
- complete repository: 35 suites and 107 tests passed
- CommonJS, ESM, and UMD builds plus both size limits passed
- ESLint reported zero errors and three pre-existing warnings
- full lint remained red for three Prettier files, including changed src/lodash/mix.ts; the other two are unchanged base files
This is a narrowly scoped request: retain the current dangerous-key guard, make the ownership check source-independent, add the two regressions, and format the changed source. The downstream @ant-design/plots 2.6.8 / @antv/g2 5.4.8 reproduction remains exploitable while @antv/util 3.3.11 is the published version.
_deepMixinsrc/lodash/deep-mix.tsto skip dangerous keys (__proto__,constructor,prototype)deepMixwith prototype pollution regression cases_mixinsrc/lodash/mix.tsto also skip__proto__andprototypekeysmixwith prototype pollution regression cases3.3.12Original prompt
This pull request was created from Copilot chat.
🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.