Closed 80avin closed 3 months ago
@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.
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.
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.
@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.
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 requiringeval
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.
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.customEval
Instead of evalType
would negate these downsides while providing some benefits benefits.
evalType: 'safe'
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.
@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:
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: https://github.com/JSONPath-Plus/JSONPath/issues/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.
Your API proposal looks sound (I agree with the second one too) and perhaps this PR can be adjusted to implement.
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
However, the idea of allowing the library user to provide his own eval
alternative is also good. I'll update the PR with it.
I have implemented the new API eliminating preventEval
and evalType
in favour of eval
.
Please comment your suggestions.
will this PR ever be merged?
@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 All resolved.
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).
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.
If you want to take one last look, I think it should be ready for a release now.
Tried few expressions in browser demo only, and it looks good to me.
Released as v9.0.0. Thanks for your contributions!
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
eval: "safe" | "native" | boolean | (code, context) => value | Script)
ignoreEvalError: boolean
which can be used to consider the errors in eval expressions as match.Checklist
npm test
, ensuring linting passesFixes https://github.com/JSONPath-Plus/JSONPath/issues/60 , https://github.com/JSONPath-Plus/JSONPath/issues/182 , https://github.com/JSONPath-Plus/JSONPath/issues/169