mozilla / rhino

Rhino is an open-source implementation of JavaScript written entirely in Java
https://rhino.github.io
Other
4.18k stars 850 forks source link

Support importPackage, print, load when using Rhino through the ScriptEngine interface #1043

Open e-masaki opened 3 years ago

e-masaki commented 3 years ago

Add two JARs in classpath.

I get the following error when evaluating "importPackage" function using Java "ScriptEngine" interface:

javax.script.ScriptException:` ReferenceError: "importPackage" is not defined. in eval at line number 0 at column number 0
    at org.mozilla.javascript.engine.RhinoScriptEngine.eval(RhinoScriptEngine.java:123)
    at javax.script.AbstractScriptEngine.eval(AbstractScriptEngine.java:264)

Java code:

ScriptEngineManager manager = new ScriptEngineManager();
ScriptEngine engine = manager.getEngineByName("rhino");
try {
    engine.eval("importPackage(Packages.test.js)");
} catch (ScriptException e) {
    e.printStackTrace();
}
p-bakker commented 3 years ago

This is expected imho: importPackage (and importClass) aren't included by default so it seems

tonygermano commented 3 years ago

Was importPackage included by default in the version of Rhino embedded in java 6/7? If so, we probably want feature parity. That being said, I never use that method, and there are plenty of other ways to "import" java classes.

e-masaki commented 3 years ago

importPackage are enabled by default in java 6/7. When I use JSR-223 script engine(java.net) along with rhino, I can use it by default in java 8 or later.

Due to the specifications of the my tool, other way cannot be used.

I can do that by using Rhino without going through the ScriptEngine interface. I also want to use print. as follows:

Context context = Context.enter();
try {
    Global global = new Global(context);
    ImporterTopLevel.init(context, global, false);
    context.evaluateString(global, "importPackage(Packages.test.js)", "eval", 0, null);
    context.evaluateString(global, "print('hoge');", "eval", 0, null);            
} finally {
    Context.exit();
}

Next, I tried to share the scope and context with ScriptEngine instance, but couldn't.

p-bakker commented 3 years ago

@gbrail as you authored the ScriptEngine impl. in 1.7.13, what do you think about this issue?

Should importPachage (and importClass) be enabled (by default) when using Rhino through the ScriptEngine? Anything else maybe was enabled when Rhino was shipped with Java and we might want to add/enable (by default) when using rhino-engine?

e-masaki commented 3 years ago

Sure, it's good to enable importPackage by default, but if we can enable it with standard Java or ScriptEngine features, you don't have to. For example, In nashorn, importPackage are supported if we load the compatibility script. ex. load("nashorn:mozilla_compat.js") In addition, Use --global-per-engine option, We can use a single instance of the global object. ex. System.setProperty("nashorn.args", "--global-per-engine")

Also, I want to enable print and load when using rhino-engine.

gbrail commented 3 years ago

I don't think any of those things should be enabled by default. A lot of applications embed Rhino into places in which an arbitrary script should not be able to load any Java package or read any file from the filesystem, which is what "importPackage" and "load" do. Having that enabled by default in every ScriptEngine instance is very dangerous. Even "print" should be optional IMO because it's also not a standard part of JavaScript and depending on how Rhino is being used it could also produce unexpected results.

I think that it makes sense to add options to ScriptEngine so that it adds importPackage, load, and print, but definitely not by default.

On Wed, Oct 13, 2021 at 5:06 PM e-masaki @.***> wrote:

Sure, it's good to enable importPackage by default, but if we can enable it with standard Java or ScriptEngine features, you don't have to. For example, In nashorn, importPackage are supported if we load the compatibility script. ex. load("nashorn:mozilla_compat.js") In addition, Use --global-per-engine option, We can use a single instance of the global object. ex. System.setProperty("nashorn.args", "--global-per-engine")

Also, I want to enable print and load when using rhino-engine.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mozilla/rhino/issues/1043#issuecomment-942810387, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAD7I24NYIU3IKSGWSTS73DUGYNHVANCNFSM5FNNPC4Q . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

p-bakker commented 3 years ago

I don't think any of those things should be enabled by default

Yeah, agree, was just wondering for the pov of backwards compatibility.

Also, I want to enable print

Based on looking at the code, I think print is already available

but if we can enable it with standard Java or ScriptEngine features

Not being an expert in the ScriptEngine interface: are bindings the standard way to configure a Script Engine? Looks like we currently use that allow setting the language version (javax.script.language_version) and optimization level (org.mozilla.javascript.optimization_level).

Using that, we could relative easily enable all Java Interop features like importClass/importPackage/JavaAdapter/Packages./org./java.*/etc

.load(...) however is a different matter, as .load() is not something that is in the code of Rhino, its only part of the interactive Shell. But I guess similar functionality should be (optionally) made available in the ScriptEngine. Not sure what you want to load though, maybe it makes more sense to offer the option to enable CommonJS modules?

Feel like forking the Rhino repo and implementing this yourself? Can provide pointers where to look/get started

e-masaki commented 3 years ago

Based on looking at the code, I think print is already available

Sorry, print cannot be used in cases where Rhino is used directly. This was solved by using the Global scope.

Not being an expert in the ScriptEngine interface: are bindings the standard way to configure a Script Engine? Looks like we currently use that allow setting the language version (javax.script.language_version) and optimization level (org.mozilla.javascript.optimization_level).

Yes, internally, I think it's common to set it in the Context.

To do this through the ScriptEngine interface, I wanted to do something like System.setProperty(option) or ScriptEngine.eval(load compatibility script).

p-bakker commented 3 years ago

to do something like System.setProperty(option) or ScriptEngine.eval(load compatibility script).

Why do you want that vs. doing things through the bindings, like is already implemented for the optLevel and languageVersion?

Wondering if exposing this through System.setProperty(...) isn't a security issue, as a user might enable more that the embedder intended.

With the ScriptEngine.eval(someScriptName) approach, the challenge is how to offer finegrained control over what get enabled of not. And, if you're already calling Java code like .evel, why is setting the bindings not a valid option?

e-masaki commented 3 years ago

Since the current program uses Nashorn, I simply adapted it to Nashorn's specifications. I also thought it would be possible to have it enabled from outside the library.

There are other ways to enable it from outside, and I'm not particularly attached to my idea. As you said, there are security issues, so I agree with the idea you suggested.

p-bakker commented 2 years ago

Feel like forking the Rhino repo and implementing this yourself? Can provide pointers where to look/get started

@e-masaki

e-masaki commented 2 years ago

Feel like forking the Rhino repo and implementing this yourself? Can provide pointers where to look/get started

I have since investigated this, but originally the load function was not available in Java 7. So, I think we should be able to use importPackage/importClass.

As far as I investigated, using the ImporterTopLevel.init method on an existing scope did not make the importPackage available.

In the initScope(Context, ScriptContext) method of the RhinoScriptEngine class, we can use ImporterTopLevel(Context) to generate a topLevelScope instead of cx.initStandardObjects() only when the option is specified. I think this is the simplest implementation.

e-masaki commented 2 years ago

I tried the following with the initScope(Context cx, ScriptContext sc) method of the RhinoScriptEngine class.

First of all, I tried to add importPackage to engineScope by running ImporterTopLevel.init(Context cx, Scriptable scope, boolean sealed).

Scriptable engineScope = new BindingsObject(
    sc.getBindings(ScriptContext.ENGINE_SCOPE));
engineScope.setParentScope(null);
engineScope.setPrototype(topLevelScope);
ImporterTopLevel.init(cx, engineScope, false); // add this

However, the importPackage could not be used. I tried the same thing with topLevelScope, but no luck.

if (topLevelScope == null) {
    topLevelScope = cx.initStandardObjects();
    ImporterTopLevel.init(cx, topLevelScope, false); // add this

Next, I changed the topLevelScope to be created using the ImporterTopLevel(Context cx) constructor, but no luck.

if (topLevelScope == null) {
    //topLevelScope = cx.initStandardObjects(); // commenting out
    topLevelScope = new ImporterTopLevel(cx); // add this

Next, I set the scope generated by using the ImporterTopLevel(Context cx) constructor as the parent of engineScope.

if (topLevelScope == null) {
    topLevelScope = cx.initStandardObjects();
    hoge = new ImporterTopLevel(cx); // add this
    ...
}

Scriptable engineScope = new BindingsObject(
    sc.getBindings(ScriptContext.ENGINE_SCOPE));
//engineScope.setParentScope(null); // commenting out
engineScope.setParentScope(hoge); // add this

I gave up because it adversely affected the global scope object shared with another context of the same engine. I used Global(Context cx) instead of ImporterTopLevel(Context cx) constructor and the result was the same.

Is there any other way?

p-bakker commented 2 years ago

I must say I'd expected the things you tried to work as well, but looking into how those things normally get initialized, it looks like it's done slightly different, see https://github.com/mozilla/rhino/blob/master/src/org/mozilla/javascript/ScriptRuntime.java#L298

Maybe you can give that a spin?

dazoulay-simplicite commented 2 years ago

Hello, as discussed in https://github.com/mozilla/rhino/issues/657 (thanks @p-bakker for linking me here), I am looking for a pure JSR-223-level solution to make importPackage and importClass available to the Rhino scripts

@gbrail said (I guess from the date it was about version 1.7.13):

I think that it makes sense to add options to ScriptEngine so that it adds importPackage (...)

Is a new option now exists in version 1.7.14 to do so ?

And - sorry for the dumb question - how do I programatically set this option ? I saw put methods at several levels (ScriptEngineManager, ScriptEngine, Bindings)...

Thank you for your help/advice.

dazoulay-simplicite commented 2 years ago

Hello,

I am still unable to enable using only the JSR-223 API = only javax.script.* classes. I am using the standard org.mozilla.rhino-engine (version 1.7.14) JSR-223 wrapper and the corresponding org.mozilla.rhino (also version 1.7.14)

All posts I find somehow related to this topic contains code using non JSR-223 org.mozilla.javascript.* classes which I can't do.

I suppose I miss something obvious but I can't figure out what...

Example:

import javax.script.*;

public class Test {
    public static void main(String[] args) {
        try {
            ScriptEngine engine = new ScriptEngineManager().getEngineByName("rhino");
            ScriptContext ctx = new SimpleScriptContext();

            Bindings bindings = ctx.getBindings(ScriptContext.ENGINE_SCOPE);
            bindings.put("who", "David");

            // **Success**
            System.out.println(engine.eval(
                "(function() { return 'Hello ' + who; })();",
                bindings));

            // **Failure** => javax.script.ScriptException: ReferenceError: "importPackage" is not defined
            System.out.println(engine.eval(
                "importPackage(Packages.org.json);\n" +
                "(function() { return new JSONObject().put('hello', who).toString(); })();",
                bindings));
        } catch (Exception e) {
            e.printStackTrace();
        }
    }
}

Note: I don't have this issue if I use a legacy JSR-223 wrapper = cat.inspiracio.rhino-js-engine (version 1.7.10) with a previous Rhino version (up to 1.7.13 because this legacy wrapper stopped working with version 1.7.14)

Thank you for your help.

shapirobh commented 1 year ago

I am running into the same problem as @dazoulay-simplicite .

Any information on how to get around this?

sepili commented 11 months ago

@e-masaki @gbrail @p-bakker I have gone through above discussions but not able to conclude how to resolve importPackage and print statements issues. We have old js files which has importPackage and print statements which were working fine with rhino 1.7.13 ScriptEngine but failing with 1.7.14. javax.script.ScriptException: ReferenceError: "importPackage" is not defined. (eval#1) in eval at line number 1 at column number 0

So my doubt is whether we should not use these statements at all or any workaround for backward compatibility? any sample code for ScriptEngine based use from outside.

Thanks.

tonygermano commented 11 months ago

I ended up going down the same path as @e-masaki while looking into this, and ultimately came to change line 83 here: https://github.com/mozilla/rhino/blob/ef18cb92067d2ff2e14a038d29e2d4adce0de446/src/org/mozilla/javascript/engine/RhinoScriptEngine.java#L79-L102 to

topLevelScope = new ImporterTopLevel(cx);

This does appear to work. However, the downside is that any classes imported this way also get added to the Bindings object that is shared with Java.

I think the issues people are having with print are due to the bug described here https://github.com/mozilla/rhino/issues/1356. I'm running from the latest master branch and print works for me.

One thing that does seem to work without any modification to Rhino code is wrapping the whole script in a with like this (I ran the test in java 17 for the text block support):

import javax.script.*;

public class Test {
    public static void main(String[] args) {
        try {
            ScriptEngine engine = new ScriptEngineManager().getEngineByName("rhino");
            ScriptContext ctx = new SimpleScriptContext();

            Bindings bindings = ctx.getBindings(ScriptContext.ENGINE_SCOPE);
            bindings.put("who", "David");

            // **Success**
            System.out.println(engine.eval(
                "(function() { return 'Hello ' + who; })();",
                bindings));

            System.out.println(engine.eval(
                """
                with(new JavaImporter()) {
                    importPackage(java.util)

                    // function declaration gets clobbered by the importer, but still adds to the bindings
                    function TreeMap() {}

                    // var declaration takes precedence over the importer
                    var ArrayList = function ArrayList() {}

                    var treeMap = new TreeMap().toString()
                    var arrayList = new ArrayList().toString()

                    // uses HashMap from the importer, but does NOT add to the bindings
                    var hashMap = new HashMap({hello: who}).toString()
                    JSON.stringify({
                        treeMap: treeMap,
                        arrayList: arrayList,
                        hashMap: hashMap}, null, 2)
                }
                """,
                bindings));
            System.out.println(bindings.keySet().toString());
        } catch (Exception e) {
            e.printStackTrace();
        }
    }
}

This is the output:

Hello David
{
  "treeMap": "{}",
  "arrayList": "[object Object]",
  "hashMap": "{hello=David}"
}
[treeMap, ArrayList, arrayList, TreeMap, hashMap, who]
EduFrazao commented 8 months ago

Hi. I'm facing the same issue. I was using version 1.7.13 with cat.inspiracio.rhino-js-engine version 1.7.10, and things were running fine, until I get an error that was fixed in version 1.7.14 (#247)

I use JSR223 and I can't manage a way to make this work. I already have a "general" compatibility script that is loaded before any script here.

   /**
  * Println used to work with Java7 Rhino.
  */
if (typeof println == 'undefined') this.println = print;

/*
 * Java importer
 */
if (typeof importClass == 'undefined' || typeof importPackage == 'undefined') {
    var __importer = new JavaImporter();
    this.importClass = __importer.importClass;
    this.importPackage = __importer.importPackage;
}

Now my scripts can import JavaClasses (and those classes are visibible on ScriptBindings), but when I try to instantiate them, an error.

This is an example of script that fails

importClass(Packages.org.vaadin.aceeditor.AceEditor);
    importClass(Packages.org.vaadin.aceeditor.AceMode);
    importClass(Packages.org.vaadin.aceeditor.AceTheme);
    function createComponent(id) {
        var scriptEditor = new AceEditor();
        scriptEditor.setWidth("100%");
        scriptEditor.setHeight("100%");
        scriptEditor.setMode(AceMode.javascript);
        scriptEditor.setTheme(AceTheme.eclipse);
        return scriptEditor;
    }

I use getInterface feature, to get a JavaProxy and call "createComponent" method later. When I call it, the code fails on

var scriptEditor = new AceEditor();

With this error:

Caused by: javax.script.ScriptException: Invalid JavaScript value of type java.lang.Class (eval#6) in eval at line number 6 at column number 0

The error is thrown on this snipet: https://github.com/mozilla/rhino/blob/7d2c2425ce7ed2025a4d3d9042159379056e00ff/src/org/mozilla/javascript/Interpreter.java#L1912-L1916

EduFrazao commented 8 months ago

Hi. I managed to get it working, but I need to change two files:

First, org.mozilla.javascript.engine.BindingsObject

Changing:

@Override
    public Object get(String name, Scriptable start) {
        if (!bindings.containsKey(name)) {
            return Scriptable.NOT_FOUND;
        }
        return Context.jsToJava(bindings.get(name), Object.class);
    }

To:

@Override
    public Object get(String name, Scriptable start) {
        if (!bindings.containsKey(name)) {
            return Scriptable.NOT_FOUND;
        }
        final Object ret = bindings.get(name);
        if (ret != null && ret instanceof NativeJavaClass) {
            return ret;
        }
        return Context.jsToJava(bindings.get(name), Object.class);
    }

The NativeJavaClass was unwarped when used by own script, and when unwraped, Java classes cannot be instantiated by JavaScript code.

This solves the problem of instantiating imported Java classes, but after that, my script was getting a NullPointerException when called from a proxy collected by getInterface.

It's a script without arguments, and I need to change the method invokeMethodRaw on org.mozilla.javascript.engine.RhinoScriptEngine

From this:

if (args != null) {
  for (int i = 0; i < args.length; i++) {
      args[i] = Context.javaToJS(args[i], scope);
  }
}

To this:

if (args != null) {
  for (int i = 0; i < args.length; i++) {
      args[i] = Context.javaToJS(args[i], scope);
  }
} else {
 args = ScriptRuntime.emptyArgs;
}

Guys, this seems to be a bug or I did something on my code trying to use importClass function?