diff --git a/shadow-bin/npm b/shadow-bin/npm index b4440fda1..3c79efcd1 100755 --- a/shadow-bin/npm +++ b/shadow-bin/npm @@ -11,7 +11,7 @@ void (async () => { process.exitCode = 1 - const { spawnPromise } = await shadowNpmBin('npm', process.argv.slice(2), { stdio: 'inherit' }) + const { spawnPromise } = await shadowNpmBin(process.argv.slice(2), { stdio: 'inherit' }) // See https://nodejs.org/api/child_process.html#event-exit. spawnPromise.process.on('exit', (code, signalName) => { diff --git a/shadow-bin/npx b/shadow-bin/npx index 65056b49a..d9196b936 100755 --- a/shadow-bin/npx +++ b/shadow-bin/npx @@ -7,11 +7,11 @@ void (async () => { const rootPath = path.join(__dirname, '..') Module.enableCompileCache?.(path.join(rootPath, '.cache')) - const shadowNpmBin = require(path.join(rootPath, 'dist/shadow-npm-bin.js')) + const shadowNpxBin = require(path.join(rootPath, 'dist/shadow-npx-bin.js')) process.exitCode = 1 - const { spawnPromise } = await shadowNpmBin('npx', process.argv.slice(2), { stdio: 'inherit' }) + const { spawnPromise } = await shadowNpxBin(process.argv.slice(2), { stdio: 'inherit' }) // See https://nodejs.org/api/child_process.html#event-exit. spawnPromise.process.on('exit', (code, signalName) => { diff --git a/src/shadow/npm/bin.test.mts b/src/shadow/npm/bin.test.mts new file mode 100644 index 000000000..e544c0d2c --- /dev/null +++ b/src/shadow/npm/bin.test.mts @@ -0,0 +1,127 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +import shadowNpmBin from './bin.mts' + +// Mock all dependencies with vi.hoisted for better type safety. +const mockInstallNpmLinks = vi.hoisted(() => vi.fn()) +const mockSpawn = vi.hoisted(() => vi.fn()) +const mockFindUp = vi.hoisted(() => vi.fn()) + +vi.mock('../../utils/shadow-links.mts', () => ({ + installNpmLinks: mockInstallNpmLinks, +})) + +vi.mock('@socketsecurity/registry/lib/spawn', () => ({ + spawn: mockSpawn, +})) + +vi.mock('../../utils/fs.mts', () => ({ + findUp: mockFindUp, +})) + +vi.mock('../../constants.mts', async importOriginal => { + const actual = (await importOriginal()) as Record + return { + ...actual, + default: { + ...actual?.default, + shadowBinPath: '/mock/shadow-bin', + shadowNpmInjectPath: '/mock/inject.js', + execPath: '/usr/bin/node', + npmGlobalPrefix: '/usr/local', + npmCachePath: '/home/.npm', + nodeNoWarningsFlags: [], + nodeDebugFlags: [], + nodeHardenFlags: [], + nodeMemoryFlags: [], + processEnv: {}, + ENV: { + INLINED_SOCKET_CLI_SENTRY_BUILD: false, + }, + SUPPORTS_NODE_PERMISSION_FLAG: false, + }, + } +}) + +describe('shadowNpmBin', () => { + beforeEach(() => { + vi.clearAllMocks() + + // Default mock implementations. + mockInstallNpmLinks.mockResolvedValue('/usr/bin/npm') + mockFindUp.mockResolvedValue(null) + mockSpawn.mockReturnValue({ + process: { + send: vi.fn(), + on: vi.fn(), + }, + then: vi.fn().mockImplementation(cb => + cb({ + success: true, + code: 0, + stdout: '', + stderr: '', + }), + ), + }) + }) + + it('should accept args as an array and handle findLast correctly', async () => { + const args = ['install', '--no-progress', '--loglevel=error'] + const result = await shadowNpmBin(args) + + expect(result).toHaveProperty('spawnPromise') + expect(mockSpawn).toHaveBeenCalled() + + // Verify spawn was called with correct arguments. + const spawnArgs = mockSpawn.mock.calls[0] + expect(spawnArgs).toBeDefined() + }) + + it('should handle array with terminator correctly', async () => { + const args = ['install', '--no-progress', '--', 'extra', 'args'] + const result = await shadowNpmBin(args) + + expect(result).toHaveProperty('spawnPromise') + expect(mockSpawn).toHaveBeenCalled() + }) + + it('should handle empty args array', async () => { + const args: string[] = [] + const result = await shadowNpmBin(args) + + expect(result).toHaveProperty('spawnPromise') + expect(mockSpawn).toHaveBeenCalled() + }) + + it('should handle readonly array correctly', async () => { + const args: readonly string[] = ['install', '--no-progress'] as const + const result = await shadowNpmBin(args) + + expect(result).toHaveProperty('spawnPromise') + expect(mockSpawn).toHaveBeenCalled() + }) + + it('should not throw "findLast is not a function" error', async () => { + // This test specifically validates the fix for issue #911. + // The bug was caused by passing a string instead of an array, + // which made rawBinArgs.findLast() fail because strings don't + // have the findLast method. + const args = ['install', '--progress'] + + await expect(shadowNpmBin(args)).resolves.toHaveProperty('spawnPromise') + }) + + it('should correctly identify progress flags using findLast', async () => { + // Test that findLast correctly finds the last progress flag. + const args = ['install', '--progress', '--no-progress'] + await shadowNpmBin(args) + + // Verify spawn was called - the actual flag processing happens inside. + expect(mockSpawn).toHaveBeenCalled() + const spawnArgs = mockSpawn.mock.calls[0][1] as string[] + + // Should include --no-progress in the final args since it was last. + expect(spawnArgs).toContain('--no-progress') + }) +}) diff --git a/src/shadow/npx/bin.test.mts b/src/shadow/npx/bin.test.mts new file mode 100644 index 000000000..08b0ad88b --- /dev/null +++ b/src/shadow/npx/bin.test.mts @@ -0,0 +1,127 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +import shadowNpxBin from './bin.mts' + +// Mock all dependencies with vi.hoisted for better type safety. +const mockInstallNpxLinks = vi.hoisted(() => vi.fn()) +const mockSpawn = vi.hoisted(() => vi.fn()) +const mockFindUp = vi.hoisted(() => vi.fn()) + +vi.mock('../../utils/shadow-links.mts', () => ({ + installNpxLinks: mockInstallNpxLinks, +})) + +vi.mock('@socketsecurity/registry/lib/spawn', () => ({ + spawn: mockSpawn, +})) + +vi.mock('../../utils/fs.mts', () => ({ + findUp: mockFindUp, +})) + +vi.mock('../../constants.mts', async importOriginal => { + const actual = (await importOriginal()) as Record + return { + ...actual, + default: { + ...actual?.default, + shadowBinPath: '/mock/shadow-bin', + shadowNpmInjectPath: '/mock/inject.js', + execPath: '/usr/bin/node', + npmGlobalPrefix: '/usr/local', + npmCachePath: '/usr/local/.npm', + nodeNoWarningsFlags: [], + nodeDebugFlags: [], + nodeHardenFlags: [], + nodeMemoryFlags: [], + processEnv: {}, + ENV: { + INLINED_SOCKET_CLI_SENTRY_BUILD: false, + }, + SUPPORTS_NODE_PERMISSION_FLAG: false, + }, + } +}) + +describe('shadowNpxBin', () => { + beforeEach(() => { + vi.clearAllMocks() + + // Default mock implementations. + mockInstallNpxLinks.mockResolvedValue('/usr/bin/npx') + mockFindUp.mockResolvedValue(null) + mockSpawn.mockReturnValue({ + process: { + send: vi.fn(), + on: vi.fn(), + }, + then: vi.fn().mockImplementation(cb => + cb({ + success: true, + code: 0, + stdout: '', + stderr: '', + }), + ), + }) + }) + + it('should accept args as an array and handle findLast correctly', async () => { + const args = ['cowsay', 'hello', '--no-progress'] + const result = await shadowNpxBin(args) + + expect(result).toHaveProperty('spawnPromise') + expect(mockSpawn).toHaveBeenCalled() + + // Verify spawn was called with correct arguments. + const spawnArgs = mockSpawn.mock.calls[0] + expect(spawnArgs).toBeDefined() + }) + + it('should handle array with terminator correctly', async () => { + const args = ['cowsay', '--', 'extra', 'args'] + const result = await shadowNpxBin(args) + + expect(result).toHaveProperty('spawnPromise') + expect(mockSpawn).toHaveBeenCalled() + }) + + it('should handle empty args array', async () => { + const args: string[] = [] + const result = await shadowNpxBin(args) + + expect(result).toHaveProperty('spawnPromise') + expect(mockSpawn).toHaveBeenCalled() + }) + + it('should handle readonly array correctly', async () => { + const args: readonly string[] = ['cowsay', 'hello'] as const + const result = await shadowNpxBin(args) + + expect(result).toHaveProperty('spawnPromise') + expect(mockSpawn).toHaveBeenCalled() + }) + + it('should not throw "findLast is not a function" error', async () => { + // This test specifically validates the fix for issue #911. + // The bug was caused by passing a string instead of an array, + // which made rawBinArgs.findLast() fail because strings don't + // have the findLast method. + const args = ['cowsay', '--progress'] + + await expect(shadowNpxBin(args)).resolves.toHaveProperty('spawnPromise') + }) + + it('should correctly identify progress flags using findLast', async () => { + // Test that findLast correctly finds the last progress flag. + const args = ['cowsay', '--progress', '--no-progress'] + await shadowNpxBin(args) + + // Verify spawn was called - the actual flag processing happens inside. + expect(mockSpawn).toHaveBeenCalled() + const spawnArgs = mockSpawn.mock.calls[0][1] as string[] + + // Should include --no-progress in the final args since it was last. + expect(spawnArgs).toContain('--no-progress') + }) +})