Repository: wix/Detox
Pull request #2398 (2020-10-09 12:26:56)

fix(local-cli): lighter, cross-platform quoting
- [x] This is a small change 

---

**Description:**

It appears that using `shell-quote` npm package in the most straightforward way is not really possible, due to Win32 integration bugs.

That's why I limited the current implementation to use the former Detox technique (escaping with single and double quotes), but I've modernized it a bit, so it is able to pass my updated test suite.

Also, there were a few tests that I had previously changed due to alternative `shell-quote` escaping implementation, so I am reverting them back to the norm.



Patch:
diff --git a/detox/local-cli/utils/shellQuote.js b/detox/local-cli/utils/shellQuote.js
index 35449305e..8174e9c86 100644
--- a/detox/local-cli/utils/shellQuote.js
+++ b/detox/local-cli/utils/shellQuote.js
@@ -1,4 +1,9 @@
-// This is very incomplete, don't use this for user input!
-module.exports = function shellQuote(input) {
-  return process.platform !== 'win32' ? `'${input}'` : `"${input}"`;
+const { autoEscape } = require('../../src/utils/shellUtils');
+
+function quote(argv) {
+  return argv.map(arg => autoEscape(arg)).join(' ');
+}
+
+module.exports = {
+  quote,
 };
diff --git a/detox/package.json b/detox/package.json
index 30a279710..7a88c09b3 100644
--- a/detox/package.json
+++ b/detox/package.json
@@ -55,8 +55,6 @@
     "proper-lockfile": "^3.0.2",
     "resolve-from": "^5.0.0",
     "sanitize-filename": "^1.6.1",
-    "shell-quote": "^1.7.2",
-    "shell-utils": "^1.0.9",
     "signal-exit": "^3.0.3",
     "tail": "^2.0.0",
     "telnet-client": "1.2.8",
diff --git a/detox/src/utils/pipeCommands.js b/detox/src/utils/pipeCommands.js
index 30014edb5..f07e836c6 100644
--- a/detox/src/utils/pipeCommands.js
+++ b/detox/src/utils/pipeCommands.js
@@ -1,16 +1,17 @@
-const SPECIAL_CHARS = /([\^\$\[\]\*\.\\])/g;
-
-const escapeInQuotedString = (fragment) => fragment.replace(/"/g, '\\"');
-const escapeInQuotedRegexp = (fragment) => fragment.replace(SPECIAL_CHARS, "\\$1");
+const {
+  escapeInDoubleQuotedString,
+  escapeInDoubleQuotedRegexp,
+  isRunningInCMDEXE,
+} = require('./shellUtils');
 
 function win32Implementation() {
-  const searchRegexpWin32 = (pattern) => `findstr /R /C:"${escapeInQuotedString(pattern)}"`;
-  const searchFragmentWin32 = (fragment) => `findstr /C:"${escapeInQuotedString(fragment)}"`;
+  const searchRegexpWin32 = (pattern) => `findstr /R /C:"${escapeInDoubleQuotedString(pattern)}"`;
+  const searchFragmentWin32 = (fragment) => `findstr /C:"${escapeInDoubleQuotedString(fragment)}"`;
 
   return {
     escape: {
-      inQuotedString: escapeInQuotedString,
-      inQuotedRegexp: escapeInQuotedRegexp,
+      inQuotedString: escapeInDoubleQuotedString,
+      inQuotedRegexp: escapeInDoubleQuotedRegexp,
     },
     search: {
       regexp: searchRegexpWin32,
@@ -20,13 +21,13 @@ function win32Implementation() {
 }
 
 function nixImplementation() {
-  const searchRegexpNix = (pattern) => `grep "${escapeInQuotedString(pattern)}"`;
-  const searchFragmentNix = (fragment) => `grep -e "${escapeInQuotedString(fragment)}"`;
+  const searchRegexpNix = (pattern) => `grep "${escapeInDoubleQuotedString(pattern)}"`;
+  const searchFragmentNix = (fragment) => `grep -e "${escapeInDoubleQuotedString(fragment)}"`;
 
   return {
     escape: {
-      inQuotedString: escapeInQuotedString,
-      inQuotedRegexp: escapeInQuotedRegexp,
+      inQuotedString: escapeInDoubleQuotedString,
+      inQuotedRegexp: escapeInDoubleQuotedRegexp,
     },
     search: {
       regexp: searchRegexpNix,
@@ -35,8 +36,6 @@ function nixImplementation() {
   };
 }
 
-const isRunningInCMDEXE = process.platform === 'win32' && !process.env['SHELL'];
-
-module.exports = isRunningInCMDEXE
+module.exports = isRunningInCMDEXE()
   ? win32Implementation()
   : nixImplementation();
diff --git a/detox/src/utils/shellUtils.js b/detox/src/utils/shellUtils.js
new file mode 100644
index 000000000..13a4085d2
--- /dev/null
+++ b/detox/src/utils/shellUtils.js
@@ -0,0 +1,66 @@
+const BACK_SLASH = '\\';
+const SINGLE_QUOTE = "'";
+const DOUBLE_QUOTE = '"';
+
+const ESCAPED_DOUBLE_QUOTE = BACK_SLASH + DOUBLE_QUOTE;
+function escapeInDoubleQuotedString(fragment) {
+  return fragment.replace(/"/g, ESCAPED_DOUBLE_QUOTE);
+}
+
+const ESCAPED_SINGLE_QUOTE = SINGLE_QUOTE + DOUBLE_QUOTE + SINGLE_QUOTE + DOUBLE_QUOTE + SINGLE_QUOTE;
+function escapeWithSingleQuotedString(fragment) {
+  return SINGLE_QUOTE + fragment.replace(/'/g, ESCAPED_SINGLE_QUOTE) + SINGLE_QUOTE;
+}
+
+function escapeWithDoubleQuotedString(fragment) {
+  return DOUBLE_QUOTE + escapeInDoubleQuotedString(fragment) + DOUBLE_QUOTE;
+}
+
+const SPECIAL_CHARS = /([\^\$\[\]\*\.\\])/g;
+function escapeInDoubleQuotedRegexp(fragment) {
+  return fragment.replace(SPECIAL_CHARS, '\\$1');
+}
+
+function isRunningInCMDEXE() {
+  return /* istanbul ignore next */ process.platform === 'win32' &&
+         /* istanbul ignore next */ !process.env['SHELL'];
+}
+
+const UNSAFE = /[\s!"#$&'()*;<=>^?`{,}|~\[\\\]]/m;
+/* @see https://unix.stackexchange.com/a/357932 */
+function hasUnsafeShellChars(str) {
+  return UNSAFE.test(str);
+}
+
+function autoEscapeCmd(str) {
+  if (!hasUnsafeShellChars(str)) {
+    return str;
+  }
+
+  return escapeWithDoubleQuotedString(str);
+}
+
+function autoEscapeShell(str) {
+  if (!hasUnsafeShellChars(str)) {
+    return str;
+  }
+
+  return escapeWithSingleQuotedString(str);
+}
+
+const autoEscape = isRunningInCMDEXE()
+  /* istanbul ignore next */ ? autoEscapeCmd
+  /* istanbul ignore next */ : autoEscapeShell;
+
+module.exports = {
+  escapeInDoubleQuotedString,
+  escapeInDoubleQuotedRegexp,
+  escapeWithSingleQuotedString,
+  escapeWithDoubleQuotedString,
+  isRunningInCMDEXE,
+  hasUnsafeShellChars,
+  autoEscape: Object.assign(autoEscape, {
+    cmd: autoEscapeCmd,
+    shell: autoEscapeShell,
+  }),
+};


Test patch:
diff --git a/detox/local-cli/test.js b/detox/local-cli/test.js
index 2d34c1a7b..6e60c9198 100644
--- a/detox/local-cli/test.js
+++ b/detox/local-cli/test.js
@@ -2,13 +2,13 @@ const _ = require('lodash');
 const cp = require('child_process');
 const path = require('path');
 const unparse = require('yargs-unparser');
-const { quote } = require('shell-quote');
+const { quote } = require('./utils/shellQuote');
+const splitArgv = require('./utils/splitArgv');
 const DetoxRuntimeError = require('../src/errors/DetoxRuntimeError');
 const DeviceRegistry = require('../src/devices/DeviceRegistry');
 const { loadLastFailedTests, resetLastFailedTests } = require('../src/utils/lastFailedTests');
 const { composeDetoxConfig } = require('../src/configuration');
 const log = require('../src/utils/logger').child({ __filename });
-const splitArgv = require('./utils/splitArgv');
 const { getPlatformSpecificString, printEnvironmentVariables } = require('./utils/misc');
 const { prependNodeModulesBinToPATH } = require('./utils/misc');
 
diff --git a/detox/local-cli/test.test.js b/detox/local-cli/test.test.js
index 5e9744422..aa197e291 100644
--- a/detox/local-cli/test.test.js
+++ b/detox/local-cli/test.test.js
@@ -51,7 +51,7 @@ describe('CLI', () => {
       test('should pass --use-custom-logger true', () => expect(cliCall().command).toMatch(/--use-custom-logger true/));
       test('should not override process.env', () => expect(cliCall().env).toStrictEqual({}));
       test('should produce a default command (integration test)', () => {
-        const args = `--opts e2e/mocha.opts --grep \\:android\\: --invert --config-path ${detoxConfigPath} --use-custom-logger true`;
+        const args = `--opts e2e/mocha.opts --grep :android: --invert --config-path ${detoxConfigPath} --use-custom-logger true`;
         expect(cliCall().command).toBe(`mocha ${args} e2e`);
       });
     });
@@ -239,7 +239,7 @@ describe('CLI', () => {
       });
 
       test('should produce a default command (integration test, ios)', () => {
-        const args = `--config e2e/config.json --testNamePattern \\^\\(\\(\\?\\!\\:android:\\).\\)\\*\\$ --maxWorkers 1`;
+        const args = `--config e2e/config.json --testNamePattern '^((?!:android:).)*$' --maxWorkers 1`;
         expect(cliCall().command).toBe(`jest ${args} e2e`);
       });
 
@@ -260,7 +260,7 @@ describe('CLI', () => {
       });
 
       test('should produce a default command (integration test)', () => {
-        const args = `--config e2e/config.json --testNamePattern \\^\\(\\(\\?\\!\\:ios:\\).\\)\\*\\$ --maxWorkers 1`;
+        const args = `--config e2e/config.json --testNamePattern '^((?!:ios:).)*$' --maxWorkers 1`;
         expect(cliCall().command).toBe(`jest ${args} e2e`);
       });
 
@@ -285,11 +285,11 @@ describe('CLI', () => {
         detoxConfig.configurations.androidTest.type = 'android.emulator';
 
         await run(`${__configuration} androidTest`);
-        expect(cliCall(0).command).toContain(`--testNamePattern \\^\\(\\(\\?\\!\\:ios:\\).\\)\\*\\$`);
+        expect(cliCall(0).command).toContain(`--testNamePattern '^((?!:ios:).)*$'`);
         expect(cliCall(0).env.configuration).toBe('androidTest');
 
         await run(`${__configuration} iosTest`);
-        expect(cliCall(1).command).toContain(`--testNamePattern \\^\\(\\(\\?\\!\\:android:\\).\\)\\*\\$`);
+        expect(cliCall(1).command).toContain(`--testNamePattern '^((?!:android:).)*$'`);
         expect(cliCall(1).env.configuration).toBe('iosTest');
       }
     );
diff --git a/detox/src/utils/shellUtils.test.js b/detox/src/utils/shellUtils.test.js
new file mode 100644
index 000000000..745c8e0d2
--- /dev/null
+++ b/detox/src/utils/shellUtils.test.js
@@ -0,0 +1,116 @@
+const {
+  escapeInDoubleQuotedString,
+  escapeInDoubleQuotedRegexp,
+  escapeWithSingleQuotedString,
+  escapeWithDoubleQuotedString,
+  isRunningInCMDEXE,
+  hasUnsafeShellChars,
+  autoEscape,
+} = require('./shellUtils');
+
+describe('shellUtils', function() {
+  describe('escapeInDoubleQuotedString', () => {
+    test.each([
+      ['test string', 'test string'],
+      ['"test string"', `\\"test string\\"`],
+    ])('should transform [ %s ] to [ %s ]', (input, expected) => {
+      expect(escapeInDoubleQuotedString(input)).toBe(expected);
+    });
+  });
+
+  describe('escapeInDoubleQuotedRegexp', () => {
+    test.each([
+      ['test string', 'test string'],
+      ['^tes\\t[ ]*.string$', '\\^tes\\\\t\\[ \\]\\*\\.string\\$'],
+    ])('should transform [ %s ] to [ %s ]', (input, expected) => {
+      expect(escapeInDoubleQuotedRegexp(input)).toBe(expected);
+    });
+  });
+
+  describe('escapeWithSingleQuotedString', () => {
+    test.each([
+      ['test string', `'test string'`],
+      ["d'Artagnan", `'d'"'"'Artagnan'`],
+    ])('should transform [ %s ] to [ %s ]', (input, expected) => {
+      expect(escapeWithSingleQuotedString(input)).toBe(expected);
+    });
+  });
+
+  describe('escapeWithDoubleQuotedString', () => {
+    test.each([
+      ['test string', '"test string"'],
+      ['"test string"', `"\\"test string\\""`],
+    ])('should transform [ %s ] to [ %s ]', (input, expected) => {
+      expect(escapeWithDoubleQuotedString(input)).toBe(expected);
+    });
+  });
+
+  describe('isRunningInCMDEXE', () => {
+    test('should return a boolean value', () => {
+      expect(typeof isRunningInCMDEXE()).toBe('boolean');
+    });
+  });
+
+  describe('hasUnsafeShellChars', () => {
+    test.each([
+      /* pin-pointer tests */
+      [false, '',  'just an empty string'],
+      [true, ' ',  'a whitespace character'],
+      [true, '\t', 'a whitespace character'],
+      [true, '\n', 'a newline character'],
+      [true, '!',  'a history expansion'],
+      [true, '"',  'shell syntax'],
+      [true, '#',  'a comment start'],
+      [true, '$',  'shell syntax'],
+      [true, '&',  'shell syntax'],
+      [true, `'`,  'shell syntax'],
+      [true, '(',  'globs and wildcards'],
+      [true, ')',  'globs and wildcards'],
+      [true, '*',  'a sh wildcard'],
+      [true, ';',  'shell syntax'],
+      [true, '<',  'shell syntax'],
+      [true, '=',  'zsh syntax'],
+      [true, '>',  'shell syntax'],
+      [true, '?',  'a sh wildcard'],
+      [true, '[',  'a sh wildcard'],
+      [true, "\\", 'shell syntax'],
+      [true, ']',  'a sh wildcard'],
+      [true, '^',  'a history expansion, zsh wildcard'],
+      [true, '`',  'shell syntax'],
+      [true, '{',  'a brace expansion start'],
+      [true, ',',  'unsafe inside a brace expansion'],
+      [true, '}',  'a brace expansion end'],
+      [true, '|',  'shell syntax'],
+      [true, '~',  'a home directory expansion'],
+      [false, '-',  'almost safe, except when a filename begins with dash, it needs extra handling'],
+      [false, '.',  'almost safe, except that dot files are excluded from * globs by default.'],
+      [false, ':',  'almost safe, expect when it can indicate a remote file (hostname:filename)'],
+
+      /* integration tests */
+      [false, '123-abc-абв.test.js',  'just a mere test filename'],
+      [true, 'some tests/my test.js',  'a filename with spaces'],
+    ])('should return %j for %j because it is %s', (expected, str) => {
+      expect(hasUnsafeShellChars(str)).toBe(expected);
+    });
+  });
+
+  describe('autoEscape.cmd', () => {
+    test.each([
+      ['test', 'test'],
+      ['test string', '"test string"'],
+      ['test "this" string', '"test \\"this\\" string"'],
+    ])('should transform [ %s ] to [ %s ]', (input, expected) => {
+      expect(autoEscape.cmd(input)).toBe(expected);
+    });
+  });
+
+  describe('autoEscape.shell', () => {
+    test.each([
+      ['test', 'test'],
+      ["test string", "'test string'"],
+      ["test 'this' string", `'test '"'"'this'"'"' string'`],
+    ])('should transform [ %s ] to [ %s ]', (input, expected) => {
+      expect(autoEscape.shell(input)).toBe(expected);
+    });
+  });
+});