From d6a5128aa819a85e2a06e57307329d2b35938397 Mon Sep 17 00:00:00 2001
From: Koen Kanters <koenkanters94@gmail.com>
Date: Fri, 4 Apr 2025 21:57:11 +0200
Subject: [PATCH] fix: Fix settings being overwriting when env var is set to a
 ref (#26988)

---
 lib/util/settings.ts  | 15 ++++++------
 test/settings.test.ts | 56 +++++++++++++++++++++++++++++++++++--------
 2 files changed, 54 insertions(+), 17 deletions(-)

diff --git a/lib/util/settings.ts b/lib/util/settings.ts
index b93645c15..9efefc93d 100644
--- a/lib/util/settings.ts
+++ b/lib/util/settings.ts
@@ -181,24 +181,22 @@ export function write(): void {
     const settings = getPersistedSettings();
     const toWrite: KeyValue = objectAssignDeep({}, settings);
 
-    applyEnvironmentVariables(toWrite);
-
     // Read settings to check if we have to split devices/groups into separate file.
     const actual = yaml.read(CONFIG_FILE_PATH);
 
     // In case the setting is defined in a separate file (e.g. !secret network_key) update it there.
-    for (const path of [
+    for (const [ns, key] of [
         ['mqtt', 'server'],
         ['mqtt', 'user'],
         ['mqtt', 'password'],
         ['advanced', 'network_key'],
         ['frontend', 'auth_token'],
     ]) {
-        if (actual[path[0]] && actual[path[0]][path[1]]) {
-            const ref = parseValueRef(actual[path[0]][path[1]]);
+        if (actual[ns] && actual[ns][key]) {
+            const ref = parseValueRef(actual[ns][key]);
             if (ref) {
-                yaml.updateIfChanged(data.joinPath(ref.filename), ref.key, toWrite[path[0]][path[1]]);
-                toWrite[path[0]][path[1]] = actual[path[0]][path[1]];
+                yaml.updateIfChanged(data.joinPath(ref.filename), ref.key, toWrite[ns][key]);
+                toWrite[ns][key] = actual[ns][key];
             }
         }
     }
@@ -226,6 +224,9 @@ export function write(): void {
 
     writeDevicesOrGroups('devices');
     writeDevicesOrGroups('groups');
+
+    applyEnvironmentVariables(toWrite);
+
     yaml.writeIfChanged(CONFIG_FILE_PATH, toWrite);
 
     _settings = read();
diff --git a/test/settings.test.ts b/test/settings.test.ts
index a850155db..cf1071bb6 100644
--- a/test/settings.test.ts
+++ b/test/settings.test.ts
@@ -105,6 +105,8 @@ describe('Settings', () => {
     });
 
     it('Should apply environment variables as overrides', () => {
+        write(secretFile, {password: 'the-password'}, false);
+        process.env.ZIGBEE2MQTT_CONFIG_MQTT_PASSWORD = '!secret.yaml password';
         process.env.ZIGBEE2MQTT_CONFIG_SERIAL_DISABLE_LED = 'true';
         process.env.ZIGBEE2MQTT_CONFIG_ADVANCED_CHANNEL = '15';
         process.env.ZIGBEE2MQTT_CONFIG_ADVANCED_OUTPUT = 'attribute_and_json';
@@ -122,12 +124,6 @@ describe('Settings', () => {
             },
         };
 
-        write(configurationFile, {});
-        write(devicesFile, contentDevices);
-        expect(settings.write()); // trigger writing of ENVs
-        expect(settings.validate()).toStrictEqual([]);
-
-        const s = settings.get();
         // @ts-expect-error workaround
         const expected = objectAssignDeep.noMutate({}, settings.testing.defaults);
         expected.devices = {
@@ -144,14 +140,54 @@ describe('Settings', () => {
         expected.map_options.graphviz.colors.fill = {enddevice: '#ff0000', coordinator: '#00ff00', router: '#0000ff'};
         expected.mqtt.base_topic = 'testtopic';
         expected.mqtt.server = 'testserver';
+        expected.mqtt.password = 'the-password';
         expected.advanced.network_key = 'GENERATE';
 
-        expect(s).toStrictEqual(expected);
+        write(configurationFile, {mqtt: {password: 'config-password'}});
+        write(devicesFile, contentDevices);
 
-        settings.set(['advanced', 'channel'], 25);
+        const writeAndCheck = (): void => {
+            expect(settings.write()); // trigger writing of ENVs
+            expect(settings.validate()).toStrictEqual([]);
+            expect(settings.get()).toStrictEqual(expected);
 
-        expect(settings.get().advanced.channel).toStrictEqual(15);
-        expect(read(configurationFile)).toMatchObject({advanced: {channel: 15}});
+            settings.set(['advanced', 'channel'], 25);
+            expect(settings.get().advanced.channel).toStrictEqual(15);
+            expect(read(configurationFile)).toMatchObject({advanced: {channel: 15}});
+
+            expect(read(secretFile)).toMatchObject({password: 'the-password'});
+            expect(read(configurationFile)).toHaveProperty('mqtt.password', '!secret.yaml password');
+        };
+
+        // Write trice to ensure there are no side effects.
+        writeAndCheck();
+        writeAndCheck();
+        writeAndCheck();
+    });
+
+    it('Should write environment variables as overrides to configuration.yaml, not in the ref file', () => {
+        write(secretFile, {password: 'password-in-secret-file'}, false);
+        write(configurationFile, {mqtt: {password: '!secret password', server: 'server'}});
+        process.env.ZIGBEE2MQTT_CONFIG_MQTT_PASSWORD = 'password-in-env-var';
+
+        const writeAndCheck = (): void => {
+            expect(settings.write()); // trigger writing of ENVs
+            expect(settings.validate()).toStrictEqual([]);
+
+            const s = settings.get();
+            // @ts-expect-error workaround
+            const expected = objectAssignDeep.noMutate({groups: {}, devices: {}}, settings.testing.defaults);
+            expected.mqtt.password = 'password-in-env-var';
+            expected.mqtt.server = 'server';
+            expect(s).toStrictEqual(expected);
+            expect(read(secretFile)).toMatchObject({password: 'password-in-secret-file'});
+            expect(read(configurationFile)).toMatchObject({mqtt: {password: 'password-in-env-var', server: 'server'}});
+        };
+
+        // Write trice to ensure there are no side effects.
+        writeAndCheck();
+        writeAndCheck();
+        writeAndCheck();
     });
 
     it('Should add devices', () => {