From a3b698d0169c1040e6ea7c9e27e65996272aa07a Mon Sep 17 00:00:00 2001
From: Ryan Hendrickson <ryan.hendrickson@alum.mit.edu>
Date: Mon, 2 Oct 2017 18:29:18 -0400
Subject: [PATCH] Fix: improve escaping for script arguments

**Summary**

Extra command-line arguments to scripts were not being escaped
correctly. This patch uses puka to add robust shell quoting 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.
---
 __tests__/commands/run.js | 14 ++++++----
 __tests__/integration.js  | 58 ++++++++++++++++-----------------------
 package.json              |  1 +
 src/cli/commands/run.js   | 11 ++------
 src/util/misc.js          | 18 ------------
 yarn.lock                 |  4 +++
 6 files changed, 39 insertions(+), 67 deletions(-)

diff --git a/__tests__/commands/run.js b/__tests__/commands/run.js
index 386c7e3bd7..7477289c04 100644
--- a/__tests__/commands/run.js
+++ b/__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);
   });
diff --git a/__tests__/integration.js b/__tests__/integration.js
index 5c44f2310a..4533f8c55f 100644
--- a/__tests__/integration.js
+++ b/__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';
 
 jasmine.DEFAULT_TIMEOUT_INTERVAL = 120000;
@@ -76,7 +76,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];
 }
@@ -211,9 +211,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(
@@ -223,48 +222,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();
diff --git a/package.json b/package.json
index e59a66929f..a4069bf14a 100644
--- a/package.json
+++ b/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",
diff --git a/src/cli/commands/run.js b/src/cli/commands/run.js
index a26a663d71..f3c941ae65 100644
--- a/src/cli/commands/run.js
+++ b/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') {
diff --git a/src/util/misc.js b/src/util/misc.js
index 1725a666ce..440ec2f98d 100644
--- a/src/util/misc.js
+++ b/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);
diff --git a/yarn.lock b/yarn.lock
index 704f9a0c58..2ce753ca65 100644
--- a/yarn.lock
+++ b/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"