From 66b6b8f6dfc8af5f03a999979d09af3a4fb6772c Mon Sep 17 00:00:00 2001 From: Csaba Tuncsik Date: Fri, 29 Aug 2025 06:47:28 +0200 Subject: [PATCH] fix(core): Resolve SSH path issues on Windows (#18889) --- .../source-control-git.service.test.ts | 108 ++++++++- ...rce-control-preferences.service.ee.test.ts | 217 +++++++++++++++++- .../source-control-git.service.ee.ts | 13 +- .../source-control-preferences.service.ee.ts | 20 +- 4 files changed, 349 insertions(+), 9 deletions(-) diff --git a/packages/cli/src/environments.ee/source-control/__tests__/source-control-git.service.test.ts b/packages/cli/src/environments.ee/source-control/__tests__/source-control-git.service.test.ts index 822a462c86..2bc0b8d7e8 100644 --- a/packages/cli/src/environments.ee/source-control/__tests__/source-control-git.service.test.ts +++ b/packages/cli/src/environments.ee/source-control/__tests__/source-control-git.service.test.ts @@ -5,6 +5,7 @@ import type { SimpleGit } from 'simple-git'; import { SourceControlGitService } from '../source-control-git.service.ee'; import type { SourceControlPreferences } from '../types/source-control-preferences'; +import type { SourceControlPreferencesService } from '../source-control-preferences.service.ee'; const MOCK_BRANCHES = { all: ['origin/master', 'origin/feature/branch'], @@ -15,18 +16,21 @@ const MOCK_BRANCHES = { current: 'master', }; +const mockGitInstance = { + branch: jest.fn().mockResolvedValue(MOCK_BRANCHES), + env: jest.fn().mockReturnThis(), +}; + jest.mock('simple-git', () => { return { - simpleGit: jest.fn().mockImplementation(() => ({ - branch: jest.fn().mockResolvedValue(MOCK_BRANCHES), - })), + simpleGit: jest.fn().mockImplementation(() => mockGitInstance), }; }); describe('SourceControlGitService', () => { const sourceControlGitService = new SourceControlGitService(mock(), mock(), mock()); - beforeAll(() => { + beforeEach(() => { sourceControlGitService.git = simpleGit(); }); @@ -105,4 +109,100 @@ describe('SourceControlGitService', () => { expect(content).toBe(expectedContent); }); }); + + describe('path normalization', () => { + describe('cross-platform path handling', () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + + it('should normalize Windows paths to POSIX format for SSH command', async () => { + // Arrange + const mockPreferencesService = mock(); + const windowsPath = 'C:\\Users\\Test\\.n8n\\ssh_private_key_temp'; + const sshFolder = 'C:\\Users\\Test\\.n8n\\.ssh'; + + // Mock the getPrivateKeyPath to return a Windows path + mockPreferencesService.getPrivateKeyPath.mockResolvedValue(windowsPath); + + const gitService = new SourceControlGitService(mock(), mock(), mockPreferencesService); + + // Act + await gitService.setGitSshCommand('/git/folder', sshFolder); + + // Assert - verify Windows paths are normalized to POSIX format + expect(mockGitInstance.env).toHaveBeenCalledWith( + 'GIT_SSH_COMMAND', + expect.stringContaining('C:/Users/Test/.n8n/ssh_private_key_temp'), // Forward slashes + ); + expect(mockGitInstance.env).toHaveBeenCalledWith( + 'GIT_SSH_COMMAND', + expect.stringContaining('C:/Users/Test/.n8n/.ssh/known_hosts'), // Forward slashes + ); + // Ensure no backslashes remain in the SSH command + expect(mockGitInstance.env).toHaveBeenCalledWith( + 'GIT_SSH_COMMAND', + expect.not.stringContaining('\\'), + ); + }); + + it('should create properly quoted SSH command', async () => { + // Arrange + const mockPreferencesService = mock(); + const privateKeyPath = 'C:/Users/Test User/.n8n/ssh_private_key_temp'; + const sshFolder = 'C:/Users/Test User/.n8n/.ssh'; + + // Mock the getPrivateKeyPath to return a path with spaces + mockPreferencesService.getPrivateKeyPath.mockResolvedValue(privateKeyPath); + + const gitService = new SourceControlGitService(mock(), mock(), mockPreferencesService); + + // Act + await gitService.setGitSshCommand('/git/folder', sshFolder); + + // Assert - verify paths with spaces are properly quoted + expect(mockGitInstance.env).toHaveBeenCalledWith( + 'GIT_SSH_COMMAND', + expect.stringContaining('"C:/Users/Test User/.n8n/ssh_private_key_temp"'), // Quoted path with spaces + ); + expect(mockGitInstance.env).toHaveBeenCalledWith( + 'GIT_SSH_COMMAND', + expect.stringContaining('"C:/Users/Test User/.n8n/.ssh/known_hosts"'), // Quoted known_hosts path + ); + expect(mockGitInstance.env).toHaveBeenCalledWith( + 'GIT_SSH_COMMAND', + expect.stringContaining('UserKnownHostsFile='), + ); + expect(mockGitInstance.env).toHaveBeenCalledWith( + 'GIT_SSH_COMMAND', + expect.stringContaining('StrictHostKeyChecking=no'), + ); + }); + + it('should escape double quotes in paths to prevent command injection', async () => { + // Arrange + const mockPreferencesService = mock(); + const pathWithQuotes = 'C:/Users/Test"User/.n8n/ssh_private_key_temp'; + const sshFolder = 'C:/Users/Test"User/.n8n/.ssh'; + + // Mock the getPrivateKeyPath to return a path with quotes + mockPreferencesService.getPrivateKeyPath.mockResolvedValue(pathWithQuotes); + + const gitService = new SourceControlGitService(mock(), mock(), mockPreferencesService); + + // Act + await gitService.setGitSshCommand('/git/folder', sshFolder); + + // Assert - verify the SSH command was properly escaped + expect(mockGitInstance.env).toHaveBeenCalledWith( + 'GIT_SSH_COMMAND', + expect.stringContaining('Test\\"User'), // Escaped quote + ); + expect(mockGitInstance.env).toHaveBeenCalledWith( + 'GIT_SSH_COMMAND', + expect.not.stringContaining('Test"User'), // No unescaped quote in final command + ); + }); + }); + }); }); diff --git a/packages/cli/src/environments.ee/source-control/__tests__/source-control-preferences.service.ee.test.ts b/packages/cli/src/environments.ee/source-control/__tests__/source-control-preferences.service.ee.test.ts index d35c7ac9c7..800831a839 100644 --- a/packages/cli/src/environments.ee/source-control/__tests__/source-control-preferences.service.ee.test.ts +++ b/packages/cli/src/environments.ee/source-control/__tests__/source-control-preferences.service.ee.test.ts @@ -1,9 +1,16 @@ import { mock } from 'jest-mock-extended'; -import type { InstanceSettings } from 'n8n-core'; +import type { InstanceSettings, Cipher } from 'n8n-core'; +import { readFile, access, mkdir } from 'fs/promises'; +import os from 'os'; +import path from 'path'; import { SourceControlPreferencesService } from '../source-control-preferences.service.ee'; import type { SourceControlPreferences } from '../types/source-control-preferences'; +// Restore real fs modules for these tests since we need actual file operations +jest.unmock('node:fs'); +jest.unmock('node:fs/promises'); + describe('SourceControlPreferencesService', () => { const instanceSettings = mock({ n8nFolder: '' }); const service = new SourceControlPreferencesService( @@ -24,4 +31,212 @@ describe('SourceControlPreferencesService', () => { const validationResult = await service.validateSourceControlPreferences(validPreferences); expect(validationResult).toBeTruthy(); }); + + describe('line ending normalization', () => { + let tempDir: string; + + beforeEach(async () => { + tempDir = path.join(os.tmpdir(), 'n8n-test-' + Date.now()); + await mkdir(tempDir, { recursive: true }); + }); + + it('should normalize CRLF line endings to LF when writing private key', async () => { + // Arrange + const keyWithCRLF = + '-----BEGIN OPENSSH PRIVATE KEY-----\r\ntest\r\nkey\r\ndata\r\n-----END OPENSSH PRIVATE KEY-----\r\n'; + const expectedNormalizedKey = keyWithCRLF.replace(/\r\n/g, '\n').replace(/\r/g, '\n'); + + const mockCipher = mock(); + mockCipher.decrypt.mockReturnValue(keyWithCRLF); + + const instanceSettings = mock({ n8nFolder: tempDir }); + const service = new SourceControlPreferencesService( + instanceSettings, + mock(), + mockCipher, + mock(), + mock(), + ); + + // Mock the getKeyPairFromDatabase method to return a key pair + jest.spyOn(service as any, 'getKeyPairFromDatabase').mockResolvedValue({ + encryptedPrivateKey: 'encrypted', + publicKey: 'public', + }); + + // Act + const tempFilePath = await service.getPrivateKeyPath(); + + // Assert - check the actual file content has normalized line endings + const fileContent = await readFile(tempFilePath, 'utf8'); + expect(fileContent).not.toContain('\r'); + expect(fileContent).toBe(expectedNormalizedKey); + }); + + it('should normalize mixed CR and CRLF line endings to LF when writing private key', async () => { + // Arrange + const keyWithMixedEndings = + '-----BEGIN OPENSSH PRIVATE KEY-----\r\ntest\rkey\r\ndata\r-----END OPENSSH PRIVATE KEY-----\n'; + + const mockCipher = mock(); + mockCipher.decrypt.mockReturnValue(keyWithMixedEndings); + + const instanceSettings = mock({ n8nFolder: tempDir }); + const service = new SourceControlPreferencesService( + instanceSettings, + mock(), + mockCipher, + mock(), + mock(), + ); + + // Mock the getKeyPairFromDatabase method + jest.spyOn(service as any, 'getKeyPairFromDatabase').mockResolvedValue({ + encryptedPrivateKey: 'encrypted', + publicKey: 'public', + }); + + // Act + const tempFilePath = await service.getPrivateKeyPath(); + + // Assert - check the actual file content has normalized line endings + const fileContent = await readFile(tempFilePath, 'utf8'); + expect(fileContent).not.toContain('\r'); + expect(fileContent).toBe( + '-----BEGIN OPENSSH PRIVATE KEY-----\ntest\nkey\ndata\n-----END OPENSSH PRIVATE KEY-----\n', + ); + }); + + it('should leave existing LF line endings unchanged when writing private key', async () => { + // Arrange + const keyWithLF = + '-----BEGIN OPENSSH PRIVATE KEY-----\ntest\nkey\ndata\n-----END OPENSSH PRIVATE KEY-----\n'; + + const mockCipher = mock(); + mockCipher.decrypt.mockReturnValue(keyWithLF); + + const instanceSettings = mock({ n8nFolder: tempDir }); + const service = new SourceControlPreferencesService( + instanceSettings, + mock(), + mockCipher, + mock(), + mock(), + ); + + // Mock the getKeyPairFromDatabase method + jest.spyOn(service as any, 'getKeyPairFromDatabase').mockResolvedValue({ + encryptedPrivateKey: 'encrypted', + publicKey: 'public', + }); + + // Act + const tempFilePath = await service.getPrivateKeyPath(); + + // Assert - check the actual file content remains unchanged + const fileContent = await readFile(tempFilePath, 'utf8'); + expect(fileContent).toBe(keyWithLF); + }); + }); + + describe('file security and permissions', () => { + let tempDir: string; + + beforeEach(async () => { + tempDir = path.join(os.tmpdir(), 'n8n-test-' + Date.now()); + await mkdir(tempDir, { recursive: true }); + }); + + it('should always use restrictive permissions for SSH private keys', async () => { + // Arrange + const testKey = + '-----BEGIN OPENSSH PRIVATE KEY-----\ntest-key-content\n-----END OPENSSH PRIVATE KEY-----\n'; + + const mockCipher = mock(); + mockCipher.decrypt.mockReturnValue(testKey); + + const instanceSettings = mock({ n8nFolder: tempDir }); + const service = new SourceControlPreferencesService( + instanceSettings, + mock(), + mockCipher, + mock(), + mock(), + ); + + // Mock the getKeyPairFromDatabase method + jest.spyOn(service as any, 'getKeyPairFromDatabase').mockResolvedValue({ + encryptedPrivateKey: 'encrypted', + publicKey: 'public', + }); + + // Act + const tempFilePath = await service.getPrivateKeyPath(); + + // Assert - check the actual file permissions + const fs = require('fs'); + const stats = await fs.promises.stat(tempFilePath); + const permissions = (stats.mode & parseInt('777', 8)).toString(8); + + expect(permissions).toBe('600'); // Owner read/write only + expect(stats.mode & 0o077).toBe(0); // Group and others have no permissions + }); + + it('should fail securely when file cannot be created', async () => { + // Arrange + const instanceSettings = mock({ n8nFolder: '/nonexistent/path' }); + const service = new SourceControlPreferencesService( + instanceSettings, + mock(), + mock(), + mock(), + mock(), + ); + + const testKey = + '-----BEGIN OPENSSH PRIVATE KEY-----\ntest-key-content\n-----END OPENSSH PRIVATE KEY-----\n'; + jest.spyOn(service as any, 'getPrivateKeyFromDatabase').mockResolvedValue(testKey); + + // Act & Assert - should throw UnexpectedError when file creation fails + await expect(service.getPrivateKeyPath()).rejects.toThrow( + 'Failed to create SSH private key file', + ); + }); + + it('should remove existing file before creating new one with fsRm force option', async () => { + // Arrange + const testKey = + '-----BEGIN OPENSSH PRIVATE KEY-----\ntest-key-content\n-----END OPENSSH PRIVATE KEY-----\n'; + + const mockCipher = mock(); + mockCipher.decrypt.mockReturnValue(testKey); + + const instanceSettings = mock({ n8nFolder: tempDir }); + const service = new SourceControlPreferencesService( + instanceSettings, + mock(), + mockCipher, + mock(), + mock(), + ); + + // Mock the getKeyPairFromDatabase method + jest.spyOn(service as any, 'getKeyPairFromDatabase').mockResolvedValue({ + encryptedPrivateKey: 'encrypted', + publicKey: 'public', + }); + + // Create a test file first + const tempFilePath1 = await service.getPrivateKeyPath(); + expect(await access(tempFilePath1)).toBeUndefined(); // File exists + + // Act - call again to test file removal and recreation + const tempFilePath2 = await service.getPrivateKeyPath(); + + // Assert - should succeed without errors (fsRm with force: true handles existing file) + expect(tempFilePath2).toBe(tempFilePath1); + const fileContent = await readFile(tempFilePath2, 'utf8'); + expect(fileContent).toBe(testKey); + }); + }); }); diff --git a/packages/cli/src/environments.ee/source-control/source-control-git.service.ee.ts b/packages/cli/src/environments.ee/source-control/source-control-git.service.ee.ts index 9eb75f6a82..4d8b84ed01 100644 --- a/packages/cli/src/environments.ee/source-control/source-control-git.service.ee.ts +++ b/packages/cli/src/environments.ee/source-control/source-control-git.service.ee.ts @@ -106,7 +106,18 @@ export class SourceControlGitService { const privateKeyPath = await this.sourceControlPreferencesService.getPrivateKeyPath(); const sshKnownHosts = path.join(sshFolder, 'known_hosts'); - const sshCommand = `ssh -o UserKnownHostsFile=${sshKnownHosts} -o StrictHostKeyChecking=no -i ${privateKeyPath}`; + + // Convert paths to POSIX format for SSH command (works cross-platform) + // Use regex to handle both Windows (\) and POSIX (/) separators regardless of current platform + const normalizedPrivateKeyPath = privateKeyPath.split(/[/\\]/).join('/'); + const normalizedKnownHostsPath = sshKnownHosts.split(/[/\\]/).join('/'); + + // Escape double quotes to prevent command injection + const escapedPrivateKeyPath = normalizedPrivateKeyPath.replace(/"/g, '\\"'); + const escapedKnownHostsPath = normalizedKnownHostsPath.replace(/"/g, '\\"'); + + // Quote paths to handle spaces and special characters + const sshCommand = `ssh -o UserKnownHostsFile="${escapedKnownHostsPath}" -o StrictHostKeyChecking=no -i "${escapedPrivateKeyPath}"`; this.gitOptions = { baseDir: gitFolder, diff --git a/packages/cli/src/environments.ee/source-control/source-control-preferences.service.ee.ts b/packages/cli/src/environments.ee/source-control/source-control-preferences.service.ee.ts index 2caf0eb4b5..5ebeb322a0 100644 --- a/packages/cli/src/environments.ee/source-control/source-control-preferences.service.ee.ts +++ b/packages/cli/src/environments.ee/source-control/source-control-preferences.service.ee.ts @@ -6,7 +6,7 @@ import { validate } from 'class-validator'; import { rm as fsRm } from 'fs/promises'; import { Cipher, InstanceSettings } from 'n8n-core'; import { jsonParse, UnexpectedError } from 'n8n-workflow'; -import { writeFile, chmod, readFile } from 'node:fs/promises'; +import { writeFile, readFile } from 'node:fs/promises'; import path from 'path'; import { @@ -96,9 +96,23 @@ export class SourceControlPreferencesService { const tempFilePath = path.join(this.instanceSettings.n8nFolder, 'ssh_private_key_temp'); - await writeFile(tempFilePath, dbPrivateKey); + // Ensure proper line endings (LF only) for SSH keys, especially on Windows + const normalizedKey = dbPrivateKey.replace(/\r\n/g, '\n').replace(/\r/g, '\n'); - await chmod(tempFilePath, 0o600); + try { + // Remove existing file if it exists to avoid permission conflicts + // Using force: true ignores ENOENT but allows other errors to surface + await fsRm(tempFilePath, { force: true }); + + // Always use restrictive permissions for SSH private keys (security requirement) + await writeFile(tempFilePath, normalizedKey, { mode: 0o600 }); + } catch (error) { + this.logger.error('Failed to write SSH private key to temp file', { + tempFilePath, + error: error instanceof Error ? error.message : String(error), + }); + throw new UnexpectedError('Failed to create SSH private key file', { cause: error }); + } return tempFilePath; }