Skip to content

Commit 1f86df3

Browse files
CopilotLipataHristo HristovHristo313
authored
fix: command injection vulnerabilities in PackageManager and start command (#1438)
* Initial plan * Fix command injection vulnerabilities in PackageManager and start command Co-authored-by: Lipata <[email protected]> * Remove test artifact from package.json Co-authored-by: Lipata <[email protected]> * fix: revert yarn.lock changes * fix: revert yarn lock changes --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: Lipata <[email protected]> Co-authored-by: Hristo Hristov <[email protected]> Co-authored-by: Hristo Hristov <[email protected]> Co-authored-by: Nikolay Alipiev <[email protected]>
1 parent a23b067 commit 1f86df3

File tree

4 files changed

+117
-63
lines changed

4 files changed

+117
-63
lines changed

‎packages/cli/lib/commands/start.ts‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,11 +7,16 @@ import { PositionalArgs, StartCommandType } from "./types";
77
import { ArgumentsCamelCase } from "yargs";
88

99
const execSyncNpmStart = (port: number, options: ExecSyncOptions): void => {
10+
const args = ['start'];
1011
if (port) {
11-
Util.execSync(`npm start -- --port=${port}`, options);
12-
return;
12+
// Validate port is a number to prevent command injection
13+
if (!Number.isInteger(port) || port < 0 || port > 65535) {
14+
Util.error(`Invalid port number: ${port}`, "red");
15+
return;
16+
}
17+
args.push('--', `--port=${port}`);
1318
}
14-
Util.execSync(`npm start`, options);
19+
Util.spawnSync('npm', args, options);
1520
};
1621

1722
const command: StartCommandType = {

‎packages/core/packages/PackageManager.ts‎

Lines changed: 25 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { exec } from "child_process";
1+
import { spawn } from "child_process";
22
import * as path from "path";
33
import { TemplateManager } from "../../cli/lib/TemplateManager";
44
import { Config, FS_TOKEN, IFileSystem, ProjectTemplate } from "../types";
@@ -149,10 +149,10 @@ export class PackageManager {
149149

150150
public static addPackage(packageName: string, verbose: boolean = false): boolean {
151151
const managerCommand = this.getManager();
152-
const command = this.getInstallCommand(managerCommand, packageName);
152+
const args = this.getInstallArgs(packageName);
153153
try {
154154
// tslint:disable-next-line:object-literal-sort-keys
155-
Util.execSync(command, { stdio: "pipe", encoding: "utf8" });
155+
Util.spawnSync(managerCommand, args, { stdio: "pipe", encoding: "utf8" });
156156
} catch (error) {
157157
Util.log(`Error installing package ${packageName} with ${managerCommand}`);
158158
if (verbose) {
@@ -165,7 +165,7 @@ export class PackageManager {
165165
}
166166

167167
public static async queuePackage(packageName: string, verbose = false) {
168-
const command = this.getInstallCommand(this.getManager(), packageName).replace("--save", "--no-save");
168+
const args = this.getInstallArgs(packageName).map(arg => arg === '--save' ? '--no-save' : arg);
169169
const [packName, version] = packageName.split(/@(?=[^\/]+$)/);
170170
const packageJSON = this.getPackageJSON();
171171
if (!packageJSON.dependencies) {
@@ -190,13 +190,24 @@ export class PackageManager {
190190
// D.P. Concurrent install runs should be supported
191191
// https://github.com/npm/npm/issues/5948
192192
// https://github.com/npm/npm/issues/2500
193+
const managerCommand = this.getManager();
193194
const task = new Promise<{ packageName, error, stdout, stderr }>((resolve, reject) => {
194-
const child = exec(
195-
command, {},
196-
(error, stdout, stderr) => {
197-
resolve({ packageName, error, stdout, stderr });
198-
}
199-
);
195+
const child = spawn(managerCommand, args);
196+
let stdout = '';
197+
let stderr = '';
198+
child.stdout?.on('data', (data) => {
199+
stdout += data.toString();
200+
});
201+
child.stderr?.on('data', (data) => {
202+
stderr += data.toString();
203+
});
204+
child.on('close', (code) => {
205+
const error = code !== 0 ? new Error(`Process exited with code ${code}`) : null;
206+
resolve({ packageName, error, stdout, stderr });
207+
});
208+
child.on('error', (err) => {
209+
resolve({ packageName, error: err, stdout, stderr });
210+
});
200211
});
201212
task["packageName"] = packName;
202213
this.installQueue.push(task);
@@ -281,6 +292,10 @@ export class PackageManager {
281292
}
282293
}
283294

295+
private static getInstallArgs(packageName: string): string[] {
296+
return ['install', packageName, '--quiet', '--save'];
297+
}
298+
284299
private static getManager(/*config:Config*/): string {
285300
//stub to potentially swap out managers
286301
return "npm";

‎spec/unit/packageManager-spec.ts‎

Lines changed: 73 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -246,15 +246,16 @@ describe("Unit - Package Manager", () => {
246246
spyOn(ProjectConfig, "localConfig").and.callFake(() => mockProjectConfig);
247247
spyOn(ProjectConfig, "setConfig");
248248
spyOn(TestPackageManager, "addPackage").and.callThrough();
249-
spyOn(Util, "execSync");
249+
spyOn(Util, "spawnSync");
250250
spyOn(Util, "log");
251251
spyOn(TestPackageManager, "removePackage");
252252
spyOn(TestPackageManager, "getPackageJSON").and.callFake(() => mockDeps);
253253

254254
await TestPackageManager.ensureIgniteUISource(true, mockTemplateMgr, true);
255255
expect(TestPackageManager.addPackage).toHaveBeenCalledWith(`@infragistics/ignite-ui-full@"~20.1"`, true);
256-
expect(Util.execSync).toHaveBeenCalledWith(
257-
`npm install @infragistics/ignite-ui-full@"~20.1" --quiet --save`,
256+
expect(Util.spawnSync).toHaveBeenCalledWith(
257+
`npm`,
258+
['install', `@infragistics/ignite-ui-full@"~20.1"`, '--quiet', '--save'],
258259
jasmine.any(Object)
259260
);
260261
expect(TestPackageManager.removePackage).toHaveBeenCalledWith("ignite-ui", true);
@@ -264,8 +265,9 @@ describe("Unit - Package Manager", () => {
264265
mockTemplateMgr.generateConfig = mockProjectConfig;
265266
await TestPackageManager.ensureIgniteUISource(true, mockTemplateMgr, true);
266267
expect(TestPackageManager.addPackage).toHaveBeenCalledWith(`@infragistics/ignite-ui-full@"^17.1"`, true);
267-
expect(Util.execSync).toHaveBeenCalledWith(
268-
`npm install @infragistics/ignite-ui-full@"^17.1" --quiet --save`,
268+
expect(Util.spawnSync).toHaveBeenCalledWith(
269+
`npm`,
270+
['install', `@infragistics/ignite-ui-full@"^17.1"`, '--quiet', '--save'],
269271
jasmine.any(Object)
270272
);
271273

@@ -274,8 +276,9 @@ describe("Unit - Package Manager", () => {
274276
mockTemplateMgr.generateConfig = mockProjectConfig;
275277
await TestPackageManager.ensureIgniteUISource(true, mockTemplateMgr, true);
276278
expect(TestPackageManager.addPackage).toHaveBeenCalledWith(`@infragistics/ignite-ui-full@">=0.1.0 <0.2.0"`, true);
277-
expect(Util.execSync).toHaveBeenCalledWith(
278-
`npm install @infragistics/ignite-ui-full@">=0.1.0 <0.2.0" --quiet --save`,
279+
expect(Util.spawnSync).toHaveBeenCalledWith(
280+
`npm`,
281+
['install', `@infragistics/ignite-ui-full@">=0.1.0 <0.2.0"`, '--quiet', '--save'],
279282
jasmine.any(Object)
280283
);
281284
});
@@ -376,7 +379,7 @@ describe("Unit - Package Manager", () => {
376379
});
377380
it("Should run addPackage properly with error code", async () => {
378381
spyOn(Util, "log");
379-
spyOn(Util, "execSync").and.callFake(() => {
382+
spyOn(Util, "spawnSync").and.callFake(() => {
380383
const err = new Error("Error");
381384
err["status"] = 1;
382385
throw err;
@@ -385,17 +388,24 @@ describe("Unit - Package Manager", () => {
385388
expect(Util.log).toHaveBeenCalledTimes(2);
386389
expect(Util.log).toHaveBeenCalledWith(`Error installing package example-package with npm`);
387390
expect(Util.log).toHaveBeenCalledWith(`Error`);
388-
expect(Util.execSync).toHaveBeenCalledWith(
389-
`npm install example-package --quiet --save`, { stdio: "pipe", encoding: "utf8" });
391+
expect(Util.spawnSync).toHaveBeenCalledWith(
392+
`npm`, ['install', 'example-package', '--quiet', '--save'], { stdio: "pipe", encoding: "utf8" });
390393
});
391394
it("Should run addPackage properly without error code", async () => {
392395
spyOn(Util, "log");
393-
spyOn(Util, "execSync").and.returnValue("");
396+
spyOn(Util, "spawnSync").and.returnValue({
397+
status: 0,
398+
pid: 0,
399+
output: [],
400+
stdout: "",
401+
stderr: "",
402+
signal: null
403+
});
394404
PackageManager.addPackage("example-package", true);
395405
expect(Util.log).toHaveBeenCalledTimes(1);
396406
expect(Util.log).toHaveBeenCalledWith(`Package example-package installed successfully`);
397-
expect(Util.execSync).toHaveBeenCalledWith(
398-
`npm install example-package --quiet --save`, { stdio: "pipe", encoding: "utf8" });
407+
expect(Util.spawnSync).toHaveBeenCalledWith(
408+
`npm`, ['install', 'example-package', '--quiet', '--save'], { stdio: "pipe", encoding: "utf8" });
399409
});
400410

401411
it("queuePackage should start package install", async () => {
@@ -412,12 +422,16 @@ describe("Unit - Package Manager", () => {
412422
};
413423
// should ignore already installed
414424
spyOn(App.container, "get").and.returnValue(mockFs);
415-
const execSpy = spyOn(child_process, "exec");
425+
const spawnSpy = spyOn(child_process, "spawn").and.returnValue({
426+
stdout: { on: jasmine.createSpy() },
427+
stderr: { on: jasmine.createSpy() },
428+
on: jasmine.createSpy()
429+
} as any);
416430
PackageManager.queuePackage("test-pack");
417431
expect(Util.log).toHaveBeenCalledTimes(0);
418-
expect(child_process.exec).toHaveBeenCalledTimes(1);
419-
expect(child_process.exec).toHaveBeenCalledWith(
420-
`npm install test-pack --quiet --no-save`, {}, jasmine.any(Function));
432+
expect(child_process.spawn).toHaveBeenCalledTimes(1);
433+
expect((child_process.spawn as any)).toHaveBeenCalledWith(
434+
`npm`, ['install', 'test-pack', '--quiet', '--no-save']);
421435
});
422436

423437
it("queuePackage should ignore existing package installs", async () => {
@@ -435,15 +449,19 @@ describe("Unit - Package Manager", () => {
435449
// should ignore already installed
436450
spyOn(App.container, "get").and.returnValue(mockFs);
437451
spyOn(Util, "log");
438-
const execSpy = spyOn(child_process, "exec");
452+
const spawnSpy = spyOn(child_process, "spawn").and.returnValue({
453+
stdout: { on: jasmine.createSpy() },
454+
stderr: { on: jasmine.createSpy() },
455+
on: jasmine.createSpy()
456+
} as any);
439457
PackageManager.queuePackage("test-pack");
440458
expect(Util.log).toHaveBeenCalledTimes(0);
441-
expect(child_process.exec).toHaveBeenCalledTimes(0);
459+
expect(child_process.spawn).toHaveBeenCalledTimes(0);
442460

443461
// should ignore if already in queue
444462
PackageManager.queuePackage("test-pack2");
445463
PackageManager.queuePackage("test-pack2");
446-
expect(child_process.exec).toHaveBeenCalledTimes(1);
464+
expect(child_process.spawn).toHaveBeenCalledTimes(1);
447465
});
448466

449467
it("Should wait for and log queued package installs", async () => {
@@ -461,17 +479,41 @@ describe("Unit - Package Manager", () => {
461479
// spyOn(require("module"), "_load").and.returnValue(mockRequire);
462480
spyOn(Util, "log");
463481
spyOn(App.container, "get").and.returnValue(mockFs);
464-
const fakeExec = (_cmd: any, _opts: any, callback: (error: Error | null, stdout: string, stderr: string) => void) => {
465-
setTimeout(() => callback(null, 'stdout data', 'stderr data'), 20);
466-
};
467-
468-
(fakeExec as any).__promisify__ = () => {
469-
return new Promise((resolve, reject) => {
470-
setTimeout(() => resolve({ stdout: 'stdout data', stderr: 'stderr data' }), 20);
482+
483+
const createMockChild = (exitCode: number, stdoutData: string, stderrData: string) => {
484+
const mockChild = {
485+
stdout: { on: jasmine.createSpy() },
486+
stderr: { on: jasmine.createSpy() },
487+
on: jasmine.createSpy()
488+
};
489+
490+
// Setup stdout data handler
491+
mockChild.stdout.on.and.callFake((event: string, handler: any) => {
492+
if (event === 'data') {
493+
setTimeout(() => handler(Buffer.from(stdoutData)), 10);
494+
}
471495
});
496+
497+
// Setup stderr data handler
498+
mockChild.stderr.on.and.callFake((event: string, handler: any) => {
499+
if (event === 'data') {
500+
setTimeout(() => handler(Buffer.from(stderrData)), 10);
501+
}
502+
});
503+
504+
// Setup close handler
505+
mockChild.on.and.callFake((event: string, handler: any) => {
506+
if (event === 'close') {
507+
setTimeout(() => handler(exitCode), 20);
508+
}
509+
});
510+
511+
return mockChild;
472512
};
473513

474-
const execSpy = spyOn(child_process, 'exec').and.callFake(fakeExec as any);
514+
const spawnSpy = spyOn(child_process, 'spawn').and.callFake(() => {
515+
return createMockChild(0, 'stdout data', 'stderr data') as any;
516+
});
475517

476518
PackageManager.queuePackage("test-pack");
477519
PackageManager.queuePackage("test-pack2");
@@ -484,17 +526,9 @@ describe("Unit - Package Manager", () => {
484526
resetSpy(Util.log);
485527

486528
// on error
487-
const fakeExecWithError = (_cmd: any, _opts: any, callback: (error: Error | null, stdout: string, stderr: string) => void) => {
488-
setTimeout(() => callback(new Error('Execution failed'), '', 'stderr'), 20);
489-
};
490-
491-
(fakeExecWithError as any).__promisify__ = () => {
492-
return new Promise((resolve, reject) => {
493-
setTimeout(() => reject(new Error('Execution failed')), 20);
494-
});
495-
};
496-
497-
execSpy.and.callFake(fakeExecWithError as any);
529+
spawnSpy.and.callFake(() => {
530+
return createMockChild(1, '', 'stderr') as any;
531+
});
498532

499533
PackageManager.queuePackage("test-pack3");
500534
await PackageManager.flushQueue(true, true);

‎spec/unit/start-spec.ts‎

Lines changed: 11 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ describe("Unit - start command", () => {
1212
beforeEach(() => {
1313
spyOn(Util, "log");
1414
spyOn(Util, "error");
15-
spyOn(Util, "execSync");
15+
spyOn(Util, "spawnSync");
1616
spyOn(buildCmd, "build");
1717
});
1818

@@ -25,7 +25,7 @@ describe("Unit - start command", () => {
2525
"Start command is supported only on existing project created with igniteui-cli",
2626
"red");
2727
expect(Util.log).not.toHaveBeenCalled();
28-
expect(Util.execSync).not.toHaveBeenCalled();
28+
expect(Util.spawnSync).not.toHaveBeenCalled();
2929
});
3030

3131
it("Starts an Angular project", async () => {
@@ -41,17 +41,17 @@ describe("Unit - start command", () => {
4141

4242
await startCmd.handler({ _: ["start"], $0: "start" });
4343
expect(buildCmd.build).toHaveBeenCalled();
44-
expect(Util.execSync).toHaveBeenCalledWith("npm start", { stdio: "inherit", killSignal: "SIGINT" });
44+
expect(Util.spawnSync).toHaveBeenCalledWith("npm", ['start'], { stdio: "inherit", killSignal: "SIGINT" });
4545
expect(Util.log).toHaveBeenCalledWith(`Starting project.`, "green");
4646

4747
config.project.defaultPort = 3567;
4848
await startCmd.handler({ _: ["start"], $0: "start" });
4949
// tslint:disable-next-line: max-line-length
50-
expect(Util.execSync).toHaveBeenCalledWith("npm start -- --port=3567", { stdio: "inherit", killSignal: "SIGINT" });
50+
expect(Util.spawnSync).toHaveBeenCalledWith("npm", ['start', '--', '--port=3567'], { stdio: "inherit", killSignal: "SIGINT" });
5151

5252
await startCmd.handler({ port: 1234, _: ["start"], $0: "start" });
5353
// tslint:disable-next-line: max-line-length
54-
expect(Util.execSync).toHaveBeenCalledWith("npm start -- --port=1234", { stdio: "inherit", killSignal: "SIGINT" });
54+
expect(Util.spawnSync).toHaveBeenCalledWith("npm", ['start', '--', '--port=1234'], { stdio: "inherit", killSignal: "SIGINT" });
5555

5656
expect(Util.error).not.toHaveBeenCalled();
5757
});
@@ -69,17 +69,17 @@ describe("Unit - start command", () => {
6969

7070
await startCmd.handler({ _: ["start"], $0: "start" });
7171
expect(buildCmd.build).toHaveBeenCalled();
72-
expect(Util.execSync).toHaveBeenCalledWith("npm start", { stdio: "inherit", killSignal: "SIGINT" });
72+
expect(Util.spawnSync).toHaveBeenCalledWith("npm", ['start'], { stdio: "inherit", killSignal: "SIGINT" });
7373
expect(Util.log).toHaveBeenCalledWith(`Starting project.`, "green");
7474

7575
/* the following checks are no longer valid, as `config.project.defaultPort` is deprecated for react projects
7676
in favor of using an .env file on project root lv, containing default variables like port. Ex: PORT=3002
7777
this change is required by how `react scripts` work, and to ensure passing a PORT via a platform agnostic approach.
7878
config.project.defaultPort = 3567;
7979
await startCmd.execute({});
80-
expect(Util.execSync).toHaveBeenCalledWith("npm start -- --port=3567", { stdio: "inherit", killSignal: "SIGINT" });
80+
expect(Util.spawnSync).toHaveBeenCalledWith("npm", ['start', '--', '--port=3567'], { stdio: "inherit", killSignal: "SIGINT" });
8181
await startCmd.execute({ port: 1234 });
82-
expect(Util.execSync).toHaveBeenCalledWith("npm start -- --port=1234", { stdio: "inherit", killSignal: "SIGINT" });
82+
expect(Util.spawnSync).toHaveBeenCalledWith("npm", ['start', '--', '--port=1234'], { stdio: "inherit", killSignal: "SIGINT" });
8383
*/
8484
expect(Util.error).not.toHaveBeenCalled();
8585
});
@@ -97,17 +97,17 @@ describe("Unit - start command", () => {
9797

9898
await startCmd.handler({ _: ["start"], $0: "start" });
9999
expect(buildCmd.build).toHaveBeenCalled();
100-
expect(Util.execSync).toHaveBeenCalledWith("npm start", { stdio: "inherit", killSignal: "SIGINT" });
100+
expect(Util.spawnSync).toHaveBeenCalledWith("npm", ['start'], { stdio: "inherit", killSignal: "SIGINT" });
101101
expect(Util.log).toHaveBeenCalledWith(`Starting project.`, "green");
102102

103103
config.project.defaultPort = 3567;
104104
await startCmd.handler({ _: ["start"], $0: "start" });
105105
expect(process.env.PORT).toEqual("3567");
106-
expect(Util.execSync).toHaveBeenCalledWith("npm start", { stdio: "inherit", killSignal: "SIGINT" });
106+
expect(Util.spawnSync).toHaveBeenCalledWith("npm", ['start'], { stdio: "inherit", killSignal: "SIGINT" });
107107

108108
await startCmd.handler({ port: 1234, _: ["start"], $0: "start" });
109109
expect(process.env.PORT).toEqual("1234");
110-
expect(Util.execSync).toHaveBeenCalledWith("npm start", { stdio: "inherit", killSignal: "SIGINT" });
110+
expect(Util.spawnSync).toHaveBeenCalledWith("npm", ['start'], { stdio: "inherit", killSignal: "SIGINT" });
111111

112112
expect(Util.error).not.toHaveBeenCalled();
113113
});

0 commit comments

Comments
 (0)