add safe eval for browser and eval option

[80avin]

Adding a safe alternative to Function/eval in browser.
This is also required since the chrome extensions & chrome apps have already become strict about safety.

I was trying to use it in a chrome extension but couldn't because of unsafe-eval. Hence this fix tries to introduce a safe evaluation method which satisfies Content Security Policy.

eval: "safe" is effective only in browser and doesn't uses a scripting engine which supports a minimal form of javascript (no functions or complex expressions).

PR description

• Add safe version of eval/Function for Content Security Policy
• Update test/index.html to use compiled module
• update demo html to include prefilled sample & prevent reload on submit
• babel/rollup related changes
• Add new option eval: "safe" | "native" | boolean | (code, context) => value | Script)
• Add new option ignoreEvalError: boolean which can be used to consider the errors in eval expressions as match.

Checklist

• - Added tests
• - Ran npm test, ensuring linting passes
• - Adjust README documentation if relevant

Fixes #60 , #182 , #169

[80avin]

@brettz9 Could you please check and see if the approach is right ?

I particularly need advice on the rollup/babel/eslint configuration changes as I'm totally new to them.

Also, if possible, how should we go with the support of "use strict" and funciton syntax in scripts ?

If I get a green flag in these, I'll work on documentation.

[brettz9]

I don't have time to look at this now, and no guarantees about getting to it, but I think an optimal solution, if you can't get the multi-line evaluation supported, would be to allow the user to supply their own evaluator, e.g., optionally passing in eval(), so that users could still maintain support without too much added effort yet without requiring eval statements which create conflicts with content service policies such as Chrome extensions apparently enforce.

[80avin]

user still has the choice to go with unsafe way using a new options evalType which can have values none or native or safe.
Thanks for the quick reply though. I'll move ahead with the cleanup & documentation. I should also be able to get multiline support.

[80avin]

@brettz9 Looks like the PR is complete.
I added the tests, types, updated Readme and all tests are passing now.

I'm in no rush, so whenever you (or some other contributor) find time, please look into it and suggest changes.

[brettz9]

I plan to give a deeper review later, but wanted to first address these smaller questions, and also ask that we maintain 100% testing coverage. (I see that the coverage badge is no longer reporting 100%, so some of the new lines of code apparently aren't covered in tests.)

I'd prefer we avoid the Babel ESLint parser unless there is really a compelling reason, though it'd be fine to increase the parserOptions.ecmaVersion to use the latest standard features in source.

From what I can see, the rest all looks very good.

[jacobroschen]

I don't have time to look at this now, and no guarantees about getting to it, but I think an optimal solution, if you can't get the multi-line evaluation supported, would be to allow the user to supply their own evaluator, e.g., optionally passing in eval(), so that users could still maintain support without too much added effort yet without requiring eval statements which create conflicts with content service policies such as Chrome extensions apparently enforce.

@brettz9, my two unsolicited cents are in agreement with your thoughts about this PR for a few couple reasons.

1. This adds dependencies for every consumer, even if they don't use the "safe" eval option.
2. While this PR passes all tests, jsep does not support all functionality that eval() supports. This not only results in a breaking change, but will result in a third set of behavior that this library needs to support (default eval allow, preventEval: true, and now whatever jsep supports.
3. Adding a new config parameter such as customEval Instead of evalType would negate these downsides while providing some benefits benefits.
   1. Prevents the burden of a third mode for the maintenance of this library
   2. Provides greater flexibility for consumers (ie. a more limited eval function, or they want to use something like QuickJS for a safer, but just as powerful eval function)
   3. Removes the extra dependency for consumers that don't use the evalType: 'safe'
   4. Removes the weird ability to disable eval with two different config options evalType and preventEval
      If we'd like to go down this path, there are two different config option styles that seem reasonable.

const customEval = (code: string, context: any): any => {
    // evaluate code against context, return any matching result(s)
};
const jsonPath = new JSONPath({ eval: customEval });
jsonPath.evaulate('$.test');

An alternative API that fits into the current architecture a bit better, which would require the consumer to pass in a class to the config.

class CustomEval {
  constructor(code: string) {
    // any compiling you need to do.
    this.code = code;
  }
  
  eval(context: any) {
    // however you'd like to evaluate the code (`this.code`) against `context`
    // return undefined, or match(es)
    return undefined;
  }
}

const jsonPath = new JSONPath({ eval: CustomEval });
jsonPath.evaluate('$.test');

In both scenarios, this library would still replace the shorthand properties(ex @parentProperty, @property, etc.) into _$_parentProperty, _$_property, etc while providing these values as context properties.

While option 2 has a bit more boilerplate, I think it provides more flexibility for consumers of this library and it enables automatic caching of the compiled script.

[brettz9]

@jacobroschen : Thank you for your thoughts. They are most welcome. However, I have a few thoughts in response:

As far as adding dependencies, the bundle size is rather good, no less considering what it is doing:

• 3.4 KB minified and gzipped: https://bundlephobia.com/package/[email protected]
• 661 B minfiied and gzipped: https://bundlephobia.com/package/@jsep-plugin/[email protected]
• 504 B minfiied and gzipped: https://bundlephobia.com/package/@jsep-plugin/[email protected]

And although it does not support everything eval-wise, for the much more common use case of just accessing properties, it really does deliver something important.

I agree we can drop preventEval (as part of a breaking change), but that can be done as part of this PR as well. While this technically removes some functionality, I don't know how much this is going to harm anyone since it should not compromise security nor would I expect performance to significantly suffer by allowing what is already normally allowed in a jsonpath engine.

It has been a long-time goal for us to have a safe default, and recently we had still another request for a safe default: #182 . I rejected that because I didn't want to gut the core functionality our users have become accustomed to, but only because we didn't have an alternative.

I remain open to further thoughts, as well as to input of others, but unless there are concerns with jsep specifically, it seems like a hearty compromise without coming at much cost.

[brettz9]

Your API proposal looks sound (I agree with the second one too) and perhaps this PR can be adjusted to implement.

[80avin]

I agree with @brettz9 . The primary reason why I decided to use jsep in this library was to provide a safe & lightweight evaluation out of the box. Another similar library jsonpath achieves this by using static-eval and esprima which have minified + gzip sizes as

• 19.4kB https://bundlephobia.com/package/[email protected]
• 29kB https://bundlephobia.com/package/[email protected]

However, the idea of allowing the library user to provide his own eval alternative is also good. I'll update the PR with it.

[80avin]

I have implemented the new API eliminating preventEval and evalType in favour of eval.
Please comment your suggestions.

[guoliang]

will this PR ever be merged?

[80avin]

@guoliang Thanks for reminding

@brettz9 I have removed the conflicts which had appeared during the time.
To me, the PR looks complete. Can you review on your part also ?

Also, since your last review, I added a new option ignoreEvalError which can be used to suppress the errors in the eval expressions.
The usecase is that if our expression passes only on certain types but gives errors on other types ( .toLowerCase() will error on anything other than string ), in that case we would want to consider the error as no match instead of crash.

[brettz9]

If you can make the mentioned changes, I think I'll be ready for a final review. Looks good so far...

[80avin]

@brettz9 All resolved.

[brettz9]

I added one commit to fully pass linting and the testing scripts.

Did you mean for the ignoreEvalError option to be ignoreEvalErrors instead?

Also, please prepare a message for those upgrading to this breaking change version (e.g., what users of preventEval should now do instead).

[80avin]

ignorEvalErrors sounds good as it'll be ignoring many errors, one for each path.

I hope this message suffices.

- Breaking change: remove `preventEval` property. Prefer `eval: false` instead.
- Breaking change: changed behaviour of `eval` property. In the browser, `eval`/`Function` won't be used by default to evaluate expressions. Instead, we'll safely evaluate using a subset of JavaScript. 
- feat: add `eval` property.
- feat: add `ignoreEvalErrors` property.

[brettz9]

If you want to take one last look, I think it should be ready for a release now.

[80avin]

Tried few expressions in browser demo only, and it looks good to me.

[brettz9]

Released as v9.0.0. Thanks for your contributions!