fix: Improve startup settings validation (#27988)

Author: Nerivec

lib/util/onboarding.ts

@@ -1,11 +1,10 @@
 import {existsSync, mkdirSync} from "node:fs";
 import {createServer} from "node:http";
 import {parse} from "node:querystring";
-
 import {findAllDevices} from "zigbee-herdsman/dist/adapter/adapterDiscovery";
-
 import data from "./data";
 import * as settings from "./settings";
+import {YAMLFileException} from "./yaml";
 
 type OnboardSettings = {
     mqtt_base_topic?: string;
@@ -518,22 +517,66 @@ async function startFailureServer(errors: string): Promise<void> {
     await new Promise((resolve) => server?.close(resolve));
 }
 
+async function onSettingsErrors(errors: string[]): Promise<void> {
+    let pErrors = "";
+
+    console.error("\n\n!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!");
+    console.error("            READ THIS CAREFULLY\n");
+    console.error("Refusing to start because configuration is not valid, found the following errors:");
+
+    for (const error of errors) {
+        console.error(`- ${error}`);
+
+        pErrors += `<p>- ${escapeHtml(error)}</p>`;
+    }
+
+    console.error("\nIf you don't know how to solve this, read https://www.zigbee2mqtt.io/guide/configuration");
+    console.error("\n!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!\n\n");
+
+    if (!process.env.Z2M_ONBOARD_NO_SERVER && !process.env.Z2M_ONBOARD_NO_FAILURE_PAGE) {
+        await startFailureServer(pErrors);
+    }
+}
+
 export async function onboard(): Promise<boolean> {
     if (!existsSync(data.getPath())) {
         mkdirSync(data.getPath(), {recursive: true});
     }
 
     const confExists = existsSync(data.joinPath("configuration.yaml"));
 
-    if (!confExists) {
-        settings.writeMinimalDefaults();
-    } else {
+    if (confExists) {
+        // initial caching, ensure file is valid yaml first
+        try {
+            settings.getPersistedSettings();
+        } catch (error) {
+            await onSettingsErrors(
+                error instanceof YAMLFileException
+                    ? [`Your configuration file: '${error.file}' is invalid (use https://jsonformatter.org/yaml-validator to find and fix the issue)`]
+                    : [`${error}`],
+            );
+
+            return false;
+        }
+
         // migrate first
         const {migrateIfNecessary} = await import("./settingsMigration.js");
 
         migrateIfNecessary();
+
+        // make sure existing settings are valid before applying envs
+        const errors = settings.validateNonRequired();
+
+        if (errors.length > 0) {
+            await onSettingsErrors(errors);
+
+            return false;
+        }
+
         // trigger initial writing of `ZIGBEE2MQTT_CONFIG_*` ENVs
         settings.write();
+    } else {
+        settings.writeMinimalDefaults();
     }
 
     // use `configuration.yaml` file to detect "brand new install"
@@ -553,24 +596,7 @@ export async function onboard(): Promise<boolean> {
     const errors = settings.validate();
 
     if (errors.length > 0) {
-        let pErrors = "";
-
-        console.error("\n\n!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!");
-        console.error("            READ THIS CAREFULLY\n");
-        console.error("Refusing to start because configuration is not valid, found the following errors:");
-
-        for (const error of errors) {
-            console.error(`- ${error}`);
-
-            pErrors += `<p>- ${escapeHtml(error)}</p>`;
-        }
-
-        console.error("\nIf you don't know how to solve this, read https://www.zigbee2mqtt.io/guide/configuration");
-        console.error("\n!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!\n\n");
-
-        if (!process.env.Z2M_ONBOARD_NO_SERVER && !process.env.Z2M_ONBOARD_NO_FAILURE_PAGE) {
-            await startFailureServer(pErrors);
-        }
+        await onSettingsErrors(errors);
 
         return false;
     }

lib/util/settings.ts

@@ -7,7 +7,7 @@ import objectAssignDeep from "object-assign-deep";
 import data from "./data";
 import schemaJson from "./settings.schema.json";
 import utils from "./utils";
-import yaml, {YAMLFileException} from "./yaml";
+import yaml from "./yaml";
 
 export {schemaJson};
 // When updating also update:
@@ -260,15 +260,7 @@ export function write(): void {
 }
 
 export function validate(): string[] {
-    try {
-        getPersistedSettings();
-    } catch (error) {
-        if (error instanceof YAMLFileException) {
-            return [`Your YAML file: '${error.file}' is invalid (use https://jsonformatter.org/yaml-validator to find and fix the issue)`];
-        }
-
-        return [`${error}`];
-    }
+    getPersistedSettings();
 
     if (!ajvSetting(_settings)) {
         // biome-ignore lint/style/noNonNullAssertion: When `ajvSetting()` return false it always has `errors`
@@ -321,6 +313,19 @@ export function validate(): string[] {
     return errors;
 }
 
+export function validateNonRequired(): string[] {
+    getPersistedSettings();
+
+    if (!ajvSetting(_settings)) {
+        // biome-ignore lint/style/noNonNullAssertion: When `ajvSetting()` return false it always has `errors`
+        const errors = ajvSetting.errors!.filter((e) => e.keyword !== "required");
+
+        return errors.map((v) => `${v.instancePath.substring(1)} ${v.message}`);
+    }
+
+    return [];
+}
+
 function read(): Partial<Settings> {
     const s = yaml.read(CONFIG_FILE_PATH) as Partial<Settings>;
 
@@ -478,12 +483,12 @@ export function apply(settings: Record<string, unknown>, throwOnError = true): b
     const newSettings = objectAssignDeep.noMutate(_settings, settings);
 
     utils.removeNullPropertiesFromObject(newSettings, NULLABLE_SETTINGS);
-    ajvSetting(newSettings);
 
-    if (throwOnError) {
-        const errors = ajvSetting.errors?.filter((e) => e.keyword !== "required");
+    if (!ajvSetting(newSettings) && throwOnError) {
+        // biome-ignore lint/style/noNonNullAssertion: When `ajvSetting()` return false it always has `errors`
+        const errors = ajvSetting.errors!.filter((e) => e.keyword !== "required");
 
-        if (errors?.length) {
+        if (errors.length) {
             const error = errors[0];
             throw new Error(`${error.instancePath.substring(1)} ${error.message}`);
         }

lib/util/yaml.ts

@@ -21,7 +21,7 @@ export class YAMLFileException extends YAMLException {
 function read(file: string): KeyValue {
     try {
         const result = yaml.load(fs.readFileSync(file, "utf8"));
-        assert(result instanceof Object);
+        assert(result instanceof Object, `The content of ${file} is expected to be an object`);
         return result as KeyValue;
     } catch (error) {
         if (error instanceof YAMLException) {

test/onboarding.test.ts

@@ -1,8 +1,8 @@
 // biome-ignore assist/source/organizeImports: import mocks first
 import * as data from "./mocks/data";
 
-import {rmSync} from "node:fs";
-
+import {rmSync, writeFileSync} from "node:fs";
+import {join} from "node:path";
 import type {IncomingMessage, OutgoingHttpHeader, OutgoingHttpHeaders, RequestListener, Server, ServerResponse} from "node:http";
 import type {findAllDevices} from "zigbee-herdsman/dist/adapter/adapterDiscovery";
 import {onboard} from "../lib/util/onboarding";
@@ -752,7 +752,13 @@ describe("Onboarding", () => {
     });
 
     it("handles validation failure", async () => {
-        settings.set(["serial", "adapter"], "emberz");
+        const reReadSpy = vi.spyOn(settings, "reRead");
+
+        // set after onboarding server is done to reach bottom code path
+        reReadSpy.mockImplementationOnce(() => {
+            settings.set(["serial", "adapter"], "emberz");
+            settings.reRead();
+        });
 
         let p;
         const getHtml = await new Promise<string>((resolve, reject) => {
@@ -769,6 +775,88 @@ describe("Onboarding", () => {
 
         await expect(p).resolves.toStrictEqual(false);
         expect(getHtml).toContain("adapter must be equal to one of the allowed values");
+
+        reReadSpy.mockRestore();
+    });
+
+    it("handles non-required validation failure before applying envs", async () => {
+        settings.set(["serial"], "/dev/ttyUSB0");
+
+        let p;
+        const getHtml = await new Promise<string>((resolve, reject) => {
+            mockHttpOnListen.mockImplementationOnce(async () => {
+                try {
+                    resolve(await runFailure());
+                } catch (error) {
+                    reject(error);
+                }
+            });
+
+            p = onboard();
+        });
+
+        await expect(p).resolves.toStrictEqual(false);
+        expect(getHtml).toContain("serial must be object");
+    });
+
+    it("handles invalid yaml file", async () => {
+        settings.testing.clear();
+
+        const configFile = join(data.mockDir, "configuration.yaml");
+
+        writeFileSync(
+            configFile,
+            `
+                good: 9
+                \t wrong
+        `,
+        );
+
+        let p;
+        const getHtml = await new Promise<string>((resolve, reject) => {
+            mockHttpOnListen.mockImplementationOnce(async () => {
+                try {
+                    resolve(await runFailure());
+                } catch (error) {
+                    reject(error);
+                }
+            });
+
+            p = onboard();
+        });
+
+        await expect(p).resolves.toStrictEqual(false);
+        expect(getHtml).toContain("Your configuration file");
+        expect(getHtml).toContain("is invalid");
+
+        data.removeConfiguration();
+    });
+
+    it("handles error while loading yaml file", async () => {
+        settings.testing.clear();
+
+        const configFile = join(data.mockDir, "configuration.yaml");
+
+        writeFileSync(configFile, "badfile");
+
+        let p;
+        const getHtml = await new Promise<string>((resolve, reject) => {
+            mockHttpOnListen.mockImplementationOnce(async () => {
+                try {
+                    resolve(await runFailure());
+                } catch (error) {
+                    reject(error);
+                }
+            });
+
+            p = onboard();
+        });
+
+        await expect(p).resolves.toStrictEqual(false);
+        expect(getHtml).toContain("AssertionError");
+        expect(getHtml).toContain("expected to be an object");
+
+        data.removeConfiguration();
     });
 
     it("handles creating data path", async () => {

test/settings.test.ts

@@ -847,25 +847,6 @@ describe("Settings", () => {
         expect(settings.get().blocklist).toStrictEqual(["0x123", "0x1234"]);
     });
 
-    it("Should throw error when yaml file is invalid", () => {
-        fs.writeFileSync(
-            configurationFile,
-            `
-             good: 9
-             \t wrong
-        `,
-        );
-
-        settings.testing.clear();
-        const error = `Your YAML file: '${configurationFile}' is invalid (use https://jsonformatter.org/yaml-validator to find and fix the issue)`;
-        expect(settings.validate()).toEqual(expect.arrayContaining([error]));
-    });
-
-    it("Should throw error when yaml file does not exist", () => {
-        settings.testing.clear();
-        expect(settings.validate()[0]).toContain("ENOENT: no such file or directory, open ");
-    });
-
     it("Configuration shouldnt be valid when invalid QOS value is used", () => {
         write(configurationFile, {
             ...minimalConfig,