Does not handle non-expressions correctly

[cpcallen]

eval('var x = 21; x * 2') returns 42, but safeEval('var x = 21; x * 2') crashes:

evalmachine.<anonymous>:11
  SAFE_EVAL_390371=var x = 21; x * 2
                   ^^^

SyntaxError: Unexpected token var

[simonbuchan]

vm.runInNewContext('var x = 21; x * 2') works, so I'm a little confused why safe-eval is doing the fancy stuff.

[phablulo]

hey folks, wrap it in a function

safeEval('var x = 21; x * 2')

won't work but

safeEval('(function() {var x = 21; return x * 2})()')

does work.

[cpcallen]

@phablulo: You fail to see the point: safeEval should return the same result as eval for any input (just without dangerous side-effects in the case of malicious code). Otherwise it's not really suitable as a drop-in replacement.

Also note that your wrapped code is not the same as then original version (because you've inserted a return statement).

[ChrisCinelli]

It would be an easy fix but there are 2 tests that are failing:

  it('should support context API', function () {
    var code = '{pid: process.pid, apple: a()}'
    var context = {
      process: process,
      a: function () { return 'APPLE' }
    }
    var evaluated = safeEval(code, context)
    assert(evaluated.pid > 0)
    assert(evaluated.apple === 'APPLE')
  })

  it('should parse JSON', function () {
    var code = '{name: "Borat", hobbies: ["disco dance", "sunbathing"]}'
    var evaluated = safeEval(code)
    assert(evaluated.name === 'Borat')
    assert(evaluated.hobbies[0] === 'disco dance')
    assert(evaluated.hobbies[1] === 'sunbathing')
  })

The reason is that the normal eval is also failing them:

$ node
> eval('{name: "Borat", hobbies: ["disco dance", "sunbathing"]}')
SyntaxError: Unexpected token :

>

If this npm module is supposed to be a drop-in replacement for eval then I suggest to merge a fix, bump to version 1.0.0 and remove the 2 test above. Or better, fix the first and implement the second as a negative test.

We can argue that this library is not a drop-in replacement for many reasons (support of context, trying to remove the eval side effects, etc). If we accept these facts, this is not a bug and does not need to be fixed. Maybe a note should be added on the readme.md to explain this edge case and the extra support for plain objects.

[fabiosantoscode]

@ChrisCinelli I believe that is, because it's an object, it has to be wrapped in parentheses. I think you should check if the first non-whitespace character is a {, and if so, wrap the thing in parentheses. Barring anyone acting smart and doing an orphan block (which I've literally never seen anyone else do in my life) it should work just fine.

[fabiosantoscode]

Share your code so far and I can send a PR to help you fix the other test. I've done stuff like this before.

[simonbuchan]

@fabiosantoscode, safeEval currently already returns the value, since it's injecting the prefix. @ChrisCinelli's talking about the fact that it doesn't match eval(). which does error (correctly) in this case, as it takes statements, not expressions. The question he was asking is if safeEval() should match eval() and error, in which case the tests are wrong, and my question is why safeEval() bothers injecting anything, as the underlying API allows you to get the expected value anyway.