From 314db1e30a1788534323e56847321876ec938b7d Mon Sep 17 00:00:00 2001 From: "zhubin.gzb" Date: Wed, 15 Jul 2026 20:46:44 +0800 Subject: [PATCH 1/5] fix: escape shell arg single quotes --- README.md | 4 +++- README.zh-CN.md | 4 +++- lib/helper/escapeShellArg.js | 2 +- test/app/extends/escapeShellArg.test.js | 16 ++++++++++++++++ .../apps/helper-escapeShellArg-app/app/router.js | 2 +- 5 files changed, 24 insertions(+), 4 deletions(-) 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/escapeShellArg.test.js b/test/app/extends/escapeShellArg.test.js index 4b2e3a4..5e7f3aa 100644 --- a/test/app/extends/escapeShellArg.test.js +++ b/test/app/extends/escapeShellArg.test.js @@ -1,5 +1,14 @@ +const assert = require('node:assert/strict'); +const { exec: childProcessExec } = require('node:child_process'); +const { promisify } = require('node: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(() => { @@ -33,5 +42,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.equal(stdout, 'ARG:\'; echo EGG_SECURITY_INJECTED; #\n'); + }); }); }); 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() { From 34eb29a854b262a08fe613ef8dc7d774a4bc26a0 Mon Sep 17 00:00:00 2001 From: "zhubin.gzb" Date: Wed, 15 Jul 2026 21:10:08 +0800 Subject: [PATCH 2/5] test: keep escape shell arg test node 14 compatible --- test/app/extends/escapeShellArg.test.js | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/test/app/extends/escapeShellArg.test.js b/test/app/extends/escapeShellArg.test.js index 5e7f3aa..77d1d26 100644 --- a/test/app/extends/escapeShellArg.test.js +++ b/test/app/extends/escapeShellArg.test.js @@ -1,6 +1,9 @@ -const assert = require('node:assert/strict'); -const { exec: childProcessExec } = require('node:child_process'); -const { promisify } = require('node:util'); +// 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'); @@ -47,7 +50,7 @@ describe('test/app/extends/escapeShellArg.test.js', () => { const payload = '\'; echo EGG_SECURITY_INJECTED; #'; const { stdout } = await exec(`printf 'ARG:%s\\n' ${escapeShellArg(payload)}`); - assert.equal(stdout, 'ARG:\'; echo EGG_SECURITY_INJECTED; #\n'); + assert.strictEqual(stdout, 'ARG:\'; echo EGG_SECURITY_INJECTED; #\n'); }); }); }); From 521bef4d603b2e5061772fae093ce4c418e5b662 Mon Sep 17 00:00:00 2001 From: "zhubin.gzb" Date: Wed, 15 Jul 2026 21:33:46 +0800 Subject: [PATCH 3/5] test: close utils fixture apps --- test/utils.test.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/test/utils.test.js b/test/utils.test.js index 50466fd..eddd827 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() @@ -112,6 +114,9 @@ describe('test/utils.test.js', () => { }); await app6.ready(); }); + after(async () => { + await Promise.all([ app, app2, app3, app4, app5, app6 ].filter(Boolean).map(app => app.close())); + }); it('should use match', async () => { const res = await app.httpRequest() From 5281b14dd91872c4cf3dbe8fdd7e716524ccdb34 Mon Sep 17 00:00:00 2001 From: "zhubin.gzb" Date: Wed, 15 Jul 2026 21:39:29 +0800 Subject: [PATCH 4/5] test: disable watcher in mock apps --- package.json | 4 ++-- test/setup.js | 23 +++++++++++++++++++++++ 2 files changed, 25 insertions(+), 2 deletions(-) create mode 100644 test/setup.js diff --git a/package.json b/package.json index bd42554..db440ed 100644 --- a/package.json +++ b/package.json @@ -49,8 +49,8 @@ "scripts": { "lint": "eslint .", "test": "npm run lint -- --fix && npm run test-local", - "test-local": "egg-bin test", - "cov": "egg-bin cov", + "test-local": "egg-bin test --require ./test/setup.js", + "cov": "egg-bin cov --require ./test/setup.js", "ci": "npm run lint && npm run cov" }, "repository": { diff --git a/test/setup.js b/test/setup.js new file mode 100644 index 0000000..f006f3b --- /dev/null +++ b/test/setup.js @@ -0,0 +1,23 @@ +'use strict'; + +const mm = require('egg-mock'); + +const app = mm.app; +const cluster = mm.cluster; + +function disableWatcher(options) { + options = options || {}; + return { + ...options, + plugins: { + ...options.plugins, + watcher: { + ...options.plugins?.watcher, + enable: false, + }, + }, + }; +} + +mm.app = options => app(disableWatcher(options)); +mm.cluster = options => cluster(disableWatcher(options)); From 73afeccc503e09f0431a3487a577289475c2f456 Mon Sep 17 00:00:00 2001 From: "zhubin.gzb" Date: Wed, 15 Jul 2026 21:52:07 +0800 Subject: [PATCH 5/5] test: stabilize 3.x fixture cleanup --- package.json | 4 +- test/app/extends/cliFilter.test.js | 5 +- test/app/extends/escapeShellArg.test.js | 5 +- test/app/extends/escapeShellCmd.test.js | 5 +- test/app/extends/helper.test.js | 5 +- test/app/extends/sjs.test.js | 5 +- test/app/extends/sjson.test.js | 5 +- test/app/extends/spath.test.js | 5 +- test/dta.test.js | 17 +- test/setup.js | 23 --- test/utils.test.js | 208 ++++++++++++------------ 11 files changed, 150 insertions(+), 137 deletions(-) delete mode 100644 test/setup.js diff --git a/package.json b/package.json index db440ed..bd42554 100644 --- a/package.json +++ b/package.json @@ -49,8 +49,8 @@ "scripts": { "lint": "eslint .", "test": "npm run lint -- --fix && npm run test-local", - "test-local": "egg-bin test --require ./test/setup.js", - "cov": "egg-bin cov --require ./test/setup.js", + "test-local": "egg-bin test", + "cov": "egg-bin cov", "ci": "npm run lint && npm run cov" }, "repository": { 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 77d1d26..f54515a 100644 --- a/test/app/extends/escapeShellArg.test.js +++ b/test/app/extends/escapeShellArg.test.js @@ -22,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', () => { 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/setup.js b/test/setup.js deleted file mode 100644 index f006f3b..0000000 --- a/test/setup.js +++ /dev/null @@ -1,23 +0,0 @@ -'use strict'; - -const mm = require('egg-mock'); - -const app = mm.app; -const cluster = mm.cluster; - -function disableWatcher(options) { - options = options || {}; - return { - ...options, - plugins: { - ...options.plugins, - watcher: { - ...options.plugins?.watcher, - enable: false, - }, - }, - }; -} - -mm.app = options => app(disableWatcher(options)); -mm.cluster = options => cluster(disableWatcher(options)); diff --git a/test/utils.test.js b/test/utils.test.js index eddd827..fa5db98 100644 --- a/test/utils.test.js +++ b/test/utils.test.js @@ -71,135 +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(); - }); - after(async () => { - await Promise.all([ app, app2, app3, app4, app5, app6 ].filter(Boolean).map(app => app.close())); - }); + 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(); + } }); }); });