feat: Enhance NodeCG process management and add IPC security tests

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.

src/main/nodecg/process-manager.ts

@@ -32,12 +32,14 @@ type NodecgProcessManagerDeps = {
 };
 
 export type NodecgProcessManager = {
-  startNodecgProcess: () => Promise<ChildProcess>;
+  startNodecgProcess: () => Promise<void>;
   waitForNodecgReady: (startTime: number) => Promise<void>;
   stopNodecgProcessGracefully: () => Promise<void>;
-  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<void> | null = null;
   let stopNodecgPromise: Promise<void> | null = null;
   let lastExit: { code: number | null; signal: NodeJS.Signals | null } | null = null;
   let lastStderrLine: string | null = null;
 
-  const startNodecgProcess = async (): Promise<ChildProcess> => {
-    // 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<void> => {
+    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<void> => {
@@ -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,
   };
 }
 

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",
   };
 }
 

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;
+}

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<boolean>((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[] = [];