Escape arguments when installer invokes commands

The usage of shell: true means that no escaping is done on the arguments
passed to the commands invoked by the installer. This leads to the
installation failing if the user for example puts a space in the
username for the admin account.

Fix by escaping the arguments passed on command lines. This is
unreasonably complicated on Windows 😡.

packages/create/create.js

@@ -16,6 +16,7 @@ const { copyPluginTemplates } = require("./template");
 let dev = false;
 const scriptExt = process.platform === "win32" ? ".cmd" : "";
 const finished = util.promisify(stream.finished);
+const { escapeArg } = require("./escape_arg");
 
 // I hate this, there is a bug with rl.close on windows that was meant to have been fixed in Node14.4
 // See nodejs/node#21771 and nodejs/node#30701 for the apparent fix that was applied
@@ -106,15 +107,25 @@ async function safeOutputFile(file, data, options={}) {
 }
 
 async function execFile(cmd, args) {
-	logger.verbose(`executing ${cmd} ${args.join(" ")}`);
+	const escaped = args.map(escapeArg);
+	logger.verbose(`executing ${cmd} ${escaped.join(" ")}`);
 	return new Promise((resolve, reject) => {
-		let child = child_process.execFile(cmd, args, { shell: true }, (err, stdout, stderr) => {
-			if (err) {
-				reject(err);
-			} else {
-				resolve({ stdout, stderr });
+		let child = child_process.execFile(
+			cmd,
+			escaped,
+			{
+				shell: true,
+				// eslint-disable-next-line node/no-process-env
+				env: process.platform === "win32" ? { ...process.env, pct: "%" } : undefined,
+			},
+			(err, stdout, stderr) => {
+				if (err) {
+					reject(err);
+				} else {
+					resolve({ stdout, stderr });
+				}
 			}
-		});
+		);
 		let stdout = new LineSplitter({ readableObjectMode: true });
 		stdout.on("data", line => { logger.verbose(line.toString()); });
 		child.stdout.pipe(stdout);

packages/create/escape_arg.js

@@ -0,0 +1,62 @@
+"use strict";
+
+function escapeArg(arg) {
+	// When shell: true is used execFile does not escaping of arguments, they are
+	// simply joined by space and then passed to the shell.
+
+	// For Windows the command is cmd.exe /d /s /c "command arg arg ..."
+	// The parsing and escaping on windows is cursed. The following references are not for mortals:
+	// https://daviddeley.com/autohotkey/parameters/parameters.htm#WIN
+	// https://stackoverflow.com/a/4095133
+	// https://ss64.com/nt/syntax-esc.html
+	if (process.platform === "win32") {
+		if (!/[\t "%&<>\\^|]/.test(arg)) {
+			return arg;
+		}
+
+		const escaped = [];
+		let pos = 0;
+		while (pos < arg.length) {
+			if (arg[pos] === "\\") {
+				// If a backslash is followed by a quote or the end of the string, then
+				// double the number of backslashes because why not!
+				let peek = pos + 1;
+				while (peek !== arg.length && arg[peek] === "\\") {
+					peek += 1;
+				}
+				if (peek === arg.length || arg[peek] === '"') {
+					const count = peek - pos;
+					escaped.push("\\".repeat(count * 2));
+					pos = peek;
+					continue;
+				} else {
+					escaped.push("\\");
+				}
+			} else if (arg[pos] === '"') {
+				escaped.push('""');
+			} else if (arg[pos] === "%") {
+				// Expect a pct environment variable to contain %, because cmd does variable substitutions
+				// before parsing string quotes, and there's no mechanism for escaping % symbols, and this
+				// is a half sane workaround. The less sane workaround is to replace % with %%cd:~,%.
+				escaped.push("%pct%");
+			} else {
+				escaped.push(arg[pos]);
+			}
+			pos += 1;
+		}
+		return `"${escaped.join("")}"`;
+	}
+
+	// For all other platforms the command is /bin/sh -c <command arg arg ...>
+	// The command and its arguments are passed as a single argument to the shell.
+	// See https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_02
+	// eslint-disable-next-line no-lonely-if
+	if (!/[|&;<>()$`\\"' \t*?\[#~=%]/.test(arg)) {
+		return arg;
+	}
+	return `"${arg.replace(/[$`"\\]/g, "\\$&")}"`;
+}
+
+module.exports = {
+	escapeArg,
+};

test/create/echo_args.js

@@ -0,0 +1,4 @@
+"use strict";
+if (module === require.main) {
+	process.stdout.write(JSON.stringify(process.argv.slice(2)));
+}

test/create/escape_arg.js

@@ -0,0 +1,71 @@
+"use strict";
+const child_process = require("child_process");
+const util = require("util");
+const assert = require("assert").strict;
+const path = require("path");
+
+const { escapeArg } = require("../../packages/create/escape_arg");
+const execFile = util.promisify(child_process.execFile);
+
+async function exec(file, args) {
+	const { stdout, stderr } = await execFile(
+		file,
+		args.map(escapeArg),
+		// eslint-disable-next-line node/no-process-env
+		{ shell: true, cwd: __dirname, env: { ...process.env, pct: "%"} }
+	);
+	return { stdout: JSON.parse(stdout), stderr };
+}
+
+const strings = [
+	'"a"',
+	"a sentence of text.",
+	" irregular  \tspaces   ",
+	'"C:\\path\\"',
+	"C:\\path\\",
+	'\\ "\\" \\\\ "\\\\" \\\\\\ "\\\\\\" \\\\\\\\ "\\\\\\\\" \\\\\\\\\\ "\\\\\\\\\\" \\\\\\\\\\\ "\\\\\\\\\\\\"',
+	'"C:\\path"',
+	"C:\\path",
+	"%OS%",
+	"!OS!",
+	"😮",
+];
+for (let i=1; i < 128; i++) {
+	const char = String.fromCodePoint(i);
+	// Skip newline and carriage return because those are difficult to escape
+	// Skip most alphanumeric characters as they don't behave differently to each other.
+	if (/[\n\r1-9B-Zb-z]/.test(char)) {
+		continue;
+	}
+	strings.push(char, `${char}${char}`, `${char}"`);
+}
+
+// Test 100 strings at a time.
+const tests = [[]];
+for (const string of strings) {
+	if (tests[tests.length - 1].length >= 100) {
+		tests.push([]);
+	}
+	tests[tests.length - 1].push(string);
+}
+
+
+describe("create/escape_arg", function() {
+	it("should correctly escape strings when args are forwarded", async function() {
+		const ext = process.platform === "win32" ? ".cmd" : ".sh";
+		for (const args of tests) {
+			assert.deepEqual(
+				await exec(`.${path.sep}forward_args${ext}`, ["echo_args.js", ...args]),
+				{ stdout: args, stderr: "" }
+			);
+		}
+	});
+	it("should correctly escape strings when invoking node directly", async function() {
+		for (const args of tests) {
+			assert.deepEqual(
+				await exec("node", ["echo_args.js", ...args]),
+				{ stdout: args, stderr: "" }
+			);
+		}
+	});
+});

test/create/forward_args.cmd

@@ -0,0 +1,3 @@
+@ECHO off
+REM this is how npm and package.json bin entries forwards arguments on Windows
+node.exe %*

test/create/forward_args.sh

@@ -0,0 +1,3 @@
+#!/bin/sh
+# this is how package.json bin entries forwards arguments on Linux
+exec node "$@"