diff --git a/packages/cli/lib/commands/start.ts b/packages/cli/lib/commands/start.ts index 6211bcd2a..eb8995246 100644 --- a/packages/cli/lib/commands/start.ts +++ b/packages/cli/lib/commands/start.ts @@ -7,11 +7,16 @@ import { PositionalArgs, StartCommandType } from "./types"; import { ArgumentsCamelCase } from "yargs"; const execSyncNpmStart = (port: number, options: ExecSyncOptions): void => { + const args = ['start']; if (port) { - Util.execSync(`npm start -- --port=${port}`, options); - return; + // Validate port is a number to prevent command injection + if (!Number.isInteger(port) || port < 0 || port > 65535) { + Util.error(`Invalid port number: ${port}`, "red"); + return; + } + args.push('--', `--port=${port}`); } - Util.execSync(`npm start`, options); + Util.spawnSync('npm', args, options); }; const command: StartCommandType = { diff --git a/packages/core/packages/PackageManager.ts b/packages/core/packages/PackageManager.ts index 955d555d7..64e795988 100644 --- a/packages/core/packages/PackageManager.ts +++ b/packages/core/packages/PackageManager.ts @@ -1,4 +1,4 @@ -import { exec } from "child_process"; +import { spawn } from "child_process"; import * as path from "path"; import { TemplateManager } from "../../cli/lib/TemplateManager"; import { Config, FS_TOKEN, IFileSystem, ProjectTemplate } from "../types"; @@ -149,10 +149,10 @@ export class PackageManager { public static addPackage(packageName: string, verbose: boolean = false): boolean { const managerCommand = this.getManager(); - const command = this.getInstallCommand(managerCommand, packageName); + const args = this.getInstallArgs(packageName); try { // tslint:disable-next-line:object-literal-sort-keys - Util.execSync(command, { stdio: "pipe", encoding: "utf8" }); + Util.spawnSync(managerCommand, args, { stdio: "pipe", encoding: "utf8" }); } catch (error) { Util.log(`Error installing package ${packageName} with ${managerCommand}`); if (verbose) { @@ -165,7 +165,7 @@ export class PackageManager { } public static async queuePackage(packageName: string, verbose = false) { - const command = this.getInstallCommand(this.getManager(), packageName).replace("--save", "--no-save"); + const args = this.getInstallArgs(packageName).map(arg => arg === '--save' ? '--no-save' : arg); const [packName, version] = packageName.split(/@(?=[^\/]+$)/); const packageJSON = this.getPackageJSON(); if (!packageJSON.dependencies) { @@ -190,13 +190,24 @@ export class PackageManager { // D.P. Concurrent install runs should be supported // https://github.com/npm/npm/issues/5948 // https://github.com/npm/npm/issues/2500 + const managerCommand = this.getManager(); const task = new Promise<{ packageName, error, stdout, stderr }>((resolve, reject) => { - const child = exec( - command, {}, - (error, stdout, stderr) => { - resolve({ packageName, error, stdout, stderr }); - } - ); + const child = spawn(managerCommand, args); + let stdout = ''; + let stderr = ''; + child.stdout?.on('data', (data) => { + stdout += data.toString(); + }); + child.stderr?.on('data', (data) => { + stderr += data.toString(); + }); + child.on('close', (code) => { + const error = code !== 0 ? new Error(`Process exited with code ${code}`) : null; + resolve({ packageName, error, stdout, stderr }); + }); + child.on('error', (err) => { + resolve({ packageName, error: err, stdout, stderr }); + }); }); task["packageName"] = packName; this.installQueue.push(task); @@ -281,6 +292,10 @@ export class PackageManager { } } + private static getInstallArgs(packageName: string): string[] { + return ['install', packageName, '--quiet', '--save']; + } + private static getManager(/*config:Config*/): string { //stub to potentially swap out managers return "npm"; diff --git a/spec/unit/packageManager-spec.ts b/spec/unit/packageManager-spec.ts index dde69811b..49c7287ea 100644 --- a/spec/unit/packageManager-spec.ts +++ b/spec/unit/packageManager-spec.ts @@ -246,15 +246,16 @@ describe("Unit - Package Manager", () => { spyOn(ProjectConfig, "localConfig").and.callFake(() => mockProjectConfig); spyOn(ProjectConfig, "setConfig"); spyOn(TestPackageManager, "addPackage").and.callThrough(); - spyOn(Util, "execSync"); + spyOn(Util, "spawnSync"); spyOn(Util, "log"); spyOn(TestPackageManager, "removePackage"); spyOn(TestPackageManager, "getPackageJSON").and.callFake(() => mockDeps); await TestPackageManager.ensureIgniteUISource(true, mockTemplateMgr, true); expect(TestPackageManager.addPackage).toHaveBeenCalledWith(`@infragistics/ignite-ui-full@"~20.1"`, true); - expect(Util.execSync).toHaveBeenCalledWith( - `npm install @infragistics/ignite-ui-full@"~20.1" --quiet --save`, + expect(Util.spawnSync).toHaveBeenCalledWith( + `npm`, + ['install', `@infragistics/ignite-ui-full@"~20.1"`, '--quiet', '--save'], jasmine.any(Object) ); expect(TestPackageManager.removePackage).toHaveBeenCalledWith("ignite-ui", true); @@ -264,8 +265,9 @@ describe("Unit - Package Manager", () => { mockTemplateMgr.generateConfig = mockProjectConfig; await TestPackageManager.ensureIgniteUISource(true, mockTemplateMgr, true); expect(TestPackageManager.addPackage).toHaveBeenCalledWith(`@infragistics/ignite-ui-full@"^17.1"`, true); - expect(Util.execSync).toHaveBeenCalledWith( - `npm install @infragistics/ignite-ui-full@"^17.1" --quiet --save`, + expect(Util.spawnSync).toHaveBeenCalledWith( + `npm`, + ['install', `@infragistics/ignite-ui-full@"^17.1"`, '--quiet', '--save'], jasmine.any(Object) ); @@ -274,8 +276,9 @@ describe("Unit - Package Manager", () => { mockTemplateMgr.generateConfig = mockProjectConfig; await TestPackageManager.ensureIgniteUISource(true, mockTemplateMgr, true); expect(TestPackageManager.addPackage).toHaveBeenCalledWith(`@infragistics/ignite-ui-full@">=0.1.0 <0.2.0"`, true); - expect(Util.execSync).toHaveBeenCalledWith( - `npm install @infragistics/ignite-ui-full@">=0.1.0 <0.2.0" --quiet --save`, + expect(Util.spawnSync).toHaveBeenCalledWith( + `npm`, + ['install', `@infragistics/ignite-ui-full@">=0.1.0 <0.2.0"`, '--quiet', '--save'], jasmine.any(Object) ); }); @@ -376,7 +379,7 @@ describe("Unit - Package Manager", () => { }); it("Should run addPackage properly with error code", async () => { spyOn(Util, "log"); - spyOn(Util, "execSync").and.callFake(() => { + spyOn(Util, "spawnSync").and.callFake(() => { const err = new Error("Error"); err["status"] = 1; throw err; @@ -385,17 +388,24 @@ describe("Unit - Package Manager", () => { expect(Util.log).toHaveBeenCalledTimes(2); expect(Util.log).toHaveBeenCalledWith(`Error installing package example-package with npm`); expect(Util.log).toHaveBeenCalledWith(`Error`); - expect(Util.execSync).toHaveBeenCalledWith( - `npm install example-package --quiet --save`, { stdio: "pipe", encoding: "utf8" }); + expect(Util.spawnSync).toHaveBeenCalledWith( + `npm`, ['install', 'example-package', '--quiet', '--save'], { stdio: "pipe", encoding: "utf8" }); }); it("Should run addPackage properly without error code", async () => { spyOn(Util, "log"); - spyOn(Util, "execSync").and.returnValue(""); + spyOn(Util, "spawnSync").and.returnValue({ + status: 0, + pid: 0, + output: [], + stdout: "", + stderr: "", + signal: null + }); PackageManager.addPackage("example-package", true); expect(Util.log).toHaveBeenCalledTimes(1); expect(Util.log).toHaveBeenCalledWith(`Package example-package installed successfully`); - expect(Util.execSync).toHaveBeenCalledWith( - `npm install example-package --quiet --save`, { stdio: "pipe", encoding: "utf8" }); + expect(Util.spawnSync).toHaveBeenCalledWith( + `npm`, ['install', 'example-package', '--quiet', '--save'], { stdio: "pipe", encoding: "utf8" }); }); it("queuePackage should start package install", async () => { @@ -412,12 +422,16 @@ describe("Unit - Package Manager", () => { }; // should ignore already installed spyOn(App.container, "get").and.returnValue(mockFs); - const execSpy = spyOn(child_process, "exec"); + const spawnSpy = spyOn(child_process, "spawn").and.returnValue({ + stdout: { on: jasmine.createSpy() }, + stderr: { on: jasmine.createSpy() }, + on: jasmine.createSpy() + } as any); PackageManager.queuePackage("test-pack"); expect(Util.log).toHaveBeenCalledTimes(0); - expect(child_process.exec).toHaveBeenCalledTimes(1); - expect(child_process.exec).toHaveBeenCalledWith( - `npm install test-pack --quiet --no-save`, {}, jasmine.any(Function)); + expect(child_process.spawn).toHaveBeenCalledTimes(1); + expect((child_process.spawn as any)).toHaveBeenCalledWith( + `npm`, ['install', 'test-pack', '--quiet', '--no-save']); }); it("queuePackage should ignore existing package installs", async () => { @@ -435,15 +449,19 @@ describe("Unit - Package Manager", () => { // should ignore already installed spyOn(App.container, "get").and.returnValue(mockFs); spyOn(Util, "log"); - const execSpy = spyOn(child_process, "exec"); + const spawnSpy = spyOn(child_process, "spawn").and.returnValue({ + stdout: { on: jasmine.createSpy() }, + stderr: { on: jasmine.createSpy() }, + on: jasmine.createSpy() + } as any); PackageManager.queuePackage("test-pack"); expect(Util.log).toHaveBeenCalledTimes(0); - expect(child_process.exec).toHaveBeenCalledTimes(0); + expect(child_process.spawn).toHaveBeenCalledTimes(0); // should ignore if already in queue PackageManager.queuePackage("test-pack2"); PackageManager.queuePackage("test-pack2"); - expect(child_process.exec).toHaveBeenCalledTimes(1); + expect(child_process.spawn).toHaveBeenCalledTimes(1); }); it("Should wait for and log queued package installs", async () => { @@ -461,17 +479,41 @@ describe("Unit - Package Manager", () => { // spyOn(require("module"), "_load").and.returnValue(mockRequire); spyOn(Util, "log"); spyOn(App.container, "get").and.returnValue(mockFs); - const fakeExec = (_cmd: any, _opts: any, callback: (error: Error | null, stdout: string, stderr: string) => void) => { - setTimeout(() => callback(null, 'stdout data', 'stderr data'), 20); - }; - - (fakeExec as any).__promisify__ = () => { - return new Promise((resolve, reject) => { - setTimeout(() => resolve({ stdout: 'stdout data', stderr: 'stderr data' }), 20); + + const createMockChild = (exitCode: number, stdoutData: string, stderrData: string) => { + const mockChild = { + stdout: { on: jasmine.createSpy() }, + stderr: { on: jasmine.createSpy() }, + on: jasmine.createSpy() + }; + + // Setup stdout data handler + mockChild.stdout.on.and.callFake((event: string, handler: any) => { + if (event === 'data') { + setTimeout(() => handler(Buffer.from(stdoutData)), 10); + } }); + + // Setup stderr data handler + mockChild.stderr.on.and.callFake((event: string, handler: any) => { + if (event === 'data') { + setTimeout(() => handler(Buffer.from(stderrData)), 10); + } + }); + + // Setup close handler + mockChild.on.and.callFake((event: string, handler: any) => { + if (event === 'close') { + setTimeout(() => handler(exitCode), 20); + } + }); + + return mockChild; }; - const execSpy = spyOn(child_process, 'exec').and.callFake(fakeExec as any); + const spawnSpy = spyOn(child_process, 'spawn').and.callFake(() => { + return createMockChild(0, 'stdout data', 'stderr data') as any; + }); PackageManager.queuePackage("test-pack"); PackageManager.queuePackage("test-pack2"); @@ -484,17 +526,9 @@ describe("Unit - Package Manager", () => { resetSpy(Util.log); // on error - const fakeExecWithError = (_cmd: any, _opts: any, callback: (error: Error | null, stdout: string, stderr: string) => void) => { - setTimeout(() => callback(new Error('Execution failed'), '', 'stderr'), 20); - }; - - (fakeExecWithError as any).__promisify__ = () => { - return new Promise((resolve, reject) => { - setTimeout(() => reject(new Error('Execution failed')), 20); - }); - }; - - execSpy.and.callFake(fakeExecWithError as any); + spawnSpy.and.callFake(() => { + return createMockChild(1, '', 'stderr') as any; + }); PackageManager.queuePackage("test-pack3"); await PackageManager.flushQueue(true, true); diff --git a/spec/unit/start-spec.ts b/spec/unit/start-spec.ts index da36a998a..13cd6c70e 100644 --- a/spec/unit/start-spec.ts +++ b/spec/unit/start-spec.ts @@ -12,7 +12,7 @@ describe("Unit - start command", () => { beforeEach(() => { spyOn(Util, "log"); spyOn(Util, "error"); - spyOn(Util, "execSync"); + spyOn(Util, "spawnSync"); spyOn(buildCmd, "build"); }); @@ -25,7 +25,7 @@ describe("Unit - start command", () => { "Start command is supported only on existing project created with igniteui-cli", "red"); expect(Util.log).not.toHaveBeenCalled(); - expect(Util.execSync).not.toHaveBeenCalled(); + expect(Util.spawnSync).not.toHaveBeenCalled(); }); it("Starts an Angular project", async () => { @@ -41,17 +41,17 @@ describe("Unit - start command", () => { await startCmd.handler({ _: ["start"], $0: "start" }); expect(buildCmd.build).toHaveBeenCalled(); - expect(Util.execSync).toHaveBeenCalledWith("npm start", { stdio: "inherit", killSignal: "SIGINT" }); + expect(Util.spawnSync).toHaveBeenCalledWith("npm", ['start'], { stdio: "inherit", killSignal: "SIGINT" }); expect(Util.log).toHaveBeenCalledWith(`Starting project.`, "green"); config.project.defaultPort = 3567; await startCmd.handler({ _: ["start"], $0: "start" }); // tslint:disable-next-line: max-line-length - expect(Util.execSync).toHaveBeenCalledWith("npm start -- --port=3567", { stdio: "inherit", killSignal: "SIGINT" }); + expect(Util.spawnSync).toHaveBeenCalledWith("npm", ['start', '--', '--port=3567'], { stdio: "inherit", killSignal: "SIGINT" }); await startCmd.handler({ port: 1234, _: ["start"], $0: "start" }); // tslint:disable-next-line: max-line-length - expect(Util.execSync).toHaveBeenCalledWith("npm start -- --port=1234", { stdio: "inherit", killSignal: "SIGINT" }); + expect(Util.spawnSync).toHaveBeenCalledWith("npm", ['start', '--', '--port=1234'], { stdio: "inherit", killSignal: "SIGINT" }); expect(Util.error).not.toHaveBeenCalled(); }); @@ -69,7 +69,7 @@ describe("Unit - start command", () => { await startCmd.handler({ _: ["start"], $0: "start" }); expect(buildCmd.build).toHaveBeenCalled(); - expect(Util.execSync).toHaveBeenCalledWith("npm start", { stdio: "inherit", killSignal: "SIGINT" }); + expect(Util.spawnSync).toHaveBeenCalledWith("npm", ['start'], { stdio: "inherit", killSignal: "SIGINT" }); expect(Util.log).toHaveBeenCalledWith(`Starting project.`, "green"); /* the following checks are no longer valid, as `config.project.defaultPort` is deprecated for react projects @@ -77,9 +77,9 @@ describe("Unit - start command", () => { this change is required by how `react scripts` work, and to ensure passing a PORT via a platform agnostic approach. config.project.defaultPort = 3567; await startCmd.execute({}); - expect(Util.execSync).toHaveBeenCalledWith("npm start -- --port=3567", { stdio: "inherit", killSignal: "SIGINT" }); + expect(Util.spawnSync).toHaveBeenCalledWith("npm", ['start', '--', '--port=3567'], { stdio: "inherit", killSignal: "SIGINT" }); await startCmd.execute({ port: 1234 }); - expect(Util.execSync).toHaveBeenCalledWith("npm start -- --port=1234", { stdio: "inherit", killSignal: "SIGINT" }); + expect(Util.spawnSync).toHaveBeenCalledWith("npm", ['start', '--', '--port=1234'], { stdio: "inherit", killSignal: "SIGINT" }); */ expect(Util.error).not.toHaveBeenCalled(); }); @@ -97,17 +97,17 @@ describe("Unit - start command", () => { await startCmd.handler({ _: ["start"], $0: "start" }); expect(buildCmd.build).toHaveBeenCalled(); - expect(Util.execSync).toHaveBeenCalledWith("npm start", { stdio: "inherit", killSignal: "SIGINT" }); + expect(Util.spawnSync).toHaveBeenCalledWith("npm", ['start'], { stdio: "inherit", killSignal: "SIGINT" }); expect(Util.log).toHaveBeenCalledWith(`Starting project.`, "green"); config.project.defaultPort = 3567; await startCmd.handler({ _: ["start"], $0: "start" }); expect(process.env.PORT).toEqual("3567"); - expect(Util.execSync).toHaveBeenCalledWith("npm start", { stdio: "inherit", killSignal: "SIGINT" }); + expect(Util.spawnSync).toHaveBeenCalledWith("npm", ['start'], { stdio: "inherit", killSignal: "SIGINT" }); await startCmd.handler({ port: 1234, _: ["start"], $0: "start" }); expect(process.env.PORT).toEqual("1234"); - expect(Util.execSync).toHaveBeenCalledWith("npm start", { stdio: "inherit", killSignal: "SIGINT" }); + expect(Util.spawnSync).toHaveBeenCalledWith("npm", ['start'], { stdio: "inherit", killSignal: "SIGINT" }); expect(Util.error).not.toHaveBeenCalled(); });