Fix silent message corruption from absent/blank channel scripts (#344)

server/src/main/java/com/mirth/connect/server/builders/JavaScriptBuilder.java

@@ -27,6 +27,7 @@
 import com.mirth.connect.model.MetaData;
 import com.mirth.connect.model.Rule;
 import com.mirth.connect.model.Step;
+import com.google.common.annotations.VisibleForTesting;
 import com.mirth.connect.model.Transformer;
 import com.mirth.connect.model.codetemplates.CodeTemplate;
 import com.mirth.connect.model.codetemplates.CodeTemplateLibrary;
@@ -45,6 +46,12 @@ public class JavaScriptBuilder {
     private static ExtensionController extensionController = ControllerFactory.getFactory().createExtensionController();
     private static CodeTemplateController codeTemplateController = ControllerFactory.getFactory().createCodeTemplateController();
 
+    @VisibleForTesting
+    public static void setControllersForTesting(ExtensionController ec, CodeTemplateController ctc) {
+        extensionController = ec;
+        codeTemplateController = ctc;
+    }
+
     /*
      * Generates the global JavaScript contained in all new scopes created
      */

server/src/main/java/com/mirth/connect/server/util/javascript/JavaScriptUtil.java

@@ -129,7 +129,7 @@ public Object doCall() throws Exception {
             throw e;
         }
 
-        if (result != null) {
+        if (result != null && !(Undefined.isUndefined(result))) {
             String resultString = (String) Context.jsToJava(result, java.lang.String.class);
 
             if (resultString != null) {
@@ -197,7 +197,7 @@ public static String executePreprocessorScripts(JavaScriptTask<Object> task, Con
                 }
             }
 
-            if (result != null) {
+            if (result != null && !(Undefined.isUndefined(result))) {
                 String resultString = (String) Context.jsToJava(result, java.lang.String.class);
 
                 // Set the processed message in case something goes wrong in the channel processor. Also update the global result so the channel processor uses the updated message
@@ -226,7 +226,7 @@ public static String executePreprocessorScripts(JavaScriptTask<Object> task, Con
                 }
             }
 
-            if (result != null) {
+            if (result != null && !(Undefined.isUndefined(result))) {
                 String resultString = (String) Context.jsToJava(result, java.lang.String.class);
 
                 // Set the processed message if there was a result.
@@ -330,7 +330,7 @@ private static Response getPostprocessorResponse(Object result) {
                 // TODO: is it okay that we use Status.SENT here?
                 response = new Response(Status.SENT, object.toString());
             }
-        } else if ((result != null) && !(result instanceof Undefined)) {
+        } else if ((result != null) && !(Undefined.isUndefined(result))) {
             // This branch will catch all objects that aren't Response, NativeJavaObject, Undefined, or null
             // Assume it's a string, and return a successful response
             // TODO: is it okay that we use Status.SENT here?
@@ -710,6 +710,13 @@ public static boolean compileAndAddScript(String channelId, MirthContextFactory
         // Note: If the defaultScript is NULL, this means that the script should
         // always be inserted without being compared.
 
+        // A null or blank script does nothing; treat it as absent instead of
+        // compiling a wrapper whose body is the literal string "null" (MIRTH/OIE #344)
+        if (StringUtils.isBlank(script)) {
+            compiledScriptCache.removeCompiledScript(scriptId);
+            return false;
+        }
+
         boolean scriptInserted = false;
         String generatedScript = null;
 

server/src/test/java/com/mirth/connect/server/util/javascript/JavaScriptUtilTest.java

@@ -0,0 +1,157 @@
+/*
+ * Copyright (c) Mirth Corporation. All rights reserved.
+ *
+ * http://www.mirthcorp.com
+ *
+ * The software in this package is published under the terms of the MPL license a copy of which has
+ * been included with this distribution in the LICENSE.txt file.
+ */
+
+package com.mirth.connect.server.util.javascript;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import java.net.URL;
+import java.util.HashMap;
+import java.util.HashSet;
+
+import org.junit.After;
+import org.junit.BeforeClass;
+import org.junit.Test;
+
+import com.google.inject.AbstractModule;
+import com.google.inject.Guice;
+import com.google.inject.Injector;
+import com.mirth.connect.donkey.model.message.ConnectorMessage;
+import com.mirth.connect.donkey.model.message.MessageContent;
+import com.mirth.connect.model.codetemplates.ContextType;
+import com.mirth.connect.server.builders.JavaScriptBuilder;
+import com.mirth.connect.server.controllers.CodeTemplateController;
+import com.mirth.connect.server.controllers.ConfigurationController;
+import com.mirth.connect.server.controllers.ControllerFactory;
+import com.mirth.connect.server.controllers.EventController;
+import com.mirth.connect.server.controllers.ExtensionController;
+import com.mirth.connect.server.controllers.ScriptController;
+import com.mirth.connect.server.util.CompiledScriptCache;
+
+public class JavaScriptUtilTest {
+
+    private static final String SCRIPT_ID = "JavaScriptUtilTest-script";
+
+    @BeforeClass
+    public static void setUpBeforeClass() {
+        // Same mocked ControllerFactory pattern as FileReceiverTest, so this class is
+        // self-sufficient regardless of which test classes ran (and injected) before it.
+        ControllerFactory controllerFactory = mock(ControllerFactory.class);
+
+        EventController eventController = mock(EventController.class);
+        when(controllerFactory.createEventController()).thenReturn(eventController);
+
+        ConfigurationController configurationController = mock(ConfigurationController.class);
+        when(controllerFactory.createConfigurationController()).thenReturn(configurationController);
+
+        ExtensionController extensionController = mock(ExtensionController.class);
+        when(controllerFactory.createExtensionController()).thenReturn(extensionController);
+
+        CodeTemplateController codeTemplateController = mock(CodeTemplateController.class);
+        when(controllerFactory.createCodeTemplateController()).thenReturn(codeTemplateController);
+
+        Injector injector = Guice.createInjector(new AbstractModule() {
+            @Override
+            protected void configure() {
+                requestStaticInjection(ControllerFactory.class);
+                bind(ControllerFactory.class).toInstance(controllerFactory);
+            }
+        });
+        injector.getInstance(ControllerFactory.class);
+
+        /*
+         * JavaScriptBuilder captures its controllers in static fields at class-load time. If an
+         * earlier test class loaded it with a mocked factory that left them null, repair them so
+         * generateGlobalSealedScript/appendCodeTemplates don't NPE.
+         */
+        JavaScriptBuilder.setControllersForTesting(extensionController, codeTemplateController);
+    }
+
+    private MirthContextFactory contextFactory() {
+        return new MirthContextFactory(new URL[0], new HashSet<>(), false);
+    }
+
+    @After
+    public void cleanup() {
+        CompiledScriptCache.getInstance().removeCompiledScript(SCRIPT_ID);
+    }
+
+    @Test
+    public void compileAndAddScriptWithNullScriptDoesNotCompile() throws Exception {
+        boolean inserted = JavaScriptUtil.compileAndAddScript("channelId", contextFactory(), SCRIPT_ID, null, ContextType.CHANNEL_PREPROCESSOR);
+        assertFalse(inserted);
+        assertNull(CompiledScriptCache.getInstance().getCompiledScript(SCRIPT_ID));
+    }
+
+    @Test
+    public void compileAndAddScriptWithBlankScriptDoesNotCompile() throws Exception {
+        boolean inserted = JavaScriptUtil.compileAndAddScript("channelId", contextFactory(), SCRIPT_ID, "   \n", ContextType.CHANNEL_PREPROCESSOR);
+        assertFalse(inserted);
+        assertNull(CompiledScriptCache.getInstance().getCompiledScript(SCRIPT_ID));
+    }
+
+    @Test
+    public void compileAndAddScriptWithRealScriptCompiles() throws Exception {
+        boolean inserted = JavaScriptUtil.compileAndAddScript("channelId", contextFactory(), SCRIPT_ID, "var x = 1; return 'x';", ContextType.CHANNEL_PREPROCESSOR);
+        assertTrue(inserted);
+        assertNotNull(CompiledScriptCache.getInstance().getCompiledScript(SCRIPT_ID));
+    }
+
+    private static final String CHANNEL_ID = "JavaScriptUtilTest-channel";
+
+    private ConnectorMessage messageWithRaw() {
+        ConnectorMessage message = mock(ConnectorMessage.class);
+        MessageContent rawContent = mock(MessageContent.class);
+        when(message.getRaw()).thenReturn(rawContent);
+        when(rawContent.getContent()).thenReturn("MSH|^~\\&|X");
+        when(message.getChannelId()).thenReturn(CHANNEL_ID);
+        return message;
+    }
+
+    private JavaScriptTask<Object> task(MirthContextFactory contextFactory) {
+        return new JavaScriptTask<>(contextFactory, "JavaScriptUtilTest") {
+            @Override
+            public Object doCall() {
+                return null;
+            }
+        };
+    }
+
+    @Test
+    public void preprocessorReturningNothingYieldsNullNotUndefined() throws Exception {
+        String scriptId = ScriptController.getScriptId(ScriptController.PREPROCESSOR_SCRIPT_KEY, CHANNEL_ID);
+        MirthContextFactory contextFactory = contextFactory();
+        try {
+            JavaScriptUtil.compileAndAddScript(CHANNEL_ID, contextFactory, scriptId, "var unused = 1;", ContextType.CHANNEL_PREPROCESSOR);
+            String result = JavaScriptUtil.executePreprocessorScripts(task(contextFactory), messageWithRaw(), new HashMap<>(), null);
+            assertNull(result);
+        } finally {
+            CompiledScriptCache.getInstance().removeCompiledScript(scriptId);
+        }
+    }
+
+    @Test
+    public void preprocessorReturningStringYieldsThatString() throws Exception {
+        String scriptId = ScriptController.getScriptId(ScriptController.PREPROCESSOR_SCRIPT_KEY, CHANNEL_ID);
+        MirthContextFactory contextFactory = contextFactory();
+        try {
+            JavaScriptUtil.compileAndAddScript(CHANNEL_ID, contextFactory, scriptId, "return 'processed';", ContextType.CHANNEL_PREPROCESSOR);
+            String result = JavaScriptUtil.executePreprocessorScripts(task(contextFactory), messageWithRaw(), new HashMap<>(), null);
+            assertEquals("processed", result);
+        } finally {
+            CompiledScriptCache.getInstance().removeCompiledScript(scriptId);
+        }
+    }
+}

server/src/test/resources/mirth.properties

@@ -0,0 +1,114 @@
+# Mirth Connect configuration file
+
+# directories
+dir.appdata = appdata
+dir.tempdata = ${dir.appdata}/temp
+
+# ports
+http.port = 8080
+https.port = 8443
+
+# password requirements
+password.minlength = 0
+password.minupper = 0
+password.minlower = 0
+password.minnumeric = 0
+password.minspecial = 0
+password.retrylimit = 0
+password.lockoutperiod = 0
+password.expiration = 0
+password.graceperiod = 0
+password.reuseperiod = 0
+password.reuselimit = 0
+
+# Only used for migration purposes, do not modify
+version = 4.6.0
+
+# keystore
+keystore.path = ${dir.appdata}/keystore.jks
+keystore.storepass = 81uWxplDtB
+keystore.keypass = 81uWxplDtB
+keystore.type = JCEKS
+
+# server
+http.contextpath = /
+server.url = 
+
+http.host = 0.0.0.0
+https.host = 0.0.0.0
+
+https.client.protocols = TLSv1.3,TLSv1.2
+https.server.protocols = TLSv1.3,TLSv1.2,SSLv2Hello
+https.ciphersuites = TLS_CHACHA20_POLY1305_SHA256,TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305_SHA256,TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305_SHA256,TLS_DHE_RSA_WITH_CHACHA20_POLY1305_SHA256,TLS_AES_256_GCM_SHA384,TLS_AES_128_GCM_SHA256,TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384,TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384,TLS_RSA_WITH_AES_256_GCM_SHA384,TLS_ECDH_ECDSA_WITH_AES_256_GCM_SHA384,TLS_ECDH_RSA_WITH_AES_256_GCM_SHA384,TLS_DHE_RSA_WITH_AES_256_GCM_SHA384,TLS_DHE_DSS_WITH_AES_256_GCM_SHA384,TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256,TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256,TLS_RSA_WITH_AES_128_GCM_SHA256,TLS_ECDH_ECDSA_WITH_AES_128_GCM_SHA256,TLS_ECDH_RSA_WITH_AES_128_GCM_SHA256,TLS_DHE_RSA_WITH_AES_128_GCM_SHA256,TLS_DHE_DSS_WITH_AES_128_GCM_SHA256,TLS_EMPTY_RENEGOTIATION_INFO_SCSV
+https.ephemeraldhkeysize = 2048
+
+# If set to true, the Connect REST API will require all incoming requests to contain an "X-Requested-With" header.
+# This protects against Cross-Site Request Forgery (CSRF) security vulnerabilities.
+server.api.require-requested-with = true
+
+# CORS headers
+server.api.accesscontrolalloworigin = *
+server.api.accesscontrolallowcredentials = false
+server.api.accesscontrolallowmethods = GET, POST, DELETE, PUT
+server.api.accesscontrolallowheaders = Content-Type
+server.api.accesscontrolexposeheaders = 
+server.api.accesscontrolmaxage = 
+
+# Determines whether or not channels are deployed on server startup.
+server.startupdeploy = true
+
+# Determines whether libraries in the custom-lib directory will be included on the server classpath.
+# To reduce potential classpath conflicts you should create Resources and use them on specific channels/connectors instead, and then set this value to false.
+server.includecustomlib = false
+
+# administrator
+administrator.maxheapsize = 512m
+
+# properties file that will store the configuration map and be loaded during server startup
+configurationmap.path = ${dir.appdata}/configuration.properties
+
+# The language version for the Rhino JavaScript engine (supported values: 1.0, 1.1, ..., 1.8, es6).
+rhino.languageversion = es6
+
+# options: derby, mysql, postgres, oracle, sqlserver
+database = derby
+
+# examples:
+#   Derby                       jdbc:derby:${dir.appdata}/mirthdb;create=true
+#   PostgreSQL                  jdbc:postgresql://localhost:5432/mirthdb
+#   MySQL                       jdbc:mysql://localhost:3306/mirthdb
+#   Oracle                      jdbc:oracle:thin:@localhost:1521:DB
+#   SQL Server/Sybase (jTDS)    jdbc:jtds:sqlserver://localhost:1433/mirthdb
+#   Microsoft SQL Server        jdbc:sqlserver://localhost:1433;databaseName=mirthdb
+#   If you are using the Microsoft SQL Server driver, please also specify database.driver below 
+database.url = jdbc:derby:${dir.appdata}/mirthdb;create=true
+
+# If using a custom or non-default driver, specify it here.
+# example:
+# Microsoft SQL server: database.driver = com.microsoft.sqlserver.jdbc.SQLServerDriver
+# (Note: the jTDS driver is used by default for sqlserver)
+#database.driver =
+
+# Maximum number of connections allowed for the main read/write connection pool
+database.max-connections = 20
+# Maximum number of connections allowed for the read-only connection pool
+database-readonly.max-connections = 20
+
+# database credentials
+database.username = 
+database.password = 
+
+#On startup, Maximum number of retries to establish database connections in case of failure
+database.connection.maxretry = 2
+
+#On startup, Maximum wait time in milliseconds for retry to establish database connections in case of failure
+database.connection.retrywaitinmilliseconds = 10000
+
+# If true, various read-only statements are separated into their own connection pool.
+# By default the read-only pool will use the same connection information as the master pool,
+# but you can change this with the "database-readonly" options. For example, to point the
+# read-only pool to a different JDBC URL:
+#
+# database-readonly.url = jdbc:...
+# 
+database.enable-read-write-split = true