fix: escape shell arg single quotes

README.md

@@ -456,7 +456,9 @@ after fix:
 
 ### .escapeShellArg()
 
-Escape command line arguments. Add single quotes around a string and quotes/escapes any existing single quotes allowing you to pass a string directly to a shell function and having it be treated as a single safe argument.
+Escape POSIX shell command line arguments. Add single quotes around a string and quotes/escapes any existing single quotes allowing you to pass a string directly to a shell function and having it be treated as a single safe argument.
+
+Prefer `child_process.execFile()` or `child_process.spawn()` with an arguments array when possible. This helper is for a single argument in a POSIX shell command string, and is not a Windows `cmd.exe` or PowerShell escaping helper.
 
 ```js
 const ip = '127.0.0.1 && cat /etc/passwd'

README.zh-CN.md

@@ -367,7 +367,9 @@ __处理过程较复杂，性能损耗较大，尽量避免使用__
 
 ### .escapeShellArg()
 
-命令行参数转义。给字符串增加一对单引号并且能引用或者转码任何已经存在的单引号， 这样以确保能够直接将一个字符串传入 shell 函数，并且还是确保安全的。
+POSIX shell 命令行参数转义。给字符串增加一对单引号并且能引用或者转码任何已经存在的单引号， 这样以确保能够直接将一个字符串传入 shell 函数，并且还是确保安全的。
+
+如果可以，优先使用 `child_process.execFile()` 或 `child_process.spawn()` 的参数数组。这个 helper 只用于 POSIX shell 命令字符串里的单个参数，不适用于 Windows `cmd.exe` 或 PowerShell 转义。
 
 ```js
 const ip = '127.0.0.1 && cat /etc/passwd'

lib/helper/escapeShellArg.js

@@ -3,7 +3,7 @@
 function escapeShellArg(string) {
 
   const str = '' + string;
-  return '\'' + str.replace(/\\/g, '\\\\').replace(/\'/g, '\\\'') + '\'';
+  return '\'' + str.replace(/'/g, '\'\\\'\'') + '\'';
 
 }
 module.exports = escapeShellArg;

test/app/extends/cliFilter.test.js

@@ -10,7 +10,10 @@ describe('test/app/extends/cliFilter.test.js', () => {
     return app.ready();
   });
 
-  after(mm.restore);
+  after(async () => {
+    await app.close();
+    await mm.restore();
+  });
 
   describe('helper.cliFilter()', () => {
     it('should convert special chars in param and not convert chars in whitelists', () => {

test/app/extends/escapeShellArg.test.js

@@ -1,5 +1,17 @@
+// eslint-disable-next-line no-restricted-modules
+const assert = require('assert');
+// eslint-disable-next-line no-restricted-modules
+const { exec: childProcessExec } = require('child_process');
+// eslint-disable-next-line no-restricted-modules
+const { promisify } = require('util');
+
 const mm = require('egg-mock');
 
+const escapeShellArg = require('../../../lib/helper/escapeShellArg');
+
+const exec = promisify(childProcessExec);
+const itOnPosix = process.platform === 'win32' ? it.skip : it;
+
 describe('test/app/extends/escapeShellArg.test.js', () => {
   let app;
   before(() => {
@@ -10,7 +22,10 @@ describe('test/app/extends/escapeShellArg.test.js', () => {
     return app.ready();
   });
 
-  after(mm.restore);
+  after(async () => {
+    await app.close();
+    await mm.restore();
+  });
 
   describe('helper.escapeShellArg()', () => {
     it('should add single quotes around a string', () => {
@@ -33,5 +48,12 @@ describe('test/app/extends/escapeShellArg.test.js', () => {
         .expect(200)
         .expect('true');
     });
+
+    itOnPosix('should keep single quotes inside one shell argument', async () => {
+      const payload = '\'; echo EGG_SECURITY_INJECTED; #';
+      const { stdout } = await exec(`printf 'ARG:%s\\n' ${escapeShellArg(payload)}`);
+
+      assert.strictEqual(stdout, 'ARG:\'; echo EGG_SECURITY_INJECTED; #\n');
+    });
   });
 });

test/app/extends/escapeShellCmd.test.js

@@ -10,7 +10,10 @@ describe('test/app/extends/escapeShellCmd.test.js', () => {
     return app.ready();
   });
 
-  after(mm.restore);
+  after(async () => {
+    await app.close();
+    await mm.restore();
+  });
 
   describe('helper.escapeShellCmd()', () => {
     it('should convert chars in blacklists', () => {

test/app/extends/helper.test.js

@@ -25,7 +25,10 @@ describe('test/app/extends/helper.test.js', () => {
     await app3.ready();
   });
 
-  after(mm.restore);
+  after(async () => {
+    await Promise.all([ app, app2, app3 ].map(app => app.close()));
+    await mm.restore();
+  });
 
   describe('helper.escape()', () => {
     it('should work', () => {

test/app/extends/sjs.test.js

@@ -10,7 +10,10 @@ describe('test/app/extends/sjs.test.js', () => {
     return app.ready();
   });
 
-  after(mm.restore);
+  after(async () => {
+    await app.close();
+    await mm.restore();
+  });
 
   describe('helper.sjs()', () => {
     it('should convert special chars on js context and not convert chart in whitelists', () => {

test/app/extends/sjson.test.js

@@ -10,7 +10,10 @@ describe('test/app/extends/sjson.test.js', () => {
     return app.ready();
   });
 
-  after(mm.restore);
+  after(async () => {
+    await app.close();
+    await mm.restore();
+  });
 
   describe('helper.sjson()', () => {
     it('should not convert json string when json is safe', () => {

test/app/extends/spath.test.js

@@ -10,7 +10,10 @@ describe('test/app/extends/spath.test.js', () => {
     return app.ready();
   });
 
-  after(mm.restore);
+  after(async () => {
+    await app.close();
+    await mm.restore();
+  });
 
   describe('helper.spath()', () => {
     it('should pass when filepath is safe', () => {

test/dta.test.js

@@ -8,6 +8,20 @@ function sleep(ms) {
   });
 }
 
+async function waitForLog(app, text, loggerName) {
+  let lastError;
+  for (let i = 0; i < 20; i++) {
+    try {
+      app.expectLog(text, loggerName);
+      return;
+    } catch (err) {
+      lastError = err;
+      await sleep(100);
+    }
+  }
+  throw lastError;
+}
+
 describe('test/dta.test.js', () => {
   let app;
   before(() => {
@@ -81,8 +95,7 @@ describe('test/dta.test.js', () => {
     await app.httpRequest()
       .get('/%2c%2f%')
       .expect(404);
-    if (process.platform === 'win32') await sleep(2000);
-    app.expectLog('decode file path', 'coreLogger');
+    await waitForLog(app, 'decode file path', 'coreLogger');
   });
 
 });

test/fixtures/apps/helper-escapeShellArg-app/app/router.js

@@ -6,7 +6,7 @@ module.exports = app => {
 
   app.get('/escapeShellArg-2', async function() {
     const port = '8889\'|chmod 777 /tmp/muma.sh;echo \\';
-    this.body = `cp.exec('./start.sh '+${this.helper.escapeShellArg(port)})` === 'cp.exec(\'./start.sh \'+\'8889\\\'|chmod 777 /tmp/muma.sh;echo \\\\\')';
+    this.body = `cp.exec('./start.sh '+${this.helper.escapeShellArg(port)})` === 'cp.exec(\'./start.sh \'+\'8889\'\\\'\'|chmod 777 /tmp/muma.sh;echo \\\')';
   });
 
   app.get('/escapeShellArg-3', async function() {