Share one merged configuration and fail closed on a bad drop-in file

Add ConfigurationController.getPropertiesConfiguration(), which returns
mirth.properties with the conf/mirth.properties.d drop-ins already
applied. Mirth and WebStartServlet now read that single shared copy
instead of each loading and merging the file themselves. The returned
configuration is owned by the controller and must be treated as
read-only; the controller writes to it during startup (for example when
it generates keystore passwords), and consumers only read it.

A drop-in file that cannot be read or parsed now fails closed. Previously
overlay() threw during controller initialization, where the exception was
swallowed and the server came up on the base configuration with the
drop-ins silently dropped; if a drop-in was hardening a setting, that
reverts to the weaker base value. Catch the failure, record it, and
refuse to start with a clear message. The launcher's early readers stay
lenient and skip a bad file with a warning, matching how they already
treat mirth.properties. A malformed \uXXXX escape is treated the same as
an unreadable file.

Add a regression test that a write to the merged configuration does not
leak back into the base file saved to mirth.properties.

Signed-off-by: Finnegan's Owner <44065187+pacmano1@users.noreply.github.com>

Author: pacmano1

server/src/main/java/com/mirth/connect/server/Mirth.java

@@ -59,7 +59,6 @@
 import com.mirth.connect.server.controllers.MigrationController;
 import com.mirth.connect.server.controllers.ScriptController;
 import com.mirth.connect.server.controllers.UsageController;
-import com.mirth.connect.server.extprops.DropInProperties;
 import com.mirth.connect.server.controllers.UserController;
 import com.mirth.connect.server.logging.JuliToLog4JService;
 import com.mirth.connect.server.logging.LogOutputStream;
@@ -183,18 +182,16 @@ public void run() {
      * @return true if the resources required by the server have been successfully loaded
      */
     public boolean initResources() {
-        InputStream mirthPropertiesStream = null;
-
-        try {
-            mirthPropertiesStream = ResourceUtil.getResourceStream(this.getClass(), "mirth.properties");
-            mirthProperties = PropertiesConfigurationUtil.create(mirthPropertiesStream);
-            mirthProperties = DropInProperties.overlay(mirthProperties, ResourceUtil.getMirthPropertiesDropInDirectory());
-        } catch (Exception e) {
-            // Reset so a failed drop-in overlay aborts startup instead of booting with a partial configuration
-            mirthProperties = PropertiesConfigurationUtil.create();
-            logger.error("could not load mirth.properties", e);
-        } finally {
-            IOUtils.closeQuietly(mirthPropertiesStream);
+        // Read the shared, drop-in-merged copy the configuration controller already loaded, rather
+        // than reloading and re-merging mirth.properties here.
+        mirthProperties = configurationController.getPropertiesConfiguration();
+
+        // Refuse to start if configuration could not be loaded cleanly (e.g. an unreadable
+        // mirth.properties.d drop-in file) rather than run with a silently reverted configuration.
+        String configurationLoadError = configurationController.getConfigurationLoadError();
+        if (configurationLoadError != null) {
+            logger.error("Refusing to start: " + configurationLoadError);
+            return false;
         }
 
         InputStream versionPropertiesStream = null;
@@ -225,7 +222,10 @@ public void startup() {
         configurationController.initializeSecuritySettings();
         configurationController.initializeDatabaseSettings();
 
-        // Refresh the in-memory config in case the configuration controller changed it
+        // Pull in any settings the controller changed during initialization (e.g. generated keystore
+        // passwords). With the default controller mirthProperties is already the controller's own
+        // configuration, so this is a no-op; a controller that hands out a separate configuration is
+        // refreshed here.
         configurationController.updatePropertiesConfiguration(mirthProperties);
 
         try {

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

@@ -9,6 +9,7 @@
 
 package com.mirth.connect.server.controllers;
 
+import java.io.InputStream;
 import java.util.Calendar;
 import java.util.List;
 import java.util.Locale;
@@ -21,6 +22,7 @@
 import com.mirth.commons.encryption.Digester;
 import com.mirth.commons.encryption.Encryptor;
 import com.mirth.connect.client.core.ControllerException;
+import com.mirth.connect.client.core.PropertiesConfigurationUtil;
 import com.mirth.connect.model.ChannelDependency;
 import com.mirth.connect.model.ChannelMetadata;
 import com.mirth.connect.model.ChannelTag;
@@ -32,6 +34,8 @@
 import com.mirth.connect.model.ServerConfiguration;
 import com.mirth.connect.model.ServerSettings;
 import com.mirth.connect.model.UpdateSettings;
+import com.mirth.connect.server.extprops.DropInProperties;
+import com.mirth.connect.server.util.ResourceUtil;
 import com.mirth.connect.util.ConfigurationProperty;
 import com.mirth.connect.util.ConnectionTestResponse;
 
@@ -79,6 +83,32 @@ public static ConfigurationController getInstance() {
      */
     public abstract void updatePropertiesConfiguration(PropertiesConfiguration config);
 
+    /**
+     * Returns the loaded mirth.properties configuration, with any conf/mirth.properties.d drop-in
+     * overrides applied. Server-side consumers should read from this shared copy rather than loading
+     * and merging the file themselves. The returned configuration is owned by the controller;
+     * consumers must treat it as read-only, as the controller writes to it during startup (for
+     * example, when it generates keystore passwords). This default loads on demand;
+     * DefaultConfigurationController overrides it to return the copy it already holds.
+     */
+    public PropertiesConfiguration getPropertiesConfiguration() {
+        try (InputStream is = ResourceUtil.getResourceStream(getClass(), "mirth.properties")) {
+            return DropInProperties.overlay(PropertiesConfigurationUtil.create(is), ResourceUtil.getMirthPropertiesDropInDirectory());
+        } catch (Exception e) {
+            return PropertiesConfigurationUtil.create();
+        }
+    }
+
+    /**
+     * Returns a message describing a fatal configuration problem that must prevent the server from
+     * starting, such as an unreadable conf/mirth.properties.d drop-in file, or null when
+     * configuration loaded cleanly. The default reports no error; DefaultConfigurationController
+     * overrides it.
+     */
+    public String getConfigurationLoadError() {
+        return null;
+    }
+
     /**
      * Returns the default encryptor.
      * 

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

@@ -118,8 +118,8 @@
 import com.mirth.connect.model.converters.ObjectXMLSerializer;
 import com.mirth.connect.plugins.directoryresource.DirectoryResourceProperties;
 import com.mirth.connect.server.ExtensionLoader;
-import com.mirth.connect.server.mybatis.KeyValuePair;
 import com.mirth.connect.server.extprops.DropInProperties;
+import com.mirth.connect.server.mybatis.KeyValuePair;
 import com.mirth.connect.server.tools.ClassPathResource;
 import com.mirth.connect.server.util.DatabaseUtil;
 import com.mirth.connect.server.util.PasswordRequirementsChecker;
@@ -170,6 +170,9 @@ public class DefaultConfigurationController extends ConfigurationController {
     // The file-backed configuration that saveMirthConfig() writes. Kept separate from mirthConfig
     // so that values from mirth.properties.d drop-in files are never baked into mirth.properties.
     protected static PropertiesConfiguration mirthFileConfig = mirthConfig;
+    // Set when a mirth.properties.d drop-in file cannot be read, so the server refuses to start
+    // rather than run with a silently reverted configuration. Null when configuration loaded cleanly.
+    private static volatile String configurationLoadError;
     private static EncryptionSettings encryptionConfig;
     private static DatabaseSettings databaseConfig;
     private static String apiBypassword;
@@ -244,7 +247,16 @@ public void initialize() {
                 logger.error("Unable to update mirth.properties version during migration.", e);
             }
 
-            mirthConfig = DropInProperties.overlay(mirthFileConfig, ResourceUtil.getMirthPropertiesDropInDirectory());
+            try {
+                mirthConfig = DropInProperties.overlay(mirthFileConfig, ResourceUtil.getMirthPropertiesDropInDirectory());
+            } catch (ConfigurationException e) {
+                // Fail closed: a broken drop-in file must stop startup rather than silently reverting
+                // to the base configuration, which could serve weaker settings than the administrator
+                // intended. mirthConfig stays at the base config and getConfigurationLoadError() reports
+                // the problem so the server refuses to start.
+                configurationLoadError = "Unable to load conf/mirth.properties.d drop-in configuration: " + e.getMessage();
+                logger.error(configurationLoadError, e);
+            }
 
             // load the server version
             versionPropertiesStream = ResourceUtil.getResourceStream(this.getClass(), "version.properties");
@@ -1454,6 +1466,16 @@ public void migrateKeystore() {
         }
     }
 
+    @Override
+    public PropertiesConfiguration getPropertiesConfiguration() {
+        return mirthConfig;
+    }
+
+    @Override
+    public String getConfigurationLoadError() {
+        return configurationLoadError;
+    }
+
     @Override
     public void updatePropertiesConfiguration(PropertiesConfiguration config) {
         config.copy(mirthConfig);

server/src/main/java/com/mirth/connect/server/extprops/DropInProperties.java

@@ -49,7 +49,8 @@ public static void load(Properties properties, File dropInDir, LoggerWrapper log
         for (File file : files) {
             try (InputStream is = new FileInputStream(file)) {
                 properties.load(is);
-            } catch (IOException e) {
+            } catch (IOException | IllegalArgumentException e) {
+                // IllegalArgumentException covers a malformed \\uXXXX escape in Properties.load()
                 logger.error("Unable to read drop-in properties file: " + file.getPath(), e);
             }
         }
@@ -58,7 +59,8 @@ public static void load(Properties properties, File dropInDir, LoggerWrapper log
     /**
      * Returns a configuration with any drop-in files applied on top of the given base
      * configuration. The base configuration is never modified; it is returned as-is when there are
-     * no drop-in files.
+     * no drop-in files. A drop-in file that cannot be read or parsed throws a ConfigurationException
+     * so that the caller can fail closed rather than start with a partially applied configuration.
      */
     public static PropertiesConfiguration overlay(PropertiesConfiguration base, File dropInDir) throws ConfigurationException {
         File[] files = listDropInFiles(dropInDir);
@@ -79,7 +81,10 @@ public static PropertiesConfiguration overlay(PropertiesConfiguration base, File
         for (File file : files) {
             try (InputStream is = new FileInputStream(file)) {
                 dropIns.load(is);
-            } catch (IOException e) {
+            } catch (IOException | IllegalArgumentException e) {
+                // IllegalArgumentException covers a malformed \\uXXXX escape in Properties.load().
+                // A file that cannot be read or parsed is fatal here so a broken drop-in fails startup
+                // rather than silently reverting to the base configuration.
                 throw new ConfigurationException("Unable to read drop-in properties file: " + file.getPath(), e);
             }
         }

server/src/main/java/com/mirth/connect/server/servlets/WebStartServlet.java

@@ -49,7 +49,6 @@
 import com.mirth.connect.server.controllers.ConfigurationController;
 import com.mirth.connect.server.controllers.ControllerFactory;
 import com.mirth.connect.server.controllers.ExtensionController;
-import com.mirth.connect.server.extprops.DropInProperties;
 import com.mirth.connect.server.tools.ClassPathResource;
 import com.mirth.connect.server.util.ResourceUtil;
 import com.mirth.connect.util.MirthSSLUtil;
@@ -360,17 +359,8 @@ private String getDigest(File directory, String filePath) throws Exception {
     }
 
     protected PropertiesConfiguration getMirthProperties() throws FileNotFoundException, ConfigurationException {
-        PropertiesConfiguration mirthProperties = PropertiesConfigurationUtil.create();
-        
-        InputStream mirthPropsIs = null;
-        try {
-            mirthPropsIs = ResourceUtil.getResourceStream(getClass(), "mirth.properties");
-            mirthProperties = PropertiesConfigurationUtil.create(mirthPropsIs);
-            mirthProperties = DropInProperties.overlay(mirthProperties, ResourceUtil.getMirthPropertiesDropInDirectory());
-        } finally {
-            ResourceUtil.closeResourceQuietly(mirthPropsIs);
-        }
-        return mirthProperties;
+        // Read the shared, drop-in-merged copy rather than reloading the file on every request.
+        return configurationController.getPropertiesConfiguration();
     }
 
     private String getContextPathProp(PropertiesConfiguration mirthProperties) {

server/src/test/java/com/mirth/connect/server/extprops/DropInPropertiesTest.java

@@ -60,6 +60,26 @@ public void overlayOverridesAndAddsWithoutModifyingBase() throws Exception {
         assertFalse(base.containsKey("c"));
     }
 
+    @Test
+    public void overlayResultIsIndependentOfBase() throws Exception {
+        PropertiesConfiguration base = new PropertiesConfiguration();
+        base.setProperty("a", "base");
+
+        File dir = tempFolder.newFolder();
+        writeFile(dir, "10-first.properties", "b = dropin\n");
+
+        PropertiesConfiguration merged = DropInProperties.overlay(base, dir);
+
+        // Writing to the merged result (as the controller does when it re-encrypts a password or
+        // generates a keystore password) must not leak back into the base configuration that
+        // saveMirthConfig() writes out to mirth.properties.
+        merged.setProperty("a", "changed");
+        merged.setProperty("c", "new");
+
+        assertEquals("base", base.getString("a"));
+        assertFalse(base.containsKey("c"));
+    }
+
     @Test
     public void overlayKeepsCommaValuesWhole() throws Exception {
         PropertiesConfiguration base = new PropertiesConfiguration();