kenmirrors/MagicMirror
1
0
Fork 0
mirror of https://github.com/MichMich/MagicMirror.git synced 2026-07-23 15:54:34 -07:00
Code Issues Projects Releases Wiki Activity

fix(updatenotification): harden git command execution + simplify checkUpdates (#4115)

Browse Source 
This fixes CodeQL alert
[#16](https://github.com/MagicMirrorOrg/MagicMirror/security/code-scanning/16)
by replacing shell-built git commands with `execFile` + `cwd` in
updatenotification’s `git_helper`.

It also includes a small cleanup in `checkUpdates()` (remove unnecessary
async/Promise wrapper) and updates the related unit tests.
This commit is contained in:
Kristjan ESPERANTO
2026-04-27 23:06:29 +02:00
committed by GitHub
parent 7e1286257c
commit 9dc4b7a88f
2 changed files with 104 additions and 106 deletions
Download Patch File Download Diff File Expand all files Collapse all files
defaultmodules/updatenotification/git_helper.js
+18 -22
View File
@@ -1,5 +1,5 @@
const util = require("node:util");
const exec = util.promisify(require("node:child_process").exec);
const execFile = util.promisify(require("node:child_process").execFile);
const fs = require("node:fs");
const path = require("node:path");
const Log = require("logger");
@@ -14,14 +14,14 @@ class GitHelper {
return new RegExp(`s*([a-z,0-9]+[.][.][a-z,0-9]+) ${branch}`, "g");
}
async execShell (command) {
const { stdout = "", stderr = "" } = await exec(command);
async execGit (moduleFolder, ...args) {
const { stdout = "", stderr = "" } = await execFile("git", args, { cwd: moduleFolder });
return { stdout, stderr };
}
async isGitRepo (moduleFolder) {
const { stderr } = await this.execShell(`cd ${moduleFolder} && git remote -v`);
const { stderr } = await this.execGit(moduleFolder, "remote", "-v");
if (stderr) {
Log.error(`Failed to fetch git data for ${moduleFolder}: ${stderr}`);
@@ -69,7 +69,7 @@ class GitHelper {
if (repo.module === "MagicMirror") {
// the hash is only needed for the mm repo
const { stderr, stdout } = await this.execShell(`cd ${repo.folder} && git rev-parse HEAD`);
const { stderr, stdout } = await this.execGit(repo.folder, "rev-parse", "HEAD");
if (stderr) {
Log.error(`Failed to get current commit hash for ${repo.module}: ${stderr}`);
@@ -78,7 +78,7 @@ class GitHelper {
gitInfo.hash = stdout;
}
const { stderr, stdout } = await this.execShell(`cd ${repo.folder} && git status -sb`);
const { stderr, stdout } = await this.execGit(repo.folder, "status", "-sb");
if (stderr) {
Log.error(`Failed to get git status for ${repo.module}: ${stderr}`);
@@ -123,7 +123,7 @@ class GitHelper {
return gitInfo;
}
const { stderr } = await this.execShell(`cd ${repo.folder} && git fetch -n --dry-run`);
const { stderr } = await this.execGit(repo.folder, "fetch", "-n", "--dry-run");
// example output:
// From https://github.com/MagicMirrorOrg/MagicMirror
@@ -140,7 +140,7 @@ class GitHelper {
// get behind with refs
try {
const { stdout } = await this.execShell(`cd ${repo.folder} && git rev-list --ancestry-path --count ${refDiff}`);
const { stdout } = await this.execGit(repo.folder, "rev-list", "--ancestry-path", "--count", refDiff);
gitInfo.behind = parseInt(stdout);
// for MagicMirror-Repo and "master" branch avoid getting notified when no tag is in refDiff
@@ -148,14 +148,14 @@ class GitHelper {
if (gitInfo.behind > 0 && gitInfo.module === "MagicMirror" && gitInfo.current === "master") {
let tagList = "";
try {
const { stdout } = await this.execShell(`cd ${repo.folder} && git ls-remote -q --tags --refs`);
const { stdout } = await this.execGit(repo.folder, "ls-remote", "-q", "--tags", "--refs");
tagList = stdout.trim();
} catch (err) {
Log.error(`Failed to get tag list for ${repo.module}: ${err}`);
}
// check if tag is between commits and only report behind > 0 if so
try {
const { stdout } = await this.execShell(`cd ${repo.folder} && git rev-list --ancestry-path ${refDiff}`);
const { stdout } = await this.execGit(repo.folder, "rev-list", "--ancestry-path", refDiff);
let cnt = 0;
for (const ref of stdout.trim().split("\n")) {
if (tagList.includes(ref)) cnt++; // tag found
@@ -193,19 +193,15 @@ class GitHelper {
return this.gitResultList;
}
async checkUpdates () {
var updates = [];
checkUpdates () {
const updates = [];
const allRepos = await this.gitResultList.map((module) => {
return new Promise((resolve) => {
if (module.behind > 0 && module.module !== "MagicMirror") {
Log.info(`Update found for module: ${module.module}`);
updates.push(module);
}
resolve(module);
});
});
await Promise.all(allRepos);
for (const moduleInfo of this.gitResultList) {
if (moduleInfo.behind > 0 && moduleInfo.module !== "MagicMirror") {
Log.info(`Update found for module: ${moduleInfo.module}`);
updates.push(moduleInfo);
}
}
return updates;
}
tests/unit/functions/updatenotification_spec.js
+86 -84
View File
@@ -4,10 +4,10 @@ import { vi, describe, beforeEach, afterEach, it, expect } from "vitest";
* Creates a fresh GitHelper instance with isolated mocks for each test run.
* @param {{ current: import("vitest").Mock | null }} fsStatSyncMockRef reference to the mocked fs.statSync.
* @param {{ current: { error: import("vitest").Mock; info: import("vitest").Mock } | null }} loggerMockRef reference to logger stubs.
* @param {{ current: import("vitest").MockInstance | null }} execShellSpyRef reference to the execShell spy.
* @param {{ current: import("vitest").MockInstance | null }} execGitSpyRef reference to the execGit spy.
* @returns {Promise<unknown>} resolved GitHelper instance.
*/
async function createGitHelper (fsStatSyncMockRef, loggerMockRef, execShellSpyRef) {
async function createGitHelper (fsStatSyncMockRef, loggerMockRef, execGitSpyRef) {
vi.resetModules();
fsStatSyncMockRef.current = vi.fn();
@@ -23,7 +23,7 @@ async function createGitHelper (fsStatSyncMockRef, loggerMockRef, execShellSpyRe
const gitHelperModule = await import(`../../../${defaults.defaultModulesDir}/updatenotification/git_helper.js`);
const GitHelper = gitHelperModule.default || gitHelperModule;
const instance = new GitHelper();
execShellSpyRef.current = vi.spyOn(instance, "execShell");
execGitSpyRef.current = vi.spyOn(instance, "execGit");
instance.__loggerMock = loggerMockRef.current;
return instance;
}
@@ -31,7 +31,7 @@ async function createGitHelper (fsStatSyncMockRef, loggerMockRef, execShellSpyRe
describe("Updatenotification", () => {
const fsStatSyncMockRef = { current: null };
const loggerMockRef = { current: null };
const execShellSpyRef = { current: null };
const execGitSpyRef = { current: null };
let gitHelper;
let gitRemoteOut;
@@ -43,10 +43,10 @@ describe("Updatenotification", () => {
let gitFetchErr;
let gitTagListOut;
const getExecutedCommands = () => execShellSpyRef.current.mock.calls.map(([command]) => command);
const getExecutedCommands = () => execGitSpyRef.current.mock.calls.map((call) => call.slice(1).join(" "));
beforeEach(async () => {
gitHelper = await createGitHelper(fsStatSyncMockRef, loggerMockRef, execShellSpyRef);
gitHelper = await createGitHelper(fsStatSyncMockRef, loggerMockRef, execGitSpyRef);
fsStatSyncMockRef.current.mockReturnValue({ isDirectory: () => true });
@@ -59,40 +59,42 @@ describe("Updatenotification", () => {
gitFetchErr = "";
gitTagListOut = "";
execShellSpyRef.current.mockImplementation((command) => {
if (command.includes("git remote -v")) {
execGitSpyRef.current.mockImplementation((_folder, ...args) => {
const command = args.join(" ");
if (command === "remote -v") {
return Promise.resolve({ stdout: gitRemoteOut, stderr: "" });
}
if (command.includes("git rev-parse HEAD")) {
if (command === "rev-parse HEAD") {
return Promise.resolve({ stdout: gitRevParseOut, stderr: "" });
}
if (command.includes("git status -sb")) {
if (command === "status -sb") {
return Promise.resolve({ stdout: gitStatusOut, stderr: "" });
}
if (command.includes("git fetch -n --dry-run")) {
if (command === "fetch -n --dry-run") {
return Promise.resolve({ stdout: gitFetchOut, stderr: gitFetchErr });
}
if (command.includes("git rev-list --ancestry-path --count")) {
if (command.startsWith("rev-list --ancestry-path --count ")) {
return Promise.resolve({ stdout: gitRevListCountOut, stderr: "" });
}
if (command.includes("git rev-list --ancestry-path")) {
if (command.startsWith("rev-list --ancestry-path ")) {
return Promise.resolve({ stdout: gitRevListOut, stderr: "" });
}
if (command.includes("git ls-remote -q --tags --refs")) {
if (command === "ls-remote -q --tags --refs") {
return Promise.resolve({ stdout: gitTagListOut, stderr: "" });
}
return Promise.resolve({ stdout: "", stderr: "" });
});
if (gitHelper.execShell !== execShellSpyRef.current) {
throw new Error("execShell spy not applied");
if (gitHelper.execGit !== execGitSpyRef.current) {
throw new Error("execGit spy not applied");
}
});
@@ -119,10 +121,10 @@ describe("Updatenotification", () => {
expect(repos[0]).toMatchSnapshot();
expect(getExecutedCommands()).toMatchInlineSnapshot(`
[
"cd mock-path && git rev-parse HEAD",
"cd mock-path && git status -sb",
"cd mock-path && git fetch -n --dry-run",
"cd mock-path && git rev-list --ancestry-path --count 60e0377..332e429 develop",
"rev-parse HEAD",
"status -sb",
"fetch -n --dry-run",
"rev-list --ancestry-path --count 60e0377..332e429 develop",
]
`);
});
@@ -134,20 +136,20 @@ describe("Updatenotification", () => {
expect(repos[0]).toMatchSnapshot();
expect(getExecutedCommands()).toMatchInlineSnapshot(`
[
"cd mock-path && git rev-parse HEAD",
"cd mock-path && git status -sb",
"rev-parse HEAD",
"status -sb",
]
`);
});
it("excludes repo if status can't be retrieved", async () => {
const errorMessage = "Failed to retrieve status";
execShellSpyRef.current.mockImplementationOnce(() => Promise.reject(new Error(errorMessage)));
execGitSpyRef.current.mockImplementationOnce(() => Promise.reject(new Error(errorMessage)));
expect(gitHelper.gitRepos).toHaveLength(1);
const repos = await gitHelper.getRepos();
expect(repos).toHaveLength(0);
expect(execShellSpyRef.current.mock.calls.length).toBeGreaterThan(0);
expect(execGitSpyRef.current.mock.calls.length).toBeGreaterThan(0);
});
});
@@ -169,12 +171,12 @@ describe("Updatenotification", () => {
expect(repos[0]).toMatchSnapshot();
expect(getExecutedCommands()).toMatchInlineSnapshot(`
[
"cd mock-path && git rev-parse HEAD",
"cd mock-path && git status -sb",
"cd mock-path && git fetch -n --dry-run",
"cd mock-path && git rev-list --ancestry-path --count 60e0377..332e429 master",
"cd mock-path && git ls-remote -q --tags --refs",
"cd mock-path && git rev-list --ancestry-path 60e0377..332e429 master",
"rev-parse HEAD",
"status -sb",
"fetch -n --dry-run",
"rev-list --ancestry-path --count 60e0377..332e429 master",
"ls-remote -q --tags --refs",
"rev-list --ancestry-path 60e0377..332e429 master",
]
`);
});
@@ -185,20 +187,20 @@ describe("Updatenotification", () => {
const repos = await gitHelper.getRepos();
expect(repos[0]).toMatchSnapshot();
expect(getExecutedCommands()).toMatchInlineSnapshot(`
[
"cd mock-path && git rev-parse HEAD",
"cd mock-path && git status -sb",
"cd mock-path && git fetch -n --dry-run",
"cd mock-path && git rev-list --ancestry-path --count 60e0377..332e429 master",
"cd mock-path && git ls-remote -q --tags --refs",
"cd mock-path && git rev-list --ancestry-path 60e0377..332e429 master",
]
`);
[
"rev-parse HEAD",
"status -sb",
"fetch -n --dry-run",
"rev-list --ancestry-path --count 60e0377..332e429 master",
"ls-remote -q --tags --refs",
"rev-list --ancestry-path 60e0377..332e429 master",
]
`);
});
it("excludes repo if status can't be retrieved", async () => {
const errorMessage = "Failed to retrieve status";
execShellSpyRef.current.mockImplementationOnce(() => Promise.reject(new Error(errorMessage)));
execGitSpyRef.current.mockImplementationOnce(() => Promise.reject(new Error(errorMessage)));
const repos = await gitHelper.getRepos();
expect(repos).toHaveLength(0);
@@ -224,15 +226,15 @@ describe("Updatenotification", () => {
const repos = await gitHelper.getRepos();
expect(repos[0]).toMatchSnapshot();
expect(getExecutedCommands()).toMatchInlineSnapshot(`
[
"cd mock-path && git rev-parse HEAD",
"cd mock-path && git status -sb",
"cd mock-path && git fetch -n --dry-run",
"cd mock-path && git rev-list --ancestry-path --count 60e0377..332e429 master",
"cd mock-path && git ls-remote -q --tags --refs",
"cd mock-path && git rev-list --ancestry-path 60e0377..332e429 master",
]
`);
[
"rev-parse HEAD",
"status -sb",
"fetch -n --dry-run",
"rev-list --ancestry-path --count 60e0377..332e429 master",
"ls-remote -q --tags --refs",
"rev-list --ancestry-path 60e0377..332e429 master",
]
`);
});
it("returns status information early if isBehindInStatus", async () => {
@@ -241,20 +243,20 @@ describe("Updatenotification", () => {
const repos = await gitHelper.getRepos();
expect(repos[0]).toMatchSnapshot();
expect(getExecutedCommands()).toMatchInlineSnapshot(`
[
"cd mock-path && git rev-parse HEAD",
"cd mock-path && git status -sb",
"cd mock-path && git fetch -n --dry-run",
"cd mock-path && git rev-list --ancestry-path --count 60e0377..332e429 master",
"cd mock-path && git ls-remote -q --tags --refs",
"cd mock-path && git rev-list --ancestry-path 60e0377..332e429 master",
]
`);
[
"rev-parse HEAD",
"status -sb",
"fetch -n --dry-run",
"rev-list --ancestry-path --count 60e0377..332e429 master",
"ls-remote -q --tags --refs",
"rev-list --ancestry-path 60e0377..332e429 master",
]
`);
});
it("excludes repo if status can't be retrieved", async () => {
const errorMessage = "Failed to retrieve status";
execShellSpyRef.current.mockImplementationOnce(() => Promise.reject(new Error(errorMessage)));
execGitSpyRef.current.mockImplementationOnce(() => Promise.reject(new Error(errorMessage)));
const repos = await gitHelper.getRepos();
expect(repos).toHaveLength(0);
@@ -280,15 +282,15 @@ describe("Updatenotification", () => {
const repos = await gitHelper.getRepos();
expect(repos[0]).toMatchSnapshot();
expect(getExecutedCommands()).toMatchInlineSnapshot(`
[
"cd mock-path && git rev-parse HEAD",
"cd mock-path && git status -sb",
"cd mock-path && git fetch -n --dry-run",
"cd mock-path && git rev-list --ancestry-path --count 60e0377..332e429 master",
"cd mock-path && git ls-remote -q --tags --refs",
"cd mock-path && git rev-list --ancestry-path 60e0377..332e429 master",
]
`);
[
"rev-parse HEAD",
"status -sb",
"fetch -n --dry-run",
"rev-list --ancestry-path --count 60e0377..332e429 master",
"ls-remote -q --tags --refs",
"rev-list --ancestry-path 60e0377..332e429 master",
]
`);
});
it("returns status information early if isBehindInStatus", async () => {
@@ -297,20 +299,20 @@ describe("Updatenotification", () => {
const repos = await gitHelper.getRepos();
expect(repos[0]).toMatchSnapshot();
expect(getExecutedCommands()).toMatchInlineSnapshot(`
[
"cd mock-path && git rev-parse HEAD",
"cd mock-path && git status -sb",
"cd mock-path && git fetch -n --dry-run",
"cd mock-path && git rev-list --ancestry-path --count 60e0377..332e429 master",
"cd mock-path && git ls-remote -q --tags --refs",
"cd mock-path && git rev-list --ancestry-path 60e0377..332e429 master",
]
`);
[
"rev-parse HEAD",
"status -sb",
"fetch -n --dry-run",
"rev-list --ancestry-path --count 60e0377..332e429 master",
"ls-remote -q --tags --refs",
"rev-list --ancestry-path 60e0377..332e429 master",
]
`);
});
it("excludes repo if status can't be retrieved", async () => {
const errorMessage = "Failed to retrieve status";
execShellSpyRef.current.mockImplementationOnce(() => Promise.reject(new Error(errorMessage)));
execGitSpyRef.current.mockImplementationOnce(() => Promise.reject(new Error(errorMessage)));
const repos = await gitHelper.getRepos();
expect(repos).toHaveLength(0);
@@ -334,12 +336,12 @@ describe("Updatenotification", () => {
const repos = await gitHelper.getRepos();
expect(repos[0]).toMatchSnapshot();
expect(getExecutedCommands()).toMatchInlineSnapshot(`
[
"cd mock-path && git status -sb",
"cd mock-path && git fetch -n --dry-run",
"cd mock-path && git rev-list --ancestry-path --count 19f7faf..9d83101 master",
]
`);
[
"status -sb",
"fetch -n --dry-run",
"rev-list --ancestry-path --count 19f7faf..9d83101 master",
]
`);
});
});
});