fix(security): prevent unauthorized secret expansion in socket payloads (#4184)

This PR attempts to fix the unauthorized secret expansion vulnerability
reported in
[GHSA-q4gh-4ffp-5cg8](https://github.com/MagicMirrorOrg/MagicMirror/security/advisories/GHSA-q4gh-4ffp-5cg8).

Previously, if a module sent a payload through the socket containing any
`**SECRET_FOO**` placeholder, the server would unconditionally expand it
with the real environment variable. This meant a manipulated module
could theoretically extract secrets that belonged to other modules.

To prevent this, the expansion logic is now much stricter and scoped to
the individual module:
* In `app.js`, we now store a copy of the redacted config
(`global.configRedacted`) to keep track of which module uses which
secrets.
* In `node_helper.js`, before handling a socket notification, we build a
specific "allow-list" (`Set`) of secrets that are actually present in
the calling module's config.
* `replaceSecretPlaceholder` in `server_functions.js` was updated to
accept this `Set` and will now only expand placeholders that the module
is explicitly authorized to know. Unlisted placeholders are safely
ignored.

I also updated the unit tests to cover the new allow-list behavior.

Since this security stuff is tricky and gives me headaches all the time,
I've added more comments than usual.

I've tried several ways to make it a little simpler, but unfortunately,
I couldn't come up with anything easier than that. I'd appreciate it if
someone could take a critical look at the logic to make sure I didn't
miss anything!

js/app.js

@@ -187,6 +187,8 @@ function App () {
 		try {
 			const configObj = Utils.loadConfig();
 			global.config = configObj.fullConf;
+			// Keep a copy of the redacted config to later verify module secret permissions
+			global.configRedacted = configObj.redactedConf;
 			const config = global.config;
 			Utils.checkConfigFile(configObj);
 

js/node_helper.js

@@ -2,6 +2,26 @@ const express = require("express");
 const Log = require("logger");
 const { replaceSecretPlaceholder } = require("#server_functions");
 
+/**
+ * Determine which secrets a module is allowed to restore. A module may only
+ * restore the `**SECRET_***` placeholders that appear in its own config — the
+ * exact inverse of how the config is redacted before it is sent to the browser.
+ * @param {string} moduleName - Name of the module.
+ * @returns {Set<string>} The secret names the module may restore.
+ */
+function getAllowedSecrets (moduleName) {
+	const modules = global.configRedacted?.modules || [];
+	const moduleConfig = modules.find((m) => m.module === moduleName);
+	const allowed = new Set();
+	if (moduleConfig) {
+		// Stringify the config to easily find all expected **SECRET_*** placeholders
+		for (const [, secretName] of JSON.stringify(moduleConfig).matchAll(/\*\*(SECRET_[^*]+)\*\*/g)) {
+			allowed.add(secretName);
+		}
+	}
+	return allowed;
+}
+
 class NodeHelper {
 	init () {
 		Log.log("Initializing new module helper ...");
@@ -90,7 +110,10 @@ class NodeHelper {
 			socket.onAny((notification, payload) => {
 				if (config?.hideConfigSecrets && payload && typeof payload === "object") {
 					try {
-						const payloadStr = replaceSecretPlaceholder(JSON.stringify(payload));
+						// Calculate exactly which secrets this module is allowed to receive
+						const allowedSecrets = getAllowedSecrets(this.name);
+						// Expand only these safe, module-specific secrets in the payload
+						const payloadStr = replaceSecretPlaceholder(JSON.stringify(payload), allowedSecrets);
 						this.socketNotificationReceived(notification, JSON.parse(payloadStr));
 					} catch (e) {
 						Log.error("Error substituting variables in payload: ", e);

js/server_functions.js

@@ -17,21 +17,31 @@ function getStartup (req, res) {
 }
 
 /**
- * A method that replaces the secret placeholders `**SECRET_ABC**` with the environment variable SECRET_ABC
- * @param {string} input - the input string
- * @returns {string} the input with real variable content
+ * Replace `**SECRET_ABC**` placeholders with the value of `process.env.SECRET_ABC`.
+ *
+ * If `allowedSecrets` is given, only those secret names are restored and every
+ * other placeholder is left untouched. Without it, all secrets are restored
+ * (used by the CORS proxy, which only runs on the trusted server side).
+ * @param {string} input - String that may contain `**SECRET_***` placeholders.
+ * @param {Set<string>} [allowedSecrets] - Secret names that may be restored.
+ * @returns {string} The input with the allowed placeholders replaced.
  */
-function replaceSecretPlaceholder (input) {
-	if (global.config.cors !== "allowAll") {
-		return input.replaceAll(/\*\*(SECRET_[^*]+)\*\*/g, (match, group) => {
-			return process.env[group];
-		});
-	} else {
+function replaceSecretPlaceholder (input, allowedSecrets) {
+	if (global.config.cors === "allowAll") {
 		if (input.includes("**SECRET_")) {
 			Log.error("Replacing secrets doesn't work with CORS `allowAll`, you need to set `cors` to `disabled` or `allowWhitelist` in `config.js`");
 		}
 		return input;
 	}
+	return input.replaceAll(/\*\*(SECRET_[^*]+)\*\*/g, (placeholder, secretName) => {
+		// Block replacing secrets that are not explicitly allowed.
+		if (allowedSecrets && !allowedSecrets.has(secretName)) {
+			return placeholder;
+		}
+
+		// Load the real value from the environment. Fallback to placeholder if missing.
+		return process.env[secretName] || placeholder;
+	});
 }
 
 /**

tests/unit/functions/server_functions_spec.js

@@ -47,6 +47,33 @@ describe("server_functions tests", () => {
 		});
 	});
 
+	describe("The replaceSecretPlaceholder method with an allowedSecrets set", () => {
+		beforeEach(() => {
+			global.config = { cors: "allowWhitelist" };
+			process.env.SECRET_ALLOWED = "allowed-value";
+			process.env.SECRET_DENIED = "denied-value";
+		});
+
+		it("Restores only allowed secrets and keeps denied placeholders untouched", () => {
+			const teststring = "allowed=**SECRET_ALLOWED** denied=**SECRET_DENIED**";
+			const result = replaceSecretPlaceholder(teststring, new Set(["SECRET_ALLOWED"]));
+			expect(result).toBe("allowed=allowed-value denied=**SECRET_DENIED**");
+			expect(result).not.toContain("denied-value");
+		});
+
+		it("Does not restore any placeholder when the set is empty", () => {
+			const teststring = "value=**SECRET_ALLOWED**";
+			const result = replaceSecretPlaceholder(teststring, new Set());
+			expect(result).toBe(teststring);
+		});
+
+		it("Falls back to the placeholder if the allowed secret doesn't exist in environment", () => {
+			const teststring = "value=**SECRET_MISSING**";
+			const result = replaceSecretPlaceholder(teststring, new Set(["SECRET_MISSING"]));
+			expect(result).toBe(teststring);
+		});
+	});
+
 	describe("The cors method", () => {
 		let fetchSpy;
 		let fetchResponseHeadersGet;