ramu11 commented on PR #25778: URL: https://github.com/apache/camel/pull/25778#issuecomment-5437337123
> Well-implemented new QuickJS language component with solid security model (JSON-only data bindings, no live Java objects), comprehensive tests (37 tests covering security, concurrency, lifecycle, serialization), thorough documentation, and correct DSL integration. > > One defense-in-depth security suggestion: > > **`globalThis.java_invoke` / `globalThis.quickjs4j_engine` bypass** > > The `EVAL_WRAPPER` correctly shadows `java_invoke` and `quickjs4j_engine` as `const` in the function's local scope. However, user scripts can bypass the shadow via `globalThis.java_invoke(...)` or `globalThis.quickjs4j_engine` since the global properties are not deleted. > > The practical risk is **limited** — `java_invoke` only dispatches to pre-registered builtins (the `camelEval` invokable and the `quickjs4j_engine` metadata module), not arbitrary Java methods. But the security tests only verify the bare identifier `java_invoke(...)` is blocked, not the `globalThis` path. > > Consider adding to the wrapper before the eval: > > ```js > delete globalThis.java_invoke; > delete globalThis.quickjs4j_engine; > ``` > > This would complete the sandboxing and prevent any future quickjs4j changes from exposing additional capabilities through the host bridge. > > ### 📋 PR Metadata > Aspect Current Suggested > Milestone _(none)_ `4.23.0` > _This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying._ > > _Claude Code on behalf of Guillaume Nodet_ Thanks for catching this. I investigated the `globalThis` path in more detail, including the QuickJS4J engine lifecycle. You are correct that the current lexical shadowing does not prevent access through `globalThis`, so the existing security test does not cover the complete exposure path. There is one important lifecycle detail with the suggested `delete` approach, though. The QuickJS4J engine is reused, and `java_invoke` is also required internally by the generated invocation wrapper after `camelEval` returns to deliver the result. Removing the global property would therefore break the current evaluation/result handling and leave the reused engine without `java_invoke` for subsequent evaluations. I think the appropriate fix is to temporarily shadow/replace the exposed host globals during execution of the user script and restore them before returning from `camelEval`, rather than deleting them. I will also extend the security tests to cover the `globalThis`/bracket-access path and verify that the same engine continues to work for subsequent evaluations. I’ll address this as a small defense-in-depth fix in the current PR since it aligns with the existing security model and documentation, without changing the intended Option A design. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
