Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Replacing eval() calls in syscall/js #1162

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

Merged
merged 1 commit into from
Oct 17, 2022

Conversation

matthewbelisle-wf
Copy link
Contributor

@matthewbelisle-wf matthewbelisle-wf commented Oct 17, 2022

Minor tweak to syscall/js to follow a security best practice as advised by MDN:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/eval#never_use_eval!

Additionally, modern JavaScript interpreters convert JavaScript to machine code. This means that any concept of variable naming gets obliterated. Thus, any use of eval() will force the browser to do long expensive variable name lookups to figure out where the variable exists in the machine code and set its value. Additionally, new things can be introduced to that variable through eval() such as changing the type of that variable, forcing the browser to re-evaluate all of the generated machine code to compensate.

Fortunately, there's a very good alternative to eval(): using the Function constructor. Bad code with eval():

In addition to the security benefits, this means that gopherjs can now be used in environments with a CSP that doesn't allow scritp-src: 'unsafe-eval'. (see comment below)

@nevkontakte nevkontakte enabled auto-merge October 17, 2022 19:42
@nevkontakte
Copy link
Member

Thank you for the contribution!

@nevkontakte nevkontakte merged commit 044c47a into gopherjs:master Oct 17, 2022
getValueType = js.Global.Call("eval", `(function(x) {
id = js.Global.Call("Function", "x", "return x")
instanceOf = js.Global.Call("Function", "x", "y", "return x instanceof y")
getValueType = js.Global.Call("Function", "x", `
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hajimehoshi I'm wondering if you have any thoughts on this one? It seems to me like this should be okay but would definitely like a second opinion. The Function constructor is the preferred alternative to eval() apparently.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/eval#never_use_eval!

Fortunately, there's a very good alternative to eval(): using the Function constructor.

何卒よろしくお願い申し上げます🙇‍♂️

@matthewbelisle-wf
Copy link
Contributor Author

You bet @nevkontakte thanks for the quick merge!

@matthewbelisle-wf
Copy link
Contributor Author

For posterity, even though this did make the JS follow MDN best practices, it still cannot run in a CSP context without script-src 'unsafe-eval' because both eval() and Function() require the unsafe-eval directive.

@nevkontakte
Copy link
Member

That makes sense, since Function() does still evaluate an arbitrary string as code. If you want to make this work with unsafe-eval, you could introduce a $typeof() helper function in prelude and call it from the syscall/js package, instead of defining it dinamically.

@matthewbelisle-wf
Copy link
Contributor Author

Thanks for the info @nevkontakte , is this what you're describing?

#1168

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
Morty Proxy This is a proxified and sanitized view of the page, visit original site.