Fix: improve escaping for script arguments

**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.

Author: rhendric

__tests__/commands/run-integration.js

@@ -0,0 +1,57 @@
+/* @flow */
+
+import makeTemp from '../_temp.js';
+import * as fs from '../../src/util/fs.js';
+import {run as buildRun} from './_helpers.js';
+import {BufferReporter} from '../../src/reporters/index.js';
+import {run} from '../../src/cli/commands/run.js';
+
+const path = require('path');
+
+const fixturesLoc = path.join(__dirname, '..', 'fixtures', 'run');
+const runRun = buildRun.bind(null, BufferReporter, fixturesLoc, (args, flags, config, reporter): Promise<void> => {
+  return run(config, reporter, flags, args);
+});
+
+jasmine.DEFAULT_TIMEOUT_INTERVAL = 150000;
+
+test('fully quote and escape extra args to run', async () => {
+  const trickyStrings = ['$X', '%X%', '^', '!', '\\', '>', '<', '|', '&', "'", '"', '`', '  '];
+  const cases = [];
+  for (const a of trickyStrings) {
+    for (const b of trickyStrings) {
+      cases.push(a + b + b + b);
+      if (a !== b) {
+        cases.push(a + a + b + b);
+        cases.push(a + b + a + b);
+        cases.push(a + b + b + a);
+        cases.push(a + a + a + b);
+        cases.push(a + a + b + a);
+        cases.push(a + b + a + a);
+      }
+    }
+  }
+  cases.sort(() => Math.random() - 0.5);
+  const tempDir = await makeTemp();
+  const prefix = path.resolve(__dirname, '../..');
+  const origPrefix = process.env.PREFIX;
+  process.env.PREFIX = prefix;
+  try {
+    const batch = 10;
+    const jobs = [];
+    for (let i = 0; i < 30; i += batch) {
+      const args = cases.slice(i, i + batch);
+      const outFile = path.join(tempDir, `out${Math.random().toString(16).slice(2)}`);
+      jobs.push(
+        runRun(['write-args', outFile, ...args], {}, 'escape-args', async () => {
+          const output = await fs.readFile(outFile);
+          expect(output).toBe(args.join(' '));
+        }),
+      );
+    }
+    await Promise.all(jobs);
+  } finally {
+    process.env.PREFIX = origPrefix;
+    await fs.unlink(tempDir);
+  }
+});

__tests__/commands/run.js

@@ -13,6 +13,7 @@ jasmine.DEFAULT_TIMEOUT_INTERVAL = 90000;
 const execCommand: $FlowFixMe = require('../../src/util/execute-lifecycle-script').execCommand;
 
 const path = require('path');
+const q = process.platform === 'win32' ? '"' : "'";
 
 beforeEach(() => execCommand.mockClear());
 
@@ -58,7 +59,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 + ` ${q}--hello${q}`, config.cwd];
 
     expect(execCommand.mock.calls[0]).toEqual(prestart);
     expect(execCommand.mock.calls[1]).toEqual(start);
@@ -69,7 +70,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, `${q}${script}${q}`, config.cwd];
 
     expect(execCommand).toBeCalledWith(...args);
   });
@@ -88,7 +89,7 @@ test('properly handle env command', (): Promise<void> => {
 test('retains 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 args = ['cat-names', config, `${q}${script}${q} ${q}--filter${q} ${q}cat names${q}`, config.cwd];
 
     expect(execCommand).toBeCalledWith(...args);
   });
@@ -97,7 +98,9 @@ test('retains string delimiters if args have spaces', (): Promise<void> => {
 test('retains 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 cq = q === '"' ? '^"' : q;
+    const dq = q === '"' ? '\\^"' : '"';
+    const args = ['cat-names', config, `${q}${script}${q} ${q}--filter${q} ${cq}${dq}cat names${dq}${cq}`, config.cwd];
 
     expect(execCommand).toBeCalledWith(...args);
   });

__tests__/fixtures/run/escape-args/package.json

@@ -0,0 +1,5 @@
+{
+  "scripts": {
+    "write-args": "node write-args.js"
+  }
+}

__tests__/fixtures/run/escape-args/write-args.js

@@ -0,0 +1 @@
+require('fs').writeFileSync(process.argv[2], process.argv.slice(3).join(' '));

src/cli/commands/run.js

@@ -11,9 +11,22 @@ 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 quoteForCmd(arg: string): string {
+  // See the below blog post for an explanation of what's going on here:
+  // eslint-disable-next-line max-len
+  // https://blogs.msdn.microsoft.com/twistylittlepassagesallalike/2011/04/23/everyone-quotes-command-line-arguments-the-wrong-way/
+  const caretEscape = /"/.test(arg);
+  arg = `"${arg.replace(/\\+(?=$|")/g, '$&$&').replace(/"/g, '\\"')}"`;
+  if (caretEscape) {
+    arg = arg.replace(/[()%!^"<>&|]/g, '^$&');
+  }
+  return arg;
+}
+
+const quoteForShell = process.platform === 'win32' ? quoteForCmd : arg => `'${arg.replace(/'/g, "'\\''")}'`;
+
 function joinArgs(args: Array<string>): string {
-  return args.reduce((joinedArgs, arg) => joinedArgs + ' "' + arg.replace(/"/g, '\\"') + '"', '');
+  return args.length ? ' ' + args.map(quoteForShell).join(' ') : '';
 }
 
 export function setFlags(commander: Object) {}
@@ -35,7 +48,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);