fix: properly sanitize npm_config_registry env-var @W-22606077

package.json

@@ -15,12 +15,13 @@
     "@salesforce/kit": "^3.2.6",
     "@salesforce/plugin-info": "^3.4.115",
     "@salesforce/sf-plugins-core": "^12",
+    "cross-spawn": "^7.0.6",
     "got": "^13.0.0",
     "npm": "^10.9.3",
     "npm-run-path": "^4.0.1",
     "proxy-agent": "^6.5.0",
     "semver": "^7.7.4",
-    "shelljs": "0.10.0"
+    "which": "^5"
   },
   "devDependencies": {
     "@oclif/plugin-command-snapshot": "^5.3.13",
@@ -29,10 +30,13 @@
     "@salesforce/plugin-command-reference": "^3.1.84",
     "@salesforce/plugin-telemetry": "^3.7.2",
     "@salesforce/ts-sinon": "^1.4.33",
+    "@types/cross-spawn": "^6.0.6",
     "@types/shelljs": "^0.10.0",
     "@types/sinon-chai": "^3.2.12",
+    "@types/which": "^3.0.4",
     "eslint-plugin-sf-plugin": "^1.20.33",
     "oclif": "^4.22.4",
+    "shelljs": "^0.10.0",
     "sinon-chai": "^3.7.0",
     "ts-node": "^10.9.2",
     "typescript": "^5.9.3"

src/hooks/diagnostics.ts

@@ -16,21 +16,84 @@
 import { SfDoctor } from '@salesforce/plugin-info';
 import { Lifecycle } from '@salesforce/core';
 import { NpmModule } from '../shared/npmCommand.js';
+
 type HookFunction = (options: { doctor: SfDoctor }) => Promise<[void]>;
 export const hook: HookFunction = (options) => Promise.all([registryCheck(options)]);
 
+/**
+ * Validates that a string is a well-formed HTTP/HTTPS URL
+ *
+ * @param urlString - The URL string to validate
+ * @returns true if valid, false otherwise
+ */
+const isValidRegistryUrl = (urlString: string): boolean => {
+  try {
+    const url = new URL(urlString);
+    // Only allow http/https protocols to prevent protocol-based attacks
+    return url.protocol === 'http:' || url.protocol === 'https:';
+  } catch {
+    return false;
+  }
+};
+
+/**
+ * Sanitizes a registry URL to prevent command injection
+ * Validates URL format and ensures no shell metacharacters
+ *
+ * @param registryUrl - The registry URL to sanitize
+ * @returns The sanitized URL or undefined if invalid
+ */
+const sanitizeRegistryUrl = (registryUrl: string): string | undefined => {
+  if (!registryUrl || typeof registryUrl !== 'string') {
+    return undefined;
+  }
+
+  // Trim whitespace
+  const trimmed = registryUrl.trim();
+
+  // Check for shell metacharacters that could enable command injection
+  const dangerousChars = /[;&|`$(){}[\]<>\\'"]/;
+  if (dangerousChars.test(trimmed)) {
+    return undefined;
+  }
+
+  // Validate as proper URL
+  if (!isValidRegistryUrl(trimmed)) {
+    return undefined;
+  }
+
+  return trimmed;
+};
+
 const registryCheck = async (options: { doctor: SfDoctor }): Promise<void> => {
   const pluginName = '@salesforce/plugin-trust';
   // find npm install
   const npm = new NpmModule('');
   const env = process.env.npm_config_registry ?? process.env.NPM_CONFIG_REGISTRY;
+
+  let sanitizedEnv: string | undefined;
   if (env) {
-    options.doctor.addSuggestion(`using npm registry ${env} from environment variable`);
+    sanitizedEnv = sanitizeRegistryUrl(env);
+    if (sanitizedEnv) {
+      options.doctor.addSuggestion(`using npm registry ${sanitizedEnv} from environment variable`);
+    } else {
+      options.doctor.addSuggestion(
+        `WARNING: npm registry environment variable contains invalid or potentially unsafe URL: ${env}`
+      );
+    }
   }
 
   const config = npm.run('config get registry').stdout.trim();
+  let sanitizedConfig: string | undefined;
   if (config) {
-    options.doctor.addSuggestion(`using npm registry ${config} from npm config`);
+    sanitizedConfig = sanitizeRegistryUrl(config);
+    if (sanitizedConfig) {
+      options.doctor.addSuggestion(`using npm registry ${sanitizedConfig} from npm config`);
+    } else {
+      options.doctor.addSuggestion(
+        `WARNING: npm config registry contains invalid or potentially unsafe URL: ${config}`
+      );
+    }
   }
 
   await Promise.all(
@@ -39,7 +102,7 @@ const registryCheck = async (options: { doctor: SfDoctor }): Promise<void> => {
         // npm and yarn registries
         'https://registry.npmjs.org',
         'https://registry.yarnpkg.com',
-        env ?? config,
+        sanitizedEnv ?? sanitizedConfig ?? '',
       ]),
     ]
       // incase customRegistry is undefined, prevent printing an extra line

src/shared/npmCommand.ts

@@ -20,7 +20,8 @@ import { createRequire } from 'node:module';
 
 import fs from 'node:fs';
 import npmRunPath from 'npm-run-path';
-import shelljs, { ShellString } from 'shelljs';
+import crossSpawn from 'cross-spawn';
+import which from 'which';
 import { SfError } from '@salesforce/core';
 import { sleep, parseJson } from '@salesforce/kit';
 import { Ux } from '@salesforce/sf-plugins-core';
@@ -51,13 +52,18 @@ export type NpmShowResults = {
   };
 };
 
-type NpmCommandOptions = shelljs.ExecOptions & {
+type NpmCommandOptions = {
   json?: boolean;
   registry?: string;
   cliRoot?: string;
+  cwd?: string;
 };
 
-type NpmCommandResult = shelljs.ShellString;
+type NpmCommandResult = {
+  code: number;
+  stdout: string;
+  stderr: string;
+};
 
 type NpmPackage = {
   bin: {
@@ -69,18 +75,20 @@ export class NpmCommand {
   public static runNpmCmd(cmd: string, options = {} as NpmCommandOptions): NpmCommandResult {
     const nodeExecutable = NpmCommand.findNode(options.cliRoot);
     const npmCli = NpmCommand.npmCli();
-    const command = `"${nodeExecutable}" "${npmCli}" ${cmd} --registry=${options.registry ?? ''}`;
-    const npmCmdResult = shelljs.exec(command, {
-      ...options,
-      silent: true,
-      async: false,
+    const args = [npmCli, ...cmd.split(/\s+/), `--registry=${options.registry ?? ''}`];
+    const result = crossSpawn.sync(nodeExecutable, args, {
+      cwd: options.cwd,
       env: npmRunPath.env({ env: process.env }),
     });
-    if (npmCmdResult.code !== 0) {
-      throw new SfError(npmCmdResult.stderr, 'ShellExecError');
+    const stdout = result.stdout?.toString() ?? '';
+    const stderr = result.stderr?.toString() ?? '';
+    const code = result.status ?? 1;
+
+    if (code !== 0) {
+      throw new SfError(stderr, 'ShellExecError');
     }
 
-    return npmCmdResult;
+    return { code, stdout, stderr };
   }
 
   public static npxCli(): string {
@@ -109,7 +117,6 @@ export class NpmCommand {
 
       try {
         if (filepath.endsWith('node')) {
-          // This checks if the filepath is executable on Mac or Linux, if it is not it errors.
           fs.accessSync(filepath, fs.constants.X_OK);
           return true;
         }
@@ -122,24 +129,36 @@ export class NpmCommand {
     if (root) {
       const sfdxBinDirs = NpmCommand.findSfdxBinDirs(root);
       if (sfdxBinDirs.length > 0) {
-        // Find the node executable
-        const node = shelljs.find(sfdxBinDirs).filter((file) => isExecutable(file))[0];
+        const allFiles = sfdxBinDirs.flatMap((dir) => NpmCommand.findFilesRecursively(dir));
+        const node = allFiles.find((file) => isExecutable(file));
         if (node) {
           return fs.realpathSync(node);
         }
       }
     }
 
-    // Check to see if node is installed
-    const nodeShellString = shelljs.which('node');
-    if (nodeShellString?.code === 0 && nodeShellString?.stdout) return nodeShellString.stdout;
+    const nodePath = which.sync('node', { nothrow: true });
+    if (nodePath) return nodePath;
 
     throw setErrorName(
       new SfError('Cannot locate node executable.', 'CannotFindNodeExecutable'),
       'CannotFindNodeExecutable'
     );
   }
 
+  private static findFilesRecursively(dir: string): string[] {
+    const results: string[] = [];
+    for (const entry of fs.readdirSync(dir, { withFileTypes: true })) {
+      const fullPath = path.join(dir, entry.name);
+      if (entry.isDirectory()) {
+        results.push(...NpmCommand.findFilesRecursively(fullPath));
+      } else {
+        results.push(fullPath);
+      }
+    }
+    return results;
+  }
+
   /**
    * Returns the path to the npm-cli.js file in this package's node_modules
    *
@@ -178,14 +197,15 @@ export class NpmModule {
   }
 
   public ping(registry?: string): { registry: string; time: number; details: Record<string, unknown> } {
-    return JSON.parse(NpmCommand.runNpmCmd(`ping ${registry ?? ''} --json`, { json: true, cliRoot: this.cliRoot })) as {
+    const result = NpmCommand.runNpmCmd(`ping ${registry ?? ''} --json`, { json: true, cliRoot: this.cliRoot });
+    return JSON.parse(result.stdout) as {
       registry: string;
       time: number;
       details: Record<string, unknown>;
     };
   }
 
-  public run(command: string): ShellString {
+  public run(command: string): NpmCommandResult {
     return NpmCommand.runNpmCmd(command, { cliRoot: this.cliRoot, json: command.includes('--json') });
   }
 
@@ -221,7 +241,7 @@ export class NpmModule {
     }
   }
 
-  public pack(registry: string, options?: shelljs.ExecOptions): void {
+  public pack(registry: string, options?: NpmCommandOptions): void {
     try {
       NpmCommand.runNpmCmd(`pack ${this.module}@${this.version}`, {
         ...options,
@@ -238,7 +258,7 @@ export class NpmModule {
     return;
   }
 
-  public async fetchTarball(registry: string, options?: shelljs.ExecOptions): Promise<void> {
+  public async fetchTarball(registry: string, options?: NpmCommandOptions): Promise<void> {
     await this.pollForAvailability(() => {
       this.pack(registry, options);
     });