Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(merge): Avoid protototype pollution when parsing properties #8675

Merged
merged 1 commit into from
Oct 29, 2020

Conversation

benoitf
Copy link
Contributor

@benoitf benoitf commented Oct 28, 2020

What it does

Avoid prototype pollution

How to test

Tests should pass
But you can try using configuration of plug-ins on workspace level and user level and see that merge correctly happen

Review checklist

Reminder for reviewers

@benoitf benoitf force-pushed the prototype-pollution branch from c34a31e to 1ed8fb6 Compare October 28, 2020 10:04
@benoitf benoitf added the plug-in system issues related to the plug-in system label Oct 28, 2020
@benoitf benoitf force-pushed the prototype-pollution branch from 1ed8fb6 to 593a7a9 Compare October 28, 2020 17:22
Copy link
Member

@paul-marechal paul-marechal left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the fix!

@paul-marechal paul-marechal force-pushed the prototype-pollution branch 2 times, most recently from 122fafd to 8cccac3 Compare October 28, 2020 18:15
@max-schaefer
Copy link

Does this also guard against prototype pollution via .constructor.prototype, or is that not possible in this context? (Note that {}.constructor.prototype === Object.prototype.)

@benoitf
Copy link
Contributor Author

benoitf commented Oct 29, 2020

I updated test case with constructor (it seems it's only affecting parseConfigurationData method)
@marechal-p if you can review again (I pushed a new commit)

@paul-marechal
Copy link
Member

paul-marechal commented Oct 29, 2020

There was an unescaped . in the regexp. I pushed the fix for this and also used Object.create(null) where it made sense. I don't see anything else, feel free to squash and merge.

Change-Id: I30ac10c9afce8a6fe01e197e18071e33f0e0bda7
Signed-off-by: Florent Benoit <[email protected]>
@benoitf benoitf force-pushed the prototype-pollution branch from 525f4d9 to 986c222 Compare October 29, 2020 16:39
@benoitf
Copy link
Contributor Author

benoitf commented Oct 29, 2020

@marechal-p would be nice to have it in the upcoming release

@paul-marechal paul-marechal merged commit a781485 into master Oct 29, 2020
@paul-marechal paul-marechal deleted the prototype-pollution branch October 29, 2020 18:26
@github-actions github-actions bot added this to the 1.7.0 milestone Oct 29, 2020
@paul-marechal
Copy link
Member

@benoitf it will since it looks like a complete fix now :)

@paul-marechal
Copy link
Member

paul-marechal commented Oct 29, 2020

@max-schaefer please tell us if you still see something, we can do a patch release in the worst case.

We'll now proceed with the 1.7.0 release.

@max-schaefer
Copy link

LGTM

it seems it's only affecting parseConfigurationData method

Yes, that sounds plausible. The other method has an isObject check, which will prevent the .constructor.prototype vector, since .constructor is a function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plug-in system issues related to the plug-in system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants