From ca7b752025962441196e148f8c1bc04b90117979 Mon Sep 17 00:00:00 2001 From: Kristjan ESPERANTO <35647502+KristjanESPERANTO@users.noreply.github.com> Date: Wed, 10 Jun 2026 19:38:24 +0200 Subject: [PATCH] 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 | 2 ++ js/node_helper.js | 25 ++++++++++++++++- js/server_functions.js | 28 +++++++++++++------ tests/unit/functions/server_functions_spec.js | 27 ++++++++++++++++++ 4 files changed, 72 insertions(+), 10 deletions(-) diff --git a/js/app.js b/js/app.js index af831099..c79d40e4 100644 --- a/js/app.js +++ b/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); diff --git a/js/node_helper.js b/js/node_helper.js index 833c90aa..1883bd6b 100644 --- a/js/node_helper.js +++ b/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} 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); diff --git a/js/server_functions.js b/js/server_functions.js index 2d55bc3b..55f9b81c 100644 --- a/js/server_functions.js +++ b/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} [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; + }); } /** diff --git a/tests/unit/functions/server_functions_spec.js b/tests/unit/functions/server_functions_spec.js index 8c38cf9f..cd43eaf2 100644 --- a/tests/unit/functions/server_functions_spec.js +++ b/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;