Skip to content

Commit 38790e8

Browse files
rhendricBYK
authored andcommitted
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.)
1 parent 6b57563 commit 38790e8

File tree

6 files changed

+39
-67
lines changed

6 files changed

+39
-67
lines changed

__tests__/commands/run.js

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ test('properly handles extra arguments and pre/post scripts', (): Promise<void>
6464
const pkg = await fs.readJson(path.join(config.cwd, 'package.json'));
6565
const poststart = ['poststart', config, pkg.scripts.poststart, config.cwd];
6666
const prestart = ['prestart', config, pkg.scripts.prestart, config.cwd];
67-
const start = ['start', config, pkg.scripts.start + ' "--hello"', config.cwd];
67+
const start = ['start', config, pkg.scripts.start + ' --hello', config.cwd];
6868

6969
expect(execCommand.mock.calls[0]).toEqual(prestart);
7070
expect(execCommand.mock.calls[1]).toEqual(start);
@@ -75,7 +75,7 @@ test('properly handles extra arguments and pre/post scripts', (): Promise<void>
7575
test('properly handle bin scripts', (): Promise<void> => {
7676
return runRun(['cat-names'], {}, 'bin', config => {
7777
const script = path.join(config.cwd, 'node_modules', '.bin', 'cat-names');
78-
const args = ['cat-names', config, `"${script}"`, config.cwd];
78+
const args = ['cat-names', config, script, config.cwd];
7979

8080
expect(execCommand).toBeCalledWith(...args);
8181
});
@@ -114,19 +114,21 @@ test('properly handle env command', (): Promise<void> => {
114114
});
115115
});
116116

117-
test('retains string delimiters if args have spaces', (): Promise<void> => {
117+
test('adds string delimiters if args have spaces', (): Promise<void> => {
118118
return runRun(['cat-names', '--filter', 'cat names'], {}, 'bin', config => {
119119
const script = path.join(config.cwd, 'node_modules', '.bin', 'cat-names');
120-
const args = ['cat-names', config, `"${script}" "--filter" "cat names"`, config.cwd];
120+
const q = process.platform === 'win32' ? '"' : "'";
121+
const args = ['cat-names', config, `${script} --filter ${q}cat names${q}`, config.cwd];
121122

122123
expect(execCommand).toBeCalledWith(...args);
123124
});
124125
});
125126

126-
test('retains quotes if args have spaces and quotes', (): Promise<void> => {
127+
test('adds quotes if args have spaces and quotes', (): Promise<void> => {
127128
return runRun(['cat-names', '--filter', '"cat names"'], {}, 'bin', config => {
128129
const script = path.join(config.cwd, 'node_modules', '.bin', 'cat-names');
129-
const args = ['cat-names', config, `"${script}" "--filter" "\\"cat names\\""`, config.cwd];
130+
const quotedCatNames = process.platform === 'win32' ? '^"\\^"cat^ names\\^"^"' : `'"cat names"'`;
131+
const args = ['cat-names', config, `${script} --filter ${quotedCatNames}`, config.cwd];
130132

131133
expect(execCommand).toBeCalledWith(...args);
132134
});

__tests__/integration.js

Lines changed: 23 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,9 @@
22
/* eslint max-len: 0 */
33

44
import execa from 'execa';
5+
import {sh} from 'puka';
56
import makeTemp from './_temp.js';
67
import * as fs from '../src/util/fs.js';
7-
import * as misc from '../src/util/misc.js';
88
import * as constants from '../src/constants.js';
99
import {explodeLockfile} from './commands/_helpers.js';
1010

@@ -77,7 +77,7 @@ async function runYarn(args: Array<string> = [], options: Object = {}): Promise<
7777
options['extendEnv'] = false;
7878
}
7979
options['env']['FORCE_COLOR'] = 0;
80-
const {stdout, stderr} = await execa(path.resolve(__dirname, '../bin/yarn'), args, options);
80+
const {stdout, stderr} = await execa.shell(sh`${path.resolve(__dirname, '../bin/yarn')} ${args}`, options);
8181

8282
return [stdout, stderr];
8383
}
@@ -260,9 +260,8 @@ test('yarnrc binary path (executable)', async () => {
260260
expect(stdoutOutput.toString().trim()).toEqual('override called');
261261
});
262262

263-
// Windows could run these tests, but we currently suffer from an escaping issue that breaks them (#4135)
264-
if (process.platform !== 'win32') {
265-
test('yarn run <script> --opt', async () => {
263+
for (const withDoubleDash of [false, true]) {
264+
test(`yarn run <script> ${withDoubleDash ? '-- ' : ''}--opt`, async () => {
266265
const cwd = await makeTemp();
267266

268267
await fs.writeFile(
@@ -272,48 +271,37 @@ if (process.platform !== 'win32') {
272271
}),
273272
);
274273

275-
const command = path.resolve(__dirname, '../bin/yarn');
276274
const options = {cwd, env: {YARN_SILENT: 1}};
277275

278-
const {stderr: stderr, stdout: stdout} = execa(command, ['run', 'echo', '--opt'], options);
279-
280-
const stdoutPromise = misc.consumeStream(stdout);
281-
const stderrPromise = misc.consumeStream(stderr);
282-
283-
const [stdoutOutput, stderrOutput] = await Promise.all([stdoutPromise, stderrPromise]);
276+
const [stdoutOutput, stderrOutput] = await runYarn(
277+
['run', 'echo', ...(withDoubleDash ? ['--'] : []), '--opt'],
278+
options,
279+
);
284280

285281
expect(stdoutOutput.toString().trim()).toEqual('--opt');
286-
expect(stderrOutput.toString()).not.toMatch(
282+
(exp => (withDoubleDash ? exp : exp.not))(expect(stderrOutput.toString())).toMatch(
287283
/From Yarn 1\.0 onwards, scripts don't require "--" for options to be forwarded/,
288284
);
289285
});
286+
}
290287

291-
test('yarn run <script> -- --opt', async () => {
292-
const cwd = await makeTemp();
293-
294-
await fs.writeFile(
295-
path.join(cwd, 'package.json'),
296-
JSON.stringify({
297-
scripts: {echo: `echo`},
298-
}),
299-
);
300-
301-
const command = path.resolve(__dirname, '../bin/yarn');
302-
const options = {cwd, env: {YARN_SILENT: 1}};
288+
test('yarn run <script> <strings that need escaping>', async () => {
289+
const cwd = await makeTemp();
303290

304-
const {stderr: stderr, stdout: stdout} = execa(command, ['run', 'echo', '--', '--opt'], options);
291+
await fs.writeFile(
292+
path.join(cwd, 'package.json'),
293+
JSON.stringify({
294+
scripts: {stringify: `node -p "JSON.stringify(process.argv.slice(1))"`},
295+
}),
296+
);
305297

306-
const stdoutPromise = misc.consumeStream(stdout);
307-
const stderrPromise = misc.consumeStream(stderr);
298+
const options = {cwd, env: {YARN_SILENT: 1}};
308299

309-
const [stdoutOutput, stderrOutput] = await Promise.all([stdoutPromise, stderrPromise]);
300+
const trickyStrings = ['$PWD', '%CD%', '^', '!', '\\', '>', '<', '|', '&', "'", '"', '`', ' '];
301+
const [stdout] = await runYarn(['stringify', ...trickyStrings], options);
310302

311-
expect(stdoutOutput.toString().trim()).toEqual('--opt');
312-
expect(stderrOutput.toString()).toMatch(
313-
/From Yarn 1\.0 onwards, scripts don't require "--" for options to be forwarded/,
314-
);
315-
});
316-
}
303+
expect(stdout.toString().trim()).toEqual(JSON.stringify(trickyStrings));
304+
});
317305

318306
test('cache folder fallback', async () => {
319307
const cwd = await makeTemp();

package.json

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@
3636
"normalize-url": "^1.9.1",
3737
"object-path": "^0.11.2",
3838
"proper-lockfile": "^2.0.0",
39+
"puka": "^1.0.0",
3940
"read": "^1.0.7",
4041
"request": "^2.81.0",
4142
"request-capture-har": "^1.2.2",

src/cli/commands/run.js

Lines changed: 3 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -10,11 +10,7 @@ import map from '../../util/map.js';
1010

1111
const leven = require('leven');
1212
const path = require('path');
13-
14-
// Copied from https://github.com/npm/npm/blob/63f153c743f9354376bfb9dad42bd028a320fd1f/lib/run-script.js#L175
15-
function joinArgs(args: Array<string>): string {
16-
return args.reduce((joinedArgs, arg) => joinedArgs + ' "' + arg.replace(/"/g, '\\"') + '"', '');
17-
}
13+
const {quoteForShell, sh, unquoted} = require('puka');
1814

1915
export function setFlags(commander: Object) {}
2016

@@ -35,7 +31,7 @@ export async function run(config: Config, reporter: Reporter, flags: Object, arg
3531
if (await fs.exists(binFolder)) {
3632
for (const name of await fs.readdir(binFolder)) {
3733
binCommands.push(name);
38-
scripts[name] = `"${path.join(binFolder, name)}"`;
34+
scripts[name] = quoteForShell(path.join(binFolder, name));
3935
}
4036
}
4137
visitedBinFolders.add(binFolder);
@@ -82,8 +78,7 @@ export async function run(config: Config, reporter: Reporter, flags: Object, arg
8278
process.env.YARN_SILENT = '1';
8379
for (const [stage, cmd] of cmds) {
8480
// only tack on trailing arguments for default script, ignore for pre and post - #1595
85-
const defaultScriptCmd = cmd + joinArgs(args);
86-
const cmdWithArgs = stage === action ? defaultScriptCmd : cmd;
81+
const cmdWithArgs = stage === action ? sh`${unquoted(cmd)} ${args}` : cmd;
8782
await execCommand(stage, config, cmdWithArgs, config.cwd);
8883
}
8984
} else if (action === 'env') {

src/util/misc.js

Lines changed: 0 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -2,24 +2,6 @@
22

33
const _camelCase = require('camelcase');
44

5-
export function consumeStream(stream: Object): Promise<Buffer> {
6-
return new Promise((resolve, reject) => {
7-
const buffers = [];
8-
9-
stream.on(`data`, buffer => {
10-
buffers.push(buffer);
11-
});
12-
13-
stream.on(`end`, () => {
14-
resolve(Buffer.concat(buffers));
15-
});
16-
17-
stream.on(`error`, error => {
18-
reject(error);
19-
});
20-
});
21-
}
22-
235
export function sortAlpha(a: string, b: string): number {
246
// sort alphabetically in a deterministic way
257
const shortLen = Math.min(a.length, b.length);

yarn.lock

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3895,6 +3895,10 @@ public-encrypt@^4.0.0:
38953895
parse-asn1 "^5.0.0"
38963896
randombytes "^2.0.1"
38973897

3898+
puka@^1.0.0:
3899+
version "1.0.0"
3900+
resolved "https://registry.yarnpkg.com/puka/-/puka-1.0.0.tgz#1dd92f9f81f6c53390a17529b7aebaa96604ad97"
3901+
38983902
pump@^1.0.0:
38993903
version "1.0.2"
39003904
resolved "https://registry.yarnpkg.com/pump/-/pump-1.0.2.tgz#3b3ee6512f94f0e575538c17995f9f16990a5d51"

0 commit comments

Comments
 (0)