refactor: reduce unsafe command arg usage

Author: damyanpetev

packages/core/packages/PackageManager.ts

@@ -88,9 +88,8 @@ export class PackageManager {
 		const config = ProjectConfig.localConfig();
 		if (!config.packagesInstalled) {
 			let command: string;
-			let managerCommand: string;
+			const managerCommand = this.getManager();
 
-			managerCommand = this.getManager();
 			switch (managerCommand) {
 				case "npm":
 				/* passes through */
@@ -125,47 +124,50 @@ export class PackageManager {
 	public static removePackage(packageName: string, verbose: boolean = false): boolean {
 		let command: string;
 		const managerCommand = this.getManager();
+		const sanitizePackage = Util.sanitizeShellArg(packageName);
 		switch (managerCommand) {
 			case "npm":
 			/* passes through */
 			default:
-				command = `${managerCommand} uninstall ${packageName} --quiet --save`;
+				command = `${managerCommand} uninstall ${sanitizePackage} --quiet --save`;
 				break;
 		}
 		try {
 			// tslint:disable-next-line:object-literal-sort-keys
 			Util.execSync(command, { stdio: "pipe", encoding: "utf8" });
 		} catch (error) {
-			Util.log(`Error uninstalling package ${packageName} with ${managerCommand}`);
+			Util.log(`Error uninstalling package ${sanitizePackage} with ${managerCommand}`);
 			if (verbose) {
 				Util.log(error.message);
 			}
 			return false;
 		}
 
-		Util.log(`Package ${packageName} uninstalled successfully`);
+		Util.log(`Package ${sanitizePackage} uninstalled successfully`);
 		return true;
 	}
 
 	public static addPackage(packageName: string, verbose: boolean = false): boolean {
 		const managerCommand = this.getManager();
-		const command = this.getInstallCommand(managerCommand, packageName);
+		const sanitizePackage = Util.sanitizeShellArg(packageName);
+		const command = this.getInstallCommand(managerCommand, sanitizePackage);
 		try {
 			// tslint:disable-next-line:object-literal-sort-keys
 			Util.execSync(command, { stdio: "pipe", encoding: "utf8" });
 		} catch (error) {
-			Util.log(`Error installing package ${packageName} with ${managerCommand}`);
+			Util.log(`Error installing package ${sanitizePackage} with ${managerCommand}`);
 			if (verbose) {
 				Util.log(error.message);
 			}
 			return false;
 		}
-		Util.log(`Package ${packageName} installed successfully`);
+		Util.log(`Package ${sanitizePackage} installed successfully`);
 		return true;
 	}
 
 	public static async queuePackage(packageName: string, verbose = false) {
-		const command = this.getInstallCommand(this.getManager(), packageName).replace("--save", "--no-save");
+		const command = this.getInstallCommand(this.getManager(), Util.sanitizeShellArg(packageName))
+			.replace("--save", "--no-save");
 		const [packName, version] = packageName.split(/@(?=[^\/]+$)/);
 		const packageJSON = this.getPackageJSON();
 		if (!packageJSON.dependencies) {
@@ -222,7 +224,7 @@ export class PackageManager {
 	}
 
 	public static ensureRegistryUser(config: Config, message: string): boolean {
-		const fullPackageRegistry = config.igPackageRegistry;
+		const fullPackageRegistry = Util.sanitizeShellArg(config.igPackageRegistry);
 		try {
 			// tslint:disable-next-line:object-literal-sort-keys
 			Util.execSync(`npm whoami --registry=${fullPackageRegistry}`, { stdio: "pipe", encoding: "utf8" });
@@ -281,7 +283,7 @@ export class PackageManager {
 		}
 	}
 
-	private static getManager(/*config:Config*/): string {
+	private static getManager(/*config:Config*/): 'npm' {
 		//stub to potentially swap out managers
 		return "npm";
 	}

packages/core/util/Util.ts

@@ -316,6 +316,21 @@ export class Util {
 		}
 	}
 
+	/**
+	 * Fairly aggressive sanitize removing anything but ASCII, numbers and a few needed chars that have no action:
+	 * - semicolons (:), dots (.), underscores (_) for paths/URLs
+	 * - dashes (-) & forward slashes (/) for packages and paths/URLs
+	 * - at (@), non-leading tilde (~) for package scope & version
+	 * @remarks
+	 * Most shells should be UTF-enabled, but current usage is very limited thus no need for `\p{L}`
+	 */
+	public static sanitizeShellArg(arg: string): string {
+		return arg
+			.replace(/[^a-z0-9@:_~\^\/\.\-]/gi, '') // remove unsafe chars
+			.replace(/\^(?!\d)/g, '') // keep only ^<digit>
+			.replace(/^~/, ''); // remove leading ~
+	}
+
 	/**
 	 * Execute synchronous command with options
 	 * @param command Command to be executed
@@ -348,29 +363,17 @@ export class Util {
 	}
 
 	/**
-     * Execute synchronous command with options using spawnSync
-     * @param command Command to be executed
-     * @param args Command arguments
-     * @param options Command options
-     * @throws {Error} On non-zero exit code. Error has 'status', 'signal', 'output', 'stdout', 'stderr'
-     */
-	public static spawnSync(command: string, args: string[], options?: SpawnSyncOptions) {
-		try {
-			return spawnSync(command, args, options);
-		} catch (error) {
-			// Handle potential process interruption
-			// Check if the error output ends with "^C"
-			if (error.stderr && error.stderr.toString().endsWith() === "^C") {
-				return process.exit();
-			}
-
-			// Handle specific exit codes for different signals
-			if (error.status === 3221225786 || error.status > 128) {
-				return process.exit();
-			}
-
-			throw error;
-		}
+	 * Execute synchronous command with options using spawnSync
+	 * @param command Command to be executed
+	 * NOTE: `spawn` without `shell` (unsafe) is **not** equivalent to `exec` & requires `npm.cmd` to run the correct process on win
+	 * do not call with/add commands that are not known binaries without validating first
+	 * @param args Command arguments
+	 * @param options Command options
+	 * @returns {SpawnSyncReturns} object with status and stdout
+	 * @remarks Consuming code MUST handle the result and check for failure status!
+	 */
+	public static spawnSync(command: string, args: string[], options?: Omit<SpawnSyncOptions, 'shell'>) {
+		return spawnSync(command, args, options);
 	}
 
 	/**
@@ -383,7 +386,7 @@ export class Util {
 			const options: any = { cwd: path.join(parentRoot, projectName), stdio: [process.stdin, "ignore", "ignore"] };
 			Util.execSync("git init", options);
 			Util.execSync("git add .", options);
-			Util.execSync("git commit -m " + "\"Initial commit for project: " + projectName + "\"", options);
+			Util.execSync("git commit -m " + "\"Initial commit for project\"", options);
 			Util.log(Util.greenCheck() + " Git Initialized and Project '" + projectName + "' Committed");
 		} catch (error) {
 			Util.error("Git initialization failed. Install Git in order to automatically commit the project.", "yellow");

packages/ng-schematics/src/ng-new/index.ts

@@ -174,7 +174,7 @@ export function newProject(options: OptionsSchema): Rule {
 				}
 				if (!options.skipGit) {
 					const gitTask = context.addTask(
-						new RepositoryInitializerTask(options.name, { message: `Initial commit for project: ${options.name}` }),
+						new RepositoryInitializerTask(options.name, { message: `Initial commit for project` }),
 						[...installChain] //copy
 					);
 					installChain.push(gitTask);

packages/ng-schematics/src/ng-new/index_spec.ts

@@ -152,7 +152,7 @@ describe("Schematics ng-new", () => {
 				authorEmail: undefined,
 				authorName: undefined,
 				commit: true,
-				message: `Initial commit for project: ${workingDirectory}`
+				message: `Initial commit for project`
 			};
 			const expectedStart: RunSchematicTaskOptions<any> = {
 				collection: null,
@@ -206,7 +206,7 @@ describe("Schematics ng-new", () => {
 				authorEmail: undefined,
 				authorName: undefined,
 				commit: true,
-				message: `Initial commit for project: ${workingDirectory}`
+				message: `Initial commit for project`
 			};
 			expect(taskOptions.length).toBe(2);
 			expect(mockProject.upgradeIgniteUIPackages).toHaveBeenCalled();

spec/acceptance/new-spec.ts

@@ -135,7 +135,7 @@ describe("New command", () => {
 		process.chdir(projectName);
 		expect(fs.existsSync(".git")).toBeTruthy();
 		expect(Util.execSync("git log -1 --pretty=format:'%s'").toString())
-			.toMatch("Initial commit for project: " + projectName);
+			.toMatch("Initial commit for project");
 		process.chdir("../");
 		testFolder = "./angularProj";
 	});

spec/unit/new-spec.ts

@@ -288,7 +288,7 @@ describe("Unit - New command", () => {
 
 		expect(Util.execSync).toHaveBeenCalledWith("git init", jasmine.any(Object));
 		expect(Util.execSync).toHaveBeenCalledWith("git add .", jasmine.any(Object));
-		expect(Util.execSync).toHaveBeenCalledWith("git commit -m " + "\"Initial commit for project: " + projectName + "\"",
+		expect(Util.execSync).toHaveBeenCalledWith("git commit -m \"Initial commit for project\"",
 			jasmine.any(Object));
 		expect(Util.log).toHaveBeenCalledWith(
 			jasmine.stringMatching("Git Initialized and Project '" + projectName + "' Committed")

spec/unit/packageManager-spec.ts

@@ -236,7 +236,7 @@ describe("Unit - Package Manager", () => {
 		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`,
+			`npm install @infragistics/ignite-ui-full@~20.1 --quiet --save`,
 			jasmine.any(Object)
 		);
 		expect(TestPackageManager.removePackage).toHaveBeenCalledWith("ignite-ui", true);
@@ -247,17 +247,17 @@ describe("Unit - Package Manager", () => {
 		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`,
+			`npm install @infragistics/ignite-ui-full@^17.1 --quiet --save`,
 			jasmine.any(Object)
 		);
 
-		mockDeps.dependencies["ignite-ui"] = ">=0.1.0 <0.2.0";
+		mockDeps.dependencies["ignite-ui"] = ">=0.1.0"; // ranges no longer supported, but check sanitize
 		mockProjectConfig.project.igniteuiSource = "./node_modules/ignite-ui";
 		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(TestPackageManager.addPackage).toHaveBeenCalledWith(`@infragistics/ignite-ui-full@">=0.1.0"`, true);
 		expect(Util.execSync).toHaveBeenCalledWith(
-			`npm install @infragistics/ignite-ui-full@">=0.1.0 <0.2.0" --quiet --save`,
+			`npm install @infragistics/ignite-ui-full@0.1.0 --quiet --save`,
 			jasmine.any(Object)
 		);
 	});