Validate configuration map entries before saving

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

@@ -921,6 +921,24 @@ public synchronized Map<String, ConfigurationProperty> getConfigurationPropertie
 
     @Override
     public synchronized void setConfigurationProperties(Map<String, ConfigurationProperty> map, boolean persist) throws ControllerException {
+        if (map == null) {
+            throw new ControllerException("The configuration map cannot be null.");
+        }
+
+        /*
+         * The map may have been deserialized from a request body, which doesn't enforce its generic
+         * types. Check the raw entries so a bad map is rejected before it is persisted.
+         */
+        for (Entry<?, ?> entry : ((Map<?, ?>) map).entrySet()) {
+            if (!(entry.getKey() instanceof String)) {
+                throw new ControllerException("Configuration map keys must be strings, but found " + typeName(entry.getKey()) + ".");
+            }
+
+            if (!(entry.getValue() instanceof ConfigurationProperty)) {
+                throw new ControllerException("The configuration map entry \"" + entry.getKey() + "\" must be a ConfigurationProperty, but found " + typeName(entry.getValue()) + ".");
+            }
+        }
+
         if (persist) {
             saveConfigurationProperties(map);
         }
@@ -937,6 +955,10 @@ public synchronized void setConfigurationProperties(Map<String, ConfigurationPro
         this.commentMap = Collections.unmodifiableMap(commentMap);
     }
 
+    private static String typeName(Object object) {
+        return object == null ? "null" : object.getClass().getName();
+    }
+
     @Override
     public synchronized boolean setConfigurationProperty(String key, ConfigurationProperty property, String expectedVersion) throws ControllerException {
         Map<String, ConfigurationProperty> properties = getConfigurationProperties();

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

@@ -310,6 +310,24 @@ public void setConfigurationProperty_KeepsOtherEntries() throws Exception {
         assertEquals("second", saved.get("B").getComment());
     }
 
+    @Test
+    public void setConfigurationProperty_Exception_NullProperty() throws Exception {
+        AtomicReference<String> database = new AtomicReference<String>(INITIAL_CONFIGURATION_MAP_XML);
+        DefaultConfigurationController configurationController = databaseBackedConfigurationController(database);
+        Map<String, String> before = configurationController.getConfigurationMap();
+        String savedBefore = database.get();
+
+        try {
+            configurationController.setConfigurationProperty("B", null, null);
+            fail("Exception should have been thrown");
+        } catch (ControllerException e) {
+            // Expected
+        }
+
+        assertEquals(savedBefore, database.get());
+        assertEquals(before, configurationController.getConfigurationMap());
+    }
+
     @Test
     public void setConfigurationProperty_ExpectedVersionMatches() throws Exception {
         AtomicReference<String> database = new AtomicReference<String>(INITIAL_CONFIGURATION_MAP_XML);
@@ -358,6 +376,44 @@ public void setConfigurationProperty_ExpectedMissing_KeyMissing() throws Excepti
         assertEquals(expected, configurationController.getConfigurationMap());
     }
 
+    @Test
+    public void setConfigurationProperties_Exception_NullEntry() throws Exception {
+        assertInvalidEntryRejected("B", null);
+    }
+
+    @Test
+    public void setConfigurationProperties_Exception_NullKey() throws Exception {
+        assertInvalidEntryRejected(null, new ConfigurationProperty("2", "second"));
+    }
+
+    @Test
+    public void setConfigurationProperties_Exception_NonStringKey() throws Exception {
+        assertInvalidEntryRejected(5, new ConfigurationProperty("2", "second"));
+    }
+
+    @Test
+    public void setConfigurationProperties_Exception_NonConfigurationPropertyValue() throws Exception {
+        assertInvalidEntryRejected("B", "2");
+    }
+
+    @Test
+    public void setConfigurationProperties_Exception_NullMap() throws Exception {
+        AtomicReference<String> database = new AtomicReference<String>(INITIAL_CONFIGURATION_MAP_XML);
+        DefaultConfigurationController configurationController = databaseBackedConfigurationController(database);
+        Map<String, String> before = configurationController.getConfigurationMap();
+        String savedBefore = database.get();
+
+        try {
+            configurationController.setConfigurationProperties(null, true);
+            fail("Exception should have been thrown");
+        } catch (ControllerException e) {
+            // Expected
+        }
+
+        assertEquals(savedBefore, database.get());
+        assertEquals(before, configurationController.getConfigurationMap());
+    }
+
     private void assertConflict(String key, String expectedVersion) throws Exception {
         AtomicReference<String> database = new AtomicReference<String>(INITIAL_CONFIGURATION_MAP_XML);
         DefaultConfigurationController configurationController = databaseBackedConfigurationController(database);
@@ -370,6 +426,31 @@ private void assertConflict(String key, String expectedVersion) throws Exception
         assertEquals(before, configurationController.getConfigurationMap());
     }
 
+    /*
+     * Request bodies are deserialized without their generic types, so a map can hold entries that
+     * its declared type doesn't allow.
+     */
+    @SuppressWarnings({ "unchecked", "rawtypes" })
+    private void assertInvalidEntryRejected(Object key, Object value) throws Exception {
+        AtomicReference<String> database = new AtomicReference<String>(INITIAL_CONFIGURATION_MAP_XML);
+        DefaultConfigurationController configurationController = databaseBackedConfigurationController(database);
+        Map<String, String> before = configurationController.getConfigurationMap();
+        String savedBefore = database.get();
+
+        Map map = configurationController.getConfigurationProperties();
+        map.put(key, value);
+
+        try {
+            configurationController.setConfigurationProperties(map, true);
+            fail("Exception should have been thrown");
+        } catch (ControllerException e) {
+            // Expected
+        }
+
+        assertEquals(savedBefore, database.get());
+        assertEquals(before, configurationController.getConfigurationMap());
+    }
+
     private DefaultConfigurationController databaseBackedConfigurationController(AtomicReference<String> database) {
         DefaultConfigurationController.mirthConfig.setProperty(DefaultConfigurationController.CONFIGURATION_MAP_LOCATION, "database");