Replace cxjs_eval() with a Safe Implementation - #116
Conversation
…s unsafe version).
Lightning11wins
added
ai-review
Greptile SummaryThe PR replaces client-side expression evaluation with a restricted evaluator and updates JavaScript generation to provide the widget context required for scoped property resolution.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| centrallix-os/sys/js/ht_render.js | Adds the restricted cxjs_eval implementation and resolves the previously reported implicit-global, debug-log, and literal-handling issues. |
| centrallix/expression/exp_generator.c | Supplies the established client expression context arguments when generating calls to the eval helper. |
Reviews (4): Last reviewed commit: "Remove left-over debug log." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
This PR is ready for human review. |
gbeeley
left a comment
gbeeley
left a comment
There was a problem hiding this comment.
Need to address 3rd/4th param consistency with server per Greptile.
Lightning11wins
changed the title
cxjs_eval() with a Safe Implementation.cxjs_eval() with a Safe Implementation
gbeeley
left a comment
gbeeley
left a comment
There was a problem hiding this comment.
A couple of changes needed - thanks!
| const prop = rest.substring(1); | ||
| const par_node = (par_obj_name !== null && par_obj_name !== undefined) | ||
| ? wgtrGetNode(_context, par_obj_name) | ||
| : wgtrGetParent(_context); |
There was a problem hiding this comment.
This defaults to the parent of the context, rather than the parent of the current object (_this).
In server contexts, the parent object has a context-dependent meaning but usually refers to the immediate parent object of the current object. The most common use is in a QueryTree object.
On the client, the most useful meaning here would be to reference the parent of _this rather than the parent of _context which likely steps out of the current scope entirely.
There was a problem hiding this comment.
Ahhh, so like this?
const par_node = (par_obj_name !== null && par_obj_name !== undefined)
? wgtrGetNode(_context, par_obj_name)
: wgtrGetParent(_this);This issue likely arises from me only having a surface-level understanding of what eval(), _context, and _this should do.
There was a problem hiding this comment.
Yes, that looks correct - please test out and verify. Thanks!
There was a problem hiding this comment.
I'll try to test this, although I only barely understand what this function is supposed to do in this case.
|
@gbeeley That should resolve your requested changes. |
gbeeley
left a comment
gbeeley
left a comment
There was a problem hiding this comment.
Oops - one of my last recommendations was partly wrong.
| } | ||
| else if (_this !== null && _this !== undefined) | ||
| { | ||
| return wgtrGetProperty(_this, rest); |
There was a problem hiding this comment.
I take that back. There are cases where _this is a plain object rather than a widget. So this code should check first whether it is a widget via wgtrIsNode() before calling wgtrGetProperty(), otherwise use the _this[prop] approach.
An example of this is parameters passed from an event (the event params) which can be referenced as the current object as in :property.
There was a problem hiding this comment.
Hmmm, I think I understand. Should be fixed now.
|
@gbeeley I think I've fixed those issues. |
|
@greptileai please recheck - thanks! |
|
|
||
| function cxjs_eval(_context, _this, expr, permflags, cur_obj_name, par_obj_name) | ||
| { | ||
| console.log(_context, _this, expr, permflags, cur_obj_name, par_obj_name); |
There was a problem hiding this comment.
Per greptile, let's remove this debugging line before merge.
There was a problem hiding this comment.
Woops! Fixed.
2 participants
This PR will add a safe client-side implementation of eval, replacing the previous unsafe version.