From 69bb9d600cba122d3a39f8e465803a379a25bb63 Mon Sep 17 00:00:00 2001 From: Greg Priday Date: Thu, 26 Feb 2026 13:54:18 +1100 Subject: [PATCH] fix(clipboard): fix Windows escaping, macOS/Linux shell injection, and Clipboard.copy typo MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Fix Clipboard.copy() → Clipboard.copyText() in api/copy.js (TypeError fix) - Windows copyFileReference: replace Set-Clipboard -Path (glob vulnerable) with Windows Forms SetFileDropList via Base64-encoded PowerShell; eliminates bracket wildcard expansion and bypasses all shell quoting at the Node→PS boundary - macOS copyFileReference + revealInFinder: switch from execSync(osascript -e '...') to spawnSync(['osascript', '-e', script]); eliminates single-quote shell injection - Windows revealInFinder: already used spawnSync; no change needed - Linux revealInFinder: switch from execSync(xdg-open "...") to spawnSync(['xdg-open', dir]); eliminates $-expansion and double-quote injection - Fix clipboardy mock: add mock.default = mock so dynamic ESM imports resolve correctly - Add 23 unit tests in tests/unit/utils/clipboard.test.js covering all platforms and special-character edge cases - Add 2 tests to copy.test.js: clipboard success path and error suppression branch --- src/api/copy.js | 2 +- src/utils/clipboard.js | 56 ++++--- tests/setup-global-mocks.js | 20 ++- tests/unit/api/copy.test.js | 24 +++ tests/unit/utils/clipboard.test.js | 254 +++++++++++++++++++++++++++++ 5 files changed, 323 insertions(+), 33 deletions(-) create mode 100644 tests/unit/utils/clipboard.test.js diff --git a/src/api/copy.js b/src/api/copy.js index 9f78b5b..fe9ab95 100644 --- a/src/api/copy.js +++ b/src/api/copy.js @@ -185,7 +185,7 @@ export async function copy(basePath, options = {}) { // Copy to clipboard if explicitly requested if (options.clipboard) { try { - await Clipboard.copy(output); + await Clipboard.copyText(output); } catch (error) { // Don't fail the operation if clipboard copy fails result.stats.clipboardError = error.message; diff --git a/src/utils/clipboard.js b/src/utils/clipboard.js index 2466275..da6558e 100644 --- a/src/utils/clipboard.js +++ b/src/utils/clipboard.js @@ -1,4 +1,4 @@ -import { execSync } from 'child_process'; +import { execSync, spawnSync } from 'child_process'; import process from 'process'; import url from 'url'; import path from 'path'; @@ -27,9 +27,21 @@ class Clipboard { static async copyFileReference(filePath) { if (process.platform === 'win32') { try { - // Use PowerShell to copy the file reference on Windows - const command = `powershell -Command "Set-Clipboard -Path '${filePath.replace(/'/g, "''")}'"`; - execSync(command, { stdio: 'pipe' }); + // Use Windows Forms SetFileDropList via Base64-encoded PowerShell command. + // This bypasses all shell quoting (Base64 encoding) and path glob expansion + // (StringCollection is passed to SetFileDropList without any wildcard interpretation). + // Single quotes are doubled for the PS single-quoted string literal. + const psPath = filePath.replace(/'/g, "''"); + const psCommand = [ + 'Add-Type -AssemblyName System.Windows.Forms', + '$fc = New-Object System.Collections.Specialized.StringCollection', + `[void]$fc.Add('${psPath}')`, + '[System.Windows.Forms.Clipboard]::SetFileDropList($fc)', + ].join('; '); + const encoded = Buffer.from(psCommand, 'utf16le').toString('base64'); + execSync(`powershell -NoProfile -NonInteractive -EncodedCommand ${encoded}`, { + stdio: 'pipe', + }); } catch (error) { logger.debug( 'Failed to copy file reference using PowerShell, falling back to text:', @@ -48,17 +60,13 @@ class Clipboard { } } else if (process.platform === 'darwin') { try { - // Use AppleScript to copy file reference + // Use AppleScript to copy file reference. spawnSync passes the script as a + // direct argument (no shell), so single quotes in filePath are safe. const escapedFilePath = filePath.replace(/\\/g, '\\\\').replace(/"/g, '\\"'); - const script = ` - set aFile to POSIX file "${escapedFilePath}" - tell app "Finder" to set the clipboard to aFile - `.trim(); + const script = `set aFile to POSIX file "${escapedFilePath}" +tell app "Finder" to set the clipboard to aFile`; - execSync(`osascript -e '${script}'`, { - encoding: 'utf8', - stdio: 'pipe', - }); + spawnSync('osascript', ['-e', script], { encoding: 'utf8', stdio: 'pipe' }); } catch (error) { logger.debug('Failed to copy file reference, falling back to text:', error.message); // Fallback to copying path as text @@ -78,30 +86,28 @@ class Clipboard { static async revealInFinder(filePath) { if (process.platform === 'win32') { try { - execSync(`explorer /select,"${filePath}"`, { stdio: 'pipe' }); + // Use spawnSync with array args to avoid cmd.exe interpreting &, |, <, >, ^ in paths + spawnSync('explorer', [`/select,${filePath}`], { stdio: 'pipe' }); } catch (error) { logger.debug('Failed to reveal file in Explorer:', error.message); } } else if (process.platform === 'linux') { try { - // Use xdg-open to open the parent directory + // Use spawnSync with array args to avoid shell interpreting $, ", ` in paths const dir = path.dirname(filePath); - execSync(`xdg-open "${dir}"`, { stdio: 'pipe' }); + spawnSync('xdg-open', [dir], { stdio: 'pipe' }); } catch (error) { logger.debug('Failed to reveal file with xdg-open:', error.message); } } else if (process.platform === 'darwin') { try { - const escapedFilePath = filePath.replace(/"/g, '"'); - const script = ` - tell application "Finder" to reveal POSIX file "${escapedFilePath}" - tell application "Finder" to activate - `.trim(); + // Use AppleScript to reveal file. spawnSync passes the script as a direct + // argument (no shell), so single quotes in filePath are safe. + const escapedFilePath = filePath.replace(/\\/g, '\\\\').replace(/"/g, '\\"'); + const script = `tell application "Finder" to reveal POSIX file "${escapedFilePath}" +tell application "Finder" to activate`; - execSync(`osascript -e '${script}'`, { - encoding: 'utf8', - stdio: 'pipe', - }); + spawnSync('osascript', ['-e', script], { encoding: 'utf8', stdio: 'pipe' }); } catch (error) { logger.debug('Failed to reveal file in Finder:', error.message); } diff --git a/tests/setup-global-mocks.js b/tests/setup-global-mocks.js index c5c3843..dcaeb34 100644 --- a/tests/setup-global-mocks.js +++ b/tests/setup-global-mocks.js @@ -225,13 +225,19 @@ jest.mock('../src/utils/logger.js', () => ({ logger: mockLogger, })); -// Mock clipboardy to prevent ESM import issues -jest.mock('clipboardy', () => ({ - write: jest.fn().mockResolvedValue(undefined), - read: jest.fn().mockResolvedValue(''), - writeSync: jest.fn(), - readSync: jest.fn().mockReturnValue(''), -})); +// Mock clipboardy to prevent ESM import issues. +// The mock object is set as its own `.default` so that dynamic ESM imports +// (`const { default: clipboardy } = await import('clipboardy')`) resolve correctly. +jest.mock('clipboardy', () => { + const mock = { + write: jest.fn().mockResolvedValue(undefined), + read: jest.fn().mockResolvedValue(''), + writeSync: jest.fn(), + readSync: jest.fn().mockReturnValue(''), + }; + mock.default = mock; + return mock; +}); // Mock fs-extra (comprehensive mock with all functions) jest.mock('fs-extra', () => { diff --git a/tests/unit/api/copy.test.js b/tests/unit/api/copy.test.js index 4d20ec6..6edb6f9 100644 --- a/tests/unit/api/copy.test.js +++ b/tests/unit/api/copy.test.js @@ -1,6 +1,8 @@ // Unmock fs-extra for these tests to use real filesystem jest.unmock('fs-extra'); +import Clipboard from '../../../src/utils/clipboard.js'; + import { copy } from '../../../src/api/copy.js'; import { ValidationError } from '../../../src/utils/errors.js'; import fs from 'fs-extra'; @@ -153,6 +155,28 @@ describe('copy()', () => { expect(result.output).toBeDefined(); }); + + it('should call Clipboard.copyText with formatted output when clipboard: true', async () => { + const spy = jest.spyOn(Clipboard, 'copyText').mockResolvedValueOnce(); + + const result = await copy(testDir, { clipboard: true }); + + expect(spy).toHaveBeenCalledTimes(1); + expect(spy).toHaveBeenCalledWith(result.output); + spy.mockRestore(); + }); + + it('should succeed and record clipboardError when Clipboard.copyText throws', async () => { + const spy = jest + .spyOn(Clipboard, 'copyText') + .mockRejectedValueOnce(new Error('clipboard unavailable')); + + const result = await copy(testDir, { clipboard: true }); + + expect(result.output).toBeDefined(); + expect(result.stats.clipboardError).toBe('clipboard unavailable'); + spy.mockRestore(); + }); }); describe('Dry run', () => { diff --git a/tests/unit/utils/clipboard.test.js b/tests/unit/utils/clipboard.test.js new file mode 100644 index 0000000..cca8d15 --- /dev/null +++ b/tests/unit/utils/clipboard.test.js @@ -0,0 +1,254 @@ +import { execSync, spawnSync } from 'child_process'; +import Clipboard from '../../../src/utils/clipboard.js'; + +jest.mock('child_process', () => ({ + execSync: jest.fn(), + spawnSync: jest.fn(), +})); + +// Platform switching helper +const originalPlatform = process.platform; +function setPlatform(platform) { + Object.defineProperty(process, 'platform', { value: platform, writable: true }); +} + +afterEach(() => { + Object.defineProperty(process, 'platform', { value: originalPlatform, writable: true }); +}); + +describe('Clipboard', () => { + describe('copyText', () => { + it('copies text via clipboardy', async () => { + await Clipboard.copyText('hello'); + const { default: clipboardy } = await import('clipboardy'); + expect(clipboardy.write).toHaveBeenCalledWith('hello'); + expect(clipboardy.write).toHaveBeenCalledTimes(1); + }); + }); + + describe('copyFileReference — Windows', () => { + beforeEach(() => setPlatform('win32')); + + it('uses Base64-encoded PowerShell command via EncodedCommand', async () => { + await Clipboard.copyFileReference('C:\\Users\\test\\output.xml'); + + expect(execSync).toHaveBeenCalledTimes(1); + const [cmd] = execSync.mock.calls[0]; + expect(cmd).toMatch(/^powershell -NoProfile -NonInteractive -EncodedCommand /); + }); + + it('uses Windows Forms SetFileDropList (no glob expansion)', async () => { + await Clipboard.copyFileReference('C:\\Users\\test\\output.xml'); + + const [cmd] = execSync.mock.calls[0]; + const base64 = cmd.split('-EncodedCommand ')[1]; + const decoded = Buffer.from(base64, 'base64').toString('utf16le'); + expect(decoded).toContain('SetFileDropList'); + expect(decoded).toContain('System.Windows.Forms'); + }); + + it('escapes single quotes by doubling them', async () => { + await Clipboard.copyFileReference("C:\\Users\\it's a test\\file.xml"); + + const [cmd] = execSync.mock.calls[0]; + const base64 = cmd.split('-EncodedCommand ')[1]; + const decoded = Buffer.from(base64, 'base64').toString('utf16le'); + expect(decoded).toContain("it''s a test"); + }); + + it('handles square brackets without spurious backticks (uses SetFileDropList)', async () => { + await Clipboard.copyFileReference('C:\\Users\\test[1]\\output.xml'); + + const [cmd] = execSync.mock.calls[0]; + const base64 = cmd.split('-EncodedCommand ')[1]; + const decoded = Buffer.from(base64, 'base64').toString('utf16le'); + // SetFileDropList does not glob-expand, so brackets are passed literally + expect(decoded).toContain('test[1]'); + // No backtick escaping should be present + expect(decoded).not.toContain('`['); + }); + + it('handles paths with dollar signs', async () => { + await Clipboard.copyFileReference('C:\\Users\\$data\\output.xml'); + + const [cmd] = execSync.mock.calls[0]; + const base64 = cmd.split('-EncodedCommand ')[1]; + const decoded = Buffer.from(base64, 'base64').toString('utf16le'); + expect(decoded).toContain('$data'); + expect(decoded).toContain('SetFileDropList'); + }); + + it('handles Windows UNC paths', async () => { + await Clipboard.copyFileReference('\\\\server\\share\\file.xml'); + + const [cmd] = execSync.mock.calls[0]; + const base64 = cmd.split('-EncodedCommand ')[1]; + const decoded = Buffer.from(base64, 'base64').toString('utf16le'); + expect(decoded).toContain('\\\\server\\share\\file.xml'); + }); + + it('falls back to copyText on PowerShell failure', async () => { + execSync.mockImplementationOnce(() => { + throw new Error('PowerShell not found'); + }); + const spy = jest.spyOn(Clipboard, 'copyText').mockResolvedValueOnce(); + + await Clipboard.copyFileReference('C:\\test\\file.xml'); + + expect(spy).toHaveBeenCalledWith('C:\\test\\file.xml'); + spy.mockRestore(); + }); + }); + + describe('copyFileReference — macOS', () => { + beforeEach(() => setPlatform('darwin')); + + it('uses spawnSync with osascript (no shell layer)', async () => { + await Clipboard.copyFileReference('/Users/test/output.xml'); + + expect(spawnSync).toHaveBeenCalledTimes(1); + expect(execSync).not.toHaveBeenCalled(); + expect(spawnSync).toHaveBeenCalledWith( + 'osascript', + ['-e', expect.stringContaining('POSIX file "/Users/test/output.xml"')], + expect.objectContaining({ stdio: 'pipe' }), + ); + }); + + it('escapes double quotes for AppleScript', async () => { + await Clipboard.copyFileReference('/Users/test/"quoted"/file.xml'); + + const script = spawnSync.mock.calls[0][1][1]; + expect(script).toContain('\\"quoted\\"'); + }); + + it('escapes backslashes for AppleScript', async () => { + await Clipboard.copyFileReference('/Users/test\\path/file.xml'); + + const script = spawnSync.mock.calls[0][1][1]; + expect(script).toContain('test\\\\path'); + }); + + it('safely handles paths with single quotes (no shell injection)', async () => { + // Previously used execSync(`osascript -e '...'`) which was vulnerable to single-quote injection. + // Now uses spawnSync — single quotes in the path are passed verbatim to osascript. + await Clipboard.copyFileReference("/Users/it's/file.xml"); + + expect(spawnSync).toHaveBeenCalledTimes(1); + expect(execSync).not.toHaveBeenCalled(); + const script = spawnSync.mock.calls[0][1][1]; + expect(script).toContain("it's"); + }); + }); + + describe('copyFileReference — Linux', () => { + beforeEach(() => setPlatform('linux')); + + it('copies file:// URI to clipboard', async () => { + const spy = jest.spyOn(Clipboard, 'copyText').mockResolvedValueOnce(); + + await Clipboard.copyFileReference('/home/user/file.txt'); + + expect(spy).toHaveBeenCalledWith(expect.stringMatching(/^file:\/\/\/home\/user\/file\.txt$/)); + spy.mockRestore(); + }); + }); + + describe('revealInFinder — Windows', () => { + beforeEach(() => setPlatform('win32')); + + it('uses spawnSync instead of execSync to avoid shell injection', async () => { + await Clipboard.revealInFinder('C:\\Users\\test\\file.xml'); + + expect(spawnSync).toHaveBeenCalledTimes(1); + expect(execSync).not.toHaveBeenCalled(); + expect(spawnSync).toHaveBeenCalledWith('explorer', ['/select,C:\\Users\\test\\file.xml'], { + stdio: 'pipe', + }); + }); + + it('safely handles paths with shell metacharacters', async () => { + await Clipboard.revealInFinder('C:\\Users\\test&malicious|path\\file.xml'); + + expect(spawnSync).toHaveBeenCalledWith( + 'explorer', + ['/select,C:\\Users\\test&malicious|path\\file.xml'], + { stdio: 'pipe' }, + ); + expect(execSync).not.toHaveBeenCalled(); + }); + }); + + describe('revealInFinder — macOS', () => { + beforeEach(() => setPlatform('darwin')); + + it('uses spawnSync with osascript (no shell layer)', async () => { + await Clipboard.revealInFinder('/Users/test/file.xml'); + + expect(spawnSync).toHaveBeenCalledTimes(1); + expect(execSync).not.toHaveBeenCalled(); + }); + + it('escapes double quotes in AppleScript', async () => { + await Clipboard.revealInFinder('/Users/test/"quoted"/file.xml'); + + const script = spawnSync.mock.calls[0][1][1]; + expect(script).toContain('\\"quoted\\"'); + }); + + it('escapes backslashes in AppleScript', async () => { + await Clipboard.revealInFinder('/Users/test\\path/file.xml'); + + const script = spawnSync.mock.calls[0][1][1]; + expect(script).toContain('test\\\\path'); + }); + + it('no longer has the no-op replacement bug', async () => { + // Previously: filePath.replace(/"/g, '"') — a no-op + // Now: filePath.replace(/"/g, '\\"') — actually escapes + await Clipboard.revealInFinder('/Users/"test"/file.xml'); + + const script = spawnSync.mock.calls[0][1][1]; + expect(script).toContain('\\"test\\"'); + }); + + it('safely handles paths with single quotes (no shell injection)', async () => { + await Clipboard.revealInFinder("/Users/it's/file.xml"); + + expect(spawnSync).toHaveBeenCalledTimes(1); + expect(execSync).not.toHaveBeenCalled(); + const script = spawnSync.mock.calls[0][1][1]; + expect(script).toContain("it's"); + }); + }); + + describe('revealInFinder — Linux', () => { + beforeEach(() => setPlatform('linux')); + + it('uses spawnSync to avoid shell injection', async () => { + await Clipboard.revealInFinder('/home/user/dir/file.txt'); + + expect(spawnSync).toHaveBeenCalledTimes(1); + expect(execSync).not.toHaveBeenCalled(); + expect(spawnSync).toHaveBeenCalledWith('xdg-open', ['/home/user/dir'], { stdio: 'pipe' }); + }); + + it('safely handles paths with $ characters (no shell expansion)', async () => { + await Clipboard.revealInFinder('/home/$USER/project/file.txt'); + + expect(spawnSync).toHaveBeenCalledWith('xdg-open', ['/home/$USER/project'], { + stdio: 'pipe', + }); + expect(execSync).not.toHaveBeenCalled(); + }); + + it('safely handles paths with double quotes', async () => { + await Clipboard.revealInFinder('/home/user/"weird dir"/file.txt'); + + expect(spawnSync).toHaveBeenCalledWith('xdg-open', ['/home/user/"weird dir"'], { + stdio: 'pipe', + }); + expect(execSync).not.toHaveBeenCalled(); + }); + }); +});