From 80e0916665f63bc98efddb16978d34e9ed58227c Mon Sep 17 00:00:00 2001 From: jdalton Date: Fri, 14 Nov 2025 09:12:42 -0800 Subject: [PATCH 1/2] Fix shadow bin argument passing - args were shifted incorrectly The shadow-bin/npm and shadow-bin/npx files were incorrectly passing the bin name ('npm'/'npx') as the first argument, which caused args to be set to a string instead of an array. This resulted in 'findLast is not a function' errors because strings don't have the findLast method. - shadow-bin/npm: Remove 'npm' argument (already hardcoded in shadowNpmBin) - shadow-bin/npx: Use correct shadowNpxBin function and remove 'npx' argument Fixes #911 --- shadow-bin/npm | 2 +- shadow-bin/npx | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) 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) => { From 99f023fd29ed856151e7697463d580d7be0b2c9f Mon Sep 17 00:00:00 2001 From: jdalton Date: Fri, 14 Nov 2025 09:21:06 -0800 Subject: [PATCH 2/2] Add unit tests for shadow npm and npx bin functions Tests validate that args parameter is correctly passed as an array and that Array.prototype.findLast() works properly. These tests prevent regression of issue #911 where incorrect argument passing caused args to be a string instead of an array. Tests cover: - Array argument handling - Readonly array support - Empty array handling - Terminator (--) handling - findLast method availability - Progress flag identification using findLast --- src/shadow/npm/bin.test.mts | 127 ++++++++++++++++++++++++++++++++++++ src/shadow/npx/bin.test.mts | 127 ++++++++++++++++++++++++++++++++++++ 2 files changed, 254 insertions(+) create mode 100644 src/shadow/npm/bin.test.mts create mode 100644 src/shadow/npx/bin.test.mts 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') + }) +})