Merge pull request #136 from javadelight/issue-134

Ensuring only safe script contexts are passed into the engine

.project

@@ -28,12 +28,12 @@
 	</natures>
 	<filteredResources>
 		<filter>
-			<id>1602570532382</id>
+			<id>1675032386265</id>
 			<name></name>
 			<type>30</type>
 			<matcher>
 				<id>org.eclipse.core.resources.regexFilterMatcher</id>
-				<arguments>node_modules|.git|__CREATED_BY_JAVA_LANGUAGE_SERVER__</arguments>
+				<arguments>node_modules|\.git|__CREATED_BY_JAVA_LANGUAGE_SERVER__</arguments>
 			</matcher>
 		</filter>
 	</filteredResources>

README.md

@@ -110,6 +110,7 @@ for JS evaluation and better handling of monitoring for threads for possible CPU
 
 ## Version History
 
+- 0.3.0: Creating a wrapper for Script Context to be passed to eval to avoid accidental exposure. Resolves [Issue #134](https://github.com/javadelight/delight-nashorn-sandbox/issues/134)
 - 0.2.5: Support for pre-compiled scripts ([PR #119](https://github.com/javadelight/delight-nashorn-sandbox/pull/119) by [geoffreyharding](https://github.com/geoffreyharding)]
 - 0.2.4: Increased resiliency for killing threads under high load ([PR #118](https://github.com/javadelight/delight-nashorn-sandbox/pull/118) by [tellmewhattodo](https://github.com/tellmewhattodo))
 - 0.2.0: Dynamically detects what version of the JDK the library is run with, and will either use the included Nashorn version or use Nashorn dependency ([Issue #109](https://github.com/javadelight/delight-nashorn-sandbox/issues/109), [PR #101](https://github.com/javadelight/delight-nashorn-sandbox/pull/111)).

pom.xml

@@ -5,7 +5,7 @@
 
 	<groupId>org.javadelight</groupId>
 	<artifactId>delight-nashorn-sandbox</artifactId>
-	<version>0.2.5</version>
+	<version>0.3.0</version>
 	<description>A safe sandbox to execute JavaScript code from Nashorn.</description>
 	<url>https://github.com/javadelight/delight-nashorn-sandbox</url>
 

src/main/java/delight/nashornsandbox/NashornSandbox.java

@@ -133,7 +133,7 @@ public interface NashornSandbox {
    * @throws ScriptException when script syntax error occurs
    * @see #setMaxCPUTime(long)
    */
-  Object eval(String js,Bindings bindings) throws ScriptCPUAbuseException, ScriptException;
+  Object eval(String js, Bindings bindings) throws ScriptCPUAbuseException, ScriptException;
 
   /**
    * Evaluates the JavaScript string for a given script context
@@ -145,7 +145,7 @@ public interface NashornSandbox {
    * @throws ScriptException when script syntax error occurs
    * @see #setMaxCPUTime(long)
    */
-  Object eval(String js, ScriptContext scriptContext) throws ScriptCPUAbuseException, ScriptException;
+  Object eval(String js, SandboxScriptContext scriptContext) throws ScriptCPUAbuseException, ScriptException;
 
 
   /**
@@ -159,7 +159,7 @@ public interface NashornSandbox {
    * @throws ScriptException when script syntax error occurs
    * @see #setMaxCPUTime(long)
    */
-  Object eval(String js, ScriptContext scriptContext,Bindings bindings) throws ScriptCPUAbuseException, ScriptException;
+  Object eval(String js, SandboxScriptContext scriptContext,Bindings bindings) throws ScriptCPUAbuseException, ScriptException;
 
   /**
    * Obtains the value of the specified JavaScript variable.
@@ -310,5 +310,11 @@ public interface NashornSandbox {
   Object eval(CompiledScript compiledScript) throws ScriptCPUAbuseException, ScriptException;
   Object eval(CompiledScript compiledScript, Bindings bindings) throws ScriptCPUAbuseException, ScriptException;
   Object eval(CompiledScript compiledScript, ScriptContext scriptContext) throws ScriptCPUAbuseException, ScriptException;
-  Object eval(CompiledScript compiledScript, ScriptContext scriptContext,Bindings bindings) throws ScriptCPUAbuseException, ScriptException;
+  Object eval(CompiledScript compiledScript, ScriptContext scriptContext, Bindings bindings) throws ScriptCPUAbuseException, ScriptException;
+
+  /**
+ * Create a new secured script context
+ */
+SandboxScriptContext createScriptContext();
+
 }

src/main/java/delight/nashornsandbox/SandboxScriptContext.java

@@ -0,0 +1,8 @@
+package delight.nashornsandbox;
+
+import javax.script.ScriptContext;
+
+public interface SandboxScriptContext {
+
+  public ScriptContext getContext();
+}

src/main/java/delight/nashornsandbox/internal/NashornSandboxImpl.java

@@ -1,6 +1,7 @@
 package delight.nashornsandbox.internal;
 
 import delight.nashornsandbox.NashornSandbox;
+import delight.nashornsandbox.SandboxScriptContext;
 import delight.nashornsandbox.SecuredJsCache;
 import delight.nashornsandbox.exceptions.ScriptCPUAbuseException;
 import delight.nashornsandbox.exceptions.ScriptMemoryAbuseException;
@@ -14,6 +15,8 @@
 import javax.script.ScriptContext;
 import javax.script.ScriptEngine;
 import javax.script.ScriptException;
+import javax.script.SimpleScriptContext;
+
 import java.io.Writer;
 import java.lang.reflect.InvocationTargetException;
 import java.lang.reflect.Method;
@@ -125,9 +128,9 @@ public ScriptEngine createNashornScriptEngineFactory(String ... params) {
 	private synchronized void assertScriptEngine() {
 		try {
 			if (!engineAsserted.get()) {
-				produceSecureBindings();
+				produceSecureBindings(scriptEngine.getContext());
 			} else if (!engineBindingUnchanged()) {
-				resetEngineBindings();
+				resetEngineBindings(scriptEngine.getContext());
 			}
 		} catch (final Exception e) {
 			throw new RuntimeException(e);
@@ -146,10 +149,10 @@ private boolean engineBindingUnchanged() {
 		return true;
 	}
 
-	private void produceSecureBindings() {
+	private void produceSecureBindings(ScriptContext context) {
 		try {
 			final StringBuilder sb = new StringBuilder();
-			cached = scriptEngine.getBindings(ScriptContext.ENGINE_SCOPE);
+			cached = context.getBindings(ScriptContext.ENGINE_SCOPE);
 			sanitizeBindings(cached);
 			if (!allowExitFunctions) {
 				sb.append("var quit=function(){};var exit=function(){};");
@@ -169,9 +172,9 @@ private void produceSecureBindings() {
 				sb.append("var $ARG=null;var $ENV=null;var $EXEC=null;");
 				sb.append("var $OPTIONS=null;var $OUT=null;var $ERR=null;var $EXIT=null;");
 			}
-			scriptEngine.eval(sb.toString());
+			scriptEngine.eval(sb.toString(), context);
 
-			resetEngineBindings();
+			resetEngineBindings(context);
 
 			engineAsserted.set(true);
 
@@ -180,11 +183,11 @@ private void produceSecureBindings() {
 		}
 	}
 
-	protected void resetEngineBindings() {
+	protected void resetEngineBindings(ScriptContext context) {
 		final Bindings bindings = createBindings();
 		sanitizeBindings(bindings);
 		bindings.putAll(cached);
-		scriptEngine.setBindings(bindings, ScriptContext.ENGINE_SCOPE);
+		context.setBindings(bindings, ScriptContext.ENGINE_SCOPE);
 	}
 
 	protected void sanitizeBindings(Bindings bindings) {
@@ -206,7 +209,24 @@ protected void sanitizeBindings(Bindings bindings) {
 		}
 	}
 
+
+
+
 	@Override
+  public SandboxScriptContext createScriptContext() {
+		ScriptContext context = new SimpleScriptContext();
+		produceSecureBindings(context);
+    return new SandboxScriptContext() {
+
+			@Override
+			public ScriptContext getContext() {
+				return context;
+			}
+
+		};
+  }
+
+  @Override
 	public Object eval(final String js) throws ScriptCPUAbuseException, ScriptException {
 		return eval(js, null, null);
 	}
@@ -217,12 +237,12 @@ public Object eval(String js, Bindings bindings) throws ScriptCPUAbuseException,
 	}
 
 	@Override
-	public Object eval(String js, ScriptContext scriptContext) throws ScriptCPUAbuseException, ScriptException {
+	public Object eval(String js, SandboxScriptContext scriptContext) throws ScriptCPUAbuseException, ScriptException {
 		return eval(js, scriptContext, null);
 	}
 
 	@Override
-	public Object eval(final String js, final ScriptContext scriptContext, final Bindings bindings)
+	public Object eval(final String js, final SandboxScriptContext scriptContext, final Bindings bindings)
 			throws ScriptCPUAbuseException, ScriptException {
 		assertScriptEngine();
 		final JsSanitizer sanitizer = getSanitizer();
@@ -233,13 +253,17 @@ public Object eval(final String js, final ScriptContext scriptContext, final Bin
 		if (scriptContext == null) {
 			securedJs = blockAccessToEngine + sanitizer.secureJs(js);
 		} else {
-			// Unfortunately, blocking access to the engine property inteferes with setting
-			// a script context
-			// needs further investigation
+			// Unfortunately, blocking access to the engine property interferes with setting
+			// a script context needs further investigation
 			securedJs = sanitizer.secureJs(js);
 		}
 		final Bindings securedBindings = secureBindings(bindings);
-		EvaluateOperation op = new EvaluateOperation(securedJs, scriptContext, securedBindings);
+    EvaluateOperation op;
+		if (scriptContext != null) {
+		 op = new EvaluateOperation(securedJs, scriptContext.getContext(), securedBindings);
+		} else {
+		 op = new EvaluateOperation(securedJs, null, securedBindings);
+		}
 		return executeSandboxedOperation(op);
 	}
 

src/test/java/delight/nashornsandbox/TestEvalWithScriptContext.java

@@ -17,12 +17,12 @@ public class TestEvalWithScriptContext {
   @Test
   public void test() throws ScriptCPUAbuseException, ScriptException {
     final NashornSandbox sandbox = NashornSandboxes.create();
-    ScriptContext newContext1 = new SimpleScriptContext();
-    Bindings engineScope1 = newContext1.getBindings(ScriptContext.ENGINE_SCOPE);
+    SandboxScriptContext newContext1 = sandbox.createScriptContext(); 
+    Bindings engineScope1 = newContext1.getContext().getBindings(ScriptContext.ENGINE_SCOPE);
     engineScope1.put("y", 2);
 
-    ScriptContext newContext2 = new SimpleScriptContext();
-    Bindings engineScope2 = newContext2.getBindings(ScriptContext.ENGINE_SCOPE);
+    SandboxScriptContext newContext2 = sandbox.createScriptContext();
+    Bindings engineScope2 = newContext2.getContext().getBindings(ScriptContext.ENGINE_SCOPE);
     engineScope2.put("y", 4);
 
     final Object res1 = sandbox.eval("function cal() {var x = y + 1; return x;} cal();", newContext1);
@@ -56,12 +56,12 @@ public void testWithCPUAndMemory() throws ScriptCPUAbuseException, ScriptExcepti
     sandbox.setMaxCPUTime(100);
     sandbox.setMaxMemory(1000 * 1024);
     sandbox.setExecutor(Executors.newSingleThreadExecutor());
-    ScriptContext newContext1 = new SimpleScriptContext();
-    Bindings engineScope1 = newContext1.getBindings(ScriptContext.ENGINE_SCOPE);
+    SandboxScriptContext newContext1 = sandbox.createScriptContext();
+    Bindings engineScope1 = newContext1.getContext().getBindings(ScriptContext.ENGINE_SCOPE);
     engineScope1.put("y", 2);
 
-    ScriptContext newContext2 = new SimpleScriptContext();
-    Bindings engineScope2 = newContext2.getBindings(ScriptContext.ENGINE_SCOPE);
+    SandboxScriptContext newContext2 = sandbox.createScriptContext();
+    Bindings engineScope2 = newContext2.getContext().getBindings(ScriptContext.ENGINE_SCOPE);
     engineScope2.put("y", 4);
 
     final Object res1 = sandbox.eval("function cal() {var x = y + 1; return x;} cal();", newContext1);

src/test/java/delight/nashornsandbox/TestEvalWithScriptContextWithNewBindings.java

@@ -37,11 +37,11 @@ public void testWithNewSimpleBindings() throws ScriptCPUAbuseException, ScriptEx
 	@Test
 	public void testWithNewBindingsScriptContext() throws ScriptCPUAbuseException, ScriptException {
 		final NashornSandbox sandbox = NashornSandboxes.create();
-		ScriptContext newContext = new SimpleScriptContext();
+		SandboxScriptContext newContext = sandbox.createScriptContext();
 		// Create new binding to override the ECMAScript Global properties 
 		Bindings newBinding = sandbox.createBindings();
 		newBinding.put("Date", "2112018");
-		newContext.setBindings(newBinding, ScriptContext.ENGINE_SCOPE);
+		newContext.getContext().setBindings(newBinding, ScriptContext.ENGINE_SCOPE);
 
 		final Object res = sandbox.eval("function method() { return parseInt(Date);} method();", newContext);
     Assert.assertTrue(res.equals(Double.valueOf("2112018.0")) || res.equals(Integer.valueOf(2112018)));
@@ -50,14 +50,12 @@ public void testWithNewBindingsScriptContext() throws ScriptCPUAbuseException, S
 	@Test
 	public void testWithExistingBindings() throws ScriptCPUAbuseException, ScriptException {
 		final NashornSandbox sandbox = NashornSandboxes.create();
-		ScriptContext newContext = new SimpleScriptContext();
-		Bindings newBinding = newContext.getBindings(ScriptContext.ENGINE_SCOPE);
-		// This will not be updated by using existing bindings, since Date is a 
-		// ECMAScript "global" properties and it is being in ENGINE_SCOPE
+		SandboxScriptContext newContext = sandbox.createScriptContext();
+		Bindings newBinding = newContext.getContext().getBindings(ScriptContext.ENGINE_SCOPE);
 		newBinding.put("Date", "2112018");
 
 		final Object res = sandbox.eval("function method() { return parseInt(Date);} method();", newContext);
-		Assert.assertTrue(Double.isNaN((Double)res));
+		Assert.assertTrue(res.equals(2112018));
 	}
 
 

src/test/java/delight/nashornsandbox/TestEvalWithScriptContextWithVariables.java

@@ -16,8 +16,8 @@ public class TestEvalWithScriptContextWithVariables {
   @Test
   public void test() throws ScriptCPUAbuseException, ScriptException {
     final NashornSandbox sandbox = NashornSandboxes.create();
-    ScriptContext newContext1 = new SimpleScriptContext();
-    ScriptContext newContext2 = new SimpleScriptContext();
+    SandboxScriptContext newContext1 = sandbox.createScriptContext(); 
+    SandboxScriptContext newContext2 = sandbox.createScriptContext();
 
     sandbox.eval("function cal() {var x = 1; return x;}", newContext1);
     sandbox.eval("function cal() {var x = 2; return x;}", newContext2);
@@ -36,8 +36,8 @@ public void testWithCPUAndMemory() throws ScriptCPUAbuseException, ScriptExcepti
     sandbox.setMaxCPUTime(100);
     sandbox.setMaxMemory(1000 * 1024);
     sandbox.setExecutor(Executors.newSingleThreadExecutor());
-    ScriptContext newContext1 = new SimpleScriptContext();
-    ScriptContext newContext2 = new SimpleScriptContext();
+    SandboxScriptContext newContext1 = sandbox.createScriptContext();
+    SandboxScriptContext newContext2 = sandbox.createScriptContext();
 
     sandbox.eval("function cal() {var x = 1; return x;}", newContext1);
     sandbox.eval("function cal() {var x = 2; return x;}", newContext2);

src/test/java/delight/nashornsandbox/TestExit.java

@@ -34,11 +34,10 @@ public void testQuitWithBindings() throws ScriptCPUAbuseException, ScriptExcepti
 
 
 
-  @Test
-  @Ignore("This will fail as there is no confirmation on Script Contexts")
+  @Test(expected = javax.script.ScriptException.class)
   public void testQuitWithScriptContext() throws ScriptCPUAbuseException, ScriptException {
     final NashornSandbox sandbox = NashornSandboxes.create();
-    sandbox.eval("quit();", new SimpleScriptContext());
+    sandbox.eval("quit();", sandbox.createScriptContext());
   }
 
 

src/test/java/delight/nashornsandbox/TestIssue134.java

@@ -0,0 +1,24 @@
+package delight.nashornsandbox;
+
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+
+import javax.script.ScriptContext;
+import javax.script.ScriptException;
+import javax.script.SimpleScriptContext;
+
+import org.junit.Test;
+
+import delight.nashornsandbox.exceptions.ScriptCPUAbuseException;
+
+public class TestIssue134 {
+
+	@Test(expected = javax.script.ScriptException.class)
+	public void test() throws ScriptCPUAbuseException, ScriptException {
+		NashornSandbox sandbox = NashornSandboxes.create();
+		SandboxScriptContext context = sandbox.createScriptContext();
+		sandbox.eval("load('classpath:TestIssue134.js')", context);
+		sandbox.eval("load('somethingwrong')", context);
+	}
+
+}

src/test/resources/TestIssue134.js

@@ -0,0 +1,2 @@
+print('loaded');
+throw new Error('I should never be called!');