[GitHub] [jena] kinow commented on a diff in pull request #1809: Some refinement of the scripting support.

["kinow (via GitHub)" <gi...@apache.org>]

kinow commented on code in PR #1809:
URL: https://github.com/apache/jena/pull/1809#discussion_r1146144444


##########
jena-arq/src/main/java/org/apache/jena/sparql/function/scripting/ScriptFunction.java:
##########
@@ -37,20 +36,26 @@
 import org.apache.jena.atlas.lib.PoolSync;
 import org.apache.jena.query.ARQ;
 import org.apache.jena.riot.RiotNotFoundException;
-import org.apache.jena.sparql.expr.ExprEvalException;
-import org.apache.jena.sparql.expr.ExprList;
-import org.apache.jena.sparql.expr.ExprUndefFunction;
-import org.apache.jena.sparql.expr.NodeValue;
+import org.apache.jena.sparql.expr.*;
 import org.apache.jena.sparql.function.FunctionBase;
-import org.apache.jena.sparql.sse.builders.SSE_ExprBuildException;
 
 public class ScriptFunction extends FunctionBase {
+
 	static {
-        System.setProperty("polyglot.engine.WarnInterpreterOnly", "false");
+	    System.setProperty("polyglot.engine.WarnInterpreterOnly", "false");

Review Comment:
   Not sure if this is a tab, nor if the file has mixed tabs & spaces, but just in case it is and if it matters :+1: 



##########
jena-arq/src/main/java/org/apache/jena/sparql/function/scripting/ScriptFunction.java:
##########
@@ -59,36 +64,51 @@ public class ScriptFunction extends FunctionBase {
     private String lang;
     private String name;
 
+    // Collect language names (for reference).
+//    private static Set<String> languageNames = new HashSet<>();
+//    static {
+//        scriptEngineManager
+//            .getEngineFactories()
+//            .forEach(sef -> sef.getNames().forEach(languageNames::add));
+//    }
+
     public static boolean isScriptFunction(String uri) {
         if (!uri.startsWith(ARQ_NS)) {
             return false;
         }
         String localPart = uri.substring(ARQ_NS.length());
         int separatorPos = localPart.indexOf('#');
-        if (separatorPos < 0) {
+        if (separatorPos < 0)
             return false;
-        }
         String langPart = localPart.substring(0, separatorPos);
-        if (!langPart.endsWith(FUNCTION_SUFFIX)) {
-            return false;
-        }
-        return true;
+        return langPart.endsWith(FUNCTION_SUFFIX);
     }
 
     @Override
     public void checkBuild(String uri, ExprList args) {
-        if (!isScriptFunction(uri)) {
-            throw new SSE_ExprBuildException("Invalid URI: " + uri);
-        }
-
+        checkScriptingEnabled();
+        if (!isScriptFunction(uri))
+            throw new ExprException("Invalid URI: " + uri);
         String localPart = uri.substring(ARQ_NS.length());
         int separatorPos = localPart.indexOf('#');
         this.lang = localPart.substring(0, separatorPos - FUNCTION_SUFFIX.length());
         this.name = localPart.substring(separatorPos + 1);
+
+        // Check for bare names that are provided by the language e.g. 'eval' which
+        // is a JS and Python built-in function and always available.
+        if ( lang.toLowerCase(Locale.ROOT).contains("python") ) {
+            if ( Objects.equals("eval", name) || Objects.equals("exec", name) )
+                throw new ExprException(lang+" function '"+name+"' is not allowed");
+        } else {
+            // Assume javascript.
+            if ( Objects.equals("eval", name) )
+                throw new ExprException("JS function '"+name+"' is not allowed");
+        }

Review Comment:
   :+1: maybe in the future we could have sandbox classes like `PythonSandbox`, `GroovySandbox`, `JavaScriptSandbox`, etc., encapsulating the functions or other things that we want to sandbox/prohibit, then just call `sandbox.. That's how Jenkins Security Script plug-in works (but with a single [sandbox for Groovy](https://github.com/jenkinsci/script-security-plugin/blob/e8ba1732da2dd7dadce7034c5576e8f802cb230b/src/main/java/org/jenkinsci/plugins/scriptsecurity/sandbox/groovy/SecureGroovyScript.java#L390-L466) in their case).



##########
jena-arq/src/main/java/org/apache/jena/sparql/function/scripting/ScriptFunction.java:
##########
@@ -37,20 +36,26 @@
 import org.apache.jena.atlas.lib.PoolSync;
 import org.apache.jena.query.ARQ;
 import org.apache.jena.riot.RiotNotFoundException;
-import org.apache.jena.sparql.expr.ExprEvalException;
-import org.apache.jena.sparql.expr.ExprList;
-import org.apache.jena.sparql.expr.ExprUndefFunction;
-import org.apache.jena.sparql.expr.NodeValue;
+import org.apache.jena.sparql.expr.*;
 import org.apache.jena.sparql.function.FunctionBase;
-import org.apache.jena.sparql.sse.builders.SSE_ExprBuildException;
 
 public class ScriptFunction extends FunctionBase {
+
 	static {
-        System.setProperty("polyglot.engine.WarnInterpreterOnly", "false");
+	    System.setProperty("polyglot.engine.WarnInterpreterOnly", "false");
     }
-	
+
+	private static void checkScriptingEnabled() {

Review Comment:
   Wrong indentation? Not sure if related to tabs/spaces, or maybe GH UI acting funny.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@jena.apache.org
For additional commands, e-mail: pr-help@jena.apache.org