From 54ab1fcb9f1799bea7d20c616157e8b4199a77dc Mon Sep 17 00:00:00 2001 From: Pandipipas Date: Sun, 24 May 2026 22:13:04 +0200 Subject: [PATCH] feat: Enhance NodeCG process management and add IPC security tests --- docs/refactor/PHASE_2_SUMMARY.md | 79 +++++++++++ src/main/nodecg/process-manager.ts | 140 +++++++++++++------- src/tests/application-controller.test.ts | 4 +- src/tests/electron-security-surface.test.ts | 48 +++++++ src/tests/process-manager.test.ts | 42 ++++++ 5 files changed, 260 insertions(+), 53 deletions(-) create mode 100644 docs/refactor/PHASE_2_SUMMARY.md create mode 100644 src/tests/electron-security-surface.test.ts diff --git a/docs/refactor/PHASE_2_SUMMARY.md b/docs/refactor/PHASE_2_SUMMARY.md new file mode 100644 index 0000000..8f25cc4 --- /dev/null +++ b/docs/refactor/PHASE_2_SUMMARY.md @@ -0,0 +1,79 @@ +# Phase 2 Summary + +## Scope + +Executed the IPC and process-management phase only. + +Documentation used as source of truth: + +- `docs/refactor/ARCHITECTURE_AUDIT.md` +- `docs/refactor/ARCHITECTURE_RULES.md` +- `docs/refactor/TARGET_ARCHITECTURE.md` +- `docs/refactor/MIGRATION_PLAN.md` +- `docs/refactor/SESSION_HANDOFF.md` + +## IPC And Preload Decision + +No IPC or preload layer was added. + +This is intentional. The current architecture defines a zero-surface IPC model as the secure target because there is no custom renderer and no product requirement for desktop APIs to cross into web content. + +To make that decision enforceable, a regression test now scans `src/main` and fails if main-process source introduces: + +- `ipcMain` +- `ipcRenderer` +- `contextBridge` +- `preload` + +## Process Management Changes + +- Narrowed `NodecgProcessManager` so `startNodecgProcess` no longer returns the raw `ChildProcess`. +- Removed the public internal `getProcess` escape hatch from `NodecgProcessManager`. +- Added explicit NodeCG process states: + - `idle` + - `starting` + - `running` + - `stopping` + - `stopped` + - `failed` +- Added `getState` as the narrow observable process-management API. +- Made NodeCG startup idempotent while an async startup is already in progress. +- Prevented new startup while process shutdown is in progress. +- Preserved process-tree termination through `platform-process-killer.ts`. +- Preserved `ELECTRON_RUN_AS_NODE`, `shell: false`, `windowsHide: true`, and detached POSIX process-group behavior. + +## Security Notes + +- No raw Electron IPC APIs are imported in production source. +- No preload script is configured or exposed. +- No renderer/main business logic boundary was added. +- No filesystem, process, shell, or update primitives were exposed to web content. +- BrowserWindow security settings from Phase 1 remain unchanged. + +## Verification + +Commands run successfully: + +```text +npm run typecheck +npm test +npm run lint +``` + +Current test result: + +```text +55 tests passing +``` + +Additional sanity search: + +```text +rg -n "ipcMain|ipcRenderer|contextBridge|preload|nodeIntegration:\s*true|webSecurity:\s*false|\bany\b" src/main src/tests +``` + +Result: + +- No production IPC or preload surface exists. +- No unsafe Electron settings were introduced. +- Remaining IPC/preload string matches are limited to the regression test that guards the zero-surface policy. diff --git a/src/main/nodecg/process-manager.ts b/src/main/nodecg/process-manager.ts index 022823c..5f6b879 100644 --- a/src/main/nodecg/process-manager.ts +++ b/src/main/nodecg/process-manager.ts @@ -32,12 +32,14 @@ type NodecgProcessManagerDeps = { }; export type NodecgProcessManager = { - startNodecgProcess: () => Promise; + startNodecgProcess: () => Promise; waitForNodecgReady: (startTime: number) => Promise; stopNodecgProcessGracefully: () => Promise; - getProcess: () => ChildProcess | null; + getState: () => NodecgProcessState; }; +export type NodecgProcessState = "idle" | "starting" | "running" | "stopping" | "stopped" | "failed"; + export function createNodecgProcessManager({ isDev, nodecgRootPath, @@ -49,64 +51,97 @@ export function createNodecgProcessManager({ const resolvedDeps = resolveDeps(deps); let nodecgProcess: ChildProcess | null = null; + let nodecgState: NodecgProcessState = "idle"; + let startNodecgPromise: Promise | null = null; let stopNodecgPromise: Promise | null = null; let lastExit: { code: number | null; signal: NodeJS.Signals | null } | null = null; let lastStderrLine: string | null = null; - const startNodecgProcess = async (): Promise => { - // Fail fast with actionable errors before spawning child processes. - validateNodecgInstall( - nodecgRootPath, - appConfig.bundleName, - resolvedDeps.pathExists, - resolvedDeps.hasReadWriteAccess, - ); - - const portAsNumber = Number.parseInt(appConfig.nodecgPort, 10); - const isPortAvailable = await resolvedDeps.probePortAvailable(portAsNumber); - if (!isPortAvailable) { - throw new Error( - `Port ${appConfig.nodecgPort} is already in use. Stop the process using it or set NODECG_PORT before starting.`, - ); + const startNodecgProcess = (): Promise => { + if (nodecgProcess && nodecgState === "running") { + return Promise.resolve(); } - const indexPath = path.join(nodecgRootPath, "index.js"); - const child = resolvedDeps.spawnProcess(resolvedDeps.execPath, [indexPath], { - cwd: nodecgRootPath, - env: { - ...resolvedDeps.env, - NODE_ENV: isDev ? "development" : "production", - NODECG_PORT: appConfig.nodecgPort, - ELECTRON_RUN_AS_NODE: "1", - }, - stdio: ["ignore", "pipe", "pipe"], - detached: resolvedDeps.platform !== "win32", - shell: false, - windowsHide: true, - }); + if (startNodecgPromise) { + return startNodecgPromise; + } - child.stdout?.on("data", (chunk) => { - resolvedDeps.stdoutWrite(String(chunk)); - }); + if (nodecgState === "stopping") { + return Promise.reject(new Error("Cannot start NodeCG while shutdown is in progress.")); + } - child.stderr?.on("data", (chunk) => { - const line = String(chunk); - lastStderrLine = line.trim().length > 0 ? line.trim() : lastStderrLine; - resolvedDeps.stderrWrite(line); - }); + nodecgState = "starting"; + startNodecgPromise = (async () => { + // Fail fast with actionable errors before spawning child processes. + validateNodecgInstall( + nodecgRootPath, + appConfig.bundleName, + resolvedDeps.pathExists, + resolvedDeps.hasReadWriteAccess, + ); - log(`NodeCG started with pid=${child.pid} using ${NODE_RUNTIME_NAME}`); + const portAsNumber = Number.parseInt(appConfig.nodecgPort, 10); + const isPortAvailable = await resolvedDeps.probePortAvailable(portAsNumber); + if (!isPortAvailable) { + throw new Error( + `Port ${appConfig.nodecgPort} is already in use. Stop the process using it or set NODECG_PORT before starting.`, + ); + } - child.on("exit", (code, signal) => { - log(`NodeCG exited code=${code} signal=${signal ?? "none"}`); - lastExit = { code, signal }; - nodecgProcess = null; - }); + const indexPath = path.join(nodecgRootPath, "index.js"); + const child = resolvedDeps.spawnProcess(resolvedDeps.execPath, [indexPath], { + cwd: nodecgRootPath, + env: { + ...resolvedDeps.env, + NODE_ENV: isDev ? "development" : "production", + NODECG_PORT: appConfig.nodecgPort, + ELECTRON_RUN_AS_NODE: "1", + }, + stdio: ["ignore", "pipe", "pipe"], + detached: resolvedDeps.platform !== "win32", + shell: false, + windowsHide: true, + }); - lastExit = null; - lastStderrLine = null; - nodecgProcess = child; - return child; + child.stdout?.on("data", (chunk) => { + resolvedDeps.stdoutWrite(String(chunk)); + }); + + child.stderr?.on("data", (chunk) => { + const line = String(chunk); + lastStderrLine = line.trim().length > 0 ? line.trim() : lastStderrLine; + resolvedDeps.stderrWrite(line); + }); + + log(`NodeCG started with pid=${child.pid} using ${NODE_RUNTIME_NAME}`); + + child.on("exit", (code, signal) => { + log(`NodeCG exited code=${code} signal=${signal ?? "none"}`); + lastExit = { code, signal }; + + if (nodecgProcess === child) { + nodecgProcess = null; + } + + if (nodecgState !== "stopping") { + nodecgState = code === 0 ? "stopped" : "failed"; + } + }); + + lastExit = null; + lastStderrLine = null; + nodecgProcess = child; + nodecgState = "running"; + })() + .catch((error: unknown) => { + nodecgState = "failed"; + throw error; + }) + .finally(() => { + startNodecgPromise = null; + }); + + return startNodecgPromise; }; const waitForNodecgReady = async (startTime: number): Promise => { @@ -150,6 +185,7 @@ export function createNodecgProcessManager({ } if (!nodecgProcess || nodecgProcess.killed) { + nodecgState = "stopped"; return Promise.resolve(); } @@ -158,9 +194,12 @@ export function createNodecgProcessManager({ if (typeof pid !== "number") { log("NodeCG pid unavailable, skipping graceful stop"); + nodecgProcess = null; + nodecgState = "stopped"; return Promise.resolve(); } + nodecgState = "stopping"; log(`Stopping NodeCG pid=${pid}`); killProcessTree(pid, "SIGTERM", { platform: resolvedDeps.platform, @@ -183,6 +222,7 @@ export function createNodecgProcessManager({ nodecgProcess = null; } + nodecgState = "stopped"; stopNodecgPromise = null; resolve(); }; @@ -215,7 +255,7 @@ export function createNodecgProcessManager({ startNodecgProcess, waitForNodecgReady, stopNodecgProcessGracefully, - getProcess: () => nodecgProcess, + getState: () => nodecgState, }; } diff --git a/src/tests/application-controller.test.ts b/src/tests/application-controller.test.ts index 478f10b..f0fb07a 100644 --- a/src/tests/application-controller.test.ts +++ b/src/tests/application-controller.test.ts @@ -1,5 +1,4 @@ import assert from "node:assert/strict"; -import { EventEmitter } from "node:events"; import test from "node:test"; import { createApplicationController, ApplicationWindow } from "../main/app/application-controller"; @@ -68,7 +67,6 @@ function createMockManager(events: string[]): NodecgProcessManager { return { startNodecgProcess: async () => { events.push("start-nodecg"); - return new EventEmitter() as import("node:child_process").ChildProcess; }, waitForNodecgReady: async () => { events.push("wait-nodecg"); @@ -76,7 +74,7 @@ function createMockManager(events: string[]): NodecgProcessManager { stopNodecgProcessGracefully: async () => { events.push("stop-nodecg"); }, - getProcess: () => null, + getState: () => "running", }; } diff --git a/src/tests/electron-security-surface.test.ts b/src/tests/electron-security-surface.test.ts new file mode 100644 index 0000000..9bbcd81 --- /dev/null +++ b/src/tests/electron-security-surface.test.ts @@ -0,0 +1,48 @@ +import assert from "node:assert/strict"; +import fs from "node:fs"; +import path from "node:path"; +import test from "node:test"; + +const FORBIDDEN_MAIN_SURFACE_PATTERNS: Array<{ label: string; pattern: RegExp }> = [ + { label: "ipcMain", pattern: /\bipcMain\b/ }, + { label: "ipcRenderer", pattern: /\bipcRenderer\b/ }, + { label: "contextBridge", pattern: /\bcontextBridge\b/ }, + { label: "preload", pattern: /\bpreload\b/ }, +]; + +test("main source does not expose IPC or preload surface", () => { + const sourceRoot = path.join(process.cwd(), "src", "main"); + const failures: string[] = []; + + for (const filePath of readTypeScriptFiles(sourceRoot)) { + const contents = fs.readFileSync(filePath, "utf8"); + + for (const { label, pattern } of FORBIDDEN_MAIN_SURFACE_PATTERNS) { + if (pattern.test(contents)) { + failures.push(`${path.relative(process.cwd(), filePath)} contains ${label}`); + } + } + } + + assert.deepEqual(failures, []); +}); + +function readTypeScriptFiles(directoryPath: string): string[] { + const entries = fs.readdirSync(directoryPath, { withFileTypes: true }); + const files: string[] = []; + + for (const entry of entries) { + const entryPath = path.join(directoryPath, entry.name); + + if (entry.isDirectory()) { + files.push(...readTypeScriptFiles(entryPath)); + continue; + } + + if (entry.isFile() && entry.name.endsWith(".ts")) { + files.push(entryPath); + } + } + + return files; +} diff --git a/src/tests/process-manager.test.ts b/src/tests/process-manager.test.ts index 7aa0157..e279633 100644 --- a/src/tests/process-manager.test.ts +++ b/src/tests/process-manager.test.ts @@ -184,6 +184,48 @@ test("stopNodeCG reuses the same promise when invoked in parallel", async () => await firstStop; }); +test("startNodeCG reuses the same promise while startup is in progress", async () => { + const child = new MockChildProcess(2468); + let spawnCalls = 0; + let resolveProbe: (isAvailable: boolean) => void = () => { + throw new Error("probe promise was not created"); + }; + + const manager = createNodecgProcessManager({ + isDev: true, + nodecgRootPath: "/fake/nodecg", + nodecgBaseUrl: "http://127.0.0.1:9090", + appConfig: getBaseConfig(), + log: () => undefined, + deps: { + pathExists: () => true, + hasReadWriteAccess: () => true, + probePortAvailable: () => + new Promise((resolve) => { + resolveProbe = resolve; + }), + spawnProcess: () => { + spawnCalls += 1; + return child as unknown as import("node:child_process").ChildProcess; + }, + stdoutWrite: () => undefined, + stderrWrite: () => undefined, + }, + }); + + const firstStart = manager.startNodecgProcess(); + const secondStart = manager.startNodecgProcess(); + + assert.equal(firstStart, secondStart); + assert.equal(manager.getState(), "starting"); + + resolveProbe(true); + await firstStart; + + assert.equal(spawnCalls, 1); + assert.equal(manager.getState(), "running"); +}); + test("stopNodeCG normalizes negative timeout to zero", async () => { const child = new MockChildProcess(7777); const timeouts: number[] = [];