diff --git a/README.md b/README.md index 9e84af3..e172a24 100644 --- a/README.md +++ b/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' diff --git a/README.zh-CN.md b/README.zh-CN.md index fdc1b17..3518b6b 100644 --- a/README.zh-CN.md +++ b/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' diff --git a/lib/helper/escapeShellArg.js b/lib/helper/escapeShellArg.js index ff18c05..68c9a23 100644 --- a/lib/helper/escapeShellArg.js +++ b/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; diff --git a/test/app/extends/cliFilter.test.js b/test/app/extends/cliFilter.test.js index a51f9bf..b159089 100644 --- a/test/app/extends/cliFilter.test.js +++ b/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', () => { diff --git a/test/app/extends/escapeShellArg.test.js b/test/app/extends/escapeShellArg.test.js index 4b2e3a4..f54515a 100644 --- a/test/app/extends/escapeShellArg.test.js +++ b/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'); + }); }); }); diff --git a/test/app/extends/escapeShellCmd.test.js b/test/app/extends/escapeShellCmd.test.js index d8985ce..091c9e7 100644 --- a/test/app/extends/escapeShellCmd.test.js +++ b/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', () => { diff --git a/test/app/extends/helper.test.js b/test/app/extends/helper.test.js index 0bc60b7..a3e051b 100644 --- a/test/app/extends/helper.test.js +++ b/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', () => { diff --git a/test/app/extends/sjs.test.js b/test/app/extends/sjs.test.js index 07ae4e7..27ef6dc 100644 --- a/test/app/extends/sjs.test.js +++ b/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', () => { diff --git a/test/app/extends/sjson.test.js b/test/app/extends/sjson.test.js index be620a5..31e380d 100644 --- a/test/app/extends/sjson.test.js +++ b/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', () => { diff --git a/test/app/extends/spath.test.js b/test/app/extends/spath.test.js index daadd84..82e7c3f 100644 --- a/test/app/extends/spath.test.js +++ b/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', () => { diff --git a/test/dta.test.js b/test/dta.test.js index 09618a3..41f832d 100644 --- a/test/dta.test.js +++ b/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'); }); }); diff --git a/test/fixtures/apps/helper-escapeShellArg-app/app/router.js b/test/fixtures/apps/helper-escapeShellArg-app/app/router.js index 1ec4f11..2322713 100644 --- a/test/fixtures/apps/helper-escapeShellArg-app/app/router.js +++ b/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() { diff --git a/test/utils.test.js b/test/utils.test.js index 50466fd..fa5db98 100644 --- a/test/utils.test.js +++ b/test/utils.test.js @@ -12,6 +12,8 @@ describe('test/utils.test.js', () => { }); return app.ready(); }); + after(() => app.close()); + const domainWhiteList = [ '.domain.com', '*.alibaba.com', 'http://www.baidu.com', '192.*.0.*', 'foo.bar' ]; it('should return false when domains are not safe', async () => { const res = await app.httpRequest() @@ -69,132 +71,137 @@ describe('test/utils.test.js', () => { }); describe('utils.checkIfIgnore', () => { - let app, - app2, - app3, - app4, - app5, - app6; - before(async () => { - app = mm.app({ - baseDir: 'apps/utils-check-if-pass', + async function createApp(baseDir) { + const app = mm.app({ + baseDir, plugin: 'security', }); await app.ready(); - - app2 = mm.app({ - baseDir: 'apps/utils-check-if-pass2', - plugin: 'security', - }); - await app2.ready(); - - app3 = mm.app({ - baseDir: 'apps/utils-check-if-pass3', - plugin: 'security', - }); - await app3.ready(); - - app4 = mm.app({ - baseDir: 'apps/utils-check-if-pass4', - plugin: 'security', - }); - await app4.ready(); - - app5 = mm.app({ - baseDir: 'apps/utils-check-if-pass5', - plugin: 'security', - }); - await app5.ready(); - - app6 = mm.app({ - baseDir: 'apps/utils-check-if-pass6', - plugin: 'security', - }); - await app6.ready(); - }); + return app; + } it('should use match', async () => { - const res = await app.httpRequest() - .get('/match') - .expect(200); - assert(res.headers['x-csp-nonce'].length === 16); + const app = await createApp('apps/utils-check-if-pass'); + try { + const res = await app.httpRequest() + .get('/match') + .expect(200); + assert(res.headers['x-csp-nonce'].length === 16); + } finally { + await app.close(); + } }); it('global match should not work', async () => { - const res = await app.httpRequest() - .get('/luckydrq') - .expect(200); - assert(res.headers['x-csp-nonce'].length === 16); + const app = await createApp('apps/utils-check-if-pass'); + try { + const res = await app.httpRequest() + .get('/luckydrq') + .expect(200); + assert(res.headers['x-csp-nonce'].length === 16); + } finally { + await app.close(); + } }); it('own match should replace global match', async () => { - let res = await app2.httpRequest() - .get('/mymatch') - .expect(200); - assert(res.headers['x-csp-nonce'].length === 16); - res = await app2.httpRequest() - .get('/match') - .expect(200); - assert(!res.headers['x-csp-nonce']); + const app = await createApp('apps/utils-check-if-pass2'); + try { + let res = await app.httpRequest() + .get('/mymatch') + .expect(200); + assert(res.headers['x-csp-nonce'].length === 16); + res = await app.httpRequest() + .get('/match') + .expect(200); + assert(!res.headers['x-csp-nonce']); + } finally { + await app.close(); + } }); it('own match has priority over own ignore', async () => { - const res = await app2.httpRequest() - .get('/mytrueignore') - .expect(200); - assert(!res.headers['x-csp-nonce']); + const app = await createApp('apps/utils-check-if-pass2'); + try { + const res = await app.httpRequest() + .get('/mytrueignore') + .expect(200); + assert(!res.headers['x-csp-nonce']); + } finally { + await app.close(); + } }); it('should not use global ignore', async () => { - const res = await app3.httpRequest() - .get('/ignore') - .expect(200); - assert(res.headers['x-csp-nonce'].length === 16); + const app = await createApp('apps/utils-check-if-pass3'); + try { + const res = await app.httpRequest() + .get('/ignore') + .expect(200); + assert(res.headers['x-csp-nonce'].length === 16); + } finally { + await app.close(); + } }); it('own ignore should replace global ignore', async () => { - let res = await app4.httpRequest() - .get('/ignore') - .expect(200); - assert(res.headers['x-csp-nonce'].length === 16); - res = await app4.httpRequest() - .get('/myignore') - .expect(200); - assert(!res.headers['x-csp-nonce']); + const app = await createApp('apps/utils-check-if-pass4'); + try { + let res = await app.httpRequest() + .get('/ignore') + .expect(200); + assert(res.headers['x-csp-nonce'].length === 16); + res = await app.httpRequest() + .get('/myignore') + .expect(200); + assert(!res.headers['x-csp-nonce']); + } finally { + await app.close(); + } }); it('should ignore array work', async () => { - let res = await app5.httpRequest() - .get('/ignore1') - .expect(200); - assert(!res.headers['x-frame-options']); - - res = await app5.httpRequest() - .get('/ignore2') - .expect(200); - assert(!res.headers['x-frame-options']); - - res = await app5.httpRequest() - .get('/') - .expect(200); - assert(res.header['x-frame-options']); + const app = await createApp('apps/utils-check-if-pass5'); + try { + let res = await app.httpRequest() + .get('/ignore1') + .expect(200); + assert(!res.headers['x-frame-options']); + + res = await app.httpRequest() + .get('/ignore2') + .expect(200); + assert(!res.headers['x-frame-options']); + + res = await app.httpRequest() + .get('/') + .expect(200); + assert(res.header['x-frame-options']); + } finally { + await app.close(); + } }); it('should match array work', async () => { - let res = await app6.httpRequest() - .get('/match1') - .expect(200); - assert(res.headers['x-frame-options']); - - res = await app6.httpRequest() - .get('/match2') - .expect(200); - assert(res.headers['x-frame-options']); - - res = await app6.httpRequest() - .get('/') - .expect(200); - assert(!res.headers['x-frame-options']); + const app = await createApp('apps/utils-check-if-pass6'); + try { + let res = await app.httpRequest() + .get('/match1') + .expect(200); + assert(res.headers['x-frame-options']); + + res = await app.httpRequest() + .get('/match2') + .expect(200); + assert(res.headers['x-frame-options']); + + res = await app.httpRequest() + .get('/') + .expect(200); + assert(!res.headers['x-frame-options']); + } finally { + await app.close(); + } }); }); });