fix: Forbid access to files outside source control work directory (#11152)

packages/cli/src/environments/source-control/__tests__/source-control.service.test.ts

@@ -0,0 +1,46 @@
+import { mock } from 'jest-mock-extended';
+import { InstanceSettings } from 'n8n-core';
+
+import { SourceControlPreferencesService } from '@/environments/source-control/source-control-preferences.service.ee';
+import { SourceControlService } from '@/environments/source-control/source-control.service.ee';
+
+describe('SourceControlService', () => {
+	const preferencesService = new SourceControlPreferencesService(
+		new InstanceSettings(),
+		mock(),
+		mock(),
+	);
+	const sourceControlService = new SourceControlService(
+		mock(),
+		mock(),
+		preferencesService,
+		mock(),
+		mock(),
+		mock(),
+		mock(),
+	);
+
+	describe('pushWorkfolder', () => {
+		it('should throw an error if a file is given that is not in the workfolder', async () => {
+			jest.spyOn(sourceControlService, 'sanityCheck').mockResolvedValue(undefined);
+
+			await expect(
+				sourceControlService.pushWorkfolder({
+					fileNames: [
+						{
+							file: '/etc/passwd',
+							id: 'test',
+							name: 'secret-file',
+							type: 'file',
+							status: 'modified',
+							location: 'local',
+							conflict: false,
+							updatedAt: new Date().toISOString(),
+							pushed: false,
+						},
+					],
+				}),
+			).rejects.toThrow('File path /etc/passwd is invalid');
+		});
+	});
+});

packages/cli/src/environments/source-control/source-control-helper.ee.ts

@@ -1,10 +1,13 @@
 import { generateKeyPairSync } from 'crypto';
 import { constants as fsConstants, mkdirSync, accessSync } from 'fs';
+import { ApplicationError } from 'n8n-workflow';
+import { ok } from 'node:assert/strict';
 import path from 'path';
 import { Container } from 'typedi';
 
 import { License } from '@/license';
 import { Logger } from '@/logging/logger.service';
+import { isContainedWithin } from '@/utils/path-util';
 
 import {
 	SOURCE_CONTROL_GIT_KEY_COMMENT,
@@ -163,3 +166,24 @@ export function getTrackingInformationFromPostPushResult(result: SourceControlle
 			uniques.filter((file) => file.pushed && file.file.startsWith('variable_stubs')).length ?? 0,
 	};
 }
+
+/**
+ * Normalizes and validates the given source controlled file path. Ensures
+ * the path is absolute and contained within the git folder.
+ *
+ * @throws {ApplicationError} If the path is not within the git folder
+ */
+export function normalizeAndValidateSourceControlledFilePath(
+	gitFolderPath: string,
+	filePath: string,
+) {
+	ok(path.isAbsolute(gitFolderPath), 'gitFolder must be an absolute path');
+
+	const normalizedPath = path.isAbsolute(filePath) ? filePath : path.join(gitFolderPath, filePath);
+
+	if (!isContainedWithin(gitFolderPath, filePath)) {
+		throw new ApplicationError(`File path ${filePath} is invalid`);
+	}
+
+	return normalizedPath;
+}

packages/cli/src/environments/source-control/source-control.controller.ee.ts

@@ -170,6 +170,7 @@ export class SourceControlController {
 		if (this.sourceControlPreferencesService.isBranchReadOnly()) {
 			throw new BadRequestError('Cannot push onto read-only branch.');
 		}
+
 		try {
 			await this.sourceControlService.setGitUserDetails(
 				`${req.user.firstName} ${req.user.lastName}`,

packages/cli/src/environments/source-control/source-control.service.ee.ts

@@ -25,6 +25,7 @@ import {
 	getTrackingInformationFromPrePushResult,
 	getTrackingInformationFromPullResult,
 	getVariablesPath,
+	normalizeAndValidateSourceControlledFilePath,
 	sourceControlFoldersExistCheck,
 } from './source-control-helper.ee';
 import { SourceControlImportService } from './source-control-import.service.ee';
@@ -80,7 +81,7 @@ export class SourceControlService {
 		});
 	}
 
-	private async sanityCheck(): Promise<void> {
+	public async sanityCheck(): Promise<void> {
 		try {
 			const foldersExisted = sourceControlFoldersExistCheck(
 				[this.gitFolder, this.sshFolder],
@@ -217,8 +218,20 @@ export class SourceControlService {
 			throw new BadRequestError('Cannot push onto read-only branch.');
 		}
 
+		const filesToPush = options.fileNames.map((file) => {
+			const normalizedPath = normalizeAndValidateSourceControlledFilePath(
+				this.gitFolder,
+				file.file,
+			);
+
+			return {
+				...file,
+				file: normalizedPath,
+			};
+		});
+
 		// only determine file status if not provided by the frontend
-		let statusResult: SourceControlledFile[] = options.fileNames;
+		let statusResult: SourceControlledFile[] = filesToPush;
 		if (statusResult.length === 0) {
 			statusResult = (await this.getStatus({
 				direction: 'push',
@@ -240,7 +253,7 @@ export class SourceControlService {
 
 		const filesToBePushed = new Set<string>();
 		const filesToBeDeleted = new Set<string>();
-		options.fileNames.forEach((e) => {
+		filesToPush.forEach((e) => {
 			if (e.status !== 'deleted') {
 				filesToBePushed.add(e.file);
 			} else {
@@ -250,12 +263,12 @@ export class SourceControlService {
 
 		this.sourceControlExportService.rmFilesFromExportFolder(filesToBeDeleted);
 
-		const workflowsToBeExported = options.fileNames.filter(
+		const workflowsToBeExported = filesToPush.filter(
 			(e) => e.type === 'workflow' && e.status !== 'deleted',
 		);
 		await this.sourceControlExportService.exportWorkflowsToWorkFolder(workflowsToBeExported);
 
-		const credentialsToBeExported = options.fileNames.filter(
+		const credentialsToBeExported = filesToPush.filter(
 			(e) => e.type === 'credential' && e.status !== 'deleted',
 		);
 		const credentialExportResult =
@@ -269,11 +282,11 @@ export class SourceControlService {
 			});
 		}
 
-		if (options.fileNames.find((e) => e.type === 'tags')) {
+		if (filesToPush.find((e) => e.type === 'tags')) {
 			await this.sourceControlExportService.exportTagsToWorkFolder();
 		}
 
-		if (options.fileNames.find((e) => e.type === 'variables')) {
+		if (filesToPush.find((e) => e.type === 'variables')) {
 			await this.sourceControlExportService.exportVariablesToWorkFolder();
 		}
 
@@ -281,7 +294,7 @@ export class SourceControlService {
 
 		for (let i = 0; i < statusResult.length; i++) {
 			// eslint-disable-next-line @typescript-eslint/no-loop-func
-			if (options.fileNames.find((file) => file.file === statusResult[i].file)) {
+			if (filesToPush.find((file) => file.file === statusResult[i].file)) {
 				statusResult[i].pushed = true;
 			}
 		}

packages/cli/src/utils/__tests__/path-util.test.ts

@@ -0,0 +1,24 @@
+import { isContainedWithin } from '../path-util';
+
+describe('isContainedWithin', () => {
+	it('should return true when parent and child paths are the same', () => {
+		expect(isContainedWithin('/some/parent/folder', '/some/parent/folder')).toBe(true);
+	});
+
+	test.each([
+		['/some/parent/folder', '/some/parent/folder/subfolder/file.txt'],
+		['/some/parent/folder', '/some/parent/folder/../folder/subfolder/file.txt'],
+		['/some/parent/folder/', '/some/parent/folder/subfolder/file.txt'],
+		['/some/parent/folder', '/some/parent/folder/subfolder/'],
+	])('should return true for parent %s and child %s', (parent, child) => {
+		expect(isContainedWithin(parent, child)).toBe(true);
+	});
+
+	test.each([
+		['/some/parent/folder', '/some/other/folder/file.txt'],
+		['/some/parent/folder', '/some/parent/folder_but_not_really'],
+		['/one/path', '/another/path'],
+	])('should return false for parent %s and child %s', (parent, child) => {
+		expect(isContainedWithin(parent, child)).toBe(false);
+	});
+});

packages/cli/src/utils/path-util.ts

@@ -0,0 +1,16 @@
+import * as path from 'node:path';
+
+/**
+ * Checks if the given childPath is contained within the parentPath. Resolves
+ * the paths before comparing them, so that relative paths are also supported.
+ */
+export function isContainedWithin(parentPath: string, childPath: string): boolean {
+	parentPath = path.resolve(parentPath);
+	childPath = path.resolve(childPath);
+
+	if (parentPath === childPath) {
+		return true;
+	}
+
+	return childPath.startsWith(parentPath + path.sep);
+}