Skip to content

Commit 9386c44

Browse files
fix: resolve CodeQL alerts #24 and #26 (#4145)
**[#24](https://github.com/MagicMirrorOrg/MagicMirror/security/code-scanning/24) – `js/class.js`** `fnTest` works by serialising a function to a string and checking if `"xyz"` appears in it - the function is never actually called. The bare `xyz;` is never executed, so CodeQL is right to flag it. `return xyz;` makes the intent clear. So this is purely a cosmetic change. **[#26](https://github.com/MagicMirrorOrg/MagicMirror/security/code-scanning/26) – `tests/e2e/helpers/global-setup.js`** CodeQL flagged `if (exec) exec;` as a useless expression - and it was right. But the real find was one level deeper. `startApplication` hardcoded `const port = 8080`, so `MM_PORT` was always overwritten before the app started. The test named "Set port 8100 on environment variable MM_PORT" was actually testing port 8080 the whole time - it just happened to pass anyway. Removed the dead `exec` parameter, made `startApplication` read `MM_PORT` from the environment, and fixed the test so it actually checks what it says it checks.
1 parent 7da9e5a commit 9386c44

3 files changed

Lines changed: 11 additions & 11 deletions

File tree

js/class.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@
1111
(function () {
1212
let initializing = false;
1313
const fnTest = (/xyz/).test(function () {
14-
xyz;
14+
return xyz;
1515
})
1616
? /\b_super\b/
1717
: /.*/;

tests/e2e/helpers/global-setup.js

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@ exports.getPage = () => {
8888
return page;
8989
};
9090

91-
exports.startApplication = async (configFilename, exec) => {
91+
exports.startApplication = async (configFilename) => {
9292
vi.resetModules();
9393

9494
// Clear Node's require cache for config and app files to prevent stale configs and middlewares
@@ -107,8 +107,8 @@ exports.startApplication = async (configFilename, exec) => {
107107
await exports.stopApplication();
108108
}
109109

110-
// Use fixed port 8080 (tests run sequentially, no conflicts)
111-
const port = 8080;
110+
// Use MM_PORT if preset by a test, otherwise default to 8080.
111+
const port = Number(process.env.MM_PORT) || 8080;
112112
global.testPort = port;
113113

114114
// Set config sample for use in test
@@ -121,12 +121,11 @@ exports.startApplication = async (configFilename, exec) => {
121121

122122
process.env.MM_CONFIG_FILE = configPath;
123123

124-
// Override port in config - MUST be set before app loads
124+
// Ensure MM_PORT is set before app loads
125125
process.env.MM_PORT = port.toString();
126126

127127
process.env.mmTestMode = "true";
128128
process.setMaxListeners(0);
129-
if (exec) exec;
130129
global.app = require(`${global.root_path}/js/app`);
131130

132131
return global.app.start();
@@ -143,6 +142,7 @@ exports.stopApplication = async (waitTime = 100) => {
143142
await global.app.stop();
144143
delete global.app;
145144
delete global.testPort;
145+
delete process.env.MM_PORT;
146146

147147
// Wait for any pending async operations to complete before closing DOM
148148
await new Promise((resolve) => setTimeout(resolve, waitTime));

tests/e2e/port_spec.js

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -11,24 +11,24 @@ describe("port directive configuration", () => {
1111
});
1212

1313
it("should return 200", async () => {
14-
const port = global.testPort || 8080;
15-
const res = await fetch(`http://localhost:${port}`);
14+
const res = await fetch(`http://localhost:${global.testPort}`);
1615
expect(res.status).toBe(200);
1716
});
1817
});
1918

2019
describe("Set port 8100 on environment variable MM_PORT", () => {
2120
beforeAll(async () => {
22-
await helpers.startApplication("tests/configs/port_8090.js", (process.env.MM_PORT = 8100));
21+
process.env.MM_PORT = "8100";
22+
await helpers.startApplication("tests/configs/port_8090.js");
2323
});
2424

2525
afterAll(async () => {
2626
await helpers.stopApplication();
2727
});
2828

2929
it("should return 200", async () => {
30-
const port = global.testPort || 8080;
31-
const res = await fetch(`http://localhost:${port}`);
30+
expect(global.testPort).toBe(8100);
31+
const res = await fetch(`http://localhost:${global.testPort}`);
3232
expect(res.status).toBe(200);
3333
});
3434
});

0 commit comments

Comments
 (0)