fix: sub processes with spaces

HISTORY.md

@@ -6,16 +6,20 @@ All notable changes to this project will be documented in this file.
 
 * Added
   * CLI got a new switch `--short-PURLs` ([#225] via [#226])
+* Fixed
+  * Improved usability on Windows ([#161] via [#234])
 * Misc
   * Improved the error message when a lock files was missing ([#196] via [#231]) 
 * Build
   * Use _TypeScript_ `v4.8.4` now, was `v4.8.3` (via [#164])
 
+[#161]: https://github.com/CycloneDX/cyclonedx-node-npm/issues/161
 [#164]: https://github.com/CycloneDX/cyclonedx-node-npm/pull/164
 [#196]: https://github.com/CycloneDX/cyclonedx-node-npm/issues/196
 [#225]: https://github.com/CycloneDX/cyclonedx-node-npm/issues/225
 [#226]: https://github.com/CycloneDX/cyclonedx-node-npm/pull/226
 [#231]: https://github.com/CycloneDX/cyclonedx-node-npm/pull/231
+[#234]: https://github.com/CycloneDX/cyclonedx-node-npm/pull/234
 
 ## 1.0.0 - 2022-09-24
 

src/builders.ts

@@ -18,7 +18,7 @@ Copyright (c) OWASP Foundation. All Rights Reserved.
 */
 
 import { Builders, Enums, Factories, Models } from '@cyclonedx/cyclonedx-library'
-import { spawnSync } from 'child_process'
+import { execFileSync, execSync, ExecSyncOptionsWithBufferEncoding } from 'child_process'
 import { existsSync } from 'fs'
 import { PackageURL } from 'packageurl-js'
 import { dirname, resolve } from 'path'
@@ -38,12 +38,6 @@ interface BomBuilderOptions {
   shortPURLs?: BomBuilder['shortPURLs']
 }
 
-interface SpawnSyncResultError extends Error {
-  errno?: number
-  code?: string
-  signal?: NodeJS.Signals
-}
-
 type cPath = string
 type AllComponents = Map<cPath, Models.Component>
 
@@ -113,29 +107,32 @@ export class BomBuilder {
     )
   }
 
-  private getNpmCommand (process: NodeJS.Process): string {
+  private getNpmExecPath (process: NodeJS.Process): string | undefined {
     // `npm_execpath` will be whichever cli script has called this application by npm.
     // This can be `npm`, `npx`, or `undefined` if called by `node` directly.
     const execPath = process.env.npm_execpath ?? ''
+    if (execPath === '') {
+      return undefined
+    }
 
     if (this.npxMatcher.test(execPath)) {
       // `npm` must be used for executing `ls`.
       this.console.debug('DEBUG | command: npx-cli.js usage detected, checking for npm-cli.js ...')
       // Typically `npm-cli.js` is alongside `npx-cli.js`, as such we attempt to use this and validate it exists.
       // Replace the script in the path, and normalise it with resolve (eliminates any extraneous path separators).
       const npmPath = resolve(execPath.replace(this.npxMatcher, '$1npm-cli.js'))
-
-      return existsSync(npmPath)
-        ? npmPath // path detected
-        : 'npm' // fallback to global npm
+      if (existsSync(npmPath)) {
+        return npmPath
+      }
+    } else if (existsSync(execPath)) {
+      return execPath
     }
 
-    /* eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing, @typescript-eslint/strict-boolean-expressions -- need to handle optional empty-string */
-    return execPath || 'npm'
+    throw new Error(`unexpected NPM execPath: ${execPath}`)
   }
 
   private fetchNpmLs (projectDir: string, process: NodeJS.Process): any {
-    let command: string = this.getNpmCommand(process)
+    let execPath = this.getNpmExecPath(process)
     const args: string[] = [
       'ls',
       // format as parsable json
@@ -151,55 +148,46 @@ export class BomBuilder {
     for (const odt of this.omitDependencyTypes) {
       args.push('--omit', odt)
     }
-    if (command.endsWith('.js')) {
-      args.unshift('--', command)
-      command = process.execPath
+    if (execPath?.endsWith('.js') === true) {
+      args.unshift('--', execPath)
+      execPath = process.execPath
     }
 
     // TODO use instead ? : https://www.npmjs.com/package/debug ?
     this.console.info('INFO  | gather dependency tree ...')
-    this.console.debug('DEBUG | npm-ls: run %s with %j in %s', command, args, projectDir)
-    const npmLsReturns = spawnSync(command, args, {
-      // must use a shell for Windows systems in order to work
-      shell: true,
-      cwd: projectDir,
-      env: process.env,
-      encoding: 'buffer',
-      maxBuffer: Number.MAX_SAFE_INTEGER // DIRTY but effective
-    })
-    /*
-    if (npmLsReturns.stdout?.length > 0) {
-      this.console.group('DEBUG | npm-ls: STDOUT')
-      this.console.debug('%s', npmLsReturns.stdout)
+    this.console.debug('DEBUG | npm-ls: run %j with %j in %j', execPath ?? 'npm', args, projectDir)
+    let npmLsReturns: Buffer
+    try {
+      const runOptions: ExecSyncOptionsWithBufferEncoding = {
+        cwd: projectDir,
+        env: process.env,
+        encoding: 'buffer',
+        maxBuffer: Number.MAX_SAFE_INTEGER // DIRTY but effective
+      }
+      npmLsReturns = execPath === undefined
+        ? execSync('npm ' + args.join(' '), runOptions)
+        : execFileSync(execPath, args, runOptions)
+    } catch (runError: any) {
+      // this.console.group('DEBUG | npm-ls: STDOUT')
+      // this.console.debug('%s', runError.stdout)
+      // this.console.groupEnd()
+      this.console.group('WARN  | npm-ls: MESSAGE')
+      this.console.warn('%s', runError.message)
       this.console.groupEnd()
-    } else {
-      this.console.debug('DEBUG | npm-ls: no STDOUT')
-    }
-    */
-    if (npmLsReturns.stderr?.length > 0) {
-      this.console.group('WARN  | npm-ls: STDERR')
-      this.console.warn('%s', npmLsReturns.stderr)
+      this.console.group('ERROR | npm-ls: STDERR')
+      this.console.error('%s', runError.stderr)
       this.console.groupEnd()
-    } else {
-      this.console.debug('DEBUG | npm-ls: no STDERR')
-    }
-    if (npmLsReturns.status !== 0 || npmLsReturns.error instanceof Error) {
-      const error = npmLsReturns.error as SpawnSyncResultError ?? {}
-      this.console.group('ERROR | npm-ls: errors')
-      this.console.error('%j', { error, status: npmLsReturns.status, signal: npmLsReturns.signal })
-      this.console.groupEnd()
-      if (this.ignoreNpmErrors) {
-        this.console.debug('DEBUG | npm-ls exited with errors that are to be ignored.')
-      } else {
+      if (!this.ignoreNpmErrors) {
         throw new Error(`npm-ls exited with errors: ${
-          error.errno ?? '???'} ${
-          error.code ?? npmLsReturns.status ?? 'noCode'} ${
-          error.signal ?? npmLsReturns.signal ?? 'noSignal'}`)
+          runError.status as string ?? 'noStatus'} ${
+          runError.signal as string ?? 'noSignal'}`)
       }
+      this.console.debug('DEBUG | npm-ls exited with errors that are to be ignored.')
+      npmLsReturns = runError.stdout ?? Buffer.alloc(0)
     }
-
+    // this.console.debug('stdout: %s', npmLsReturns)
     try {
-      return JSON.parse(npmLsReturns.stdout.toString())
+      return JSON.parse(npmLsReturns.toString())
     } catch (jsonParseError) {
       // @ts-expect-error TS2554
       throw new Error('failed to parse npm-ls response', { cause: jsonParseError })

tests/integration/synthetics/cli.run.test.js

@@ -110,7 +110,7 @@ describe('cli.run()', () => {
       try {
         expect(() => {
           cli.run(mockProcess)
-        }).toThrow(/^npm-ls exited with errors: \?\?\? [1-9]\d* noSignal$/i)
+        }).toThrow(/^unexpected npm execpath/i)
       } finally {
         closeSync(stdout.fd)
         stderr.close()
@@ -150,7 +150,7 @@ describe('cli.run()', () => {
       try {
         expect(() => {
           cli.run(mockProcess)
-        }).toThrow(`npm-ls exited with errors: ??? ${expectedExitCode} noSignal`)
+        }).toThrow(`npm-ls exited with errors: ${expectedExitCode} noSignal`)
       } finally {
         closeSync(stdout.fd)
         stderr.close()