fix(run): improve escaping for script arguments (#4135)

**Summary**

Extra command-line arguments to scripts were not being escaped correctly. This patch adds robust shell quoting logic for both Windows and Linux/macOS.

**Test plan**

On *nix, create a `package.json` containing `"scripts":{"echo":"echo"}`. Run `yarn run -s echo -- '$X \"blah\"'`. Expect to observe ` \blah\` prior to this patch, and `$X \"blah\"` after it.

Testing on Windows should be similar, but may require fancier escaping to get the arguments into yarn in the first place. (I don't have access to a Windows box to verify the exact procedure to follow, sorry—but I did confirm that my automated tests succeed in AppVeyor.)

Author: rhendric

__tests__/commands/run.js

@@ -64,7 +64,7 @@ test('properly handles extra arguments and pre/post scripts', (): Promise<void>
     const pkg = await fs.readJson(path.join(config.cwd, 'package.json'));
     const poststart = ['poststart', config, pkg.scripts.poststart, config.cwd];
     const prestart = ['prestart', config, pkg.scripts.prestart, config.cwd];
-    const start = ['start', config, pkg.scripts.start + ' "--hello"', config.cwd];
+    const start = ['start', config, pkg.scripts.start + ' --hello', config.cwd];
 
     expect(execCommand.mock.calls[0]).toEqual(prestart);
     expect(execCommand.mock.calls[1]).toEqual(start);
@@ -75,7 +75,7 @@ test('properly handles extra arguments and pre/post scripts', (): Promise<void>
 test('properly handle bin scripts', (): Promise<void> => {
   return runRun(['cat-names'], {}, 'bin', config => {
     const script = path.join(config.cwd, 'node_modules', '.bin', 'cat-names');
-    const args = ['cat-names', config, `"${script}"`, config.cwd];
+    const args = ['cat-names', config, script, config.cwd];
 
     expect(execCommand).toBeCalledWith(...args);
   });
@@ -114,19 +114,21 @@ test('properly handle env command', (): Promise<void> => {
   });
 });
 
-test('retains string delimiters if args have spaces', (): Promise<void> => {
+test('adds string delimiters if args have spaces', (): Promise<void> => {
   return runRun(['cat-names', '--filter', 'cat names'], {}, 'bin', config => {
     const script = path.join(config.cwd, 'node_modules', '.bin', 'cat-names');
-    const args = ['cat-names', config, `"${script}" "--filter" "cat names"`, config.cwd];
+    const q = process.platform === 'win32' ? '"' : "'";
+    const args = ['cat-names', config, `${script} --filter ${q}cat names${q}`, config.cwd];
 
     expect(execCommand).toBeCalledWith(...args);
   });
 });
 
-test('retains quotes if args have spaces and quotes', (): Promise<void> => {
+test('adds quotes if args have spaces and quotes', (): Promise<void> => {
   return runRun(['cat-names', '--filter', '"cat names"'], {}, 'bin', config => {
     const script = path.join(config.cwd, 'node_modules', '.bin', 'cat-names');
-    const args = ['cat-names', config, `"${script}" "--filter" "\\"cat names\\""`, config.cwd];
+    const quotedCatNames = process.platform === 'win32' ? '^"\\^"cat^ names\\^"^"' : `'"cat names"'`;
+    const args = ['cat-names', config, `${script} --filter ${quotedCatNames}`, config.cwd];
 
     expect(execCommand).toBeCalledWith(...args);
   });

__tests__/integration.js

@@ -2,9 +2,9 @@
 /* eslint max-len: 0 */
 
 import execa from 'execa';
+import {sh} from 'puka';
 import makeTemp from './_temp.js';
 import * as fs from '../src/util/fs.js';
-import * as misc from '../src/util/misc.js';
 import * as constants from '../src/constants.js';
 import {explodeLockfile} from './commands/_helpers.js';
 
@@ -77,7 +77,7 @@ async function runYarn(args: Array<string> = [], options: Object = {}): Promise<
     options['extendEnv'] = false;
   }
   options['env']['FORCE_COLOR'] = 0;
-  const {stdout, stderr} = await execa(path.resolve(__dirname, '../bin/yarn'), args, options);
+  const {stdout, stderr} = await execa.shell(sh`${path.resolve(__dirname, '../bin/yarn')} ${args}`, options);
 
   return [stdout, stderr];
 }
@@ -260,9 +260,8 @@ test('yarnrc binary path (executable)', async () => {
   expect(stdoutOutput.toString().trim()).toEqual('override called');
 });
 
-// Windows could run these tests, but we currently suffer from an escaping issue that breaks them (#4135)
-if (process.platform !== 'win32') {
-  test('yarn run <script> --opt', async () => {
+for (const withDoubleDash of [false, true]) {
+  test(`yarn run <script> ${withDoubleDash ? '-- ' : ''}--opt`, async () => {
     const cwd = await makeTemp();
 
     await fs.writeFile(
@@ -272,48 +271,37 @@ if (process.platform !== 'win32') {
       }),
     );
 
-    const command = path.resolve(__dirname, '../bin/yarn');
     const options = {cwd, env: {YARN_SILENT: 1}};
 
-    const {stderr: stderr, stdout: stdout} = execa(command, ['run', 'echo', '--opt'], options);
-
-    const stdoutPromise = misc.consumeStream(stdout);
-    const stderrPromise = misc.consumeStream(stderr);
-
-    const [stdoutOutput, stderrOutput] = await Promise.all([stdoutPromise, stderrPromise]);
+    const [stdoutOutput, stderrOutput] = await runYarn(
+      ['run', 'echo', ...(withDoubleDash ? ['--'] : []), '--opt'],
+      options,
+    );
 
     expect(stdoutOutput.toString().trim()).toEqual('--opt');
-    expect(stderrOutput.toString()).not.toMatch(
+    (exp => (withDoubleDash ? exp : exp.not))(expect(stderrOutput.toString())).toMatch(
       /From Yarn 1\.0 onwards, scripts don't require "--" for options to be forwarded/,
     );
   });
+}
 
-  test('yarn run <script> -- --opt', async () => {
-    const cwd = await makeTemp();
-
-    await fs.writeFile(
-      path.join(cwd, 'package.json'),
-      JSON.stringify({
-        scripts: {echo: `echo`},
-      }),
-    );
-
-    const command = path.resolve(__dirname, '../bin/yarn');
-    const options = {cwd, env: {YARN_SILENT: 1}};
+test('yarn run <script> <strings that need escaping>', async () => {
+  const cwd = await makeTemp();
 
-    const {stderr: stderr, stdout: stdout} = execa(command, ['run', 'echo', '--', '--opt'], options);
+  await fs.writeFile(
+    path.join(cwd, 'package.json'),
+    JSON.stringify({
+      scripts: {stringify: `node -p "JSON.stringify(process.argv.slice(1))"`},
+    }),
+  );
 
-    const stdoutPromise = misc.consumeStream(stdout);
-    const stderrPromise = misc.consumeStream(stderr);
+  const options = {cwd, env: {YARN_SILENT: 1}};
 
-    const [stdoutOutput, stderrOutput] = await Promise.all([stdoutPromise, stderrPromise]);
+  const trickyStrings = ['$PWD', '%CD%', '^', '!', '\\', '>', '<', '|', '&', "'", '"', '`', '  '];
+  const [stdout] = await runYarn(['stringify', ...trickyStrings], options);
 
-    expect(stdoutOutput.toString().trim()).toEqual('--opt');
-    expect(stderrOutput.toString()).toMatch(
-      /From Yarn 1\.0 onwards, scripts don't require "--" for options to be forwarded/,
-    );
-  });
-}
+  expect(stdout.toString().trim()).toEqual(JSON.stringify(trickyStrings));
+});
 
 test('cache folder fallback', async () => {
   const cwd = await makeTemp();

package.json

@@ -36,6 +36,7 @@
     "normalize-url": "^1.9.1",
     "object-path": "^0.11.2",
     "proper-lockfile": "^2.0.0",
+    "puka": "^1.0.0",
     "read": "^1.0.7",
     "request": "^2.81.0",
     "request-capture-har": "^1.2.2",

src/cli/commands/run.js

@@ -10,11 +10,7 @@ import map from '../../util/map.js';
 
 const leven = require('leven');
 const path = require('path');
-
-// Copied from https://github.com/npm/npm/blob/63f153c743f9354376bfb9dad42bd028a320fd1f/lib/run-script.js#L175
-function joinArgs(args: Array<string>): string {
-  return args.reduce((joinedArgs, arg) => joinedArgs + ' "' + arg.replace(/"/g, '\\"') + '"', '');
-}
+const {quoteForShell, sh, unquoted} = require('puka');
 
 export function setFlags(commander: Object) {}
 
@@ -35,7 +31,7 @@ export async function run(config: Config, reporter: Reporter, flags: Object, arg
       if (await fs.exists(binFolder)) {
         for (const name of await fs.readdir(binFolder)) {
           binCommands.push(name);
-          scripts[name] = `"${path.join(binFolder, name)}"`;
+          scripts[name] = quoteForShell(path.join(binFolder, name));
         }
       }
       visitedBinFolders.add(binFolder);
@@ -82,8 +78,7 @@ export async function run(config: Config, reporter: Reporter, flags: Object, arg
       process.env.YARN_SILENT = '1';
       for (const [stage, cmd] of cmds) {
         // only tack on trailing arguments for default script, ignore for pre and post - #1595
-        const defaultScriptCmd = cmd + joinArgs(args);
-        const cmdWithArgs = stage === action ? defaultScriptCmd : cmd;
+        const cmdWithArgs = stage === action ? sh`${unquoted(cmd)} ${args}` : cmd;
         await execCommand(stage, config, cmdWithArgs, config.cwd);
       }
     } else if (action === 'env') {

src/util/misc.js

@@ -2,24 +2,6 @@
 
 const _camelCase = require('camelcase');
 
-export function consumeStream(stream: Object): Promise<Buffer> {
-  return new Promise((resolve, reject) => {
-    const buffers = [];
-
-    stream.on(`data`, buffer => {
-      buffers.push(buffer);
-    });
-
-    stream.on(`end`, () => {
-      resolve(Buffer.concat(buffers));
-    });
-
-    stream.on(`error`, error => {
-      reject(error);
-    });
-  });
-}
-
 export function sortAlpha(a: string, b: string): number {
   // sort alphabetically in a deterministic way
   const shortLen = Math.min(a.length, b.length);

yarn.lock

@@ -3895,6 +3895,10 @@ public-encrypt@^4.0.0:
     parse-asn1 "^5.0.0"
     randombytes "^2.0.1"
 
+puka@^1.0.0:
+  version "1.0.0"
+  resolved "https://registry.yarnpkg.com/puka/-/puka-1.0.0.tgz#1dd92f9f81f6c53390a17529b7aebaa96604ad97"
+
 pump@^1.0.0:
   version "1.0.2"
   resolved "https://registry.yarnpkg.com/pump/-/pump-1.0.2.tgz#3b3ee6512f94f0e575538c17995f9f16990a5d51"