Skip to content

🔒 Fix command injection in editor diff tool verification #64

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

Draft
TrueAlpha-spiral wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

🔒 Fix command injection in editor diff tool verification #64

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
57 changes: 29 additions & 28 deletions packages/core/src/utils/editor.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,9 +21,10 @@ import {
isEditorAvailable,
type EditorType,
} from './editor.js';
import { execSync, spawn, spawnSync } from 'child_process';
import { execFileSync, execSync, spawn, spawnSync } from 'child_process';

vi.mock('child_process', () => ({
execFileSync: vi.fn(),
execSync: vi.fn(),
spawn: vi.fn(),
spawnSync: vi.fn(),
Expand Down Expand Up @@ -79,46 +80,46 @@ describe('editor utils', () => {
// Non-windows tests
it(`should return true if first command "${commands[0]}" exists on non-windows`, () => {
Object.defineProperty(process, 'platform', { value: 'linux' });
(execSync as Mock).mockReturnValue(
(execFileSync as Mock).mockReturnValue(
Buffer.from(`/usr/bin/${commands[0]}`),
);
expect(checkHasEditorType(editor)).toBe(true);
expect(execSync).toHaveBeenCalledWith(`command -v ${commands[0]}`, {
expect(execFileSync).toHaveBeenCalledWith('which', [commands[0]], {
stdio: 'ignore',
});
});

if (commands.length > 1) {
it(`should return true if first command doesn't exist but second command "${commands[1]}" exists on non-windows`, () => {
Object.defineProperty(process, 'platform', { value: 'linux' });
(execSync as Mock)
(execFileSync as Mock)
.mockImplementationOnce(() => {
throw new Error(); // first command not found
})
.mockReturnValueOnce(Buffer.from(`/usr/bin/${commands[1]}`)); // second command found
expect(checkHasEditorType(editor)).toBe(true);
expect(execSync).toHaveBeenCalledTimes(2);
expect(execFileSync).toHaveBeenCalledTimes(2);
});
}

it(`should return false if none of the commands exist on non-windows`, () => {
Object.defineProperty(process, 'platform', { value: 'linux' });
(execSync as Mock).mockImplementation(() => {
(execFileSync as Mock).mockImplementation(() => {
throw new Error(); // all commands not found
});
expect(checkHasEditorType(editor)).toBe(false);
expect(execSync).toHaveBeenCalledTimes(commands.length);
expect(execFileSync).toHaveBeenCalledTimes(commands.length);
});

// Windows tests
it(`should return true if first command "${win32Commands[0]}" exists on windows`, () => {
Object.defineProperty(process, 'platform', { value: 'win32' });
(execSync as Mock).mockReturnValue(
(execFileSync as Mock).mockReturnValue(
Buffer.from(`C:\\Program Files\\...\\${win32Commands[0]}`),
);
expect(checkHasEditorType(editor)).toBe(true);
expect(execSync).toHaveBeenCalledWith(
`where.exe ${win32Commands[0]}`,
expect(execFileSync).toHaveBeenCalledWith(
'where.exe', [win32Commands[0]],
{
stdio: 'ignore',
},
Expand All @@ -128,25 +129,25 @@ describe('editor utils', () => {
if (win32Commands.length > 1) {
it(`should return true if first command doesn't exist but second command "${win32Commands[1]}" exists on windows`, () => {
Object.defineProperty(process, 'platform', { value: 'win32' });
(execSync as Mock)
(execFileSync as Mock)
.mockImplementationOnce(() => {
throw new Error(); // first command not found
})
.mockReturnValueOnce(
Buffer.from(`C:\\Program Files\\...\\${win32Commands[1]}`),
); // second command found
expect(checkHasEditorType(editor)).toBe(true);
expect(execSync).toHaveBeenCalledTimes(2);
expect(execFileSync).toHaveBeenCalledTimes(2);
});
}

it(`should return false if none of the commands exist on windows`, () => {
Object.defineProperty(process, 'platform', { value: 'win32' });
(execSync as Mock).mockImplementation(() => {
(execFileSync as Mock).mockImplementation(() => {
throw new Error(); // all commands not found
});
expect(checkHasEditorType(editor)).toBe(false);
expect(execSync).toHaveBeenCalledTimes(win32Commands.length);
expect(execFileSync).toHaveBeenCalledTimes(win32Commands.length);
});
});
}
Expand Down Expand Up @@ -177,7 +178,7 @@ describe('editor utils', () => {
// Non-windows tests
it(`should use first command "${commands[0]}" when it exists on non-windows`, () => {
Object.defineProperty(process, 'platform', { value: 'linux' });
(execSync as Mock).mockReturnValue(
(execFileSync as Mock).mockReturnValue(
Buffer.from(`/usr/bin/${commands[0]}`),
);
const diffCommand = getDiffCommand('old.txt', 'new.txt', editor);
Expand All @@ -190,7 +191,7 @@ describe('editor utils', () => {
if (commands.length > 1) {
it(`should use second command "${commands[1]}" when first doesn't exist on non-windows`, () => {
Object.defineProperty(process, 'platform', { value: 'linux' });
(execSync as Mock)
(execFileSync as Mock)
.mockImplementationOnce(() => {
throw new Error(); // first command not found
})
Expand All @@ -206,7 +207,7 @@ describe('editor utils', () => {

it(`should fall back to last command "${commands[commands.length - 1]}" when none exist on non-windows`, () => {
Object.defineProperty(process, 'platform', { value: 'linux' });
(execSync as Mock).mockImplementation(() => {
(execFileSync as Mock).mockImplementation(() => {
throw new Error(); // all commands not found
});

Expand All @@ -220,7 +221,7 @@ describe('editor utils', () => {
// Windows tests
it(`should use first command "${win32Commands[0]}" when it exists on windows`, () => {
Object.defineProperty(process, 'platform', { value: 'win32' });
(execSync as Mock).mockReturnValue(
(execFileSync as Mock).mockReturnValue(
Buffer.from(`C:\\Program Files\\...\\${win32Commands[0]}`),
);
const diffCommand = getDiffCommand('old.txt', 'new.txt', editor);
Expand All @@ -233,7 +234,7 @@ describe('editor utils', () => {
if (win32Commands.length > 1) {
it(`should use second command "${win32Commands[1]}" when first doesn't exist on windows`, () => {
Object.defineProperty(process, 'platform', { value: 'win32' });
(execSync as Mock)
(execFileSync as Mock)
.mockImplementationOnce(() => {
throw new Error(); // first command not found
})
Expand All @@ -251,7 +252,7 @@ describe('editor utils', () => {

it(`should fall back to last command "${win32Commands[win32Commands.length - 1]}" when none exist on windows`, () => {
Object.defineProperty(process, 'platform', { value: 'win32' });
(execSync as Mock).mockImplementation(() => {
(execFileSync as Mock).mockImplementation(() => {
throw new Error(); // all commands not found
});

Expand Down Expand Up @@ -398,8 +399,8 @@ describe('editor utils', () => {
for (const editor of terminalEditors) {
it(`should call spawnSync for ${editor}`, async () => {
await openDiff('old.txt', 'new.txt', editor, () => {});
// execSync should NOT be called
expect(execSync).toHaveBeenCalledTimes(0);
// execFileSync should NOT be called
expect(execFileSync).toHaveBeenCalledTimes(0);

// spawnSync SHOULD be called
expect(spawnSync).toHaveBeenCalledTimes(1);
Expand Down Expand Up @@ -523,37 +524,37 @@ describe('editor utils', () => {
});

it('should return true for vscode when installed and not in sandbox mode', () => {
(execSync as Mock).mockReturnValue(Buffer.from('/usr/bin/code'));
(execFileSync as Mock).mockReturnValue(Buffer.from('/usr/bin/code'));
expect(isEditorAvailable('vscode')).toBe(true);
});

it('should return false for vscode when not installed and not in sandbox mode', () => {
(execSync as Mock).mockImplementation(() => {
(execFileSync as Mock).mockImplementation(() => {
throw new Error();
});
expect(isEditorAvailable('vscode')).toBe(false);
});

it('should return false for vscode when installed and in sandbox mode', () => {
(execSync as Mock).mockReturnValue(Buffer.from('/usr/bin/code'));
(execFileSync as Mock).mockReturnValue(Buffer.from('/usr/bin/code'));
process.env.SANDBOX = 'sandbox';
expect(isEditorAvailable('vscode')).toBe(false);
});

it('should return true for vim when installed and in sandbox mode', () => {
(execSync as Mock).mockReturnValue(Buffer.from('/usr/bin/vim'));
(execFileSync as Mock).mockReturnValue(Buffer.from('/usr/bin/vim'));
process.env.SANDBOX = 'sandbox';
expect(isEditorAvailable('vim')).toBe(true);
});

it('should return true for emacs when installed and in sandbox mode', () => {
(execSync as Mock).mockReturnValue(Buffer.from('/usr/bin/emacs'));
(execFileSync as Mock).mockReturnValue(Buffer.from('/usr/bin/emacs'));
process.env.SANDBOX = 'sandbox';
expect(isEditorAvailable('emacs')).toBe(true);
});

it('should return true for neovim when installed and in sandbox mode', () => {
(execSync as Mock).mockReturnValue(Buffer.from('/usr/bin/nvim'));
(execFileSync as Mock).mockReturnValue(Buffer.from('/usr/bin/nvim'));
process.env.SANDBOX = 'sandbox';
expect(isEditorAvailable('neovim')).toBe(true);
});
Expand Down
11 changes: 6 additions & 5 deletions packages/core/src/utils/editor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* SPDX-License-Identifier: Apache-2.0
*/

import { execSync, spawn, spawnSync } from 'child_process';
import { execFileSync, spawn, spawnSync } from 'child_process';

export type EditorType =
| 'vscode'
Expand Down Expand Up @@ -36,10 +36,11 @@ interface DiffCommand {

function commandExists(cmd: string): boolean {
try {
execSync(
process.platform === 'win32' ? `where.exe ${cmd}` : `command -v ${cmd}`,
{ stdio: 'ignore' },
);
if (process.platform === 'win32') {
execFileSync('where.exe', [cmd], { stdio: 'ignore' });
} else {
execFileSync('which', [cmd], { stdio: 'ignore' });
}
return true;
} catch {
return false;
Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/utils/errors.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import {
BadRequestError,
UnauthorizedError,
ForbiddenError,
} from './errors';
} from './errors.js';

describe('toFriendlyError', () => {
it('should return the original error if it is not an object', () => {
Expand Down