-
Notifications
You must be signed in to change notification settings - Fork 262
Member access #291
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.
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
base: master
Are you sure you want to change the base?
Member access #291
Conversation
Co-authored-by: Vijay Sarvepalli <vssarvepalli@cert.org>
| // 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
@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.