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

Show file tree
Hide file tree
Changes from all commits
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
3 changes: 2 additions & 1 deletion phoenicis-containers/src/main/java/org/phoenicis/containers/GenericContainersManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,8 @@ public void deleteContainer(ContainerDTO container, Consumer<ContainerDTO> onSuc
final InteractiveScriptSession interactiveScriptSession = this.scriptInterpreter
.createInteractiveSession();

interactiveScriptSession.eval("include(\"engines." + engineId + ".shortcuts.reader\");",
interactiveScriptSession.eval(
"const ShortcutReader = include(\"engines." + engineId + ".shortcuts.reader\");",
ignored -> interactiveScriptSession.eval("new ShortcutReader()", output -> {
final org.graalvm.polyglot.Value shortcutReader = (org.graalvm.polyglot.Value) output;
shortcutReader.invokeMember("of", shortcut);
Expand Down
3 changes: 2 additions & 1 deletion ...fx/src/main/java/org/phoenicis/javafx/components/library/control/LibraryFeaturePanel.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,8 @@ public void createShortcut(ShortcutCreationDTO shortcutCreationDTO) {

final InteractiveScriptSession interactiveScriptSession = getScriptInterpreter().createInteractiveSession();

final String scriptInclude = "include(\"engines." + engineId + ".shortcuts." + engineId + "\");";
final String scriptInclude = "const Shortcut = include(\"engines." + engineId + ".shortcuts." + engineId
+ "\");";

interactiveScriptSession.eval(scriptInclude,
ignored -> interactiveScriptSession.eval("new " + engine + "Shortcut()",
Expand Down
3 changes: 2 additions & 1 deletion ...nicis-javafx/src/main/java/org/phoenicis/javafx/views/mainwindow/library/LibraryView.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,8 @@ private void createShortcut(ShortcutCreationDTO shortcutCreationDTO) {

final InteractiveScriptSession interactiveScriptSession = scriptInterpreter.createInteractiveSession();

final String scriptInclude = "include(\"engines." + engineId + "\".shortcuts." + engineId + "\");";
final String scriptInclude = "const Shortcut = include(\"engines." + engineId + "\".shortcuts." + engineId
+ "\");";

interactiveScriptSession.eval(scriptInclude,
ignored -> interactiveScriptSession.eval("new " + engine + "Shortcut()",
Expand Down
2 changes: 1 addition & 1 deletion phoenicis-library/src/main/java/org/phoenicis/library/ShortcutManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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?

ignored -> interactiveScriptSession.eval("new ShortcutReader()", output -> {
final Value shortcutReader = (Value) output;
shortcutReader.invokeMember("of", shortcutDTO);
Expand Down
4 changes: 2 additions & 2 deletions phoenicis-library/src/main/java/org/phoenicis/library/ShortcutRunner.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ public void run(String shortcutName, List<String> arguments, Consumer<Exception>
public void run(ShortcutDTO shortcutDTO, List<String> arguments, Consumer<Exception> errorCallback) {
final InteractiveScriptSession interactiveScriptSession = scriptInterpreter.createInteractiveSession();

interactiveScriptSession.eval("include(\"engines.wine.shortcuts.reader\");",
interactiveScriptSession.eval("const ShortcutReader = include(\"engines.wine.shortcuts.reader\");",
ignored -> interactiveScriptSession.eval("new ShortcutReader()", output -> {
final Value shortcutReader = (Value) output;

Expand All @@ -54,7 +54,7 @@ public void run(ShortcutDTO shortcutDTO, List<String> arguments, Consumer<Except
public void stop(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\");",
ignored -> interactiveScriptSession.eval("new ShortcutReader()", output -> {
final Value shortcutReader = (Value) output;

Expand Down
14 changes: 9 additions & 5 deletions ...icis-repository/src/test/java/org/phoenicis/repository/types/ClasspathRepositoryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,13 +73,17 @@ public void fetchInstallableApplicationsGraphicsPhotofiltreScriptName() {

@Test
public void fetchInstallableApplicationsGraphicsPhotofiltreScriptContent() {
assertEquals("include(\"engines.wine.quick_script.online_installer_script\");\n" + "\n"
+ "new OnlineInstallerScript()\n" + " .name(\"Photofiltre\")\n"
+ " .editor(\"Antonio Da Cruz\")\n" + " .applicationHomepage(\"http://photofiltre.free.fr\")\n"
assertEquals("const OnlineInstallerScript = include(\"engines.wine.quick_script.online_installer_script\");\n"
+ "\n"
+ "new OnlineInstallerScript()\n"
+ " .name(\"Photofiltre\")\n"
+ " .editor(\"Antonio Da Cruz\")\n"
+ " .applicationHomepage(\"http://photofiltre.free.fr\")\n"
+ " .author(\"Quentin PÂRIS\")\n"
+ " .url(\"http://photofiltre.free.fr/utils/pf-setup-fr-652.exe\")\n"
+ " .checksum(\"dc965875d698cd3f528423846f837d0dcf39616d\")\n" + " .category(\"Graphics\")\n"
+ " .executable(\"PhotoFiltre.exe\")\n" + " .go();",
+ " .checksum(\"dc965875d698cd3f528423846f837d0dcf39616d\")\n"
+ " .category(\"Graphics\")\n"
+ " .executable(\"PhotoFiltre.exe\");",
repository.fetchInstallableApplications().getTypes().get(0).getCategories().get(1).getApplications()
.get(0).getScripts()
.get(0).getScript().trim());
Expand Down
5 changes: 2 additions & 3 deletions ...rg/phoenicis/repository/repositoryTest/Applications/Graphics/Photofiltre/Online/script.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
include("engines.wine.quick_script.online_installer_script");
const OnlineInstallerScript = include("engines.wine.quick_script.online_installer_script");

new OnlineInstallerScript()
.name("Photofiltre")
Expand All @@ -8,5 +8,4 @@ new OnlineInstallerScript()
.url("http://photofiltre.free.fr/utils/pf-setup-fr-652.exe")
.checksum("dc965875d698cd3f528423846f837d0dcf39616d")
.category("Graphics")
.executable("PhotoFiltre.exe")
.go();
.executable("PhotoFiltre.exe");
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