diff --git a/packages/cli/src/environments/source-control/__tests__/source-control.service.test.ts b/packages/cli/src/environments/source-control/__tests__/source-control.service.test.ts new file mode 100644 index 0000000000..06dab4f97c --- /dev/null +++ b/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'); + }); + }); +}); diff --git a/packages/cli/src/environments/source-control/source-control-helper.ee.ts b/packages/cli/src/environments/source-control/source-control-helper.ee.ts index 35298d560a..00a9875741 100644 --- a/packages/cli/src/environments/source-control/source-control-helper.ee.ts +++ b/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; +} diff --git a/packages/cli/src/environments/source-control/source-control.controller.ee.ts b/packages/cli/src/environments/source-control/source-control.controller.ee.ts index 7e41fd7cae..7b8b2a7266 100644 --- a/packages/cli/src/environments/source-control/source-control.controller.ee.ts +++ b/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}`, diff --git a/packages/cli/src/environments/source-control/source-control.service.ee.ts b/packages/cli/src/environments/source-control/source-control.service.ee.ts index 32f0b39ef7..58c213f03c 100644 --- a/packages/cli/src/environments/source-control/source-control.service.ee.ts +++ b/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 { + public async sanityCheck(): Promise { 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(); const filesToBeDeleted = new Set(); - 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; } } diff --git a/packages/cli/src/utils/__tests__/path-util.test.ts b/packages/cli/src/utils/__tests__/path-util.test.ts new file mode 100644 index 0000000000..d67e97f9e0 --- /dev/null +++ b/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); + }); +}); diff --git a/packages/cli/src/utils/path-util.ts b/packages/cli/src/utils/path-util.ts new file mode 100644 index 0000000000..f42dc01890 --- /dev/null +++ b/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); +}