Skip to content

Replace cxjs_eval() with a Safe Implementation#116

Open
Lightning11wins wants to merge 4 commits into
masterfrom
add-eval
Open

Replace cxjs_eval() with a Safe Implementation#116
Lightning11wins wants to merge 4 commits into
masterfrom
add-eval

Conversation

@Lightning11wins

Copy link
Copy Markdown
Contributor

This PR will add a safe client-side implementation of eval, replacing the previous unsafe version.

@Lightning11wins Lightning11wins self-assigned this Jun 5, 2026
@Lightning11wins Lightning11wins added ai-review Request AI review for PRs. size: trivial Easy to review, probably ~100 lines or fewer. labels Jun 5, 2026
@greptile-apps

greptile-apps Bot commented Jun 5, 2026

Copy link
Copy Markdown

Greptile Summary

This PR replaces the unsafe cxjs_eval(x) (which called JavaScript's built-in eval() directly) with a structured, whitelist-based parser that handles boolean/null literals, integers, floats, quoted strings, and :object:property context lookups. The C-side generator is updated to prepend _context,_this, to eval() calls, matching the existing substitute() pattern.

  • JS (ht_render.js): cxjs_eval now parses a fixed set of value types and property-reference patterns; eliminates arbitrary code execution, though the 3rd and 4th user arguments supported by the server-side exp_fn_eval (current-object and parent-object overrides) are silently discarded by the 4-parameter JS signature.
  • C (exp_generator.c): One-line change adds \"eval\" to the condition that injects _context,_this, before user-supplied arguments in generated JS.

Confidence Score: 3/5

The safe-eval replacement is a meaningful security improvement, but it silently drops the object-scoping arguments that the server-side counterpart supports, which can produce wrong widget-property values with no diagnostic output.

When eval() is called with 3 or 4 arguments in the expression language, the generated JS passes all arguments to cxjs_eval, but the function only captures the first two user args — the current-object and parent-object overrides are silently discarded. Expressions that rely on that scoping will evaluate against the wrong object context on the client without any error or warning, producing silent wrong-value regressions.

centrallix-os/sys/js/ht_render.js — the new cxjs_eval function needs to handle or explicitly reject the 3rd and 4th user arguments to match exp_fn_eval's object-scoping behaviour.

Important Files Changed

Filename Overview
centrallix-os/sys/js/ht_render.js Replaces the old single-arg eval(x) with a structured safe parser; handles booleans, null, numbers, quoted strings, and :obj:prop references — but silently ignores the 3rd/4th user arguments (current/parent object overrides) that the server-side exp_fn_eval supports.
centrallix/expression/exp_generator.c Adds eval alongside substitute in the list of functions that get _context,_this, prepended to their argument list during JS code generation. One-line mechanical change that correctly enables the new client-side function signature.

Sequence Diagram

sequenceDiagram
    participant CX as Centrallix Expression eval(expr, perms, obj)
    participant GEN as exp_generator.c
    participant JS as Browser JS
    participant CXJS as cxjs_eval()
    participant WGTR as wgtrGetNode/wgtrGetProperty

    CX->>GEN: compile eval() node
    GEN->>JS: emit cxjs_eval(_context,_this, expr, perms, obj)
    Note over GEN,JS: i2 (obj) becomes 5th arg — ignored by cxjs_eval

    JS->>CXJS: call at runtime
    Note over CXJS: permflags=undefined → all access allowed

    alt literal value (bool/null/int/float/string)
        CXJS-->>JS: return parsed value
    else :property
        CXJS->>CXJS: check _this + permflags C
        CXJS-->>JS: return _this[prop] or null
    else :object:property
        CXJS->>WGTR: wgtrGetNode(_context, objname)
        WGTR-->>CXJS: node or null
        CXJS->>WGTR: wgtrGetProperty(node, propname)
        WGTR-->>CXJS: value or null
        CXJS-->>JS: return value
    else unrecognised
        CXJS-->>JS: console.warn + return null
    end
Loading
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
centrallix-os/sys/js/ht_render.js:314-375
**3rd and 4th user args silently ignored (current/parent object override)**

The server-side `exp_fn_eval` accepts up to four user arguments: `expr`, `permflags`, `currentobj`, and `parentobj`. When a Centrallix expression calls `eval(expr, "CO", "mywidget")`, the C generator emits `cxjs_eval(_context, _this, expr, "CO", "mywidget")`, but the JS function only captures four parameters. The 5th argument (`mywidget`) — which `exp_fn_eval` uses to override `objlist->CurrentID` for the scope of the inner evaluation — is silently discarded. Any expression using `eval()` with 3 or more arguments will silently use the wrong object context on the client, producing incorrect property lookups with no warning.

Reviews (2): Last reviewed commit: "Add support for booleans and null values..." | Re-trigger Greptile

Comment thread centrallix-os/sys/js/ht_render.js Outdated
Comment thread centrallix-os/sys/js/ht_render.js Outdated
Comment thread centrallix-os/sys/js/ht_render.js
@Lightning11wins

Copy link
Copy Markdown
Contributor Author

This PR is ready for human review.

@Lightning11wins Lightning11wins requested review from gbeeley and nboard June 5, 2026 20:34

@gbeeley gbeeley left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to address 3rd/4th param consistency with server per Greptile.

Comment thread centrallix-os/sys/js/ht_render.js Outdated
if (typeof x == 'object') x = x.toString();
return eval(x);

function cxjs_eval(_context, _this, expr, permflags)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with Greptile on the 3rd and 4th arguments. We should either flag the presence of those or handle them. Handling them would be ideal; that would mean:

  • :property - current object property reference would use the object named in parameter 3 if param 3 is supplied and not null (contingent of course on "C" being in the perm string or the perm string being absent)
  • ::property - parent object property reference would use the object named in parameter 4 if param 4 is supplied and not null (contingent on "P" being in the perm string or the perm string being absent)
    I could see the above being useful in certain types of applications involving data import or dynamic functionality. But writing an entire CXSQL expression parser and compiler in JavaScript isn't on the menu yet.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the issue here is that I don't really understand how eval() works in exp_functions.c. Is there something I should read to understand it better?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the specific info I'm missing is:

  • For :<property>, how should I determine what object's property do I default to?
  • For ::<property>, how should I determine who's parent is intended?

@Lightning11wins Lightning11wins Jun 8, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I've resolved this? I'm still not super confident in my understanding, but this implementation looks like it's doing the right thing.

@Lightning11wins Lightning11wins changed the title Replace cxjs_eval() with a Safe Implementation. Replace cxjs_eval() with a Safe Implementation Jun 10, 2026
@Lightning11wins Lightning11wins requested a review from gbeeley June 10, 2026 22:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review Request AI review for PRs. size: trivial Easy to review, probably ~100 lines or fewer.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants