Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(remote): Replace exec with execFile for improved security #270

Merged
merged 1 commit into from
Jan 6, 2025
+119 −67
Merged

feat(remote): Replace exec with execFile for improved security #270

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 32 additions & 12 deletions src/core/file/gitCommand.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,34 @@
import { exec } from 'node:child_process';
import { execFile } from 'node:child_process';
import fs from 'node:fs/promises';
import path from 'node:path';
import { promisify } from 'node:util';
import { logger } from '../../shared/logger.js';

const execAsync = promisify(exec);
const execFileAsync = promisify(execFile);

export function isValidRemoteUrl(url: string): boolean {
// Check the short form of the GitHub URL. e.g. yamadashy/repomix
const shortFormRegex = /^[a-zA-Z0-9_-]+\/[a-zA-Z0-9_-]+$/;
if (shortFormRegex.test(url)) {
return true;
}

// Check the direct form of the GitHub URL. e.g. https://github.com/yamadashy/repomix or https://gist.github.com/yamadashy/1234567890abcdef
try {
new URL(url);
return true;
} catch (error) {
return false;
}
}

export const isGitInstalled = async (
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
export const isGitInstalled = async (
/**
* Checks if Git is installed by executing the 'git --version' command.
*
* @param {Object} deps - Dependencies that can be injected for testing.
* @param {Function} deps.execFileAsync - Function to execute a file asynchronously.
* @returns {Promise<boolean>} - Returns true if Git is installed, false otherwise.
*/
export const isGitInstalled = async (

add docstring to isGitInstalled function

deps = {
execAsync,
execFileAsync,
},
) => {
try {
const result = await deps.execAsync('git --version');
const result = await deps.execFileAsync('git', ['--version']);
yamadashy marked this conversation as resolved.
Show resolved Hide resolved
return !result.stderr;
} catch (error) {
logger.trace('Git is not installed:', (error as Error).message);
Expand All @@ -25,15 +41,19 @@
directory: string,
remoteBranch?: string,
deps = {
execAsync,
execFileAsync,
},
) => {
if (!isValidRemoteUrl(url)) {
throw new Error('Invalid repository URL or user/repo format');
}

if (remoteBranch) {
await deps.execAsync(`git -C ${directory} init`);
await deps.execAsync(`git -C ${directory} remote add origin ${url}`);
await deps.execFileAsync('git', ['-C', directory, 'init']);
await deps.execFileAsync('git', ['-C', directory, 'remote', 'add', 'origin', url]);
yamadashy marked this conversation as resolved.
Show resolved Hide resolved
try {
await deps.execAsync(`git -C ${directory} fetch --depth 1 origin ${remoteBranch}`);
await deps.execAsync(`git -C ${directory} checkout FETCH_HEAD`);
await deps.execFileAsync('git', ['-C', directory, 'fetch', '--depth', '1', 'origin', remoteBranch]);
await deps.execFileAsync('git', ['-C', directory, 'checkout', 'FETCH_HEAD']);
} catch (err: unknown) {
// git fetch --depth 1 origin <short SHA> always throws "couldn't find remote ref" error
const isRefNotfoundError =
Expand All @@ -56,11 +76,11 @@

// Maybe the error is due to a short SHA, let's try again
// Can't use --depth 1 here as we need to fetch the specific commit
await deps.execAsync(`git -C ${directory} fetch origin`);
await deps.execAsync(`git -C ${directory} checkout ${remoteBranch}`);
await deps.execFileAsync('git', ['-C', directory, 'fetch', 'origin']);
await deps.execFileAsync('git', ['-C', directory, 'checkout', remoteBranch]);
}
} else {
await deps.execAsync(`git clone --depth 1 ${url} ${directory}`);
await deps.execFileAsync('git', ['clone', '--depth', '1', url, directory]);
Dismissed Show dismissed Hide dismissed
yamadashy marked this conversation as resolved.
Show resolved Hide resolved
}

// Clean up .git directory
Expand Down
136 changes: 84 additions & 52 deletions tests/core/file/gitCommand.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,75 +11,85 @@ describe('gitCommand', () => {

describe('isGitInstalled', () => {
test('should return true when git is installed', async () => {
const mockExecAsync = vi.fn().mockResolvedValue({ stdout: 'git version 2.34.1', stderr: '' });
const mockFileExecAsync = vi.fn().mockResolvedValue({ stdout: 'git version 2.34.1', stderr: '' });

const result = await isGitInstalled({ execAsync: mockExecAsync });
const result = await isGitInstalled({ execFileAsync: mockFileExecAsync });

expect(result).toBe(true);
expect(mockExecAsync).toHaveBeenCalledWith('git --version');
expect(mockFileExecAsync).toHaveBeenCalledWith('git', ['--version']);
});

test('should return false when git command fails', async () => {
const mockExecAsync = vi.fn().mockRejectedValue(new Error('Command not found: git'));
const mockFileExecAsync = vi.fn().mockRejectedValue(new Error('Command not found: git'));

const result = await isGitInstalled({ execAsync: mockExecAsync });
const result = await isGitInstalled({ execFileAsync: mockFileExecAsync });

expect(result).toBe(false);
expect(mockExecAsync).toHaveBeenCalledWith('git --version');
expect(mockFileExecAsync).toHaveBeenCalledWith('git', ['--version']);
expect(logger.trace).toHaveBeenCalledWith('Git is not installed:', 'Command not found: git');
});

test('should return false when git command returns stderr', async () => {
const mockExecAsync = vi.fn().mockResolvedValue({ stdout: '', stderr: 'git: command not found' });
const mockFileExecAsync = vi.fn().mockResolvedValue({ stdout: '', stderr: 'git: command not found' });

const result = await isGitInstalled({ execAsync: mockExecAsync });
const result = await isGitInstalled({ execFileAsync: mockFileExecAsync });

expect(result).toBe(false);
expect(mockExecAsync).toHaveBeenCalledWith('git --version');
expect(mockFileExecAsync).toHaveBeenCalledWith('git', ['--version']);
});
});

describe('execGitShallowClone', () => {
test('should execute without branch option if not specified by user', async () => {
const mockExecAsync = vi.fn().mockResolvedValue({ stdout: '', stderr: '' });
const mockFileExecAsync = vi.fn().mockResolvedValue({ stdout: '', stderr: '' });
const url = 'https://github.com/user/repo.git';
const directory = '/tmp/repo';
const remoteBranch = undefined;

await execGitShallowClone(url, directory, remoteBranch, { execAsync: mockExecAsync });
await execGitShallowClone(url, directory, remoteBranch, { execFileAsync: mockFileExecAsync });

expect(mockExecAsync).toHaveBeenCalledWith(`git clone --depth 1 ${url} ${directory}`);
expect(mockFileExecAsync).toHaveBeenCalledWith('git', ['clone', '--depth', '1', url, directory]);
});

test('should throw error when git clone fails', async () => {
const mockExecAsync = vi.fn().mockRejectedValue(new Error('Authentication failed'));
const mockFileExecAsync = vi.fn().mockRejectedValue(new Error('Authentication failed'));
const url = 'https://github.com/user/repo.git';
const directory = '/tmp/repo';
const remoteBranch = undefined;

await expect(execGitShallowClone(url, directory, remoteBranch, { execAsync: mockExecAsync })).rejects.toThrow(
'Authentication failed',
);
await expect(
execGitShallowClone(url, directory, remoteBranch, { execFileAsync: mockFileExecAsync }),
).rejects.toThrow('Authentication failed');

expect(mockExecAsync).toHaveBeenCalledWith(`git clone --depth 1 ${url} ${directory}`);
expect(mockFileExecAsync).toHaveBeenCalledWith('git', ['clone', '--depth', '1', url, directory]);
});

test('should execute commands correctly when branch is specified', async () => {
const mockExecAsync = vi.fn().mockResolvedValue({ stdout: '', stderr: '' });
const mockFileExecAsync = vi.fn().mockResolvedValue({ stdout: '', stderr: '' });

const url = 'https://github.com/user/repo.git';
const directory = '/tmp/repo';
const remoteBranch = 'main';

await execGitShallowClone(url, directory, remoteBranch, { execAsync: mockExecAsync });

expect(mockExecAsync).toHaveBeenCalledTimes(4);
expect(mockExecAsync).toHaveBeenNthCalledWith(1, `git -C ${directory} init`);
expect(mockExecAsync).toHaveBeenNthCalledWith(2, `git -C ${directory} remote add origin ${url}`);
expect(mockExecAsync).toHaveBeenNthCalledWith(3, `git -C ${directory} fetch --depth 1 origin ${remoteBranch}`);
expect(mockExecAsync).toHaveBeenNthCalledWith(4, `git -C ${directory} checkout FETCH_HEAD`);
await execGitShallowClone(url, directory, remoteBranch, { execFileAsync: mockFileExecAsync });

expect(mockFileExecAsync).toHaveBeenCalledTimes(4);
expect(mockFileExecAsync).toHaveBeenNthCalledWith(1, 'git', ['-C', directory, 'init']);
expect(mockFileExecAsync).toHaveBeenNthCalledWith(2, 'git', ['-C', directory, 'remote', 'add', 'origin', url]);
expect(mockFileExecAsync).toHaveBeenNthCalledWith(3, 'git', [
'-C',
directory,
'fetch',
'--depth',
'1',
'origin',
remoteBranch,
]);
expect(mockFileExecAsync).toHaveBeenNthCalledWith(4, 'git', ['-C', directory, 'checkout', 'FETCH_HEAD']);
});

test('should throw error when git fetch fails', async () => {
const mockExecAsync = vi
const mockFileExecAsync = vi
.fn()
.mockResolvedValueOnce('Success on first call')
.mockResolvedValueOnce('Success on second call')
Expand All @@ -89,21 +99,28 @@ describe('gitCommand', () => {
const directory = '/tmp/repo';
const remoteBranch = 'b188a6cb39b512a9c6da7235b880af42c78ccd0d';

await expect(execGitShallowClone(url, directory, remoteBranch, { execAsync: mockExecAsync })).rejects.toThrow(
'Authentication failed',
);
expect(mockExecAsync).toHaveBeenCalledTimes(3);
expect(mockExecAsync).toHaveBeenNthCalledWith(1, `git -C ${directory} init`);
expect(mockExecAsync).toHaveBeenNthCalledWith(2, `git -C ${directory} remote add origin ${url}`);
expect(mockExecAsync).toHaveBeenLastCalledWith(`git -C ${directory} fetch --depth 1 origin ${remoteBranch}`);
// The fourth call (e.g., git checkout) won't be made due to the error on the third call
await expect(
execGitShallowClone(url, directory, remoteBranch, { execFileAsync: mockFileExecAsync }),
).rejects.toThrow('Authentication failed');
expect(mockFileExecAsync).toHaveBeenCalledTimes(3);
expect(mockFileExecAsync).toHaveBeenNthCalledWith(1, 'git', ['-C', directory, 'init']);
expect(mockFileExecAsync).toHaveBeenNthCalledWith(2, 'git', ['-C', directory, 'remote', 'add', 'origin', url]);
expect(mockFileExecAsync).toHaveBeenLastCalledWith('git', [
'-C',
directory,
'fetch',
'--depth',
'1',
'origin',
remoteBranch,
]);
});

test('should handle short SHA correctly', async () => {
const url = 'https://github.com/user/repo.git';
const directory = '/tmp/repo';
const shortSha = 'ce9b621';
const mockExecAsync = vi
const mockFileExecAsync = vi
.fn()
.mockResolvedValueOnce('Success on first call')
.mockResolvedValueOnce('Success on second call')
Expand All @@ -113,14 +130,22 @@ describe('gitCommand', () => {
),
);

await execGitShallowClone(url, directory, shortSha, { execAsync: mockExecAsync });

expect(mockExecAsync).toHaveBeenCalledTimes(5);
expect(mockExecAsync).toHaveBeenNthCalledWith(1, `git -C ${directory} init`);
expect(mockExecAsync).toHaveBeenNthCalledWith(2, `git -C ${directory} remote add origin ${url}`);
expect(mockExecAsync).toHaveBeenNthCalledWith(3, `git -C ${directory} fetch --depth 1 origin ${shortSha}`);
expect(mockExecAsync).toHaveBeenNthCalledWith(4, `git -C ${directory} fetch origin`);
expect(mockExecAsync).toHaveBeenLastCalledWith(`git -C ${directory} checkout ${shortSha}`);
await execGitShallowClone(url, directory, shortSha, { execFileAsync: mockFileExecAsync });

expect(mockFileExecAsync).toHaveBeenCalledTimes(5);
expect(mockFileExecAsync).toHaveBeenNthCalledWith(1, 'git', ['-C', directory, 'init']);
expect(mockFileExecAsync).toHaveBeenNthCalledWith(2, 'git', ['-C', directory, 'remote', 'add', 'origin', url]);
expect(mockFileExecAsync).toHaveBeenNthCalledWith(3, 'git', [
'-C',
directory,
'fetch',
'--depth',
'1',
'origin',
shortSha,
]);
expect(mockFileExecAsync).toHaveBeenNthCalledWith(4, 'git', ['-C', directory, 'fetch', 'origin']);
expect(mockFileExecAsync).toHaveBeenLastCalledWith('git', ['-C', directory, 'checkout', shortSha]);
});

test("should throw error when remote ref is not found, and it's not due to short SHA", async () => {
Expand All @@ -129,20 +154,27 @@ describe('gitCommand', () => {
const remoteBranch = 'b188a6cb39b512a9c6da7235b880af42c78ccd0d';
const errMessage = `Command failed: git fetch --depth 1 origin ${remoteBranch}\nfatal: couldn't find remote ref ${remoteBranch}`;

const mockExecAsync = vi
const mockFileExecAsync = vi
.fn()
.mockResolvedValueOnce('Success on first call')
.mockResolvedValueOnce('Success on second call')
.mockRejectedValueOnce(new Error(errMessage));

await expect(execGitShallowClone(url, directory, remoteBranch, { execAsync: mockExecAsync })).rejects.toThrow(
errMessage,
);
expect(mockExecAsync).toHaveBeenCalledTimes(3);
expect(mockExecAsync).toHaveBeenNthCalledWith(1, `git -C ${directory} init`);
expect(mockExecAsync).toHaveBeenNthCalledWith(2, `git -C ${directory} remote add origin ${url}`);
expect(mockExecAsync).toHaveBeenLastCalledWith(`git -C ${directory} fetch --depth 1 origin ${remoteBranch}`);
// The fourth call (e.g., git checkout) won't be made due to the error on the third call
await expect(
execGitShallowClone(url, directory, remoteBranch, { execFileAsync: mockFileExecAsync }),
).rejects.toThrow(errMessage);
expect(mockFileExecAsync).toHaveBeenCalledTimes(3);
expect(mockFileExecAsync).toHaveBeenNthCalledWith(1, 'git', ['-C', directory, 'init']);
expect(mockFileExecAsync).toHaveBeenNthCalledWith(2, 'git', ['-C', directory, 'remote', 'add', 'origin', url]);
expect(mockFileExecAsync).toHaveBeenLastCalledWith('git', [
'-C',
directory,
'fetch',
'--depth',
'1',
'origin',
remoteBranch,
]);
});
});
});
4 changes: 2 additions & 2 deletions website/client/.vitepress/theme/components/TryIt.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
import { AlertTriangle } from 'lucide-vue-next';
import { computed, ref } from 'vue';
import ResultViewer from './ResultViewer.vue';
import { isValidateRemoteUrl, packRepository } from './api/client';
import { isValidRemoteUrl, packRepository } from './api/client';
import type { PackResult } from './api/client';
import { AnalyticsAction, analyticsUtils } from './utils/analytics';

Expand All @@ -26,7 +26,7 @@ const hasExecuted = ref(false);
// URL validation
const isValidUrl = computed(() => {
if (!url.value) return false;
return isValidateRemoteUrl(url.value.trim());
return isValidRemoteUrl(url.value.trim());
});

const TIMEOUT_MS = 30000;
Expand Down
2 changes: 1 addition & 1 deletion website/client/.vitepress/theme/components/api/client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ export async function packRepository(request: PackRequest): Promise<PackResult>
return data as PackResult;
}

export function isValidateRemoteUrl(url: string): boolean {
export function isValidRemoteUrl(url: string): boolean {
// Check the direct form of the GitHub URL. e.g. https://github.com/yamadashy/repomix
const githubUrlRegex = /^https:\/\/github\.com\/[a-zA-Z0-9_-]+\/[a-zA-Z0-9_-]+$/;
if (githubUrlRegex.test(url)) {
Expand Down
Loading