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.

Sign up for GitHub

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

Jump to bottom

Use vm.runInThisContext instead of eval? #17

Closed
kevinsawicki opened this issue Jan 10, 2015 · 7 comments · Fixed by #18
Closed

Use vm.runInThisContext instead of eval? #17

kevinsawicki opened this issue Jan 10, 2015 · 7 comments · Fixed by #18

Comments

@kevinsawicki
Copy link

Just wondering why eval is used over vm.runInThisContext which is what CoffeeScript uses.
@jkrems
Copy link
Contributor

jkrems commented Jan 10, 2015

Thanks! Yes, I felt bad committing that code. I think eval is already overkill. It should be possible to just parse the string according to JavaScript semantics. I'm not sure how much overhead vm.runInThisContext has, if it isn't completely crazy this might be worth it.

@kevinsawicki
Copy link
Author

I think it is a little safer than eval according to http://nodejs.org/api/vm.html#vm_vm_runinthiscontext_code_filename since it can't touch local variables.

eval is going to be a problem in Atom Shell apps that have a content security policy set on the page which prevents eval (among other things).

@jkrems
Copy link
Contributor

jkrems commented Jan 10, 2015

Good point with the CSP, didn't consider that. I was fine with eval from a security perspective in this case because the coffee-script parser should guarantee those fields to be literals (e.g. no code involved). I would love for this code to run client-side but it looks like vm is actually supported, at least when using browserify. Will look into this.

jkrems pushed a commit that referenced this issue Jan 11, 2015
Use vm.runInThisContext instead of eval
9d683f9 
Fixes #17

While this gets rid of `eval` in node or in "mixed" environments
like node-webkit or Atom Shell apps, it still resolves to eval
when using browserify.

For a solution that is compatible with CSP policies we'd need
actual parsing of the literals (wish coffee-script would do it).

See:
* https://github.com/substack/vm-browserify/blob/bfd7c5f59edec856dc7efe0b77a4f6b2fa20f226/index.js#L105
@jkrems jkrems mentioned this issue Jan 11, 2015
Merged
@jkrems
Copy link
Contributor

jkrems commented Jan 12, 2015

v1.0.4

@kevinsawicki
Copy link
Author

Has 1.0.4 been published yet to npmjs.org?

@jkrems
Copy link
Contributor

jkrems commented Jan 12, 2015

Oops -> #19

Published to public registry.

@kevinsawicki
Copy link
Author

Awesome, thanks so much for this fix!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Use vm.runInThisContext instead of eval groupon/cson-parser
2 participants
@jkrems @kevinsawicki