Conversation
Co-authored-by: Vijay Sarvepalli <vssarvepalli@cert.org>
src/evaluate.js
Outdated
| // function definition is included in registered functions | ||
| if (Object.values(expr.functions).includes(f)) return true; | ||
| // marked as safe already | ||
| if (f.__expr_eval_safe_def) return true; |
There was a problem hiding this comment.
This line is problematic we should remove it and move it to more reliable way. As this __expr_eval_safe_def can be user defined. It is not safe to trust it.
|
Yeh - needs a little more work. I think
See my https://github.com/sei-vsarvepalli/expr-eval-secure/tree/member-access branch that should fix all the tests and the more security problems that were found from using __expr_eval_safe_def property that can be object defined and NOT trusted. Please test, fix any README's, indentation etc. and release at your convenience. |
hello @jorenbroekema Any questions? or any feedback you need? |
|
Hi @sei-vsarvepalli, while I appreciate the efforts of this fork, I am creating an actively maintained, community-driven fork of expr-eval to address this fix and future ones. I have also been awaiting this security patch and figured to move forward with a fork of my own. 🙂 If you would like, please open a pull request addressing this change, which you have done here, or I will go ahead and make the change by the end of the day tomorrow. expr-eval-community Thank you! |
Or you could raise a PR to this branch to fix it with @sei-vsarvepalli tips, also happy to invite you as a collaborator to this one if that helps. I think more forks will just increase friction right? I just haven't found the time (unemployed, looking for a new job, christmas holidays coming up etc.) to wrap this up myself but happy to get help on it. |
|
No worries at all! I completely understand where you're coming from, and I hope my comment didn't come across the wrong way! Fewer forks would definitely make things smoother. I have seen a few forks of this package now due to the security issues that have arisen - @jorenbroekema If we could get this PR merged, that would be fantastic: jorenbroekema#3. |
Co-authored-by: Vijay Sarvepalli <vssarvepalli@cert.org>
|
@Mwpereira no worries, I know you mean well and I get that folks want to get this fixed rather sooner than later :) I had some time today to open jorenbroekema#4 which combines jorenbroekema#3 + my minor improvements + version bump and changelog entry, I had one question about a small code change that I'm not sure we still need, besides that we can merge and release it to NPM I think. I'd like to get at least you and @sei-vsarvepalli as contributors to expr-eval-fork and set up CI/CD so we can move things along faster next time with at least the 3 of us and not be blocked by my personal schedule and/or lack of time. I appreciate the idea of making it more of a community fork 😅 I'd rather not maintain it alone hehe |
3 participants
@sei-vsarvepalli
Please take a look. I tried to incorporate the fix in the code and clean it up a little but I'm left with 2 failing tests, which also fail on your branch.
Need some helping fixing those tests (or implementation). then happy to merge.