Skip to content
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.

Sign up for GitHub

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

Add a simple Module System for "include" #2050

Merged
merged 3 commits into from
Aug 11, 2019
Merged

Add a simple Module System for "include" #2050

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6d35780
- adds a simple module system supporting module.x = <<exported value>>
madoar Jul 29, 2019
ea9e443
Merge branch 'master' of github.com:PhoenicisOrg/POL-POM-5 into add-m…
madoar Aug 10, 2019
8ef7776
- add some missing const ...
madoar Aug 10, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 17 additions & 3 deletions phoenicis-scripts/src/main/java/org/phoenicis/scripts/engine/injectors/IncludeInjector.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package org.phoenicis.scripts.engine.injectors;

import org.graalvm.polyglot.Value;
import org.phoenicis.scripts.engine.implementation.PhoenicisScriptEngine;
import org.phoenicis.scripts.interpreter.ScriptException;
import org.phoenicis.scripts.interpreter.ScriptFetcher;
Expand Down Expand Up @@ -30,9 +31,22 @@ public void injectInto(PhoenicisScriptEngine phoenicisScriptEngine) {
throwException(new ScriptException("Script '" + argument + "' is not found"));
}

includedScripts.put(argument,
phoenicisScriptEngine.evalAndReturn("//# sourceURL=" + argument + "\n" + script,
this::throwException));
// wrap the loaded script in a function to prevent it from influencing the main script
String extendedString = String.format("(module) => { %s }", script);
Copy link
Member

Choose a reason for hiding this comment

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

Can we do something here to prevent module escaping?

Copy link
Collaborator Author

@madoar madoar Aug 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I believe by module escaping you mean to prevent an included script to make modifications to the state/context/variables of including script?

If this is what you mean then I don't think we can prevent module escaping for two reasons:

  1. we're currently exploiting this ourselves in the verbs and plugins. Both extend Wine with additional functions. This modifies the context of the including script and possibly even other scripts.
  2. ignoring point 1 a solution would be to execute every included script in its own graaljs context. Afterwards we could then "inject" the resulting module object in the including script. Sadly this is currently not possible, see also Sharing / Re-using values across Contexts oracle/graal#631 for more details

Copy link
Member

Choose a reason for hiding this comment

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

Let’s imagine that script = “}; malicious code //“

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's something we should consider but I guess it will blow up the PR too much.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Let’s imagine that script = “}; malicious code //“

I guess it depends on what malicious code wants to do.

If the malicious code is trying to affect the main script (i.e. the including script) I don't think it is a big danger, because as far as I know it is only able to affect existing global variables. A part of the remaining danger should be solvable by executing the included script in a custom context (this is currently blocked by oracle/graal#631).

If the malicious code wants to access parts of the Java API I think we can be protected by your approach in #2018.

Apart from that I agree with @plata, I think that we should create a new issue to continue this discussion, because it seems to be a bit over the top for this PR.

Value includeFunction = (Value) phoenicisScriptEngine.evalAndReturn(extendedString,
this::throwException);

// create an empty JS object
Value module = (Value) phoenicisScriptEngine.evalAndReturn("({})", this::throwException);

// execute the included function -> populates "module"
includeFunction.execute(module);

if (module.hasMember("default")) {
includedScripts.put(argument, module.getMember("default"));
} else {
includedScripts.put(argument, module);
}
}

return includedScripts.get(argument);
Expand Down