build(lint): disallow node process api, favor Toolkit ChildProcess api #6113

Hweinstock · web-flow · commit d6823149e6cd · 2024-12-12T15:03:37.000-08:00
## Problem contributors may use the low-level node `child_process` library, which lacks the instrumentation and robustness of the toolkit `ChildProcess` utility. Example: #5925 (comment) ## Solution - add the node `child_process` library as a "banned import" with a message pointing to our `ChildProcess` utility. - Migrate simple cases to use our `ChildProcess`, mark more complex ones as exceptions. ## Future Work - To migrate the existing cases marked as exceptions, we will need to add functionality to our `ChildProcess` api, such as enforcing a `maxBufferSize` on the output. - Additionally, some of the current uses of `child_process` are tied to implementation details in the `child_process` library that make it hard to change without a larger-scale refactor. These cases were marked as exceptions.
diff --git a/.eslintrc.js b/.eslintrc.js
@@ -199,6 +199,11 @@ module.exports = {
                         name: 'fs',
                         message: 'Avoid node:fs and use shared/fs/fs.ts when possible.',
                     },
+                    {
+                        name: 'child_process',
+                        message:
+                            'Avoid child_process, use ChildProcess from `shared/utilities/processUtils.ts` instead.',
+                    },
                 ],
             },
         ],
diff --git a/packages/core/scripts/build/generateServiceClient.ts b/packages/core/scripts/build/generateServiceClient.ts
@@ -3,7 +3,7 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
-import * as proc from 'child_process'
+import * as proc from 'child_process' // eslint-disable-line no-restricted-imports
 import * as nodefs from 'fs' // eslint-disable-line no-restricted-imports
 import * as path from 'path'
 
diff --git a/packages/core/scripts/test/launchTestUtilities.ts b/packages/core/scripts/test/launchTestUtilities.ts
@@ -3,7 +3,7 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
-import * as proc from 'child_process'
+import * as proc from 'child_process' // eslint-disable-line no-restricted-imports
 import packageJson from '../../package.json'
 import { downloadAndUnzipVSCode, resolveCliArgsFromVSCodeExecutablePath } from '@vscode/test-electron'
 import { join, resolve } from 'path'
diff --git a/packages/core/src/amazonq/lsp/lspClient.ts b/packages/core/src/amazonq/lsp/lspClient.ts
@@ -10,7 +10,7 @@
 import * as vscode from 'vscode'
 import * as path from 'path'
 import * as nls from 'vscode-nls'
-import * as cp from 'child_process'
+import { spawn } from 'child_process' // eslint-disable-line no-restricted-imports
 import * as crypto from 'crypto'
 import * as jose from 'jose'
 
@@ -199,7 +199,7 @@ export async function activate(extensionContext: ExtensionContext) {
 
     const nodename = process.platform === 'win32' ? 'node.exe' : 'node'
 
-    const child = cp.spawn(extensionContext.asAbsolutePath(path.join('resources', nodename)), [
+    const child = spawn(extensionContext.asAbsolutePath(path.join('resources', nodename)), [
         serverModule,
         ...debugOptions.execArgv,
     ])
diff --git a/packages/core/src/awsService/accessanalyzer/vue/iamPolicyChecks.ts b/packages/core/src/awsService/accessanalyzer/vue/iamPolicyChecks.ts
@@ -12,7 +12,6 @@ import { VueWebview, VueWebviewPanel } from '../../../webviews/main'
 import { ExtContext } from '../../../shared/extensions'
 import { telemetry } from '../../../shared/telemetry/telemetry'
 import { AccessAnalyzer, SharedIniFileCredentials } from 'aws-sdk'
-import { execFileSync } from 'child_process'
 import { ToolkitError } from '../../../shared/errors'
 import { makeTemporaryToolkitFolder, tryRemoveFolder } from '../../../shared/filesystemUtilities'
 import { globals } from '../../../shared'
@@ -28,6 +27,7 @@ import {
 } from './constants'
 import { DefaultS3Client, parseS3Uri } from '../../../shared/clients/s3Client'
 import { ExpiredTokenException } from '@aws-sdk/client-sso-oidc'
+import { ChildProcess } from '../../../shared/utilities/processUtils'
 
 const defaultTerraformConfigPath = 'resources/policychecks-tf-default.yaml'
 // Diagnostics for Custom checks are shared
@@ -277,7 +277,7 @@ export class IamPolicyChecksWebview extends VueWebview {
                         '--config',
                         `${globals.context.asAbsolutePath(defaultTerraformConfigPath)}`,
                     ]
-                    this.executeValidatePolicyCommand({
+                    await this.executeValidatePolicyCommand({
                         command,
                         args,
                         cfnParameterPathExists: !!cfnParameterPath,
@@ -300,7 +300,7 @@ export class IamPolicyChecksWebview extends VueWebview {
                     if (cfnParameterPath !== '') {
                         args.push('--template-configuration-file', `${cfnParameterPath}`)
                     }
-                    this.executeValidatePolicyCommand({
+                    await this.executeValidatePolicyCommand({
                         command,
                         args,
                         cfnParameterPathExists: !!cfnParameterPath,
@@ -357,7 +357,7 @@ export class IamPolicyChecksWebview extends VueWebview {
                         '--reference-policy-type',
                         `${policyType}`,
                     ]
-                    this.executeCustomPolicyChecksCommand({
+                    await this.executeCustomPolicyChecksCommand({
                         command,
                         args,
                         cfnParameterPathExists: !!cfnParameterPath,
@@ -391,7 +391,7 @@ export class IamPolicyChecksWebview extends VueWebview {
                     if (cfnParameterPath !== '') {
                         args.push('--template-configuration-file', `${cfnParameterPath}`)
                     }
-                    this.executeCustomPolicyChecksCommand({
+                    await this.executeCustomPolicyChecksCommand({
                         command,
                         args,
                         cfnParameterPathExists: !!cfnParameterPath,
@@ -454,7 +454,7 @@ export class IamPolicyChecksWebview extends VueWebview {
                     if (resources !== '') {
                         args.push('--resources', `${resources}`)
                     }
-                    this.executeCustomPolicyChecksCommand({
+                    await this.executeCustomPolicyChecksCommand({
                         command,
                         args,
                         cfnParameterPathExists: !!cfnParameterPath,
@@ -489,7 +489,7 @@ export class IamPolicyChecksWebview extends VueWebview {
                     if (cfnParameterPath !== '') {
                         args.push('--template-configuration-file', `${cfnParameterPath}`)
                     }
-                    this.executeCustomPolicyChecksCommand({
+                    await this.executeCustomPolicyChecksCommand({
                         command,
                         args,
                         cfnParameterPathExists: !!cfnParameterPath,
@@ -525,7 +525,7 @@ export class IamPolicyChecksWebview extends VueWebview {
                         '--config',
                         `${globals.context.asAbsolutePath(defaultTerraformConfigPath)}`,
                     ]
-                    this.executeCustomPolicyChecksCommand({
+                    await this.executeCustomPolicyChecksCommand({
                         command,
                         args,
                         cfnParameterPathExists: !!cfnParameterPath,
@@ -554,7 +554,7 @@ export class IamPolicyChecksWebview extends VueWebview {
                     if (cfnParameterPath !== '') {
                         args.push('--template-configuration-file', `${cfnParameterPath}`)
                     }
-                    this.executeCustomPolicyChecksCommand({
+                    await this.executeCustomPolicyChecksCommand({
                         command,
                         args,
                         cfnParameterPathExists: !!cfnParameterPath,
@@ -573,16 +573,16 @@ export class IamPolicyChecksWebview extends VueWebview {
         }
     }
 
-    public executeValidatePolicyCommand(opts: PolicyCommandOpts & { policyType?: PolicyChecksPolicyType }) {
-        telemetry.accessanalyzer_iamPolicyChecksValidatePolicy.run((span) => {
+    public async executeValidatePolicyCommand(opts: PolicyCommandOpts & { policyType?: PolicyChecksPolicyType }) {
+        await telemetry.accessanalyzer_iamPolicyChecksValidatePolicy.run(async (span) => {
             try {
                 span.record({
                     cfnParameterFileUsed: opts.cfnParameterPathExists,
                     documentType: opts.documentType,
                     inputPolicyType: opts.policyType ?? 'None',
                 })
-                const resp = execFileSync(opts.command, opts.args)
-                const findingsCount = this.handleValidatePolicyCliResponse(resp.toString())
+                const result = await ChildProcess.run(opts.command, opts.args, { collect: true })
+                const findingsCount = this.handleValidatePolicyCliResponse(result.stdout)
                 span.record({
                     findingsCount: findingsCount,
                 })
@@ -633,10 +633,10 @@ export class IamPolicyChecksWebview extends VueWebview {
         return findingsCount
     }
 
-    public executeCustomPolicyChecksCommand(
+    public async executeCustomPolicyChecksCommand(
         opts: PolicyCommandOpts & { checkType: PolicyChecksCheckType; referencePolicyType?: PolicyChecksPolicyType }
     ) {
-        telemetry.accessanalyzer_iamPolicyChecksCustomChecks.run((span) => {
+        await telemetry.accessanalyzer_iamPolicyChecksCustomChecks.run(async (span) => {
             try {
                 span.record({
                     cfnParameterFileUsed: opts.cfnParameterPathExists,
@@ -645,8 +645,8 @@ export class IamPolicyChecksWebview extends VueWebview {
                     inputPolicyType: 'None', // Note: This will change once JSON policy language is enabled for Custom policy checks
                     referencePolicyType: opts.referencePolicyType ?? 'None',
                 })
-                const resp = execFileSync(opts.command, opts.args)
-                const findingsCount = this.handleCustomPolicyChecksCliResponse(resp.toString())
+                const resp = await ChildProcess.run(opts.command, opts.args)
+                const findingsCount = this.handleCustomPolicyChecksCliResponse(resp.stdout)
                 span.record({
                     findingsCount: findingsCount,
                 })
@@ -790,7 +790,7 @@ export async function renderIamPolicyChecks(context: ExtContext): Promise<VueWeb
                 checkAccessNotGrantedResourcesTextArea,
                 customChecksFileErrorMessage,
                 cfnParameterPath: cfnParameterPath ? cfnParameterPath : '',
-                pythonToolsInstalled: arePythonToolsInstalled(),
+                pythonToolsInstalled: await arePythonToolsInstalled(),
             },
             client,
             context.regionProvider.defaultRegionId
@@ -828,20 +828,20 @@ export async function _readCustomChecksFile(input: string): Promise<string> {
 }
 
 // Check if Cfn and Tf tools are installed
-export function arePythonToolsInstalled(): boolean {
+export async function arePythonToolsInstalled(): Promise<boolean> {
     const logger: Logger = getLogger()
     let cfnToolInstalled = true
     let tfToolInstalled = true
     try {
-        execFileSync('tf-policy-validator')
+        await ChildProcess.run('tf-policy-validator')
     } catch (err: any) {
         if (isProcessNotFoundErr(err.message)) {
             tfToolInstalled = false
             logger.error('Terraform Policy Validator is not found')
         }
     }
     try {
-        execFileSync('cfn-policy-validator')
+        await ChildProcess.run('cfn-policy-validator')
     } catch (err: any) {
         if (isProcessNotFoundErr(err.message)) {
             cfnToolInstalled = false
diff --git a/packages/core/src/codewhisperer/commands/startTestGeneration.ts b/packages/core/src/codewhisperer/commands/startTestGeneration.ts
@@ -17,7 +17,7 @@ import {
 import path from 'path'
 import { testGenState } from '..'
 import { ChatSessionManager } from '../../amazonqTest/chat/storages/chatSession'
-import { ChildProcess, spawn } from 'child_process'
+import { ChildProcess, spawn } from 'child_process' // eslint-disable-line no-restricted-imports
 import { BuildStatus } from '../../amazonqTest/chat/session/session'
 import { fs } from '../../shared/fs/fs'
 import { TestGenerationJobStatus } from '../models/constants'
diff --git a/packages/core/src/codewhisperer/service/transformByQ/transformMavenHandler.ts b/packages/core/src/codewhisperer/service/transformByQ/transformMavenHandler.ts
@@ -6,7 +6,8 @@ import * as vscode from 'vscode'
 import { FolderInfo, transformByQState } from '../../models/model'
 import { getLogger } from '../../../shared/logger'
 import * as CodeWhispererConstants from '../../models/constants'
-import { spawnSync } from 'child_process' // Consider using ChildProcess once we finalize all spawnSync calls
+// Consider using ChildProcess once we finalize all spawnSync calls
+import { spawnSync } from 'child_process' // eslint-disable-line no-restricted-imports
 import { CodeTransformBuildCommand, telemetry } from '../../../shared/telemetry/telemetry'
 import { CodeTransformTelemetryState } from '../../../amazonqGumby/telemetry/codeTransformTelemetryState'
 import { ToolkitError } from '../../../shared/errors'
diff --git a/packages/core/src/codewhisperer/service/transformByQ/transformProjectValidationHandler.ts b/packages/core/src/codewhisperer/service/transformByQ/transformProjectValidationHandler.ts
@@ -6,7 +6,8 @@ import { BuildSystem, JDKVersion, TransformationCandidateProject } from '../../m
 import { getLogger } from '../../../shared/logger'
 import * as CodeWhispererConstants from '../../models/constants'
 import * as vscode from 'vscode'
-import { spawnSync } from 'child_process' // Consider using ChildProcess once we finalize all spawnSync calls
+// Consider using ChildProcess once we finalize all spawnSync calls
+import { spawnSync } from 'child_process' // eslint-disable-line no-restricted-imports
 import {
     NoJavaProjectsFoundError,
     NoMavenJavaProjectsFoundError,
diff --git a/packages/core/src/lambda/commands/createNewSamApp.ts b/packages/core/src/lambda/commands/createNewSamApp.ts
@@ -45,14 +45,14 @@ import {
     isCloud9,
     getLaunchConfigDocUrl,
 } from '../../shared/extensionUtilities'
-import { execFileSync } from 'child_process'
 import { checklogs } from '../../shared/localizedText'
 import globals from '../../shared/extensionGlobals'
 import { telemetry } from '../../shared/telemetry/telemetry'
 import { LambdaArchitecture, Result, Runtime } from '../../shared/telemetry/telemetry'
 import { getTelemetryReason, getTelemetryResult } from '../../shared/errors'
 import { openUrl, replaceVscodeVars } from '../../shared/utilities/vsCodeUtils'
 import { fs } from '../../shared'
+import { ChildProcess } from '../../shared/utilities/processUtils'
 
 export const samInitTemplateFiles: string[] = ['template.yaml', 'template.yml']
 export const samInitReadmeFile: string = 'README.TOOLKIT.md'
@@ -218,7 +218,9 @@ export async function createNewSamApplication(
         // Needs to be done or else gopls won't start
         if (goRuntimes.includes(createRuntime)) {
             try {
-                execFileSync('go', ['mod', 'tidy'], { cwd: path.join(path.dirname(templateUri.fsPath), 'hello-world') })
+                await ChildProcess.run('go', ['mod', 'tidy'], {
+                    spawnOptions: { cwd: path.join(path.dirname(templateUri.fsPath), 'hello-world') },
+                })
             } catch (err) {
                 getLogger().warn(
                     localize(
diff --git a/packages/core/src/shared/extensions/git.ts b/packages/core/src/shared/extensions/git.ts
@@ -6,7 +6,7 @@
 import * as vscode from 'vscode'
 import * as GitTypes from '../../../types/git.d'
 import { SemVer, parse as semverParse } from 'semver'
-import { execFile } from 'child_process'
+import { execFile } from 'child_process' // eslint-disable-line no-restricted-imports
 import { promisify } from 'util'
 import { VSCODE_EXTENSION_ID } from '../extensions'
 import { makeTemporaryToolkitFolder, tryRemoveFolder } from '../filesystemUtilities'
diff --git a/packages/core/src/shared/sam/cli/samCliInvokerUtils.ts b/packages/core/src/shared/sam/cli/samCliInvokerUtils.ts
@@ -3,7 +3,7 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
-import { SpawnOptions } from 'child_process'
+import { SpawnOptions } from 'child_process' // eslint-disable-line no-restricted-imports
 import { getLogger } from '../../logger'
 import { getUserAgent } from '../../telemetry/util'
 import { ChildProcessResult, ChildProcessOptions } from '../../utilities/processUtils'
diff --git a/packages/core/src/shared/sam/cli/samCliLocalInvoke.ts b/packages/core/src/shared/sam/cli/samCliLocalInvoke.ts
@@ -3,7 +3,7 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
-import * as proc from 'child_process'
+import { SpawnOptions } from 'child_process' // eslint-disable-line no-restricted-imports
 import { pushIf } from '../../utilities/collectionUtils'
 import * as nls from 'vscode-nls'
 import { getLogger, getDebugConsoleLogger, Logger } from '../../logger'
@@ -30,7 +30,7 @@ export const waitForDebuggerMessages = {
 export interface SamLocalInvokeCommandArgs {
     command: string
     args: string[]
-    options?: proc.SpawnOptions
+    options?: SpawnOptions
     /** Wait until strings specified in `debuggerAttachCues` appear in the process output.  */
     waitForCues: boolean
     timeout?: Timeout
diff --git a/packages/core/src/shared/sam/debugger/goSamDebug.ts b/packages/core/src/shared/sam/debugger/goSamDebug.ts
@@ -18,7 +18,7 @@ import { getLogger } from '../../logger'
 import fs from '../../fs/fs'
 import { ChildProcess } from '../../utilities/processUtils'
 import { Timeout } from '../../utilities/timeoutUtils'
-import { execFileSync, SpawnOptions } from 'child_process'
+import { SpawnOptions } from 'child_process' // eslint-disable-line no-restricted-imports
 import * as nls from 'vscode-nls'
 import { sleep } from '../../utilities/timeoutUtils'
 import globals from '../../extensionGlobals'
@@ -174,9 +174,11 @@ async function makeInstallScript(debuggerPath: string, isWindows: boolean): Prom
     // Go from trying to find the manifest file and uses GOPATH provided below.
     installOptions.env!['GO111MODULE'] = 'off'
 
-    function getDelveVersion(repo: string, silent: boolean): string {
+    async function getDelveVersion(repo: string, silent: boolean): Promise<string> {
         try {
-            return execFileSync('git', ['-C', repo, 'describe', '--tags', '--abbrev=0']).toString().trim()
+            return (
+                await ChildProcess.run('git', ['-C', repo, 'describe', '--tags', '--abbrev=0'], { collect: true })
+            ).stdout.trim()
         } catch (e) {
             if (!silent) {
                 throw e
@@ -187,7 +189,8 @@ async function makeInstallScript(debuggerPath: string, isWindows: boolean): Prom
 
     // It's fine if we can't get the latest Delve version, the Toolkit will use the last built one instead
     try {
-        const goPath: string = JSON.parse(execFileSync('go', ['env', '-json']).toString()).GOPATH
+        const result = await ChildProcess.run('go', ['env', '-json'], { collect: true })
+        const goPath: string = JSON.parse(result.stdout).GOPATH
         let repoPath: string = path.join(goPath, 'src', delveRepo)
 
         if (!getDelveVersion(repoPath, true)) {
@@ -200,11 +203,11 @@ async function makeInstallScript(debuggerPath: string, isWindows: boolean): Prom
             installOptions.env!['GOPATH'] = debuggerPath
             repoPath = path.join(debuggerPath, 'src', delveRepo)
             const args = ['get', '-d', `${delveRepo}/cmd/dlv`]
-            const out = execFileSync('go', args, installOptions as any)
+            const out = await ChildProcess.run('go', args, { ...(installOptions as any), collect: true })
             getLogger().debug('"go %O": %s', args, out)
         }
 
-        delveVersion = getDelveVersion(repoPath, false)
+        delveVersion = await getDelveVersion(repoPath, false)
     } catch (e) {
         getLogger().debug('Failed to get latest Delve version: %O', e as Error)
     }
diff --git a/packages/core/src/shared/utilities/processUtils.ts b/packages/core/src/shared/utilities/processUtils.ts
@@ -3,7 +3,7 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
-import * as proc from 'child_process'
+import * as proc from 'child_process' // eslint-disable-line no-restricted-imports
 import * as crossSpawn from 'cross-spawn'
 import * as logger from '../logger'
 import { Timeout, CancellationError, waitUntil } from './timeoutUtils'
@@ -100,6 +100,13 @@ export class ChildProcess {
         // TODO: allow caller to use the various loggers instead of just the single one
         this.#log = baseOptions.logging !== 'no' ? logger.getLogger() : logger.getNullLogger()
     }
+    public static async run(
+        command: string,
+        args: string[] = [],
+        options?: ChildProcessOptions
+    ): Promise<ChildProcessResult> {
+        return await new ChildProcess(command, args, options).run()
+    }
 
     // Inspired by 'got'
     /**
diff --git a/packages/core/src/test/shared/sam/cli/samCliBuild.test.ts b/packages/core/src/test/shared/sam/cli/samCliBuild.test.ts
@@ -4,7 +4,7 @@
  */
 
 import assert from 'assert'
-import { SpawnOptions } from 'child_process'
+import { SpawnOptions } from 'child_process' // eslint-disable-line no-restricted-imports
 import * as path from 'path'
 import { makeTemporaryToolkitFolder } from '../../../../shared/filesystemUtilities'
 import { makeUnexpectedExitCodeError } from '../../../../shared/sam/cli/samCliInvokerUtils'
diff --git a/packages/core/src/test/shared/sam/cli/samCliInit.test.ts b/packages/core/src/test/shared/sam/cli/samCliInit.test.ts
@@ -4,7 +4,7 @@
  */
 
 import assert from 'assert'
-import { SpawnOptions } from 'child_process'
+import { SpawnOptions } from 'child_process' // eslint-disable-line no-restricted-imports
 import {
     eventBridgeStarterAppTemplate,
     getSamCliTemplateParameter,
diff --git a/packages/core/src/test/shared/sam/cli/testSamCliProcessInvoker.ts b/packages/core/src/test/shared/sam/cli/testSamCliProcessInvoker.ts
diff --git a/packages/core/src/testInteg/shared/extensions/git.test.ts b/packages/core/src/testInteg/shared/extensions/git.test.ts
diff --git a/packages/core/src/testLint/testUtils.ts b/packages/core/src/testLint/testUtils.ts
diff --git a/scripts/createRelease.ts b/scripts/createRelease.ts
diff --git a/scripts/generateNonCodeFiles.ts b/scripts/generateNonCodeFiles.ts
diff --git a/scripts/newChange.ts b/scripts/newChange.ts
diff --git a/scripts/package.ts b/scripts/package.ts
diff --git a/scripts/prepare.ts b/scripts/prepare.ts
