diff --git a/packages/nodes-base/nodes/Git/Git.node.ts b/packages/nodes-base/nodes/Git/Git.node.ts index e2bd7d72ca9..73888c22835 100644 --- a/packages/nodes-base/nodes/Git/Git.node.ts +++ b/packages/nodes-base/nodes/Git/Git.node.ts @@ -1,11 +1,12 @@ import { DeploymentConfig, SecurityConfig } from '@n8n/config'; import { Container } from '@n8n/di'; -import { access, mkdir } from 'fs/promises'; +import { mkdir } from 'fs/promises'; import type { IExecuteFunctions, INodeExecutionData, INodeType, INodeTypeDescription, + ResolvedFilePath, } from 'n8n-workflow'; import { NodeConnectionTypes, @@ -13,6 +14,7 @@ import { assertParamIsBoolean, assertParamIsString, } from 'n8n-workflow'; +import { basename, dirname, join } from 'path'; import type { LogOptions, SimpleGit, SimpleGitOptions } from 'simple-git'; import simpleGit from 'simple-git'; import { URL } from 'url'; @@ -295,12 +297,37 @@ export class Git implements INodeType { } }; + const isFileNotFoundError = (error: unknown) => + error instanceof Error && 'code' in error && error.code === 'ENOENT'; + + const resolvePathAllowingMissingParents = async (path: string): Promise => { + try { + return await this.helpers.resolvePath(path); + } catch (error) { + if (!isFileNotFoundError(error)) { + throw error; + } + + const parentPath = dirname(path); + if (parentPath === path) { + throw error; + } + + const resolvedParentPath = await resolvePathAllowingMissingParents(parentPath); + return join(resolvedParentPath, basename(path)) as ResolvedFilePath; + } + }; + const operation = this.getNodeParameter('operation', 0); const returnItems: INodeExecutionData[] = []; for (let itemIndex = 0; itemIndex < items.length; itemIndex++) { try { const repositoryPath = this.getNodeParameter('repositoryPath', itemIndex, '') as string; - const resolvedRepositoryPath = await this.helpers.resolvePath(repositoryPath); + const resolvedRepositoryPath = + operation === 'clone' + ? await resolvePathAllowingMissingParents(repositoryPath) + : await this.helpers.resolvePath(repositoryPath); + const isFilePathBlocked = this.helpers.isFilePathBlocked(resolvedRepositoryPath); if (isFilePathBlocked) { throw new NodeOperationError( @@ -312,12 +339,7 @@ export class Git implements INodeType { const options = this.getNodeParameter('options', itemIndex, {}); if (operation === 'clone') { - // Create repository folder if it does not exist - try { - await access(resolvedRepositoryPath); - } catch (error) { - await mkdir(resolvedRepositoryPath); - } + await mkdir(dirname(resolvedRepositoryPath), { recursive: true }); } const gitConfig: string[] = []; @@ -335,7 +357,7 @@ export class Git implements INodeType { } const gitOptions: Partial = { - baseDir: resolvedRepositoryPath, + baseDir: operation === 'clone' ? dirname(resolvedRepositoryPath) : resolvedRepositoryPath, config: gitConfig, // simple-git blocks callers from setting `core.hooksPath` via `config` // unless this flag is set. We set it deliberately as a mitigation, so @@ -410,7 +432,7 @@ export class Git implements INodeType { let sourceRepository = this.getNodeParameter('sourceRepository', itemIndex, '') as string; sourceRepository = await prepareRepository(sourceRepository); - await git.clone(sourceRepository, '.'); + await git.clone(sourceRepository, resolvedRepositoryPath); returnItems.push({ json: { diff --git a/packages/nodes-base/nodes/Git/test/Git.node.test.ts b/packages/nodes-base/nodes/Git/test/Git.node.test.ts index 5a4ec7c413f..fccb3e4d1fa 100644 --- a/packages/nodes-base/nodes/Git/test/Git.node.test.ts +++ b/packages/nodes-base/nodes/Git/test/Git.node.test.ts @@ -1,6 +1,6 @@ import * as fsPromises from 'fs/promises'; import { mock } from 'jest-mock-extended'; -import type { IExecuteFunctions } from 'n8n-workflow'; +import type { IExecuteFunctions, ResolvedFilePath } from 'n8n-workflow'; import type { SimpleGit } from 'simple-git'; import { Container } from '@n8n/di'; import { SecurityConfig } from '@n8n/config'; @@ -31,9 +31,11 @@ const mockGit = { jest.mock('simple-git', () => ({ __esModule: true, - default: () => mockGit, + default: jest.fn(() => mockGit), })); +const mockSimpleGit = jest.requireMock<{ default: jest.Mock }>('simple-git').default; + // Mock filesystem operations jest.mock('fs/promises', () => ({ access: jest.fn(), @@ -545,40 +547,69 @@ describe('Git Node', () => { expect(mockGit.addConfig).toHaveBeenCalledWith('user.name', 'test user', false); }); - it('should handle clone operation and create directory when it does not exist', async () => { + it('should handle clone operation and create the parent directory', async () => { + const missingParentError = Object.assign(new Error('Directory does not exist'), { + code: 'ENOENT', + }); + mockExecuteFunctions.getNodeParameter .mockReturnValueOnce('clone') - .mockReturnValueOnce('/new-repo') + .mockReturnValueOnce('/git/new-repo') .mockReturnValueOnce({}) .mockReturnValueOnce('https://github.com/test/repo.git'); - // Simulate directory not existing - access() throws - mockFsPromises.access.mockRejectedValueOnce(new Error('Directory does not exist')); + mockExecuteFunctions.helpers.resolvePath = jest + .fn() + .mockRejectedValueOnce(missingParentError) + .mockResolvedValueOnce('/git' as ResolvedFilePath); mockFsPromises.mkdir.mockResolvedValueOnce(undefined); const result = await gitNode.execute.call(mockExecuteFunctions); - expect(mockFsPromises.access).toHaveBeenCalledWith('/new-repo'); - expect(mockFsPromises.mkdir).toHaveBeenCalledWith('/new-repo'); - expect(mockGit.clone).toHaveBeenCalledWith('https://github.com/test/repo.git', '.'); + expect(mockFsPromises.access).not.toHaveBeenCalled(); + expect(mockExecuteFunctions.helpers.resolvePath).toHaveBeenCalledWith('/git/new-repo'); + expect(mockExecuteFunctions.helpers.resolvePath).toHaveBeenCalledWith('/git'); + expect(mockFsPromises.mkdir).toHaveBeenCalledWith('/git', { recursive: true }); + expect(mockSimpleGit).toHaveBeenCalledWith(expect.objectContaining({ baseDir: '/git' })); + expect(mockGit.clone).toHaveBeenCalledWith( + 'https://github.com/test/repo.git', + '/git/new-repo', + ); expect(result[0]).toEqual([{ json: { success: true }, pairedItem: { item: 0 } }]); }); - it('should handle clone operation when directory already exists', async () => { + it('should not create the parent directory when clone path is blocked', async () => { mockExecuteFunctions.getNodeParameter .mockReturnValueOnce('clone') - .mockReturnValueOnce('/existing-repo') + .mockReturnValueOnce('/blocked/repo') + .mockReturnValueOnce({}); + mockExecuteFunctions.helpers.isFilePathBlocked = jest.fn(() => true); + + await expect(gitNode.execute.call(mockExecuteFunctions)).rejects.toThrow( + 'Access to the repository path is not allowed', + ); + + expect(mockFsPromises.mkdir).not.toHaveBeenCalled(); + expect(mockSimpleGit).not.toHaveBeenCalled(); + expect(mockGit.clone).not.toHaveBeenCalled(); + }); + + it('should pass the resolved repository path as the clone destination', async () => { + mockExecuteFunctions.getNodeParameter + .mockReturnValueOnce('clone') + .mockReturnValueOnce('/git/existing-repo') .mockReturnValueOnce({}) .mockReturnValueOnce('https://github.com/test/repo.git'); - // Simulate directory already exists - access() succeeds - mockFsPromises.access.mockResolvedValueOnce(undefined); - await gitNode.execute.call(mockExecuteFunctions); - expect(mockFsPromises.access).toHaveBeenCalledWith('/existing-repo'); - expect(mockFsPromises.mkdir).not.toHaveBeenCalled(); - expect(mockGit.clone).toHaveBeenCalledWith('https://github.com/test/repo.git', '.'); + expect(mockFsPromises.access).not.toHaveBeenCalled(); + expect(mockFsPromises.mkdir).toHaveBeenCalledWith('/git', { recursive: true }); + expect(mockSimpleGit).toHaveBeenCalledWith(expect.objectContaining({ baseDir: '/git' })); + expect(mockGit.clone).toHaveBeenCalledWith( + 'https://github.com/test/repo.git', + '/git/existing-repo', + ); }); it('should handle fetch operation', async () => {