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 issue with unexpected tokens (core-js in this case) #1

Closed
wants to merge 2 commits into from
Closed

Conversation

FranklinWaller
Copy link

@FranklinWaller FranklinWaller commented Dec 31, 2018

This fixes the case with core-js where the variable name has a '-' (Which is not valid in vars).

@commenthol
Copy link
Owner

Hi @FranklinWaller, could you please tell me a bit more why the "-" sign causes issues with core-js?
Just would like to understand the case.
Your change introduces a "silent ignore" preventing a "fast fail" which causes confusion to developers.

@FranklinWaller
Copy link
Author

@commenthol Core-js adds a variable to the window object called something like core-js-shared now this is valid if you do this: window['__core-js-share__'] = () => { etc.. } but not valid if you write var __core-js-share__ = () => { etc.. }. Since safer-eval creates variables it will throw an exception and fail to continue. (See https://github.com/commenthol/safer-eval/blob/master/src/browser.js#L45). IMO it's better to ignore those variables (And reassign them yourself) than not be able to execute at all.

@ElMatella
Copy link

In order to prevent the silent ignore, we could set a console.warn to warn the developer that this variable won't be evaluated.. Not removing those core-js variables makes the package unusable :/

@FranklinWaller
Copy link
Author

@commenthol Added a warning for unexpected characters. Please review

@commenthol
Copy link
Owner

I hope you are fine with favoring #4 instead of this PR.

@FranklinWaller
Copy link
Author

As long as it fixes the issue i'm happy 😉

commenthol pushed a commit that referenced this pull request Aug 5, 2020
Mitigation for arbitrary code execution / sandbox breakout
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.

None yet

3 participants