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.

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

Conversation

madoar
Copy link
Collaborator

@madoar madoar commented Jul 29, 2019

This PR adds a simple module system to include.

When using this PR every script loaded via include needs to export their values explicitly to the calling script. To export a value x and y you can call the following inside the included script:

module.x = <<first value to export>>;
module.y = <<second value to export>>;

In case only a single value is exported module.default can be used:

module.default = <<single export>>;

The exported values can then be included as followed:

// for a "default" export:
const Value = include(...);
// do something with Value

// for multiple exports:
const {x, y} = include(...);
// do something with x and y

Copy link
Member

@qparis qparis left a comment

Choose a reason for hiding this comment

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

Gread job!

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

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.

@@ -120,7 +120,7 @@ public void createShortcut(ShortcutDTO shortcutDTO) {
public void uninstallFromShortcut(ShortcutDTO shortcutDTO, Consumer<Exception> errorCallback) {
final InteractiveScriptSession interactiveScriptSession = scriptInterpreter.createInteractiveSession();

interactiveScriptSession.eval("include(\"engines.wine.shortcuts.reader\");",
interactiveScriptSession.eval("const ShortcutReader = include(\"engines.wine.shortcuts.reader\");",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not really related to this PR but: do we have the engine ID in the places where currently "wine" is hard coded?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a good point. Sadly no, we don't seem to have the engine id at hand here. I think we will need to add it to the ShortcutDTO class to have it available.

What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems reasonable. Can you describe the idea in an issue?

@madoar madoar merged commit b7127c6 into PhoenicisOrg:master Aug 11, 2019
@madoar madoar deleted the add-module-system branch August 11, 2019 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants