Improve modules error checking and reporting

This commit adds extra checks on the module paths.
It only allows absolute paths or relative path starting
with a ./ or ../ prefix.

A better description either for compilation and runtime
error is reported.

modules/nextflow/src/main/groovy/nextflow/Session.groovy

@@ -16,6 +16,7 @@
 
 package nextflow
 
+import nextflow.exception.ScriptCompilationException
 import static nextflow.Const.*
 
 import java.lang.reflect.Method
@@ -708,7 +709,8 @@ class Session implements ISession {
      */
     void abort(Throwable cause = null) {
         if( aborted ) return
-        log.debug "Session aborted -- Cause: ${cause?.message ?: cause ?: '-'}"
+        if( !(cause instanceof ScriptCompilationException) )
+            log.debug "Session aborted -- Cause: ${cause?.message ?: cause ?: '-'}"
         aborted = true
         error = cause
         try {

modules/nextflow/src/main/groovy/nextflow/cli/Launcher.groovy

@@ -16,8 +16,6 @@
 
 package nextflow.cli
 
-import static nextflow.Const.*
-
 import java.lang.reflect.Field
 
 import com.beust.jcommander.DynamicParameter
@@ -32,15 +30,19 @@ import groovy.util.logging.Slf4j
 import nextflow.exception.AbortOperationException
 import nextflow.exception.AbortRunException
 import nextflow.exception.ConfigParseException
+import nextflow.exception.ScriptCompilationException
 import nextflow.exception.ScriptRuntimeException
 import nextflow.trace.GraphObserver
 import nextflow.trace.ReportObserver
 import nextflow.trace.TimelineObserver
 import nextflow.trace.TraceFileObserver
 import nextflow.util.Escape
 import nextflow.util.LoggerHelper
-import org.codehaus.groovy.control.CompilationFailedException
 import org.eclipse.jgit.api.errors.GitAPIException
+import static nextflow.Const.APP_BUILDNUM
+import static nextflow.Const.APP_NAME
+import static nextflow.Const.APP_VER
+import static nextflow.Const.SPLASH
 /**
  * Main application entry point. It parses the command line and
  * launch the pipeline execution.
@@ -484,7 +486,7 @@ class Launcher {
             return(1)
         }
 
-        catch( CompilationFailedException e ) {
+        catch( ScriptCompilationException e ) {
             log.error e.message
             return(1)
         }

modules/nextflow/src/main/groovy/nextflow/exception/IllegalModulePath.groovy

@@ -0,0 +1,28 @@
+/*
+ * Copyright 2013-2019, Centre for Genomic Regulation (CRG)
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package nextflow.exception
+
+import groovy.transform.InheritConstructors
+
+/**
+ * Exception thrown when an illegal module path is specified
+ * 
+ * @author Paolo Di Tommaso <paolo.ditommaso@gmail.com>
+ */
+@InheritConstructors
+class IllegalModulePath extends Exception {
+}

modules/nextflow/src/main/groovy/nextflow/exception/ScriptCompilationException.groovy

@@ -0,0 +1,29 @@
+/*
+ * Copyright 2013-2019, Centre for Genomic Regulation (CRG)
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package nextflow.exception
+
+import groovy.transform.InheritConstructors
+
+/**
+ * Exception thrown when a script parsing fails due to
+ * compilation errors 
+ *
+ * @author Paolo Di Tommaso <paolo.ditommaso@gmail.com>
+ */
+@InheritConstructors
+class ScriptCompilationException extends Exception {
+}

modules/nextflow/src/main/groovy/nextflow/script/IncludeDef.groovy

@@ -24,7 +24,7 @@ import groovy.transform.EqualsAndHashCode
 import groovy.transform.Memoized
 import groovy.transform.PackageScope
 import nextflow.Session
-import nextflow.exception.ProcessException
+import nextflow.exception.IllegalModulePath
 /**
  * Implements a script inclusion
  *
@@ -67,8 +67,7 @@ class IncludeDef {
     }
 
     void load() {
-        if( !path )
-            throw new IllegalArgumentException("Missing module path attribute")
+        checkValidPath(path)
 
         // -- resolve the concrete against the current script
         final moduleFile = realModulePath(path)
@@ -87,22 +86,14 @@ class IncludeDef {
     @PackageScope
     @Memoized
     static BaseScript loadModule0(Path path, Map params, Session session) {
-        try {
-            final binding = new ScriptBinding() .setParams(params)
-
-            // the execution of a library file has as side effect the registration of declared processes
-            new ScriptParser(session)
-                    .setModule(true)
-                    .setBinding(binding)
-                    .runScript(path)
-                    .getScript()
-        }
-        catch( ProcessException e ) {
-            throw e
-        }
-        catch( Exception e ) {
-            throw new IllegalArgumentException("Unable to load module file: $path -- cause: ${e.cause?.message ?: e.message}", e)
-        }
+        final binding = new ScriptBinding() .setParams(params)
+
+        // the execution of a library file has as side effect the registration of declared processes
+        new ScriptParser(session)
+                .setModule(true)
+                .setBinding(binding)
+                .runScript(path)
+                .getScript()
     }
 
     @PackageScope
@@ -112,7 +103,7 @@ class IncludeDef {
         final result = include as Path
         if( result.isAbsolute() ) {
             if( result.scheme == 'file' ) return result
-            throw new IllegalArgumentException("Cannot resolve module path: ${result.toUriString()}")
+            throw new IllegalModulePath("Cannot resolve module path: ${result.toUriString()}")
         }
 
         return getOwnerPath().resolveSibling(include.toString())
@@ -136,4 +127,19 @@ class IncludeDef {
         throw new NoSuchFileException("Can't find a matching module file for include: $include")
     }
 
+    @PackageScope
+    void checkValidPath(path) {
+        if( !path )
+            throw new IllegalModulePath("Missing module path attribute")
+
+        if( path instanceof Path && path.scheme != 'file' )
+            throw new IllegalModulePath("Remote modules are not allowed -- Offending module: ${path.toUriString()}")
+
+        final str = path.toString()
+        if( !str.startsWith('/') && !str.startsWith('./') && !str.startsWith('../') )
+            throw new IllegalModulePath("Module path must start with / or ./ prefix -- Offending module: $str")
+
+    }
+
+
 }

modules/nextflow/src/main/groovy/nextflow/script/ScriptMeta.groovy

@@ -51,8 +51,8 @@ class ScriptMeta {
 
     static Map<String,Path> allScriptNames() {
         def result = new HashMap(REGISTRY.size())
-        for( Map.Entry<BaseScript,ScriptMeta> entry : REGISTRY )
-            result.put(entry.key.getClass().getName(), entry.value.scriptPath)
+        for( ScriptMeta entry : REGISTRY.values() )
+            result.put(entry.scriptName, entry.scriptPath)
         return result
     }
 
@@ -77,6 +77,8 @@ class ScriptMeta {
 
     Path getScriptPath() { scriptPath }
 
+    String getScriptName() { clazz.getName() }
+
     boolean isModule() { module }
 
     ScriptMeta(BaseScript script) {

modules/nextflow/src/main/groovy/nextflow/script/ScriptParser.groovy

@@ -23,13 +23,16 @@ import groovy.transform.CompileStatic
 import nextflow.Channel
 import nextflow.Nextflow
 import nextflow.Session
+import nextflow.ast.BranchXform
 import nextflow.ast.NextflowDSL
 import nextflow.ast.NextflowXform
-import nextflow.ast.BranchXform
+import nextflow.exception.ScriptCompilationException
+import nextflow.extension.FilesEx
 import nextflow.file.FileHelper
 import nextflow.util.Duration
 import nextflow.util.MemoryUnit
 import org.apache.commons.lang.StringUtils
+import org.codehaus.groovy.control.CompilationFailedException
 import org.codehaus.groovy.control.CompilerConfiguration
 import org.codehaus.groovy.control.customizers.ASTTransformationCustomizer
 import org.codehaus.groovy.control.customizers.ImportCustomizer
@@ -152,14 +155,25 @@ class ScriptParser {
     }
 
     ScriptParser parse(String scriptText, GroovyShell interpreter) {
-        final clazzName = computeClassName(scriptText)
-        script = (BaseScript)interpreter.parse(scriptText, clazzName)
-        final meta = ScriptMeta.get(script)
-        meta.setScriptPath(scriptPath)
-        meta.setModule(module)
-        return this
+        final String clazzName = computeClassName(scriptText)
+        try {
+            script = (BaseScript)interpreter.parse(scriptText, clazzName)
+            final meta = ScriptMeta.get(script)
+            meta.setScriptPath(scriptPath)
+            meta.setModule(module)
+            return this
+        }
+        catch (CompilationFailedException e) {
+            String type = module ? "Module" : "Script"
+            String header = "$type compilation error\n- file : ${FilesEx.toUriString(scriptPath)}"
+            String msg = e.message ?: header
+            msg = msg.replaceAll(/startup failed:\n/,'')
+            msg = msg.replaceAll(~/$clazzName(: \d+:\b*)?/, header+'\n- cause:')
+            throw new ScriptCompilationException(msg, e)
+        }
     }
 
+
     ScriptParser parse(String scriptText) {
         def interpreter = getInterpreter()
         return parse(scriptText, interpreter)

modules/nextflow/src/test/groovy/nextflow/extension/BranchOpTest.groovy

@@ -20,7 +20,7 @@ import spock.lang.Ignore
 import org.junit.Rule
 
 import nextflow.Channel
-import org.codehaus.groovy.control.MultipleCompilationErrorsException
+import nextflow.exception.ScriptCompilationException
 import test.Dls2Spec
 import test.OutputCapture
 /**
@@ -262,7 +262,7 @@ class BranchOpTest extends Dls2Spec  {
 
     def 'should pass criteria as argument' () {
         when:
-        def result = dsl_eval('''   
+        dsl_eval('''   
             criteria = branchCriteria { 
                 foo: it<5
                 bar: it>=5
@@ -291,7 +291,7 @@ class BranchOpTest extends Dls2Spec  {
                     }
         ''')
         then:
-        def e = thrown(MultipleCompilationErrorsException)
+        def e = thrown(ScriptCompilationException)
         e.message.contains 'Branch evaluation closure should declare at least one parameter or use the implicit `it` parameter'
     }
 
@@ -301,7 +301,7 @@ class BranchOpTest extends Dls2Spec  {
             Channel.empty() .branch { foo: true; foo: true }
         ''')
         then:
-        def e = thrown(MultipleCompilationErrorsException)
+        def e = thrown(ScriptCompilationException)
         e.message.contains 'Branch label already used: foo'
     }
 
@@ -311,7 +311,7 @@ class BranchOpTest extends Dls2Spec  {
             Channel.empty() .branch { foo: if(it) {}; bar: true }
         ''')
         then:
-        def e = thrown(MultipleCompilationErrorsException)
+        def e = thrown(ScriptCompilationException)
         e.message.contains 'Unexpected statement'
     }
 
@@ -321,15 +321,15 @@ class BranchOpTest extends Dls2Spec  {
             Channel.empty() .branch { def x=1 }
         ''')
         then:
-        def e = thrown(MultipleCompilationErrorsException)
+        def e = thrown(ScriptCompilationException)
         e.message.contains 'Branch evaluation closure should contain at least one branch expression'
 
         when:
         dsl_eval('''   
             Channel.empty() .branch {  }
         ''')
         then:
-        def e2 = thrown(MultipleCompilationErrorsException)
+        def e2 = thrown(ScriptCompilationException)
         e2.message.contains 'Branch evaluation closure should contain at least one branch expression'
     }
 
@@ -340,7 +340,7 @@ class BranchOpTest extends Dls2Spec  {
             Channel.empty() .branch { foo: true; if(x) {}  }
         ''')
         then:
-        def e = thrown(MultipleCompilationErrorsException)
+        def e = thrown(ScriptCompilationException)
         e.message.contains 'Unexpected statement in branch condition'
 
     }

modules/nextflow/src/test/groovy/nextflow/script/IncludeDefTest.groovy

@@ -7,6 +7,8 @@ import java.nio.file.NoSuchFileException
 import java.nio.file.Path
 
 import nextflow.ast.NextflowDSL
+import nextflow.exception.IllegalModulePath
+import nextflow.file.FileHelper
 import org.codehaus.groovy.control.CompilerConfiguration
 import org.codehaus.groovy.control.customizers.ASTTransformationCustomizer
 import test.TestHelper
@@ -32,7 +34,7 @@ class IncludeDefTest extends Specification {
         when:
         include.resolveModulePath('http://foo.com/bar')
         then:
-        thrown(IllegalArgumentException)
+        thrown(IllegalModulePath)
 
     }
 
@@ -63,6 +65,42 @@ class IncludeDefTest extends Specification {
 
     }
 
+    def 'should check valid path' () {
+        given:
+        def include = Spy(IncludeDef)
+
+        when:
+        include.checkValidPath('./module.nf')
+        then:
+        noExceptionThrown()
+
+        when:
+        include.checkValidPath('../rel/path')
+        then:
+        noExceptionThrown()
+
+        when:
+        include.checkValidPath('/abs/path')
+        then:
+        noExceptionThrown()
+
+        when:
+        include.checkValidPath('this/dir')
+        then:
+        thrown(IllegalModulePath)
+
+        when:
+        include.checkValidPath( 'http://foo.com/x.y')
+        then:
+        thrown(IllegalModulePath)
+
+        when:
+        include.checkValidPath(FileHelper.asPath('http://foo.com/x/y/z'))
+        then:
+        thrown(IllegalModulePath)
+
+    }
+
     // ==== DSL tests === 
 
     static class TestInclude extends IncludeDef {