Skip to content

fix(utils): prevent prototype pollution in deepExtend - #3287

Open
lahirumaramba wants to merge 4 commits into
mainfrom
lm-fix-deep-extend
Open

lahirumaramba wants to merge 4 commits into
mainfrom
lm-fix-deep-extend

Conversation

@lahirumaramba

Copy link
Copy Markdown
Member

Prevent prototype pollution in deepExtend

@lahirumaramba lahirumaramba added the release:stage Stage a release candidate label Oct 14, 2026
@lahirumaramba
lahirumaramba requested a review from a team October 14, 2026 21:21

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request adds a security check to the deepExtend function in src/utils/deep-copy.ts to prevent prototype pollution by skipping __proto__, constructor, and prototype properties. The feedback suggests adding unit tests to verify that these prototype pollution vectors are successfully blocked and to prevent future regressions.

Comment thread src/utils/deep-copy.ts
@lahirumaramba

Copy link
Copy Markdown
Member Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request adds prototype pollution protection to the deepExtend utility by ignoring the __proto__, constructor, and prototype properties, along with corresponding unit tests. Feedback suggests addressing a related security and correctness risk where deepExtend recursively mutates properties inherited by the target from its prototype chain. It is recommended to check if a property is an own property of the target before extending it, and to add a unit test verifying that inherited prototype properties are not mutated.

Comment thread src/utils/deep-copy.ts Outdated
Comment thread test/unit/utils/deep-copy.spec.ts
@lahirumaramba

Copy link
Copy Markdown
Member Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces prototype pollution protection in the deepExtend utility by ignoring __proto__, constructor, and prototype properties, and adds a comprehensive suite of unit tests. The review feedback highlights an issue where using target instanceof Object fails for objects created via Object.create(null), and suggests a safer type check along with an additional unit test to cover this case.

Comment thread src/utils/deep-copy.ts Outdated
Comment thread test/unit/utils/deep-copy.spec.ts
@lahirumaramba

Copy link
Copy Markdown
Member Author

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates the deepExtend utility in src/utils/deep-copy.ts to prevent prototype pollution by skipping sensitive properties (__proto__, constructor, and prototype) and checking property existence on the target. It also adds comprehensive unit tests to verify deep copying, merging, and prototype pollution prevention. Feedback on the changes suggests that blocking the 'constructor' property is redundant and limits legitimate use cases, as prototype pollution via 'constructor' is already mitigated by other checks in the implementation.

Comment thread src/utils/deep-copy.ts

This branch has not been deployed

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

Labels

release:stage Stage a release candidate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant