Validate the configuration map before persisting it (fixes #520)

server/src/main/java/com/mirth/connect/server/controllers/DefaultConfigurationController.java

@@ -922,9 +922,7 @@ public synchronized Map<String, ConfigurationProperty> getConfigurationPropertie
 
     @Override
     public synchronized void setConfigurationProperties(Map<String, ConfigurationProperty> map, boolean persist) throws ControllerException {
-        if (persist) {
-            saveConfigurationProperties(map);
-        }
+        validateConfigurationMap(map);
 
         Map<String, String> valueMap = new HashMap<String, String>();
         Map<String, String> commentMap = new HashMap<String, String>();
@@ -934,10 +932,42 @@ public synchronized void setConfigurationProperties(Map<String, ConfigurationPro
             commentMap.put(entry.getKey(), entry.getValue().getComment());
         }
 
+        // Persist only after the incoming map has been validated and fully processed, so an
+        // invalid map can never be written to the database and corrupt the configuration map on
+        // the next startup. See OpenIntegrationEngine/engine#520.
+        if (persist) {
+            saveConfigurationProperties(map);
+        }
+
         configurationMap = Collections.unmodifiableMap(valueMap);
         this.commentMap = Collections.unmodifiableMap(commentMap);
     }
 
+    /**
+     * Validates an incoming configuration map before it is persisted or applied. The configuration
+     * map API body readers deserialize into a raw map, so despite the declared generic types the
+     * map may be null or hold a non-string key or a non-{@link ConfigurationProperty} value;
+     * applying such a map fails partway through and, when persisting, only after the bad map has
+     * already been saved. See OpenIntegrationEngine/engine#520.
+     *
+     * @param map the configuration map to validate
+     * @throws ControllerException if the map is null or contains an invalid key or value
+     */
+    static void validateConfigurationMap(Map<String, ConfigurationProperty> map) throws ControllerException {
+        if (map == null) {
+            throw new ControllerException("Configuration map must not be null.");
+        }
+
+        for (Entry<?, ?> entry : map.entrySet()) {
+            if (!(entry.getKey() instanceof String)) {
+                throw new ControllerException("Configuration map contains a non-string key: " + entry.getKey() + ".");
+            }
+            if (!(entry.getValue() instanceof ConfigurationProperty)) {
+                throw new ControllerException("Configuration map contains an invalid value for key '" + entry.getKey() + "'.");
+            }
+        }
+    }
+
     @Override
     public void setStatus(int status) {
         this.status = status;

server/src/test/java/com/mirth/connect/server/controllers/DefaultConfigurationControllerTest.java

@@ -45,6 +45,9 @@
 import com.mirth.connect.client.core.ControllerException;
 import com.mirth.connect.donkey.testing.StaticOverride;
 import com.mirth.connect.model.DriverInfo;
+import java.util.HashMap;
+import java.util.Map;
+import com.mirth.connect.util.ConfigurationProperty;
 
 public class DefaultConfigurationControllerTest {
 
@@ -412,4 +415,50 @@ private String normalizeXml(String xml) throws Exception {
     		"	<driver class=\"org.sqlite.JDBC\" name=\"SQLite\" template=\"jdbc:sqlite:dbfile.db\" selectLimit=\"SELECT * FROM ? LIMIT 1\" />\n" + 
     		"</drivers>";
     // @formatter:on
+
+    @Test
+    public void setConfigurationProperties_nullMap_throwsControllerException() throws Exception {
+        DefaultConfigurationController configurationController = spy(new DefaultConfigurationController());
+        try {
+            configurationController.setConfigurationProperties(null, false);
+            fail("Expected ControllerException for a null configuration map");
+        } catch (ControllerException expected) {
+        }
+    }
+
+    @Test
+    public void validateConfigurationMap_nullValue_throwsControllerException() throws Exception {
+        Map<String, ConfigurationProperty> map = new HashMap<String, ConfigurationProperty>();
+        map.put("good", new ConfigurationProperty("value", "comment"));
+        map.put("bad", null);
+        try {
+            DefaultConfigurationController.validateConfigurationMap(map);
+            fail("Expected ControllerException for a null configuration property value");
+        } catch (ControllerException expected) {
+        }
+    }
+
+    @SuppressWarnings({ "unchecked", "rawtypes" })
+    @Test
+    public void validateConfigurationMap_nonStringKey_throwsControllerException() throws Exception {
+        Map map = new HashMap();
+        map.put(Integer.valueOf(1), new ConfigurationProperty("value", "comment"));
+        try {
+            DefaultConfigurationController.validateConfigurationMap(map);
+            fail("Expected ControllerException for a non-string key");
+        } catch (ControllerException expected) {
+        }
+    }
+
+    @Test
+    public void setConfigurationProperties_validMap_isApplied() throws Exception {
+        DefaultConfigurationController configurationController = spy(new DefaultConfigurationController());
+        doNothing().when(configurationController).loadDatabaseConfigPropsIfNecessary();
+
+        Map<String, ConfigurationProperty> map = new HashMap<String, ConfigurationProperty>();
+        map.put("alpha", new ConfigurationProperty("1", "first"));
+        configurationController.setConfigurationProperties(map, false);
+
+        assertEquals("1", configurationController.getConfigurationMap().get("alpha"));
+    }
 }