fix(clean): refuse to delete outside the project directory

The paths reaching cleanPath now include the configurable build
directory, so a buildPath such as '../build' resolved onto a sibling of
the project and deleted it. Paths that escape the project are skipped
with a warning and reported as unsuccessful.

Author: edusperoni

lib/services/project-cleanup-service.ts

@@ -20,12 +20,12 @@ export class ProjectCleanupService implements IProjectCleanupService {
 		private $fs: IFileSystem,
 		private $logger: ILogger,
 		private $projectHelper: IProjectHelper,
-		private $terminalSpinnerService: ITerminalSpinnerService
+		private $terminalSpinnerService: ITerminalSpinnerService,
 	) {}
 
 	public async clean(
 		pathsToClean: string[],
-		options?: IProjectCleanupOptions
+		options?: IProjectCleanupOptions,
 	): Promise<IProjectCleanupResult> {
 		this.spinner = this.$terminalSpinnerService.createSpinner({
 			isSilent: options?.silent,
@@ -39,10 +39,10 @@ export class ProjectCleanupService implements IProjectCleanupService {
 				(error) => {
 					this.$logger.trace(
 						`Encountered error while cleaning. Error is: ${error.message}.`,
-						error
+						error,
 					);
 					return { ok: false };
-				}
+				},
 			);
 			if (stats && "size" in cleanRes) {
 				stats.set(pathToClean, cleanRes.size);
@@ -63,7 +63,7 @@ export class ProjectCleanupService implements IProjectCleanupService {
 
 	public async cleanPath(
 		pathToClean: string,
-		options?: IProjectCleanupOptions
+		options?: IProjectCleanupOptions,
 	): Promise<IProjectPathCleanupResult> {
 		const dryRun = options?.dryRun ?? false;
 		const logPrefix = dryRun ? color.grey("(dry run) ") : "";
@@ -77,9 +77,21 @@ export class ProjectCleanupService implements IProjectCleanupService {
 		}
 
 		const filePath = path.resolve(this.$projectHelper.projectDir, pathToClean);
-		const displayPath = color.yellow(
-			`${path.relative(this.$projectHelper.projectDir, filePath)}`
+		const relativePath = path.relative(
+			this.$projectHelper.projectDir,
+			filePath,
 		);
+		const displayPath = color.yellow(`${relativePath}`);
+
+		// Paths reach here from the project config - buildPath and
+		// cli.pathsToClean among them - where a leading `..` resolves onto
+		// directories the project does not own.
+		if (relativePath.startsWith("..") || path.isAbsolute(relativePath)) {
+			this.$logger.warn(
+				`Skipping '${filePath}' because it is outside the project directory.`,
+			);
+			return { ok: false };
+		}
 
 		this.$logger.trace(`${logPrefix}Trying to clean '${filePath}'`);
 
@@ -93,13 +105,13 @@ export class ProjectCleanupService implements IProjectCleanupService {
 
 			if (stat.isDirectory()) {
 				this.$logger.trace(
-					`${logPrefix}Path '${filePath}' is a directory, deleting.`
+					`${logPrefix}Path '${filePath}' is a directory, deleting.`,
 				);
 				!dryRun && this.$fs.deleteDirectorySafe(filePath);
 				fileType = "directory";
 			} else {
 				this.$logger.trace(
-					`${logPrefix}Path '${filePath}' is a file, deleting.`
+					`${logPrefix}Path '${filePath}' is a file, deleting.`,
 				);
 				!dryRun && this.$fs.deleteFile(filePath);
 				fileType = "file";
@@ -122,7 +134,7 @@ export class ProjectCleanupService implements IProjectCleanupService {
 
 		this.$logger.trace(`${logPrefix}Path '${filePath}' not found, skipping.`);
 		this.spinner.info(
-			`${logPrefix}Skipping ${displayPath} because it doesn't exist.`
+			`${logPrefix}Skipping ${displayPath} because it doesn't exist.`,
 		);
 
 		if (options?.stats) {

test/services/project-cleanup-service.ts

@@ -0,0 +1,78 @@
+import { assert } from "chai";
+import * as path from "path";
+import { Yok } from "../../lib/common/yok";
+import { ProjectCleanupService } from "../../lib/services/project-cleanup-service";
+import { IInjector } from "../../lib/common/definitions/yok";
+
+const projectDir = path.join("/tmp", "nsm-cleanup-project");
+
+function createTestInjector(deletedPaths: string[]): IInjector {
+	const testInjector = new Yok();
+	testInjector.register("fs", {
+		exists: (p: string) => !deletedPaths.includes(p),
+		getFsStats: () => ({ isDirectory: () => true }),
+		getSize: () => 0,
+		deleteDirectorySafe: (p: string) => deletedPaths.push(p),
+		deleteFile: (p: string) => deletedPaths.push(p),
+	});
+	testInjector.register("logger", {
+		trace: (): void => undefined,
+		warn: (): void => undefined,
+		info: (): void => undefined,
+	});
+	testInjector.register("projectHelper", { projectDir });
+	testInjector.register("terminalSpinnerService", {
+		createSpinner: () => ({
+			clear: (): void => undefined,
+			start: (): void => undefined,
+			stop: (): void => undefined,
+			succeed: (): void => undefined,
+			fail: (): void => undefined,
+			text: "",
+		}),
+	});
+
+	return testInjector;
+}
+
+describe("projectCleanupService", () => {
+	let deletedPaths: string[];
+	let service: ProjectCleanupService;
+
+	beforeEach(() => {
+		deletedPaths = [];
+		service = createTestInjector(deletedPaths).resolve(ProjectCleanupService);
+	});
+
+	it("cleans a path inside the project", async () => {
+		const result = await service.clean(["platforms"], { silent: true });
+
+		assert.isTrue(result.ok);
+		assert.deepStrictEqual(deletedPaths, [path.join(projectDir, "platforms")]);
+	});
+
+	it("refuses a path that escapes the project directory", async () => {
+		const result = await service.clean(["../sibling"], { silent: true });
+
+		assert.isFalse(result.ok);
+		assert.deepStrictEqual(deletedPaths, []);
+	});
+
+	it("refuses an absolute path outside the project directory", async () => {
+		const result = await service.clean([path.join("/tmp", "elsewhere")], {
+			silent: true,
+		});
+
+		assert.isFalse(result.ok);
+		assert.deepStrictEqual(deletedPaths, []);
+	});
+
+	it("allows an absolute path that resolves inside the project", async () => {
+		const result = await service.clean([path.join(projectDir, "platforms")], {
+			silent: true,
+		});
+
+		assert.isTrue(result.ok);
+		assert.deepStrictEqual(deletedPaths, [path.join(projectDir, "platforms")]);
+	});
+});